Re: Support NAT-ed expect entries from user space

Re: Support NAT-ed expect entries from user space

[Pablo Neira Ayuso]

BORBELY Zoltan wrote:
> Hi,
> 
> I'm developing a transparent user space ftp proxy, and I'm using expect
> entries to accept incoming data connections. It works quite well, but I
> had to patch the kernel to get it work. It's quite small, so I put it
> inline:

Just a wild thought. What if you create a new conntrack (instead of an
expectation) using a small timeout in state TCP SYN_SENT that represents
the new flow that it is expected to arrive? The first packet would seen
as a resent by the connection tracking so it would accept it. However,
the TCP sequence tracking may complain about it. Nevertheles, we can
also access to the internal TCP flags to enable   IP_CT_TCP_FLAG_BE_LIBERAL.

> --- nf_conntrack_netlink-orig-2.6.22.9.c        2007-09-26 20:03:01.000000000 +0
> +++ nf_conntrack_netlink-new-2.6.22.9.c 2008-06-13 17:14:17.000000000 +0200
> @@ -39,6 +39,7 @@
>  #ifdef CONFIG_NF_NAT_NEEDED
>  #include <net/netfilter/nf_nat_core.h>
>  #include <net/netfilter/nf_nat_protocol.h>
> +#include <net/netfilter/nf_nat_helper.h>
>  #endif
> 
>  #include <linux/netfilter/nfnetlink.h>
> @@ -1439,7 +1440,11 @@
>                 goto out;
>         }
> 
> -       exp->expectfn = NULL;
> +       exp->expectfn = nf_nat_follow_master;
> +#ifdef CONFIG_NF_NAT_NEEDED
> +       exp->saved_proto.tcp.port = tuple.dst.u.tcp.port;       //!!!FIXME
> +       exp->dir = IP_CT_DIR_ORIGINAL;
> +#endif
>         exp->flags = 0;
>         exp->master = ct;
>         exp->helper = NULL;
> 
> It's ugly as hell in the current form, but I have some ideas how to
> improve it to integrate it into the main kernel tree. Maybe we can add a
> new CTA_EXPECT_SAVED_PROTO attribute and get it from user space. What
> is your opinion?

Would this change be generic enough for userspace transparent proxies or
only for FTP?

-- 
"Los honestos son inadaptados sociales" -- Les Luthiers

Re: Support NAT-ed expect entries from user space

[Patrick McHardy]

Pablo Neira Ayuso wrote:
> BORBELY Zoltan wrote:
>> Hi,
>>
>> I'm developing a transparent user space ftp proxy, and I'm using expect
>> entries to accept incoming data connections.

Sounds reasonable.

>> It works quite well, but I
>> had to patch the kernel to get it work. It's quite small, so I put it
>> inline:
> 
> Just a wild thought. What if you create a new conntrack (instead of an
> expectation) using a small timeout in state TCP SYN_SENT that represents
> the new flow that it is expected to arrive? The first packet would seen
> as a resent by the connection tracking so it would accept it. However,
> the TCP sequence tracking may complain about it. Nevertheles, we can
> also access to the internal TCP flags to enable   IP_CT_TCP_FLAG_BE_LIBERAL.

That sounds like a bit of a hack.

>> --- nf_conntrack_netlink-orig-2.6.22.9.c        2007-09-26 20:03:01.000000000 +0
>> +++ nf_conntrack_netlink-new-2.6.22.9.c 2008-06-13 17:14:17.000000000 +0200
>> @@ -39,6 +39,7 @@
>>  #ifdef CONFIG_NF_NAT_NEEDED
>>  #include <net/netfilter/nf_nat_core.h>
>>  #include <net/netfilter/nf_nat_protocol.h>
>> +#include <net/netfilter/nf_nat_helper.h>
>>  #endif
>>
>>  #include <linux/netfilter/nfnetlink.h>
>> @@ -1439,7 +1440,11 @@
>>                 goto out;
>>         }
>>
>> -       exp->expectfn = NULL;
>> +       exp->expectfn = nf_nat_follow_master;
>> +#ifdef CONFIG_NF_NAT_NEEDED
>> +       exp->saved_proto.tcp.port = tuple.dst.u.tcp.port;       //!!!FIXME
>> +       exp->dir = IP_CT_DIR_ORIGINAL;
>> +#endif
>>         exp->flags = 0;
>>         exp->master = ct;
>>         exp->helper = NULL;
>>
>> It's ugly as hell in the current form, but I have some ideas how to
>> improve it to integrate it into the main kernel tree. Maybe we can add a
>> new CTA_EXPECT_SAVED_PROTO attribute and get it from user space. What
>> is your opinion?
> 
> Would this change be generic enough for userspace transparent proxies or
> only for FTP?

