Skip to content

fix(sdk): harden ZipReader central directory parsing against hostile fields (DSPX-5103) - #418

Draft
dmihalcik-virtru wants to merge 3 commits into
DSPX-4589-02-zip-formatfrom
DSPX-5103-harden-java-zipreader-against-hostile-central-dire
Draft

dmihalcik-virtru wants to merge 3 commits into
DSPX-4589-02-zip-formatfrom
DSPX-5103-harden-java-zipreader-against-hostile-central-dire

Conversation

@dmihalcik-virtru

@dmihalcik-virtru dmihalcik-virtru commented Oct 9, 2026 •

Copy link
Copy Markdown
Member

Jira: https://virtru.atlassian.net/browse/DSPX-5103. This is the Java counterpart of DSPX-5102 / opentdf/platform#4202.

Stacked on #398 (DSPX-4589-02-zip-format). Retarget this PR to main after #398 merges.

Summary

A hostile archive could hang SDK.isTDF / TDF.loadTDF, or make them throw unchecked exceptions that callers screening untrusted input don't catch. #398 already reads the central directory fields unsigned and bounds-checks the EOCD, zip64 locator, CD offset and entry count. This PR closes the remaining gaps:

  1. Extra-field loop (DoS). The loop now walks the extra field record by record, and every step moves at least one 4-byte header forward. A record whose data size runs past the extra field is rejected with InvalidZipException, and so is a zip64 record too short for the values its sentinels promise. On main, a data size of 0xFFFC sign-extends to -4, and the loop spins forever on it. Fewer than 4 trailing bytes are ignored, as Go's archive/zip does.
  2. Negative or out-of-range sizes and offsets. Zip64 sizes and offsets above Long.MAX_VALUE are now rejected. Before, they read as negative, so position(-1) threw IllegalArgumentException or a size of -1 read the manifest as "". Also rejected: an extra field or file comment that runs past the archive, and an entry whose local header offset or compressed size doesn't fit in it. The uncompressed size is still checked lazily (fix(sdk): DSPX-4589 zip64 EOCD sentinels, truncated archive detection, and UTF-8 entry names #398 reports a short entry as truncated).
  3. No manifest size cap, on purpose. There is no fixed cap on manifest size because DSPX-4502 targets 50 TiB payloads, whose manifests exceed 1 GiB. A hostile declared size is handled by the archive bounds check (offset + compressed size <= channel.size()). A manifest whose declared size runs past the archive fails as truncated, so the reader never buffers more than the archive holds. Scaling memory use for huge manifests is DSPX-4584. (An earlier commit in this PR added a 10 MiB cap. A follow-up commit removes it.)
  4. Typed failures.
    • TDF.loadTDF and SDK.readManifest now go through the new TDFReader.open / TDFReader.readManifest. These turn container and manifest malformation into SDK.MalformedTDFException. The underlying causes are InvalidZipException, IllegalArgumentException, Gson's JsonParseException, IllegalStateException and UnsupportedOperationException.
    • SDK.isTDF also returns false for any other RuntimeException, as a commented backstop for its boolean contract.
    • New additive constructor: MalformedTDFException(String, Exception).
  5. Null statement fields. In an assertion statement, a JSON null for format, schema or value now reads as null. Before, format and schema threw UnsupportedOperationException, and value became the string "null". A non-string format or schema is now a JsonParseException.

Behavior change: callers of loadTDF / SDK.readManifest that caught IllegalArgumentException or Gson exceptions for bad input will now get SDK.MalformedTDFException (an SDKException). TDFReader's constructor itself is unchanged.

Still open: trusting payload.isEncrypted = false (DSPX-4748) is out of scope here and has not been fixed.

