[coderdan]

Closes #523. Stacked on #557.

I ran the Trail of Bits zeroize-audit skill over the storage and transport codecs in vitaminc-aead-frame (storage.rs, serde.rs, transport.rs) and the frame writer they share in vitaminc-aead (frame.rs). It covered:

• source review;
• MIR, and LLVM IR at O0, O1 and O2;
• AArch64 assembly;
• a driver crate that instantiates the generic writers;
• PoCs that use an inspecting allocator.

This PR fixes what the audit confirmed, and records why the rest is acceptable.

What the audit confirmed

Finding	What	PoC	Fix
ZA-0007 (medium)	FrameMap::entry copied every map key into a HashSet<String> that was freed unwiped. This affects plaintext frames from to_frame as well as transport serde.	64 of 64 keys left in freed blocks, both ways, plus 2 on the duplicate-key path	3dd8be9
ZA-0021 (medium)	The transport Serializer used serde's default collect_str, which formats into a String it frees unwiped.	16 of 16 values left (control: 0)	6e01bf5
ZA-0022 (medium)	The same, for KeySerializer (map keys).	8 of 8 keys left (control: 0)	6e01bf5
ZA-0019 (medium)	Transport serde::to_vec handed back a plain Vec<u8>, which the caller frees unwiped.	The frame was freed with both values in it	6e01bf5, breaking

Every wipe survives optimization. There are no OPTIMIZED_AWAY_ZEROIZE findings:

• Both reserve helpers (storage and vitaminc_aead::frame) wipe the old allocation with volatile stores before __rust_dealloc, at O0 through O3, in IR and in assembly.
• The Zeroizing drops in both to_vecs, and the Protected drop, survive the same way.

The other 18 PoCs show their findings are not exploitable. These were weak tool matches:

• rustdoc's blanket From/Into impls;
• derive(Copy) on a fieldless enum and on a borrowed slice;
• "no Drop" on types that own nothing.

The fixes

• FrameMap (vitaminc-aead): it keeps each key's range in the frame instead of a copy, and finds a repeat at end() by sorting those ranges. A repeated key is now refused when the map ends, not at the second entry(). The error is the same DuplicateKey. A new unit test covers a repeat that isn't adjacent, keys that start other keys, and the same key in a nested map.
• collect_str: the transport Serializer and KeySerializer override it and format into a Zeroizing buffer that grows the way frames do. The reserve and Appender helpers moved from storage.rs to serde.rs, which both writers now share.
• serde::to_vec returns Protected<Vec<u8>>, like storage::to_vec and to_frame. The module says this format isn't for plaintext to seal, but it also names "a credential in a configuration" as data a host may send. Only this crate's tests and doc example called it. The transport format promises no stability, so this is allowed in any release, but it is a breaking API change.
• New tests/transport_wipe.rs, built on tests/storage_wipe.rs's inspecting allocator. It checks transport to_vec and to_frame through buffer growth, map keys, Display keys and values, a refused repeat, a failed write and the dropped frame. It fails with either fix reverted: 64 copies left without the FrameMap fix, and 64 without the collect_str override.

Accepted, and recorded

STORAGE.md's Memory and timing and the new Memory section in the transport module's doc record these:

• Stack and register copies are not wiped (ZA-0023 to ZA-0026, ZA-0028, ZA-0029, all low):

  • a char's UTF-8 while it is written;
  • scalar temporaries while encoding and decoding;
  • integer map keys decoded for each comparison in distinct;
  • the shared reader's scalar decoding;
  • the decimal digits serde_core's default visit_i128/visit_u128 format into.

  Rust has no way to reach every copy the compiler makes, and wiping the one copy the source names would claim more than it gives.

• core::fmt's own buffers (ZA-0027): the heap check showed nothing left. The stack check was rejected in review: it ran too late, in a debug build, so it can't show the stack was wiped.

Caveats on the audit itself

• The skill's assembly checker is experimental on AArch64, and needed patching to parse Mach-O labels. The MIR checker had a function-splitting bug, and the IR stack-retention regex no longer matches current LLVM output. The compiler phase therefore read the IR and assembly by hand for every finding above; its notes record this.
• transport.rs has no findings. Ciphertext map keys and passthrough values are in the clear by design.

Testing

• cargo test -p vitaminc-aead -p vitaminc-aead-frame --all-features, and the doctests: pass.
• cargo clippy --no-deps --all-targets --all-features -D warnings and rustdoc with -D warnings: clean.
• cargo mutants --in-diff on this diff: 34 mutants, 23 caught, 11 unviable, 0 missed.

https://claude.ai/code/session_01EnoQSYdggmUYkqNyahDchE

[chatgpt-codex-connector]

Codex Review Summary

This comment shows the latest Codex review activity on this pull request.

