[PATCH 0/5] ppp: error path fixes

[PATCH 0/5] ppp: error path fixes

[Kristen Carlson Accardi]

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

Here are some patches which fix some issues with the error paths in
the PPP control protocol code.

Kristen Carlson Accardi (5):
  ppp: fix segfault in pppcp_send_code_reject()
  ppp: comment fix
  ppp: send Protocol-Reject
  ppp: use common code to get options from pppcp packet data
  ppp: handle Config-Reject

 gatchat/ppp.c     |   29 ++++++---
 gatchat/ppp.h     |    1 +
 gatchat/ppp_cp.c  |  179 +++++++++++++++++++++++++++++++++++++++-------------
 gatchat/ppp_cp.h  |    2 +
 gatchat/ppp_lcp.c |    5 ++
 5 files changed, 161 insertions(+), 55 deletions(-)

[PATCH 1/5] ppp: fix segfault in pppcp_send_code_reject()

[Kristen Carlson Accardi]

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

fix memory corruption caused by misplaced paren when memcpying
rejected packet data into Code-Reject packet.
---
 gatchat/ppp_cp.c |    9 ++++++---
 1 files changed, 6 insertions(+), 3 deletions(-)

diff --git a/gatchat/ppp_cp.c b/gatchat/ppp_cp.c
index 137f6b9..39e872b 100644
--- a/gatchat/ppp_cp.c
+++ b/gatchat/ppp_cp.c
@@ -454,9 +454,12 @@ static void pppcp_send_code_reject(struct pppcp_data *data,
 					guint8 *rejected_packet)
 {
 	struct pppcp_packet *packet;
+	struct pppcp_packet *old_packet =
+				(struct pppcp_packet *) rejected_packet;
 
-	packet = pppcp_packet_new(data, CODE_REJECT,
-			ntohs(((struct pppcp_packet *) rejected_packet)->length));
+	pppcp_trace(data);
+
+	packet = pppcp_packet_new(data, CODE_REJECT, ntohs(old_packet->length));
 
 	/*
 	 * Identifier must be changed for each Code-Reject sent
@@ -468,7 +471,7 @@ static void pppcp_send_code_reject(struct pppcp_data *data,
 	 * truncated if it needs to be to comply with mtu requirement
 	 */
 	memcpy(packet->data, rejected_packet,
-			ntohs(packet->length - CP_HEADER_SZ));
+			ntohs(packet->length) - CP_HEADER_SZ);
 
 	ppp_transmit(data->ppp, pppcp_to_ppp_packet(packet),
 			ntohs(packet->length));
-- 
1.6.6.1

[PATCH 2/5] ppp: comment fix

[Kristen Carlson Accardi]

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

Put some additional clarification in comment for receiving Code-Reject
and Protocol-Reject packets.
---
 gatchat/ppp_cp.c |    8 ++++++++
 1 files changed, 8 insertions(+), 0 deletions(-)

diff --git a/gatchat/ppp_cp.c b/gatchat/ppp_cp.c
index 39e872b..f83cfba 100644
--- a/gatchat/ppp_cp.c
+++ b/gatchat/ppp_cp.c
@@ -1401,6 +1401,10 @@ static guint8 pppcp_process_code_reject(struct pppcp_data *data,
 	 * determine if the code reject is catastrophic or not.
 	 * return RXJ_PLUS if this reject is acceptable, RXJ_MINUS if
 	 * it is catastrophic.
+	 *
+	 * for now we always return RXJ_MINUS.  Any code
+	 * reject will be catastrophic, since we only support the
+	 * bare minimum number of codes necessary to function.
 	 */
 	return RXJ_MINUS;
 }
@@ -1412,6 +1416,10 @@ static guint8 pppcp_process_protocol_reject(struct pppcp_data *data,
 	 * determine if the protocol reject is catastrophic or not.
 	 * return RXJ_PLUS if this reject is acceptable, RXJ_MINUS if
 	 * it is catastrophic.
+	 *
+	 * for now we always return RXJ_MINUS.  Any protocol
+	 * reject will be catastrophic, since we only support the
+	 * bare minimum number of protocols necessary to function.
 	 */
 	return RXJ_MINUS;
 }
-- 
1.6.6.1

[PATCH 3/5] ppp: send Protocol-Reject

[Kristen Carlson Accardi]

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

change ppp_decode to store the length of the decoded frame, so that
if we have a packet with a protocol we don't understand, we can send
Protocol-Reject packets.  Modify ppp_cp code to add support for sending
Protocol-Reject packet.
---
 gatchat/ppp.c     |   29 +++++++++++++++++++----------
 gatchat/ppp.h     |    1 +
 gatchat/ppp_cp.c  |   42 ++++++++++++++++++++++++++++++++++++++++++
 gatchat/ppp_cp.h  |    2 ++
 gatchat/ppp_lcp.c |    5 +++++
 5 files changed, 69 insertions(+), 10 deletions(-)

diff --git a/gatchat/ppp.c b/gatchat/ppp.c
index d771c3f..f93b52d 100644
--- a/gatchat/ppp.c
+++ b/gatchat/ppp.c
@@ -39,6 +39,11 @@
 #define PPPINITFCS16    0xffff  /* Initial FCS value */
 #define PPPGOODFCS16    0xf0b8  /* Good final FCS value */
 
+struct frame_buffer {
+	gsize len;
+	guint8 bytes[0];
+};
+
 static GList *packet_handlers = NULL;
 
 void ppp_register_packet_handler(struct ppp_packet_handler *handler)
@@ -185,12 +190,12 @@ static gint is_proto_handler(gconstpointer a, gconstpointer b)
 static void ppp_recv(GAtPPP *ppp)
 {
 	GList *list;
-	guint8 *frame;
+	struct frame_buffer *frame;
 
 	/* pop frames off of receive queue */
 	while ((frame = g_queue_pop_head(ppp->recv_queue))) {
-		guint protocol = ppp_proto(frame);
-		guint8 *packet = ppp_info(frame);
+		guint protocol = ppp_proto(frame->bytes);
+		guint8 *packet = ppp_info(frame->bytes);
 		struct ppp_packet_handler *h;
 
 		/*
@@ -203,23 +208,26 @@ static void ppp_recv(GAtPPP *ppp)
 		if (list) {
 			h = list->data;
 			h->handler(h->priv, packet);
-		}
+		} else
+			lcp_protocol_reject(ppp->lcp, frame->bytes, frame->len);
 		g_free(frame);
 	}
 }
 
 /* XXX - Implement PFC and ACFC */
-static guint8 *ppp_decode(GAtPPP *ppp, guint8 *frame)
+static struct frame_buffer *ppp_decode(GAtPPP *ppp, guint8 *frame)
 {
 	guint8 *data;
 	guint pos = 0;
 	int i = 0;
 	int len;
 	guint16 fcs;
+	struct frame_buffer *fb;
 
-	data = g_try_malloc0(ppp->mru + 10);
-	if (!data)
+	fb = g_try_malloc0(sizeof(struct frame_buffer) + ppp->mru + 10);
+	if (!fb)
 		return NULL;
+	data = fb->bytes;
 
 	/* skip the first flag char */
 	pos++;
@@ -237,6 +245,7 @@ static guint8 *ppp_decode(GAtPPP *ppp, guint8 *frame)
 	}
 
 	len = i;
+	fb->len = len;
 
 	/* see if we have a good FCS */
 	fcs = PPPINITFCS16;
@@ -244,16 +253,16 @@ static guint8 *ppp_decode(GAtPPP *ppp, guint8 *frame)
 		fcs = ppp_fcs(fcs, data[i]);
 
 	if (fcs != PPPGOODFCS16) {
-		g_free(data);
+		g_free(fb);
 		return NULL;
 	}
-	return data;
+	return fb;
 }
 
 static void ppp_feed(GAtPPP *ppp, guint8 *data, gsize len)
 {
 	guint pos = 0;
-	guint8 *frame;
+	struct frame_buffer *frame;
 
 	/* collect bytes until we detect we have received a complete frame */
 	/* examine the data.  If we are at the beginning of a new frame,
diff --git a/gatchat/ppp.h b/gatchat/ppp.h
index 05cd50f..6797603 100644
--- a/gatchat/ppp.h
+++ b/gatchat/ppp.h
@@ -156,6 +156,7 @@ void lcp_open(struct pppcp_data *data);
 void lcp_close(struct pppcp_data *data);
 void lcp_establish(struct pppcp_data *data);
 void lcp_terminate(struct pppcp_data *data);
+void lcp_protocol_reject(struct pppcp_data *lcp, guint8 *packet, gsize len);
 void auth_set_credentials(struct auth_data *data, const char *username,
 				const char *passwd);
 void auth_set_proto(struct auth_data *data, guint16 proto, guint8 method);
diff --git a/gatchat/ppp_cp.c b/gatchat/ppp_cp.c
index f83cfba..263fa8b 100644
--- a/gatchat/ppp_cp.c
+++ b/gatchat/ppp_cp.c
@@ -1442,6 +1442,48 @@ static guint8 pppcp_process_discard_request(struct pppcp_data *data,
 	return 0;
 }
 
+void pppcp_send_protocol_reject(struct pppcp_data *data,
+				guint8 *rejected_packet, gsize len)
+{
+	struct pppcp_packet *packet;
+	struct ppp_header *ppp_packet = (struct ppp_header *) rejected_packet;
+
+	pppcp_trace(data);
+
+	/*
+	 * Protocol-Reject can only be sent when we are in
+	 * the OPENED state.  If in any other state, silently discard.
+	 */
+	if (data->state != OPENED) {
+		g_free(ppp_packet);
+		return;
+	}
+
+	/*
+	 * info should contain the old packet info, plus the 16bit
+	 * protocol number we are rejecting.
+	 */
+	packet = pppcp_packet_new(data, PROTOCOL_REJECT, len);
+
+	/*
+	 * Identifier must be changed for each Protocol-Reject sent
+	 */
+	packet->identifier = new_identity(data, data->reject_identifier);
+
+	/*
+	 * rejected packet should be copied in, but it should be
+	 * truncated if it needs to be to comply with mtu requirement
+	 */
+	memcpy(packet->data, rejected_packet,
+			(ntohs(packet->length) - CP_HEADER_SZ));
+
+	ppp_transmit(data->ppp, pppcp_to_ppp_packet(packet),
+			ntohs(packet->length));
+
+	pppcp_packet_free(packet);
+
+}
+
 /*
  * parse the packet and determine which event this packet caused
  */
diff --git a/gatchat/ppp_cp.h b/gatchat/ppp_cp.h
index 69676cd..e326b5e 100644
--- a/gatchat/ppp_cp.h
+++ b/gatchat/ppp_cp.h
@@ -150,3 +150,5 @@ void pppcp_generate_event(struct pppcp_data *data,
 				enum pppcp_event_type event_type,
 				gpointer event_data, guint data_len);
 void pppcp_process_packet(gpointer priv, guint8 *new_packet);
+void pppcp_send_protocol_reject(struct pppcp_data *data,
+				guint8 *rejected_packet, gsize len);
diff --git a/gatchat/ppp_lcp.c b/gatchat/ppp_lcp.c
index 7206b4b..1ccc3cc 100644
--- a/gatchat/ppp_lcp.c
+++ b/gatchat/ppp_lcp.c
@@ -220,6 +220,11 @@ void lcp_free(struct pppcp_data *lcp)
 	pppcp_free(lcp);
 }
 
