[PATCH] xtables-addons: xt_RAWNAT: skb writable part might not include whole l4 header (ipv4 case).

[PATCH] xtables-addons: xt_RAWNAT: skb writable part might not include whole l4 header (ipv4 case).

[Dmitry Popov]

Consider TCP/IPv4 packet with IP options:
sizeof(*iph) + sizeof(struct tcphdr) is not enough to include tcp checksum.
It may hurt if this packet is fragmented.

Therefore we should use iph->ihl * 4 instead of sizeof(*iph).

Signed-off-by: Dmitry Popov <dp@highloadlab.com>
---
 extensions/xt_RAWNAT.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/extensions/xt_RAWNAT.c b/extensions/xt_RAWNAT.c
index a52e614..858f911 100644
--- a/extensions/xt_RAWNAT.c
+++ b/extensions/xt_RAWNAT.c
@@ -109,7 +109,7 @@ static void rawnat4_update_l4(struct sk_buff *skb, __be32 oldip, __be32 newip)
 
 static unsigned int rawnat4_writable_part(const struct iphdr *iph)
 {
-	unsigned int wlen = sizeof(*iph);
+	unsigned int wlen = iph->ihl * 4;
 
 	switch (iph->protocol) {
 	case IPPROTO_TCP:

Re: [PATCH] xtables-addons: xt_RAWNAT: skb writable part might not include whole l4 header (ipv4 case).

[Dmitry Popov]

Also, shouldn't xt_RAWNAT depend on nf_defrag_ipv4 module? 
xt_RAWNAT may work with ip fragments in PREROUTING chain, changing ip payload
(believing it's tcp/udp checksum) in fragment is harmful.

Re: [PATCH] xtables-addons: xt_RAWNAT: skb writable part might not include whole l4 header (ipv4 case).

[Jan Engelhardt]

On Sunday 2013-05-05 20:24, Dmitry Popov wrote:

>Also, shouldn't xt_RAWNAT depend on nf_defrag_ipv4 module? 

Dunno. Being a module for really "raw" nf_conntrack-less static NAT, I 
feel no reason to make it hard-depend on nf_defrag, and instead leave it 
up to the user whether or not to load nf_defrag.

With nf_nat having gained IPv6 support, I also feel less inclined to 
keep xt_RAWNAT around.

(In the meantime, your patch is applied.)


>xt_RAWNAT may work with ip fragments in PREROUTING chain, changing ip payload
>(believing it's tcp/udp checksum) in fragment is harmful.

I would tend to just ignore the fragment case for now, like many other 
modules. Comments against?

diff --git a/extensions/xt_RAWNAT.c b/extensions/xt_RAWNAT.c
index 858f911..0a24e77 100644
--- a/extensions/xt_RAWNAT.c
+++ b/extensions/xt_RAWNAT.c
@@ -87,6 +87,13 @@ static void rawnat4_update_l4(struct sk_buff *skb, __be32 oldip, __be32 newip)
 	struct udphdr *udph;
 	bool cond;
 
+	/*
+	 * We do not really deal with fragments. On the first packet, we can attempt
+	 * to modify the L4 header, otherwise just ignore the data.
+	 */
+	if ((iph->frag_off & htons(IP_OFFSET)) == 0)
+		return;
+
 	switch (iph->protocol) {
 	case IPPROTO_TCP:
 		tcph = transport_hdr;

Re: [PATCH] xtables-addons: xt_RAWNAT: skb writable part might not include whole l4 header (ipv4 case).

[Dmitry Popov]

On Wed, 8 May 2013 13:32:57 +0200 (CEST)
Jan Engelhardt <jengelh@inai.de> wrote:

> >Also, shouldn't xt_RAWNAT depend on nf_defrag_ipv4 module? 
> 
> Dunno. Being a module for really "raw" nf_conntrack-less static NAT, I 
> feel no reason to make it hard-depend on nf_defrag, and instead leave it 
> up to the user whether or not to load nf_defrag.
> 
> I would tend to just ignore the fragment case for now, like many other 
> modules. Comments against?
> 

Yes, I think it's a better idea. The only argument against may be a
security hole if someone relies on xt_RAWNAT and doesn't use nf_defrag.
Though it's a poor argument imho.

> With nf_nat having gained IPv6 support, I also feel less inclined to 
> keep xt_RAWNAT around.
> 

nf_nat depends on conntrack and conntrack brings a huge overhead to 
such a simple task like NAT. xt_RAWNAT simply solves NAT problem, it
definitely has to stay.

Re: [PATCH] xtables-addons: xt_RAWNAT: skb writable part might not include whole l4 header (ipv4 case).

[Jan Engelhardt]

On Wednesday 2013-05-08 17:12, Dmitry Popov wrote:
>
>> With nf_nat having gained IPv6 support, I also feel less inclined to 
>> keep xt_RAWNAT around.
>
>nf_nat depends on conntrack and conntrack brings a huge overhead to 
>such a simple task like NAT. xt_RAWNAT simply solves NAT problem, it
>definitely has to stay.

The only way to solve the NAT problem is to do without it.
Full NAT is not simple at all, it requires DPI.
RAWNAT is just a dumb l3addr replacer and does not help
getting multi-connection sessions (such as 959ish FTP) going.

Re: [PATCH] xtables-addons: xt_RAWNAT: skb writable part might not include whole l4 header (ipv4 case).

[Dmitry Popov]

On Wed, 8 May 2013 23:32:16 +0200 (CEST)
Jan Engelhardt <jengelh@inai.de> wrote:

> The only way to solve the NAT problem is to do without it.
> Full NAT is not simple at all, it requires DPI.
> RAWNAT is just a dumb l3addr replacer and does not help
> getting multi-connection sessions (such as 959ish FTP) going.

Well, in means of full nat - yes. I have no statistics of how people use 
nf_nat/xt_RAWNAT, but in my tasks I have a lot of packets that do not need DPI.
xt_RAWNAT works great and nf_nat led to packet loss. It probably was because
of main conntrack lock. Yes, I read it was removed not long ago, and haven't 
tested it since then, but anyway I do not want to use such a monster just to 
change 2-3 fields of packet. Just an use case, decision is up to you =).

Re: [PATCH] xtables-addons: xt_RAWNAT: skb writable part might not include whole l4 header (ipv4 case).

[Pablo Neira Ayuso]

On Mon, May 13, 2013 at 01:50:20PM +0400, Dmitry Popov wrote:
> On Wed, 8 May 2013 23:32:16 +0200 (CEST)
> Jan Engelhardt <jengelh@inai.de> wrote:
> 
> > The only way to solve the NAT problem is to do without it.
> > Full NAT is not simple at all, it requires DPI.
> > RAWNAT is just a dumb l3addr replacer and does not help
> > getting multi-connection sessions (such as 959ish FTP) going.
> 
> Well, in means of full nat - yes. I have no statistics of how people use 
> nf_nat/xt_RAWNAT, but in my tasks I have a lot of packets that do
> not need DPI.

Not only DPI. You're also leaking your network topology though ICMP
error messages, as the internal header is not mangled.