From 4ef7588d2b3f83775099797baac43c34e2e23200 Mon Sep 17 00:00:00 2001
From: Damien Neil <dneil@google.com>
Date: Fri, 6 Dec 2024 14:56:39 -0800
Subject: [PATCH] quic: handle ACK frame in packet which drops number space

Avoid incorrectly closing a connection with a protocol violation
error when we receive a single packet containing a CRYPTO frame
that causes us to drop a packet number space (forgetting what
packet numbers we've sent in that space) followed by an ACK frame.

Fixes golang/go#70703

Change-Id: I37554cb6a3086736cb9d772f8a3441b544d414dc
Reviewed-on: https://go-review.googlesource.com/c/net/+/634616
LUCI-TryBot-Result: Go LUCI <golang-scoped@luci-project-accounts.iam.gserviceaccount.com>
Auto-Submit: Damien Neil <dneil@google.com>
Reviewed-by: Jonathan Amsterdam <jba@google.com>
---
 quic/conn_recv.go      | 11 ++++++++
 quic/conn_recv_test.go | 60 ++++++++++++++++++++++++++++++++++++++++++
 quic/tls.go            |  6 +----
 3 files changed, 72 insertions(+), 5 deletions(-)
 create mode 100644 quic/conn_recv_test.go

diff --git a/quic/conn_recv.go b/quic/conn_recv.go
index b1354cd3a..dbfe34a34 100644
--- a/quic/conn_recv.go
+++ b/quic/conn_recv.go
@@ -285,6 +285,7 @@ func (c *Conn) handleFrames(now time.Time, dgram *datagram, ptype packetType, sp
 		__01 = packetType0RTT | packetType1RTT
 		___1 = packetType1RTT
 	)
+	hasCrypto := false
 	for len(payload) > 0 {
 		switch payload[0] {
 		case frameTypePadding, frameTypeAck, frameTypeAckECN,
@@ -322,6 +323,7 @@ func (c *Conn) handleFrames(now time.Time, dgram *datagram, ptype packetType, sp
 			if !frameOK(c, ptype, IH_1) {
 				return
 			}
+			hasCrypto = true
 			n = c.handleCryptoFrame(now, space, payload)
 		case frameTypeNewToken:
 			if !frameOK(c, ptype, ___1) {
@@ -406,6 +408,15 @@ func (c *Conn) handleFrames(now time.Time, dgram *datagram, ptype packetType, sp
 		}
 		payload = payload[n:]
 	}
+	if hasCrypto {
+		// Process TLS events after handling all frames in a packet.
+		// TLS events can cause us to drop state for a number space,
+		// so do that last, to avoid handling frames differently
+		// depending on whether they come before or after a CRYPTO frame.
+		if err := c.handleTLSEvents(now); err != nil {
+			c.abort(now, err)
+		}
+	}
 	return ackEliciting
 }
 
diff --git a/quic/conn_recv_test.go b/quic/conn_recv_test.go
new file mode 100644
index 000000000..0e94731bf
--- /dev/null
+++ b/quic/conn_recv_test.go
@@ -0,0 +1,60 @@
+// Copyright 2024 The Go Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style
+// license that can be found in the LICENSE file.
+
+//go:build go1.21
+
+package quic
+
+import (
+	"crypto/tls"
+	"testing"
+)
+
+func TestConnReceiveAckForUnsentPacket(t *testing.T) {
+	tc := newTestConn(t, serverSide, permissiveTransportParameters)
+	tc.handshake()
+	tc.writeFrames(packetType1RTT,
+		debugFrameAck{
+			ackDelay: 0,
+			ranges:   []i64range[packetNumber]{{0, 10}},
+		})
+	tc.wantFrame("ACK for unsent packet causes CONNECTION_CLOSE",
+		packetType1RTT, debugFrameConnectionCloseTransport{
+			code: errProtocolViolation,
+		})
+}
+
+// Issue #70703: If a packet contains both a CRYPTO frame which causes us to
+// drop state for a number space, and also contains a valid ACK frame for that space,
+// we shouldn't complain about the ACK.
+func TestConnReceiveAckForDroppedSpace(t *testing.T) {
+	tc := newTestConn(t, serverSide, permissiveTransportParameters)
+	tc.ignoreFrame(frameTypeAck)
+	tc.ignoreFrame(frameTypeNewConnectionID)
+
+	tc.writeFrames(packetTypeInitial,
+		debugFrameCrypto{
+			data: tc.cryptoDataIn[tls.QUICEncryptionLevelInitial],
+		})
+	tc.wantFrame("send Initial crypto",
+		packetTypeInitial, debugFrameCrypto{
+			data: tc.cryptoDataOut[tls.QUICEncryptionLevelInitial],
+		})
+	tc.wantFrame("send Handshake crypto",
+		packetTypeHandshake, debugFrameCrypto{
+			data: tc.cryptoDataOut[tls.QUICEncryptionLevelHandshake],
+		})
+
+	tc.writeFrames(packetTypeHandshake,
+		debugFrameCrypto{
+			data: tc.cryptoDataIn[tls.QUICEncryptionLevelHandshake],
+		},
+		debugFrameAck{
+			ackDelay: 0,
+			ranges:   []i64range[packetNumber]{{0, tc.lastPacket.num + 1}},
+		})
+	tc.wantFrame("handshake finishes",
+		packetType1RTT, debugFrameHandshakeDone{})
+	tc.wantIdle("connection is idle")
+}
diff --git a/quic/tls.go b/quic/tls.go
index e2f2e5bde..89b31842c 100644
--- a/quic/tls.go
+++ b/quic/tls.go
@@ -119,11 +119,7 @@ func (c *Conn) handleCrypto(now time.Time, space numberSpace, off int64, data []
 	default:
 		return errors.New("quic: internal error: received CRYPTO frame in unexpected number space")
 	}
-	err := c.crypto[space].handleCrypto(off, data, func(b []byte) error {
+	return c.crypto[space].handleCrypto(off, data, func(b []byte) error {
 		return c.tls.HandleData(level, b)
 	})
-	if err != nil {
-		return err
-	}
-	return c.handleTLSEvents(now)
 }