Tests

  • New HostileZips test helper builds stored archives by hand, with 11 hostile central-directory shapes.
  • Every shape runs under a 5 s assertTimeoutPreemptively against:
    • ZipReader: InvalidZipException
    • SDK.isTDF: false
    • SDK.readManifest: MalformedTDFException
    • TDF.loadTDF: MalformedTDFException
  • Valid-archive controls: the unmodified hand-built archive, and a zip64 extra field placed among other records with trailing padding.
  • loadTDF tests for a manifest whose declared size runs past the archive (MalformedTDFException caused by InvalidZipException) and for invalid manifest JSON (not JSON, [], wrong types, a statement with an object format).
  • A test that reads an 11 MiB manifest whole, so a size limit can't creep back in.
  • ManifestTest tests for null and non-string statement fields.
  • The shapes are added as Jazzer seeds in FuzzingInputs/fuzzZipRead and FuzzingInputs/fuzzTDF, with dspx5103- names so they don't collide with the seeds chore(ci): fuzz Jazzer targets on relevant merges, tighten fuzzZipRead, add zip seeds #416 added on main.
  • I checked that the new tests fail against fix(sdk): DSPX-4589 zip64 EOCD sentinels, truncated archive detection, and UTF-8 entry names #398's ZipReader: on the 0xFFFC shape isTDF returns true, and the zip64 size of -1 gets through.

Checks run (locally, JDK 21, compiling to release 11)

  • mvn -pl sdk -am test -Dmaven.antrun.skip=true: 322 tests, 0 failures, 0 errors, 8 skipped (re-run after the cap was removed; the skips were already there: FIPS, E2E, etc.). Proto codegen was skipped with -Dmaven.antrun.skip=true, both times. The first time, the buf BSR was rate-limiting me (resource_exhausted). For the re-run I skipped it again without retrying buf. The generated sources from an earlier successful build were used.
  • mvn -pl sdk -am test -Dtest=Fuzzing -Dmaven.antrun.skip=true (Jazzer regression mode, seeds only): 34 inputs, 0 failures, new seeds included. This ran before the cap was removed; none of the seeds depend on the cap.
  • Not run: the full mvn clean verify -P coverage, actual fuzzing (-P fuzz), the cmdline/examples modules, and xtest.

harden java ZipReader against hostile central directory fields

Signed-off-by: Dave Mihalcik <dmihalcik@virtru.com>
…fields (DSPX-5103)

A malformed archive could hang SDK.isTDF / loadTDF or make them throw
unchecked exceptions that callers screening untrusted input don't catch.

- Walk the central directory extra field record by record: every step
  advances, a record may not claim more bytes than the extra field has
  left, and zip64 values must fit inside their record.
- Reject zip64 sizes/offsets above Long.MAX_VALUE, extra fields and
  comments that run past the archive, and entries whose local header
  offset or compressed size do not fit in it.
- Cap the manifest at 10 MiB (Go default, web limit) before reading it.
- loadTDF and SDK.readManifest report malformed containers and manifests
  as SDK.MalformedTDFException; isTDF returns false for any parse
  failure.
- Read a JSON null in an assertion statement's format/schema/value as
  null instead of throwing or producing the string "null".

Adds regression tests with preemptive timeouts and Jazzer seeds for each
hostile shape.

Signed-off-by: Dave Mihalcik <dmihalcik@virtru.com>
@coderabbitai

coderabbitai Bot commented Oct 9, 2026

Copy link
Copy Markdown
Contributor

Important

Draft PR not reviewed

Draft PRs are not automatically reviewed by default.

  • Trigger a manual review

To automatically review draft PRs, update your CodeRabbit configuration:

reviews:
  auto_review:
    drafts: true
  • Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

@dmihalcik-virtru
dmihalcik-virtru force-pushed the DSPX-5103-harden-java-zipreader-against-hostile-central-dire branch from 15ca4a9 to 163cd42 Compare October 9, 2026 18:43
DSPX-4502 targets 50 TiB payloads whose manifests exceed 1 GiB, so any
fixed manifest size limit breaks legitimate TDFs. A hostile declared
size is already contained by the archive bounds checks: an entry whose
stored bytes do not fit in the archive is rejected, and a manifest that
claims more than the archive holds fails as truncated, so the reader
never buffers more than the archive contains. Memory scaling for huge
manifests is DSPX-4584.

Replace the oversized-manifest test with one for a manifest running
past the archive and one reading an 11 MiB manifest.

Signed-off-by: Dave Mihalcik <dmihalcik@virtru.com>
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.

1 participant