diff mbox

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

Message ID alpine.LNX.2.01.1305081322250.29391@nerf07.vanv.qr
State Not Applicable
Headers show

Commit Message

Jan Engelhardt May 8, 2013, 11:32 a.m. UTC
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?

--
To unsubscribe from this list: send the line "unsubscribe netfilter-devel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Comments

Dmitry Popov May 8, 2013, 3:12 p.m. UTC | #1
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.
--
To unsubscribe from this list: send the line "unsubscribe netfilter-devel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Jan Engelhardt May 8, 2013, 9:32 p.m. UTC | #2
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.
--
To unsubscribe from this list: send the line "unsubscribe netfilter-devel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Dmitry Popov May 13, 2013, 9:50 a.m. UTC | #3
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 =).
--
To unsubscribe from this list: send the line "unsubscribe netfilter-devel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Pablo Neira Ayuso May 15, 2013, 3:04 p.m. UTC | #4
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.
--
To unsubscribe from this list: send the line "unsubscribe netfilter-devel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
diff mbox

Patch

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;