PPTP connection tracking

PPTP connection tracking

[Philip Craig]

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

Hi,

I've been experimenting with the PPTP connection tracking
module.  For the most part it works quite well.  DNAT even
works with a few minor fixes.  (This doesn't include DNAT of
the PAC call id, but I can't see any use in doing that.)

There's one problem remaining.  Once the control connection
has established the call, the helper only registers a GRE
expectation for the PNS->PAC direction.  If the GRE packet in
the PAC->PNS direction arrives first, then a conntrack entry
is created for it which doesn't include the appropriate NAT
manips.  Then when the GRE packet in the PNS->PAC direction
arrives, it fails to set up the SNAT manip because
gre_unique_tuple finds that the tuple is already in use.

A solution to this which I've implemented is to register
GRE expectations for both directions.  Only one will get
matched.  Currently the other one hangs around until the
master connection is closed, but I assume there is some way
of cleaning it up.

This works fine for PPTP passthrough.  However, when I try
to initiate a PPTP connection from the NAT device, it fails
if the PAC->PNS expectation is matched.  I believe this is
because pptp_nat_expected is not called at NF_IP_LOCAL_IN, so
it doesn't set up the manip to NAT the PNS call id.

Am I on the right track here?  Is the fix to add support for
SNAT in NF_IP_LOCAL_IN?

I've attached a patch for what I've done so far.

Regards,
Phil

-- 
Philip Craig     Software Engineer     http://www.SnapGear.com
philipc@snapgear.com  Ph: +61 7 3435 2821  Fx: +61 7 3891 3630
SnapGear  -  Custom Embedded Solutions and Security Appliances


[-- Attachment #2: pptp-conntrack-nat.patch.diff --]
[-- Type: text/plain, Size: 13363 bytes --]

diff -u -r1.11 pptp-conntrack-nat.patch
--- extra/pptp-conntrack-nat.patch	12 Nov 2002 20:17:22 -0000	1.11
+++ extra/pptp-conntrack-nat.patch	29 Nov 2002 07:49:28 -0000
@@ -316,7 +316,7 @@
 +#endif /* _CONNTRACK_PPTP_H */
 --- linuxppc-020802-newnat/include/linux/netfilter_ipv4/ip_conntrack_proto_gre.h	Thu Jan  1 01:00:00 1970
 +++ linuxppc-020802-newnat14-h323/include/linux/netfilter_ipv4/ip_conntrack_proto_gre.h	Fri Aug  2 14:37:28 2002
-@@ -0,0 +1,121 @@
+@@ -0,0 +1,123 @@
 +#ifndef _CONNTRACK_PROTO_GRE_H
 +#define _CONNTRACK_PROTO_GRE_H
 +#include <asm/byteorder.h>
@@ -396,13 +396,13 @@
 +};
 +
 +#ifdef __KERNEL__
++struct ip_conntrack_expect;
 +
 +/* structure for original <-> reply keymap */
 +struct ip_ct_gre_keymap {
 +	struct list_head list;
 +
 +	struct ip_conntrack_tuple tuple;
-+	struct ip_conntrack_expect *master;
 +};
 +
 +
@@ -415,6 +415,8 @@
 +void ip_ct_gre_keymap_change(struct ip_ct_gre_keymap *km,
 +			     struct ip_conntrack_tuple *t);
 +
++/* delete keymap entries */
++void ip_ct_gre_keymap_destroy(struct ip_conntrack_expect *exp);
 +
 +
 +/* get pointer to gre key, if present */
@@ -545,7 +547,7 @@
  
  	new = LIST_FIND(&expect_list, resent_expect,
  		        struct ip_conntrack_expect *, &expect->tuple, &expect->mask);
-@@ -971,11 +976,10 @@
+@@ -1064,15 +1068,14 @@
 
  	MUST_BE_READ_LOCKED(&ip_conntrack_lock);
  	WRITE_LOCK(&ip_conntrack_expect_tuple_lock);
@@ -560,9 +562,14 @@
  	if (expect->ct_tuple.dst.protonum == 0) {
  		/* Never seen before */
  		DEBUGP("change expect: never seen before\n");
+-		if (!ip_ct_tuple_equal(&expect->tuple, newtuple) 
++		if (!ip_ct_tuple_mask_cmp(&expect->tuple, newtuple, &expect->mask)
+ 		    && LIST_FIND(&ip_conntrack_expect_list, expect_clash,
+ 			         struct ip_conntrack_expect *, newtuple, &expect->mask)) {
+ 			/* Force NAT to find an unused tuple */
 --- linux-2.4.18-newnat/net/ipv4/netfilter/ip_conntrack_pptp.c	Thu Jan  1 01:00:00 1970
 +++ linux-2.4.18-pptp3.01//net/ipv4/netfilter/ip_conntrack_pptp.c	Mon Apr  8 16:40:37 2002
-@@ -0,0 +1,540 @@
+@@ -0,0 +1,558 @@
 +/*
 + * ip_conntrack_pptp.c	- Version $Revision: 1.11 $
 + *
@@ -652,7 +659,7 @@
 +		ct->tuplehash[IP_CT_DIR_ORIGINAL].tuple.src.u.gre.key =
 +			htonl(master->help.ct_pptp_info.pac_call_id);
 +		ct->tuplehash[IP_CT_DIR_REPLY].tuple.dst.u.gre.key =
-+			htonl(master->help.ct_pptp_info.pns_call_id);
++			htonl(master->help.ct_pptp_info.pac_call_id);
 +	}
 +
 +	return 0;
@@ -669,8 +676,10 @@
 +		exp = list_entry(cur_item, struct ip_conntrack_expect,
 +				 expected_list);
 +
-+		if (!exp->sibling)
++		if (!exp->sibling) {
++			ip_ct_gre_keymap_destroy(exp);
 +			continue;
++		}
 +
 +		DEBUGP("setting timeout of conntrack %p to 0\n",
 +			exp->sibling);
@@ -693,7 +702,7 @@
 +	struct ip_conntrack_tuple inv_tuple;
 +
 +	memset(&exp, 0, sizeof(exp));
-+	/* tuple in original direction, PAC->PNS */
++	/* tuple in original direction, PNS->PAC */
 +	exp.tuple.src.ip = master->tuplehash[IP_CT_DIR_ORIGINAL].tuple.src.ip;
 +	exp.tuple.src.u.gre.key = htonl(ntohs(peer_callid));
 +	exp.tuple.dst.ip = master->tuplehash[IP_CT_DIR_ORIGINAL].tuple.dst.ip;
@@ -727,6 +736,22 @@
 +	
 +	ip_conntrack_expect_related(master, &exp);
 +
++	/* tuple in reply direction, PAC->PNS */
++	exp.tuple.src.ip = master->tuplehash[IP_CT_DIR_REPLY].tuple.src.ip;
++	exp.tuple.src.u.gre.key = htonl(ntohs(callid));
++	exp.tuple.dst.ip = master->tuplehash[IP_CT_DIR_REPLY].tuple.dst.ip;
++	exp.tuple.dst.u.gre.key = htonl(ntohs(peer_callid));
++
++	DEBUGP("calling expect_related ");
++	DUMP_TUPLE_RAW(&exp.tuple);
++	
++	/* Add GRE keymap entries */
++	ip_ct_gre_keymap_add(&exp, &exp.tuple, 0);
++	invert_tuplepr(&inv_tuple, &exp.tuple);
++	ip_ct_gre_keymap_add(&exp, &inv_tuple, 1);
++	
++	ip_conntrack_expect_related(master, &exp);
++
 +	return 0;
 +}
 +
@@ -1132,7 +1157,7 @@
 +#endif
 --- linux-2.4.18-pptp3.01//net/ipv4/netfilter/ip_conntrack_proto_gre.c	Mon Apr  8 16:40:23 2002
 +++ linux-2.4.18-pptp3.01/net/ipv4/netfilter/ip_conntrack_proto_gre.c	2002-08-29 13:01:00.000000000 +0200
-@@ -0,0 +1,334 @@
+@@ -0,0 +1,341 @@
 +/*
 + * ip_conntrack_proto_gre.c - Version $Revision: 1.11 $
 + *
@@ -1248,7 +1273,6 @@
 +	memset(km, 0, sizeof(*km));
 +
 +	memcpy(&km->tuple, t, sizeof(*t));
-+	km->master = exp;
 +
 +	if (!reply)
 +		exp->proto.gre.keymap_orig = km;
@@ -1277,6 +1301,25 @@
 +	WRITE_UNLOCK(&ip_ct_gre_lock);
 +}
 +
++/* destroy the keymap entries associated with specified expect */
++void ip_ct_gre_keymap_destroy(struct ip_conntrack_expect *exp)
++{
++	WRITE_LOCK(&ip_ct_gre_lock);
++	if (exp->proto.gre.keymap_orig) {
++		DEBUGP("removing %p from list\n", exp->proto.gre.keymap_orig);
++		list_del(&exp->proto.gre.keymap_orig->list);
++		kfree(exp->proto.gre.keymap_orig);
++		exp->proto.gre.keymap_orig = NULL;
++	}
++	if (exp->proto.gre.keymap_reply) {
++		DEBUGP("removing %p from list\n", exp->proto.gre.keymap_reply);
++		list_del(&exp->proto.gre.keymap_reply->list);
++		kfree(exp->proto.gre.keymap_reply);
++		exp->proto.gre.keymap_reply = NULL;
++	}
++	WRITE_UNLOCK(&ip_ct_gre_lock);
++}
++
 +
 +/* PUBLIC CONNTRACK PROTO HELPER FUNCTIONS */
 +
@@ -1405,18 +1448,7 @@
 +		return;
 +	}
 +
-+	WRITE_LOCK(&ip_ct_gre_lock);
-+	if (master->proto.gre.keymap_orig) {
-+		DEBUGP("removing %p from list\n", master->proto.gre.keymap_orig);
-+		list_del(&master->proto.gre.keymap_orig->list);
-+		kfree(master->proto.gre.keymap_orig);
-+	}
-+	if (master->proto.gre.keymap_reply) {
-+		DEBUGP("removing %p from list\n", master->proto.gre.keymap_reply);
-+		list_del(&master->proto.gre.keymap_reply->list);
-+		kfree(master->proto.gre.keymap_reply);
-+	}
-+	WRITE_UNLOCK(&ip_ct_gre_lock);
++	ip_ct_gre_keymap_destroy(master);
 +}
 +
 +/* protocol helper struct */
@@ -1498,7 +1530,7 @@
  		/* We now have two tuples (SRCIP/SRCPT/DSTIP/DSTPT):
 --- linux-2.4.18-newnat/net/ipv4/netfilter/ip_nat_pptp.c	Thu Jan  1 01:00:00 1970
 +++ linux-2.4.18-pptp3.01//net/ipv4/netfilter/ip_nat_pptp.c	Mon Apr  8 16:40:47 2002
-@@ -0,0 +1,425 @@
+@@ -0,0 +1,426 @@
 +/*
 + * ip_nat_pptp.c	- Version $Revision: 1.11 $
 + *
@@ -1558,7 +1590,7 @@
 +	struct ip_nat_multi_range mr;
 +	struct ip_ct_pptp_master *ct_pptp_info;
 +	struct ip_nat_pptp *nat_pptp_info;
-+	u_int32_t newsrcip, newdstip, newcid;
++	u_int32_t newip, newcid;
 +	int ret;
 +
 +	IP_NF_ASSERT(info);
@@ -1573,7 +1605,7 @@
 +
 +	/* need to alter GRE tuple because conntrack expectfn() used 'wrong'
 +	 * (unmanipulated) values */
-+	if (hooknum == NF_IP_PRE_ROUTING) {
++	if (HOOK2MANIP(hooknum) == IP_NAT_MANIP_DST) {
 +		DEBUGP("completing tuples with NAT info \n");
 +		/* we can do this, since we're unconfirmed */
 +		if (ct->tuplehash[IP_CT_DIR_ORIGINAL].tuple.dst.u.gre.key ==
@@ -1581,51 +1613,44 @@
 +			/* assume PNS->PAC */
 +			ct->tuplehash[IP_CT_DIR_ORIGINAL].tuple.src.u.gre.key =
 +				htonl(nat_pptp_info->pns_call_id);
-+//			ct->tuplehash[IP_CT_DIR_REPLY].tuple.src.u.gre.key =
-+//				htonl(nat_pptp_info->pac_call_id);
 +			ct->tuplehash[IP_CT_DIR_REPLY].tuple.dst.u.gre.key =
 +				htonl(nat_pptp_info->pns_call_id);
++			newip = master->tuplehash[IP_CT_DIR_REPLY].tuple.src.ip;
++			newcid = htonl(nat_pptp_info->pac_call_id);
 +		} else {
 +			/* assume PAC->PNS */
-+			DEBUGP("WRONG DIRECTION\n");
 +			ct->tuplehash[IP_CT_DIR_ORIGINAL].tuple.src.u.gre.key =
 +				htonl(nat_pptp_info->pac_call_id);
 +			ct->tuplehash[IP_CT_DIR_REPLY].tuple.dst.u.gre.key =
-+				htonl(nat_pptp_info->pns_call_id);
++				htonl(nat_pptp_info->pac_call_id);
++			newip = master->tuplehash[IP_CT_DIR_ORIGINAL].tuple.src.ip;
++			newcid = htonl(nat_pptp_info->pns_call_id);
 +		}
 +	}
-+
-+	if (HOOK2MANIP(hooknum) == IP_NAT_MANIP_DST) {
-+		newdstip = master->tuplehash[IP_CT_DIR_ORIGINAL].tuple.dst.ip;
-+		newcid = htonl(master->nat.help.nat_pptp_info.pac_call_id);
-+
-+		mr.rangesize = 1;
-+		mr.range[0].flags = IP_NAT_RANGE_MAP_IPS | IP_NAT_RANGE_PROTO_SPECIFIED;
-+		mr.range[0].min_ip = mr.range[0].max_ip = newdstip;
-+		mr.range[0].min = mr.range[0].max = 
-+			((union ip_conntrack_manip_proto ) { newcid }); 
-+		DEBUGP("change dest ip to %u.%u.%u.%u\n", 
-+			NIPQUAD(newdstip));
-+		DEBUGP("change dest key to 0x%x\n", ntohl(newcid));
-+		ret = ip_nat_setup_info(ct, &mr, hooknum);
-+	} else {
-+		newsrcip = master->tuplehash[IP_CT_DIR_REPLY].tuple.dst.ip;
-+		/* nat_multi_range is in network byte order, and GRE tuple
-+		 * is 32 bits, not 16 like callID */
-+		newcid = htonl(master->help.ct_pptp_info.pns_call_id);
-+
-+		mr.rangesize = 1;
-+		mr.range[0].flags = IP_NAT_RANGE_MAP_IPS
-+			            |IP_NAT_RANGE_PROTO_SPECIFIED;
-+		mr.range[0].min_ip = mr.range[0].max_ip = newsrcip;
-+		mr.range[0].min = mr.range[0].max = 
-+			((union ip_conntrack_manip_proto ) { newcid });
-+		DEBUGP("change src ip to %u.%u.%u.%u\n", 
-+			NIPQUAD(newsrcip));
-+		DEBUGP("change 'src' key to 0x%x\n", ntohl(newcid));
-+		ret = ip_nat_setup_info(ct, &mr, hooknum);
++	else {
++		if (ct->tuplehash[IP_CT_DIR_ORIGINAL].tuple.dst.u.gre.key ==
++			htonl(ct_pptp_info->pac_call_id)) {	
++			/* assume PNS->PAC */
++			newip = master->tuplehash[IP_CT_DIR_REPLY].tuple.dst.ip;
++			newcid = htonl(ct_pptp_info->pns_call_id);
++		}
++		else {
++			/* assume PAC->PNS */
++			newip = master->tuplehash[IP_CT_DIR_ORIGINAL].tuple.dst.ip;
++			newcid = htonl(ct_pptp_info->pac_call_id);
++		}
 +	}
 +
++	mr.rangesize = 1;
++	mr.range[0].flags = IP_NAT_RANGE_MAP_IPS | IP_NAT_RANGE_PROTO_SPECIFIED;
++	mr.range[0].min_ip = mr.range[0].max_ip = newip;
++	mr.range[0].min = mr.range[0].max = 
++		((union ip_conntrack_manip_proto ) { newcid }); 
++	DEBUGP("change ip to %u.%u.%u.%u\n", 
++		NIPQUAD(newip));
++	DEBUGP("change key to 0x%x\n", ntohl(newcid));
++	ret = ip_nat_setup_info(ct, &mr, hooknum);
++
 +	UNLOCK_BH(&ip_pptp_lock);
 +
 +	return ret;
@@ -1724,7 +1749,7 @@
 +	u_int16_t msg, new_cid = 0, new_pcid, *pcid = NULL, *cid = NULL;
 +	u_int32_t old_dst_ip;
 +
-+	struct ip_conntrack_tuple t;
++	struct ip_conntrack_tuple t, inv_t;
 +
 +	/* FIXME: size checks !!! */
 +	ctlh = (struct PptpControlHeader *) ((void *) pptph + sizeof(*pptph));
@@ -1742,6 +1767,7 @@
 +		}
 +		old_dst_ip = oldexp->tuple.dst.ip;
 +		t = oldexp->tuple;
++		invert_tuplepr(&inv_t, &t);
 +
 +		/* save original PAC call ID in nat_info */
 +		nat_pptp_info->pac_call_id = ct_pptp_info->pac_call_id;
@@ -1753,12 +1779,24 @@
 +		/* alter expectation */
 +		if (t.dst.ip == ct->tuplehash[IP_CT_DIR_ORIGINAL].tuple.dst.ip) {
 +			/* expectation for PNS->PAC direction */
-+			t.dst.u.gre.key = htonl(ct_pptp_info->pac_call_id);
 +			t.src.u.gre.key = htonl(nat_pptp_info->pns_call_id);
++			t.dst.u.gre.key = htonl(ct_pptp_info->pac_call_id);
++			inv_t.src.ip =
++				ct->tuplehash[IP_CT_DIR_REPLY].tuple.src.ip;
++			inv_t.dst.ip =
++				ct->tuplehash[IP_CT_DIR_REPLY].tuple.dst.ip;
++			inv_t.src.u.gre.key = htonl(nat_pptp_info->pac_call_id);
++			inv_t.dst.u.gre.key = htonl(ct_pptp_info->pns_call_id);
 +		} else {
 +			/* expectation for PAC->PNS direction */
-+			t.dst.ip = ct->tuplehash[IP_CT_DIR_REPLY].tuple.dst.ip;
-+			DEBUGP("EXPECTATION IN WRONG DIRECTION!!!\n");
++			t.src.u.gre.key = htonl(nat_pptp_info->pac_call_id);
++			t.dst.u.gre.key = htonl(ct_pptp_info->pns_call_id);
++			inv_t.src.ip =
++				ct->tuplehash[IP_CT_DIR_ORIGINAL].tuple.src.ip;
++			inv_t.dst.ip =
++				ct->tuplehash[IP_CT_DIR_ORIGINAL].tuple.dst.ip;
++			inv_t.src.u.gre.key = htonl(nat_pptp_info->pns_call_id);
++			inv_t.dst.u.gre.key = htonl(ct_pptp_info->pac_call_id);
 +		}
 +
 +		if (!ip_conntrack_change_expect(oldexp, &t)) {
@@ -1767,13 +1805,7 @@
 +			DEBUGP("can't change expect\n");
 +		}
 +		ip_ct_gre_keymap_change(oldexp->proto.gre.keymap_orig, &t);
-+		/* reply keymap */
-+		t.src.ip = ct->tuplehash[IP_CT_DIR_REPLY].tuple.src.ip;
-+		t.dst.ip = ct->tuplehash[IP_CT_DIR_REPLY].tuple.dst.ip;
-+		t.src.u.gre.key = htonl(nat_pptp_info->pac_call_id);
-+		t.dst.u.gre.key = htonl(ct_pptp_info->pns_call_id);
-+		ip_ct_gre_keymap_change(oldexp->proto.gre.keymap_reply, &t);
-+
++		ip_ct_gre_keymap_change(oldexp->proto.gre.keymap_reply, &inv_t);
 +		break;
 +	case PPTP_IN_CALL_CONNECT:
 +		pcid = &pptpReq.iccon->peersCallID;
@@ -1864,7 +1896,8 @@
 +		       dir == IP_CT_DIR_ORIGINAL ? "ORIG" : "REPLY",
 +		       hooknum == NF_IP_POST_ROUTING ? "POSTROUTING"
 +		       : hooknum == NF_IP_PRE_ROUTING ? "PREROUTING"
-+		       : hooknum == NF_IP_LOCAL_OUT ? "OUTPUT" : "???");
++		       : hooknum == NF_IP_LOCAL_OUT ? "OUTPUT"
++		       : hooknum == NF_IP_LOCAL_IN ? "INPUT" : "???");
 +		return NF_ACCEPT;
 +	}
 +
@@ -1926,7 +1959,7 @@
 +module_exit(fini);
 --- linux-2.4.18-newnat/net/ipv4/netfilter/ip_nat_proto_gre.c	Thu Jan  1 01:00:00 1970
 +++ linux-2.4.18-pptp3.01//net/ipv4/netfilter/ip_nat_proto_gre.c	Mon Apr  8 16:40:56 2002
-@@ -0,0 +1,218 @@
+@@ -0,0 +1,225 @@
 +/*
 + * ip_nat_proto_gre.c - Version $Revision: 1.11 $
 + *
@@ -1978,8 +2011,15 @@
 +	     const union ip_conntrack_manip_proto *min,
 +	     const union ip_conntrack_manip_proto *max)
 +{
-+	return ntohl(tuple->src.u.gre.key) >= ntohl(min->gre.key)
-+		&& ntohl(tuple->src.u.gre.key) <= ntohl(max->gre.key);
++	u_int32_t key;
++
++	if (maniptype == IP_NAT_MANIP_SRC)
++		key = tuple->src.u.gre.key;
++	else
++		key = tuple->dst.u.gre.key;
++
++	return ntohl(key) >= ntohl(min->gre.key)
++		&& ntohl(key) <= ntohl(max->gre.key);
 +}
 +
 +/* generate unique tuple ... */

