RE: [PATCH] ppp_generic, kernel 2.4.3

[Paul Mackerras <paulus@samba.org>]

Tim Wilson writes:

> Thanks for your reply. It seems I am finally talking to the right person (I
> had previously tried posting this on the pptp-server mailing list, and I
> also tried sending it to you directly, but no luck).

Sorry, life has been a little turbulent for me over the last couple of
months.

> Well, I do know that people set up Linux gateways as PPTP servers, and that
> they use MPPE to allow win98 clients to connect to those servers. That's
> what I was trying to do anyway. After the connect, the gateway log says that
> MPPE is negotiated, and the win98 client claims MPPE is being used, so all
> looks OK, but the gateway sends PPP frames in cleartext. If that's not a
> security hole, it is certainly not a Good Thing.

Well, it's a consequence of using a knife to drive in a nail. :)

Neither CCP nor the Linux CCP implementation are really designed to
support encryption.  There is a fairly strong assumption that if
things go pear-shaped you can always take CCP down and send stuff
uncompressed - it will be slower but it will still work.

> As my patch shows, the fix
> is quite easy, so reqardless of what we call it, might as well fix it.

Sure, we can fix the problem you've pointed out, but that won't make
for a secure MPPE implementation.  (Is that an oxymoron, actually?)
What I am saying is that even with your fix there is still a lot more
work to do if you want to make sure that you never send or accept
unencypted PPP frames.

>      Server			Client
> 1)   <----------------ConfReq
> 2)   ConfAck-------------->
> 3)   ConfReq-------------->
> 4)   <----------------ConfAck
> 
> 
> The existing code (correctly) enables the compressor when it sends the
> ConfAck (2). Then, it (incorrectly) disables the compressor when sending the
> ConfReq in (3). With my fix, that doesn't happen; the compressor is disabled
> at by reception of the ConfReq at(1), but it's not enabled yet anyway, so no
> harm done.

Good point.

> 	if( ppp->flags & SC_CCP_UP) {
> 		ppp->rstate &= ~SC_DECOMP_RUN;
> 		ppp->xstate &= ~SC_COMP_RUN;
> 		ppp->flags &= ~SC_CCP_UP;
> 	}

Yep, with the exception that I wouldn't clear SC_CCP_UP, since that is
set and cleared by pppd.

Here is an updated patch.

Paul.

diff -urN linux/drivers/net/ppp_generic.c pmac/drivers/net/ppp_generic.c
--- linux/drivers/net/ppp_generic.c	Sun Apr 22 17:07:28 2001
+++ pmac/drivers/net/ppp_generic.c	Mon Apr 23 10:12:27 2001
@@ -1993,10 +1993,10 @@
 		/*
 		 * CCP is going down - disable compression.
 		 */
-		if (inbound)
+		if (ppp->flags & SC_CCP_UP) {
 			ppp->rstate &= ~SC_DECOMP_RUN;
-		else
 			ppp->xstate &= ~SC_COMP_RUN;
+		}
 		break;
 
 	case CCP_CONFACK:
@@ -2054,7 +2054,7 @@
 		ppp->xc_state = 0;
 	}
 
-	ppp->xstate &= ~SC_DECOMP_RUN;
+	ppp->rstate &= ~SC_DECOMP_RUN;
 	if (ppp->rc_state) {
 		ppp->rcomp->decomp_free(ppp->rc_state);
 		ppp->rc_state = 0;