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

fix: overflow panics on interesting patches #5

Merged
merged 2 commits into from
Sep 18, 2023

Conversation

sno2
Copy link
Contributor

@sno2 sno2 commented Sep 17, 2023

The patch function would sometimes panic when certain inputs was passed into it. This fixes the issue using the saturating arithmetic functions.

Note: I just realized that this does not affect the crates.io crate because we never released 0.2.0. Therefore, this just removes the vulnerable code in case a release is ever created.

The `patch` function would sometimes panic when certain inputs was
passed into it. This fixes the issue using the saturating arithmetic
functions.
@kornelski
Copy link
Collaborator

Maybe overflow should be checked and rejected as a hard error? There's nothing useful to do with overflowing sizes, especially on 64-bit where this can't realistically happen with real data.

LLVM supports allocations only up to isize::MAX, so this could be used as an early check (isize::MAX + isize::MAX doesn't overflow usize).

I agree this should be patched, but I don't think this is a vulnerability. Rust enforces bounds checks on slices either way, so you can only get error or garbage data. With the saturating arithmetic you'll get different garbage data. To cause overflow the attacker has to provide invalid sizes, so the attacker is only mangling their own data.

Reworks the overflow handling to return an error instead of using the
saturating arithmetic strategy.
@kornelski kornelski merged commit 9fffac6 into space-wizards:master Sep 18, 2023
1 check 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