[PATCH] Remove redundant TCP header checks from xt_TCPOPTSTRIP

[PATCH] Remove redundant TCP header checks from xt_TCPOPTSTRIP

[Phil Oester]

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

In commit bc6bcb59 ("netfilter: xt_TCPOPTSTRIP: fix possible mangling beyond
packet boundary"), a check for short TCP header or malformed packet was added.
This check is unnecessary, as these packets are already handled in the tcp_error
function of nf_conntrack_proto_tcp.c (see /* Not whole TCP header or malformed
packet */).  In addition, there was an error in the check which was added (len
is being calculated incorrectly).  In my testing, ALL packets are being dropped
by the TCPOPTSTRIP target at present.  Revert the unnecessary/incorrect checks.

Phil

Signed-off-by: Phil Oester <kernel@linuxace.com>


[-- Attachment #2: patch-revert-bc6bcb59d --]
[-- Type: text/plain, Size: 745 bytes --]

diff --git a/net/netfilter/xt_TCPOPTSTRIP.c b/net/netfilter/xt_TCPOPTSTRIP.c
index 1eb1a44..2d43be9f 100644
--- a/net/netfilter/xt_TCPOPTSTRIP.c
+++ b/net/netfilter/xt_TCPOPTSTRIP.c
@@ -38,7 +38,6 @@ tcpoptstrip_mangle_packet(struct sk_buff *skb,
 	struct tcphdr *tcph;
 	u_int16_t n, o;
 	u_int8_t *opt;
-	int len;
 
 	/* This is a fragment, no TCP header is available */
 	if (par->fragoff != 0)
@@ -47,11 +46,6 @@ tcpoptstrip_mangle_packet(struct sk_buff *skb,
 	if (!skb_make_writable(skb, skb->len))
 		return NF_DROP;
 
-	len = skb->len - tcphoff;
-	if (len < (int)sizeof(struct tcphdr) ||
-	    tcp_hdr(skb)->doff * 4 > len)
-		return NF_DROP;
-
 	tcph = (struct tcphdr *)(skb_network_header(skb) + tcphoff);
 	opt  = (u_int8_t *)tcph;
 

Re: [PATCH] Remove redundant TCP header checks from xt_TCPOPTSTRIP

[Pablo Neira Ayuso]

On Sun, Jun 09, 2013 at 11:59:48PM -0400, Phil Oester wrote:
> In commit bc6bcb59 ("netfilter: xt_TCPOPTSTRIP: fix possible mangling beyond
> packet boundary"), a check for short TCP header or malformed packet was added.
> This check is unnecessary, as these packets are already handled in the tcp_error
> function of nf_conntrack_proto_tcp.c (see /* Not whole TCP header or malformed
> packet */).

We cannot assume nf_conntrack is loaded. We have to support stateless
setups as well.

> In addition, there was an error in the check which was added (len
> is being calculated incorrectly).  In my testing, ALL packets are being dropped
> by the TCPOPTSTRIP target at present.  Revert the unnecessary/incorrect checks.

Then, we have to fix the wrong calculation. I cannot reproduce this
here.

Re: [PATCH] Remove redundant TCP header checks from xt_TCPOPTSTRIP

[Florian Westphal]

Pablo Neira Ayuso <pablo@netfilter.org> wrote:
> We cannot assume nf_conntrack is loaded. We have to support stateless
> setups as well.
> 
> > In addition, there was an error in the check which was added (len
> > is being calculated incorrectly).  In my testing, ALL packets are being dropped
> > by the TCPOPTSTRIP target at present.  Revert the unnecessary/incorrect checks.
> 
> Then, we have to fix the wrong calculation. I cannot reproduce this
> here.

Its most likely due to tcp_hdr() use, it only works for local packets.