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

Allow packets with impossible CIDs to be ignored rather than reset #1796

Merged
merged 3 commits into from
Apr 6, 2024

Conversation

Ralith
Copy link
Collaborator

@Ralith Ralith commented Mar 30, 2024

Reduces the likelihood of Quinn endpoints responding to non-QUIC packets.

I'm not sure whether this is needed, especially after #1794. However, the cost seems low, it's a more direct solution, and defense in depth is nice.

@Ralith Ralith changed the title Validate cid Allow packets with impossible CIDs to be ignored rather than reset Mar 30, 2024
@Ralith Ralith force-pushed the validate-cid branch 2 times, most recently from df5e177 to c66a24a Compare March 30, 2024 23:39
Copy link
Member

@djc djc left a comment

Choose a reason for hiding this comment

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

Nice!

quinn-proto/src/cid_generator.rs Outdated Show resolved Hide resolved
quinn-proto/src/cid_generator.rs Show resolved Hide resolved
quinn-proto/src/cid_generator.rs Outdated Show resolved Hide resolved
quinn-proto/src/cid_generator.rs Show resolved Hide resolved
quinn-proto/src/cid_generator.rs Outdated Show resolved Hide resolved
quinn-proto/src/endpoint.rs Show resolved Hide resolved
@djc
Copy link
Member

djc commented Apr 5, 2024

I also wonder if the cost of crypto here actually makes it less attractive than the random generator? @lijunwangs do you think this would help your use case (on top of the other options we've merged recently)?

@Ralith
Copy link
Collaborator Author

Ralith commented Apr 5, 2024

That's a good question. A more thoughtful choice of hash function, rather than just using whatever ring has lying around, might help. Maybe even just std's hasher, since we're not trying to be super secure here...

@Ralith
Copy link
Collaborator Author

Ralith commented Apr 5, 2024

For clarity, extra cost is paid here in exactly two places:

  • when generating a CID, which is gated behind the decision to respond to a connection attempt
  • sending a stateless reset for an unrecognized packet

@Ralith Ralith force-pushed the validate-cid branch 3 times, most recently from d103a2b to 0913219 Compare April 6, 2024 01:57
@Ralith
Copy link
Collaborator Author

Ralith commented Apr 6, 2024

Refactored to use a non-cryptographic hash.

@Ralith Ralith force-pushed the validate-cid branch 4 times, most recently from 17ea102 to ee100bf Compare April 6, 2024 02:01
@lijunwangs
Copy link
Contributor

I also wonder if the cost of crypto here actually makes it less attractive than the random generator? @lijunwangs do you think this would help your use case (on top of the other options we've merged recently)?

Thanks @djc/ @Ralith -- this will be a great add to strengthen the security. Is the change backward compatible? Can it break existing client code?

@Ralith
Copy link
Collaborator Author

Ralith commented Apr 6, 2024

It should be backwards-compatible, yeah.

@Ralith Ralith merged commit bbf68c5 into main Apr 6, 2024
8 checks passed
@Ralith Ralith deleted the validate-cid branch April 6, 2024 16:44
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.

3 participants