Repository navigation
fix(sdk): reject malformed ZIP64 lengths and null assertion statements; refuse isEncrypted=false (DSPX-5102) - #4207
Draft
dmihalcik-virtru wants to merge 4 commits into
Conversation
reject malformed zip64 lengths, null statements and isEncrypted false in go sdk reader
…s; refuse isEncrypted=false (DSPX-5102) IsValidTdf and LoadTDF classify untrusted files, so malformed input must produce an error rather than panic the caller (#4202). - Statement.UnmarshalJSON: treat a JSON null (e.g. a case-variant "Statement": null alongside "statement") as a no-op, and decode into the alias pointer rather than its address so null cannot nil it. LoadTDF previously panicked with a nil dereference on such manifests. - zipstream: ZIP64 sizes/offsets are already bounded by the archive and central directory (DSPX-4590). Add the exact #4202 shape (stored size 0xFFFFFFFFFFFFFFFF on a lone manifest entry) as a regression case, and make ReadAllFileData/readBytes refuse negative lengths or indexes instead of reaching make([]byte, n). - schema: cleartext-flagged TDFs are unsupported. Both the strict and lax manifest schemas now require payload.isEncrypted to be true, so IsValidTdf and LoadTDF with schema validation reject false. Writers already always emit true; a test pins it. - FuzzLoadTDF ignored the KAS allowlist lookup failure path: every input failed there before reaching the manifest parser. Skip the allowlist so the fuzzer and its seeds exercise the parser, and seed it with the three DSPX-5102 shapes. FuzzReader gains the MaxUint64 seed. Fixes #4202 Also resolves DSPX-4687. Signed-off-by: Dave Mihalcik <dmihalcik@virtru.com>
Signed-off-by: Dave Mihalcik <dmihalcik@virtru.com>
|
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 |
Contributor
Benchmark results, click to expandBulk Benchmark Results
TDF3 Benchmark Results
|
Contributor
Remove the 10 MB default manifest size cap from LoadTDF and IsValidTdf. DSPX-4502 targets 50 TiB payloads whose manifests exceed 1 GiB, and the default cap made default-config LoadTDF reject such legitimate TDFs. Memory scaling for very large manifests is tracked in DSPX-4905. The defence against hostile declared ZIP sizes is the archive bounds checking in zipstream.NewReader (#4043) plus the negative-length guards in ReadAllFileData/readBytes, not a size cap: a manifest entry declaring more bytes than the archive holds is still rejected before allocation. WithMaxManifestSize stays as an opt-in limit (it still requires a positive size). In zipstream, a zero or negative manifest limit, the new default, means no limit; ReadAllFileData's contract is unchanged. Behaviour change: callers that relied on the implicit 10 MB cap to bound memory must now pass WithMaxManifestSize explicitly. Signed-off-by: Dave Mihalcik <dmihalcik@virtru.com>
Contributor
Benchmark results, click to expandBulk Benchmark Results
TDF3 Benchmark Results
|
Contributor
|
Contributor
This branch has not been deployed
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-5102 (also resolves DSPX-4687)
Fixes #4202
Proposed Changes
sdk.IsValidTdfandsdk.LoadTDFclassify untrusted files. Malformed input must produce an error, never panic the caller.mainthis is already rejected by the DSPX-4590 bounds work (fix(sdk): bound zip64 reader offsets against the archive (DSPX-4590) #4043). Every ZIP64 size and offset is checked against the central-directory start and the archive length before it is narrowed toint64. The sdk: TDF reader panics and permissive validation on malformed archives (v0.34.0) #4202 archive now fails inNewReaderwithfile data at 45+18446744073709551615 runs past the start of the central directory. This PR adds:0.manifest.jsonentry, 32-bit compressed size0xFFFFFFFF, ZIP64 extra field0xFFFFFFFFFFFFFFFF) as a regression test at three levels: zipstream unit,IsValidTdf, andLoadTDFwith Skip/Lax/Strict. It is also a seed for both fuzz targets.ReadAllFileDataandreadBytesnow returnerrZipFileSizeErrorfor a negative length or index instead of reachingmake([]byte, n). (ReadFileDataalready rejected these.)Statement.UnmarshalJSONnil dereference (sdk: TDF reader panics and permissive validation on malformed archives (v0.34.0) #4202 part 2).encoding/jsonmatches keys case-insensitively, so"Statement": nullsendsnullto the custom decoder. A JSONnullis now a no-op, following thejson.Unmarshalerconvention. The alias struct is also decoded throughauxinstead of&aux, so a null can never set it to nil. There are no other customUnmarshalJSONmethods insdk/,lib/orotdfctl/.payload.isEncrypted: false(sdk: TDF reader panics and permissive validation on malformed archives (v0.34.0) #4202 part 3). We don't support TDFs flagged as cleartext.manifest.schema.jsonandmanifest-lax.schema.jsonnow require"const": trueonisEncrypted. Only that property changed, to avoid conflicts with chore(sdk): give the bundled manifest schemas real $id values #4070.IsValidTdf(which uses Lax), andLoadTDFwithWithSchemaValidation(Lax|Strict), now rejectfalsewithErrInvalidPerSchema.true:CreateTDFand the experimental writer both go throughchunkedWriter.buildManifest. A new test pins this and checks that the written manifest passes both schemas.LoadTDFdoes not rejectisEncrypted:false. The default schema validation level isSkip(the zero value ofSchemaValidationIntensity), and I didn't add a hard reject on that path. The Go reader never reads the flag. It always verifies integrity and decrypts, so afalseflag can't turn a read into a cleartext read. A test pins this behaviour.FuzzLoadTDFwas not reaching the parser. Every input failed early at the KAS allowlist lookup ("no KAS allowlist provided and no KeyAccessServerRegistry available"). The fuzz body now passesWithIgnoreAllowlist(true), and the SDK has a stubconn. It also gets seeds for all three DSPX-5102 shapes. With the oldStatement.UnmarshalJSON, the null-statement seed panics. With the fix, all seeds pass.fix(sdk): make the manifest size limit opt-in). I removed the 10 MB default cap:defaultMaxManifestSizeinsdk/tdf_config.go, and the matchingmanifestMaxSizedefault inzipstream/tdf3_reader.go, whichIsValidTdfused. There is deliberately no default manifest size cap, because of the 50 TiB payload goal (DSPX-4502): manifests for those payloads exceed 1 GiB. Memory scaling for large manifests is tracked in DSPX-4905.WithMaxManifestSizestays as an opt-in limit and still requires a size greater than 0. Its doc comment now says the default is unlimited.TDFReader.Manifestpassesmath.MaxInt64toReadAllFileData.ReadAllFileDataitself is unchanged.WithMaxManifestSize. I didn't mark it!. The change only relaxes a limit, no API is removed or changed, and nothing that loaded before now fails.LoadTDF,IsValidTdfand zipstreamManifest()accept an 11 MB manifestWithMaxManifestSize(0)is still an error1<<40,1<<63,MaxUint64) is still rejected by the bounds checksdk/CHANGELOG.mdentry, which I left as is.spec/DSPX-5102.mdis filled in.Behaviour note / possible follow-up: the null-statement manifest is schema-valid.
IsValidTdfstill returnstruefor it, but it no longer panics anywhere.LoadTDFnow decodes it, and the first read fails the integrity check. Rejecting duplicate or case-variant manifest keys in general would close the parser-differential angle. I left it out because assertion statement values can be arbitrary JSON objects, so a blanket rule needs care. Also out of scope: the xtest container-mutation case (DSPX-4923).Checklist
Testing Instructions
What I ran locally (Go 1.27.2, golangci-lint 2.14.0; CI pins 2.13.2):
make fmt: clean. I re-ran it after the manifest-cap commit, along withsdkgo test -race ./...(all ok),TestREADMECodeBlocks(ok), sdk golangci-lint (0 issues in changed files; same 10 pre-existing ones),go build ./otdfctl/... ./service/...(ok), and fuzzing (FuzzLoadTDF30s,FuzzReader20s, no crashers).cd sdk && go test -race -count=1 ./...: all packages ok.cd sdk && go test -run TestREADMECodeBlocks: ok.assertion.go,TestNullStatementManifestDoesNotPanic,TestStatementUnmarshalJSONNulland theFuzzLoadTDFseed panic. With the old lax schema,TestIsEncryptedFalseRejectedfails.go test -fuzz FuzzLoadTDF -fuzztime 45s ./sdk(about 930k execs) andgo test -fuzz FuzzReader -fuzztime 20s ./sdk/internal/zipstream.make lint: exits non-zero, all on pre-existing issues in untouched files. It reports 3 inlib/ocrypto(gosec), 10 insdk(canonicalheader, staticcheck), 60 inserviceand 1 inotdfctl. Most likely the cause is the local golangci-lint 2.14.0 vs CI's 2.13.2. It reports 0 issues in files changed by this PR; the one it found in the new test was fixed.buf lintpassed.make govulncheck: could not run. The local govulncheck binary was built with go1.26 and can't load go1.27 dependencies.make test: stops at a failure inlib/fixtures(TestTokenManager_*, expects a 120s token buffer and gets 30s). That module is unrelated and doesn't importsdk. I then ran the remaining modules by hand:lib/flattening,lib/identifierandotdfctl: pass.service: everything passes except environment-dependent suites:service/integrationandservice/entityresolution/integration(need Docker/testcontainers),service/rttests(hits a live local platform;already_exists), andservice/pkg/serverTestStartTestSuite("without sdk config" cases).