Re: [regression] TCP_MD5SIG on established sockets

[Eric Dumazet <edumazet@google.com>]

On Tue, Jun 30, 2020 at 2:54 PM Eric Dumazet <edumazet@google.com> wrote:
>
> On Tue, Jun 30, 2020 at 2:23 PM Eric Dumazet <edumazet@google.com> wrote:
> >
> > On Tue, Jun 30, 2020 at 2:17 PM Mathieu Desnoyers
> > <mathieu.desnoyers@efficios.com> wrote:
> > >
> > > ----- On Jun 30, 2020, at 4:56 PM, Eric Dumazet edumazet@google.com wrote:
> > >
> > > > On Tue, Jun 30, 2020 at 1:44 PM David Miller <davem@davemloft.net> wrote:
> > > >>
> > > >> From: Eric Dumazet <edumazet@google.com>
> > > >> Date: Tue, 30 Jun 2020 13:39:27 -0700
> > > >>
> > > >> > The (C) & (B) case are certainly doable.
> > > >> >
> > > >> > A) case is more complex, I have no idea of breakages of various TCP
> > > >> > stacks if a flow got SACK
> > > >> > at some point (in 3WHS) but suddenly becomes Reno.
> > > >>
> > > >> I agree that C and B are the easiest to implement without having to
> > > >> add complicated code to handle various negotiated TCP option
> > > >> scenerios.
> > > >>
> > > >> It does seem to be that some entities do A, or did I misread your
> > > >> behavioral analysis of various implementations Mathieu?
> > > >>
> > > >> Thanks.
> > > >
> > > > Yes, another question about Mathieu cases is do determine the behavior
> > > > of all these stacks vs :
> > > > SACK option
> > > > TCP TS option.
> > >
> > > I will ask my customer's networking team to investigate these behaviors,
> > > which will allow me to prepare a thorough reply to the questions raised
> > > by Eric and David. I expect to have an answer within 2-3 weeks at most.
> > >
> > > Thank you!
> >
> >
> > Great, I am working on adding back support for (B) & (C) by the end of
> > this week.
>
> Note that the security issue (of sending uninit bytes to the wire) has
> been independently fixed with [1]
>
> This means syzbot was able to have MD5+TS+SACK  ~6 months ago.
>
> It seems we (linux) do not enable this combination for PASSIVE flows,
> (according to tcp_synack_options()),
> but  for ACTIVE flows we do nothing special.
>
> So maybe code in tcp_synack_options() should be mirrored to
> tcp_syn_options() for consistency.
> (disabling TS if  both MD5 and SACK are enabled)

Oh well, tcp_syn_options() is supposed to have the same logic.

Maybe we have an issue with SYNCOOKIES (with MD5 + TS + SACK)

Nice can of worms.

>
> [1]
>
> commit 9424e2e7ad93ffffa88f882c9bc5023570904b55
> Author: Eric Dumazet <edumazet@google.com>
> Date:   Thu Dec 5 10:10:15 2019 -0800
>
>     tcp: md5: fix potential overestimation of TCP option space
>
>     Back in 2008, Adam Langley fixed the corner case of packets for flows
>     having all of the following options : MD5 TS SACK
>
>     Since MD5 needs 20 bytes, and TS needs 12 bytes, no sack block
>     can be cooked from the remaining 8 bytes.
>
>     tcp_established_options() correctly sets opts->num_sack_blocks
>     to zero, but returns 36 instead of 32.
>
>     This means TCP cooks packets with 4 extra bytes at the end
>     of options, containing unitialized bytes.
>
>     Fixes: 33ad798c924b ("tcp: options clean up")
>     Signed-off-by: Eric Dumazet <edumazet@google.com>
>     Reported-by: syzbot <syzkaller@googlegroups.com>
>     Acked-by: Neal Cardwell <ncardwell@google.com>
>     Acked-by: Soheil Hassas Yeganeh <soheil@google.com>
>     Signed-off-by: David S. Miller <davem@davemloft.net>
>
> diff --git a/net/ipv4/tcp_output.c b/net/ipv4/tcp_output.c
> index be6d22b8190fa375074062032105879270af4be5..b184f03d743715ef4b2d166ceae651529be77953
> 100644
> --- a/net/ipv4/tcp_output.c
> +++ b/net/ipv4/tcp_output.c
> @@ -755,8 +755,9 @@ static unsigned int tcp_established_options(struct
> sock *sk, struct sk_buff *skb
>                         min_t(unsigned int, eff_sacks,
>                               (remaining - TCPOLEN_SACK_BASE_ALIGNED) /
>                               TCPOLEN_SACK_PERBLOCK);
> -               size += TCPOLEN_SACK_BASE_ALIGNED +
> -                       opts->num_sack_blocks * TCPOLEN_SACK_PERBLOCK;
> +               if (likely(opts->num_sack_blocks))
> +                       size += TCPOLEN_SACK_BASE_ALIGNED +
> +                               opts->num_sack_blocks * TCPOLEN_SACK_PERBLOCK;
>         }
>
>         return size;