Re: PPTP connection tracking

[Ilguiz Latypov]

Philip,

Thank you for sharing the patch.  I observed the same problem where the
client's firewall would not rewrite the replies.  That is, the replies
could be seen arriving on the external interface but not on the internal
interface.

With the aid of ethereal I noticed that the first GRE packet between the
client and the server might be the one coming from the server (with
encapslated PPP LCP request).

I am a novice in Linux kernel programming so I could only test your patch.

The first attempt to connect to a remote PPTP server (PoPToP) produced an
error on the server side.  I suspect this could be related to the fact
that I installed the same PPTP kernel modules on the server side.  I
understand that those modules are superfluous on the server (PAC) side.

The error message is produced by the run time error in encaps_gre() where
the write() function is called to send a GRE packet.  I wonder if that
issue is related to the one you mentioned in the bottom of your message.

It appears to me that I was able to establish 2 parallel connections from
Windows workstations later and observe correct reply rewriting on the
client firewall.  So the run time error might not be always reproduced.

Thanks,

--
Ilguiz Latypov
Net Integration Technologies, Inc

tel. +1 (514) 281 9191 x 117

=============================================================================
pppd: sent [LCP ConfReq id=0x1 <asyncmap 0x0> <auth chap 81> <magic 0x3f3847ac> <pcomp> <accomp>]
pptpd: GRE: xmit failed from decaps_hdlc: Operation not permitted
pptpd: CTRL: PTY read or GRE write failed (pty,gre)=(16,17)
=============================================================================

