field/extensions: bound the vector length read by AsyncReadFrom - #878
Open
tzh476 wants to merge 3 commits into
Open
field/extensions: bound the vector length read by AsyncReadFrom#878tzh476 wants to merge 3 commits into
tzh476 wants to merge 3 commits into
Conversation
AsyncReadFrom takes the vector length from the first four bytes of the reader and
uses it to allocate immediately:
sliceLen := binary.BigEndian.Uint32(bufSizeSlice[:])
n := int64(4)
(*vector) = make(Vector, sliceLen)
Nothing stands between the two. The field is a uint32 and each element occupies
4*fr.Bytes, so four bytes of input can ask for tens of gigabytes before any
element data has arrived. io.ReadFull only fails afterwards.
When the reader can report how much data is left, refuse a length that cannot
possibly be backed by it. Measured on an Apple M3 Pro, a 4 byte input claiming
2^20 elements:
before 16,777,426 B/op 6 allocs/op 506,264 ns/op
after 210 B/op 5 allocs/op 8,153 ns/op
A valid vector is unaffected: 360 B/op before and after.
The change is made in the generator template, so it is not lost on the next
regeneration; the two generated files are the result of `go generate ./internal/...`,
which reported 1233 files generated and left only these two changed.
Change-Id: I3436d9a4b46d0457985903456c041f43edecb28b
Signed-off-by: tzh476 <tzh476@gmail.com>
The change had no test. This adds one that fails without it: on master the
announced count sizes the allocation and the test reports
reading a vector that announces 2^20 elements from a 4 byte input
allocated 16777432 bytes
The assertion is on allocation volume rather than on getting an error, because a
truncated stream errors either way once io.ReadFull runs out of input. A well
formed vector is still asserted to round trip.
The test is added in the generator template alongside the existing
AsyncReadFrom coverage, so it is regenerated with the rest; the two generated
files are the output of `go generate ./internal/...`.
Change-Id: Ibf33e5692afdad719fd77e07b0a5cb79684d6113
Signed-off-by: tzh476 <tzh476@gmail.com>
The previous push carried the koalabear test but not the koalabear fix, so that half of the change would have had a test with nothing to guard it. Both generated files now match the template. Change-Id: I48f8130623ae5b5e761b27988307834ede6b6d89 Signed-off-by: tzh476 <tzh476@gmail.com>
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.
Description
Vector.AsyncReadFromtakes the vector length from the first four bytes of the reader and allocates on it immediately:Nothing stands between the decode and the allocation. The field is a
uint32and each element occupies4*fr.Bytes, so four bytes of input can ask for tens of gigabytes before any element data has arrived —io.ReadFullon the element bytes only fails afterwards, once the memory is already committed.This refuses a length that the reader demonstrably cannot back, when the reader can report its remaining size (
bytes.Reader,bytes.Buffer,strings.Readerall can). A reader that cannot is left alone, so no caller loses functionality.Measurement
Apple M3 Pro, a 4 byte input claiming 2^20 elements:
A valid vector is unaffected: 360 B/op before and after.
I used 2^20 for the benchmark to keep it cheap; the ceiling is what the field allows,
math.MaxUint32 * 4 * fr.Bytes.The change is in the template
field/*/extensions/vector.gois generated and markedDO NOT EDIT, so patching it alone would be reverted by the next regeneration. The fix is ininternal/generator/field/template/extensions/vector.go.tmpl; the two generated files here are the output ofgo generate ./internal/..., which reported 1233 files generated and left exactly these two changed.Testing
go test ./field/koalabear/... ./field/babybear/...— 13 packages, all pass, before and after regeneration.I did not add a test because the behaviour is only observable as an allocation size, which is awkward to assert portably. Happy to add one along the lines of "a 4 byte input claiming 2^20 elements returns
io.ErrUnexpectedEOF" if you would like it — that part is deterministic.Disclosure
The site was found by a static checker I wrote that looks for allocations sized by a length field decoded from untrusted bytes with no bounds check in between, and this change was prepared with AI assistance. The before/after numbers are from benchmark runs on this diff, and I will answer review questions myself.
Note
Medium Risk
Untrusted binary deserialization is security-sensitive; the guard only applies when the reader implements Len(), so generic io.Reader callers may still allocate on bogus lengths until read fails.
Overview
Vector.AsyncReadFromno longer allocates from the untrusteduint32length alone. When the reader exposes remaining size viaLen() int(e.g.bytes.Reader), it rejects lengths that cannot fit in the bytes still available—returningio.ErrUnexpectedEOFbeforemake(Vector, sliceLen)—which blocks huge allocations from a tiny malicious header.The change is applied in
internal/generator/field/template/extensions/vector.go.tmplande4_test.go.tmpl, with regenerated babybear and koalabear outputs.TestVectorAsyncReadFromLengthBoundasserts an error and low allocation for a 4-byte header claiming 2²⁰ elements, and confirms well-formed vectors still round-trip through async unmarshal.Reviewed by Cursor Bugbot for commit 1f97732. Bugbot is set up for automated code reviews on this repo. Configure here.