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

Do not use untrusted largest_ack #31

Merged

Conversation

mxinden
Copy link

@mxinden mxinden commented Oct 7, 2024

Note that this pull request does not consider the remote behavior as malicious. It simply ignores the largest ACK, if it doesn't correspond to a packet previously sent.

@larseggert
Copy link
Owner

Thanks for this! But I think receiving an ACK for an unsent packet is absolutely an error.

We used to have this code, but it seems like it got dropped at some point:
https://github.com/mozilla/neqo/blob/c112ddf18975c1263878899ab2098f8736ecd589/neqo-transport/src/connection.rs#L997-L1010

CC @martinthomson in case he remembers details.

@mxinden
Copy link
Author

mxinden commented Oct 7, 2024

That makes sense @larseggert.

What do you think of the latest commit?

This pull request now:

  • Moves the check from handle_ack up into input_frame. In other words, errors as early as possible.
  • No longer passes largest_acked to handle_ack, given that the information itself is already embedded in the handle_ack argument ack_ranges.

@mxinden
Copy link
Author

mxinden commented Oct 7, 2024

We used to have this code, but it seems like it got dropped at some point: https://github.com/mozilla/neqo/blob/c112ddf18975c1263878899ab2098f8736ecd589/neqo-transport/src/connection.rs#L997-L1010

For what it is worth, this is the commit removing the handling logic. Unfortunately it is not tied to a pull request. Thus I am not sure whether there was a reason behind removing it, or whether it was an oversight.

mozilla@3a26fae

@larseggert larseggert merged commit 11b8b17 into larseggert:fix-ack-for-unsent-pn Oct 7, 2024
2 checks passed
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