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

Initial packet number need not be zero #2462

Open
larseggert opened this issue Mar 3, 2025 · 5 comments · May be fixed by #2499
Open

Initial packet number need not be zero #2462

larseggert opened this issue Mar 3, 2025 · 5 comments · May be fixed by #2499
Assignees
Labels
enhancement New feature or request good first issue Good for newcomers help wanted Extra attention is needed

Comments

@larseggert
Copy link
Collaborator

We should investigate using a random non-zero small integer as the initial packet number for a connection, to increase entropy a bit.

@larseggert larseggert added enhancement New feature or request help wanted Extra attention is needed good first issue Good for newcomers labels Mar 3, 2025
@omansfeld
Copy link
Collaborator

I can work on this. Can you point me to the rough code location where one would implement this? I've just had a look around for ~30mins but haven't found an obvious spot yet.

@larseggert
Copy link
Collaborator Author

larseggert commented Mar 3, 2025

https://github.com/mozilla/neqo/blob/main/neqo-transport/src/crypto.rs#L430 and the code that is using it.

@omansfeld
Copy link
Collaborator

After experimenting with this a bit and looking at the relevant QUIC RFC section I've got some questions for clarification:

  1. Do I understand correctly that the intended behavior for this would be to randomize the starting packet number for the initial space of a new connection? Not the starting (e.g. initial) packet number of any packet number space. Meaning around "initial" can be a bit unclear here, thus the question.
  2. The RFC explicitly states Packet numbers in each space start at packet number 0. in the section linked above. Is this still something we'd want to investigate? I'd imagine this could lead to a bunch of breakage if servers make assumptions that we'd be starting from 0 when we're really not.
    This tracks with what I've seen so far. When implementing the change outlined in (1) I get a bunch of test failures, presumably because server and client have a different min_pn. Those go away when I set server and client to the same non-zero min_pn.
    My thought is that even making the random generation be higher up in the logic so server and client share a min_pn and therefore making the tests pass wouldn't mean much, because usually we're not in control of the server.

@omansfeld omansfeld self-assigned this Mar 6, 2025
@larseggert
Copy link
Collaborator Author

larseggert commented Mar 6, 2025

  1. Do I understand correctly that the intended behavior for this would be to randomize the starting packet number for the initial space of a new connection?

Yes. Only the initial space.

  1. The RFC explicitly states Packet numbers in each space start at packet number 0. in the section linked above. Is this still something we'd want to investigate? I'd imagine this could lead to a bunch of breakage if servers make assumptions that we'd be starting from 0 when we're really not.

Huh. I can't remember why this was added. Servers will need to support CI packets with numbers > 0, because if the CI with number 0 is lost, the first packet a server sees will have a higher number.

@martinthomson
Copy link
Member

The model that most implementation use is that the packets are effectively "lost". That's totally possible, if unlikely, so a receiver has to be able to deal with it. For Initial packets, that will push the receiver to ACK immediately, because it notes a gap, but receivers are required to ACK Initial packets immediately anyway.

In other words, this is totally fine. It's unnecessary (an adversary needs to observe a random connection ID to be able to spoof something), but it might lead to catching of injected packets if an attacker injects a packet 0 that was never sent. Part of this change therefore needs to be able to handle receipt of an ACK for a packet that was never sent and break the connection.

larseggert added a commit to larseggert/neqo that referenced this issue Mar 17, 2025
WIP. Two potential issues:
1. Did I mess up the validation in `CryptoDxState::continuation()`?
2. See the `FIXME` in `Version::confirm_version()`.

Fixes mozilla#2462

CC @omansfeld
@larseggert larseggert linked a pull request Mar 17, 2025 that will close this issue
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request good first issue Good for newcomers help wanted Extra attention is needed
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants