Repository: quic-go/quic-go

Issue:
Unable to reset connection with zero-length connection ID with stateless reset token
Because quic-go deviates from the RFC, stateless resets are never handled if the connection ID has length zero. `handlerMap.Get` always returns a handler, but the encryption will fail afterwards. No further reset token checks are performed. The same applies to short connection IDs, which are likely for clients. I would argue that collisions are not rare. RFC 9000 also says that if decryption fails, the token must be checked.

Suggestion:
Add a callback `OnDecryptionFailed` from the connections to the transport to handle stateless resets.

Sources:

From RFC 9000 10.3.1. Detecting a Stateless Reset:

> This comparison can be performed for every inbound datagram. Endpoints MAY skip this check if any packet from a datagram is successfully processed. However, the comparison MUST be performed when the first packet in an incoming datagram either cannot be associated with a connection or cannot be decrypted.

From transport.go:
```go
if handler, ok := t.handlerMap.Get(connID); ok {
	handler.handlePacket(p)
	return
}
// RFC 9000 section 10.3.1 requires that the stateless reset detection logic is run for both
// packets that cannot be associated with any connections, and for packets that can't be decrypted.
// We deviate from the RFC and ignore the latter: If a packet's connection ID is associated with an
// existing connection, it is dropped there if if it can't be decrypted.
// Stateless resets use random connection IDs, and at reasonable connection ID lengths collisions are
// exceedingly rare. In the unlikely event that a stateless reset is misrouted to an existing connection,
// it is to be expected that the next stateless reset will be correctly detected.
if isStatelessReset := t.maybeHandleStatelessReset(p.data); isStatelessReset {
	return
}
```


Discussion:
@marten-seemann: Checking the stateless reset token before the connection ID is a pretty large performance penalty (2 instead of 1 map lookups), happening in the hot path of the transport, so that's a non-starter.

I could get behind your suggestion to add a `OnDecryptionFailed` callback. We'd need to be very careful about handling coalesced packets though, I'd argue that the callback should only be called if _none_ of the packets can be decrypted.

Want to send us a PR?@birneee: Can you explain why you think it should only be checked if every decryption fails?

A few thoughts on the RFC:

> However, the comparison MUST be performed when the first packet in an incoming datagram either cannot be associated with a connection or cannot be decrypted.

not sure why only looking at the first packet in a datagram, as only the last one could be a stateless reset because it is short-header-like.

> This design ensures that a Stateless Reset is -- to the extent possible -- indistinguishable from a regular packet with a short header.

a callback `OnShortHeaderPacketDecryptionFailed` to the transport should be sufficient

> Senders MUST NOT coalesce QUIC packets with different connection IDs into a single UDP datagram

From the RFC it is not clear to me if coalesced packets can contain stateless resets.
From a network perspective, a stateless reset packet could look like a packet with the same connection ID.
If it does not start with the same connection ID, it think it is fine to drop it.

> The receiver of coalesced QUIC packets MUST individually process each QUIC packet and separately acknowledge them, as if they were received as the payload of different UDP datagrams.

> if decryption fails [...] the receiver [...] MUST attempt to process the remaining packets.

as I understand, a valid behavior would be to process the first packets in a datagram and then detect a stateless reset in the last packet because it cannot be decrypted. I am not sure why someone would send such a packet, but it would require checking only the last packet independently of the previous ones in the datagram.

Suggestion:

let the connection call back via `OnShortHeaderPacketDecryptionFailed` to the transport, if the decryption of any short header packet fails. The only exception is if the stateless reset is coalesced and it does not look like a short header packet with the same connection id as the preceding packets, then it will be dropped.@marten-seemann: > A few thoughts on the RFC:
> 
> > However, the comparison MUST be performed when the first packet in an incoming datagram either cannot be associated with a connection or cannot be decrypted.
> 
> not sure why only looking at the first packet in a datagram, as only the last one could be a stateless reset because it is short-header-like.

Some implementations satisfy the 1200 bytes minimum packet size requirement by appending some garbage data (or all 0s) to a coalesced packet. This is a valid implementation, and it shouldn't lead to a check for a stateless reset token.



> let the connection call back via `OnShortHeaderPacketDecryptionFailed` to the transport, if the decryption of any short header packet fails. The only exception is if the stateless reset is coalesced and it does not look like a short header packet with the same connection id as the preceding packets, then it will be dropped.

Sounds good to me, modulo (maybe) the caveat for coalesced packets.@birneee: > Some implementations satisfy the 1200 bytes minimum packet size requirement by appending some garbage data (or all 0s) to a coalesced packet. 

