Messages in this thread Patch in this message | | | From | Eric Dumazet <> | Date | Tue, 30 Jun 2020 14:54:39 -0700 | Subject | Re: [regression] TCP_MD5SIG on established sockets |
| |
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)
[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;
| |