| 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. | expand |
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)) {
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).
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?
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.
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.
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.
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.
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.
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;
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(-)