Ok appending random data makes sense to me, but still not why some packet of the coalesced packet has to be checked for a reset token if the **first** long header packet could not be decrypted.

> This is a valid implementation, and it shouldn't lead to a check for a stateless reset token.

I don't see the problem, if by chance the random garbage data looks like a short header packet with the right connection id, the packet must even be tried to be decrypted, so checking for the reset token is negligible.@marten-seemann: > > This is a valid implementation, and it shouldn't lead to a check for a stateless reset token.
> 
> I don't see the problem, if by chance the random garbage data looks like a short header packet with the right connection id, the packet must even be tried to be decrypted, so checking for the reset token is negligible.

Yes, you're right about that. My argument is not a performance argument. My point is that a coalesced packet can _never_ be a stateless reset, because that's what RFC 9000 specifies: The stateless reset is a packet on its own.@birneee: Oh, now I see. And even stateless resets in datagrams starting with a long header must be detected. So it is true that a stateless reset can _never be part_ of a coalesced packet.  But technically, packets that look like coalesced packets can actually _be_ a stateless reset packet. So `OnShortHeaderPacketDecryptionFailed` is not sufficient. Better would be `OnFirstDatagramPacketDecryptionFailed`, which then causes the transport to check the entire datagram.

From RFC 10.3:
> A Stateless Reset uses an entire UDP datagram

> Endpoints MUST send Stateless Resets formatted as a packet with a short header. However, endpoints MUST treat any packet ending in a valid stateless reset token as a Stateless Reset, as other QUIC versions might allow the use of a long header.@marten-seemann: We don't have to deal with this caveat as long as we don't support a QUIC version that allows long header stateless resets, do we?

That would simplify things, as there's no such QUIC version around, nor do I expect such a QUIC version to emerge anytime soon.@birneee: In general, it conflicts with the statement in RFC 9000 about future versions of QUIC.
It also seems logical to me that stateless resets should be detected regardless of the used QUIC version, since the endpoint sending the token is unlikely to have any knowledge of the supported QUIC versions of the peer.
However, RFC 8999 does not mention anything about this.

So I think it should be fine for now to just check non-coalesced short-header packets.@marten-seemann: Sounds like a plan! Are you going to work on a PR?@birneee: I will, but it's not super high on my priority list@marten-seemann: This issue has come back to bite me... and cost me a few hours of debugging work. This will not happen again! 🤬

I'm putting it on the list for the next milestone.@marten-seemann: > But technically, packets that look like coalesced packets can actually be a stateless reset packet

Rereading this thread, now I understand! The random data of a packet could look like a coalesced packet.

While that's true, I'm not sure if it's worth catching. It's totally valid to discard a fraction of stateless resets, as long as we don't discard all of them (as we currently do in the case of zero-length CIDs). After all, the packet could have been lost in the network. We'll just send another packet and receive another stateless reset in response.@marten-seemann: Instead of callback that goes back from the connection to the `Transport`, we could also have the `connIDManager` keep track of the stateless resets assigned to the connection. This number is limited by the `active_connection_id_limit`.@marten-seemann: > Rereading this thread, now I understand! The random data of a packet could look like a coalesced packet.

This is wrong though. It cannot look like a coalesced packet. Here's why:

RFC 9000 Section 10.3 specifies how a Stateless Reset looks like:
```
Stateless Reset {
  Fixed Bits (2) = 1,
  Unpredictable Bits (38..),
  Stateless Reset Token (128),
}
```

The first 2 bits (the Fixed Bits), are 1 (in network byte order), i.e. the first bit is `0b01xxxxxx`. Therefore a stateless reset always looks like a short header packet.

The RFC is also very clear that a stateless reset can never be coalesced:
> A Stateless Reset uses an entire UDP datagram, starting with the first two bits of the packet header.

Solution patch:
diff --git a/conn_id_manager.go b/conn_id_manager.go
index 4ca436a6..5513b645 100644
--- a/conn_id_manager.go
+++ b/conn_id_manager.go
@@ -294,6 +294,22 @@ func (h *connIDManager) RetireConnIDForPath(pathID pathID) {
 	delete(h.pathProbing, pathID)
 }
 
+func (h *connIDManager) IsActiveStatelessResetToken(token protocol.StatelessResetToken) bool {
+	if h.activeStatelessResetToken != nil {
+		if *h.activeStatelessResetToken == token {
+			return true
+		}
+	}
+	if h.pathProbing != nil {
+		for _, entry := range h.pathProbing {
+			if entry.StatelessResetToken == token {
+				return true
+			}
+		}
+	}
+	return false
+}
+
 // Using the connIDManager after it has been closed can have disastrous effects:
 // If the connection ID is rotated, a new entry would be inserted into the packet handler map,
 // leading to a memory leak of the connection struct.
