netdev - Re: [PATCH net] tcp: Avoid time_after32() underflow when handling syncookies

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite for Android: free password hash cracker in your pocket

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <CANn89i+G0jCU=JtSit3X9w+SaExgbbo-d1x4UEkTEJRdypN3gQ@mail.gmail.com>
Date:   Thu, 28 Nov 2019 14:04:19 -0800
From:   Eric Dumazet <edumazet@...gle.com>
To:     Guillaume Nault <gnault@...hat.com>
Cc:     David Miller <davem@...emloft.net>,
        Jakub Kicinski <jakub.kicinski@...ronome.com>,
        netdev <netdev@...r.kernel.org>, Arnd Bergmann <arnd@...db.de>
Subject: Re: [PATCH net] tcp: Avoid time_after32() underflow when handling syncookies

On Thu, Nov 28, 2019 at 1:36 PM Guillaume Nault <gnault@...hat.com> wrote:
>
> In tcp_synq_overflow() and tcp_synq_no_recent_overflow(), the
> time_after32() call might underflow and return the opposite of the
> expected result.
>
> This happens after socket initialisation, when ->synq_overflow_ts and
> ->rx_opt.ts_recent_stamp are still set to zero. In this case, they
> can't be compared reliably to the current value of jiffies using
> time_after32(), because jiffies may be too far apart (especially soon
> after system startup, when it's close to 2^32).
>
> In such a situation, the erroneous time_after32() result prevents
> tcp_synq_overflow() from updating ->synq_overflow_ts and
> ->rx_opt.ts_recent_stamp, so the problem remains until jiffies wraps
> and exceeds HZ.
>
> Practical consequences should be quite limited though, because the
> time_after32() call of tcp_synq_no_recent_overflow() would also
> underflow (unless jiffies wrapped since the first time_after32() call),
> thus detecting a socket overflow and triggering the syncookie
> verification anyway.
>
> Also, since commit 399040847084 ("bpf: add helper to check for a valid
> SYN cookie") and commit 70d66244317e ("bpf: add bpf_tcp_gen_syncookie
> helper"), tcp_synq_overflow() and tcp_synq_no_recent_overflow() can be
> triggered from BPF programs. Even though such programs would normally
> pair these two operations, so both underflows would compensate each
> other as described above, we'd better avoid exposing the problem
> outside of the kernel networking stack.
>
> Let's fix it by initialising ->rx_opt.ts_recent_stamp and
> ->synq_overflow_ts to a value that can be safely compared to jiffies
> using time_after32(). Use "jiffies - TCP_SYNCOOKIE_VALID - 1", to
> indicate that we're not in a socket overflow phase.
>
> Fixes: cca9bab1b72c ("tcp: use monotonic timestamps for PAWS")
> Signed-off-by: Guillaume Nault <gnault@...hat.com>
> ---
>  net/core/sock_reuseport.c | 10 ++++++++++
>  net/ipv4/tcp.c            |  8 ++++++++
>  2 files changed, 18 insertions(+)
>
> diff --git a/net/core/sock_reuseport.c b/net/core/sock_reuseport.c
> index f19f179538b9..87c287433a52 100644
> --- a/net/core/sock_reuseport.c
> +++ b/net/core/sock_reuseport.c
> @@ -11,6 +11,7 @@
>  #include <linux/idr.h>
>  #include <linux/filter.h>
>  #include <linux/rcupdate.h>
> +#include <net/tcp.h>
>
>  #define INIT_SOCKS 128
>
> @@ -85,6 +86,15 @@ int reuseport_alloc(struct sock *sk, bool bind_inany)
>         reuse->socks[0] = sk;
>         reuse->num_socks = 1;
>         reuse->bind_inany = bind_inany;
> +
> +       /* synq_overflow_ts can be used for syncookies. Ensure that it has a
> +        * recent value, so that tcp_synq_overflow() and
> +        * tcp_synq_no_recent_overflow() can safely use time_after32().
> +        * Initialise it 'TCP_SYNCOOKIE_VALID + 1' jiffies in the past, to
> +        * ensure that we start in the 'no recent overflow' case.
> +        */
> +       reuse->synq_overflow_ts = jiffies - TCP_SYNCOOKIE_VALID - 1;
> +
>         rcu_assign_pointer(sk->sk_reuseport_cb, reuse);
>
>  out:
> diff --git a/net/ipv4/tcp.c b/net/ipv4/tcp.c
> index 9b48aec29aca..e9555db95dff 100644
> --- a/net/ipv4/tcp.c
> +++ b/net/ipv4/tcp.c
> @@ -443,6 +443,14 @@ void tcp_init_sock(struct sock *sk)
>         tp->tsoffset = 0;
>         tp->rack.reo_wnd_steps = 1;
>
> +       /* ts_recent_stamp can be used for syncookies. Ensure that it has a
> +        * recent value, so that tcp_synq_overflow() and
> +        * tcp_synq_no_recent_overflow() can safely use time_after32().
> +        * Initialise it 'TCP_SYNCOOKIE_VALID + 1' jiffies in the past, to
> +        * ensure that we start in the 'no recent overflow' case.
> +        */
> +       tp->rx_opt.ts_recent_stamp = jiffies - TCP_SYNCOOKIE_VALID - 1;
> +
>         sk->sk_state = TCP_CLOSE;
>
>         sk->sk_write_space = sk_stream_write_space;
> --
> 2.21.0
>

A listener could be live for one year, and flip its ' I am under
synflood' status every 24 days (assuming HZ=1000)

You only made sure the first 24 days are ok, but the problem is still there.

We need to refresh the values, maybe in tcp_synq_no_recent_overflow()

(Note the issue has been there forever on 32bit arches)