[v2] netfilter: synproxy: erroneous TCP mss option fixed.
diff mbox series

Message ID 1561441324-19193-1-git-send-email-ibrahim.ercan@labristeknoloji.com
State Changes Requested
Delegated to: Pablo Neira
Headers show
Series
  • [v2] netfilter: synproxy: erroneous TCP mss option fixed.
Related show

Commit Message

Ibrahim Ercan June 25, 2019, 5:42 a.m. UTC
Syn proxy isn't setting mss value correctly on client syn-ack packet.
It was sending same mss value with client send instead of the value user set in iptables rule. This patch fix that wrong behavior by passing client mss information to synproxy_send_client_synack correctly.

Signed-off-by: Ibrahim Ercan <ibrahim.ercan@labristeknoloji.com>
---
 net/ipv4/netfilter/ipt_SYNPROXY.c  | 9 ++++++---
 net/ipv6/netfilter/ip6t_SYNPROXY.c | 9 ++++++---
 2 files changed, 12 insertions(+), 6 deletions(-)

Comments

Pablo Neira Ayuso June 27, 2019, 6:57 p.m. UTC | #1
On Tue, Jun 25, 2019 at 08:42:04AM +0300, Ibrahim Ercan wrote:
> Syn proxy isn't setting mss value correctly on client syn-ack packet.
> It was sending same mss value with client send instead of the value user set in iptables rule. This patch fix that wrong behavior by passing client mss information to synproxy_send_client_synack correctly.
> 
> Signed-off-by: Ibrahim Ercan <ibrahim.ercan@labristeknoloji.com>
> ---
>  net/ipv4/netfilter/ipt_SYNPROXY.c  | 9 ++++++---
>  net/ipv6/netfilter/ip6t_SYNPROXY.c | 9 ++++++---
>  2 files changed, 12 insertions(+), 6 deletions(-)
> 
> diff --git a/net/ipv4/netfilter/ipt_SYNPROXY.c b/net/ipv4/netfilter/ipt_SYNPROXY.c
> index 64d9563..e0bd504 100644
> --- a/net/ipv4/netfilter/ipt_SYNPROXY.c
> +++ b/net/ipv4/netfilter/ipt_SYNPROXY.c
> @@ -69,13 +69,13 @@ synproxy_send_tcp(struct net *net,
>  static void
>  synproxy_send_client_synack(struct net *net,
>  			    const struct sk_buff *skb, const struct tcphdr *th,
> -			    const struct synproxy_options *opts)
> +			    const struct synproxy_options *opts, const u16 client_mssinfo)
>  {
>  	struct sk_buff *nskb;
>  	struct iphdr *iph, *niph;
>  	struct tcphdr *nth;
>  	unsigned int tcp_hdr_size;
> -	u16 mss = opts->mss;
> +	u16 mss = client_mssinfo;
>  
>  	iph = ip_hdr(skb);
>  
> @@ -264,6 +264,7 @@ synproxy_tg4(struct sk_buff *skb, const struct xt_action_param *par)
>  	struct synproxy_net *snet = synproxy_pernet(net);
>  	struct synproxy_options opts = {};
>  	struct tcphdr *th, _th;
> +	u16 client_mssinfo;
>  
>  	if (nf_ip_checksum(skb, xt_hooknum(par), par->thoff, IPPROTO_TCP))
>  		return NF_DROP;
> @@ -283,6 +284,8 @@ synproxy_tg4(struct sk_buff *skb, const struct xt_action_param *par)
>  			opts.options |= XT_SYNPROXY_OPT_ECN;
>  
>  		opts.options &= info->options;
> +		client_mssinfo = opts.mss;
> +		opts.mss = info->mss;

No need for this new client_mssinfo variable, right? I mean, you can
just set:

        opts.mss = info->mss;

and use it from synproxy_send_client_synack().

This patch will be smaller.

>  		if (opts.options & XT_SYNPROXY_OPT_TIMESTAMP)
>  			synproxy_init_timestamp_cookie(info, &opts);
>  		else
> @@ -290,7 +293,7 @@ synproxy_tg4(struct sk_buff *skb, const struct xt_action_param *par)
>  					  XT_SYNPROXY_OPT_SACK_PERM |
>  					  XT_SYNPROXY_OPT_ECN);
>  
> -		synproxy_send_client_synack(net, skb, th, &opts);
> +		synproxy_send_client_synack(net, skb, th, &opts, client_mssinfo);
>  		consume_skb(skb);
>  		return NF_STOLEN;
>  	} else if (th->ack && !(th->fin || th->rst || th->syn)) {
Florian Westphal June 27, 2019, 7 p.m. UTC | #2
Pablo Neira Ayuso <pablo@netfilter.org> wrote:
> >  		opts.options &= info->options;
> > +		client_mssinfo = opts.mss;
> > +		opts.mss = info->mss;
> 
> No need for this new client_mssinfo variable, right? I mean, you can
> just set:
> 
>         opts.mss = info->mss;
> 
> and use it from synproxy_send_client_synack().

I thought that as well but we need both mss values,
the one configured in the target (info->mss) and the
ine received from the peer.

The former is what we announce to peer in the syn/ack
(as tcp option), the latter is what we need to encode
in the syncookie (to decode it on cookie ack).
Pablo Neira Ayuso June 27, 2019, 7:08 p.m. UTC | #3
On Thu, Jun 27, 2019 at 09:00:19PM +0200, Florian Westphal wrote:
> Pablo Neira Ayuso <pablo@netfilter.org> wrote:
> > >  		opts.options &= info->options;
> > > +		client_mssinfo = opts.mss;
> > > +		opts.mss = info->mss;
> > 
> > No need for this new client_mssinfo variable, right? I mean, you can
> > just set:
> > 
> >         opts.mss = info->mss;
> > 
> > and use it from synproxy_send_client_synack().
> 
> I thought that as well but we need both mss values,
> the one configured in the target (info->mss) and the
> ine received from the peer.
> 
> The former is what we announce to peer in the syn/ack
> (as tcp option), the latter is what we need to encode
> in the syncookie (to decode it on cookie ack).

I see, probably place client_mss field into the synproxy_options
structure?
Florian Westphal June 27, 2019, 7:21 p.m. UTC | #4
Pablo Neira Ayuso <pablo@netfilter.org> wrote:
> On Thu, Jun 27, 2019 at 09:00:19PM +0200, Florian Westphal wrote:
> > Pablo Neira Ayuso <pablo@netfilter.org> wrote:
> > > >  		opts.options &= info->options;
> > > > +		client_mssinfo = opts.mss;
> > > > +		opts.mss = info->mss;
> > > 
> > > No need for this new client_mssinfo variable, right? I mean, you can
> > > just set:
> > > 
> > >         opts.mss = info->mss;
> > > 
> > > and use it from synproxy_send_client_synack().
> > 
> > I thought that as well but we need both mss values,
> > the one configured in the target (info->mss) and the
> > ine received from the peer.
> > 
> > The former is what we announce to peer in the syn/ack
> > (as tcp option), the latter is what we need to encode
> > in the syncookie (to decode it on cookie ack).
> 
> I see, probably place client_mss field into the synproxy_options
> structure?

I worked on a fix for this too (Ibrahim was faster), I
tried to rename opts.mss so we have

u16 mss_peer;
u16 mss_configured;

but I got confused myself as to where which mss is to be used.

perhaps
u16 mss_option;
u16 mss_encode;

... would have been better.
Pablo Neira Ayuso June 27, 2019, 7:27 p.m. UTC | #5
On Thu, Jun 27, 2019 at 09:21:09PM +0200, Florian Westphal wrote:
> Pablo Neira Ayuso <pablo@netfilter.org> wrote:
> > On Thu, Jun 27, 2019 at 09:00:19PM +0200, Florian Westphal wrote:
> > > Pablo Neira Ayuso <pablo@netfilter.org> wrote:
> > > > >  		opts.options &= info->options;
> > > > > +		client_mssinfo = opts.mss;
> > > > > +		opts.mss = info->mss;
> > > > 
> > > > No need for this new client_mssinfo variable, right? I mean, you can
> > > > just set:
> > > > 
> > > >         opts.mss = info->mss;
> > > > 
> > > > and use it from synproxy_send_client_synack().
> > > 
> > > I thought that as well but we need both mss values,
> > > the one configured in the target (info->mss) and the
> > > ine received from the peer.
> > > 
> > > The former is what we announce to peer in the syn/ack
> > > (as tcp option), the latter is what we need to encode
> > > in the syncookie (to decode it on cookie ack).
> > 
> > I see, probably place client_mss field into the synproxy_options
> > structure?
> 
> I worked on a fix for this too (Ibrahim was faster), I
> tried to rename opts.mss so we have
> 
> u16 mss_peer;
> u16 mss_configured;
> 
> but I got confused myself as to where which mss is to be used.
> 
> perhaps
> u16 mss_option;
> u16 mss_encode;
> 
> ... would have been better.

I would leave the opts.mss as is by now. Given there will be a
conflict between nf-next and nf, I was trying to minimize the number
of chunks for this fix, but that's just my preference (I'll have to
sort out this it seems).

