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 <[email protected]>
Auto-Submit: Damien Neil <[email protected]>
Reviewed-by: Jonathan Amsterdam <[email protected]>

Author: neild

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
 }
 

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")
+}

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)
 }