I'm wondering, how is this expectation creation working at all?
The NULL expectfn makes me think it will crash as soon as the
expectation arrives. This *needs* support from the helpers to
properly set the expectfn.

And more specific to this problem: back when Harald was working
on userspace helpers, the idea was to add a dummy helper specifically
so we have one to assign to the connection. The helper would (IIRC)
just queue the expected packets and userspace could take it from
there. Of course queuing could be made optional and (f.i.) it could
just use nf_nat_follow_master.

And related to this patch: the direction needs to be provided
by userspace to be generically useful. The saved_port (and saved_ip
possibly) could either be provided by userspace or selected
based on a couple of flags.

Re: Support NAT-ed expect entries from user space

[BORBELY Zoltan]

Hi,

On Mon, Jun 16, 2008 at 10:10:46PM +0200, Pablo Neira Ayuso wrote:
> BORBELY Zoltan wrote:
> > Hi,
> > 
> > I'm developing a transparent user space ftp proxy, and I'm using expect
> > entries to accept incoming data connections. It works quite well, but I
> > had to patch the kernel to get it work. It's quite small, so I put it
> > inline:
> 
> Just a wild thought. What if you create a new conntrack (instead of an
> expectation) using a small timeout in state TCP SYN_SENT that represents
> the new flow that it is expected to arrive? The first packet would seen
> as a resent by the connection tracking so it would accept it. However,
> the TCP sequence tracking may complain about it. Nevertheles, we can
> also access to the internal TCP flags to enable   IP_CT_TCP_FLAG_BE_LIBERAL.

Just a quick thought, but I think it won't work, because we don't know
the port of the incoming packet. This is why I had to use expect entries.

> > --- nf_conntrack_netlink-orig-2.6.22.9.c        2007-09-26 20:03:01.000000000 +0
> > +++ nf_conntrack_netlink-new-2.6.22.9.c 2008-06-13 17:14:17.000000000 +0200
> > @@ -39,6 +39,7 @@
> >  #ifdef CONFIG_NF_NAT_NEEDED
> >  #include <net/netfilter/nf_nat_core.h>
> >  #include <net/netfilter/nf_nat_protocol.h>
> > +#include <net/netfilter/nf_nat_helper.h>
> >  #endif
> > 
> >  #include <linux/netfilter/nfnetlink.h>
> > @@ -1439,7 +1440,11 @@
> >                 goto out;
> >         }
> > 
> > -       exp->expectfn = NULL;
> > +       exp->expectfn = nf_nat_follow_master;
> > +#ifdef CONFIG_NF_NAT_NEEDED
> > +       exp->saved_proto.tcp.port = tuple.dst.u.tcp.port;       //!!!FIXME
> > +       exp->dir = IP_CT_DIR_ORIGINAL;
> > +#endif
> >         exp->flags = 0;
> >         exp->master = ct;
> >         exp->helper = NULL;
> > 
> > It's ugly as hell in the current form, but I have some ideas how to
> > improve it to integrate it into the main kernel tree. Maybe we can add a
> > new CTA_EXPECT_SAVED_PROTO attribute and get it from user space. What
> > is your opinion?
> 
> Would this change be generic enough for userspace transparent proxies or
> only for FTP?

User space transparent proxies need the followings:
* determine the real destination address of the redirected connection
  (works great with nfct_query(NFCT_Q_GET))
* forge the source address of an outgoing connection
  (works great with nfct_query(NFCT_Q_CREATE))
* allocate a random port and listen on it: this is the part that doesn't
  work out of the box, and this patch helps to solve it.
With these, you can do everything from user space :-)

How to listen on a non-local random address and port?
1. allocate a socket and listen on it
2. set the helper of the master conntrack (you know, it's forbidden to
   create expect entries without a conntrack helper, so I created a dummy
   helper module which does nothing and I assign that to the master
   conntrack)
3. create an expectation

Without my patch, the destination address must be local. With this very
small patch, the connection will be redirected to the local socket
perfectly. As I said the patch is ugly, but it's a proof of concept
what I'd like to do :-)

Bye,
Bozo

Re: Support NAT-ed expect entries from user space

[BORBELY Zoltan]

Hi,

On Mon, Jun 16, 2008 at 10:52:28PM +0200, Patrick McHardy wrote:
> I'm wondering, how is this expectation creation working at all?
> The NULL expectfn makes me think it will crash as soon as the
> expectation arrives. This *needs* support from the helpers to
> properly set the expectfn.

The nf_nat_follow_master did the trick for me if I set the expectation
entry from user space. With NULL expectfn it didn't work.

> And more specific to this problem: back when Harald was working
> on userspace helpers, the idea was to add a dummy helper specifically
> so we have one to assign to the connection. The helper would (IIRC)
> just queue the expected packets and userspace could take it from
> there. Of course queuing could be made optional and (f.i.) it could
> just use nf_nat_follow_master.

I'd like to create a cross-platform user space ftp proxy, not a nf
conntrack+nat helper module, so my goals are a bit different. The
netfilter code contains everything I need, and the netlink interface
is quite good to instruct the kernel code to do as the proxy wants.

> And related to this patch: the direction needs to be provided
> by userspace to be generically useful. The saved_port (and saved_ip
> possibly) could either be provided by userspace or selected
> based on a couple of flags.

Yes, it's a good idea to send these parameters as attributes to the
expect creation netlink message.

Bye,
Bozo

Re: Support NAT-ed expect entries from user space

[Patrick McHardy]

BORBELY Zoltan wrote:
> Hi,
>
> On Mon, Jun 16, 2008 at 10:52:28PM +0200, Patrick McHardy wrote:
>   
>> I'm wondering, how is this expectation creation working at all?
>> The NULL expectfn makes me think it will crash as soon as the
>> expectation arrives. This *needs* support from the helpers to
>> properly set the expectfn.
>>     
>
> The nf_nat_follow_master did the trick for me if I set the expectation
> entry from user space. With NULL expectfn it didn't work.
>   

Yes, so the kernel is broken.
>   
>> And more specific to this problem: back when Harald was working
>> on userspace helpers, the idea was to add a dummy helper specifically
>> so we have one to assign to the connection. The helper would (IIRC)
>> just queue the expected packets and userspace could take it from
>> there. Of course queuing could be made optional and (f.i.) it could
>> just use nf_nat_follow_master.
>>     
>
> I'd like to create a cross-platform user space ftp proxy, not a nf
> conntrack+nat helper module, so my goals are a bit different. The
> netfilter code contains everything I need, and the netlink interface
> is quite good to instruct the kernel code to do as the proxy wants.
>   

I understand that, the expectation part looks like a subset of what
a helper module does though, with the only differences that a helper
might want to queue the packet. And since expectfn setup also doesn't
belong in nf_conntrack_netlink.c (especially not NAT related expectfns),
this is how I think it should be done.

Re: Support NAT-ed expect entries from user space

[Patrick McHardy]

Patrick McHardy wrote:
> BORBELY Zoltan wrote:
>> On Mon, Jun 16, 2008 at 10:52:28PM +0200, Patrick McHardy wrote:
>>  
>>> I'm wondering, how is this expectation creation working at all?
>>> The NULL expectfn makes me think it will crash as soon as the
>>> expectation arrives. This *needs* support from the helpers to
>>> properly set the expectfn.
>>>     
>>
>> The nf_nat_follow_master did the trick for me if I set the expectation
>> entry from user space. With NULL expectfn it didn't work.
>>   
> 
> Yes, so the kernel is broken.


This turned out to be wrong, the expectfn is optional.

Re: Support NAT-ed expect entries from user space

[BORBELY Zoltan]

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

Hi,

On Tue, Jun 17, 2008 at 12:43:37AM +0200, Patrick McHardy wrote:
> I understand that, the expectation part looks like a subset of what
> a helper module does though, with the only differences that a helper
> might want to queue the packet. And since expectfn setup also doesn't
> belong in nf_conntrack_netlink.c (especially not NAT related expectfns),
> this is how I think it should be done.

I attached a new version of the expect setup patch. I think it's general
enough to include into the kernel. What's your opinion? The saved_ip
field is only used by the nf_nat_sip and nf_nat_h323 helpers, we only
need it if we want to set expectfn of our choice.

Bye,
Bozo

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

--- linux-2.6.25.7/net/netfilter/nf_conntrack_netlink.c	2008-06-20 11:21:38.000000000 +0200
+++ linux/net/netfilter/nf_conntrack_netlink.c	2008-06-23 17:00:26.000000000 +0200
@@ -37,8 +37,9 @@
 #include <net/netfilter/nf_conntrack_l4proto.h>
 #include <net/netfilter/nf_conntrack_tuple.h>
 #ifdef CONFIG_NF_NAT_NEEDED
 #include <net/netfilter/nf_nat_core.h>
 #include <net/netfilter/nf_nat_protocol.h>
+#include <net/netfilter/nf_nat_helper.h>
 #endif
 
 #include <linux/netfilter/nfnetlink.h>
@@ -1666,6 +1667,7 @@
 	struct nf_conntrack_expect *exp;
 	struct nf_conn *ct;
 	struct nf_conn_help *help;
+	struct nlattr *tb[CTA_EXPNAT_MAX+1];
 	int err = 0;
 
 	/* caller guarantees that those three CTA_EXPECT_* exist */
@@ -1699,6 +1701,27 @@
 	}
 
 	exp->expectfn = NULL;