Then, adding follow up patches to rename fields would be great indeed
as you suggest.
Pablo Neira Ayuso July 1, 2019, 6:58 p.m. UTC | #6
Hi Ibrahim,

On Thu, Jun 27, 2019 at 09:27:05PM +0200, Pablo Neira Ayuso wrote:
> On Thu, Jun 27, 2019 at 09:21:09PM +0200, Florian Westphal wrote:
> > Pablo Neira Ayuso <pablo@netfilter.org> wrote:
> > > On Thu, Jun 27, 2019 at 09:00:19PM +0200, Florian Westphal wrote:
> > > > Pablo Neira Ayuso <pablo@netfilter.org> wrote:
[...]
> > > I see, probably place client_mss field into the synproxy_options
> > > structure?
> > 
> > I worked on a fix for this too (Ibrahim was faster), I
> > tried to rename opts.mss so we have
> > 
> > u16 mss_peer;
> > u16 mss_configured;
> > 
> > but I got confused myself as to where which mss is to be used.
> > 
> > perhaps
> > u16 mss_option;
> > u16 mss_encode;
> > 
> > ... would have been better.
> 
> I would leave the opts.mss as is by now. Given there will be a
> conflict between nf-next and nf, I was trying to minimize the number
> of chunks for this fix, but that's just my preference (I'll have to
> sort out this it seems).
> 
> Then, adding follow up patches to rename fields would be great indeed
> as you suggest.

