Phase 2: Security Hardening

[tazarov]

Summary

Phase 2: Security Hardening
Goal: The deserialization path is safe to expose to untrusted inputs
Status: Verified (5/5 must-haves)

Hardens all deserialization paths in Decode() against crafted/corrupt .gin files. Adds bounds checks to all 10 stream-controlled allocation sites, introduces ErrVersionMismatch and ErrInvalidFormat sentinel errors, rejects unknown binary format versions, and removes the legacy fallback branch that silently re-attempted zstd decompression.

Changes

Plan 02-01: Deserialization Security Hardening

Bounds checks on all 10 stream-controlled allocation sites, version validation with sentinel errors, legacy fallback removed.

Key files:

• Created: serialize_security_test.go — 14 new security-focused tests
• Modified: serialize.go — 7 bounds constants, 2 sentinel errors, version validation, bounds checks in all read functions, legacy fallback removal

Task commits (TDD):

1. 8a1c0a9 test: failing tests for version validation and legacy rejection
2. 0221d5f fix: sentinel errors, version check, legacy removal
3. 43fa9aa test: failing tests for bounds checks on all allocation sites
4. 5d93c6e fix: constants and guards on all sites

Requirements Addressed

• SEC-01: Deserialization bounds checks added for all unbounded allocation sites
• SEC-02: Binary format version validation added to Decode() — reject unknown versions
• SEC-03: Maximum size limits enforced during deserialization to prevent memory exhaustion

Verification

• Decode() rejects binary data with Version != 3
• Decode() rejects data without recognized magic bytes (no legacy fallback)
• Decode() rejects crafted payloads with oversized allocations at all stream-controlled sites
• Existing round-trip serialization tests pass with no regressions
• ErrVersionMismatch and ErrInvalidFormat are detectable via errors.Is()

Key Decisions

• Absolute constant (16MB) for readRGSet bounds instead of parameter threading — avoids signature changes to 3+ callers
• Header-derived bounds for readDocIDMapping, readNumericIndexes, readStringLengthIndexes — structural anchors exist
• All bounds violations wrap ErrInvalidFormat with actual/max values for debugging

[tazarov]

@claude PTAL

[claude]

Claude finished @tazarov's task in 1m 57s —— View job

---

Code Review: Phase 2 Security Hardening

Tasks:

• Gather context
• Read changed files (serialize.go, serialize_security_test.go)
• Review serialize.go changes
• Review serialize_security_test.go
• Post final review

---

Overall the PR is solid and covers the stated requirements well. The TDD approach is evident and the bounds constants are clearly justified. A few issues worth addressing before merging:

---

Issues

1. Inner magic bytes check doesn't wrap ErrInvalidFormat (serialize.go:303)

// readHeader
if string(idx.Header.Magic[:]) != MagicBytes {
    return errors.New("invalid magic bytes")  // ← bare error, not ErrInvalidFormat
}

The outer magic check (in Decode) correctly wraps ErrInvalidFormat, but the inner magic check (the "GIN\x01" field inside the decompressed stream) uses a bare errors.New. This means corrupted files that pass the outer magic check ("GINu" / "GINz") but fail the inner magic check cannot be detected with errors.Is(err, ErrInvalidFormat). This is inconsistent with the PR goal and the ErrInvalidFormat contract.

Fix:

return errors.Wrapf(ErrInvalidFormat, "invalid inner magic bytes: %q", string(idx.Header.Magic[:]))

Fix this →

2. "data too short" at the top of Decode also doesn't wrap ErrInvalidFormat (serialize.go:194)

if len(data) < 4 {
    return nil, errors.New("data too short")  // ← bare error
}

Minor: callers checking for any format-level error with errors.Is(err, ErrInvalidFormat) won't catch this. Same fix pattern as above.

---

Observations (non-blocking)

3. readRGSet — numRGs uint32 is unbounded

var numRGs uint32
// ... no bounds check on numRGs
return RGSetFromRoaring(bitmap, int(numRGs)), nil

numRGs is passed as int(numRGs) to RGSetFromRoaring. A crafted 4GB value won't allocate memory here (roaring bitmap is the actual data owner), but it could cause surprising IsSet results downstream. Whether this needs a bound depends on whether RGSetFromRoaring is purely metadata — if so, it's fine and this is just a note.

4. Near-duplicate tests

TestDecodeBoundsPathDirectory (line 127) and TestDecodeCraftedPayload (line 303) are essentially identical: both write 0xFFFFFFFF into data[24:28] and assert ErrInvalidFormat. One of them could be removed or made to test a distinct property (e.g., a different oversized field) to add coverage.

5. Exported sentinels lack GoDoc

var (
    ErrVersionMismatch = errors.New("version mismatch")
    ErrInvalidFormat   = errors.New("invalid format")
)

These are public API — they should have GoDoc comments (e.g., // ErrVersionMismatch is returned by Decode when the binary format version does not match the expected version.).

---

What's Well Done

• Coverage: All 10 stream-controlled allocation sites are addressed, and the constants have clear justifications in their comments.
• Sentinel design: Using errors.Wrapf(ErrInvalidFormat, ...) throughout (not fmt.Errorf) is the right call for errors.Is compatibility.
• Header-derived bounds for readDocIDMapping, readNumericIndexes, and readStringLengthIndexes is the correct approach — structural anchors are more trustworthy than a second magic constant.
• Legacy fallback removal is clean and intentional. The previous silent retry was a latent security hole.
• TestSentinelErrors explicitly validates errors.Is works through pkg/errors wrapping — good defensive test.
• TDD commit sequence is clean and easy to follow.

---

The two ErrInvalidFormat wrapping gaps (issues 1 and 2) are the main items worth fixing. Everything else is non-blocking polish.

[tazarov]

@claude

[claude]

Claude finished @tazarov's task in 1m 32s —— View job

---

Review Follow-up Complete

Tasks:

• Gather context
• Read serialize.go — issues 1 and 2 from the previous review were already fixed in the current branch (both errors.Wrapf(ErrInvalidFormat, ...) wraps are in place)
• Add GoDoc comments to exported sentinel errors
• Commit and push (256c3c1)

---

What was found

The two blocking issues from the previous review were already addressed in the current commits:

• serialize.go:197 — "data too short" already uses errors.Wrap(ErrInvalidFormat, "data too short")
• serialize.go:306 — inner magic check already uses errors.Wrapf(ErrInvalidFormat, "invalid inner magic bytes: %q", ...)

The only remaining polish item was the missing GoDoc on the exported sentinels, which I've now added:

// ErrVersionMismatch is returned by Decode when the binary format version
// does not match the expected version (Version constant).
ErrVersionMismatch = errors.New("version mismatch")

// ErrInvalidFormat is returned by Decode when the binary data is structurally
// invalid: unrecognized magic bytes, oversized allocations, or corrupt fields.
ErrInvalidFormat = errors.New("invalid format")

The two non-blocking observations from the previous review (near-duplicate tests, numRGs unbounded note) don't require changes — they were noted as informational only.

---

| Branch: phase-2/security-hardening

[tazarov]

@claude

[claude]

Claude finished @tazarov's task in 2m 6s —— View job

---

Code Review: Phase 2 Security Hardening (Final State)

Tasks:

• Gather context
• Read serialize.go changes
• Read serialize_security_test.go
• Post final review

---

The PR is in excellent shape. All previous blocking issues have been resolved, and the implementation is clean and correct. Here's what I verified:

All Previous Issues Addressed

• ✅ serialize.go:306 — inner magic check wraps ErrInvalidFormat correctly: errors.Wrapf(ErrInvalidFormat, "invalid inner magic bytes: %q", ...)
• ✅ serialize.go:197 — "data too short" wraps ErrInvalidFormat: errors.Wrap(ErrInvalidFormat, "data too short")
• ✅ serialize.go:34–37 — GoDoc on both sentinel errors added

Allocation Site Review

All 10 stream-controlled allocation sites are correctly bounded:

Site	Field	Bound	Method
readRGSet	numRGs	idx.Header.NumRowGroups	Header-derived
readRGSet	dataLen	maxRGSetSize (16MB)	Absolute constant
readPathDirectory	NumPaths	maxNumPaths (65535)	Absolute constant
readPathDirectory	pathLen	uint16 type (64KB)	Type-bounded — no additional check needed
readStringIndexes	numTerms	maxTermsPerPath (1M)	Absolute constant
readStringIndexes	termLen	uint16 type (64KB)	Type-bounded — no additional check needed
readStringLengthIndexes	numRGs	Header.NumRowGroups	Header-derived
readNumericIndexes	numRGs	Header.NumRowGroups	Header-derived
readTrigramIndexes	numTrigrams	maxTrigramsPerPath (10M)	Absolute constant
readBloomFilter	numWords	maxBloomWords (1M words = 8MB)	Absolute constant
readHyperLogLogs	numRegisters	maxHLLRegisters (65536)	Absolute constant
readDocIDMapping	numDocs	Header.NumDocs	Header-derived

The choice of uint16 for termLen and pathLen is a natural type bound that eliminates the need for an explicit check — a good encoding-level defense.

Test Coverage

The near-duplicate test concern from the previous review was resolved cleanly: TestDecodeCraftedPayload is now TestDecodeCraftedInnerMagic and tests a distinct scenario (corrupting bytes [4:8] = inner magic) vs. TestDecodeBoundsPathDirectory (corrupting bytes [24:28] = NumPaths). Each test now exercises a unique code path.

One Minor Observation (non-blocking)

In readBloomFilter, numBits (uint32) is read from the stream but not cross-validated against numWords. A crafted file could set numBits = 0xFFFFFFFF with numWords = 0, creating a BloomFilter with inconsistent metadata. However, since numBits is only used for NumBits() (informational) and no memory is allocated from it, this is not exploitable for OOM — the actual memory is bounded by numWords. Noting only for completeness.

Summary

This PR is merge-ready. The security hardening is thorough, the sentinel error contract is consistent throughout all code paths, the bounds constants are clearly justified, and the test suite directly validates each constraint. The TDD approach and clean commit sequence make the changes easy to audit.

---

| Branch: phase-2/security-hardening