ConsumeBytes: return nil on out-of-bounds read instead of allocating the requested size - #43
Merged
Conversation
|
|
oioio-space
force-pushed
the
consume-bytes-nil-on-oob
branch
from
September 4, 2026 17:05
5506703 to
f37794d
Compare
scudette
reviewed
Sep 5, 2026
…the requested size Follow-up to Velocidex#40 / Velocidex#41. The out-of-bounds path returned make([]byte, size) where size comes straight from the stream, so a malformed record could make the parser allocate an attacker-chosen amount for a read that had already failed. Return nil, the zero value the other Consume* methods use; every call site tolerates a nil slice. Regression test added.
oioio-space
force-pushed
the
consume-bytes-nil-on-oob
branch
from
September 5, 2026 07:23
f37794d to
736ec51
Compare
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
3 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.
Follow-up to #40 / #41, covering the one item #41 deliberately left out.
ConsumeBytesreturnedmake([]byte, size)on its out-of-bounds path. Sincesizecomes straight from the stream (an argument length, a string length), a malformed record could make the parser allocate an attacker-chosen amount for a read that had already failed. It now returnsnil, consistent with the zero value the otherConsume*methods return when the buffer is exhausted.Every call site was checked for nil-safety: results only flow into
bytes.NewBuffer/bytes.NewReader(thenbinary.Read, which returnsio.EOFand is already handled as an error),UTF16LEToUTF8(which returns early on empty input),range,string(...), or plain assignment. No index expressions on aConsumeBytesresult exist in the module.bounds_test.gogainsTestConsumeBytesOutOfBounds: a size beyond the remaining buffer, and a 1 GiB request against an empty buffer, both assert a nil result (i.e. no allocation of the requested size).go test ./...is unchanged from master apart from the new test (the twodumpevtxexec tests fail identically on master in an environment without that binary).