An astute observer pointed out that my patch had an extra closing */ in
it. I won't even attempt to explain how that happened, but believe me, I
am suitably mortified. Can I get a mulligan?
Here is the correct patch, along with the original text.
This patch corrects a bug in CCP establishment which can result in a
major security hole.
The bug can cause PPP to NOT install and use a compressor module for
sending, even though the compressor is sucessfully negotiated by CCP.
Since encryption is sometimes implemented as a compressor module (e.g.
MPPE), this bug can cause PPP to send cleartext even though encryption
appears to be sucessfully negotiated.
The bug does not always show up--it depends on the order of CCP messages
exchanged during establishment, and therefore is not deterministic.
The specific problem is handling a sent or received CCP ConfReq. A sent
ConfReq should reset my decompressor; a received ConfReq should reset my
compressor. The original code had this logic exactly reversed.
I am not currently subscribed to the list so please respond directly.
--- drivers/net/ppp_generic.c.orig Sat Apr 21 13:33:00 2001
+++ drivers/net/ppp_generic.c Sat Apr 21 19:41:59 2001
@@ -1967,15 +1967,30 @@
switch (CCP_CODE(dp)) {
case CCP_CONFREQ:
+
+ /* A ConfReq starts negotiation of compression
+ * in one direction of transmission,
+ * and hence brings it down...but which way?
+ *
+ * Remember:
+ * A ConfReq indicates what the sender would like to receive
+ */
+ if(inbound)
+ /* He is proposing what I should send */
+ ppp->xstate &= ~SC_COMP_RUN;
+ else
+ /* I am proposing what he should send */
+ ppp->rstate &= ~SC_DECOMP_RUN;
+
+ break;
+
case CCP_TERMREQ:
case CCP_TERMACK:
/*
- * CCP is going down - disable compression.
+ * CCP is going down, both directions of transmission
*/
- if (inbound)
- ppp->rstate &= ~SC_DECOMP_RUN;
- else
- ppp->xstate &= ~SC_COMP_RUN;
+ ppp->rstate &= ~SC_DECOMP_RUN;
+ ppp->xstate &= ~SC_COMP_RUN;
break;
case CCP_CONFACK:
Tim Wilson writes:
> The bug can cause PPP to NOT install and use a compressor module for
> sending, even though the compressor is sucessfully negotiated by CCP.
> Since encryption is sometimes implemented as a compressor module (e.g.
> MPPE), this bug can cause PPP to send cleartext even though encryption
> appears to be sucessfully negotiated.
Hmmm... using CCP to negotiate encryption is a Bad Idea IMHO. I know
Microsoft does, but that isn't really a recommendation. :) Certainly
pppd and the Linux PPP kernel driver don't include support for some of
the things that the MPPE spec says you have to do, like taking down
the link if MPPE doesn't get negotiated successfully, and not sending
or receiving any data before MPPE is up.
All of which goes to say that MPPE support is not a "feature point" of
the Linux PPP implementation. So a potential insecurity in an MPPE
implementation is hardly a bug.
Anyway...
> The bug does not always show up--it depends on the order of CCP messages
> exchanged during establishment, and therefore is not deterministic.
The bug is only going to show up if CCP gets re-negotiated, i.e. if
CCP get negotiated and comes up and then one side starts sending
Configure-requests to renegotiate the compression method and
parameters.
> The specific problem is handling a sent or received CCP ConfReq. A sent
> ConfReq should reset my decompressor; a received ConfReq should reset my
> compressor. The original code had this logic exactly reversed.
Actually, after having another look at RFC 1962 I think that either
sending or receiving a ConfReq takes CCP out of Opened state and
should stop compression in *both* directions. So on balance I would
change it like you have for TermReq and TermAck, but make the same
change for ConfReq as well.
BTW, the spacing/indentation in the patch you sent was broken; the
patch seemed to have 1-space indentation whereas it should be 1-tab
indentation. Does your mailer convert tabs to single spaces maybe?
Paul.
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).
> Hmmm... using CCP to negotiate encryption is a Bad Idea IMHO. I know
> Microsoft does, but that isn't really a recommendation. :) Certainly
> pppd and the Linux PPP kernel driver don't include support for some of
> the things that the MPPE spec says you have to do, like taking down
> the link if MPPE doesn't get negotiated successfully, and not sending
> or receiving any data before MPPE is up.
>
> All of which goes to say that MPPE support is not a "feature point" of
> the Linux PPP implementation. So a potential insecurity in an MPPE
> implementation is hardly a bug.
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. As my patch shows, the fix
is quite easy, so reqardless of what we call it, might as well fix it.
> The bug is only going to show up if CCP gets re-negotiated, i.e. if
> CCP get negotiated and comes up and then one side starts sending
> Configure-requests to renegotiate the compression method and
> parameters.
Sorry, that's not the case. Let me see if I can explain using a little ASCII
art. Here's a legal exchange for initially opening CCP:
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.
> Actually, after having another look at RFC 1962 I think that either
> sending or receiving a ConfReq takes CCP out of Opened state and
> should stop compression in *both* directions. So on balance I would
> change it like you have for TermReq and TermAck, but make the same
> change for ConfReq as well.
I'll take a look at the RFCs myself, but that sounds right. However, you
can't just handle the ConfReq like I was handling the TermReq/TermAck--take
another look at the scenario above. The compressor would get reset at (3),
we'd end up with the same problem. The ConfReq should make CCP go down only
if it's already up, something like this:
...
case CCP_CONFREQ:
case CCP_TERMREQ:
case CCP_TERMACK:
if( ppp->flags & SC_CCP_UP) {
ppp->rstate &= ~SC_DECOMP_RUN;
ppp->xstate &= ~SC_COMP_RUN;
ppp->flags &= ~SC_CCP_UP;
}
break;
...
(This assumes that SC_CCP_UP is the right thing to check--I'm not sure about
the distinction between SC_CCP_UP and SC_CCP_OPEN--haven't studied the code
enough. But you probably know that stuff already).
Incidentally, I looked at the 2.2 code and that's what it does.
> BTW, the spacing/indentation in the patch you sent was broken; the
> patch seemed to have 1-space indentation whereas it should be 1-tab
> indentation. Does your mailer convert tabs to single spaces maybe?
Sorry. I'll try to fix that for next time. If we can agree on a fix for this
problem, I'll be happy to let you submit the patch. It's your code.
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;