netdev - Re: [PATCH net v3 1/3] tcp: fix rejected syncookies due to stale timestamps

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: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <db49bc30-9909-b15c-2b36-d805a4487e07@gmail.com>
Date:   Thu, 5 Dec 2019 19:41:34 -0800
From:   Eric Dumazet <eric.dumazet@...il.com>
To:     Guillaume Nault <gnault@...hat.com>,
        David Miller <davem@...emloft.net>,
        Jakub Kicinski <jakub.kicinski@...ronome.com>
Cc:     netdev@...r.kernel.org, Eric Dumazet <edumazet@...gle.com>,
        Arnd Bergmann <arnd@...db.de>,
        John Stultz <john.stultz@...aro.org>,
        Thomas Gleixner <tglx@...utronix.de>
Subject: Re: [PATCH net v3 1/3] tcp: fix rejected syncookies due to stale
 timestamps



On 12/5/19 5:49 PM, Guillaume Nault wrote:
> If no synflood happens for a long enough period of time, then the
> synflood timestamp isn't refreshed and jiffies can advance so much
> that time_after32() can't accurately compare them any more.
> 
> Therefore, we can end up in a situation where time_after32(now,
> last_overflow + HZ) returns false, just because these two values are
> too far apart. In that case, the synflood timestamp isn't updated as
> it should be, which can trick tcp_synq_no_recent_overflow() into
> rejecting valid syncookies.
> 
> For example, let's consider the following scenario on a system
> with HZ=1000:
> 
>   * The synflood timestamp is 0, either because that's the timestamp
>     of the last synflood or, more commonly, because we're working with
>     a freshly created socket.
> 
>   * We receive a new SYN, which triggers synflood protection. Let's say
>     that this happens when jiffies == 2147484649 (that is,
>     'synflood timestamp' + HZ + 2^31 + 1).
> 
>   * Then tcp_synq_overflow() doesn't update the synflood timestamp,
>     because time_after32(2147484649, 1000) returns false.
>     With:
>       - 2147484649: the value of jiffies, aka. 'now'.
>       - 1000: the value of 'last_overflow' + HZ.
> 
>   * A bit later, we receive the ACK completing the 3WHS. But
>     cookie_v[46]_check() rejects it because tcp_synq_no_recent_overflow()
>     says that we're not under synflood. That's because
>     time_after32(2147484649, 120000) returns false.
>     With:
>       - 2147484649: the value of jiffies, aka. 'now'.
>       - 120000: the value of 'last_overflow' + TCP_SYNCOOKIE_VALID.
> 
>     Of course, in reality jiffies would have increased a bit, but this
>     condition will last for the next 119 seconds, which is far enough
>     to accommodate for jiffie's growth.
> 
> Fix this by updating the overflow timestamp whenever jiffies isn't
> within the [last_overflow, last_overflow + HZ] range. That shouldn't
> have any performance impact since the update still happens at most once
> per second.
> 
> Now we're guaranteed to have fresh timestamps while under synflood, so
> tcp_synq_no_recent_overflow() can safely use it with time_after32() in
> such situations.
> 
> Stale timestamps can still make tcp_synq_no_recent_overflow() return
> the wrong verdict when not under synflood. This will be handled in the
> next patch.
> 
> For 64 bits architectures, the problem was introduced with the
> conversion of ->tw_ts_recent_stamp to 32 bits integer by commit
> cca9bab1b72c ("tcp: use monotonic timestamps for PAWS").
> The problem has always been there on 32 bits architectures.
> 
> Fixes: cca9bab1b72c ("tcp: use monotonic timestamps for PAWS")
> Fixes: 1da177e4c3f4 ("Linux-2.6.12-rc2")
> Signed-off-by: Guillaume Nault <gnault@...hat.com>
> ---
>  include/linux/time.h | 13 +++++++++++++
>  include/net/tcp.h    |  5 +++--
>  2 files changed, 16 insertions(+), 2 deletions(-)
> 
> diff --git a/include/linux/time.h b/include/linux/time.h
> index 0760a4f5a15c..30efc1f0f67d 100644
> --- a/include/linux/time.h
> +++ b/include/linux/time.h
> @@ -97,4 +97,17 @@ static inline bool itimerspec64_valid(const struct itimerspec64 *its)
>   */
>  #define time_after32(a, b)	((s32)((u32)(b) - (u32)(a)) < 0)
>  #define time_before32(b, a)	time_after32(a, b)
> +
> +/**
> + * time_before32 - check if a 32-bit timestamp is within a given time range

Wrong name ? This should be time_between32

> + * @t:	the time which may be within [l,h]
> + * @l:	the lower bound of the range
> + * @h:	the higher bound of the range
> + *
> + * time_before32(t, l, h) returns true if @l <= @t <= @h. All operands are
> + * treated as 32-bit integers.
> + *
> + * Equivalent to !(time_before32(@t, @l) || time_after32(@t, @h)).
> + */
> +#define time_between32(t, l, h) ((u32)(h) - (u32)(l) >= (u32)(t) - (u32)(l))
>  #endif
> diff --git a/include/net/tcp.h b/include/net/tcp.h
> index 36f195fb576a..7d734ba391fc 100644
> --- a/include/net/tcp.h
> +++ b/include/net/tcp.h
> @@ -494,14 +494,15 @@ static inline void tcp_synq_overflow(const struct sock *sk)
>  		reuse = rcu_dereference(sk->sk_reuseport_cb);
>  		if (likely(reuse)) {
>  			last_overflow = READ_ONCE(reuse->synq_overflow_ts);
> -			if (time_after32(now, last_overflow + HZ))
> +			if (!time_between32(now, last_overflow,
> +					    last_overflow + HZ))
>  				WRITE_ONCE(reuse->synq_overflow_ts, now);
>  			return;
>  		}
>  	}
>  
>  	last_overflow = tcp_sk(sk)->rx_opt.ts_recent_stamp;
> -	if (time_after32(now, last_overflow + HZ))
> +	if (!time_between32(now, last_overflow, last_overflow + HZ))
>  		tcp_sk(sk)->rx_opt.ts_recent_stamp = now;
>  }
>  
>