@Ibrahim: Would you follow up with patch v3?

I'd name this 'mss_backend' to opts, instead of adding client_mss as
parameter. Since this is the MSS that the server backend behind the
proxy.

I don't mind names, if you prefer Florian's, that's also fine. I'd
just like not to add a new parameter to synproxy_send_client_synack().

Thanks.
─░brahim Ercan July 22, 2019, 8:31 a.m. UTC | #7
On Mon, Jul 1, 2019 at 9:58 PM Pablo Neira Ayuso <pablo@netfilter.org> wrote:
>
> Hi Ibrahim,
>
> On Thu, Jun 27, 2019 at 09:27:05PM +0200, Pablo Neira Ayuso wrote:
> > On Thu, Jun 27, 2019 at 09:21:09PM +0200, Florian Westphal wrote:
> > > Pablo Neira Ayuso <pablo@netfilter.org> wrote:
> > > > On Thu, Jun 27, 2019 at 09:00:19PM +0200, Florian Westphal wrote:
> > > > > Pablo Neira Ayuso <pablo@netfilter.org> wrote:
> [...]
> > > > I see, probably place client_mss field into the synproxy_options
> > > > structure?
> > >
> > > I worked on a fix for this too (Ibrahim was faster), I
> > > tried to rename opts.mss so we have
> > >
> > > u16 mss_peer;
> > > u16 mss_configured;
> > >
> > > but I got confused myself as to where which mss is to be used.
> > >
> > > perhaps
> > > u16 mss_option;
> > > u16 mss_encode;
> > >
> > > ... would have been better.
> >
> > I would leave the opts.mss as is by now. Given there will be a
> > conflict between nf-next and nf, I was trying to minimize the number
> > of chunks for this fix, but that's just my preference (I'll have to
> > sort out this it seems).
> >
> > Then, adding follow up patches to rename fields would be great indeed
> > as you suggest.
>
> @Ibrahim: Would you follow up with patch v3?
>
> I'd name this 'mss_backend' to opts, instead of adding client_mss as
> parameter. Since this is the MSS that the server backend behind the
> proxy.
>
> I don't mind names, if you prefer Florian's, that's also fine. I'd
> just like not to add a new parameter to synproxy_send_client_synack().
>
> Thanks.