Review	Status	Commit	Review trigger
📝 Code Review	✅ Completed 2026-10-26T00:09:14.405118Z	b229baf	PR opened
🔒 Security Review	✅ Completed 2026-10-26T00:10:46.125643Z	b229baf	PR opened

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

• Open a pull request for review
• Mark a draft as ready
• Comment "@codex review" or "@codex security review".

Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings.

[chatgpt-codex-connector]

💡 Codex Review

vitaminc/bindings/go/vcffi/storage.go

Lines 181 to 183 in b229baf

	if writesNull(rv.Elem()) {
	w.buf = append(w.buf, StorageSome)
	}

Preserve presence at every nested optional pointer level

For a ***int whose outer two pointers are non-nil and whose innermost pointer is nil, this condition emits only one StorageSome, producing 56435301f500. UnmarshalStorage consumes that marker for the outer pointer and interprets the null as the middle pointer being absent, so a successful round trip silently changes the value. This is reproducible with var leaf *int; middle := &leaf; original := &middle. Preserve the enclosing presence marker when the pointee itself serializes as a marked optional value, not just when it writes bare null.

---

vitaminc/packages/aead-frame/src/serde.rs

Lines 846 to 848 in b229baf

	if let Some(key) = text {
	self.at = Some(entry_at(self.names, key, index));
	return seed.deserialize(KeyDeserializer(key)).map(Some);

Preserve binary mode when reading storage text keys

For storage maps with text keys, this routes deserialization through KeyDeserializer, which inherits serde's default is_human_readable() == true, even though storage's KeyWriter explicitly returns false. A supported custom key type that writes text in its compact representation and selects its reader using is_human_readable() therefore receives inconsistent modes and can reject or misinterpret its own stored key. Ensure storage text-key deserialization reports binary mode, as the main storage deserializer and key writer do.

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

• Open a pull request for review
• Mark a draft as ready
• Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

[github-actions]

✅ No CRAP threshold violations

1500 function(s) analyzed · threshold 30

[github-actions]

🧬 Mutation testing (cargo-mutants, --in-diff)

caught	missed	unviable	timeout
23	0	11	0

✅ Every mutant in the changed lines was caught by a test.

[coderdan]

Both findings from the Codex review are fixed. Neither is in code this PR changes, so each fix is in the PR that owns the code, and the stack is restacked on top.

Preserve presence at every nested optional pointer level (vcffi/storage.go): fixed in #526 (1d4740a).

• A pointer was marked only when its target wrote as null. A pointer to a marked pointer was left unmarked, so ***int with a nil innermost pointer wrote f5 00, and read back one level shorter.
• It is now marked whenever its target starts with null or F5: a nil pointer or interface, Some, or a pointer to one, checked recursively.
• TestStoragePointerLevelsReadBack round-trips ***int with nil at each depth, pins your example's bytes (f5 f5 00), and checks a pointer to Some. It fails with the old rule.
• The same commit moves marshalText's doc comment back above it; an earlier edit had separated them.

Preserve binary mode when reading storage text keys (aead-frame/src/serde.rs): fixed in #520 (869f271).

• The shared reader now knows the profile it reads for. KeyDeserializer reports is_human_readable() == false in storage, matching KeyWriter, and stays human readable in the transport, matching KeySerializer.
• a_text_key_reads_in_the_mode_it_was_written_in uses a key type that writes a different text in each mode. It round-trips through both profiles, and fails without the fix.

Rebasing #532 needed one change: tests/storage.rs's new transport call unwraps the Protected, folded into the commit that changed to_vec. Tests pass at every branch tip, and all five PRs are mergeable.

[cipherstash-bot]

Recommendation: 🟢 merge as it is

Nothing must change before merging. The comments below are test gaps and one misplaced doc line, and they can wait for a follow-up. The new wipe test covers the heap side well. The moved repeated-key refusal and the collect_str overrides have no test of their own in the transport profile.

How this review was made

Agent	Model	Review type	Result
claude	claude-opus-5-5	test-gap	3 found, 4 posted
claude	claude-opus-5-5	rust	1 found, 1 posted
claude	claude-opus-5-5	golang	0 found, 0 posted
codex	gpt-5.6-terra	test-gap	2 found, 1 posted
codex	gpt-5.6-terra	rust	0 found, 0 posted
codex	gpt-5.6-terra	golang	0 found, 0 posted

Synthesis: claude-sonnet-5-5 merged the findings, removed duplicates and dropped findings it could not confirm in the code. 1 posted finding(s) were raised by two or more models.

Plain language: claude-sonnet-5-5 read every comment as a new reader would. 1 comment(s) had a problem that stopped the reader acting; it rewrote 1.

Stack: position 2 of 3 (:asterisk: #532, #534, #557). *️⃣ marks this pull request.

Context loaded: the description, 1 linked issue(s) and 5 discussion entries.

[auxesis]

Nice one @coderdan!

Approving in advance of the test gaps being closed.