*: Reject malformed variable-length integers#2090
Merged
Merged
Conversation
5fd6b38 to
315abf5
Compare
hiddeco
commented
May 7, 2026
| // start of some binary data and returns the decoded number, the rest | ||
| // of the bytes, and an error if the encoded value does not fit in a | ||
| // uint. | ||
| func DecodeLEB128(input []byte) (uint, []byte, error) { |
Member
Author
There was a problem hiding this comment.
Member
There was a problem hiding this comment.
This package was actually introduced in v6, so never release exposed on a stable release.
315abf5 to
6c32091
Compare
pjbgf
reviewed
May 7, 2026
6c32091 to
7439d40
Compare
Variable-length integer decoders accepted continuation chains long enough to push the running shift past the bit-width of their accumulator, producing out-of-range values that propagated into buffer allocation hints and on-disk size fields. Mirror the bound from canonical Git's `unpack_object_header_buffer`[1]: reject any continuation byte that would shift past the type's capacity. The same rule applies to `VariableLengthSize`, `DecodeLEB128`, `DecodeLEB128FromReader` and `ReadVariableWidthInt`; `DecodeLEB128` now returns an error so callers surface invalid input rather than silently zeroing the result. Add defense-in-depth at every call site that allocates from a parsed length: cap `bytes.Buffer.Grow` hints in `Parser.parentReader` and `patchDeltaWriter` at 1 GiB / `maxPatchPreemptionSize`, cap `parserCache.Reset` at a 4 Mi-entry hint so a hostile `ObjectsQty` cannot request a multi-gigabyte slice, validate `OFS-delta` back-references in the streaming and mmap paths (must be `> 0` and `<= o.offset`), and reject `mmap.fsobject` sizes whose top bit is set rather than truncating into a negative `int64`. Cover the new behavior with unit, parser-level regression and fuzz tests (`FuzzVariableLengthSize`, `FuzzDecodeLEB128`, `FuzzParser`). [1]: https://github.com/git/git/blob/v2.54.0/packfile.c#L1135-L1158 Assisted-by: Claude Opus 4.7 Signed-off-by: Hidde Beydals <hidde@hhh.computer>
7439d40 to
5b89cb5
Compare
hiddeco
added a commit
to hiddeco/go-git
that referenced
this pull request
May 7, 2026
Variable-length integer decoders accepted continuation chains long enough to push the running shift past the bit-width of their accumulator, producing out-of-range values that propagated into buffer allocation hints and on-disk size fields. Mirror the bound from canonical Git's `unpack_object_header_buffer`[1]: reject any continuation byte that would shift past the type's capacity. The same rule applies to `Scanner.readLength`, the package-local `decodeLEB128`/`decodeLEB128ByteReader`, and `ReadVariableWidthInt`; `decodeLEB128` now returns an error so callers surface invalid input rather than silently zeroing the result. Add defense-in-depth at every call site that allocates from a parsed length: cap `bytes.Buffer.Grow` hints in the parser at 1 GiB, clamp the slice and maps initialized from `Parser.count` at a 4 Mi-entry hint so a hostile object count cannot request a multi-gigabyte allocation, and validate `OFS-delta` back-references in the scanner (must be `> 0` and `<= h.Offset`). Cover the new behavior with unit, parser-level regression and fuzz tests (`FuzzReadLength`, `FuzzDecodeLEB128`, `FuzzDecodeLEB128ByteReader`, `FuzzParser`). [1]: https://github.com/git/git/blob/v2.54.0/packfile.c#L1135-L1158 Backport of go-git#2090. Assisted-by: Claude Opus 4.7 Signed-off-by: Hidde Beydals <hidde@hhh.computer>
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
2 participants
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.
Each delta operation's declared size is validated against the size still remaining in the target buffer, not the total target size. The previous check against the total accepted a sequence of operations whose cumulative output would exceed the declared target, leaving the unsigned remaining counter unable to reach zero and the loop free to keep writing past the declared target until the delta payload was consumed.
Three sibling code paths shared the flaw and are updated together:
patchDelta(used byPatchDeltaandApplyDelta),ReaderFromDelta, andpatchDeltaWriter(used by the packfile parser).The loop structure is rewritten as
for remainingTargetSz > 0so the invariant is explicit, and a post-loop check rejects deltas that leave bytes unconsumed -- matching thedata != topsanity check inpatch-delta.c1. Empty-target deltas (header-only, no operations) are now accepted, also matching upstream.Several incidental defects on the same call paths are fixed:
patchDelta's copy-from-src failure usedbreak, which only exited the switch and let the loop keep consuming delta bytes;patchDeltaWriterreturned(0, ZeroHash, nil)on invalid input, silently reporting success; andReaderFromDelta's copy-from-src failure closed the writer without an error, silently truncating the consumer stream. Each now propagatesErrInvalidDelta(orErrDeltaCmd) consistently.packfile.getMemoryObjectwas re-inflating delta payloads into the buffer the scanner had already populated, appending a duplicate copy that previously went unnoticed because the loop silently ignored anything past the target. The fix matches the cached-content pattern already used inScanner.WriteObject.