Re: [PATCH] ppp_generic, kernel 2.4.

Re: [PATCH] ppp_generic, kernel 2.4.

[Dieter Nützel]

Hello Tim,

it seems to me to that there is one little commentary close (*/) to much.


+   * A ConfReq indicates what the sender would like to receive */
+   */

should be


+   * A ConfReq indicates what the sender would like to receive
+   */

Have a nice weekend.

-Dieter
-- 
Dieter Nützel
Graduate Student, Computer Science

University of Hamburg
Department of Computer Science
Cognitive Systems Group
Vogt-Kölln-Straße 30
D-22527 Hamburg, Germany

email: nuetzel@kogs.informatik.uni-hamburg.de
@home: Dieter.Nuetzel@hamburg.de

[PATCH] ppp_generic, kernel 2.4.

[Tim Wilson]

[-- Attachment #1: Type: text/plain, Size: 2081 bytes --]

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.

Please forgive if I make a procedural error in submitting this patch;
I'm trying to follow the instructions in the FAQ but this is my first
time. The FAQ said to cc Linus and/or Alan Cox for security issues, so I
am doing that..

I am not currently subscribed to the list so please respond directly.

The patch is attached and also shown below.



--- drivers/net/ppp_generic.c.orig Sat Apr 21 13:33:00 2001
+++ drivers/net/ppp_generic.c Sat Apr 21 13:44:38 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 to 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:




[-- Attachment #2: ccp_negotiate_fix.patch --]
[-- Type: text/plain, Size: 975 bytes --]

--- drivers/net/ppp_generic.c.orig	Sat Apr 21 13:33:00 2001
+++ drivers/net/ppp_generic.c	Sat Apr 21 13:44:38 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 to 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: