xt_TCPMSS: SYN packets are allowed to contain data

The check for data only needs to apply where the packet length could be increased by adding the MSS option. (The MSS option itself applies to the sender's maximum receive size which is not relevant to any data in its own packet.)

This moves the check for (header size != packet size) to after attempting to modify an existing MSS option. Another check is needed before looking through the header to ensure it doesn't claim to be larger than the packet size.

The ERROR level printk() is also removed as it can be triggered by remote hosts and is not useful:

[4941777.937417] xt_TCPMSS: bad length (40 bytes)
[4941782.409724] xt_TCPMSS: bad length (40 bytes)
[4941790.762332] xt_TCPMSS: bad length (40 bytes)

Signed-off-by Simon Arlott <[email protected]>

---
 net/netfilter/xt_TCPMSS.c |   21 +++++++++++----------
 1 files changed, 11 insertions(+), 10 deletions(-)

net/netfilter/xt_TCPMSS.c +11 -10

index eda64c1..76f92bf 100644
@@ -60,17 +60,9 @@ tcpmss_mangle_packet(struct sk_buff *skb,
 	tcplen = skb->len - tcphoff;
 	tcph = (struct tcphdr *)(skb_network_header(skb) + tcphoff);
 
-	/* Since it passed flags test in tcp match, we know it is is
-	   not a fragment, and has data >= tcp header length.  SYN
-	   packets should not contain data: if they did, then we risk
-	   running over MTU, sending Frag Needed and breaking things
-	   badly. --RR */
-	if (tcplen != tcph->doff*4) {
-		if (net_ratelimit())
-			printk(KERN_ERR "xt_TCPMSS: bad length (%u bytes)\n",
-			       skb->len);
+	/* Header cannot be larger than the packet */
+	if (tcplen < tcph->doff*4)
 		return -1;
-	}
 
 	if (info->mss == XT_TCPMSS_CLAMP_PMTU) {
 		if (dst_mtu(skb_dst(skb)) <= minlen) {

@@ -115,6 +107,15 @@ tcpmss_mangle_packet(struct sk_buff *skb,
 		}
 	}
 
+	/* Since it passed flags test in tcp match, we know it is
+	   not a fragment, and has data >= tcp header length.  SYN
+	   packets should not contain data: if they did, then we risk
+	   running over MTU, sending Frag Needed and breaking things
+	   badly. --RR
+	*/
+	if (tcplen > tcph->doff*4)
+		return -1;
+
 	/*
 	 * MSS Option not found ?! add it..
 	 */

Quoted 10 lines · Simon Arlott

The check for data only needs to apply where the packet length could be increased by adding the MSS option. (The MSS option itself applies to the sender's maximum receive size which is not relevant to any data in its own packet.)

This moves the check for (header size != packet size) to after attempting to modify an existing MSS option. Another check is needed before looking through the header to ensure it doesn't claim to be larger than the packet size.

What's the path from tcp_v[4,6]_rcv() to these tests?

1) Header larger than the packet is already tested in about 5 places, and my patch "tcp: harmonize tcp_vx_rcv header length assumptions" tries to get them all down to just *one* test.

Quoted 8 lines · William Allen Simpson

Quoted 5 lines · Simon Arlott

This moves the check for (header size != packet size) to after attempting to modify an existing MSS option. Another check is needed before looking through the header to ensure it doesn't claim to be larger than the packet size.

What's the path from tcp_v[4,6]_rcv() to these tests?

Quoted 3 lines

1) Header larger than the packet is already tested in about 5 places, and my patch "tcp: harmonize tcp_vx_rcv header length assumptions" tries to get them all down to just *one* test.

We're talking about a netfilter module here, which has to deal with forwarded traffic and can only rely on the IP header checks done in ip_rcv().

Quoted 7 lines · Patrick McHardy

Quoted 6 lines · William Allen Simpson

Quoted 5 lines · Simon Arlott

This moves the check for (header size != packet size) to after attempting to modify an existing MSS option. Another check is needed before looking through the header to ensure it doesn't claim to be larger than the packet size.

Quoted 8 lines

Quoted 5 lines

What's the path from tcp_v[4,6]_rcv() to these tests?

1) Header larger than the packet is already tested in about 5 places, and my patch "tcp: harmonize tcp_vx_rcv header length assumptions" tries to get them all down to just *one* test.

We're talking about a netfilter module here, which has to deal with forwarded traffic and can only rely on the IP header checks done in ip_rcv().

My gateway (where these error messages occur) is running 2.6.29, and skb->len (from the prink) is 40 bytes.

If this is 20 (IPv4 Header) + 20 (TCP Header) = 40 bytes, then there is no data and the header offset is wrong so it hasn't been checked.

Quoted 5 lines · Simon Arlott

The check for data only needs to apply where the packet length could be increased by adding the MSS option. (The MSS option itself applies to the sender's maximum receive size which is not relevant to any data in its own packet.)

Quoted 5 lines

This moves the check for (header size != packet size) to after attempting to modify an existing MSS option. Another check is needed before looking through the header to ensure it doesn't claim to be larger than the packet size.

2) There certainly *can* be data on SYN. That code is already in 2.6.33....

Quoted 2 lines · William Allen Simpson

2) There certainly *can* be data on SYN. That code is already in 2.6.33....

I could change the comment too, but the same logic applies when there is data and no MSS option - the packet can't be increased in size if it would then exceed 576 bytes and/or the destination MTU.

If it's possible to know that the packet can have an additional option added without exceeding MTU then this could be changed. The data part would need to be moved to make space at the end of the header.

Quoted 6 lines · Simon Arlott

On Tue, January 19, 2010 09:17, William Allen Simpson wrote:
I could change the comment too, but the same logic applies when there is data and no MSS option - the packet can't be increased in size if it would then exceed 576 bytes and/or the destination MTU.

Please change the comment.

If there is no MSS option, it should *not* be added, under *ANY* circumstances. That violates the end-to-end arguments (some call them principles).

MSS isn't about the _destination_ MTU, it's about the *source*. If you cannot guarantee you know the source MTU, there's no basis for deciding the MSS.

Quoted 3 lines · Simon Arlott

If this is 20 (IPv4 Header) + 20 (TCP Header) = 40 bytes, then there is no data and the header offset is wrong so it hasn't been checked.

That's odd. If the packet is really only 40 bytes large, then there are no TCP options, so your patch shouldn't have any effect.

The TCPMSS target is dropping SYN packets where:
1) There is data, or
2) The data offset makes the TCP header larger than the packet.

Both of these result in an error level printk.

This change fixes the drop of SYN packets with data (because the MSS option can safely be modified) and passes packets with no MSS option instead of adding one (which is not valid).

Quoted 5 lines · Simon Arlott

The TCPMSS target is dropping SYN packets where:
1) There is data, or
2) The data offset makes the TCP header larger than the packet.

Quoted 6 lines

Both of these result in an error level printk.

This change fixes the drop of SYN packets with data (because the MSS option can safely be modified) and passes packets with no MSS option instead of adding one (which is not valid).

Can you explain why the automatic addition of a MSS option is removed?

Quoted 14 lines · Jan Engelhardt

Quoted 11 lines · Simon Arlott

The TCPMSS target is dropping SYN packets where:
1) There is data, or
2) The data offset makes the TCP header larger than the packet.

Both of these result in an error level printk.

This change fixes the drop of SYN packets with data (because the MSS option can safely be modified) and passes packets with no MSS option instead of adding one (which is not valid).

Quoted 1 line

Can you explain why the automatic addition of a MSS option is removed?

That is, of course, for the git log. If I followed the thread right, it was that adding the option could exceed the MTU. Well, can't we check for the outgoing MTU?

Quoted 16 lines · Jan Engelhardt

Quoted 14 lines · Jan Engelhardt

Quoted 11 lines · Simon Arlott

The TCPMSS target is dropping SYN packets where:
1) There is data, or
2) The data offset makes the TCP header larger than the packet.

Both of these result in an error level printk.

This change fixes the drop of SYN packets with data (because the MSS option can safely be modified) and passes packets with no MSS option instead of adding one (which is not valid).

Quoted 6 lines

Quoted 1 line

Can you explain why the automatic addition of a MSS option is removed?

That is, of course, for the git log. If I followed the thread right, it

was that adding the option could exceed the MTU. Well, can't we check for the outgoing MTU?

The MSS option is for the MRU of whoever sent the SYN packet. There's no way of knowing this information so it's not possible to avoid using an MSS that is too large. With no option, "any" segment size could be used, which implies 536 to match the MRU of 576.

