Skip to content

Validate deserialized fields in MergingDigest.fromBytes() - #228

Open
August829 wants to merge 2 commits into
tdunning:mainfrom
August829:fix/validate-fromBytes-deserialization
Open

Validate deserialized fields in MergingDigest.fromBytes()#228
August829 wants to merge 2 commits into
tdunning:mainfrom
August829:fix/validate-fromBytes-deserialization

Conversation

@August829

Copy link
Copy Markdown

Summary

Fixes #227: MergingDigest.fromBytes(ByteBuffer) reads compression, n, bufferSize, and lastUsedCell from untrusted input without any validation, allowing crafted byte sequences to crash the JVM thread with NegativeArraySizeException or ArrayIndexOutOfBoundsException.

This PR adds comprehensive input validation to both VERBOSE_ENCODING and SMALL_ENCODING paths:

  • compression must be in (0, 1e9] to prevent integer overflow in array sizing
  • n / bufferSize / lastUsedCell must be non-negative and cross-validated against each other
  • centroid count must not exceed the buffer's remaining bytes (16 bytes per centroid for VERBOSE, 8 for SMALL)
  • centroid count must not exceed the digest's internal array capacity
  • All weights must be positive
  • Centroid means must be in non-decreasing order (as the maintainer requested)
  • Total weight must be >= centroid count (sanity check, as requested)

All invalid inputs now throw IllegalArgumentException with descriptive messages instead of crashing with raw RuntimeException subclasses.

Test plan

  • 9 new @Test(expected = IllegalArgumentException.class) tests covering:
    • SMALL_ENCODING with negative n
    • VERBOSE_ENCODING with n exceeding array capacity
    • VERBOSE_ENCODING with overflow-inducing compression (1e10)
    • VERBOSE_ENCODING with negative compression
    • VERBOSE_ENCODING with negative weight
    • VERBOSE_ENCODING with decreasing centroid means
    • SMALL_ENCODING with lastUsedCell > n
    • SMALL_ENCODING with negative weight
    • SMALL_ENCODING with decreasing centroid means
  • 2 round-trip tests (VERBOSE and SMALL) confirming valid digests still serialize/deserialize correctly
  • Run full test suite (CI) to verify no regressions

🤖 Generated with Claude Code

Yu Bao and others added 2 commits August 14, 2026 10:34
Fixes tdunning#227: MergingDigest.fromBytes() reads compression, n, bufferSize,
and lastUsedCell from untrusted input without validation, allowing crafted
byte sequences to trigger NegativeArraySizeException or
ArrayIndexOutOfBoundsException (denial of service).

This commit adds comprehensive validation:
- compression must be in (0, 1e9] to prevent integer overflow
- n/bufferSize/lastUsedCell must be non-negative and cross-validated
- centroid count must not exceed buffer remaining bytes
- centroid count must not exceed digest array capacity
- all weights must be positive
- centroid means must be in non-decreasing order
- total weight must be >= centroid count

All invalid inputs now throw IllegalArgumentException with descriptive
messages. Includes 11 new test cases covering all three original crash
triggers plus semantic validation and round-trip correctness.

🤖 Generated with [Claude Code](https://claude.com/claude-code)

Co-Authored-By: Claude <noreply@anthropic.com>
Two bugs in the initial validation:

1. NaN/Infinity bypass: IEEE 754 comparisons like `weight <= 0` and
   `mean < prevMean` silently pass NaN (all comparisons with NaN return
   false). Fix: use negated form `!(w > 0)` which correctly catches NaN,
   plus explicit `Double.isInfinite()` / `Double.isNaN()` checks. Extract
   into `checkWeight()` and `checkMean()` helper methods with Javadoc
   explaining the IEEE 754 rationale.

2. OOM from MAX_COMPRESSION = 1e9: compression of 1e9 causes ~2 billion
   element arrays (~16GB), trivially triggering OutOfMemoryError from a
   32-byte input. Lower MAX_COMPRESSION to 1e6 (~2M array entries, ~16MB),
   which is generous for any real use case.

Add 7 new test cases: NaN mean, NaN weight, Infinity weight, Infinity
mean, NaN compression (verbose encoding), NaN weight and NaN mean (small
encoding).

🤖 Generated with [Claude Code](https://claude.com/claude-code)

Co-Authored-By: Claude <noreply@anthropic.com>
@August829

Copy link
Copy Markdown
Author

Hi @tdunning ,

Thanks for the reply. I’ve submitted a pull request. One more suggestion: could you set up a SECURITY.md to allow more private vulnerability submissions?

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.

Unvalidated Length Fields Enable Crash in MergingDigest.fromBytes()

1 participant