+void lcp_protocol_reject(struct pppcp_data *lcp, guint8 *packet, gsize len)
+{
+	pppcp_send_protocol_reject(lcp, packet, len);
+}
+
 struct pppcp_data *lcp_new(GAtPPP *ppp)
 {
 	struct pppcp_data *pppcp;
-- 
1.6.6.1

[PATCH 4/5] ppp: use common code to get options from pppcp packet data

[Kristen Carlson Accardi]

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

---
 gatchat/ppp_cp.c |   71 ++++++++++++++++++++++++++++++-----------------------
 1 files changed, 40 insertions(+), 31 deletions(-)

diff --git a/gatchat/ppp_cp.c b/gatchat/ppp_cp.c
index 263fa8b..d7d70b7 100644
--- a/gatchat/ppp_cp.c
+++ b/gatchat/ppp_cp.c
@@ -1140,6 +1140,22 @@ static void remove_config_option(gpointer elem, gpointer user_data)
 	data->config_options = g_list_delete_link(data->config_options, list);
 }
 
+static struct ppp_option *extract_ppp_option(guint8 *packet_data)
+{
+	struct ppp_option *option;
+	guint8 otype = packet_data[0];
+	guint8 olen = packet_data[1];
+
+	option = g_try_malloc0(olen);
+	if (option == NULL)
+		return NULL;
+
+	option->type = otype;
+	option->length = olen;
+	memcpy(option->data, &packet_data[2], olen-2);
+	return option;
+}
+
 static guint8 pppcp_process_configure_request(struct pppcp_data *data,
 					struct pppcp_packet *packet)
 {
@@ -1157,14 +1173,13 @@ static guint8 pppcp_process_configure_request(struct pppcp_data *data,
 	 * check the options.
 	 */
 	while (i < len) {
-		guint8 otype = packet->data[i];
-		guint8 olen = packet->data[i+1];
-		option = g_try_malloc0(olen);
+		option = extract_ppp_option(&packet->data[i]);
 		if (option == NULL)
 			break;
-		option->type = otype;
-		option->length = olen;
-		memcpy(option->data, &packet->data[i+2], olen-2);
+
+		/* skip ahead to the next option */
+		i += option->length;
+
 		if (action->option_scan)
 			rval = action->option_scan(option, data);
 		switch (rval) {
@@ -1182,10 +1197,9 @@ static guint8 pppcp_process_configure_request(struct pppcp_data *data,
 						option);
 			break;
 		case OPTION_ERR:
-			g_printerr("unhandled option type %d\n", otype);
+			g_printerr("unhandled option type %d\n", option->type);
+			g_free(option);
 		}
-		/* skip ahead to the next option */
-		i += olen;
 	}
 
 	/* make sure all required config options were included */
@@ -1242,16 +1256,12 @@ static guint8 pppcp_process_configure_ack(struct pppcp_data *data,
 	 * and apply them.
 	 */
 	while (i < len) {
-		guint8 otype = packet->data[i];
-		guint8 olen = packet->data[i + 1];
-		acked_option = g_try_malloc0(olen);
+		acked_option = extract_ppp_option(&packet->data[i]);
 		if (acked_option == NULL)
 			break;
-		acked_option->type = otype;
-		acked_option->length = olen;
-		memcpy(acked_option->data, &packet->data[i + 2], olen - 2);
 		list = g_list_find_custom(data->config_options,
-				GUINT_TO_POINTER((guint) otype), is_option);
+				GUINT_TO_POINTER((guint) acked_option->type),
+				is_option);
 		if (list) {
 			/*
 			 * once we've applied the option, delete it from
@@ -1266,10 +1276,11 @@ static guint8 pppcp_process_configure_ack(struct pppcp_data *data,
 				g_list_delete_link(data->config_options, list);
 		} else
 			g_printerr("oops -- found acked option %d we didn't request\n", acked_option->type);
-		g_free(acked_option);
 
 		/* skip ahead to the next option */
-		i += olen;
+		i += acked_option->length;
+
+		g_free(acked_option);
 	}
 	return RCA;
 }
@@ -1300,14 +1311,13 @@ static guint8 pppcp_process_configure_nak(struct pppcp_data *data,
 	 * modify a value there, or add a new option.
 	 */
 	while (i < len) {
-		guint8 otype = packet->data[i];
-		guint8 olen = packet->data[i+1];
-		naked_option = g_try_malloc0(olen);
+		naked_option = extract_ppp_option(&packet->data[i]);
 		if (naked_option == NULL)
 			break;
-		naked_option->type = otype;
-		naked_option->length = olen;
-		memcpy(naked_option->data, &packet->data[i + 2], olen - 2);
+
+		/* skip ahead to the next option */
+		i += naked_option->length;
+
 		if (action->option_scan)
 			rval = action->option_scan(naked_option, data);
 		if (rval == OPTION_ACCEPT) {
@@ -1316,7 +1326,8 @@ static guint8 pppcp_process_configure_nak(struct pppcp_data *data,
 			 * match.
 			 */
 			list = g_list_find_custom(data->config_options,
-				GUINT_TO_POINTER((guint) otype), is_option);
+				GUINT_TO_POINTER((guint) naked_option->type),
+				is_option);
 			if (list) {
 				/* modify current option value to match */
 				config_option = list->data;
@@ -1326,10 +1337,11 @@ static guint8 pppcp_process_configure_nak(struct pppcp_data *data,
 				 * we need to reallocate
 				 */
 				if ((config_option->length ==
-					naked_option->length) && (olen - 2)) {
+					naked_option->length) &&
+							(naked_option - 2)) {
 						memcpy(config_option->data,
-							naked_option->data,
-							olen - 2);
+						   naked_option->data,
+						   naked_option->length - 2);
 				} else {
 					/* XXX implement this */
 					g_printerr("uh oh, option value doesn't match\n");
@@ -1344,9 +1356,6 @@ static guint8 pppcp_process_configure_nak(struct pppcp_data *data,
 			g_printerr("oops, option wasn't acceptable\n");
 			g_free(naked_option);
 		}
-
-		/* skip ahead to the next option */
-		i += olen;
 	}
 	return RCN;
 }
-- 
1.6.6.1

[PATCH 5/5] ppp: handle Config-Reject

[Kristen Carlson Accardi]

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

if our peer sends us a Config-Reject packet, we must delete that
config item and not request that it be negotiated when we send our
next Config-Request.
---
 gatchat/ppp_cp.c |   49 ++++++++++++++++++++++++++++++++++++++-----------
 1 files changed, 38 insertions(+), 11 deletions(-)

diff --git a/gatchat/ppp_cp.c b/gatchat/ppp_cp.c
index d7d70b7..ac1a37d 100644
--- a/gatchat/ppp_cp.c
+++ b/gatchat/ppp_cp.c
@@ -1363,22 +1363,49 @@ static guint8 pppcp_process_configure_nak(struct pppcp_data *data,
 static guint8 pppcp_process_configure_reject(struct pppcp_data *data,
 					struct pppcp_packet *packet)
 {
+	guint len;
+	GList *list;
+	struct ppp_option *rejected_option;
+	guint i = 0;
+
+	len = ntohs(packet->length) - CP_HEADER_SZ;
+
 	/*
 	 * make sure identifier matches that of last sent configure
 	 * request
 	 */
-	if (packet->identifier == data->config_identifier) {
-		/*
-		 * check to see which options were rejected
-		 * Rejected options must be a subset of requested
-		 * options.
-		 *
-		 * when a new configure-request is sent, we may
-		 * not request any of these options be negotiated
-		 */
-		return RCN;
+	if (packet->identifier != data->config_identifier)
+		return 0;
+
+	/*
+	 * check to see which options were rejected
+	 * Rejected options must be a subset of requested
+	 * options.
+	 *
+	 * when a new configure-request is sent, we may
+	 * not request any of these options be negotiated
+	 */
+	while (i < len) {
+		rejected_option = extract_ppp_option(&packet->data[i]);
+		if (rejected_option == NULL)
+			break;
+
+		/* skip ahead to the next option */
+		i += rejected_option->length;
+
+		/* find this option in our config options list */
+		list = g_list_find_custom(data->config_options,
+			GUINT_TO_POINTER((guint) rejected_option->type),
+			is_option);
+		if (list) {
+			/* delete this config option */
+			g_free(list->data);
+			data->config_options =
+				g_list_delete_link(data->config_options, list);
+		}
+		g_free(rejected_option);
 	}
-	return 0;
+	return RCN;
 }
 
 static guint8 pppcp_process_terminate_request(struct pppcp_data *data,
-- 
1.6.6.1

Re: [PATCH 0/5] ppp: error path fixes

[Marcel Holtmann]

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

Hi Kristen,

> Here are some patches which fix some issues with the error paths in
> the PPP control protocol code.
> 
> Kristen Carlson Accardi (5):
>   ppp: fix segfault in pppcp_send_code_reject()
>   ppp: comment fix
>   ppp: send Protocol-Reject
>   ppp: use common code to get options from pppcp packet data
>   ppp: handle Config-Reject
> 
>  gatchat/ppp.c     |   29 ++++++---
>  gatchat/ppp.h     |    1 +
>  gatchat/ppp_cp.c  |  179 +++++++++++++++++++++++++++++++++++++++-------------
>  gatchat/ppp_cp.h  |    2 +
>  gatchat/ppp_lcp.c |    5 ++
>  5 files changed, 161 insertions(+), 55 deletions(-)

all five patches have been applied.

Also I pushed the support for NBNS options upstream. With these changes
I don't see the endless loop anymore.

Problem is still that we can't leave the CHAP secrets empty. For
networks where the username and password are not in use, we should be
able to leave them empty.

Regards

Marcel

Re: [PATCH 0/5] ppp: error path fixes

[Kristen Carlson Accardi]

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

On Fri, 26 Mar 2010 19:29:00 -0700
Marcel Holtmann <marcel@holtmann.org> wrote:

> Hi Kristen,
> 
> > Here are some patches which fix some issues with the error paths in
> > the PPP control protocol code.
> > 
> > Kristen Carlson Accardi (5):
> >   ppp: fix segfault in pppcp_send_code_reject()
> >   ppp: comment fix
> >   ppp: send Protocol-Reject
> >   ppp: use common code to get options from pppcp packet data
> >   ppp: handle Config-Reject
> > 
> >  gatchat/ppp.c     |   29 ++++++---
> >  gatchat/ppp.h     |    1 +
> >  gatchat/ppp_cp.c  |  179 +++++++++++++++++++++++++++++++++++++++-------------
> >  gatchat/ppp_cp.h  |    2 +
> >  gatchat/ppp_lcp.c |    5 ++
> >  5 files changed, 161 insertions(+), 55 deletions(-)
> 
> all five patches have been applied.
> 
> Also I pushed the support for NBNS options upstream. With these changes
> I don't see the endless loop anymore.

Great, I'm glad that worked.

> 
> Problem is still that we can't leave the CHAP secrets empty. For
> networks where the username and password are not in use, we should be
> able to leave them empty.

Even after applying the auth patches I sent yesterday?  I was able to
duplicate this problem and the patches I sent yesterday fixed it.  But
since you asked for a resend, maybe you tested without apply the right
patch first?  Let me know if this problem is resolved for you, or if
you are still having an issue.