[1/1] netfilter: Ignore bogus SACK option values in TCP conntrack

The netfilter TCP conntrack used to mark packets with bogus SACK option
values as INVALID. However, it seems broken SEQ anonymizers, which are
responsible for such traffic, are not going to die out soon and conntrack
effectively blocks traffic coming through such devices.

Better be liberal at conntrack level: when SACK is bogus, ignore it.

Signed-off-by: Jozsef Kadlecsik <kadlec@blackhole.kfki.hu>
Reported-by: Corey Hickey <bugfood-ml@fatooh.org>
Tested-by: Corey Hickey <bugfood-ml@fatooh.org>
---
 net/netfilter/nf_conntrack_proto_tcp.c |    5 +++++
 1 files changed, 5 insertions(+), 0 deletions(-)

On 2013-09-02 11:58, Jozsef Kadlecsik wrote:
> The netfilter TCP conntrack used to mark packets with bogus SACK option
> values as INVALID. However, it seems broken SEQ anonymizers, which are
> responsible for such traffic, are not going to die out soon and conntrack
> effectively blocks traffic coming through such devices.
> 
> Better be liberal at conntrack level: when SACK is bogus, ignore it.
> 
> Signed-off-by: Jozsef Kadlecsik <kadlec@blackhole.kfki.hu>
> Reported-by: Corey Hickey <bugfood-ml@fatooh.org>
> Tested-by: Corey Hickey <bugfood-ml@fatooh.org>
> ---
>  net/netfilter/nf_conntrack_proto_tcp.c |    5 +++++
>  1 files changed, 5 insertions(+), 0 deletions(-)
> 
> diff --git a/net/netfilter/nf_conntrack_proto_tcp.c b/net/netfilter/nf_conntrack_proto_tcp.c
> index 44d1ea3..cd67de8 100644
> --- a/net/netfilter/nf_conntrack_proto_tcp.c
> +++ b/net/netfilter/nf_conntrack_proto_tcp.c
> @@ -640,6 +640,11 @@ static bool tcp_in_window(const struct nf_conn *ct,
>  	in_recv_win = !receiver->td_maxwin ||
>  		      after(end, sender->td_end - receiver->td_maxwin - 1);
>  
> +	/* Fall back to ACK when SACK is bogus */
> +	if (!(before(sack, receiver->td_end + 1) &&
> +	      after(sack, receiver->td_end - MAXACKWINDOW(sender) - 1)))
> +		sack = ack;
> +
>  	pr_debug("tcp_in_window: I=%i II=%i III=%i IV=%i\n",
>  		 before(seq, sender->td_maxend + 1),
>  		 (in_recv_win ? 1 : 0),
> 


I've been running Jozsef's patch on linux-next compiled on 2013-08-19. I
can confirm that it fixes the problem I'm having with broken SEQ
randomization on FWSM, and I haven't seen any problems caused by it.

http://www.spinics.net/lists/netdev/msg246898.html

I'm still planning to work with my boss to disable SEQ randomization,
hopefully this week.

Thanks,
Corey
--
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

On Mon, Sep 02, 2013 at 08:58:00PM +0200, Jozsef Kadlecsik wrote:
> The netfilter TCP conntrack used to mark packets with bogus SACK option
> values as INVALID. However, it seems broken SEQ anonymizers, which are
> responsible for such traffic, are not going to die out soon and conntrack
> effectively blocks traffic coming through such devices.
> 
> Better be liberal at conntrack level: when SACK is bogus, ignore it.

But if conntrack were being "liberal" then it wouldn't care about the value of
the ACKs either, no?  This sort of defeats the purpose of TCP window tracking.
At the very least, this workaround should be dependent upon
nf_conntrack_tcp_be_liberal != 0.  

Also note that David Miller refused to accept a patch working around this
issue in the TCP stack [1].  Why should netfilter do so?

Phil


[1] http://marc.info/?l=linux-netdev&m=137714232805063&w=2
--
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

On Mon, 2 Sep 2013, Phil Oester wrote:

> On Mon, Sep 02, 2013 at 08:58:00PM +0200, Jozsef Kadlecsik wrote:
> > The netfilter TCP conntrack used to mark packets with bogus SACK option
> > values as INVALID. However, it seems broken SEQ anonymizers, which are
> > responsible for such traffic, are not going to die out soon and conntrack
> > effectively blocks traffic coming through such devices.
> > 
> > Better be liberal at conntrack level: when SACK is bogus, ignore it.
> 
> But if conntrack were being "liberal" then it wouldn't care about the 
> value of the ACKs either, no?  This sort of defeats the purpose of TCP 
> window tracking.

No, it doesn't defeat it - we fall back to checking the ACK value against 
the window.

> At the very least, this workaround should be dependent upon 
> nf_conntrack_tcp_be_liberal != 0.
> 
> Also note that David Miller refused to accept a patch working around this
> issue in the TCP stack [1].  Why should netfilter do so?
> 
> [1] http://marc.info/?l=linux-netdev&m=137714232805063&w=2

The purpose of that patch is to get back as much performance of TCP as 
possible, by working around the broken SACK options.

This patch lets the traffic at least through, otherwise it's simply 
blocked by conntrack. Similarly to the TCP stack, conntrack should ignore 
bogus SACK values instead of effectively dropping the stream.

This is a long time issue. To be honest, I believed such anonymizer 
devices would have disappeared (fixed) by now. However it is apparently 
not so and at the same time conntrack actually breaks TCP robustness. 
Therefore I think it should be fixed.

Best regards,
Jozsef
-
E-mail  : kadlec@blackhole.kfki.hu, kadlecsik.jozsef@wigner.mta.hu
PGP key : http://www.kfki.hu/~kadlec/pgp_public_key.txt
Address : Wigner Research Centre for Physics, Hungarian Academy of Sciences
          H-1525 Budapest 114, POB. 49, Hungary
--
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

On Tue, Sep 03, 2013 at 09:31:19AM +0200, Jozsef Kadlecsik wrote:
> On Mon, 2 Sep 2013, Phil Oester wrote:
> > But if conntrack were being "liberal" then it wouldn't care about the 
> > value of the ACKs either, no?  This sort of defeats the purpose of TCP 
> > window tracking.
> 
> No, it doesn't defeat it - we fall back to checking the ACK value against 
> the window.
> 
> > At the very least, this workaround should be dependent upon 
> > nf_conntrack_tcp_be_liberal != 0.
> > 
> > Also note that David Miller refused to accept a patch working around this
> > issue in the TCP stack [1].  Why should netfilter do so?
> > 
> > [1] http://marc.info/?l=linux-netdev&m=137714232805063&w=2
> 
> The purpose of that patch is to get back as much performance of TCP as 
> possible, by working around the broken SACK options.
> 
> This patch lets the traffic at least through, otherwise it's simply 
> blocked by conntrack. Similarly to the TCP stack, conntrack should ignore 
> bogus SACK values instead of effectively dropping the stream.
> 
> This is a long time issue. To be honest, I believed such anonymizer 
> devices would have disappeared (fixed) by now. However it is apparently 
> not so and at the same time conntrack actually breaks TCP robustness. 
> Therefore I think it should be fixed.

This should still only be allowed if nf_conntrack_tcp_be_liberal is enabled
IMHO (or some new sysctl like nf_conntrack_tcp_sack_be_liberal?).  Personally
I don't care about these broken middleboxes, and would rather not have SACK
validation disabled by default.  

Phil
--
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

On Wed, 4 Sep 2013, Phil Oester wrote:

> On Tue, Sep 03, 2013 at 09:31:19AM +0200, Jozsef Kadlecsik wrote:
> > On Mon, 2 Sep 2013, Phil Oester wrote:
> > > But if conntrack were being "liberal" then it wouldn't care about the 
> > > value of the ACKs either, no?  This sort of defeats the purpose of TCP 
> > > window tracking.
> > 
> > No, it doesn't defeat it - we fall back to checking the ACK value against 
> > the window.
> > 
> > > At the very least, this workaround should be dependent upon 
> > > nf_conntrack_tcp_be_liberal != 0.
> > > 
> > > Also note that David Miller refused to accept a patch working around this
> > > issue in the TCP stack [1].  Why should netfilter do so?
> > > 
> > > [1] http://marc.info/?l=linux-netdev&m=137714232805063&w=2
> > 
> > The purpose of that patch is to get back as much performance of TCP as 
> > possible, by working around the broken SACK options.
> > 
> > This patch lets the traffic at least through, otherwise it's simply 
> > blocked by conntrack. Similarly to the TCP stack, conntrack should ignore 
> > bogus SACK values instead of effectively dropping the stream.
> > 
> > This is a long time issue. To be honest, I believed such anonymizer 
> > devices would have disappeared (fixed) by now. However it is apparently 
> > not so and at the same time conntrack actually breaks TCP robustness. 
> > Therefore I think it should be fixed.
> 
> This should still only be allowed if nf_conntrack_tcp_be_liberal is 
> enabled IMHO (or some new sysctl like 
> nf_conntrack_tcp_sack_be_liberal?).  Personally I don't care about these 
> broken middleboxes, and would rather not have SACK validation disabled 
> by default.

nf_conntrack_tcp_be_liberal disables window tracking completely, including 
SACK validation too.

I have been thinking on this from time to time and couldn't come up with a 
solution which is satisfying: even if an nf_conntrack_tcp_sack_be_liberal
flag is added to the patch, if it's default off, then that's almost the 
same situations as we have at the present.

Unless someone can back the patch with good arguments, I say forget about 
it.

Best regards,
Jozsef
-
E-mail  : kadlec@blackhole.kfki.hu, kadlecsik.jozsef@wigner.mta.hu
PGP key : http://www.kfki.hu/~kadlec/pgp_public_key.txt
Address : Wigner Research Centre for Physics, Hungarian Academy of Sciences
          H-1525 Budapest 114, POB. 49, Hungary
--
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

On Fri, Sep 13, 2013 at 10:16:14PM +0200, Jozsef Kadlecsik wrote:
> I have been thinking on this from time to time and couldn't come up with a 
> solution which is satisfying: even if an nf_conntrack_tcp_sack_be_liberal
> flag is added to the patch, if it's default off, then that's almost the 
> same situations as we have at the present.

With the additional sysctl, at least the 0.1% of admins which are bit by
these braindead anonymizer boxes would have the option of working around
them without completely disabling TCP window tracking.  

Phil
--
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