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

Author: neild

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

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

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