[PATCH] PPTP conntrack bugfixes

[PATCH] PPTP conntrack bugfixes

[Philip Craig]

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

I've attached 2 patches for the pptp connection tracking.

The first patch changes the way we mangle the call ids.
Since the call id is in the data portion of the TCP packet,
we must make a copy of the skb first,  otherwise bad things
happen if TCP tries to retransmit this data.  I've used
ip_nat_mangle_tcp_packet() for this purpose, although it is
slightly less efficient than needed since it handles resizing
as well.

The second patch unexpects the related connections when a
CALL_DISCONNECT_NOTIFY is received.  I've had situations
where leaving these expectations caused subsequent attempts
to connect to fail.  I suspect this is partly due to a
buggy PPTP server reusing call IDs, but there is no need to
keep these expectations around any longer.

While making the second patch, I noticed that
pptp_timeout_related() has a comment asking if a lock is
needed.  I suspect that we need to hold a write lock on
ip_conntrack_lock here?

Harald, is this format for patches okay?  I can diff them
against the patch in patch-o-matic if needed.

-- 
Philip Craig - philipc@snapgear.com - http://www.SnapGear.com
SnapGear - Custom Embedded Solutions and Security Appliances

[-- Attachment #2: pptp-mangle.patch --]
[-- Type: text/plain, Size: 5512 bytes --]

diff -u -r1.8 -r1.8.2.1
--- linux-2.4.x/net/ipv4/netfilter/ip_nat_pptp.c	3 Dec 2002 04:05:36 -0000	1.8
+++ linux-2.4.x/net/ipv4/netfilter/ip_nat_pptp.c	5 Feb 2003 05:38:06 -0000	1.8.2.1
@@ -130,7 +130,8 @@
 		  size_t datalen,
 		  struct ip_conntrack *ct,
 		  enum ip_conntrack_info ctinfo,
-		  struct ip_conntrack_expect *exp)
+		  struct ip_conntrack_expect *exp,
+		  struct sk_buff **pskb)
 
 {
 	struct PptpControlHeader *ctlh;
@@ -138,7 +139,8 @@
 	struct ip_ct_pptp_master *ct_pptp_info = &ct->help.ct_pptp_info;
 	struct ip_nat_pptp *nat_pptp_info = &ct->nat.help.nat_pptp_info;
 
-	u_int16_t msg, *cid = NULL, new_callid;
+	u_int16_t msg, new_callid;
+	unsigned int cid = 0;
 
 	/* FIXME: size checks !!! */
 	ctlh = (struct PptpControlHeader *) ((void *) pptph + sizeof(*pptph));
@@ -148,7 +150,7 @@
 	
 	switch (msg = ntohs(ctlh->messageType)) {
 		case PPTP_OUT_CALL_REQUEST:
-			cid = &pptpReq.ocreq->callID;
+			cid = (void *)&pptpReq.ocreq->callID - (void *)pptph;
 			/* FIXME: ideally we would want to reserve a call ID
 			 * here.  current netfilter NAT core is not able to do
 			 * this :( For now we use TCP source port. This breaks
@@ -162,13 +164,13 @@
 			ct_pptp_info->pns_call_id = ntohs(new_callid);
 			break;
 		case PPTP_IN_CALL_REPLY:
-			cid = &pptpReq.icreq->callID;
+			cid = (void *)&pptpReq.icreq->callID - (void *)pptph;
 			break;
 		case PPTP_CALL_CLEAR_REQUEST:
-			cid = &pptpReq.clrreq->callID;
+			cid = (void *)&pptpReq.clrreq->callID - (void *)pptph;
 			break;
 		case PPTP_CALL_DISCONNECT_NOTIFY:
-			cid = &pptpReq.disc->callID;
+			cid = (void *)&pptpReq.disc->callID - (void *)pptph;
 			break;
 
 		default:
@@ -191,11 +193,10 @@
 	IP_NF_ASSERT(cid);
 
 	DEBUGP("altering call id from 0x%04x to 0x%04x\n",
-		ntohs(*cid), ntohs(new_callid));
+		ntohs(*(u_int16_t *)((void *)pptph + cid)), ntohs(new_callid));
 	/* mangle packet */
-	tcph->check = ip_nat_cheat_check(*cid^0xFFFF, 
-					 new_callid, tcph->check);
-	*cid = new_callid;
+	ip_nat_mangle_tcp_packet(pskb, ct, ctinfo, cid, sizeof(new_callid),
+			(char *)&new_callid, sizeof(new_callid));
 
 	return NF_ACCEPT;
 }
@@ -206,15 +207,17 @@
 		 size_t datalen,
 		 struct ip_conntrack *ct,
 		 enum ip_conntrack_info ctinfo,
-		 struct ip_conntrack_expect *oldexp)
+		 struct ip_conntrack_expect *oldexp,
+		 struct sk_buff **pskb)
 {
 	struct PptpControlHeader *ctlh;
 	union pptp_ctrl_union pptpReq;
 	struct ip_ct_pptp_master *ct_pptp_info = &ct->help.ct_pptp_info;
 	struct ip_nat_pptp *nat_pptp_info = &ct->nat.help.nat_pptp_info;
 
-	u_int16_t msg, new_cid = 0, new_pcid, *pcid = NULL, *cid = NULL;
+	u_int16_t msg, new_cid = 0, new_pcid;
 	u_int32_t old_dst_ip;
+	unsigned int pcid = 0, cid = 0;
 
 	struct ip_conntrack_tuple t, inv_t;
 	struct ip_conntrack_tuple *orig_t, *reply_t;
@@ -227,8 +230,8 @@
 
 	switch (msg = ntohs(ctlh->messageType)) {
 	case PPTP_OUT_CALL_REPLY:
-		pcid = &pptpReq.ocack->peersCallID;	
-		cid = &pptpReq.ocack->callID;
+		pcid = (void *)&pptpReq.ocack->peersCallID - (void *)pptph;
+		cid = (void *)&pptpReq.ocack->callID - (void *)pptph;
 		if (!oldexp) {
 			DEBUGP("outcall but no expectation\n");
 			break;
@@ -274,7 +277,7 @@
 		ip_ct_gre_keymap_change(oldexp->proto.gre.keymap_reply, &inv_t);
 		break;
 	case PPTP_IN_CALL_CONNECT:
-		pcid = &pptpReq.iccon->peersCallID;
+		pcid = (void *)&pptpReq.iccon->peersCallID - (void *)pptph;
 		if (!oldexp)
 			break;
 		old_dst_ip = oldexp->tuple.dst.ip;
@@ -299,7 +302,7 @@
 		/* only need to nat in case PAC is behind NAT box */
 		break;
 	case PPTP_WAN_ERROR_NOTIFY:
-		pcid = &pptpReq.wanerr->peersCallID;
+		pcid = (void *)&pptpReq.wanerr->peersCallID - (void *)pptph;
 		break;
 	default:
 		DEBUGP("unknown inbound packet %s\n",
@@ -318,18 +321,18 @@
 	/* mangle packet */
 	IP_NF_ASSERT(pcid);
 	DEBUGP("altering peer call id from 0x%04x to 0x%04x\n",
-		ntohs(*pcid), ntohs(new_pcid));
-	tcph->check = ip_nat_cheat_check(*pcid^0xFFFF, 
-					 new_pcid, tcph->check);
-	*pcid = new_pcid;
+		ntohs(*(u_int16_t *)((void *)pptph + pcid)), ntohs(new_pcid));
+	ip_nat_mangle_tcp_packet(pskb, ct, ctinfo, pcid, sizeof(new_pcid),
+			(char *)&new_pcid, sizeof(new_pcid));
 
 	if (new_cid) {
 		IP_NF_ASSERT(cid);
 		DEBUGP("altering call id from 0x%04x to 0x%04x\n",
-			ntohs(*cid), ntohs(new_cid));
-		tcph->check = ip_nat_cheat_check(*cid^0xFFFF,
-						new_cid, tcph->check);
-		*cid = new_cid;
+			ntohs(*(u_int16_t *)((void *)pptph + cid)),
+			ntohs(new_cid));
+		ip_nat_mangle_tcp_packet(pskb, ct, ctinfo,
+				cid, sizeof(new_cid),
+				(char *)&new_cid, sizeof(new_cid));
 	}
 
 	/* great, at least we don't need to resize packets */
@@ -347,7 +350,6 @@
 	struct tcphdr *tcph = (void *) iph + iph->ihl*4;
 	unsigned int datalen = (*pskb)->len - iph->ihl*4 - tcph->doff*4;
 	struct pptp_pkt_hdr *pptph;
-	void *datalimit;
 
 	int dir;
 
@@ -379,16 +381,15 @@
 	/* FIXME: checks on pptp version, magic cookie, etc. */
 
 	pptph = (struct pptp_pkt_hdr *) ((void *)tcph + tcph->doff*4);
-	datalimit = (void *) pptph + datalen;
 
 	LOCK_BH(&ip_pptp_lock);
 
 	if (dir == IP_CT_DIR_ORIGINAL) {
 		/* reuqests sent by client to server (PNS->PAC) */
-		pptp_outbound_pkt(tcph, pptph, datalen, ct, ctinfo, exp);
+		pptp_outbound_pkt(tcph, pptph, datalen, ct, ctinfo, exp, pskb);
 	} else {
 		/* response from the server to the client (PAC->PNS) */
-		pptp_inbound_pkt(tcph, pptph, datalen, ct, ctinfo, exp);
+		pptp_inbound_pkt(tcph, pptph, datalen, ct, ctinfo, exp, pskb);
 	}
 
 	UNLOCK_BH(&ip_pptp_lock);

[-- Attachment #3: pptp-unexpect.patch --]
[-- Type: text/plain, Size: 1237 bytes --]

diff -u -r1.10 -r1.10.2.1
--- linux-2.4.x/net/ipv4/netfilter/ip_conntrack_pptp.c	29 Nov 2002 06:37:32 -0000	1.10
+++ linux-2.4.x/net/ipv4/netfilter/ip_conntrack_pptp.c	4 Feb 2003 07:10:46 -0000	1.10.2.1
@@ -96,16 +96,19 @@
 /* timeout GRE data connections */
 static int pptp_timeout_related(struct ip_conntrack *ct)
 {
-	struct list_head *cur_item;
+	struct list_head *cur_item, *next;
 	struct ip_conntrack_expect *exp;
 
 	/* FIXME: do we have to lock something ? */
-	list_for_each(cur_item, &ct->sibling_list) {
+	for (cur_item = ct->sibling_list.next;
+	    cur_item != &ct->sibling_list; cur_item = next) {
+		next = cur_item->next;
 		exp = list_entry(cur_item, struct ip_conntrack_expect,
 				 expected_list);
 
 		if (!exp->sibling) {
 			ip_ct_gre_keymap_destroy(exp);
+			ip_conntrack_unexpect_related(exp);
 			continue;
 		}
 
@@ -319,7 +322,6 @@
 
 		/* untrack this call id, unexpect GRE packets */
 		pptp_timeout_related(ct);
-		/* NEWNAT: look up exp for call id and unexpct_related */
 		break;
 
 	case PPTP_WAN_ERROR_NOTIFY:
@@ -481,8 +483,6 @@
 
 		/* untrack this call id, unexpect GRE packets */
 		pptp_timeout_related(ct);
-		/* no need to call unexpect_related since master conn
-		 * dies anyway */
 	}
 
 

Re: [PATCH] PPTP conntrack bugfixes

[Harald Welte]

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

On Wed, Feb 05, 2003 at 06:20:14PM +1000, Philip Craig wrote:
> I've attached 2 patches for the pptp connection tracking.

thanks, pptp conntrack/nat seems to be more popular than i thought ;)

> The first patch changes the way we mangle the call ids.
> Since the call id is in the data portion of the TCP packet,
> we must make a copy of the skb first,  otherwise bad things
> happen if TCP tries to retransmit this data.  I've used
> ip_nat_mangle_tcp_packet() for this purpose, although it is
> slightly less efficient than needed since it handles resizing
> as well.

mh. I've never thought of this stuff being used for a _local_ pptp
server - and this is the only case where tcp would retransmit something.

I think I'll change your patch to not make this additional call to
ip_nat_mangle_tcp_packet().

> The second patch unexpects the related connections when a
> CALL_DISCONNECT_NOTIFY is received.  I've had situations
> where leaving these expectations caused subsequent attempts
> to connect to fail.  I suspect this is partly due to a
> buggy PPTP server reusing call IDs, but there is no need to
> keep these expectations around any longer.

yes, this totally makes sense. thanks.

> While making the second patch, I noticed that
> pptp_timeout_related() has a comment asking if a lock is
> needed.  I suspect that we need to hold a write lock on
> ip_conntrack_lock here?

Yes, but it was a more complex issue than that.  I need to  think this
through thoroughly.

> Harald, is this format for patches okay?  I can diff them
> against the patch in patch-o-matic if needed.

it's perfectly fine.  Expect me to take some time for mergeing and testing.

> Philip Craig - philipc@snapgear.com - http://www.SnapGear.com
> SnapGear - Custom Embedded Solutions and Security Appliances

-- 
- Harald Welte / laforge@gnumonks.org               http://www.gnumonks.org/
============================================================================
"If this were a dictatorship, it'd be a heck of a lot easier, just so long
 as I'm the dictator."  --  George W. Bush Dec 18, 2000

[-- Attachment #2: Type: application/pgp-signature, Size: 232 bytes --]

Re: [PATCH] PPTP conntrack bugfixes

[Harald Welte]

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

On Wed, Feb 05, 2003 at 06:20:14PM +1000, Philip Craig wrote:

> The first patch changes the way we mangle the call ids.
> Since the call id is in the data portion of the TCP packet,
> we must make a copy of the skb first,  otherwise bad things
> happen if TCP tries to retransmit this data.  I've used
> ip_nat_mangle_tcp_packet() for this purpose, although it is
> slightly less efficient than needed since it handles resizing
> as well.

great.  I have now decided to use ip_nat_mangle_tcp_packet, otherwise
the code really gets too ugly.  They only change I made is the
calculation of the 'offset' parameter needed for the mangle_tcp_packet()
function.

> The second patch unexpects the related connections when a
> CALL_DISCONNECT_NOTIFY is received.  I've had situations
> where leaving these expectations caused subsequent attempts
> to connect to fail.  I suspect this is partly due to a
> buggy PPTP server reusing call IDs, but there is no need to
> keep these expectations around any longer.

as stated before, this is correct.

I'll submit the updated patch to CVS soon.

> Philip Craig - philipc@snapgear.com - http://www.SnapGear.com
> SnapGear - Custom Embedded Solutions and Security Appliances
-- 
- Harald Welte <laforge@gnumonks.org>               http://www.gnumonks.org/
============================================================================
"If this were a dictatorship, it'd be a heck of a lot easier, just so long
 as I'm the dictator."  --  George W. Bush Dec 18, 2000

[-- Attachment #2: Type: application/pgp-signature, Size: 232 bytes --]