fix(protected): zeroize controlled types on drop (#181)#185
Open
coderdan wants to merge 4 commits into
Open
Conversation
coderdan
commented
Jun 6, 2026
Contributor
Author
Follow-up: swept the PR for the "clone/copy a secret instead of moving it" patternRemoving 1.
|
Protected/Equatable/Exportable derived Zeroize and asserted the ZeroizeOnDrop marker but had no Drop impl, so the inner secret was never wiped when the wrapper went out of scope. - Add ZeroizeOnDrop (real Drop) to all three types; this requires a T: Zeroize bound on each struct (a conditional Drop must match the struct bounds, E0367). - Remove Copy from Protected/Exportable — Copy and Drop are mutually exclusive, and a bitwise copy would leave un-zeroized duplicates of the secret. Callers that need a duplicate now use explicit Clone. - Add into_inner_unchecked (ptr::read + mem::forget) so risky_unwrap / flatten / transpose still move the secret out to the caller without the new Drop wiping the moved-out value. - Make Controlled: Zeroize a supertrait — a true invariant that collapses most of the bound cascade (every T: not found Controlled not found impl gets Zeroize for free); Usage gains a delegating Zeroize impl to satisfy it. - Fix flatten_array (silently relied on Copy) and a doctest likewise. Verified with /zeroize-audit: trait-aware source pass is clean, and an -O2 IR probe shows Protected<[u8;32]>/Protected<Vec<u8>> drop glue emits volatile zero-stores + seq_cst fences (not DSE-removable). Regression tests drop_zeroizes_inner and risky_unwrap_does_not_zeroize added. Refs #181
Downstream cascade from the Protected/Equatable/Exportable zeroize-on-drop fix: - random: the Generatable impls for Protected/Equatable/Exportable now carry the T: Zeroize bound the wrapper types require. - permutation: PermutationKey wraps Exportable<Protected<[u8; N]>>, a secret that now zeroizes on drop, so it can no longer derive Copy (Copy/Drop are exclusive, and a bitwise copy would leave un-zeroized key duplicates). Call sites that consumed a borrowed key (complement, the invert test) now clone explicitly. Refs #181
invert() took self by value but only ever borrowed it internally (depermute_array takes &PermutationKey, building a fresh key). Under the old Copy impl this was invisible; removing Copy for the zeroizing Drop surfaced it, and complement() had reached for .clone() to get an owned value to consume — re-introducing exactly the secret duplication this PR removes Copy to prevent. Change invert() to &self (it never needed ownership) and drop the clone in complement() and the inversion test. The .0.map() already threads the inner array through by move, so no copy of the key is ever made.
flatten_array built its output with `[Default::default(); N]` + `*x.risky_ref()`, copying each inner secret out of a borrowed element — requiring `T: Copy + Default` and duplicating the secret per element. Use `[_; N]::map` to move each `Protected<T>` through `risky_unwrap`, so the inner value is moved (never copied) into the result. Drops the `Copy`/`Default` bounds (resolving the existing TODO) and removes the transient secret duplication, consistent with this PR removing `Copy` from the controlled types.
coderdan
force-pushed
the
fix/protected-zeroize-on-drop
branch
from
b4dba74 to
36cc1d3
Compare
Contributor
There was a problem hiding this comment.
Pull request overview
This PR fixes a security/correctness gap in vitaminc_protected where Protected, Exportable, and Equatable claimed ZeroizeOnDrop behavior without actually zeroizing on drop, and updates downstream code to satisfy the new T: Zeroize bounds and non-Copy semantics.
Changes:
- Implement real zeroize-on-drop behavior by deriving
ZeroizeOnDropand pushingT: Zeroizebounds onto the controlled wrapper types. - Add move-out helpers (
into_inner_unchecked) and refactorrisky_unwrap/flatten/transposeto preserve “move the secret out” semantics under the newDrop. - Propagate required bounds and
Copyremovals through downstream crates (random,permutation) and add aZeroizeimpl forUsage.
Reviewed changes
Copilot reviewed 9 out of 9 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| packages/random/src/generatable.rs | Adds T: Zeroize bounds for Generatable impls over controlled wrappers. |
| packages/protected/src/zeroed.rs | Requires Zeroize for Zeroed impls of controlled wrappers to match new invariants. |
| packages/protected/src/usage/mod.rs | Adds Zeroize for Usage and tightens Acceptable bound for Protected. |
| packages/protected/src/protected/mod.rs | Derives ZeroizeOnDrop, adds into_inner_unchecked, updates helpers, and adds regression tests. |
| packages/protected/src/lib.rs | Tightens ReplaceT and sealing bounds to align with T: Zeroize requirements. |
| packages/protected/src/exportable/mod.rs | Derives ZeroizeOnDrop, adds into_inner_unchecked, removes Copy semantics, updates unwrap path. |
| packages/protected/src/equatable/mod.rs | Derives ZeroizeOnDrop, adds into_inner_unchecked, updates unwrap path and bounds. |
| packages/protected/src/controlled.rs | Makes Controlled: Zeroize and updates iterator constraints/construction. |
| packages/permutation/src/key.rs | Removes Copy from PermutationKey and adjusts APIs to borrow instead of consume where possible. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+25
to
+32
| fn into_inner_unchecked(self) -> T { | ||
| // SAFETY: `self.0` is read exactly once, then `self` is forgotten so its | ||
| // `Drop` never runs against the now-moved-out field (no double-free, no | ||
| // use-after-zeroize). | ||
| let inner = unsafe { core::ptr::read(&self.0) }; | ||
| core::mem::forget(self); | ||
| inner | ||
| } |
Comment on lines
+71
to
+77
| fn into_inner_unchecked(self) -> T { | ||
| // SAFETY: `self.0` is read once, then `self` is forgotten so its `Drop` | ||
| // does not also wipe the moved-out value. | ||
| let inner = unsafe { core::ptr::read(&self.0) }; | ||
| core::mem::forget(self); | ||
| inner | ||
| } |
Comment on lines
+113
to
+119
| fn into_inner_unchecked(self) -> T { | ||
| // SAFETY: `self.0` is read once, then `self` is forgotten so its `Drop` | ||
| // does not also wipe the moved-out value. | ||
| let inner = unsafe { core::ptr::read(&self.0) }; | ||
| core::mem::forget(self); | ||
| inner | ||
| } |
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.
Closes #181.
Problem
vitaminc_protected::{Protected, Equatable, Exportable}derivedZeroizeand (forProtected) asserted theZeroizeOnDropmarker, but had noDropimpl — the marker is a contract claim, not enforcement. Dropping aProtected<Vec<u8>>freed the heap buffer without wiping it;Protected<[u8; N]>was never touched. The chain-of-custody threaded through aead/encrypt was correct in shape but delivered no actual leaf wipe.Fix
ZeroizeOnDropon all three types (#[derive(…, ZeroizeOnDrop)]), which requires aT: Zeroizebound on each struct (a conditionalDropmust match the struct bounds — E0367).into_inner_unchecked(ptr::read+mem::forget) sorisky_unwrap/flatten/transposestill move the secret out to the caller without the newDropwiping the value the caller now owns.Controlled: ControlledPrivate + Zeroizesupertrait — a true invariant ("every controlled type wraps zeroizable material") that collapses most of the bound cascade; everywhere T: Controlledimpl getsZeroizefor free.Usagegains a delegatingZeroizeimpl to satisfy it.random'sGeneratableimpls carry the bound;permutation::PermutationKeylosesCopyand its borrow-then-consume call sites clone explicitly.Copyis removed fromProtected,Exportable, andPermutationKey.CopyandDropare mutually exclusive — and a bitwise copy of a secret would leave un-zeroized duplicates, defeating the fix. Code that copied these by value must now.clone()explicitly (a deliberate duplication of a secret).Equatablenever implementedCopy, so it is unaffected.Validation (
/zeroize-audit)ZeroizeOnDrop.drop_zeroizes_innertest observes (via a flag-settingZeroizeinner) thatDropruns the wipe;risky_unwrap_does_not_zeroizeconfirms the move-out path hands back a live value.encrypt's optimized IR now contains acore::ptr::drop_in_placemonomorphization plus the key/decrypt paths with 168store volatile i8 0+ 168seq_cstfences — the zeroize wipes fire and are not DSE-removable. Before this change there were zero.Test status
Whole workspace compiles;
clippy --all-targets -D warningsclean;cargo fmt --checkclean. All tests pass except the pre-existing localstack-dependentvitaminc-kms::test_finalize(connection refused to127.0.0.1:4566), unrelated to this change.Follow-up
The 3
mem::forgethits the audit's grep-based scanner flags ininto_inner_uncheckedare false positives — the pairedptr::readtransfers ownership to the caller; forgetting the husk prevents a double-wipe (pinned byrisky_unwrap_does_not_zeroize).