quic: clean up crypto streams when dropping packet protection keys

When dropping packet protection keys for a number space: Check to see if there is unused CRYPTO data received from the peer in the space. If so, close the connection with an error. This can only happen if the peer has sent us data with a gap in it. We can never read the data that fills that gap (because we're dropping the key it would be encrypted with), and this situation cannot happen without the peer sending invalid TLS handshake data. Drop any buffered CRYPTO data being sent to the peer. Under normal operations, we may have data that was sent to the peer but which we haven't received an ACK for yet. The peer has received the data (or we wouldn't be dropping the number space) and we will never see the ACK (because we're dropping the key it would be encrypted with). Fixes golang/go#70704 Change-Id: I53380169cb59a2a6f87e69b38522ba81ad38c2b0 Reviewed-on: https://go-review.googlesource.com/c/net/+/634617 LUCI-TryBot-Result: Go LUCI <golang-scoped@luci-project-accounts.iam.gserviceaccount.com> Reviewed-by: Jonathan Amsterdam <jba@google.com> Auto-Submit: Damien Neil <dneil@google.com>
golang · Dec 10, 2024 · 6705db9 · 6705db9
1 parent 4ef7588
commit 6705db9
Show file tree

Hide file tree

Showing 3 changed files with 50 additions and 0 deletions.
diff --git a/quic/conn.go b/quic/conn.go
@@ -216,6 +216,9 @@ func (c *Conn) confirmHandshake(now time.Time) {
 // discardKeys discards unused packet protection keys.
 // https://www.rfc-editor.org/rfc/rfc9001#section-4.9
 func (c *Conn) discardKeys(now time.Time, space numberSpace) {
+	if err := c.crypto[space].discardKeys(); err != nil {
+		c.abort(now, err)
+	}
 	switch space {
 	case initialSpace:
 		c.keysInitial.discard()

diff --git a/quic/crypto_stream.go b/quic/crypto_stream.go
@@ -139,3 +139,21 @@ func (s *cryptoStream) sendData(off int64, b []byte) {
 	s.out.copy(off, b)
 	s.outunsent.sub(off, off+int64(len(b)))
 }
+
+// discardKeys is called when the packet protection keys for the stream are dropped.
+func (s *cryptoStream) discardKeys() error {
+	if s.in.end-s.in.start != 0 {
+		// The peer sent some unprocessed CRYPTO data that we're about to discard.
+		// Close the connetion with a TLS unexpected_message alert.
+		// https://www.rfc-editor.org/rfc/rfc5246#section-7.2.2
+		const unexpectedMessage = 10
+		return localTransportError{
+			code:   errTLSBase + unexpectedMessage,
+			reason: "excess crypto data",
+		}
+	}
+	// Discard any unacked (but presumably received) data in our output buffer.
+	s.out.discardBefore(s.out.end)
+	*s = cryptoStream{}
+	return nil
+}
diff --git a/quic/tls_test.go b/quic/tls_test.go
@@ -615,3 +615,32 @@ func TestConnAEADLimitReached(t *testing.T) {
 	tc.advance(1 * time.Second)
 	tc.wantIdle("auth failures at limit: conn does not process additional packets")
 }
+
+func TestConnKeysDiscardedWithExcessCryptoData(t *testing.T) {
+	tc := newTestConn(t, serverSide, permissiveTransportParameters)
+	tc.ignoreFrame(frameTypeAck)
+	tc.ignoreFrame(frameTypeNewConnectionID)
+	tc.ignoreFrame(frameTypeCrypto)
+
+	// One byte of excess CRYPTO data, separated from the valid data by a one-byte gap.
+	tc.writeFrames(packetTypeInitial,
+		debugFrameCrypto{
+			off:  int64(len(tc.cryptoDataIn[tls.QUICEncryptionLevelInitial]) + 1),
+			data: []byte{0},
+		})
+	tc.writeFrames(packetTypeInitial,
+		debugFrameCrypto{
+			data: tc.cryptoDataIn[tls.QUICEncryptionLevelInitial],
+		})
+
+	// We don't drop the Initial keys and discover the excess data until the client
+	// sends a Handshake packet.
+	tc.writeFrames(packetTypeHandshake,
+		debugFrameCrypto{
+			data: tc.cryptoDataIn[tls.QUICEncryptionLevelHandshake],
+		})
+	tc.wantFrame("connection closed due to excess Initial CRYPTO data",
+		packetType1RTT, debugFrameConnectionCloseTransport{
+			code: errTLSBase + 10,
+		})
+}