Repository navigation
Fix/parse array length check - #2
Merged
Merged
Conversation
The generated Parse methods read a length field from the wire and then call make with that length as the slice size. With u32-sized fields, an attacker who controls the bytes can declare a length up to 4 GiB while the actual input is far smaller; the make runs to completion before the byte-by-byte copy loop discovers the truncation, so an anonymous correspondent can coerce a recipient into a large allocation per message. On a 32-bit build, the resulting int(uint32) sign-flip causes runtime.makeslice to panic outright. Emit a length check before make in the IDRef case of parseArray. The comparison is performed as uint64 so a u32 length above MaxInt32 does not silently sign-extend through int and then trip the panic. The katzenpost pigeonhole protocol relies on this generator; the bug allowed a single Sphinx packet aimed at a courier kaetzchen to provoke multi-gigabyte allocations in the service-node binary. The committed test fixtures under gen/tests/ are stale relative to a prior encoder-feature commit and would need a separate regeneration pass; that is out of scope for this change. The per-fixture handwritten tests under gen/tests/*/*_test.go continue to pass. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
The marshalBinary, encodeBinary, and validate methods emitted by the generator reference context-scoped fields directly in their bodies (e.g. flag.Flagval) but the methods themselves take no parameters. For a struct declared 'with context X' the result does not compile; TestFilesBuild has been failing on testdata/trunnel/contexts.trunnel and testdata/tor/link_handshake.trunnel since the encoder feature landed. Until the encoder gains context-parameter support to match the Parse side, suppress emission for context-using structs. The Parse path already threads contexts through its signature and is unaffected. Structs without contexts continue to receive a full encoder. TestFilesBuild now passes for every fixture. The per-fixture handwritten tests under gen/tests/contexts continue to pass. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
inspect.NewBranches returns one Branch per union case, including a default branch covering the values not matched by any explicit case. When that default branch (or any case) ends in a Fail directive, the corpus generator was still picking a tag value from the branch's interval set and committing it to the surrounding struct, then relying on members() to produce zero body vectors. For a struct like socks5_client_request, where the atype union has a 'default: fail' clause, the corpus generator emitted entries with an atype byte drawn from the unmatched interval; the parser then rejected those bytes with 'disallowed case' and the corpus tests failed. test_socks5 has been carrying such an entry in its committed corpus since before the struct-ref guard landed. Skip any branch whose case contains a Fail member before sampling a tag value. Non-fail branches continue to produce corpus as before; fixtures without 'default: fail' are unaffected. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
GenerateFiles called g.structure(s) for each struct and propagated the first ErrNotImplemented straight out, with the result that a single unsupported struct anywhere in the file caused gen.Package to write no test file at all. The struct-ref guard added in a816242 ("Disable corpus generation for unions with struct references") intended to disable corpus generation for those structs only, but in practice it disabled corpus generation for the whole package any time even one such struct was present (socks5.trunnel for example). Catch ErrNotImplemented at the per-struct boundary and continue; other errors still propagate. The caller treats a missing Suite as zero test vectors for that type, which is exactly what we want. TestLeftover previously relied on ErrNotImplemented propagating to the caller; reshape it to assert the empty corpus instead. TestFiles likewise loses the unconditional num>0 assertion and now logs and skips when the generator produced no vectors for a struct. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
a816242 ("Disable corpus generation for unions with struct references") detected struct-ref union cases and bailed out of the entire union with ErrNotImplemented, on the assumption that any struct-ref case poisoned the whole union. That is broader than the underlying limitation: the corpus generator handles sibling branches without struct refs perfectly well; the historical corpus for gen/tests/unionbasic.basic already proved this by carrying entries for T_INTEGER, T_INTARRAY, and T_STRING while omitting the T_DATE struct-ref branch. Move the struct-ref check into the per-branch loop alongside the fail-branch skip. The non-struct-ref branches now produce corpus; the struct-ref branch is silently skipped. Coverage for unionbasic and socks5_client_request improves accordingly. Structs whose every branch contains a struct ref still produce no corpus, exactly as they did under the old guard. The Package layer's per-struct ErrNotImplemented swallow added in d68ea0f remains the safety net for other unimplemented features (such as Leftover). Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Picks up five generator improvements that had not been propagated: - gen: bound IDRef-length array allocation by remaining buffer (fea6885). Adds the 'data too short' check ahead of each variable-length make. - tv: skip union 'fail' branches when generating corpus (e28cf18...). Drops the invalid 0xb0 atype entry that Socks5ClientRequestCorpus and Socks5ServerReplyCorpus carried. - tv: skip individual unsupported structs instead of poisoning the file (d68ea0f...). Allows the rest of socks5.trunnel to generate corpus. - tv: refine struct-ref guard to per-branch granularity (5a59c01...). Restores TestSocks5ClientRequestCorpus and TestSocks5ServerReplyCorpus with valid IPv4 and IPv6 cases (the DOMAINNAME case is silently skipped per-branch). - gen: union-encoder nil guard from 88e7bea ("Fix union encodeBinary nil pointer bug"). All twelve corpus tests in test_socks5/ now pass. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
TestGeneratedFiles in gen/ compares generator output against the
fixtures committed under gen/tests/. The committed goldens had
fallen behind several earlier generator additions and never been
brought current; this regeneration is the catch-up pass.
The diff bundles five categories of update:
1. The binary encoder, MarshalBinary, and validate methods added
in f9a6973 / f05ee80 and refined in 88e7bea, present in the
generator output but absent from the goldens.
2. The union-encoder nil guard introduced in 88e7bea.
3. The IDRef-length bounds check introduced in this branch
(fea6885).
4. The context-encoder skip introduced in this branch
(794da36...). For gen/tests/contexts, the context-using
structs now emit only Parse; the no-context Point still
gets a full encoder.
5. The branch-level struct-ref granularity introduced in this
branch (5a59c01...). gen/tests/unionbasic now produces both
TestBasicCorpus (with three non-struct-ref entries) and
FuzzBasic, restoring coverage that was implicit before. Two
new packages (leftover and unionlo) gain empty
gen-marshallers_test.go and gen-fuzz.go shells because their
specs no longer trip the all-or-nothing ErrNotImplemented bail
in tv.GenerateFiles; the generated files declare the package
and nothing more, which is correct.
A small handful of corpus-byte sequences shift (e.g.
unioncmds.gen-marshallers_test.go) because the new fail-branch and
struct-ref skips consume one fewer RNG sample per such branch,
which cascades through subsequent random draws. The resulting
corpus continues to parse and was verified by go test ./gen/...
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
… entry The IDRef-length bounds check landed in fea6885 keeps a single make() proportional to the remaining buffer, but it does nothing to bound the buffer itself: an upstream that hands the parser a generous N MiB of bytes whose internal length fields are all internally consistent will allocate N MiB. For katzenpost in particular, the wire layer cap is 500 MB because that is sized for the PKI document path; trunnel messages are individually much smaller than that and have no business consuming hundreds of megabytes. Emit a package-level var MaxParseSize (default 16 MiB) and check it once at the top of every Parse... convenience constructor. Callers holding fully untrusted bytes naturally use the convenience entry point; the struct's own Parse(data, ...) method stays uncapped so that recursive struct refs and in-stream parses do not re-validate on every step. Downstream packages may tighten the cap by assigning to MaxParseSize before the first parse call. TestFilesBuild previously called Marshallers once per file in a group and built the resulting source files together, which worked only because the generator never emitted package-level state. With MaxParseSize now declared at package scope, two such files collide. The CLI never invokes the generator that way: parse.Files reads the whole group into one []*ast.File and Marshallers is called once. Rework the test to mirror that real usage. Also add a focused test under gen/tests/vararray asserting the cap rejects oversized input and that the struct method is exempt as documented. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Mechanical propagation of 2ee086b... All generated marshaller files in the fixture tree (and test_socks5) gain a package-level MaxParseSize variable and a check at the top of each Parse... convenience constructor. No protocol or semantic change beyond the new cap; the per-fixture handwritten tests under gen/tests/*/*_test.go and test_socks5/socks5_test.go continue to pass. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
The bound emitted by fea6885 checks uint64(n) > uint64(len(cur)). For an array of u8 elements that is exact: make([]uint8, n) consumes n bytes and the iteration needs n bytes from cur. For a u16/u32/u64 array the allocation is n*elemBytes bytes; the old check still bounds the make() by O(len(cur)), but loosely (up to 8x for a u64 array). Tightening the bound to uint64(n) > uint64(len(cur))/elemBytes makes the make() strictly bounded by len(cur) itself, regardless of element width. elementByteSize(base) reports the per-element width from the AST. IntType returns Size/8 (1, 2, 4, or 8). CharType and any other member type fall back to 1, which is the conservative lower bound: struct members can be larger than one byte, but we cannot know without static analysis, and a 1-byte assumption never under-bounds the check. The katzenpost pigeonhole schema uses only u8 variable arrays, so this change is a no-op for that consumer; the only fixture diff is gen/tests/vararray, whose u32 Words array now bounds against len(cur)/4. A focused test asserts the tightened bound rejects a 13-byte buffer for a declared 3-word array (which needs 14 bytes) and accepts the exact 14-byte case. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
The repository has been a fork for some time, and the inherited Travis configuration is no longer in service. The new workflow runs on push to main or master and on pull requests, exercising go build, go vet, and the full test suite with the race detector across Go 1.23.x and 1.24.x on ubuntu-latest, with the module cache restored between runs. The README sheds the stale Travis build badge and the Coveralls badge (the latter was populated only by the Travis job and would otherwise freeze in place), and gains a badge pointing at the new workflow. The go.dev and Go Report Card badges remain undisturbed. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.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
1 participant
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.
No description provided.