Sorry for late reply. I was offline for 3 weeks. I will send new patch asap.
Pablo Neira Ayuso July 22, 2019, 8:45 a.m. UTC | #8
On Mon, Jul 22, 2019 at 11:31:45AM +0300, ─░brahim Ercan wrote:
> On Mon, Jul 1, 2019 at 9:58 PM Pablo Neira Ayuso <pablo@netfilter.org> wrote:
> >
> > Hi Ibrahim,
> >
> > On Thu, Jun 27, 2019 at 09:27:05PM +0200, Pablo Neira Ayuso wrote:
> > > On Thu, Jun 27, 2019 at 09:21:09PM +0200, Florian Westphal wrote:
> > > > Pablo Neira Ayuso <pablo@netfilter.org> wrote:
> > > > > On Thu, Jun 27, 2019 at 09:00:19PM +0200, Florian Westphal wrote:
> > > > > > Pablo Neira Ayuso <pablo@netfilter.org> wrote:
> > [...]
> > > > > I see, probably place client_mss field into the synproxy_options
> > > > > structure?
> > > >
> > > > I worked on a fix for this too (Ibrahim was faster), I
> > > > tried to rename opts.mss so we have
> > > >
> > > > u16 mss_peer;
> > > > u16 mss_configured;
> > > >
> > > > but I got confused myself as to where which mss is to be used.
> > > >
> > > > perhaps
> > > > u16 mss_option;
> > > > u16 mss_encode;
> > > >
> > > > ... would have been better.
> > >
> > > I would leave the opts.mss as is by now. Given there will be a
> > > conflict between nf-next and nf, I was trying to minimize the number
> > > of chunks for this fix, but that's just my preference (I'll have to
> > > sort out this it seems).
> > >
> > > Then, adding follow up patches to rename fields would be great indeed
> > > as you suggest.
> >
> > @Ibrahim: Would you follow up with patch v3?
> >
> > I'd name this 'mss_backend' to opts, instead of adding client_mss as
> > parameter. Since this is the MSS that the server backend behind the
> > proxy.
> >
> > I don't mind names, if you prefer Florian's, that's also fine. I'd
> > just like not to add a new parameter to synproxy_send_client_synack().
> >
> > Thanks.
> 
> Sorry for late reply. I was offline for 3 weeks. I will send new patch asap.

Please, do, based it on kernel 5.2

Fernando already made a patch for 5.3-rc, we'll take your patch for
-stable, as a backport.

Patch
diff mbox series

