netdev - Potential violation of RFC793 3.9, missing challenge ACK

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-next>] [day] [month] [year] [list]

Message-ID:
 <TYAPR01MB64091EA3717FC588B4ACF045C4ED2@TYAPR01MB6409.jpnprd01.prod.outlook.com>
Date: Thu, 16 May 2024 16:12:30 +0900
From: <hotaka.miyazaki@...ertrust.co.jp>
To: <netdev@...r.kernel.org>
Subject: Potential violation of RFC793 3.9, missing challenge ACK

Hello.

I have a question about the following part of the tcp_ack function in net/ipv4/tcp_input.c.
```
	/* If the ack includes data we haven't sent yet, discard
 	* this segment (RFC793 Section 3.9).
 	*/
	if (after(ack, tp->snd_nxt))
  	  return -SKB_DROP_REASON_TCP_ACK_UNSENT_DATA;
```
I think that this part violates RFC793 3.9 (and the equivalent part in RFC9293 (3.10.7.4)).

According to the RFC, “If the ACK acks something not yet sent (SEG.ACK > SND.NXT) then send an ACK, drop the segment, and return“ [1]. 
However, the code appears not to ack before discarding a segment.

Does anyone know whether the violation is intended and the reason if it is intended?
If that is not intended, I would like to fix it. What do you think of that?

I think that the following patch would fix the violation. 
The fixed code is similar to the SKB_DROP_REASON_TCP_TOO_OLD_ACK error case.
```
diff --git a/net/ipv4/tcp_input.c b/net/ipv4/tcp_input.c
index 5d874817a78d..a0ed2f671912 100644
--- a/net/ipv4/tcp_input.c
+++ b/net/ipv4/tcp_input.c
@@ -3877,11 +3877,14 @@ static int tcp_ack(struct sock *sk, const struct sk_buff *skb, int flag)
            	goto old_ack;
    	}

-   	/* If the ack includes data we haven't sent yet, discard
-    	* this segment (RFC793 Section 3.9).
+   	/* If the ack includes data we haven't sent yet,
+    	* send an ACK and discard this segment (RFC793 Section 3.9).
     	*/
-   	if (after(ack, tp->snd_nxt))
+   	if (after(ack, tp->snd_nxt)) {
+           	if (!(flag & FLAG_NO_CHALLENGE_ACK))
+                   	tcp_send_challenge_ack(sk);
            	return -SKB_DROP_REASON_TCP_ACK_UNSENT_DATA;
+   	}

    	if (after(ack, prior_snd_una)) {
            	flag |= FLAG_SND_UNA_ADVANCED;
```

Thanks for reading.

---

[1]: RFC793 3.9 on page 72 (fifth check)
```
   	ESTABLISHED STATE

      	If SND.UNA < SEG.ACK =< SND.NXT then, set SND.UNA <- SEG.ACK.
      	Any segments on the retransmission queue which are thereby
      	entirely acknowledged are removed.  Users should receive
      	positive acknowledgments for buffers which have been SENT and
      	fully acknowledged (i.e., SEND buffer should be returned with
      	"ok" response).  If the ACK is a duplicate
      	(SEG.ACK < SND.UNA), it can be ignored.  If the ACK acks
      	something not yet sent (SEG.ACK > SND.NXT) then send an ACK,
      	drop the segment, and return.
```