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 correctness issues for detectors #66

Merged
merged 2 commits into from
Jun 12, 2022
Merged

Conversation

deuill
Copy link
Contributor

@deuill deuill commented Jun 7, 2022

This PR includes commits that fix correctness issues for various detectors -- I'll include test cases for the more esoteric issues, but these have been replicated via cargo-fuzz in any case.

This commit fixes incorrect bounds checking for the `is_cr2` detector, which would previously check for data bufffers of `len() > 9`, but would access `buf[10]`, which is invalid for buffers of `len() == 10`.
@deuill
Copy link
Contributor Author

deuill commented Jun 7, 2022

I'm running fuzz tests against infer and have come against a few more issues, so I'll likely re-frame this PR to include commits across different detectors.

@deuill deuill marked this pull request as draft June 7, 2022 11:41
@deuill deuill changed the title Fix incorrect bounds checks for is_cr2 Fix correctness issues for detectors Jun 7, 2022
Detection for MS-OOXML files will attempt to load a 32-bit address from
the user-provided payload, adding a static offset that may lead to
overflow if the original address is already near the max value for the
type used.

This commit has starting offset calculation be safe via use of
`checked_add`, returning `None` if the user-provided address is beyond
the bounds allowed.
@deuill deuill marked this pull request as ready for review June 7, 2022 15:31
@@ -67,7 +67,12 @@ fn msooxml(buf: &[u8]) -> Option<DocType> {
// skip to the second local file header
// since some documents include a 520-byte extra field following the file
// header, we need to scan for the next header
let mut start_offset = (u32::from_le_bytes(buf[18..22].try_into().unwrap()) + 49) as usize;
let mut start_offset = match u32::from_le_bytes(buf[18..22].try_into().unwrap()).checked_add(49)
Copy link
Contributor Author

@deuill deuill Jun 7, 2022

Choose a reason for hiding this comment

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

Here's a file (generated by fuzz tests) that replicates this: overflow.docx

@bojand bojand merged commit 3f4f379 into bojand:master Jun 12, 2022
@bojand
Copy link
Owner

bojand commented Jun 12, 2022

Hello, thanks for the PR. Published in 0.8.1.

@deuill
Copy link
Contributor Author

deuill commented Jun 17, 2022

Thanks so much for the quick turnaround!

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.

None yet

2 participants