PPTP connection tracking and NAT patches

PPTP connection tracking and NAT patches

[Jeff Hall]

Below are patches which correct problems I encountered while testing
the PPTP conntrack and NAT modules. These patches are to V1.7 of 
ip_conntrack_pptp.c and to the 2.4.20 patch level of ip_nat_core.c with
V1.12 of pptp-conntrack-nat.patch and the NAT ICMP patch (2003/02/26).

The ip_conntrack_pptp.c patch prevents a kernel crash caused by an attempt
to dereference a NULL value. The original code failed when the next link
pointed to the parent ip_conntrack rather than the next ip_conntrack_expect.

The ip_nat_core.c patch ensures that the PPTP helper function is called even
after the expected GRE connection is established. I have not evaluated any
other protocols with helper functions to see if this will cause problems.
Others with more experience in netfilter programming may know. I am sure that
the PPTP NAT helper function needs to be called otherwise both the TCP and
GRE connections fail to disappear when the VPN tunnel is closed.

I also noticed that since the helper function is called for each expectation
both inbound and outbound packets are sent to ip_nat_mangle_tcp_packet twice
until an expected GRE connection is established. However, since this does
not appear do cause any problems and I was not confident of the best way to
prevent the extra manipulation, I did not attempt any patch.

I was also having problems running a PoPToP server on the firewall server.
The reason turned out to be that I had not chosen CONFIG_IP_NF_NAT_LOCAL
thinking that I did not need to NAT my local connections since my local
machines IP was not the same as the IP I am using for NAT. It turns out that
even if a connection does not satisfy any NAT rule the helper function is
called in do_bindings. Without CONFIG_IP_NF_NAT_LOCAL set the helper function
was being called for DST manipulations but not for SRC manipulations. My
question to netfilter gurus is shouldn't the helper function be skipped if
the connection doesn't satisfy any NAT rule?

I did quite a bit of testing of these modules in an environment where we
are making connections using the Windows RAS client from behind a NAT'ing
firewall and also need to run PoPToP on the firewall server for employees
working from home. The modules with these patches never failed. I was even
able to make two connections from behind a NAT'ing wireless access point 
to the firewall PoPToP server (with a patch to PoPToP to generate non-
zero call IDs).




--- linux-2.4.20.prepatch/net/ipv4/netfilter/ip_conntrack_pptp.c	Thu Feb 27 04:02:44 2003
+++ linux-2.4.20/net/ipv4/netfilter/ip_conntrack_pptp.c	Sat Mar 15 22:25:01 2003
@@ -113,12 +113,17 @@
 	/* delete other expectation */
 	if (exp->expected_list.next != &exp->expected_list) {
 		struct ip_conntrack_expect *other_exp;
+		struct list_head *cur_item, *next;
 
-		other_exp = list_entry(exp->expected_list.next,
-				       struct ip_conntrack_expect,
-				       expected_list);
 		DEBUGP("unexpecting other direction\n");
-		ip_conntrack_unexpect_related(other_exp);
+
+		for (cur_item = master->sibling_list.next;
+		    cur_item != &master->sibling_list; cur_item = next) {
+			next = cur_item->next;
+			other_exp = list_entry(cur_item, struct ip_conntrack_expect,
+					 expected_list);
+			if ( other_exp != exp ) ip_conntrack_unexpect_related(other_exp);
+		}
 	}
 
 	return 0;
--- linux-2.4.20.prepatch/net/ipv4/netfilter/ip_nat_core.c	Sat Mar 15 21:42:37 2003
+++ linux-2.4.20/net/ipv4/netfilter/ip_nat_core.c	Sun Mar 16 05:08:49 2003
@@ -798,7 +798,7 @@
 	if (helper) {
 		struct ip_conntrack_expect *exp = NULL;
 		struct list_head *cur_item;
-		int ret = NF_ACCEPT;
+		int ret = NF_ACCEPT, helper_called = 0;
 
 		DEBUGP("do_bindings: helper existing for (%p)\n", ct);
 
@@ -826,10 +826,11 @@
 					READ_UNLOCK(&ip_conntrack_lock);
 					return ret;
 				}
+				helper_called = 1;
 			}
 		}
