PPTP connection tracking fixes

PPTP connection tracking fixes

[paulm]

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) {

Re: PPTP connection tracking fixes

[Philip Craig]

paulm@routefree.com wrote:
> 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.

Note that Harald has incorporated my patch in CVS with a
couple of small changes.

> 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.

Actually both of these are true for me.

> 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

My PPTP server never received the incoming packet first in my
testing.  Adding a rule to drop the outgoing packet causes the
problem you describe to occur, and removing the #ifdef fixes it.
I haven't looked into the code much to see what this is going
to break yet..

> 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.
<snip>
> 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.

I came across this also in the past week.  The only additional
occurences I have found are a few places in the h323 patch.

-- 
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 fixes

[Philip Craig]

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

paulm@routefree.com wrote:
> 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?

If you do this and turn on CONFIG_NETFILTER_DEBUG, you get:

IP_NF_ASSERT: ipt_do_table:ip_tables.c:290

which is a result of ipt_do_table being called for the LOCAL_IN
hook in the nat table.

I've attached a patch that moves the #ifdef down a bit, so that
bindings can still be set up for expectations, but otherwise
we skip doing ip_nat_rule_find() for the LOCAL_IN hook.

-- 
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: 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);
 			}

another PPTP conntrack problem (no fix this time)

[Paul Mielke]

Hi, Philip.

Thanks for following up on the issues I mentioned in my recent message (BTW there
seems to be something funny happening to my messages to netfilter-devel, as they
seem to appear there either late or never).

I have noticed one more serious problem with the PPTP connection tracking
implementation, but I do not yet have a fix to propose:

The support for generating an ICMP response to an error on a GRE packet is hopelessly
broken.  We ran into this in the course of testing two layers of nested PPTP
connections.  In our particular error case, the second layer of PPTP (only from
Windows XP for some reason) generates a packet with DF set that is too large
by the time the two layers of encapsulation get added.  When the ICMP response
is generated, the rejected packet has already been through NAT.  When the logic
in icmp_error_track attempts to look up the connection in order to NAT the header
of the dropped packet that is buried inside the ICMP response, it is unable to find
the GRE connection tuple.  As a result, it sends the encapsulated header of
the dropped packet back in its un-NATted form.  The original sender who needs to
adjust his MTU can't map the packet to a connection, because the dropped header
is untranslated.  As a result, the connection just hangs and no further traffic gets
through.

Suggestions?  We are working on a fix, but the only idea I have had so far is
pretty distasteful (put a special purpose check for GRE and call a routine that
does a brute force search through the GRE keymap hash looking for a match to the
callid).  It's not even clear this will work.  Failing that, we'd need another hash that
indexes the keymaps in the "inverse" direction, if you see what I mean.

Regards,
Paul

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

Re: another PPTP conntrack problem (no fix this time)

[Philip Craig]

Paul Mielke wrote:
> The support for generating an ICMP response to an error on a GRE packet is hopelessly
> broken.  We ran into this in the course of testing two layers of nested PPTP
> connections.  In our particular error case, the second layer of PPTP (only from
> Windows XP for some reason) generates a packet with DF set that is too large
> by the time the two layers of encapsulation get added.  When the ICMP response
> is generated, the rejected packet has already been through NAT.  When the logic
> in icmp_error_track attempts to look up the connection in order to NAT the header
> of the dropped packet that is buried inside the ICMP response, it is unable to find
> the GRE connection tuple.  As a result, it sends the encapsulated header of
> the dropped packet back in its un-NATted form.  The original sender who needs to
> adjust his MTU can't map the packet to a connection, because the dropped header
> is untranslated.  As a result, the connection just hangs and no further traffic gets
> through.
 >
> Suggestions?  We are working on a fix, but the only idea I have had so far is
> pretty distasteful (put a special purpose check for GRE and call a routine that
> does a brute force search through the GRE keymap hash looking for a match to the
> callid).  It's not even clear this will work.  Failing that, we'd need another hash that
> indexes the keymaps in the "inverse" direction, if you see what I mean.

I'm not entirely clear on your setup.  How many machines are
involved, where are the tunnels, and where is NAT happening?

This sounds like a problem with NAT of ICMP errors, rather than
a PPTP conntrack problem.  There have been at least 2 patches
on the list regarding NAT of the inner ICMP header.  I don't
think either have made it into standard kernels yet.  Have you
tried them?

-- 
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: another PPTP conntrack problem (no fix this time)

[Harald Welte]

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

