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

YaHTTP: Prevent integer overflow on very large chunks #12892

Merged
merged 1 commit into from Jun 13, 2023

Conversation

rgacogne
Copy link
Member

@rgacogne rgacogne commented Jun 6, 2023

Short description

If the chunk_size is very close to the maximum value of an integer, we trigger an integer overflow when checking if we have a trailing newline after the payload.
Reported by OSS-Fuzz as:

Upstream request at cmouse/yahttp#28

Checklist

I have:

  • read the CONTRIBUTING.md document
  • compiled this code
  • tested this code
  • included documentation (including possible behaviour changes)
  • documented the code
  • added or modified regression test(s)
  • added or modified unit test(s)

If the chunk_size is very close to the maximum value of an integer,
we trigger an integer overflow when checking if we have a trailing
newline after the payload.
Reported by OSS-Fuzz as:
https://oss-fuzz.com/testcase-detail/6439610474692608
https://bugs.chromium.org/p/oss-fuzz/issues/detail?id=56804
@@ -177,6 +179,9 @@ namespace YaHTTP {
throw ParseError("Unable to parse chunk size");
}
if (chunk_size == 0) { state = 3; break; } // last chunk
if (chunk_size > (std::numeric_limits<decltype(chunk_size)>::max() - 2)) {
Copy link
Contributor

Choose a reason for hiding this comment

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

- 2 for a potential \r\n? Perhaps a comment would help here.

Copy link
Member Author

Choose a reason for hiding this comment

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

Agreed, and since we try to keep this code in sync with the upstream repository if we want to make that change we should open a PR there :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants