Repository navigation
Conversation
… Protected, so the tests show the safe way to compare holds() got the expected bytes as a plain Vec from zip_ref and freed it unwiped (ZA-0102); it now moves them into a Zeroizing inside the closure. The decimal encode() helper unwrapped the encoding and freed its Vec unwiped (ZA-0103); it now copies the bytes out through a borrow, so the Vec stays in its Protected and is wiped when it drops.
… nanoseconds in place, so no unwiped return slot holds them integer() called Int::sign_and_word and timestamp() called timestamp_nanos. Neither is inlined across the crate boundary, and each returned the plaintext through a 32-byte slot on the encoder's stack that nothing wiped (ZA-0104, ZA-0105). The encoders now compute these values themselves, so in a release build they stay in registers until they are written to the protected buffer. The encoded bytes do not change; new tests check that both encoders agree with the aead functions they replace.
… value is not left in an unwiped stack slot order() converted the value to an Int at the target's width and passed it to int() by pointer, through a 32-byte slot on its stack that nothing wiped (ZA-0106). int() now takes the target and does the conversion itself, so the converted value stays in a register until it is written to the protected buffer. The encoded bytes do not change.
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
Contributor
🧬 Mutation testing (cargo-mutants,
|
| caught | missed | unviable | timeout |
|---|---|---|---|
| 42 | 0 | 20 | 0 |
✅ Every mutant in the changed lines was caught by a test.
Contributor
✅ No CRAP threshold violations1292 function(s) analyzed · threshold 30 |
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.
This closes the plaintext copies that the zeroize re-audit of #489 found in vitaminc-normalize and vitaminc-ore. It stacks on #489 and should merge after it.
No encoded byte changes and no public API changes. The tests that check literal bytes, and the tests that check against
orderable-bytes, pass without changes.What changed
holdsVecthatzip_refhands it into azeroize::Zeroizing, so the copy is wiped when the closure ends.zeroizeis now a dev-dependency of vitaminc-normalize.decimal::tests::encodeupdate, which borrows the vector. The vector stays in itsProtectedand is wiped when it drops. The returned array of test bytes is plain, and a comment says why that is fine.integerInt::sign_and_word. Only the width is branched on, as before. A new test checks that the result always matchessign_and_word.timestamptimestamp_nanosdoes, rather than by calling it. A new test checks that both accept the same counts.order()/int()convertis folded intoint, which now takes the target and the value, converts, and writes the bytes in one step. NoIntat the target width is built and passed by pointer.The audit said that removing ZA-0104 to ZA-0106 would need unsafe code. It does not. Each slot came from a call that was not inlined and returned or took a 32-byte value through memory. Doing the work in the encoder removes the slot. No unsafe code and no volatile writes were added.
Compiler evidence (AArch64 only)
The builds used the commands from the audit's
rust-compiler-analysis/notes.md:RUSTFLAGS="--emit=llvm-ir,asm" CARGO_INCREMENTAL=0 cargo +nightly rustc -p <crate> --lib --release --all-features -- -C codegen-units=1cargo +nightly rustc -p <crate> --lib --all-features -- --emit=llvm-ir,asm -C opt-level=2 -C codegen-units=1ZA-0104,
integer(release and the audit's build are the same)Before:
After: no
alloca, and the value argument is nowreadonly captures(none). The frame holds only the saved registers:ZA-0105,
timestampBefore, in release and in the audit's build:
After, in release: no
alloca, nosub sp. Only the saved registers are stored (stp x24, x23, [sp, #-64]!...ldp x24, x23, [sp], #64; ret).After, in the audit's build: one 32-byte slot is left. It is the
Option<i128>thatInt::to_i128returns (%self1 = alloca [32 x i8],bl ..Int7to_i128,ldp x21, x22, [sp, #16]). It holds the count before it is turned into nanoseconds. It is there only because that build compiles vitaminc-aead at opt-level 0, soto_i128is not inlined. In release,to_i128is inlined and the slot is gone. The audit dropped the same slot indateanddecimalas a side effect of that build (F-RUST-IR-0003), so it is left as it is here too.ZA-0106,
order()→int()Before (release):
After:
intis inlined intoorder, and%valis gone in release and in the audit's build. The only stack stores left on the integer path are theVecheader of the returned buffer (capacity, pointer, length), not the value. In the audit's build, theto_i128slots (%1,%_10.i) are still there, as they were before (%1,%_7.i). They are the same opt-level 0 side effect as above and are gone in release.ZA-0102 and ZA-0103 (lib tests at opt-level 3)
In
holds, the expectedVecnow gets 8store volatile i8 0before__rust_dealloc. Before, it was freed with no wipe. Inencode, the encoding'sVecis wiped by theProtecteddrop before it is freed. The 14-byte array thatencodereturns is a plain array of test bytes, as intended.Not covered
order(), the date, timestamp and decimal paths still copy the plaintextIntinto a 32-byte slot (%days,%count,%mantissa) to call vitaminc-normalize's publicdate,timestampanddecimal. These encoders take theIntby value, and the Rust calling convention passes a value that size through a copy in the caller's memory. The audit did not report these slots. Removing them would need those encoders to change signature, or be marked#[inline], so they are left for a follow-up. Inside vitaminc-normalize,normalize()no longer makes such copies in release: before this change it had four 32-byte slots (%i,%count,%1,%_4.i), and now it has none.Controlledcombinators in vitaminc-protected hand the closure the bare inner value, will be fixed in a separate, later PR. This PR does not change vitaminc-protected. Theholdsfix uses the same idea that PR is expected to build in, which is to give the closure a wrapper that wipes.