All of lore.kernel.org
 help / color / mirror / Atom feed
* Re: [PATCH 3/5] netfilter: xt_TCPMSS: Fix violation of RFC879 in absence of MSS option
  2013-06-11  8:43   ` David Laight
@ 2013-06-10  7:19     ` Phil Oester
  2013-06-11 15:00       ` David Laight
  0 siblings, 1 reply; 18+ messages in thread
From: Phil Oester @ 2013-06-10  7:19 UTC (permalink / raw)
  To: David Laight; +Cc: Pablo Neira Ayuso, netfilter-devel, davem, netdev

On Tue, Jun 11, 2013 at 09:43:07AM +0100, David Laight wrote:
> Is setting the mss to 536 actually ever sensible?
> RFC 879 might say that it is the default (and the minimum
> that must be supported), but in practise the actual mss
> is very likely to be only slightly shorter than the standard
> ethernet mss.
> Although strict conformance with RFC 879 might require the mss
> be clamped to 536, pragmatically a value much nearer 1400 would
> make sense - systems with very low mtu/mss are probably likely
> to advertise it.

Read the associated bugzilla - there was at least one real world
example where setting a higher MSS was causing breakage.

Phil

https://bugzilla.netfilter.org/show_bug.cgi?id=662

^ permalink raw reply	[flat|nested] 18+ messages in thread

* Re: [PATCH 3/5] netfilter: xt_TCPMSS: Fix violation of RFC879 in absence of MSS option
  2013-06-11 15:00       ` David Laight
@ 2013-06-10  8:27         ` Phil Oester
  2013-06-11 16:09           ` David Laight
  0 siblings, 1 reply; 18+ messages in thread
From: Phil Oester @ 2013-06-10  8:27 UTC (permalink / raw)
  To: David Laight; +Cc: Pablo Neira Ayuso, netfilter-devel, davem, netdev

