Skip to content

sdk: TDF reader trusts the unauthenticated manifest spec version to pick integrity digest encoding #4059

Description

@pflynn-virtru

Summary

Reader decides whether to verify integrity digests as hex or as raw bytes by reading the TDF spec version out of the manifest:

isLegacyTDF := r.manifest.TDFVersion == ""

Four call sites: sdk/tdf.go:965 (WriteTo), :1070 (ReadAt), :1478 (assertion hash), :1633 (validateRootSignature).

Nothing authenticates that field. The root signature aggregates only Segment.Hash (sdk/tdf.go:1408-1418), as already noted in the comment at sdk/tdf.go:1076-1078. So the encoding of every integrity check is selected by an unauthenticated, freely-editable manifest value.

This is not currently exploitable — hex is an invertible encoding of the same digest, so flipping the flag makes verification fail, never wrongly succeed. It is a robustness and interop problem, not a vulnerability. But it means archival files are readable or not based on a metadata field rather than on their actual contents.

Precedent

#3597 hit the identical 4.3.0 hex-vs-raw split on the policy binding and resolved it the other way — by making the reader self-describing instead of version-dependent:

// The two are unambiguous by length after base64 decode (32 vs 64 bytes),
// so no version signal is needed.
func decodePolicyBinding(b64Hash string) ([]byte, error)

The same discrimination is available for the integrity digests:

Consumer legacy (hex) current (raw) detectable?
hmacIntegrity (sdk/tdf.go:1561) 64 chars 32 bytes yes
readAEADTag (sdk/tdf.go:1582) 32 chars 16 bytes yes — segmentHashAlg disambiguates it from the HMAC case
assertion hash (sdk/tdf.go:1478) — — no

The assertion path is the exception and needs a different treatment. hashOfAssertionAsHex is always hex there; the flag selects whether the hex bytes or the decoded bytes are fed into completeHashBuilder, so there is no length signal on the wire. That one would have to compute both candidates and accept either, i.e. dual-accept rather than detection.

Proposal

Drop the isLegacyTDF version trust for segment and root integrity in favour of length detection, and convert the assertion path to try-both. The spec version then becomes pure metadata with no bearing on whether a file verifies.

Why now

#4060 makes the Go reader additionally accept tdf_spec_version as a name for that field, because files written with the off-spec name currently fail to decrypt — the reader sees no version, applies hex, and the integrity check fails. That PR is the right narrow fix for the naming error, but it widens the set of manifest keys feeding a security-adjacent branch from one to three. If the reader stopped consulting the version for verification at all, the naming question would be purely cosmetic.

Related: #3597 (open, currently conflicting), #4060.

Activity

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Metadata

Metadata

Assignees

No one assigned

    Labels

    No labels
    No labels

    Type

    No type

    Projects

    No projects

      Milestone

      No milestone

      Relationships

      None yet

      Development

      No branches or pull requests

      Issue actions