Re: PPTP connection tracking

[Ilguiz Latypov]

[-- Attachment #1: Type: TEXT/PLAIN, Size: 558 bytes --]


I changed pptpctrl.c to issue pseudo-unique Call ID's for every incoming
TCP connection.  Each connection spawns a process.

Is this modification reasonable?  I will submit it to pptpd maintainer as
well.

--
Ilguiz Latypov
Net Integration Technologies, Inc

tel. +1 (514) 281 9191 x 117

On Mon, 2 Dec 2002, Ilguiz Latypov wrote:

> The error message is produced by the run time error in encaps_gre() where
> the write() function is called to send a GRE packet.  I wonder if that
> issue is related to the one you mentioned in the bottom of your message.


[-- Attachment #2: Type: TEXT/plain, Size: 1059 bytes --]

Index: ctrlpacket.c
===================================================================
RCS file: /home/cvs/wvpackages/pptpd/ctrlpacket.c,v
retrieving revision 1.1.1.2
diff -u -r1.1.1.2 ctrlpacket.c
--- ctrlpacket.c	4 Jun 2001 18:09:34 -0000	1.1.1.2
+++ ctrlpacket.c	29 Nov 2002 22:31:41 -0000
@@ -25,6 +25,8 @@
 #include <unistd.h>
 #include <string.h>
 #include <errno.h>
+#include <sys/time.h>
+#include <time.h>
 
 #include "pptpdefs.h"
 #include "pptpctrl.h"
@@ -630,6 +632,18 @@
 u_int16_t getcall()
 {
 	static u_int16_t i = 0;
+
+	/* Start with a random Call ID.  This is to allocate unique Call ID's 
+	 * across multiple TCP PPTP connections.  In this way remote clients 
+	 * masqueraded by a firewall will put unique peer call ID's into
+	 * GRE packets that will have the same source IP address of the 
+	 * firewall. */
+	if (!i) {
+		struct timeval tv;
+		if (gettimeofday(&tv, NULL) == 0) {
+			i = ((tv.tv_sec & 0x0FFF) << 4) + (tv.tv_usec >> 16);
+		}
+	}
 
 	if(!_pac_init) {
 		_pac_call_id = htons(-1);

Re: PPTP connection tracking

[Philip Craig]

Ilguiz Latypov wrote:
> The first attempt to connect to a remote PPTP server (PoPToP) produced an
> error on the server side.  I suspect this could be related to the fact
> that I installed the same PPTP kernel modules on the server side.  I
> understand that those modules are superfluous on the server (PAC) side.

While you don't need the PPTP connection tracking on the
server side, it shouldn't cause the connection to fail.
I've been testing with various combinations of connection
tracking on server/client/firewall.

> The error message is produced by the run time error in encaps_gre() where
> the write() function is called to send a GRE packet.  I wonder if that
> issue is related to the one you mentioned in the bottom of your message.
> 
> It appears to me that I was able to establish 2 parallel connections from
> Windows workstations later and observe correct reply rewriting on the
> client firewall.  So the run time error might not be always reproduced.
> 
> =============================================================================
> pppd: sent [LCP ConfReq id=0x1 <asyncmap 0x0> <auth chap 81> <magic 0x3f3847ac> <pcomp> <accomp>]
> pptpd: GRE: xmit failed from decaps_hdlc: Operation not permitted
> pptpd: CTRL: PTY read or GRE write failed (pty,gre)=(16,17)
> =============================================================================

You get an operation not permitted error if you have a filter
rule on the server that drops the GRE packet in the OUTPUT
chain.

I think I'm close to solving the problems with my patch, so
I'll post a new version shortly.

Regards,
Phil

-- 
Philip Craig     Software Engineer     http://www.SnapGear.com
philipc@snapgear.com  Ph: +61 7 3435 2821  Fx: +61 7 3891 3630
SnapGear  -  Custom Embedded Solutions and Security Appliances

Re: PPTP connection tracking

[Philip Craig]

Ilguiz Latypov wrote:
> I changed pptpctrl.c to issue pseudo-unique Call ID's for every incoming
> TCP connection.  Each connection spawns a process.
> 
> Is this modification reasonable?  I will submit it to pptpd maintainer as
> well.

I came across this problem in my testing as well.  Assigning
a random ID is not really correct, but it's the easiest thing
to do to get it working.  It's better than always using 0 at
least.

Regards,
Phil

-- 
Philip Craig     Software Engineer     http://www.SnapGear.com
philipc@snapgear.com  Ph: +61 7 3435 2821  Fx: +61 7 3891 3630
SnapGear  -  Custom Embedded Solutions and Security Appliances

PPTP connection tracking

[Paul Mielke]

Greetings netfilter-devel denizens,

I have taken the PPTP connection tracking patch version 1.11
and applied it to the 2.4.20 kernel's version of netfilter. 
I then applied Philip Craig's subsequent patch to get his
fixes.  I would like to express my appreciation for the
excellent work done by Philip, Harald and the other folks
who have contributed to the PPTP/GRE connection tracking
implementation.

In the course of getting this to work on my target systems,
it became clear that our situation differs from those of
the previous developers in the following respects:

1)  In our case, we need to support the PPTP server running
on the firewall machine itself, as well as the pass-through
cases.

2)  At least one of our platforms is big-endian.

The change required to make 1) work is pretty straightforward.
Case 2) is a bit uglier.