+#ifdef CONFIG_NF_NAT_NEEDED
+	if (cda[CTA_EXPECT_NAT]) {
+		exp->expectfn = nf_nat_follow_master;
+		err = nla_parse_nested(tb, CTA_EXPNAT_MAX,
+				       cda[CTA_EXPECT_NAT], NULL);
+		if (err < 0)
+			goto out;
+
+		if (tb[CTA_EXPNAT_SAVED_PROTO])
+			exp->saved_proto.all = nla_get_be16(tb[CTA_EXPNAT_SAVED_PROTO]);
+		if (tb[CTA_EXPNAT_DIRECTION]) {
+			exp->dir = nla_get_u8(tb[CTA_EXPNAT_DIRECTION]);
+			if (exp->dir != IP_CT_DIR_ORIGINAL &&
+			    exp->dir != IP_CT_DIR_REPLY) {
+				err = -EINVAL;
+				goto out;
+			}
+		} else
+			exp->dir = IP_CT_DIR_ORIGINAL;
+	}
+#endif
 	exp->flags = 0;
 	exp->master = ct;
 	exp->helper = NULL;
--- linux-2.6.25.7/include/linux/netfilter/nfnetlink_conntrack.h	2008-06-16 22:24:36.000000000 +0200
+++ linux/include/linux/netfilter/nfnetlink_conntrack.h	2008-06-23 16:29:08.000000000 +0200
@@ -138,6 +138,7 @@
 	CTA_EXPECT_TIMEOUT,
 	CTA_EXPECT_ID,
 	CTA_EXPECT_HELP_NAME,
+	CTA_EXPECT_NAT,
 	__CTA_EXPECT_MAX
 };
 #define CTA_EXPECT_MAX (__CTA_EXPECT_MAX - 1)
@@ -149,4 +150,12 @@
 };
 #define CTA_HELP_MAX (__CTA_HELP_MAX - 1)
 
+enum ctattr_expnat {
+	CTA_EXPNAT_UNSPEC,
+	CTA_EXPNAT_SAVED_PROTO,
+	CTA_EXPNAT_DIRECTION,
+	__CTA_EXPNAT_MAX
+};
+#define CTA_EXPNAT_MAX (__CTA_EXPNAT_MAX - 1)
+
 #endif /* _IPCONNTRACK_NETLINK_H */

Re: Support NAT-ed expect entries from user space

[Patrick McHardy]

BORBELY Zoltan wrote:
> On Tue, Jun 17, 2008 at 12:43:37AM +0200, Patrick McHardy wrote:
>   
>> I understand that, the expectation part looks like a subset of what
>> a helper module does though, with the only differences that a helper
>> might want to queue the packet. And since expectfn setup also doesn't
>> belong in nf_conntrack_netlink.c (especially not NAT related expectfns),
>> this is how I think it should be done.
>>     
>
> I attached a new version of the expect setup patch. I think it's general
> enough to include into the kernel. What's your opinion? The saved_ip
> field is only used by the nf_nat_sip and nf_nat_h323 helpers, we only
> need it if we want to set expectfn of our choice.
>
>   
> +++ linux/net/netfilter/nf_conntrack_netlink.c	2008-06-23 17:00:26.000000000 +0200
> +#ifdef CONFIG_NF_NAT_NEEDED
> +	if (cda[CTA_EXPECT_NAT]) {
> +		exp->expectfn = nf_nat_follow_master;
> +		err = nla_parse_nested(tb, CTA_EXPNAT_MAX,
> +				       cda[CTA_EXPECT_NAT], NULL);
> +		if (err < 0)
> +			goto out;
> +
> +		if (tb[CTA_EXPNAT_SAVED_PROTO])
> +			exp->saved_proto.all = nla_get_be16(tb[CTA_EXPNAT_SAVED_PROTO]);
> +		if (tb[CTA_EXPNAT_DIRECTION]) {
> +			exp->dir = nla_get_u8(tb[CTA_EXPNAT_DIRECTION]);
> +			if (exp->dir != IP_CT_DIR_ORIGINAL &&
> +			    exp->dir != IP_CT_DIR_REPLY) {
> +				err = -EINVAL;
> +				goto out;
> +			}
> +		} else
> +			exp->dir = IP_CT_DIR_ORIGINAL;
> +	}
> +#endif
>   

As I said previously, NAT related things don't belong in
nf_conntrack_netlink.c. The existing ones should be moved
out since they add module dependencies that are simply wrong
(conntrack should *never* depend on NAT).

This patch also adds an interface that is too specialized
for your use case instead of doing it in a way that is
generically useful, which would mean at least providing
helper private expectfn selection. The expectfn selection
should probably use the expectation classes, although I'm
open to be convinced otherwise.