The other reason for not being able to add it is that it may increase

packet size beyond an MRU/MTU limit if there is data. There's no

we'll see an ICMP error message if this occurs, because the limit

have to be local and the return path does not need to be the same. The original host won't know that the packet is going to be increased in

Quoted 13 lines · Jan Engelhardt

Quoted 11 lines · Simon Arlott

The TCPMSS target is dropping SYN packets where:
1) There is data, or
2) The data offset makes the TCP header larger than the packet.

Both of these result in an error level printk.

This change fixes the drop of SYN packets with data (because the MSS option can safely be modified) and passes packets with no MSS option instead of adding one (which is not valid).

Quoted 1 line

Can you explain why the automatic addition of a MSS option is removed?

That is, of course, for the git log. If I followed the thread right, it was that adding the option could exceed the MTU. Well, can't we check for the outgoing MTU?

Quoted 13 lines · Jan Engelhardt

Quoted 12 lines · Jan Engelhardt

Quoted 11 lines · Simon Arlott

The TCPMSS target is dropping SYN packets where:
1) There is data, or
2) The data offset makes the TCP header larger than the packet.

Both of these result in an error level printk.

This change fixes the drop of SYN packets with data (because the MSS option can safely be modified) and passes packets with no MSS option instead of adding one (which is not valid).

Quoted 5 lines

Quoted 1 line

Can you explain why the automatic addition of a MSS option is removed?

That is, of course, for the git log. If I followed the thread right, it was that adding the option could exceed the MTU. Well, can't we check for the outgoing MTU?

We certainly can, and in fact the packet would get fragmented by the IP layer in case we would exceed the PMTU. Additionally we currently check that the packet contains no data, even with the first version of this patch, so there's no way the packet could exceed the MTU.

This feature has been there from day one since the TCPMSS target has been merged and people are using this with knowledge of their MTUs to work around broken ISPs. I'm not apply this.

The first version seemed fine to me though :)

The TCPMSS target can be applied to more than just one direction of traffic. I'm modifying incoming traffic too, so adding the MSS option and setting it to over 536 is wrong (although the first ICMP error will fix it).

Existing users use this target precisely because their hosts are sending an unwanted MSS value, so it will never need to be added.

Quoted 10 lines · Patrick McHardy

Quoted 4 lines · Jan Engelhardt

Quoted 1 line

Can you explain why the automatic addition of a MSS option is removed?

That is, of course, for the git log. If I followed the thread right, it was that adding the option could exceed the MTU. Well, can't we check for the outgoing MTU?

We certainly can, and in fact the packet would get fragmented by the IP layer in case we would exceed the PMTU. Additionally we currently check that the packet contains no data, even with the first version of this patch, so there's no way the packet could exceed the MTU.

If DF is set and the MTU is exceeded (for the SYN packet) at a hop further away, the original host will not understand that it needs to allow for the MSS option being added.

(Header + Data + New MSS Option) can't exceed 576 bytes and there's no way to know that more than 576 bytes is allowed because the ICMP error message may not go via the same host that is mangling the packet.

Of course, it could just allow fragmentation for this one SYN packet but that doesn't work for IPv6.

Quoted 3 lines

This feature has been there from day one since the TCPMSS target has been merged and people are using this with knowledge of their MTUs to work around broken ISPs. I'm not apply this.

The TCPMSS target can be applied to more than just one direction of traffic. I'm modifying incoming traffic too, so adding the MSS option and setting it to over 536 is wrong (although the first ICMP error will fix it).

Existing users use this target precisely because their hosts are sending an unwanted MSS value, so it will never need to be added.

Quoted 1 line

The first version seemed fine to me though :)

The first version is ok with me. Only SYN packets with data and no MSS option will be dropped. William objects to ever adding the MSS option.

Although ideally SYN packets with data and no MSS option should be accepted without adding an option. Dropping arbitrary traffic (especially when new kernels allow data to be sent with SYN packets) is not a good idea. If that is ok with you then I'll make another patch to do it and update the comments.

The TCPMSS target is dropping SYN packets where:
1) There is data, or
2) The data offset makes the TCP header larger than the packet.

Both of these result in an error level printk. This printk has been removed.

This change avoids dropping SYN packets containing data. If there is also no MSS option (as well as data), one will not be added because of possible complications due to the increased packet size.