Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat(s2n-quic-transport): discard client initial keys when we have handshake keys #1894

Merged
merged 6 commits into from
Aug 4, 2023

Conversation

WesleyRosenblum
Copy link
Contributor

@WesleyRosenblum WesleyRosenblum commented Aug 1, 2023

Description of changes:

QUIC-TLS§4.9.1 makes it clear that Initial secrets should be discarded aggressively:

Packets protected with Initial secrets (Section 5.2) are not authenticated, meaning that an attacker could spoof packets with the intent to disrupt a connection. To limit these attacks, Initial packet protection keys are discarded more aggressively than other keys.

The successful use of Handshake packets indicates that no more Initial packets need to be exchanged, as these keys can only be produced after receiving all CRYPTO frames from Initial packets. Thus, a client MUST discard Initial keys when it first sends a Handshake packet and a server MUST discard Initial keys when it first successfully processes a Handshake packet. Endpoints MUST NOT send Initial packets after this point.

Currently, the s2n-quic client waits until we have successfully sent a Handshake packet to discard Initial keys. If for some reason we are unable to send the Handshake packet or did not need to send a Handshake packet, the Initial keys are retained, even though they are no longer needed. This could happen, for example, if the Server's Handshake packet was lost.

This change will discard the Initial keys on the client as long as we have installed the Handshake keys, reducing the window in which the type of attacks mentioned in the RFC are possible.

Call-outs:

While making this change, I noticed that in the scenario where the server's handshake packet is lost, we do not arm the PTO timer in the Handshake space. This violates this requirement from QUIC-RECOVERY§6.2.2:

When the PTO fires, the client MUST send a Handshake packet if it has Handshake keys, otherwise it MUST send an Initial packet in a UDP datagram with a payload of at least 1200 bytes.

This becomes more problematic if the Initial space is discarded without sending a Handshake packet, as no PTO timer will be set at all. To address this, I have explicitly reset the PTO timer in the Handshake space when the Initial space is discarded. This should also address the issues that were previously fixed in #1818. Once this PR is merged, we can consider reverting those changes if they are no longer necessary.

Testing:

Added an integration test

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

@WesleyRosenblum WesleyRosenblum marked this pull request as ready for review August 2, 2023 01:46
@WesleyRosenblum WesleyRosenblum changed the title feat(s2n-quic-transport): discard initial sooner feat(s2n-quic-transport): discard client initial keys when we have handshake keys Aug 2, 2023
Comment on lines +79 to +80
/// Drops all outgoing handshake packets
struct DropHandshakeTx;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great use of this feature!

@WesleyRosenblum WesleyRosenblum merged commit 1ff2cd2 into main Aug 4, 2023
128 of 129 checks passed
@WesleyRosenblum WesleyRosenblum deleted the WesleyRosenblum/discardinitial branch August 4, 2023 21:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants