Skip to content

deserialize: validate offsets for structs, slices, lists (zeam #843)#63

Open
ch4r10t33r wants to merge 2 commits into
blockblaz:masterfrom
ch4r10t33r:fix/deserialize-offset-bounds-843
Open

deserialize: validate offsets for structs, slices, lists (zeam #843)#63
ch4r10t33r wants to merge 2 commits into
blockblaz:masterfrom
ch4r10t33r:fix/deserialize-offset-bounds-843

Conversation

@ch4r10t33r
Copy link
Copy Markdown

Problem

Peer-supplied SSZ with invalid offset tables could slice past the buffer and panic during deserialize (struct variable fields, variable []T, variable arrays, and utils.List decode). See zeam#843.

Change

  • Variable arrays: require minimum length, aligned offset prefix, prefix within buffer, start <= end, monotonic offsets.
  • Variable []T: validate first/second offsets, item count, and each subsequent offset read before slicing.
  • Structs: bounds-check every fixed-field read and variable-field slice; enforce start <= end, offsets within buffer, monotonicity, and indices[0] == i for the first variable field.
  • Unions: require serialized.len >= 1 before reading the selector byte.
  • utils.List.sszDecode: fixed-size items reject ragged length and n > N; dynamic path bounds-checks the offset-prefix length before slicing.

Tests / tooling

  • Two regression tests for malformed struct payloads.
  • build.zig.zon: minimum_zig_version = "0.16.0" (matches CI; satisfies anyzig when present).

Downstream

After merge, zeam can switch from a vendored copy back to a build.zig.zon git+ dependency on the new commit.

Malformed peer-supplied SSZ could slice past the buffer and panic in struct second pass, variable []T decode, variable arrays, and List sszDecode.

- Mirror array-deserializer guards: alignment, prefix in bounds, start/end ordering.
- Struct: bounds-check fixed-field reads and variable-field slices; require first var offset to match fixed-prefix length.
- Union: require non-empty payload before reading selector.
- utils.List: fixed-size path rejects ragged length and count > N; dynamic path bounds-checks offset prefix.

Adds regression tests for struct offsets past the buffer.

Closes blockblaz/zeam#843 when consumed downstream.
@ch4r10t33r ch4r10t33r requested a review from gballet as a code owner May 8, 2026 13:00
Copy link
Copy Markdown
Collaborator

@gballet gballet left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Test coverage is too low given the amount of error returns that were added:

┌─────────────────────────────────────────────────────────────────────────────────────┬───────────────────┐
  │                                      Code path                                      │      Tested?      │
  ├─────────────────────────────────────────────────────────────────────────────────────┼───────────────────┤
  │ Struct variable-field start > serialized.len (line 634)                             │ ✅ both new tests │
  ├─────────────────────────────────────────────────────────────────────────────────────┼───────────────────┤
  │ Struct fixed-field overflow (lines 600/608/612)                                     │ ❌                │
  ├─────────────────────────────────────────────────────────────────────────────────────┼───────────────────┤
  │ Struct start > end / start < indices[prev] / first-field start != i (lines 635-637) │ ❌                │
  ├─────────────────────────────────────────────────────────────────────────────────────┼───────────────────┤
  │ Variable array path (lines 461-475)                                                 │ ❌                │
  ├─────────────────────────────────────────────────────────────────────────────────────┼───────────────────┤
  │ Variable []T slice path (lines 524-558) — incl. the new n_items == 0 rejection      │ ❌                │
  ├─────────────────────────────────────────────────────────────────────────────────────┼───────────────────┤
  │ Union 1-byte check (line 645)                                                       │ ❌                │
  ├─────────────────────────────────────────────────────────────────────────────────────┼───────────────────┤
  │ utils.List ragged length + n > N (utils.zig:74-76)                                  │ ❌                │
  └─────────────────────────────────────────────────────────────────────────────────────┴───────────────────┘

Comment thread build.zig.zon Outdated
Comment thread src/lib.zig Outdated
Comment thread src/lib.zig Outdated
Comment thread src/lib.zig Outdated
- Remove minimum_zig_version from build.zig.zon (follow-up PR per maintainer).
- Variable array: use .little; misalignment -> error.OffsetAlignment; drop redundant size vs len check.
- Variable []T: .little; misalignment and n_items==0 -> OffsetAlignment.
- Add regression tests for struct fixed overflow, nested truncation, ordering cases, variable array/slice paths, union empty guard (minInLength 0), List ragged and count>N; fix List test leaks with defer deinit.

Addresses blockblaz#63 (review)
@ch4r10t33r ch4r10t33r requested a review from gballet May 9, 2026 13:14
@ch4r10t33r
Copy link
Copy Markdown
Author

@gballet I have addressed your comments.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants