security: cap untrusted-length allocations, add cargo-fuzz harness, fix overflow panics#32
Merged
Merged
Conversation
…ix overflow panics Caps every length-prefixed read and file-derived scan/error-log/parameter count against the file's actual remaining size before allocating, closing the memory-DoS vector described in #26 (a crafted/corrupt RAW file could previously request an allocation sized from an attacker-controlled count with no relation to the file's real size). Adds a cargo-fuzz harness (crates/opentfraw/fuzz) with two targets, `open` and `read_scan_peaks`, per #23 and #26. Fuzzing it immediately found three real crash bugs, now fixed: - u32 multiplication overflow in PacketHeader label-stream skip lengths (descriptor_list_size / unknown_stream_size * 4 computed in u32 before widening). - u32 addition overflow computing num_scans from first_scan/last_scan. - u64 addition overflow combining data_addr with a scan offset, and a u32 subtraction underflow converting a requested scan number to a scan-index offset when it fell below the file's first_scan_number. Also adds unit tests for the pure-decode functions in bytes.rs, header.rs, audit_tag.rs, generic_data.rs, and error_log.rs (issue #23), plus regression tests for the fixes above in reader.rs and scan_data.rs, and a 60-second fuzz smoke-test CI job gated on crates/opentfraw/src/** changing on pull requests. Closes #26, Closes #23
Deploying with
|
| Status | Name | Latest Commit | Updated (UTC) |
|---|---|---|---|
| ✅ Deployment successful! View logs |
opentfraw-docs | 0aeddd6 | Jul 19 2026, 01:33 PM |
…ons-and-fuzz # Conflicts: # .github/workflows/ci.yml # CHANGELOG.md
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Handles #26 and #23 together since both need the same cargo-fuzz scaffolding.
BinaryReader::read_bytes) and every file-derived scan/error-log/scan-parameter count against the file's actual remaining size before allocating, instead of allocating first and letting the subsequent read fail. Closes the memory-DoS vector in Cap untrusted-length allocations in the low-level reader; no fuzz harness exists #26.cargo-fuzzharness atcrates/opentfraw/fuzzwith two targets:open(RawFileReader::open) andread_scan_peaks(open + read every declared scan's peaks) - the two fully-adversarial-input entry points named in Add unit tests (and a fuzz harness) for the binary decoders #23/Cap untrusted-length allocations in the low-level reader; no fuzz harness exists #26.PacketHeaderlabel-stream skip lengths (descriptor_list_size/unknown_stream_size * 4computed in u32 before widening tousize).num_scansfromfirst_scan/last_scan(extracted to a testablecompute_num_scanshelper).data_addrwith a scan offset, and a u32 subtraction underflow converting a requested scan number to a scan-index offset when it fell below the file's declaredfirst_scan_number(extracted toscan_index_entry/scan_number_to_index).bytes.rs,header.rs,audit_tag.rs,generic_data.rs, anderror_log.rsper Add unit tests (and a fuzz harness) for the binary decoders #23, using hand-built byte fixtures (valid/truncated/out-of-range), no vendor comparison per CONTRIBUTING.md's clean-room policy.fuzz-smokeCI job: runs both fuzz targets for a fixed 60s each, gated oncrates/opentfraw/src/**changing on pull requests (separate from any longer scheduled/manual run), seeded from a small real RAW fixture via the same PRIDE download patternvalidate-mzmlalready uses.Note: this branch's working tree also had pre-existing uncommitted changes unrelated to this task (a Python pytest suite referencing issue #21, with no
tests/directory present - appears to be orphaned WIP from an earlier session). Those were deliberately left uncommitted/untouched and are not part of this PR.Test plan
cargo fmt --all -- --checkcargo clippy --workspace --all-targets -- -D warningscargo test --workspace --all-targets(100 unit tests + conformance test, all passing)cargo +nightly clippy/fmt --checkclean on the fuzz crate itselfRawFileReader::open+dumpexample against 13 real Thermo RAW files spanning every instrument family in the local corpus (LCQ, LTQ, Orbitrap Fusion/Eclipse/Exploris/Ascend/Astral, Q Exactive, TSQ Altis/Quantiva) - no regressions from the allocation capscargo fuzz run open/read_scan_peaksfor 60-180s each with a real-file-seeded corpus - clean (no crashes) after the three bugs above were fixedfuzz-smokejob and the rest of the matrixCloses #26, Closes #23