netdev - Re: [PATCH net] tcp: do not accept ACK of bytes we never sent

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: <CAK6E8=dCNTuZvyHJYUzv-BmFVkxa=cnDazgLdCtDLvrGmEWT0w@mail.gmail.com>
Date: Tue, 5 Dec 2023 08:47:02 -0800
From: Yuchung Cheng <ycheng@...gle.com>
To: Eric Dumazet <edumazet@...gle.com>
Cc: "David S . Miller" <davem@...emloft.net>, Jakub Kicinski <kuba@...nel.org>, 
	Paolo Abeni <pabeni@...hat.com>, Neal Cardwell <ncardwell@...gle.com>, 
	Soheil Hassas Yeganeh <soheil@...gle.com>, netdev@...r.kernel.org, eric.dumazet@...il.com, 
	Yepeng Pan <yepeng.pan@...pa.de>, Christian Rossow <rossow@...pa.de>
Subject: Re: [PATCH net] tcp: do not accept ACK of bytes we never sent

On Tue, Dec 5, 2023 at 8:18 AM Eric Dumazet <edumazet@...gle.com> wrote:
>
> This patch is based on a detailed report and ideas from Yepeng Pan
> and Christian Rossow.
>
> ACK seq validation is currently following RFC 5961 5.2 guidelines:
>
>    The ACK value is considered acceptable only if
>    it is in the range of ((SND.UNA - MAX.SND.WND) <= SEG.ACK <=
>    SND.NXT).  All incoming segments whose ACK value doesn't satisfy the
>    above condition MUST be discarded and an ACK sent back.  It needs to
>    be noted that RFC 793 on page 72 (fifth check) says: "If the ACK is a
>    duplicate (SEG.ACK < SND.UNA), it can be ignored.  If the ACK
>    acknowledges something not yet sent (SEG.ACK > SND.NXT) then send an
>    ACK, drop the segment, and return".  The "ignored" above implies that
>    the processing of the incoming data segment continues, which means
>    the ACK value is treated as acceptable.  This mitigation makes the
>    ACK check more stringent since any ACK < SND.UNA wouldn't be
>    accepted, instead only ACKs that are in the range ((SND.UNA -
>    MAX.SND.WND) <= SEG.ACK <= SND.NXT) get through.
Thank you Eric for the fix. It appears the newer RFC
https://www.rfc-editor.org/rfc/rfc9293.html also has this issue that
needs a revision?

>
> This can be refined for new (and possibly spoofed) flows,
> by not accepting ACK for bytes that were never sent.
>
> This greatly improves TCP security at a little cost.
>
> I added a Fixes: tag to make sure this patch will reach stable trees,
> even if the 'blamed' patch was adhering to the RFC.
>
> tp->bytes_acked was added in linux-4.2
>
> Following packetdrill test (courtesy of Yepeng Pan) shows
> the issue at hand:
>
> 0 socket(..., SOCK_STREAM, IPPROTO_TCP) = 3
> +0 setsockopt(3, SOL_SOCKET, SO_REUSEADDR, [1], 4) = 0
> +0 bind(3, ..., ...) = 0
> +0 listen(3, 1024) = 0
>
> // ---------------- Handshake ------------------- //
>
> // when window scale is set to 14 the window size can be extended to
> // 65535 * (2^14) = 1073725440. Linux would accept an ACK packet
> // with ack number in (Server_ISN+1-1073725440. Server_ISN+1)
> // ,though this ack number acknowledges some data never
> // sent by the server.
>
> +0 < S 0:0(0) win 65535 <mss 1400,nop,wscale 14>
> +0 > S. 0:0(0) ack 1 <...>
> +0 < . 1:1(0) ack 1 win 65535
> +0 accept(3, ..., ...) = 4
>
> // For the established connection, we send an ACK packet,
> // the ack packet uses ack number 1 - 1073725300 + 2^32,
> // where 2^32 is used to wrap around.
> // Note: we used 1073725300 instead of 1073725440 to avoid possible
> // edge cases.
> // 1 - 1073725300 + 2^32 = 3221241997
>
> // Oops, old kernels happily accept this packet.
> +0 < . 1:1001(1000) ack 3221241997 win 65535
>
> // After the kernel fix the following will be replaced by a challenge ACK,
> // and prior malicious frame would be dropped.
> +0 > . 1:1(0) ack 1001
>
> Fixes: 354e4aa391ed ("tcp: RFC 5961 5.2 Blind Data Injection Attack Mitigation")
> Signed-off-by: Eric Dumazet <edumazet@...gle.com>
> Reported-by: Yepeng Pan <yepeng.pan@...pa.de>
> Reported-by: Christian Rossow <rossow@...pa.de>
> ---
>  net/ipv4/tcp_input.c | 6 +++++-
>  1 file changed, 5 insertions(+), 1 deletion(-)
>
> diff --git a/net/ipv4/tcp_input.c b/net/ipv4/tcp_input.c
> index bcb55d98004c5213f0095613124d5193b15b2793..62cccc2e89ec68b3badae03168f1bfcd2698e0b7 100644
> --- a/net/ipv4/tcp_input.c
> +++ b/net/ipv4/tcp_input.c
> @@ -3871,8 +3871,12 @@ static int tcp_ack(struct sock *sk, const struct sk_buff *skb, int flag)
>          * then we can probably ignore it.
>          */
>         if (before(ack, prior_snd_una)) {
> +               u32 max_window;
> +
> +               /* do not accept ACK for bytes we never sent. */
> +               max_window = min_t(u64, tp->max_window, tp->bytes_acked);
>                 /* RFC 5961 5.2 [Blind Data Injection Attack].[Mitigation] */
> -               if (before(ack, prior_snd_una - tp->max_window)) {
> +               if (before(ack, prior_snd_una - max_window)) {
>                         if (!(flag & FLAG_NO_CHALLENGE_ACK))
>                                 tcp_send_challenge_ack(sk);
>                         return -SKB_DROP_REASON_TCP_TOO_OLD_ACK;
> --
> 2.43.0.rc2.451.g8631bc7472-goog
>