Bugfix: numArguments cap and ConsumeUint8 bounds check - #41
Merged
Conversation
|
|
numArguments in ParseTemplateInstance is read from the stream twice: once on the fast path, once more in the !pres branch taken whenever a short_id has not been seen before. The existing cap only applied to the first read, so it never protected the second - and the !pres branch is the only one reachable from a .evtx file forged from scratch, since a short_id cannot be "known" without first being defined by that same branch. A forged numArguments there drove the arg-header append loop with an attacker-controlled uint32 (up to ~4.29e9), unbounded. ConsumeUint8s bounds check (self.offset > len(self.buff)) is off by one: at offset == len(buff) the check is false and the following self.buff[self.offset] indexes one past the end, panicking. The sibling Consume* methods already use the correct form (e.g. ConsumeUint16: offset+2 > len(buff)). Both are reachable from an untrusted .evtx file - a log forged on a compromised host is a known anti-forensic technique, so a parser that can be made to panic or allocate unboundedly is a denial of service against whoever is investigating it. Adds table-driven regression tests for both: a forged numArguments via the !pres branch, and ConsumeUint8 at/around the exact end of the buffer. Fixes Velocidex#40 Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Function introduced in go 1.21
scudette
force-pushed
the
fix-numargs-consumeuint8
branch
from
September 4, 2026 09:22
c9776d6 to
291cfc3
Compare
oioio-space
pushed a commit
to oioio-space/evtx
that referenced
this pull request
Sep 4, 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
added a commit
to oioio-space/evtx
that referenced
this pull request
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.
scudette
pushed a commit
that referenced
this pull request
Sep 8, 2026
…the requested size (#43) Follow-up to #40 / #41, covering the one item #41 deliberately left out. `ConsumeBytes` returned `make([]byte, size)` on its out-of-bounds path. Since `size` comes 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 returns `nil`, consistent with the zero value the other `Consume*` methods return when the buffer is exhausted. Every call site was checked for nil-safety: results only flow into `bytes.NewBuffer`/`bytes.NewReader` (then `binary.Read`, which returns `io.EOF` and is already handled as an error), `UTF16LEToUTF8` (which returns early on empty input), `range`, `string(...)`, or plain assignment. No index expressions on a `ConsumeBytes` result exist in the module. `bounds_test.go` gains `TestConsumeBytesOutOfBounds`: 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 two `dumpevtx` exec tests fail identically on master in an environment without that binary).
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.
Fixes #40.
Two independent bounds issues in
evtx.go, both reachable from an untrusted.evtxfile (a log forged on a compromised host is a known anti-forensictechnique, so this is a real DoS surface for anyone parsing evidence):
ParseTemplateInstancereadsnumArgumentsfrom the stream twice: once onthe fast path (known template), and again in the
!presbranch, takenwhenever
short_idhas not been seen before. The existing cap(
numArguments > 1024*10) is only applied to the first read. It does notprotect the second — which is the read a file forged from scratch always
reaches, since a
short_idcannot be "known" without first being defined bythat same branch. An attacker-controlled
numArgumentsup to ~4.29e9 thendrives the arg-header append loop unbounded. Moved the cap to apply once,
after both possible reads, right before the value is used as a loop bound.
ConsumeUint8's bounds check,if self.offset > len(self.buff), is off byone: at
offset == len(buff)the check is false, and the followingself.buff[self.offset]indexes one past the end and panics. The siblingConsume*methods already use the correct form (e.g.ConsumeUint16:offset+2 > len(buff)); this bringsConsumeUint8in line.Added
bounds_test.gowith table-driven regression tests for both: a forgednumArgumentsreaching the arg-header loop through the!presbranch (checkedagainst both a near-
uint32max value and a value just over the cap), andConsumeUint8at the exact end of the buffer and just past it. Both tests wereverified to fail against the unpatched code — the first times out, the second
panics with
index out of range [8] with length 8— and to pass against thefix.
Two notes on scope, to keep this focused on the two defects from #40:
ConsumeBytes, whichallocates
make([]byte, size)with an attacker-controlledsizeon theout-of-bounds path instead of returning nil like every other
Consume*method. Happy to send it separately if you'd like it.
go test ./...has two pre-existing failures here (TestEvtx/TestCollector,TestEvtx/TestTemplates); they exec a./dumpevtxbinary that isn't built inmy environment and fail identically on unmodified
master.go vetlikewisereports one pre-existing
unreachable codefinding atevtx.go:1033that ispresent on
master. Neither is touched by this change.