On Thu, Jan 30, 2003 at 05:34:40PM +1000, Philip Craig wrote:
> This sounds like a problem with NAT of ICMP errors, rather than
> a PPTP conntrack problem.  There have been at least 2 patches
> on the list regarding NAT of the inner ICMP header.  I don't
> think either have made it into standard kernels yet.  Have you
> tried them?

Yes, you are correct. After finishing some final testing one of those
patches will make it to the kernel during the next couple of days.

This bug is IIRC also in bugzilla.netfilter.org

> Philip Craig     Software Engineer     http://www.SnapGear.com
 

-- 
Live long and prosper
- Harald Welte / laforge@gnumonks.org               http://www.gnumonks.org/
============================================================================
GCS/E/IT d- s-: a-- C+++ UL++++$ P+++ L++++$ E--- W- N++ o? K- w--- O- M- 
V-- PS+ PE-- Y+ PGP++ t++ 5-- !X !R tv-- b+++ DI? !D G+ e* h+ r% y+(*)

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

Re: another PPTP conntrack problem (no fix this time)

[Paul Mielke]

At 05:34 PM 1/30/2003 +1000, Philip Craig wrote:
>I'm not entirely clear on your setup.  How many machines are
>involved, where are the tunnels, and where is NAT happening?
>
>This sounds like a problem with NAT of ICMP errors, rather than
>a PPTP conntrack problem.  There have been at least 2 patches
>on the list regarding NAT of the inner ICMP header.  I don't
>think either have made it into standard kernels yet.  Have you
>tried them?

Hi, Philip.

No, I haven't tried the other patches you mention.  I am pretty sure, though,
that this problem is GRE specific.  I'm not sure I'll be able to explain the
setup clearly in writing without resorting to drawing diagrams, but I'll make
a go at it:

The setup involves three machines:

The PPTP client, running Windows XP.

Two PPTP servers, call them Near and Far.  The client is "behind" the Near
server, meaning that Near is his gateway to the outside world and is therefore
doing Source NAT on outbound traffic.

The sequence to produce the bug is:

The client establishes a PPTP session to the Near PPTP server.

Then the client establishes another PPTP session to the Far server, routed
through the PPTP connection to the Near server.  I know this sounds a bit
odd, but it makes sense if I add that the first connection between the client
and Near is a wireless LAN connection over which we run PPTP.

Now the client does a normal TCP connection to Far, e.g. telnet or FTP,
through the "inner" tunnel.  If he does something that provokes Far to send
him a big packet (> 1400 bytes in our instance), then the packet arrives from
Far to Near, gets unNAT'ed, so that the destination is the WLAN address of
Client and the callid is the original callid generated by the Windows XP DUN
client (always either 0, 0x4000, 0x8000 or 0xc000).

Now we hit the crux:  at this point, the packet has only one layer of GRE
encapsulation.  Now the routing code on Near realizes that he has to jam
this packet down the PPP interface of the outer PPTP tunnel in order to get
it back to Client.  But once the second layer of GRE encapsulation is added,
the packet exceeds the MTU of that interface (1400 in our case).  The ICMP
DestUnreach packet is built and the included header of the rejected packet
is still unNATted.  When icmp_error_track is invoked, it tries to reNAT the
packet, but is unable to recognize the connection because when the unNATed
tuple is inverted, it violates the expectations of the gre_keymap hash:

The header of the rejected packet contains the following:

source = real address of Far
dest = real (unNATted) address of Client's end of the outer PPTP tunnel
callid = client's real callid (the one that Far knows is the post-NAT one)

But the gre_keymap hash only contains the following combinations:

source = Far
dest = post-NAT address of Client
callid = post-NAT callid of Client
answer from lookup = callid of Far to create the full GRE 4-tuple

This one is used for looking up incoming packets from Far and finding
the real dest and callid for Client.

source = real address of Client
dest = Far
callid = Far's callid
answer from lookup = real callid of Client to complete 4-tuple

If I have this all right, then the fundamental problem is that there is no
way for the GRE NAT code to recognize the connection, since the gre_keymap
lookup fails.   There is no entry in the gre_keymap hash that matches the
three-tuple in the inner header.

This causes the ICMP packet to go back to Far with the inner header
still containing the real internal address of Client, instead of the external
address of Near.  As a result, Far doesn't map the ICMP DestUnreach
to the connection and ignores it.

Does this seem to make sense?  Please let me know if you think I am
mistaken in my analysis and that the previous ICMP patches would help.

Best regards,
Paul

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

