feat(png): carry the C2PA manifest store in the caBX chunk - #511
Open
justin13888 wants to merge 12 commits into
Open
justin13888 wants to merge 12 commits into
justin13888 wants to merge 12 commits into
Conversation
Read and write the C2PA manifest store (C2PA 2.4 §A.3.2) as one more raw, MetadataBlock-ready ancillary payload, the way eXIf/iCCP/XMP already travel. Decode: `DecodedPng::c2pa` / `PngMetadata::c2pa` carry the first `caBX` verbatim and uncompressed; a later `caBX` is counted in `c2pa_duplicates` (saturating), never concatenated, since PNG has no multi-chunk store. The store is charged to the cumulative `with_max_metadata_bytes` budget like every other attacker-sized payload — skipped past the remainder, not an error. Encode: `with_c2pa(store)` embeds a caller-computed store and `with_c2pa_reserved(len)` writes `len` zero bytes in its place, as the last chunk before the first IDAT (after PLTE/tRNS and every other ancillary chunk) so a reservation is filled by a second equal-length encode that changes no byte outside the chunk. `encode_with_report` and `PngReport::c2pa` name the chunk's whole span — length, type, payload and CRC — as `C2paSpan`, the `c2pa.hash.data` exclusion §18.5.4 asks for. `EncodeImage` is untouched. The chunk type is spelled once, `chunk::CABX`, with its property bits asserted per PNG §5.4 Table 6: ancillary and private set, reserved clear, and unsafe-to-copy *clear* on the fourth byte — the polarity the issue's prose had backwards. That bit is the container's own enforcement of the facade's no-copy-forward law (`C2paPolicy`). libpng carries `caBX` as an unknown chunk, which is the framing proof: for the same payload it frames the same length/type/CRC bytes as gamut, decodes gamut's file pixel-exact with the chunk in place, and gamut reads the store back from libpng's file. The behavioural oracle (c2pa-rs) is #447. Refs #440
… into feat/440-png-cabx
… into feat/440-png-cabx
`find_c2pa` walked to end of input, unlike every other chunk walk in the crate (`parse_stream`, `walk_metadata_chunks` and `deconstruct` all stop at IEND). Latent today, since its only caller is the encoder reading back its own fresh output, but it made the "the encoder's report and the file's report cannot disagree" claim false for any later caller: a `caBX` appended after IEND — bytes §13.2 calls a trailer, outside the datastream — would have been reported as the file's manifest store. The walk now stops at the first IDAT or at IEND, whichever comes first, which states the store rule in one place: the first CRC-valid `caBX` before the first IDAT. Stopping at IDAT is C2PA 2.4 §A.3.2's placement — the store precedes IDAT and data after it is bad-form — and it is what keeps a chunk appended to a finished file from being taken as a store the file does not carry.
`fill_c2pa(&mut png, &span, store)` writes a finished manifest store into the `caBX` chunk a span names, rewriting the payload and the chunk CRC and nothing else. It is the second half of the reserve-then-fill flow C2PA 2.4 §18.5 describes, and the shape that flow actually needs: a signer hashes the reserved file with the chunk's span excluded, then fills it. Until now the only way to fill a reservation was to encode again with `with_c2pa`. That reaches the same bytes but costs a second full encode — at `Level::Best` with `FilterStrategy::BruteForce`, the whole brute-force set again — and it makes the signature depend on the encoder reproducing its output byte for byte. Filling in place is O(store) and depends on nothing but the chunk's own bytes, and it is the only route at all for a file gamut did not write. Every argument is validated before the first byte is written, with a distinct typed error each: the span must lie inside the image, frame a chunk (payload exactly `chunk.start + 8 .. chunk.end - 4`), name a `caBX`, and receive a store of exactly the reserved length. A store of the wrong length is rejected rather than resized: resizing would move every byte after the chunk and invalidate the hash the signer signed.
C2PA 2.4 §A.3.2 places the manifest store before IDAT and calls data after it bad-form, but the decode surfaced the first CRC-valid `caBX` wherever it sat. That let anyone append a `caBX` to a finished PNG and have it read back as that file's provenance — including into a file that carries no store at all, where the appended chunk became the only answer. The encoder never writes there, so nothing gamut produces was affected; the exposure was on read. The store is now the first CRC-valid `caBX` before the first IDAT, in all three readers that answer the question — `decode`, both `metadata` entry points, and `PngReport::c2pa` — with the addition done in one place so they cannot drift apart. An ignored chunk stays visible rather than being silently dropped: the counter now covers both reasons a `caBX` is not the store, a later one and one after IDAT, so `c2pa == None` with a non-zero count is exactly the shape of an appended store. It is renamed `c2pa_ignored` to say that, since "duplicates" is false when the file carries no original, and it becomes a `usize`: as a saturating `u8` a file with 300 ignored chunks reported 255, a number the file does not contain.
`walk_metadata_chunks` grew a second result — the count of `caBX` chunks found after IDAT — and the bare tuple tripped `clippy::type_complexity`. The alias carries the explanation the tuple could not: which chunks come back, and why the post-IDAT ones are a number rather than chunks.
The span docs claimed the report names the chunk the decode surfaces as its payload. That is false under a budget: `PngReport::c2pa` has no byte budget, so a store past `with_max_metadata_bytes` is still spanned while `decode().c2pa` is `None` — a caller gating on the report could get a `None` it had been told could not happen. The same block claimed the ignored count agrees with `chunk(b"caBX")`, which it deliberately does not, since that counts CRC-invalid and post-IDAT chunks too. Both now say what is true: the report answers *where the bytes are* and has no budget, the decode answers *what was admitted*, and each count answers its own question. Exclude the span from a hash; read the payload from the decode. Also states two things the code already did but the docs did not. The reserve-then-fill flow's step 3 is `fill_c2pa`, with re-encoding named as the costlier alternative rather than the route. And "last chunk before IDAT" is this writer's guarantee about files it produces, not a property of the format: PNG §14.3.2 says ordering relative to other *ancillary* chunks is never assumable and an editor may insert one after ours, so readers assume only "before IDAT" — which is exactly what they do.
The mutation gate caught the gap: `span.chunk.end > png.len()` could be weakened to `>=` and every test still passed, because each fixture put an IDAT and an IEND after the store, so the chunk never ended at the buffer's end. `fill_c2pa` takes a `&mut [u8]`, so the exact fit is a legitimate call — the prefix of a file up to the end of its store, or a file whose store is its last chunk — and under `>=` every such call is refused.
`fill_c2pa` validated the span against itself and against the buffer's bounds, then read exactly four bytes of `png` — the chunk type — and wrote. It never compared the span with the length the chunk itself declares, so a span naming a longer chunk than the one actually there passed every guard: the payload and the recomputed CRC went over whatever followed, in practice the IDAT, and the call returned `Ok(())`. The mirror case, a span shorter than the chunk declares, planted the CRC inside the real payload, leaving a chunk `find_c2pa` then skips — the store silently not carried. Reaching it needs only a mismatched span, which the API allows by construction: `C2paSpan`'s fields are `pub`, `#[non_exhaustive]` blocks literal construction but not field assignment, and both hand-out points return owned values. Taking a span from one file and filling another is supported on purpose — the exact-fit case does it — which is precisely why the declared length has to be checked rather than assumed. No panic and no memory-safety issue: every write stayed inside the slice. But the documented contract says a span whose bytes are not a `caBX` chunk is rejected, and these bytes are not one.
`c2pa_ignored`'s docs claimed that `c2pa == None` with a non-zero count meant someone had appended a store to a file carrying none. The unit test forty lines below asserted the counter-example: two `caBX` chunks both *before* IDAT, the first over the metadata budget, gives exactly that pair with nothing appended. The converse failed too — the canonical append is a `caBX` after IEND, which is a trailer neither walk reaches, so it counted zero. A caller gating injection detection on the field got both a false positive and a false negative on the one case the sentence named. The count now covers every CRC-valid `caBX` in the datastream that was not surfaced as the store, which adds the budget-skipped store-position chunk `collect` already saw and previously passed over. First-wins is unchanged: the first chunk still claims the store position whether or not it is admitted, so an oversized store cannot be substituted by a smaller one after it. The docs — the two struct fields, STATUS.md and the README — now state the three cases it counts, that it does not distinguish them, and that a chunk after IEND is outside the datastream and therefore outside the count, with `deconstruct`'s trailer segment named as where that shape is visible instead. Both claims are pinned by tests.
The declared-length check read the length field through four indexed reads, `start`, `start + 1`, `start + 2`, `start + 3`. The mutation gate found the first of those offsets unkillable: for any payload under 64 KiB the top two length bytes are both zero, so reading one in place of the other changes nothing a fixture of that size can observe. The offsets are gone rather than papered over with a 64 KiB fixture. The eight header bytes are taken as one borrow and split where §5.3 splits them, and the length field is compared as bytes against the span's own payload length in network order — so there is no offset arithmetic left to get wrong, and a wrong split fails the type check that follows. The test gains the case the byte comparison earns: a 300-byte store, where a span agreeing only in the low byte (44) is rejected, which no single-byte length could have shown.
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
1 participant
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.
Stacked on #485 (
feat/224-png-encoder-efficiency): this PR is opened against that branch and must merge after it. Nothing #485 did is modified; this adds thecaBXslot to the ancillary pass #485 already plumbs. The base has moved twice since this branch opened (fac39dc→c59df6f→cce9939) and is merged in here by merge commit both times (never rebase); every contact file auto-merged with no conflicts.This is an unattended run: no human approved the plan. The decision record below is what a human reads afterwards.
Summary
gamut-pngnow reads and writes the C2PA manifest store in thecaBXchunk (C2PA 2.4 §A.3.2) as one more raw,MetadataBlock-ready ancillary payload — the same mechanismeXIf/iCCP/XMP already use, one chunk type added to both sides. Issue #440, part of epic #239.decode, bothmetadataentry points,PngReport::c2pa, and the encoder's own read-back): the first CRC-validcaBXbefore the firstIDAT. First, because a file carries exactly one store; CRC-valid, because §13.1 makes a mismatch skippable and the decode skips it; beforeIDAT, because §A.3.2 puts it there and calls data after it bad-form — so acaBXappended to a finished file is never read as that file's provenance.DecodedPng::c2pa/PngMetadata::c2pacarry that chunk verbatim and uncompressed.c2pa_ignored(usize) counts every CRC-validcaBXin the datastream that was not surfaced as the store — a later one, one afterIDAT, or the store-position chunk itself when it busted the metadata budget — and deliberately does not say which. AcaBXafterIENDis a trailer, outside the datastream and outside the count;deconstruct's trailer segment is where that shape is visible. The store is charged to the cumulativewith_max_metadata_bytesbudget like every other attacker-sized payload.with_c2pa(store)embeds a caller-computed store;with_c2pa_reserved(len)writeslenzero bytes in its place, as the last chunk before the firstIDAT, so the store's offset depends only on what precedes it.encode_with_reportandPngReport::c2pa()name the chunk's whole span — length, type, payload and CRC — asC2paSpan, which is thec2pa.hash.dataexclusion §18.5.4 asks for;fill_c2pa(&mut png, &span, store)then writes the finished store into that span in place, rewriting only the payload and the chunk CRC. That is O(store) instead of the O(encode) of a secondwith_c2papass, and it does not tie the signature to the encoder reproducing its output byte for byte. Every argument is validated before the first byte is written — bounds, framing, chunk type, the chunk's own declared length, and the store's length — so a span that does not describe the bytes it points at is rejected rather than written through, and a rejected call leaves the file untouched.pngneed not be the buffer the span came from, which is why the declared length is checked rather than assumed.chunk::CABXis the one place the type is spelled; a test asserts its property bits per PNG §5.4 Table 6 — ancillary and private set, reserved clear, and unsafe-to-copy clear on byte 3 (Table 6's polarity; issue gamut-png: carry the C2PA manifest store in the caBX chunk #440's prose had it backwards, its bytes did not, and the reviewer independently confirmed the reading).caBXas an unknown chunk: for the same payload it frames the same length/type/CRC bytes as gamut, decodes gamut's file pixel-exact with the chunk in place, and gamut reads the store back from libpng's file.c2pa-rs(behavioural) is tooling/c2pa-oracle: differential oracle against c2pa-rs, both directions #447.Semver: minor (
#[non_exhaustive]structs gain fields; newpubitemsC2paSpan,PngEncodeReport,fill_c2pa, three encoder methods, one report method).Validation
Run in the lane worktree (branch
feat/440-png-cabx), all at the current heade1dddb7— after both base merges and both rounds of review repairs.mise run fmt/fmt-checkneed the nested-worktree prefix__CARGO_TEST_ROOT=<worktree root>(a known environment artefact: cargo otherwise walks past the worktree root to the primary checkout'sCargo.tomlwhen loading thetooling/*manifests).CARGO_BUILD_JOBS=2 cargo test -p gamut-png --all-features__CARGO_TEST_ROOT=… mise run fmtthenmise run fmt-checkmise run check-testsconvco check origin/feat/224-png-encoder-efficiency..HEADCARGO_BUILD_JOBS=2 cargo clippy -p gamut-png --all-targets --all-features -- -D warningsmise run lint(whole workspace, capped scope)mise run test(whole workspace, capped scope)test result: oklines,tests/c2pa.rsamong them)GAMUT_MUTANTS_BASE=origin/feat/224-png-encoder-efficiency mise run mutants --diff --crate gamut-png --budget 16Two survivors were found and closed during the run, each verified dead before the clean re-run above.
60aec64closedreplace > with >=infill_c2pa's bounds check, which no fixture reached because every one of them put anIDATand anIENDafter the store, so the chunk never ended at the buffer's end.e1dddb7closedreplace + with *in the declared-length read: the length field was read through four indexed offsets, and for any payload under 64 KiB the top two bytes are both zero, so reading one in place of the other is unobservable. Rather than add a 64 KiB fixture, the offsets were removed — the eight header bytes are taken as one borrow, split where §5.3 splits them, and the length compared as bytes — and the test gained a 300-byte store, where a span agreeing only in the low byte is rejected.No
Cargo.tomltouched, socheck-release-deps/check-ffi-featuresare not triggered; no C-surface type changed, socheck-ffi-headeris not either.CI check state at
60aec64: Format & Metadata, Clippy & Doctests and Incremental (PR diff) 0–3 all pass;Coverage (test gate)was still running when this body was last updated — it legitimately takes hours on this workspace — andFull workspaceis skipped by design for a PR.For the record, since it is visible in this PR's history: the runs on the two earlier heads (
7f60deband its re-runs) were red for a repository-wide infrastructure outage tracked as #517, not for anything in this diff. Every job died in its first step, Install system build deps —sudo apt-get updateexit 100,E: Failed to fetch https://dl.google.com/linux/chrome-stable/…/Packages.gz Hash Sum mismatchfrom the runner image's preinstalled Google Chrome apt repo — so pkg-config/mise/sccache never installed and later steps failed withsccache: command not found(127). No gamut code was compiled before the failure, and the only required job that installs no system deps (Format & Metadata) passed throughout. That outage cleared upstream on its own, and the checks above are the first ones on this branch to run gamut's code since. Nothing was weakened, skipped or bypassed at any point; the local gate set above is the evidence for this change.Risks and rollout
caBXare byte-identical to before on both sides (the size-contract and libpng oracle suites pass unchanged); a file withcaBXpreviously had it ignored and now has it copied into memory under the metadata budget.IDATrule makes the reader stricter: acaBXafterIDATthat an earlier build of this branch would have surfaced is now counted as ignored instead. That is the intended fix, and no file gamut writes is affected (the encoder never writes there).fill_c2pamutates a caller's buffer in place; it validates span, framing, chunk type, the chunk's declared length and the store's length first, and writes nothing on rejection. The declared-length check is new in round 2 and makes previously-accepted mismatched spans an error — that is the fix, and no span this crate hands out is affected.c2pa_ignorednow also counts a store-positioncaBXskipped for the metadata budget, so a file that reported 1 may now report 2. No file gamut writes is affected; the field is new on this branch and unreleased.21602ed,cb30ea8,0342473,704fa25,a4f9e7f,35957e0,60aec64,eca33fa,c49be4c,e1dddb7); the two merge commits carry no changes of their own.Issue
Closes #440.
Decisions taken
Appended by the lane, in the record's shape:
Unresolved review notes
c2pa_ignoredis the only malformed-file signal the decoder reports; if a decoder-wide anomalies channel is ever added, this field is the first candidate to fold into it, and it would let the two ignore-reasons (a duplicate, versus a chunk appended afterIDAT) be told apart, which one count cannot.fill_c2pavalidates that the span frames acaBXchunk of the declared length but does not re-verify the existing CRC before overwriting, so filling a span in a file whose store was already corrupted silently repairs the CRC. That is the right behaviour for the reserve-then-fill flow and wrong for nothing gamut does today, but it is worth a reviewer's eye.c2pa_ignoredcounts three different malformations without distinguishing them, which the docs now say plainly. Telling them apart needs a decoder-wide anomalies channel (the note above), not a wider integer.