diff --git a/net/ipv4/netfilter/ipt_SYNPROXY.c b/net/ipv4/netfilter/ipt_SYNPROXY.c
index 64d9563..e0bd504 100644
--- a/net/ipv4/netfilter/ipt_SYNPROXY.c
+++ b/net/ipv4/netfilter/ipt_SYNPROXY.c
@@ -69,13 +69,13 @@  synproxy_send_tcp(struct net *net,
 static void
 synproxy_send_client_synack(struct net *net,
 			    const struct sk_buff *skb, const struct tcphdr *th,
-			    const struct synproxy_options *opts)
+			    const struct synproxy_options *opts, const u16 client_mssinfo)
 {
 	struct sk_buff *nskb;
 	struct iphdr *iph, *niph;
 	struct tcphdr *nth;
 	unsigned int tcp_hdr_size;
-	u16 mss = opts->mss;
+	u16 mss = client_mssinfo;
 
 	iph = ip_hdr(skb);
 
@@ -264,6 +264,7 @@  synproxy_tg4(struct sk_buff *skb, const struct xt_action_param *par)
 	struct synproxy_net *snet = synproxy_pernet(net);
 	struct synproxy_options opts = {};
 	struct tcphdr *th, _th;
+	u16 client_mssinfo;
 
 	if (nf_ip_checksum(skb, xt_hooknum(par), par->thoff, IPPROTO_TCP))
 		return NF_DROP;
@@ -283,6 +284,8 @@  synproxy_tg4(struct sk_buff *skb, const struct xt_action_param *par)
 			opts.options |= XT_SYNPROXY_OPT_ECN;
 
 		opts.options &= info->options;
+		client_mssinfo = opts.mss;
+		opts.mss = info->mss;
 		if (opts.options & XT_SYNPROXY_OPT_TIMESTAMP)
 			synproxy_init_timestamp_cookie(info, &opts);
 		else
@@ -290,7 +293,7 @@  synproxy_tg4(struct sk_buff *skb, const struct xt_action_param *par)
 					  XT_SYNPROXY_OPT_SACK_PERM |
 					  XT_SYNPROXY_OPT_ECN);
 
-		synproxy_send_client_synack(net, skb, th, &opts);
+		synproxy_send_client_synack(net, skb, th, &opts, client_mssinfo);
 		consume_skb(skb);
 		return NF_STOLEN;
 	} else if (th->ack && !(th->fin || th->rst || th->syn)) {
diff --git a/net/ipv6/netfilter/ip6t_SYNPROXY.c b/net/ipv6/netfilter/ip6t_SYNPROXY.c
index 41325d5..676de53 100644
--- a/net/ipv6/netfilter/ip6t_SYNPROXY.c
+++ b/net/ipv6/netfilter/ip6t_SYNPROXY.c
@@ -83,13 +83,13 @@  synproxy_send_tcp(struct net *net,
 static void
 synproxy_send_client_synack(struct net *net,
 			    const struct sk_buff *skb, const struct tcphdr *th,
-			    const struct synproxy_options *opts)
+			    const struct synproxy_options *opts, const u16 client_mssinfo)
 {
 	struct sk_buff *nskb;
 	struct ipv6hdr *iph, *niph;
 	struct tcphdr *nth;
 	unsigned int tcp_hdr_size;
-	u16 mss = opts->mss;
+	u16 mss = client_mssinfo;
 
 	iph = ipv6_hdr(skb);
 
@@ -278,6 +278,7 @@  synproxy_tg6(struct sk_buff *skb, const struct xt_action_param *par)
 	struct synproxy_net *snet = synproxy_pernet(net);
 	struct synproxy_options opts = {};
 	struct tcphdr *th, _th;
+	u16 client_mssinfo;
 
 	if (nf_ip6_checksum(skb, xt_hooknum(par), par->thoff, IPPROTO_TCP))
 		return NF_DROP;
@@ -297,6 +298,8 @@  synproxy_tg6(struct sk_buff *skb, const struct xt_action_param *par)
 			opts.options |= XT_SYNPROXY_OPT_ECN;
 
 		opts.options &= info->options;
+		client_mssinfo = opts.mss;
+		opts.mss = info->mss;
 		if (opts.options & XT_SYNPROXY_OPT_TIMESTAMP)
 			synproxy_init_timestamp_cookie(info, &opts);
 		else
@@ -304,7 +307,7 @@  synproxy_tg6(struct sk_buff *skb, const struct xt_action_param *par)
 					  XT_SYNPROXY_OPT_SACK_PERM |
 					  XT_SYNPROXY_OPT_ECN);
 
-		synproxy_send_client_synack(net, skb, th, &opts);
+		synproxy_send_client_synack(net, skb, th, &opts, client_mssinfo);
 		consume_skb(skb);
 		return NF_STOLEN;