Re: another PPTP conntrack problem (no fix this time)

[Philip Craig]

Paul Mielke wrote:
> Does this seem to make sense?  Please let me know if you think I am
> mistaken in my analysis and that the previous ICMP patches would help.

You've convinced me that there is a problem here.  I think
that gre_pkt_to_tuple() needs to handle the case when
gre_keymap_lookup() fails by trying to do an inverse
lookup.  I haven't tried fixing this yet.

I suspect that you will still need the other ICMP patches
as well once you have the gre lookup succeeding though.

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

[PATCH] Re: another PPTP conntrack problem (no fix this time)

[Philip Craig]

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

Philip Craig wrote:
> Paul Mielke wrote:
> 
>>Does this seem to make sense?  Please let me know if you think I am
>>mistaken in my analysis and that the previous ICMP patches would help.
> 
> 
> You've convinced me that there is a problem here.  I think
> that gre_pkt_to_tuple() needs to handle the case when
> gre_keymap_lookup() fails by trying to do an inverse
> lookup.  I haven't tried fixing this yet.
> 
> I suspect that you will still need the other ICMP patches
> as well once you have the gre lookup succeeding though.
> 

I've attached a patch that Paul has tested that fixes this
problem.  He had to use it in conjunction with an ICMP patch
posted by Balazs Scheidler.

The problem is that there are only gre keymaps for packets
in the state where they first arrive at a machine.  But when
we need to generate an ICMP error, the gre packet may have
been NATed, and so it no longer matches the keymaps.

The attached patch extends gre_keymap_lookup to also compare
against the inverted keymaps.

I'm not happy with this patch, but I don't have the time to
fix it properly at the moment, so I've post it here.  There
are 2 problems with the patch.

Firstly, it is combining two tasks into the one function.
That means that incoming packets are compared against the
inverted keymap, when they shouldn't be.  Similarly, when
generating ICMP errors, the packet is matched against the
original keymap when it only needs to be matched against
inverted one.

Secondly, matching against inverted tuples is not going to
work when we are doing both SNAT and DNAT.  This problem may
not be specific to the gre conntrack either; ip_conntrack_core.c
has the following comment in icmp_error_track():

/* FIXME: NAT code has to handle half-done double NAT --RR */

I think the fix to both of these problems is to add another
pkt_to_tuple function to the ip_conntrack_protocol structure
which converts a half-NATed packet to a tuple.

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

[-- Attachment #2: gre-lookup.patch --]
[-- Type: text/plain, Size: 1301 bytes --]

diff -u -r1.4 ip_conntrack_proto_gre.c
--- ip_conntrack_proto_gre.c	4 Dec 2002 23:44:24 -0000	1.4
+++ ip_conntrack_proto_gre.c	6 Feb 2003 03:38:39 -0000
@@ -79,6 +79,17 @@
 	        (km->tuple.dst.u.all == t->dst.u.all));
 }
 
+static inline int gre_key_inverse_cmpfn(const struct ip_ct_gre_keymap *km,
+					const struct ip_conntrack_tuple *t)
+{
+	return ((km->tuple.dst.ip == t->src.ip) &&
+		(km->tuple.src.ip == t->dst.ip) &&
+	        (km->tuple.dst.protonum == t->dst.protonum) &&
+		(km->tuple.dst.u.gre.protocol == t->dst.u.gre.protocol) &&
+		(km->tuple.dst.u.gre.version == t->dst.u.gre.version) &&
+	        (km->tuple.src.u.gre.key == t->dst.u.gre.key));
+}
+
 /* look up the source key for a given tuple */
 static u_int32_t gre_keymap_lookup(struct ip_conntrack_tuple *t)
 {
@@ -88,12 +99,18 @@
 	READ_LOCK(&ip_ct_gre_lock);
 	km = LIST_FIND(&gre_keymap_list, gre_key_cmpfn,
 			struct ip_ct_gre_keymap *, t);
-	if (!km) {
-		READ_UNLOCK(&ip_ct_gre_lock);
-		return 0;
+	if (km) {
+		key = km->tuple.src.u.gre.key;
+	} else {
+		km = LIST_FIND(&gre_keymap_list, gre_key_inverse_cmpfn,
+				struct ip_ct_gre_keymap *, t);
+		if (km) {
+			key = km->tuple.dst.u.gre.key;
+		} else {
+			key = 0;
+		}
 	}
 
-	key = km->tuple.src.u.gre.key;
 	READ_UNLOCK(&ip_ct_gre_lock);
 
 	return key;