Repository navigation
Conversation
`can_coalesce` added up `offset`, `length` and `MAX_HOLESIZE` of blob locations in `u32`. Offsets and lengths come from the index files and are not validated, so entries close to `u32::MAX` made the additions overflow: a panic with overflow checks enabled, and a wrapped (and thus wrong) coalescing decision in release builds. A wrongly coalesced pair makes `append` compute a bogus read length for the partial pack read used by restore, check, prune and copy. Use `checked_add` for the end of both locations and never coalesce when it overflows. The remaining comparisons are rewritten in a form that cannot underflow; they are equivalent to the previous ones for non-overflowing values. As `append` is only called after `can_coalesce` succeeded, its arithmetic is covered as well. Extend `test_coalesce` with cases at the end of the `u32` range.
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.
Problem
BlobLocations::can_coalescecomputesself.offset + self.length + MAX_HOLESIZEandother.offset + other.length - self.offsetinu32. The offsets and lengths come from the index files (and pack headers) and are not validated, so a corrupt or crafted entry close tou32::MAXmakes these additions overflow:attempt to add with overflow,appendthen computesother.offset + other.length - self.offsetfor the combined partial read, which can wrap as well.coalesceis used by restore, check, prune and copy to merge neighbouring blobs into a single partial pack read.This is the same class of problem as #585 (unchecked
u32arithmetic on values taken from the index), in a different place.Fix
Compute the end of both locations with
checked_addand returnfalseif either overflows. The three conditions are rewritten so that they cannot underflow (other.offset >= endis checked first, thenother.offset - end <= MAX_HOLESIZE, andother_end - self.offsetis safe becauseother_end >= other.offset >= end >= self.offset). For values that do not overflow they are equivalent to the old conditions.appendis only reached aftercan_coalescesucceeded, so itsother.offset + other.length - self.offsetcan no longer overflow either.Tests
test_coalescegets four more cases at the end of theu32range: a valid coalesce right belowu32::MAX, and three cases where the end of the first/second location overflows. The existing 11 cases are unchanged and still pass. Before the change the new cases fail withattempt to add with overflowatblob.rs:187.cargo fmt --checkandcargo clippyare clean for the touched code.cargo test --workspaceis green apart from five snapshot tests intests/integration(backup,chunker,rewrite) that fail identically on an unmodifiedmainbecause of anerror_countfield in the snapshots; they are unrelated to this change.Disclaimer: Found the issue by test-driving an LM-powered vulnerability scanner I work on heavily.