diff --git a/connection.go b/connection.go
index 4ed639ac..8ca6ae5c 100644
--- a/connection.go
+++ b/connection.go
@@ -977,7 +977,7 @@ func (s *connection) handleOnePacket(rp receivedPacket) (wasProcessed bool, _ er
 			if counter > 0 {
 				p.buffer.Split()
 			}
-			processed, err := s.handleShortHeaderPacket(p)
+			processed, err := s.handleShortHeaderPacket(p, counter > 0)
 			if err != nil {
 				return false, err
 			}
@@ -992,7 +992,7 @@ func (s *connection) handleOnePacket(rp receivedPacket) (wasProcessed bool, _ er
 	return wasProcessed, nil
 }
 
-func (s *connection) handleShortHeaderPacket(p receivedPacket) (wasProcessed bool, _ error) {
+func (s *connection) handleShortHeaderPacket(p receivedPacket, isCoalesced bool) (wasProcessed bool, _ error) {
 	var wasQueued bool
 
 	defer func() {
@@ -1009,6 +1009,17 @@ func (s *connection) handleShortHeaderPacket(p receivedPacket) (wasProcessed boo
 	}
 	pn, pnLen, keyPhase, data, err := s.unpacker.UnpackShortHeader(p.rcvTime, p.data)
 	if err != nil {
+		// Stateless reset packets (see RFC 9000, section 10.3):
+		// * fill the entire UDP datagram (i.e. they cannot be part of a coalesced packet)
+		// * are short header packets (first bit is 0)
+		// * have the QUIC bit set (second bit is 1)
+		// * are at least 21 bytes long
+		if !isCoalesced && len(p.data) >= protocol.MinReceivedStatelessResetSize && p.data[0]&0b11000000 == 0b01000000 {
+			token := protocol.StatelessResetToken(p.data[len(p.data)-16:])
+			if s.connIDManager.IsActiveStatelessResetToken(token) {
+				return false, &StatelessResetError{}
+			}
+		}
 		wasQueued, err = s.handleUnpackError(err, p, logging.PacketType1RTT)
 		return false, err
 	}
diff --git a/internal/protocol/protocol.go b/internal/protocol/protocol.go
index d056cb9d..b1109ff4 100644
--- a/internal/protocol/protocol.go
+++ b/internal/protocol/protocol.go
@@ -123,6 +123,10 @@ const MinUnknownVersionPacketSize = MinInitialPacketSize
 // MinStatelessResetSize is the minimum size of a stateless reset packet that we send
 const MinStatelessResetSize = 1 /* first byte */ + 20 /* max. conn ID length */ + 4 /* max. packet number length */ + 1 /* min. payload length */ + 16 /* token */
 
+// MinReceivedStatelessResetSize is the minimum size of a received stateless reset,
+// as specified in section 10.3 of RFC 9000.
+const MinReceivedStatelessResetSize = 5 + 16
+
 // MinConnectionIDLenInitial is the minimum length of the destination connection ID on an Initial packet.
 const MinConnectionIDLenInitial = 8
 



Test patch:
diff --git a/conn_id_manager_test.go b/conn_id_manager_test.go
index 4e7b1ca4..e38fed40 100644
--- a/conn_id_manager_test.go
+++ b/conn_id_manager_test.go
@@ -154,22 +154,34 @@ func TestConnIDManagerHandshakeCompletion(t *testing.T) {
 }
 
 func TestConnIDManagerConnIDRotation(t *testing.T) {
+	toToken := func(connID protocol.ConnectionID) protocol.StatelessResetToken {
+		var token protocol.StatelessResetToken
+		copy(token[:], connID.Bytes())
+		copy(token[connID.Len():], connID.Bytes())
+		return token
+	}
+
 	var frameQueue []wire.Frame
+	var addedTokens, removedTokens []protocol.StatelessResetToken
 	m := newConnIDManager(
 		protocol.ParseConnectionID([]byte{1, 2, 3, 4}),
-		func(protocol.StatelessResetToken) {},
-		func(protocol.StatelessResetToken) {},
+		func(token protocol.StatelessResetToken) { addedTokens = append(addedTokens, token) },
+		func(token protocol.StatelessResetToken) { removedTokens = append(removedTokens, token) },
 		func(f wire.Frame) { frameQueue = append(frameQueue, f) },
 	)
 	// the first connection ID is used as soon as the handshake is complete
 	m.SetHandshakeComplete()
+	firstConnID := protocol.ParseConnectionID([]byte{4, 3, 2, 1})
 	require.NoError(t, m.Add(&wire.NewConnectionIDFrame{
 		SequenceNumber:      1,
-		ConnectionID:        protocol.ParseConnectionID([]byte{4, 3, 2, 1}),
-		StatelessResetToken: protocol.StatelessResetToken{16, 15, 14, 13, 12, 11, 10, 9, 8, 7, 6, 5, 4, 3, 2, 1},
+		ConnectionID:        firstConnID,
+		StatelessResetToken: toToken(protocol.ParseConnectionID([]byte{4, 3, 2, 1})),
 	}))
-	require.Equal(t, protocol.ParseConnectionID([]byte{4, 3, 2, 1}), m.Get())
+	require.Equal(t, firstConnID, m.Get())
 	frameQueue = nil
+	require.True(t, m.IsActiveStatelessResetToken(toToken(firstConnID)))
+	require.Equal(t, addedTokens, []protocol.StatelessResetToken{toToken(firstConnID)})
+	addedTokens = addedTokens[:0]
 
 	// Note that we're missing the connection ID with sequence number 2.
 	// It will be received later.
@@ -182,8 +194,9 @@ func TestConnIDManagerConnIDRotation(t *testing.T) {
 		require.NoError(t, m.Add(&wire.NewConnectionIDFrame{
 			SequenceNumber:      uint64(3 + i),
 			ConnectionID:        connID,
-			StatelessResetToken: protocol.StatelessResetToken{16, 15, 14, 13, 12, 11, 10, 9, 8, 7, 6, 5, 4, 3, 2, 1},
+			StatelessResetToken: toToken(connID),
 		}))
+		require.False(t, m.IsActiveStatelessResetToken(toToken(connID)))
 	}
 
 	var counter int
@@ -191,11 +204,19 @@ func TestConnIDManagerConnIDRotation(t *testing.T) {
 		require.Empty(t, frameQueue)
 		m.SentPacket()
 		counter++
-		if m.Get() != protocol.ParseConnectionID([]byte{4, 3, 2, 1}) {
+		if connID := m.Get(); connID != firstConnID {
 			require.Equal(t, queuedConnIDs[0], m.Get())
 			require.Equal(t, []wire.Frame{&wire.RetireConnectionIDFrame{SequenceNumber: 1}}, frameQueue)
+			require.Equal(t, removedTokens, []protocol.StatelessResetToken{toToken(firstConnID)})
+			require.Equal(t, addedTokens, []protocol.StatelessResetToken{toToken(connID)})
+			addedTokens = addedTokens[:0]
+			removedTokens = removedTokens[:0]
+			require.True(t, m.IsActiveStatelessResetToken(toToken(connID)))
+			require.False(t, m.IsActiveStatelessResetToken(toToken(firstConnID)))
 			break
 		}
+		require.True(t, m.IsActiveStatelessResetToken(toToken(firstConnID)))
+		require.Empty(t, addedTokens)
 	}
 	require.GreaterOrEqual(t, counter, protocol.PacketsPerConnectionID/2)
 	require.LessOrEqual(t, counter, protocol.PacketsPerConnectionID*3/2)
@@ -223,7 +244,7 @@ func TestConnIDManagerPathMigration(t *testing.T) {
 	_, ok := m.GetConnIDForPath(1)
 	require.False(t, ok)
 
-	// add a connection ID
+	// add two connection IDs
 	require.NoError(t, m.Add(&wire.NewConnectionIDFrame{
 		SequenceNumber:      1,
 		ConnectionID:        protocol.ParseConnectionID([]byte{4, 3, 2, 1}),
@@ -241,11 +262,13 @@ func TestConnIDManagerPathMigration(t *testing.T) {
 	require.Empty(t, removedTokens)
 
 	addedTokens = addedTokens[:0]
+	require.False(t, m.IsActiveStatelessResetToken(protocol.StatelessResetToken{5, 4, 3, 2, 5, 4, 3, 2}))
 	connID, ok = m.GetConnIDForPath(2)
 	require.True(t, ok)
 	require.Equal(t, protocol.ParseConnectionID([]byte{5, 4, 3, 2}), connID)
 	require.Equal(t, []protocol.StatelessResetToken{{5, 4, 3, 2, 5, 4, 3, 2}}, addedTokens)
 	require.Empty(t, removedTokens)
+	require.True(t, m.IsActiveStatelessResetToken(protocol.StatelessResetToken{5, 4, 3, 2, 5, 4, 3, 2}))
 
 	addedTokens = addedTokens[:0]
 	// asking for the connection for path 1 again returns the same connection ID
@@ -294,12 +317,14 @@ func TestConnIDManagerPathMigration(t *testing.T) {
 	require.Equal(t, protocol.ParseConnectionID([]byte{7, 6, 5, 4}), connID)
 	require.Equal(t, []protocol.StatelessResetToken{{16, 15, 14, 13}}, addedTokens)
 	require.Empty(t, removedTokens)
+	require.True(t, m.IsActiveStatelessResetToken(protocol.StatelessResetToken{16, 15, 14, 13}))
 
 	// a RETIRE_CONNECTION_ID frame for path 1 is queued when retiring the connection ID
 	m.RetireConnIDForPath(1)
 	require.Equal(t, []wire.Frame{&wire.RetireConnectionIDFrame{SequenceNumber: 4}}, frameQueue)
 	require.Equal(t, []protocol.StatelessResetToken{{16, 15, 14, 13}}, removedTokens)
 	removedTokens = removedTokens[:0]
+	require.False(t, m.IsActiveStatelessResetToken(protocol.StatelessResetToken{16, 15, 14, 13}))
 
 	m.Close()
 	require.Equal(t, []protocol.StatelessResetToken{
diff --git a/integrationtests/self/stateless_reset_test.go b/integrationtests/self/stateless_reset_test.go
index 44510280..744a10d3 100644
--- a/integrationtests/self/stateless_reset_test.go
+++ b/integrationtests/self/stateless_reset_test.go
@@ -14,7 +14,7 @@ import (
 )
 
 func TestStatelessResets(t *testing.T) {
-	t.Run("0 byte connection IDs", func(t *testing.T) {
+	t.Run("zero-length connection IDs", func(t *testing.T) {
 		testStatelessReset(t, 0)
 	})
 	t.Run("10 byte connection IDs", func(t *testing.T) {
@@ -28,9 +28,8 @@ func testStatelessReset(t *testing.T, connIDLen int) {
 
 	c := newUDPConnLocalhost(t)
 	tr := &quic.Transport{
-		Conn:               c,
-		StatelessResetKey:  &statelessResetKey,
-		ConnectionIDLength: connIDLen,
+		Conn:              c,
+		StatelessResetKey: &statelessResetKey,
 	}
 	defer tr.Close()
 	ln, err := tr.Listen(getTLSConfig(), getQuicConfig(nil))
@@ -65,18 +64,31 @@ func testStatelessReset(t *testing.T, connIDLen int) {
 	require.NoError(t, proxy.Start())
 	defer proxy.Close()
 
-	cl := &quic.Transport{
-		Conn:               newUDPConnLocalhost(t),
-		ConnectionIDLength: connIDLen,
+	var conn quic.Connection
+	if connIDLen > 0 {
+		cl := &quic.Transport{
+			Conn:               newUDPConnLocalhost(t),
+			ConnectionIDLength: connIDLen,
+		}
+		defer cl.Close()
+		var err error
+		conn, err = cl.Dial(
+			context.Background(),
+			proxy.LocalAddr(),
+			getTLSClientConfig(),
+			getQuicConfig(&quic.Config{MaxIdleTimeout: 2 * time.Second}),
+		)
+		require.NoError(t, err)
+	} else {
+		conn, err = quic.Dial(
+			context.Background(),
+			newUDPConnLocalhost(t),
+			proxy.LocalAddr(),
+			getTLSClientConfig(),
+			getQuicConfig(&quic.Config{MaxIdleTimeout: 2 * time.Second}),
+		)
+		require.NoError(t, err)
 	}
-	defer cl.Close()
-	conn, err := cl.Dial(
-		context.Background(),
-		proxy.LocalAddr(),
-		getTLSClientConfig(),
-		getQuicConfig(&quic.Config{MaxIdleTimeout: 2 * time.Second}),
-	)
-	require.NoError(t, err)
 	str, err := conn.AcceptStream(context.Background())
 	require.NoError(t, err)
 	data := make([]byte, 6)
@@ -94,9 +106,8 @@ func testStatelessReset(t *testing.T, connIDLen int) {
 	// We need to create a new Transport here, since the old one is still sending out
 	// CONNECTION_CLOSE packets for (recently) closed connections).
 	tr2 := &quic.Transport{
-		Conn:               c,
-		ConnectionIDLength: connIDLen,
-		StatelessResetKey:  &statelessResetKey,
+		Conn:              c,
+		StatelessResetKey: &statelessResetKey,
 	}
 	defer tr2.Close()
 	ln2, err := tr2.Listen(getTLSConfig(), getQuicConfig(nil))