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

If we fail to output anything when generating a datagram we should fail #22425

Closed
wants to merge 1 commit into from

Conversation

mattcaswell
Copy link
Member

If when generating a datagram we are unable to output anything then we are not making progress and we should fail, otherwise we may get into an infinite loop (i.e. continually trying to output a datagram and continually failing to do so in an infinite loop).

Fixes #22412

If when generating a datagram we are unable to output anything then we are
not making progress and we should fail, otherwise we may get into an
infinite loop (i.e. continually trying to output a datagram and continually
failing to do so in an infinite loop).

Fixes openssl#22412
@mattcaswell mattcaswell added branch: master Merge to master branch approval: review pending This pull request needs review by a committer approval: otc review pending This pull request needs review by an OTC member labels Oct 18, 2023
@mattcaswell
Copy link
Member Author

Should the size of the token be bounded in some way? I could not see anything in the RFC about this, but in the fuzz file that found this the token consumes most of the space that we have in the datagram leaving little space for anything else.

@t8m
Copy link
Member

t8m commented Oct 18, 2023

It would seem reasonable to me to fail the connection if we receive retry packets with such large token (after validating the retry integrity tag of course). This is a really pathological retry packet.

@t8m t8m added triaged: bug The issue/pr is/fixes a bug tests: exempted The PR is exempt from requirements for testing labels Oct 18, 2023
@hlandau
Copy link
Member

hlandau commented Oct 19, 2023

Working on an alternative fix

@hlandau
Copy link
Member

hlandau commented Oct 19, 2023

#22436

@kroeckx
Copy link
Member

kroeckx commented Oct 23, 2023

Should this get closed?

@t8m
Copy link
Member

t8m commented Oct 24, 2023

Yep, the alternative was merged.

@t8m t8m closed this Oct 24, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approval: otc review pending This pull request needs review by an OTC member approval: review pending This pull request needs review by a committer branch: master Merge to master branch tests: exempted The PR is exempt from requirements for testing triaged: bug The issue/pr is/fixes a bug
Projects
None yet
Development

Successfully merging this pull request may close these issues.

quic fuzzer hang
4 participants