Skip to contentSkip to conversation

Remove redundant TCP header checks from xt_TCPOPTSTRIP

PATCH In review Phil Oester 3 participants 3 messages Jun 10, 2013

Subsystems (2)
Networking netfilter
lore
Share & tools
lore

Phil Oester

author
#0 lore raw

PATCH 1/1 Remove redundant TCP header checks from xt_TCPOPTSTRIP

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 <[email protected]>
net/netfilter/xt_TCPOPTSTRIP.c +0 -6
index 1eb1a44..2d43be9f 100644
@@ -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;
maintainer
#1 lore raw
Quoted 5 lines · Phil Oester

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.

Quoted 3 lines

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.

maintainer
#2 lore raw
Quoted 9 lines · Pablo Neira Ayuso

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

Quoted 3 lines

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.