search

quinn-rs / quinn

Async-friendly QUIC implementation in Rust
Apache License 2.0
3.76k stars 380 forks source link

Quinn server does not discard all the Initial packets with a payload size smaller than the minimum allowed maximum datagram size of 1200 bytes. #1653

Closed QUICTester closed 5 months ago

QUICTester commented 1 year ago

Hi,

During our tests involving Quinn (4395b969) implementation, we identified a protocol violation in the server implementation.

Bug detail: The server only discards the first Initial packet if it's carried in a UDP datagram with a payload size smaller than the minimum allowed maximum datagram size of 1200 bytes. If a second packet does not meet the payload size requirement, the server does not discard the second packet.

Packet sequence to replicate this behavior:

1. The client sends an Initial packet carrying a Ping frame and Padding frame
2. The client sends an Initial packet carrying a CRYPTO frame (Client Hello) without the Padding frame.

Section 14.1, RFC 9000: "A server MUST discard an Initial packet that is carried in a UDP datagram with a payload that is smaller than the smallest allowed maximum datagram size of 1200 bytes."

Sending a UDP datagram of this size ensures that the network path supports a reasonable Path Maximum Transmission Unit (PMTU) in both directions, as QUIC MUST NOT be employed if the network path cannot handle a maximum datagram size of at least 1200 bytes. Although this can be confirmed in the first Initial packet, checking all the Initial packets will be better. This may also ensure the client uses a Padding frame to obscure the length of packet content (prevent any potential traffic analysis attack).

Fix: Discard all the Initial packets with a payload size smaller than the minimum allowed maximum datagram size of 1200 bytes.

djc commented 1 year ago

Looks like not everybody agrees this should be an issue: https://github.com/quic-go/quic-go/issues/4061#issuecomment-1702367744.

Maybe start a thread on the mailing list to see whether the spec should get an erratum?

(Also don't love the opaque user account, maybe use a real/personal account to interact with other people?)

QUICTester commented 1 year ago

Thank you for your reply. Agree.

(Sorry, our work are currently under double-blind reviewing process. We will add our contact information once our work is published.)

Ralith commented 5 months ago

@QUICTester was a consensus reached?

QUICTester commented 5 months ago

@Ralith Yes, we agreed it does not cause any impact on ensuring that the network path supports a reasonable Path Maximum Transmission Unit (PMTU). However, we still believe the specification needs to clarify this (to prevent ambiguity like this in the future). I think we can close this now.

Thank you.

Ralith commented 5 months ago

Thanks for the update!