On Tue, Jun 11, 2013 at 04:00:06PM +0100, David Laight wrote:
> To quote that bug:
> 
> I stumbled upon this problem in debian bug #541658[1] ("[iceweasel] cannot open 
> research.microsoft.com" - only worth reading for entertainment purposes) and,
> after that bug was closed, analysed it in my blog[2] until a friend of mine
> found out why the page loads when clamping mss to pmtu is disabled or
> restricted to a range (like with "iptables -A FORWARD -p tcp --tcp-flags
> SYN,RST SYN -m tcpmss --mss 1400:1536 -j TCPMSS --clamp-mss-to-pmtu") but
> doesn't load with "simple" clamping. His really great and detailed analysation
> of the problem may be seen at [3].
> 
> If I read/understand that correctly, clamping to 1400 worked - there was
> no need to clamp all the way down to 536.

You are not understanding the issue correctly.  The reason the command worked with
"-m tcpmss --mss 1400:1536" is because that implies an MSS option was provided.
The issue occurs only when NO MSS option is sent.  In these cases, we cannot
ASSUME that it is ok to use some arbitrarily high value (1400 as you propose).
The RFC is clear on this point.

Phil

^ permalink raw reply	[flat|nested] 18+ messages in thread

* [PATCH 0/5] netfilter fixes for 3.10-rc5
@ 2013-06-10 16:07 Pablo Neira Ayuso
  2013-06-10 16:07 ` [PATCH 1/5] netfilter: nfnetlink_acct: fix incomplete dumping of objects Pablo Neira Ayuso
                   ` (5 more replies)
  0 siblings, 6 replies; 18+ messages in thread
From: Pablo Neira Ayuso @ 2013-06-10 16:07 UTC (permalink / raw)
  To: netfilter-devel; +Cc: davem, netdev

Hi David,

The following patchset contains four fixes for Netfilter and one fix
for IPVS, they are:

* Fix data leak to user-space via getsockopt IP_VS_SO_GET_DESTS, from
  Dan Carpenter.

* Fix xt_TCPMSS if no TCP MSS is specified in syn packets, to avoid the
  violation of RFC879, from Phil Oester.

* Fix incomplete dump of objects via nfnetlink_acct and nfnetlink_cttimeout,
  from myself.

* Fix missing HW protocol in packets passed to user-space via NFQUEUE,
  from myself.

You can pull these changes from:

git://git.kernel.org/pub/scm/linux/kernel/git/pablo/nf.git master

Thanks!

----------------------------------------------------------------

The following changes since commit 5343a7f8be11951cb3095b91e8e4eb506cfacc0f:

  net_sched: htb: do not mix 1ns and 64ns time units (2013-06-04 17:44:07 -0700)

are available in the git repository at:

  git://git.kernel.org/pub/scm/linux/kernel/git/pablo/nf.git master

for you to fetch changes up to a8241c63517ec0b900695daa9003cddc41c536a1:

  ipvs: info leak in __ip_vs_get_dest_entries() (2013-06-10 14:53:00 +0200)

----------------------------------------------------------------
Dan Carpenter (1):
      ipvs: info leak in __ip_vs_get_dest_entries()

Pablo Neira Ayuso (3):
      netfilter: nfnetlink_acct: fix incomplete dumping of objects
      netfilter: nfnetlink_cttimeout: fix incomplete dumping of objects
      netfilter: nfnetlink_queue: fix missing HW protocol

Phil Oester (1):
      netfilter: xt_TCPMSS: Fix violation of RFC879 in absence of MSS option

 net/netfilter/ipvs/ip_vs_ctl.c       |    1 +
 net/netfilter/nfnetlink_acct.c       |    7 +++++--
 net/netfilter/nfnetlink_cttimeout.c  |    7 +++++--
 net/netfilter/nfnetlink_queue_core.c |    6 +++---
 net/netfilter/xt_TCPMSS.c            |    6 ++++++
 5 files changed, 20 insertions(+), 7 deletions(-)

^ permalink raw reply	[flat|nested] 18+ messages in thread

* [PATCH 1/5] netfilter: nfnetlink_acct: fix incomplete dumping of objects
  2013-06-10 16:07 [PATCH 0/5] netfilter fixes for 3.10-rc5 Pablo Neira Ayuso
@ 2013-06-10 16:07 ` Pablo Neira Ayuso
  2013-06-10 16:07 ` [PATCH 2/5] netfilter: nfnetlink_cttimeout: " Pablo Neira Ayuso
                   ` (4 subsequent siblings)
  5 siblings, 0 replies; 18+ messages in thread
From: Pablo Neira Ayuso @ 2013-06-10 16:07 UTC (permalink / raw)
  To: netfilter-devel; +Cc: davem, netdev

Fix broken incomplete object dumping if the list of objects does not
fit into one single netlink message.

Reported-by: Gabriel Lazar <Gabriel.Lazar@com.utcluj.ro>
Signed-off-by: Pablo Neira Ayuso <pablo@netfilter.org>
---
 net/netfilter/nfnetlink_acct.c |    7 +++++--
 1 file changed, 5 insertions(+), 2 deletions(-)

diff --git a/net/netfilter/nfnetlink_acct.c b/net/netfilter/nfnetlink_acct.c
index dc3fd5d..c7b6d46 100644
--- a/net/netfilter/nfnetlink_acct.c
+++ b/net/netfilter/nfnetlink_acct.c
@@ -149,9 +149,12 @@ nfnl_acct_dump(struct sk_buff *skb, struct netlink_callback *cb)
 
 	rcu_read_lock();
 	list_for_each_entry_rcu(cur, &nfnl_acct_list, head) {
-		if (last && cur != last)
-			continue;
+		if (last) {
+			if (cur != last)
+				continue;
 
+			last = NULL;
+		}
 		if (nfnl_acct_fill_info(skb, NETLINK_CB(cb->skb).portid,
 				       cb->nlh->nlmsg_seq,
 				       NFNL_MSG_TYPE(cb->nlh->nlmsg_type),
-- 
1.7.10.4

^ permalink raw reply related	[flat|nested] 18+ messages in thread

* [PATCH 2/5] netfilter: nfnetlink_cttimeout: fix incomplete dumping of objects
  2013-06-10 16:07 [PATCH 0/5] netfilter fixes for 3.10-rc5 Pablo Neira Ayuso
  2013-06-10 16:07 ` [PATCH 1/5] netfilter: nfnetlink_acct: fix incomplete dumping of objects Pablo Neira Ayuso
@ 2013-06-10 16:07 ` Pablo Neira Ayuso
  2013-06-10 16:07 ` [PATCH 3/5] netfilter: xt_TCPMSS: Fix violation of RFC879 in absence of MSS option Pablo Neira Ayuso
                   ` (3 subsequent siblings)
  5 siblings, 0 replies; 18+ messages in thread
From: Pablo Neira Ayuso @ 2013-06-10 16:07 UTC (permalink / raw)
  To: netfilter-devel; +Cc: davem, netdev

Fix broken incomplete object dumping if the list of objects does not
fit into one single netlink message.

Reported-by: Gabriel Lazar <Gabriel.Lazar@com.utcluj.ro>
Signed-off-by: Pablo Neira Ayuso <pablo@netfilter.org>
---
 net/netfilter/nfnetlink_cttimeout.c |    7 +++++--
 1 file changed, 5 insertions(+), 2 deletions(-)

diff --git a/net/netfilter/nfnetlink_cttimeout.c b/net/netfilter/nfnetlink_cttimeout.c
index 701c88a..65074df 100644
--- a/net/netfilter/nfnetlink_cttimeout.c
+++ b/net/netfilter/nfnetlink_cttimeout.c
@@ -220,9 +220,12 @@ ctnl_timeout_dump(struct sk_buff *skb, struct netlink_callback *cb)
 
 	rcu_read_lock();
 	list_for_each_entry_rcu(cur, &cttimeout_list, head) {
-		if (last && cur != last)
-			continue;
+		if (last) {
+			if (cur != last)
+				continue;
 
+			last = NULL;
+		}
 		if (ctnl_timeout_fill_info(skb, NETLINK_CB(cb->skb).portid,
 					   cb->nlh->nlmsg_seq,
 					   NFNL_MSG_TYPE(cb->nlh->nlmsg_type),
-- 
1.7.10.4

^ permalink raw reply related	[flat|nested] 18+ messages in thread

* [PATCH 3/5] netfilter: xt_TCPMSS: Fix violation of RFC879 in absence of MSS option
  2013-06-10 16:07 [PATCH 0/5] netfilter fixes for 3.10-rc5 Pablo Neira Ayuso
  2013-06-10 16:07 ` [PATCH 1/5] netfilter: nfnetlink_acct: fix incomplete dumping of objects Pablo Neira Ayuso
  2013-06-10 16:07 ` [PATCH 2/5] netfilter: nfnetlink_cttimeout: " Pablo Neira Ayuso
@ 2013-06-10 16:07 ` Pablo Neira Ayuso
  2013-06-11  8:43   ` David Laight
  2013-06-11 18:36   ` John Heffner
  2013-06-10 16:07 ` [PATCH 4/5] netfilter: nfnetlink_queue: fix missing HW protocol Pablo Neira Ayuso
                   ` (2 subsequent siblings)
  5 siblings, 2 replies; 18+ messages in thread
From: Pablo Neira Ayuso @ 2013-06-10 16:07 UTC (permalink / raw)
  To: netfilter-devel; +Cc: davem, netdev

From: Phil Oester <kernel@linuxace.com>

The clamp-mss-to-pmtu option of the xt_TCPMSS target can cause issues
connecting to websites if there was no MSS option present in the
original SYN packet from the client. In these cases, it may add a
MSS higher than the default specified in RFC879. Fix this by never
setting a value > 536 if no MSS option was specified by the client.

This closes netfilter's bugzilla #662.

Signed-off-by: Phil Oester <kernel@linuxace.com>
Signed-off-by: Pablo Neira Ayuso <pablo@netfilter.org>
---
 net/netfilter/xt_TCPMSS.c |    6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/net/netfilter/xt_TCPMSS.c b/net/netfilter/xt_TCPMSS.c
index a75240f..afaebc7 100644
--- a/net/netfilter/xt_TCPMSS.c
+++ b/net/netfilter/xt_TCPMSS.c
@@ -125,6 +125,12 @@ tcpmss_mangle_packet(struct sk_buff *skb,
 
 	skb_put(skb, TCPOLEN_MSS);
 
+	/* RFC 879 states that the default MSS is 536 without specific
+	 * knowledge that the destination host is prepared to accept larger.
+	 * Since no MSS was provided, we MUST NOT set a value > 536.
+	 */
+	newmss = min(newmss, (u16)536);
+
 	opt = (u_int8_t *)tcph + sizeof(struct tcphdr);
 	memmove(opt + TCPOLEN_MSS, opt, tcplen - sizeof(struct tcphdr));
 
-- 
1.7.10.4

^ permalink raw reply related	[flat|nested] 18+ messages in thread

* [PATCH 4/5] netfilter: nfnetlink_queue: fix missing HW protocol
  2013-06-10 16:07 [PATCH 0/5] netfilter fixes for 3.10-rc5 Pablo Neira Ayuso
                   ` (2 preceding siblings ...)
  2013-06-10 16:07 ` [PATCH 3/5] netfilter: xt_TCPMSS: Fix violation of RFC879 in absence of MSS option Pablo Neira Ayuso
@ 2013-06-10 16:07 ` Pablo Neira Ayuso
  2013-06-10 16:07 ` [PATCH 5/5] ipvs: info leak in __ip_vs_get_dest_entries() Pablo Neira Ayuso
  2013-06-10 20:32 ` [PATCH 0/5] netfilter fixes for 3.10-rc5 David Miller
  5 siblings, 0 replies; 18+ messages in thread
From: Pablo Neira Ayuso @ 2013-06-10 16:07 UTC (permalink / raw)
  To: netfilter-devel; +Cc: davem, netdev

Locally generated IPv4 and IPv6 traffic gets skb->protocol unset,
thus passing zero.

ip6tables -I OUTPUT -j NFQUEUE
libmnl/examples/netfilter# ./nf-queue 0 &
ping6 ::1
packet received (id=1 hw=0x0000 hook=3)
                         ^^^^^^

Signed-off-by: Pablo Neira Ayuso <pablo@netfilter.org>
---
 net/netfilter/nfnetlink_queue_core.c |    6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/net/netfilter/nfnetlink_queue_core.c b/net/netfilter/nfnetlink_queue_core.c
index 4e27fa0..5352b2d 100644
--- a/net/netfilter/nfnetlink_queue_core.c
+++ b/net/netfilter/nfnetlink_queue_core.c
@@ -637,9 +637,6 @@ nfqnl_enqueue_packet(struct nf_queue_entry *entry, unsigned int queuenum)
 	if (queue->copy_mode == NFQNL_COPY_NONE)
 		return -EINVAL;
 
-	if ((queue->flags & NFQA_CFG_F_GSO) || !skb_is_gso(entry->skb))
-		return __nfqnl_enqueue_packet(net, queue, entry);
-
 	skb = entry->skb;
 
 	switch (entry->pf) {
@@ -651,6 +648,9 @@ nfqnl_enqueue_packet(struct nf_queue_entry *entry, unsigned int queuenum)
 		break;
 	}
 
+	if ((queue->flags & NFQA_CFG_F_GSO) || !skb_is_gso(skb))
+		return __nfqnl_enqueue_packet(net, queue, entry);
+
 	nf_bridge_adjust_skb_data(skb);
 	segs = skb_gso_segment(skb, 0);
 	/* Does not use PTR_ERR to limit the number of error codes that can be
-- 
1.7.10.4

^ permalink raw reply related	[flat|nested] 18+ messages in thread

* [PATCH 5/5] ipvs: info leak in __ip_vs_get_dest_entries()
  2013-06-10 16:07 [PATCH 0/5] netfilter fixes for 3.10-rc5 Pablo Neira Ayuso
                   ` (3 preceding siblings ...)
  2013-06-10 16:07 ` [PATCH 4/5] netfilter: nfnetlink_queue: fix missing HW protocol Pablo Neira Ayuso
@ 2013-06-10 16:07 ` Pablo Neira Ayuso
  2013-06-10 20:32 ` [PATCH 0/5] netfilter fixes for 3.10-rc5 David Miller
  5 siblings, 0 replies; 18+ messages in thread
From: Pablo Neira Ayuso @ 2013-06-10 16:07 UTC (permalink / raw)
  To: netfilter-devel; +Cc: davem, netdev

From: Dan Carpenter <dan.carpenter@oracle.com>

The entry struct has a 2 byte hole after ->port and another 4 byte
hole after ->stats.outpkts.  You must have CAP_NET_ADMIN in your
namespace to hit this information leak.

Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com>
Acked-by: Julian Anastasov <ja@ssi.bg>
Signed-off-by: Simon Horman <horms@verge.net.au>
Signed-off-by: Pablo Neira Ayuso <pablo@netfilter.org>
---
 net/netfilter/ipvs/ip_vs_ctl.c |    1 +
 1 file changed, 1 insertion(+)

diff --git a/net/netfilter/ipvs/ip_vs_ctl.c b/net/netfilter/ipvs/ip_vs_ctl.c
index 5b142fb..9e6c2a0 100644
--- a/net/netfilter/ipvs/ip_vs_ctl.c
+++ b/net/netfilter/ipvs/ip_vs_ctl.c
@@ -2542,6 +2542,7 @@ __ip_vs_get_dest_entries(struct net *net, const struct ip_vs_get_dests *get,
 		struct ip_vs_dest *dest;
 		struct ip_vs_dest_entry entry;
 
+		memset(&entry, 0, sizeof(entry));
 		list_for_each_entry(dest, &svc->destinations, n_list) {
 			if (count >= get->num_dests)
 				break;
-- 
1.7.10.4

^ permalink raw reply related	[flat|nested] 18+ messages in thread

* Re: [PATCH 0/5] netfilter fixes for 3.10-rc5
  2013-06-10 16:07 [PATCH 0/5] netfilter fixes for 3.10-rc5 Pablo Neira Ayuso
                   ` (4 preceding siblings ...)
  2013-06-10 16:07 ` [PATCH 5/5] ipvs: info leak in __ip_vs_get_dest_entries() Pablo Neira Ayuso
@ 2013-06-10 20:32 ` David Miller
  5 siblings, 0 replies; 18+ messages in thread
From: David Miller @ 2013-06-10 20:32 UTC (permalink / raw)
  To: pablo; +Cc: netfilter-devel, netdev

From: Pablo Neira Ayuso <pablo@netfilter.org>
Date: Mon, 10 Jun 2013 18:07:36 +0200

> The following patchset contains four fixes for Netfilter and one fix
> for IPVS, they are:
> 
> * Fix data leak to user-space via getsockopt IP_VS_SO_GET_DESTS, from
>   Dan Carpenter.
> 
> * Fix xt_TCPMSS if no TCP MSS is specified in syn packets, to avoid the
>   violation of RFC879, from Phil Oester.
> 
> * Fix incomplete dump of objects via nfnetlink_acct and nfnetlink_cttimeout,
>   from myself.
> 
> * Fix missing HW protocol in packets passed to user-space via NFQUEUE,
>   from myself.
> 
> You can pull these changes from:
> 
> git://git.kernel.org/pub/scm/linux/kernel/git/pablo/nf.git master

Pulled, thanks Pablo!

^ permalink raw reply	[flat|nested] 18+ messages in thread

* RE: [PATCH 3/5] netfilter: xt_TCPMSS: Fix violation of RFC879 in absence of MSS option
  2013-06-10 16:07 ` [PATCH 3/5] netfilter: xt_TCPMSS: Fix violation of RFC879 in absence of MSS option Pablo Neira Ayuso
@ 2013-06-11  8:43   ` David Laight
  2013-06-10  7:19     ` Phil Oester
  2013-06-11 18:36   ` John Heffner
  1 sibling, 1 reply; 18+ messages in thread
From: David Laight @ 2013-06-11  8:43 UTC (permalink / raw)
  To: Pablo Neira Ayuso, netfilter-devel; +Cc: davem, netdev

> The clamp-mss-to-pmtu option of the xt_TCPMSS target can cause issues
> connecting to websites if there was no MSS option present in the
> original SYN packet from the client. In these cases, it may add a
> MSS higher than the default specified in RFC879. Fix this by never
> setting a value > 536 if no MSS option was specified by the client.
> 
> This closes netfilter's bugzilla #662.
...
> +	/* RFC 879 states that the default MSS is 536 without specific
> +	 * knowledge that the destination host is prepared to accept larger.
> +	 * Since no MSS was provided, we MUST NOT set a value > 536.
> +	 */
> +	newmss = min(newmss, (u16)536);
> +

Is setting the mss to 536 actually ever sensible?
RFC 879 might say that it is the default (and the minimum
that must be supported), but in practise the actual mss
is very likely to be only slightly shorter than the standard
ethernet mss.
Although strict conformance with RFC 879 might require the mss
be clamped to 536, pragmatically a value much nearer 1400 would
make sense - systems with very low mtu/mss are probably likely
to advertise it.

	David




^ permalink raw reply	[flat|nested] 18+ messages in thread

* RE: [PATCH 3/5] netfilter: xt_TCPMSS: Fix violation of RFC879 in absence of MSS option
  2013-06-10  7:19     ` Phil Oester
@ 2013-06-11 15:00       ` David Laight
  2013-06-10  8:27         ` Phil Oester
  0 siblings, 1 reply; 18+ messages in thread
From: David Laight @ 2013-06-11 15:00 UTC (permalink / raw)
  To: Phil Oester; +Cc: Pablo Neira Ayuso, netfilter-devel, davem, netdev

> On Tue, Jun 11, 2013 at 09:43:07AM +0100, David Laight wrote:
> > Is setting the mss to 536 actually ever sensible?
> > RFC 879 might say that it is the default (and the minimum
> > that must be supported), but in practise the actual mss
> > is very likely to be only slightly shorter than the standard
> > ethernet mss.
> > Although strict conformance with RFC 879 might require the mss
> > be clamped to 536, pragmatically a value much nearer 1400 would
> > make sense - systems with very low mtu/mss are probably likely
> > to advertise it.
> 
> Read the associated bugzilla - there was at least one real world
> example where setting a higher MSS was causing breakage.
> 
> Phil
> 
> https://bugzilla.netfilter.org/show_bug.cgi?id=662

To quote that bug:

I stumbled upon this problem in debian bug #541658[1] ("[iceweasel] cannot open 
research.microsoft.com" - only worth reading for entertainment purposes) and,
after that bug was closed, analysed it in my blog[2] until a friend of mine
found out why the page loads when clamping mss to pmtu is disabled or
restricted to a range (like with "iptables -A FORWARD -p tcp --tcp-flags
SYN,RST SYN -m tcpmss --mss 1400:1536 -j TCPMSS --clamp-mss-to-pmtu") but
doesn't load with "simple" clamping. His really great and detailed analysation
of the problem may be seen at [3].

If I read/understand that correctly, clamping to 1400 worked - there was
no need to clamp all the way down to 536.

	David

^ permalink raw reply	[flat|nested] 18+ messages in thread

* RE: [PATCH 3/5] netfilter: xt_TCPMSS: Fix violation of RFC879 in absence of MSS option
  2013-06-10  8:27         ` Phil Oester
@ 2013-06-11 16:09           ` David Laight
  2013-06-11 16:25             ` Pablo Neira Ayuso
  0 siblings, 1 reply; 18+ messages in thread
From: David Laight @ 2013-06-11 16:09 UTC (permalink / raw)
  To: Phil Oester; +Cc: Pablo Neira Ayuso, netfilter-devel, davem, netdev

> On Tue, Jun 11, 2013 at 04:00:06PM +0100, David Laight wrote:
> > To quote that bug:
> >
> > I stumbled upon this problem in debian bug #541658[1] ("[iceweasel] cannot open
> > research.microsoft.com" - only worth reading for entertainment purposes) and,
> > after that bug was closed, analysed it in my blog[2] until a friend of mine
> > found out why the page loads when clamping mss to pmtu is disabled or
> > restricted to a range (like with "iptables -A FORWARD -p tcp --tcp-flags
> > SYN,RST SYN -m tcpmss --mss 1400:1536 -j TCPMSS --clamp-mss-to-pmtu") but
> > doesn't load with "simple" clamping. His really great and detailed analysation
> > of the problem may be seen at [3].
> >
> > If I read/understand that correctly, clamping to 1400 worked - there was
> > no need to clamp all the way down to 536.
> 
> You are not understanding the issue correctly.  The reason the command worked with
> "-m tcpmss --mss 1400:1536" is because that implies an MSS option was provided.
> The issue occurs only when NO MSS option is sent.  In these cases, we cannot
> ASSUME that it is ok to use some arbitrarily high value (1400 as you propose).
> The RFC is clear on this point.

My problem is that I don't want TCP connections to drop the mss to
536 when talking to minimal/old implementations that don't add any
options to SYN packets.

	David

^ permalink raw reply	[flat|nested] 18+ messages in thread

* Re: [PATCH 3/5] netfilter: xt_TCPMSS: Fix violation of RFC879 in absence of MSS option
  2013-06-11 16:09           ` David Laight
@ 2013-06-11 16:25             ` Pablo Neira Ayuso
  2013-06-11 18:00               ` Jeff Haran
  0 siblings, 1 reply; 18+ messages in thread
From: Pablo Neira Ayuso @ 2013-06-11 16:25 UTC (permalink / raw)
  To: David Laight; +Cc: Phil Oester, netfilter-devel, davem, netdev

On Tue, Jun 11, 2013 at 05:09:11PM +0100, David Laight wrote:
> > On Tue, Jun 11, 2013 at 04:00:06PM +0100, David Laight wrote:
> > > To quote that bug:
> > >
> > > I stumbled upon this problem in debian bug #541658[1] ("[iceweasel] cannot open
> > > research.microsoft.com" - only worth reading for entertainment purposes) and,
> > > after that bug was closed, analysed it in my blog[2] until a friend of mine
> > > found out why the page loads when clamping mss to pmtu is disabled or
> > > restricted to a range (like with "iptables -A FORWARD -p tcp --tcp-flags
> > > SYN,RST SYN -m tcpmss --mss 1400:1536 -j TCPMSS --clamp-mss-to-pmtu") but
> > > doesn't load with "simple" clamping. His really great and detailed analysation
> > > of the problem may be seen at [3].
> > >
> > > If I read/understand that correctly, clamping to 1400 worked - there was
> > > no need to clamp all the way down to 536.
> > 
> > You are not understanding the issue correctly.  The reason the command worked with
> > "-m tcpmss --mss 1400:1536" is because that implies an MSS option was provided.
> > The issue occurs only when NO MSS option is sent.  In these cases, we cannot
> > ASSUME that it is ok to use some arbitrarily high value (1400 as you propose).
> > The RFC is clear on this point.
> 
> My problem is that I don't want TCP connections to drop the mss to
> 536 when talking to minimal/old implementations that don't add any
> options to SYN packets.

That will not happen if you use:

... -m tcpmss --mss 1400:1536 ...

as in your example above.

^ permalink raw reply	[flat|nested] 18+ messages in thread

* RE: [PATCH 3/5] netfilter: xt_TCPMSS: Fix violation of RFC879 in absence of MSS option
  2013-06-11 16:25             ` Pablo Neira Ayuso
@ 2013-06-11 18:00               ` Jeff Haran
  2013-06-11 18:14                 ` Rick Jones
  0 siblings, 1 reply; 18+ messages in thread
From: Jeff Haran @ 2013-06-11 18:00 UTC (permalink / raw)
  To: Pablo Neira Ayuso, David Laight
  Cc: Phil Oester, netfilter-devel, davem, netdev

> -----Original Message-----
> From: netfilter-devel-owner@vger.kernel.org [mailto:netfilter-devel-owner@vger.kernel.org] On Behalf Of Pablo Neira Ayuso
> Sent: Tuesday, June 11, 2013 9:25 AM
> To: David Laight
> Cc: Phil Oester; netfilter-devel@vger.kernel.org; davem@davemloft.net; netdev@vger.kernel.org
> Subject: Re: [PATCH 3/5] netfilter: xt_TCPMSS: Fix violation of RFC879 in absence of MSS option
> 
> On Tue, Jun 11, 2013 at 05:09:11PM +0100, David Laight wrote:
> > > On Tue, Jun 11, 2013 at 04:00:06PM +0100, David Laight wrote:
> > > > To quote that bug:
> > > >
> > > > I stumbled upon this problem in debian bug #541658[1] ("[iceweasel] cannot open
> > > > research.microsoft.com" - only worth reading for entertainment purposes) and,
> > > > after that bug was closed, analysed it in my blog[2] until a friend of mine
> > > > found out why the page loads when clamping mss to pmtu is disabled or
> > > > restricted to a range (like with "iptables -A FORWARD -p tcp --tcp-flags
> > > > SYN,RST SYN -m tcpmss --mss 1400:1536 -j TCPMSS --clamp-mss-to-pmtu") but
> > > > doesn't load with "simple" clamping. His really great and detailed analysation
> > > > of the problem may be seen at [3].
> > > >
> > > > If I read/understand that correctly, clamping to 1400 worked - there was
> > > > no need to clamp all the way down to 536.
> > >
> > > You are not understanding the issue correctly.  The reason the command worked with
> > > "-m tcpmss --mss 1400:1536" is because that implies an MSS option was provided.
> > > The issue occurs only when NO MSS option is sent.  In these cases, we cannot
> > > ASSUME that it is ok to use some arbitrarily high value (1400 as you propose).
> > > The RFC is clear on this point.
> >
> > My problem is that I don't want TCP connections to drop the mss to
> > 536 when talking to minimal/old implementations that don't add any
> > options to SYN packets.
> 
> That will not happen if you use:
> 
> ... -m tcpmss --mss 1400:1536 ...
> 
> as in your example above.

There is likely no "right" answer for this issue, but for what it's worth I can report a somewhat related experience I had a few years ago when updating my PC at home with a new Suse distro. The machine had been running previous versions of Suse with my Comcast cable internet service just fine for several years. I downloaded the ISO from opensuse, burned the DVD, did the install but when it came back up again the internet service was quite unreliable. It would work for a few minutes, but then no packets would flow. I happened to run ifconfig eth0 and it showed an MTU of 576. It seems the version of the DHCP client that came with the new distro honored the DHCP MTU option, but Comcast was advertising DHCP offers with an MTU of 576.

Problem was, the cable modem that Comcast supplied apparently didn't like that short MTU. I found that if I power cycled the modem, packets would flow again, for a little while. When I modified the configuration of the DHCP client to ignore the MTU option and use a fixed 1500, it started working fine like it did before. I suppose the Windows versions of the time ignored the MTU option by default, so this was not something that impacted most of Comcast's users.

The moral of the story for me was that despite what the RFC says, the de facto default MTU of the Internet is 1500 and MSS's derived from shorter MTUs are likely to cause interoperability problems.

Jeff Haran


^ permalink raw reply	[flat|nested] 18+ messages in thread

* Re: [PATCH 3/5] netfilter: xt_TCPMSS: Fix violation of RFC879 in absence of MSS option
  2013-06-11 18:00               ` Jeff Haran
@ 2013-06-11 18:14                 ` Rick Jones
  2013-06-11 18:31                   ` Jeff Haran
  0 siblings, 1 reply; 18+ messages in thread
From: Rick Jones @ 2013-06-11 18:14 UTC (permalink / raw)
  To: Jeff Haran
  Cc: Pablo Neira Ayuso, David Laight, Phil Oester, netfilter-devel,
	davem, netdev

> There is likely no "right" answer for this issue, but for what it's
> worth I can report a somewhat related experience I had a few years
> ago when updating my PC at home with a new Suse distro. The machine
> had been running previous versions of Suse with my Comcast cable
> internet service just fine for several years. I downloaded the ISO
> from opensuse, burned the DVD, did the install but when it came back
> up again the internet service was quite unreliable. It would work for
> a few minutes, but then no packets would flow. I happened to run
> ifconfig eth0 and it showed an MTU of 576. It seems the version of
> the DHCP client that came with the new distro honored the DHCP MTU
> option, but Comcast was advertising DHCP offers with an MTU of 576.

Presumably then, your system rejected any incoming packet which was 
larger than the 576 byte MTU it got from the Comcast DHCP server..

I can think of two reasons for larger packets to be arriving at your 
system then:

1) UDP

2) Broken TCPs ass-u-me-ing a TCP MSS larger than 536 bytes when there 
wasn't an MSS option in the SYN(s).

Did your SuSE system send actual TCP MSS options based on the 576 byte MTU?


rick jones

^ permalink raw reply	[flat|nested] 18+ messages in thread

* RE: [PATCH 3/5] netfilter: xt_TCPMSS: Fix violation of RFC879 in absence of MSS option
  2013-06-11 18:14                 ` Rick Jones
@ 2013-06-11 18:31                   ` Jeff Haran
  2013-06-21  8:27                     ` Jan Engelhardt
  0 siblings, 1 reply; 18+ messages in thread
From: Jeff Haran @ 2013-06-11 18:31 UTC (permalink / raw)
  To: 'Rick Jones'
  Cc: Pablo Neira Ayuso, David Laight, Phil Oester, netfilter-devel,
	davem, netdev

> -----Original Message-----
> From: netfilter-devel-owner@vger.kernel.org [mailto:netfilter-devel-owner@vger.kernel.org] On Behalf Of Rick Jones
> Sent: Tuesday, June 11, 2013 11:14 AM
> To: Jeff Haran
> Cc: Pablo Neira Ayuso; David Laight; Phil Oester; netfilter-devel@vger.kernel.org; davem@davemloft.net; netdev@vger.kernel.org
> Subject: Re: [PATCH 3/5] netfilter: xt_TCPMSS: Fix violation of RFC879 in absence of MSS option
> 
> > There is likely no "right" answer for this issue, but for what it's
> > worth I can report a somewhat related experience I had a few years
> > ago when updating my PC at home with a new Suse distro. The machine
> > had been running previous versions of Suse with my Comcast cable
> > internet service just fine for several years. I downloaded the ISO
> > from opensuse, burned the DVD, did the install but when it came back
> > up again the internet service was quite unreliable. It would work for
> > a few minutes, but then no packets would flow. I happened to run
> > ifconfig eth0 and it showed an MTU of 576. It seems the version of
> > the DHCP client that came with the new distro honored the DHCP MTU
> > option, but Comcast was advertising DHCP offers with an MTU of 576.
> 
> Presumably then, your system rejected any incoming packet which was
> larger than the 576 byte MTU it got from the Comcast DHCP server..
> 
> I can think of two reasons for larger packets to be arriving at your
> system then:
> 
> 1) UDP
> 
> 2) Broken TCPs ass-u-me-ing a TCP MSS larger than 536 bytes when there
> wasn't an MSS option in the SYN(s).
> 
> Did your SuSE system send actual TCP MSS options based on the 576 byte MTU?
> 
> 
> rick jones

I didn't debug it further so I can't answer that question. But the problem was clearly in the cable modem. Power cycling the modem brought back connectivity, for a little while. I was able to muddle through this since I work with this stuff for a living, but if I had been an IP-unsavvy Joe Sixpack who was trying out a Linux live CD on his home PC as an alternative to Windows, I suspect I would have given up in disgust.

Something to think about for those of us who would like to see wider adoption of Linux on the PC desktop.

Jeff Haran


^ permalink raw reply	[flat|nested] 18+ messages in thread

* Re: [PATCH 3/5] netfilter: xt_TCPMSS: Fix violation of RFC879 in absence of MSS option
  2013-06-10 16:07 ` [PATCH 3/5] netfilter: xt_TCPMSS: Fix violation of RFC879 in absence of MSS option Pablo Neira Ayuso
  2013-06-11  8:43   ` David Laight
@ 2013-06-11 18:36   ` John Heffner
  1 sibling, 0 replies; 18+ messages in thread
From: John Heffner @ 2013-06-11 18:36 UTC (permalink / raw)
  To: Pablo Neira Ayuso; +Cc: netfilter-devel, David Miller, Netdev

While it's not likely to harm anything, this isn't quite right for
IPv6, with a lower bound MTU of 1280.

Thanks,
  -John

(Sorry, re-sending as gmail silently turned on HTML formatting.)



On Mon, Jun 10, 2013 at 12:07 PM, Pablo Neira Ayuso <pablo@netfilter.org> wrote:
>
> From: Phil Oester <kernel@linuxace.com>
>
> The clamp-mss-to-pmtu option of the xt_TCPMSS target can cause issues
> connecting to websites if there was no MSS option present in the
> original SYN packet from the client. In these cases, it may add a
> MSS higher than the default specified in RFC879. Fix this by never
> setting a value > 536 if no MSS option was specified by the client.
>
> This closes netfilter's bugzilla #662.
>
> Signed-off-by: Phil Oester <kernel@linuxace.com>
> Signed-off-by: Pablo Neira Ayuso <pablo@netfilter.org>
> ---
>  net/netfilter/xt_TCPMSS.c |    6 ++++++
>  1 file changed, 6 insertions(+)
>
> diff --git a/net/netfilter/xt_TCPMSS.c b/net/netfilter/xt_TCPMSS.c
> index a75240f..afaebc7 100644
> --- a/net/netfilter/xt_TCPMSS.c
> +++ b/net/netfilter/xt_TCPMSS.c
> @@ -125,6 +125,12 @@ tcpmss_mangle_packet(struct sk_buff *skb,
>
>         skb_put(skb, TCPOLEN_MSS);
>
> +       /* RFC 879 states that the default MSS is 536 without specific
> +        * knowledge that the destination host is prepared to accept larger.
> +        * Since no MSS was provided, we MUST NOT set a value > 536.
> +        */
> +       newmss = min(newmss, (u16)536);
> +
>         opt = (u_int8_t *)tcph + sizeof(struct tcphdr);
>         memmove(opt + TCPOLEN_MSS, opt, tcplen - sizeof(struct tcphdr));
>
> --
> 1.7.10.4
>
> --
> To unsubscribe from this list: send the line "unsubscribe netdev" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

^ permalink raw reply	[flat|nested] 18+ messages in thread

* RE: [PATCH 3/5] netfilter: xt_TCPMSS: Fix violation of RFC879 in absence of MSS option
  2013-06-11 18:31                   ` Jeff Haran
@ 2013-06-21  8:27                     ` Jan Engelhardt
  0 siblings, 0 replies; 18+ messages in thread
From: Jan Engelhardt @ 2013-06-21  8:27 UTC (permalink / raw)
  To: Jeff Haran
  Cc: 'Rick Jones',
	Pablo Neira Ayuso, David Laight, Phil Oester, netfilter-devel,
	davem, netdev


On Tuesday 2013-06-11 20:31, Jeff Haran wrote:
>> > It seems the version of
>> > the DHCP client that came with the new distro honored the DHCP MTU
>> > option, but Comcast was advertising DHCP offers with an MTU of 576.
>>
>> Did your SuSE system send actual TCP MSS options based on the 576
>> byte MTU?

Yes and it makes for horrible performance. In fact, the "Unitymedia"
cable provider in Germany does exactly the same stupid thing, sending
MTU=576 in their DHCP responses, despite the link actually being
capable of MTU 1500 ("capable" as in, not returning a Fragmentation
Needed ICMP). The only way to get around this provider's idiocy is to
manually set MTU=1500 in SUSE's network config, which overrides the
DHCP values. The way it works is probably by way of using the
scripting functions of prominent DHCP and VPN clients (the C programs
only does the network job and otherwise calls /bin/sh-type logic to
actually modify the interface parameters) like dhclient,dhcpcd,vpnc.


>> Presumably then, your system rejected any incoming packet which was
>> larger than the 576 byte MTU it got from the Comcast DHCP server..

The MTU is not used to block incoming packets. That would not make
sense either (because you already have received the packet and
therefore can use it). In fact, TCP-receive-offloading hardware may
even pass to the kernel packets that are larger than both the output
MTU of your own system as well as larger as the output MTU of the
router you got it from.

^ permalink raw reply	[flat|nested] 18+ messages in thread

end of thread, other threads:[~2013-06-21  8:27 UTC | newest]

Thread overview: 18+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2013-06-10 16:07 [PATCH 0/5] netfilter fixes for 3.10-rc5 Pablo Neira Ayuso
2013-06-10 16:07 ` [PATCH 1/5] netfilter: nfnetlink_acct: fix incomplete dumping of objects Pablo Neira Ayuso
2013-06-10 16:07 ` [PATCH 2/5] netfilter: nfnetlink_cttimeout: " Pablo Neira Ayuso
2013-06-10 16:07 ` [PATCH 3/5] netfilter: xt_TCPMSS: Fix violation of RFC879 in absence of MSS option Pablo Neira Ayuso
2013-06-11  8:43   ` David Laight
2013-06-10  7:19     ` Phil Oester
2013-06-11 15:00       ` David Laight
2013-06-10  8:27         ` Phil Oester
2013-06-11 16:09           ` David Laight
2013-06-11 16:25             ` Pablo Neira Ayuso
2013-06-11 18:00               ` Jeff Haran
2013-06-11 18:14                 ` Rick Jones
2013-06-11 18:31                   ` Jeff Haran
2013-06-21  8:27                     ` Jan Engelhardt
2013-06-11 18:36   ` John Heffner
2013-06-10 16:07 ` [PATCH 4/5] netfilter: nfnetlink_queue: fix missing HW protocol Pablo Neira Ayuso
2013-06-10 16:07 ` [PATCH 5/5] ipvs: info leak in __ip_vs_get_dest_entries() Pablo Neira Ayuso
2013-06-10 20:32 ` [PATCH 0/5] netfilter fixes for 3.10-rc5 David Miller

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.