The thing that prevented case 1) from working is the following
fragment in the routine ip_nat_fn() in ip_nat_standalone.c starting
around line 111:

         case IP_CT_NEW:
#ifdef CONFIG_IP_NF_NAT_LOCAL
                 /* LOCAL_IN hook doesn't have a chain and thus doesn't care
                  * about new packets -HW */
                 if (hooknum == NF_IP_LOCAL_IN)
                         return NF_ACCEPT;
#endif

The result of this code in the CONFIG_IP_NF_NAT_LOCAL case is that
the rest of the NAT initialization code gets skipped for a LOCAL_IN
packet.  This means that one direction of the manipulations doesn't
get set up.  The way I think about it is that hook NF_IP_LOCAL_IN
is the equivalent of NF_POST_ROUTING for an INPUT packet.  The
DST manip gets initialized in NF_PRE_ROUTING, but if you skip
this code for LOCAL_IN then the SRC manip doesn't get initialized
on the first inbound packet.  By the time the reply is sent, the
connection entry is already hashed and it's too late to mess with
it.  My solution was just to turn the

#ifdef CONFIG_IP_NF_NAT_LOCAL

into

#if 0

thus removing this section of code.  Things seem to work fine after
this change, but perhaps there are other cases that I don't understand
in which this change would break things.  Harald, do you see any harm
in this solution?

