Repository navigation
fix(sdk): harden ZipReader central directory parsing against hostile fields (DSPX-5103) - #418
Draft
dmihalcik-virtru wants to merge 3 commits into
Conversation
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>
Contributor
|
Important Draft PR not reviewedDraft PRs are not automatically reviewed by default.
To automatically review draft PRs, update your CodeRabbit configuration: reviews:
auto_review:
drafts: true
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. Comment |
dmihalcik-virtru
force-pushed
the
DSPX-5103-harden-java-zipreader-against-hostile-central-dire
branch
from
October 9, 2026 18:43
15ca4a9 to
163cd42
Compare
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>
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
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.
Jira: https://virtru.atlassian.net/browse/DSPX-5103. This is the Java counterpart of DSPX-5102 / opentdf/platform#4202.
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:InvalidZipException, and so is a zip64 record too short for the values its sentinels promise. Onmain, a data size of0xFFFCsign-extends to -4, and the loop spins forever on it. Fewer than 4 trailing bytes are ignored, as Go'sarchive/zipdoes.Long.MAX_VALUEare now rejected. Before, they read as negative, soposition(-1)threwIllegalArgumentExceptionor 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).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.)TDF.loadTDFandSDK.readManifestnow go through the newTDFReader.open/TDFReader.readManifest. These turn container and manifest malformation intoSDK.MalformedTDFException. The underlying causes areInvalidZipException,IllegalArgumentException, Gson'sJsonParseException,IllegalStateExceptionandUnsupportedOperationException.SDK.isTDFalso returns false for any otherRuntimeException, as a commented backstop for its boolean contract.MalformedTDFException(String, Exception).nullforformat,schemaorvaluenow reads asnull. Before,formatandschemathrewUnsupportedOperationException, andvaluebecame the string"null". A non-stringformatorschemais now aJsonParseException.Behavior change: callers of
loadTDF/SDK.readManifestthat caughtIllegalArgumentExceptionor Gson exceptions for bad input will now getSDK.MalformedTDFException(anSDKException).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
HostileZipstest helper builds stored archives by hand, with 11 hostile central-directory shapes.assertTimeoutPreemptivelyagainst:ZipReader:InvalidZipExceptionSDK.isTDF:falseSDK.readManifest:MalformedTDFExceptionTDF.loadTDF:MalformedTDFExceptionloadTDFtests for a manifest whose declared size runs past the archive (MalformedTDFExceptioncaused byInvalidZipException) and for invalid manifest JSON (not JSON,[], wrong types, a statement with an objectformat).ManifestTesttests for null and non-string statement fields.FuzzingInputs/fuzzZipReadandFuzzingInputs/fuzzTDF, withdspx5103-names so they don't collide with the seeds chore(ci): fuzz Jazzer targets on relevant merges, tighten fuzzZipRead, add zip seeds #416 added onmain.ZipReader: on the0xFFFCshapeisTDFreturns 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.mvn clean verify -P coverage, actual fuzzing (-P fuzz), the cmdline/examples modules, and xtest.