-		/* Helper might want to manip the packet even when there is no expectation */
-		if (!exp && helper->flags & IP_NAT_HELPER_F_ALWAYS) {
+		/* Helper might want to manip the packet even when there is no expectation - or if the expectations have been established */
+		if (!helper_called && helper->flags & IP_NAT_HELPER_F_ALWAYS) {
 			DEBUGP("calling nat helper for packet without expectation\n");
 			ret = helper->help(ct, NULL, info, ctinfo, 
 					   hooknum, pskb);

Re: PPTP connection tracking and NAT patches

[Harald Welte]

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

On Thu, Mar 20, 2003 at 03:32:46AM -0500, Jeff Hall wrote:
> Below are patches which correct problems I encountered while testing
> the PPTP conntrack and NAT modules. 

Thanks for sharing your experience with the pptp helper.

> The ip_conntrack_pptp.c patch prevents a kernel crash caused by an attempt
> to dereference a NULL value. The original code failed when the next link
> pointed to the parent ip_conntrack rather than the next ip_conntrack_expect.

Ok, I see.  This indeed is a mistake and I will include your patch.

> The ip_nat_core.c patch ensures that the PPTP helper function is called even
> after the expected GRE connection is established. I have not evaluated any
> other protocols with helper functions to see if this will cause problems.

Again agreed.  This change is necessarry.

> I also noticed that since the helper function is called for each expectation
> both inbound and outbound packets are sent to ip_nat_mangle_tcp_packet twice
> until an expected GRE connection is established. However, since this does
> not appear do cause any problems and I was not confident of the best way to
> prevent the extra manipulation, I did not attempt any patch.

yes, there is some room for optimization... but I don't think it's worth
the effort.

> I was also having problems running a PoPToP server on the firewall server.
> The reason turned out to be that I had not chosen CONFIG_IP_NF_NAT_LOCAL
> thinking that I did not need to NAT my local connections since my local
> machines IP was not the same as the IP I am using for NAT. It turns out that
> even if a connection does not satisfy any NAT rule the helper function is
> called in do_bindings. Without CONFIG_IP_NF_NAT_LOCAL set the helper function
> was being called for DST manipulations but not for SRC manipulations. My
> question to netfilter gurus is shouldn't the helper function be skipped if
> the connection doesn't satisfy any NAT rule?

Mh.  If we don't have any nat mappings, we shouldn't call the helper.

> I did quite a bit of testing of these modules in an environment where we
> are making connections using the Windows RAS client from behind a NAT'ing
> firewall and also need to run PoPToP on the firewall server for employees
> working from home. The modules with these patches never failed. I was even
> able to make two connections from behind a NAT'ing wireless access point 
> to the firewall PoPToP server (with a patch to PoPToP to generate non-
> zero call IDs).

great.

-- 
- 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 and NAT patches

[Philip Craig]

Harald Welte wrote:
> On Thu, Mar 20, 2003 at 03:32:46AM -0500, Jeff Hall wrote:
>>I was also having problems running a PoPToP server on the firewall server.
>>The reason turned out to be that I had not chosen CONFIG_IP_NF_NAT_LOCAL
>>thinking that I did not need to NAT my local connections since my local
>>machines IP was not the same as the IP I am using for NAT. It turns out that
>>even if a connection does not satisfy any NAT rule the helper function is
>>called in do_bindings. Without CONFIG_IP_NF_NAT_LOCAL set the helper function
>>was being called for DST manipulations but not for SRC manipulations. My
>>question to netfilter gurus is shouldn't the helper function be skipped if
>>the connection doesn't satisfy any NAT rule?
> 
> 
> Mh.  If we don't have any nat mappings, we shouldn't call the helper.

We use the TCP source port of the control channel for the call ID when
NATting.  For this to work, we have to NAT both forwarded and local
connections, otherwise there is a possibility of a call ID clash.
Since the original call ID is usually different from the TCP source
port, there will be a nat mapping for local connections, and the
helper needs to be called.  If we get reservation of call IDs, then
this behaviour can be changed.

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

Re: PPTP connection tracking and NAT patches

[Harald Welte]

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

On Thu, Mar 27, 2003 at 10:16:40AM +1000, Philip Craig wrote:
> >Mh.  If we don't have any nat mappings, we shouldn't call the helper.
> 
> We use the TCP source port of the control channel for the call ID when
> NATting.  For this to work, we have to NAT both forwarded and local
> connections, otherwise there is a possibility of a call ID clash.

This is true.  But I don't think we should make this automatic.  The
only case where this clash actually could happen is when both NAT'ed and
local connections use the same source IP.  

We already have other difficulties in this case, see the 'locally bound
UDP port can still be used for NAT mappings' discussion.

If somebody wants to NAT PPTP sessions (local and remote), he should
insert a SNAT rule in POSTROUTING which then matches local and remote
connections.

> Since the original call ID is usually different from the TCP source
> port, there will be a nat mapping for local connections, and the
> helper needs to be called.  If we get reservation of call IDs, then
> this behaviour can be changed.

yes, in the end we whould have some mechanism for callID reservation,
even if it is just a simple 16bit counter that is incremented every time
we need to use a CallID and wraps around after 16bit are expired.

I don't think it's worth the effort doing a 'real' allocation (including
a 'free') which would be per-IP.  Nobody will want to do 2^16 PPTP
sessions from a single IP... and even if he would, there are other
problems ;)

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

-- 
- 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 and NAT patches

[Harald Welte]

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

On Thu, Mar 20, 2003 at 03:32:46AM -0500, Jeff Hall wrote:
> +			if ( other_exp != exp )

I've changed this into 
			if (other_exp != exp && other_exp->seq == exp->seq)

to make sure we really delete only the corresponding expectation in the
other direction.  At least theoretically we can have more than one call
in a single session...

Can you please try (+test) ip_conntrack_pptp.c version 1.8 from
netfilter-extensions CVS in combination with the
'pending/25_natcore_nohelper.patch' of patch-o-matic?

Thanks.

-- 
- 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 and NAT patches

[Jeff Hall]

Harald:

In testing these patches I had problems with unreplied connections
being created. Solving that problem let me to the two patches below.
The first corrects a housekeeping oversight. The second is to prevent
execution of a helper on an expected connection if the master connection
has no manips.

In response to a message from Phillip Craig about NAT'ing both local and
remote PPTP sessions you suggested a postrouting rule to match both
local and remote sessions. I was going to try testing that scenario but
found that while I could create a rule to match both types of connections
the code in the PPTP NAT helper functions would not get called unless the
NAT rule actually modified the packet (i.e. num_manips !0). I thought the
initialized field might provide an answer but I believe that initialized
will be non-zero when a connection doesn't match a rule but the chain 
policy is accept. If all this is true, then this might have wider impli-
cations to protocols other than PPTP and so I thought it worth mentioning.

Anyways, with the two patches below I was able to successfully make 
PPTP connections through and to a machine.

Jeff Hall


On Wed, 26 Mar 2003, Harald Welte wrote:

> On Thu, Mar 20, 2003 at 03:32:46AM -0500, Jeff Hall wrote:
> > +			if ( other_exp != exp )
> 
> I've changed this into 
> 			if (other_exp != exp && other_exp->seq == exp->seq)
> 
> to make sure we really delete only the corresponding expectation in the
> other direction.  At least theoretically we can have more than one call
> in a single session...
> 
> Can you please try (+test) ip_conntrack_pptp.c version 1.8 from
> netfilter-extensions CVS in combination with the
> 'pending/25_natcore_nohelper.patch' of patch-o-matic?
> 
> Thanks.
> 
> -- 
> - 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
> 

--- ip_conntrack_pptp.v1_8      Tue Apr  8 06:02:29 2003
+++ ip_conntrack_pptp.c Thu Apr  3 03:56:59 2003
@@ -124,6 +124,7 @@
 			/* remove only if occurred at same sequence number */
 			if (other_exp != exp && other_exp->seq == exp->seq) {
 				DEBUGP("unexpecting other direction\n");
+				ip_ct_gre_keymap_destroy(other_exp);
 				ip_conntrack_unexpect_related(other_exp);
 			}
 		}

--- ip_nat_standalone.c.prepatch        Sun Mar 16 01:41:45 2003
+++ ip_nat_standalone.c Thu Apr  3 19:52:09 2003
@@ -121,8 +121,13 @@
 			if (ct->master
 			    && master_ct(ct)->nat.info.helper
 			    && master_ct(ct)->nat.info.helper->expect) {
-				ret = call_expect(master_ct(ct), pskb, 
+				if(master_ct(ct)->nat.info.num_manips) {
+					ret = call_expect(master_ct(ct), pskb, 
 						hooknum, ct, info);
+				} else {
+					WRITE_UNLOCK(&ip_nat_lock);
+					return NF_ACCEPT;
+				}
 			} else {
 #ifdef CONFIG_IP_NF_NAT_LOCAL
 				/* LOCAL_IN hook doesn't have a chain and thus

PPTP connection tracking and NAT patches

[Jeff Hall]

The explanation I gave for the problem running the PoPToP server in my
previous message was incorrect. What I should have said was that without
CONFIG_IP_NF_NAT_LOCAL there is no LOCAL_IN hook, and pptp_nat_expected
is not called to setup the SRC manip in the original direction (no manip
is required) AND the DST manip in the reply direction. The missing DST
manip in the reply direction broke the connection.