Unable to reset connection with zero-length connection ID with stateless reset token

[birneee]

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:

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
}

[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