The endianness issue is a good deal uglier.  The root cause of the
problem is that the PPTP conntrack patch adds a 32 bit element to
the union ip_conntrack_manip_proto and also to the corresponding
destination portion of ip_conntrack_tuple, making them both a mix
of 16 bit and 32 bit elements.  There are a number of places in the
netfilter code that use structure initializers to set a value for 
one of these structures and it now does the wrong thing on a big-
endian machine.  In fact, I claim it also does a more subtly
incorrect thing on a little-endian machine.  Consider the following
example from ftp_nat_expected() in ip_nat_ftp.c:

         /* ... unless we're doing a MANIP_DST, in which case, make
            sure we map to the correct port */
         if (HOOK2MANIP(hooknum) == IP_NAT_MANIP_DST) {
                 mr.range[0].flags |= IP_NAT_RANGE_PROTO_SPECIFIED;
                 mr.range[0].min = mr.range[0].max
                         = ((union ip_conntrack_manip_proto)
                                 { htons(exp_ftp_info->port) });

The structure "mr" is (struct ip_nat_multi_range) and is declared
on the stack and never explicitly cleared.  The code generated by
the compiler we're using (gcc 2.95.2) does a 16 bit store word into
the address of the min and max elements.  This puts the data in
the right place on an LE cpu, but in the wrong place on a BE cpu.
On an LE cpu, the upper 16 bits of the union remain unitialized,
which may also cause problems depending on how the union is referenced
later.

Unfortunately this idiom exists in a number of places.  I'm in
the process of looking for more, but I have already found and fixed
two bugs caused by this endianness issue.  Diffs are attached below.

Comments?

Regards,
Paul Mielke
BroadOn Communications Corp.

diff -u netfilter-2.4.20_philpatch/net/ipv4/netfilter/ip_nat_ftp.c netfilter_pptfixes/net/ipv4/netfilter/ip_nat_ftp.c
--- netfilter-2.4.20_philpatch/net/ipv4/netfilter/ip_nat_ftp.c	2002-11-28 18:53:15.000000000 -0500
+++ netfilter_pptp_fixes/net/ipv4/netfilter/ip_nat_ftp.c	2003-01-17 16:03:30.000000000 -0500
@@ -47,6 +47,7 @@
  
  	DEBUGP("nat_expected: We have a connection!\n");
  	exp_ftp_info = &ct->master->help.exp_ftp_info;
+	memset((void *)&mr, 0, sizeof (struct ip_nat_multi_range));
  
  	LOCK_BH(&ip_ftp_lock);
  
@@ -82,9 +83,8 @@
  	   sure we map to the correct port */
  	if (HOOK2MANIP(hooknum) == IP_NAT_MANIP_DST) {
  		mr.range[0].flags |= IP_NAT_RANGE_PROTO_SPECIFIED;
-		mr.range[0].min = mr.range[0].max
-			= ((union ip_conntrack_manip_proto)
-				{ htons(exp_ftp_info->port) });
+		mr.range[0].min.tcp.port = mr.range[0].max.tcp.port =
+			htons(exp_ftp_info->port);
  	}
  	return ip_nat_setup_info(ct, &mr, hooknum);
  }
@@ -182,6 +182,7 @@
  	DEBUGP("FTP_NAT: seq %u + %u in %u\n",
  	       expect->seq, ct_ftp_info->len,
  	       ntohl(tcph->seq));
+	memset((void *)&newtuple, 0, sizeof(struct ip_conntrack_tuple));
  
  	/* Change address inside packet to match way we're mapping
  	   this connection. */
diff -u netfilter-2.4.20_philpatch/net/ipv4/netfilter/ip_conntrack_core.c netfilter_pptp_fixes/net/ipv4/netfilter/ip_conntrack_core.c
--- netfilter-2.4.20_philpatch/net/ipv4/netfilter/ip_conntrack_core.c	2002-12-08 23:12:10.000000000 -0500
+++ netfilter_pptp_fixes/net/ipv4/netfilter/ip_conntrack_core.c	2003-01-17 16:09:21.000000000 -0500
@@ -1302,9 +1302,14 @@
  getorigdst(struct sock *sk, int optval, void *user, int *len)
  {
  	struct ip_conntrack_tuple_hash *h;
-	struct ip_conntrack_tuple tuple = { { sk->rcv_saddr, { sk->sport } },
-					    { sk->daddr, { sk->dport },
-					      IPPROTO_TCP } };
+	struct ip_conntrack_tuple tuple;
+
+	memset((void *)&tuple, 0, sizeof (struct ip_conntrack_tuple));
+	tuple.src.ip = sk->rcv_saddr;
+	tuple.src.u.tcp.port = sk->sport;
+	tuple.dst.ip = sk->daddr;
+	tuple.dst.u.tcp.port = sk->dport;
+	tuple.dst.protonum = IPPROTO_TCP;
  
  	/* We only do TCP at the moment: is there a better way? */
  	if (strcmp(sk->prot->name, "TCP") != 0) {

Paul Mielke                        paulm@routefree.com
RouteFree, Inc.                   (650) 739-5377

Re: PPTP connection tracking

[Harald Welte]

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

On Sun, Jan 19, 2003 at 03:01:09PM -0800, Paul Mielke wrote:

> The thing that prevented case 1) from working is the following
> fragment in the routine ip_nat_fn() in ip_nat_standalone.c starting
> around line 111:
> 
>          case IP_CT_NEW:
> #ifdef CONFIG_IP_NF_NAT_LOCAL
>                  /* LOCAL_IN hook doesn't have a chain and thus doesn't care
>                   * about new packets -HW */
>                  if (hooknum == NF_IP_LOCAL_IN)
>                          return NF_ACCEPT;
> #endif

you are definitely correct, althought your solution is problematic.  In
the case NAT_LOCAL is enabled, we are still not allowed to call
ip_nat_rule_find, because this in turn calls ipt_do_table()... but
there is no chain attached to the LOCAL_IN hook of the 'nat' table, and
bad things start to happen.

I have put a proposed patch into CVS:
(patch-o-matic/pending/10_local-nat-expectfn.patch)

This patch rather moves the particular piece of code a couple of lines
down, so we assure that an expectfn (if it exists) gets called.

Can you please test if your setup works with this patch?

> Paul Mielke                        paulm@routefree.com
> RouteFree, Inc.                   (650) 739-5377

-- 
- Harald Welte <laforge@netfilter.org>             http://www.netfilter.org/
============================================================================
  "Fragmentation is like classful addressing -- an interesting early
   architectural error that shows how much experimentation was going
   on while IP was being designed."                    -- Paul Vixie

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

Re: PPTP connection tracking

[Philip Craig]

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

Hi Harald,

Harald Welte wrote:
> I have put a proposed patch into CVS:
> (patch-o-matic/pending/10_local-nat-expectfn.patch)
> 
> This patch rather moves the particular piece of code a couple of lines
> down, so we assure that an expectfn (if it exists) gets called.
> 
> Can you please test if your setup works with this patch?

Paul and I have tested that the attached patch fixes the problem.
It is similar to yours, except that it returns earlier.  I don't
think the calls to place_in_hashes() and do_bindings() are
necessary if we just need to return NF_ACCEPT without doing any
NAT?

Regards,

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

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

diff -u -r1.3 ip_nat_standalone.c
--- linux-2.4.x/net/ipv4/netfilter/ip_nat_standalone.c	9 Dec 2002 15:18:06 -0000	1.3
+++ linux-2.4.x/net/ipv4/netfilter/ip_nat_standalone.c	21 Jan 2003 08:20:45 -0000
@@ -109,12 +109,6 @@
 		}
 		/* Fall thru... (Only ICMPs can be IP_CT_IS_REPLY) */
 	case IP_CT_NEW:
-#ifdef CONFIG_IP_NF_NAT_LOCAL
-		/* LOCAL_IN hook doesn't have a chain and thus doesn't care
-		 * about new packets -HW */
-		if (hooknum == NF_IP_LOCAL_IN)
-			return NF_ACCEPT;
-#endif
 		info = &ct->nat.info;
 
 		WRITE_LOCK(&ip_nat_lock);
@@ -130,6 +124,14 @@
 				ret = call_expect(master_ct(ct), pskb, 
 						  hooknum, ct, info);
 			} else {
+#ifdef CONFIG_IP_NF_NAT_LOCAL
+				/* LOCAL_IN hook doesn't have a chain and thus
+				 * doesn't care about new packets -HW */
+				if (hooknum == NF_IP_LOCAL_IN) {
+					WRITE_UNLOCK(&ip_nat_lock);
+					return NF_ACCEPT;
+				}
+#endif
 				ret = ip_nat_rule_find(pskb, hooknum, in, out,
 						       ct, info);
 			}

Re: PPTP connection tracking

[Harald Welte]

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

On Tue, Feb 18, 2003 at 10:50:51AM +1000, Philip Craig wrote:
> >Can you please test if your setup works with this patch?
> 
> Paul and I have tested that the attached patch fixes the problem.
> It is similar to yours, except that it returns earlier.  I don't
> think the calls to place_in_hashes() and do_bindings() are
> necessary if we just need to return NF_ACCEPT without doing any
> NAT?

yes, but  I can imagine the case where somebody actually _does_ want to
do some nat from the expectfn() - and we should keep that option open...

> Philip Craig - philipc@snapgear.com - http://www.SnapGear.com

-- 
- Harald Welte <laforge@netfilter.org>             http://www.netfilter.org/
============================================================================
  "Fragmentation is like classful addressing -- an interesting early
   architectural error that shows how much experimentation was going
   on while IP was being designed."                    -- Paul Vixie

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

Re: PPTP connection tracking

[Philip Craig]

Harald Welte wrote:
> On Tue, Feb 18, 2003 at 10:50:51AM +1000, Philip Craig wrote:
> 
>>>Can you please test if your setup works with this patch?
>>
>>Paul and I have tested that the attached patch fixes the problem.
>>It is similar to yours, except that it returns earlier.  I don't
>>think the calls to place_in_hashes() and do_bindings() are
>>necessary if we just need to return NF_ACCEPT without doing any
>>NAT?
> 
> 
> yes, but  I can imagine the case where somebody actually _does_ want to
> do some nat from the expectfn() - and we should keep that option open...

That is exactly the problem this patch is solving.  We want
to do NAT in expectfn() for PPTP, but the #ifdef for local NAT
was too early and expectfn() wasn't being called at all.  So this
patch moves the #ifdef into a code path that isn't followed if
expectfn() needs to be called.

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