diff --git a/.github/workflows/test.yml b/.github/workflows/test.yml index 16a672df..2e3700bb 100644 --- a/.github/workflows/test.yml +++ b/.github/workflows/test.yml @@ -229,6 +229,14 @@ jobs: echo "RLIMIT_MEMLOCK soft: $(ulimit -l) KiB" cargo test + - name: test (protected residue scans, optimised) + # What the `Controlled` combinators leave behind on the stack and the + # heap, measured by scanning dead memory. The stack guarantee is + # stated for optimised builds, and the plain `cargo test` above + # skips those cases at opt-level 0, so they run here under + # `--release`. + run: cargo test -p vitaminc-protected --release --test residue_stack --test residue_heap + - name: test (aead optional scalar features) # Only dev-dependencies enable these on vitaminc-aead, so a build # of the crate alone skips the tests that pin the chrono, diff --git a/README.md b/README.md index 3443460d..a63b10be 100644 --- a/README.md +++ b/README.md @@ -60,7 +60,7 @@ to redact its debug output and wipe its contents when it is dropped: use vitaminc::protected::{Controlled, Protected}; let mut token = Protected::new(String::from("example-token")); -token.update(|value| value.push_str("-updated")); +token.risky_update(|value| value.push_str("-updated")); let debug = format!("{token:?}"); assert!(debug.contains("***")); diff --git a/packages/aead/src/ciphertext/mod.rs b/packages/aead/src/ciphertext/mod.rs index 1060781d..cd6a388c 100644 --- a/packages/aead/src/ciphertext/mod.rs +++ b/packages/aead/src/ciphertext/mod.rs @@ -82,10 +82,10 @@ mod tests { let ciphertext = CipherTextBuilder::new() .append_nonce(nonce) .append_target_plaintext(plaintext) - .accepts_ciphertext_and_tag_ok(|mut ciphertext| { + .accepts_ciphertext_and_tag_ok(|ciphertext| { ciphertext.copy_from_slice(&[2u8; 10]); ciphertext.extend([3u8; 16]); - Ok(ciphertext) + Ok(()) }) .build()?; @@ -108,10 +108,10 @@ mod tests { let ciphertext = CipherTextBuilder::new() .append_nonce(nonce) .append_target_plaintext(plaintext) - .accepts_ciphertext_and_tag_ok(|mut ciphertext| { + .accepts_ciphertext_and_tag_ok(|ciphertext| { ciphertext.copy_from_slice(&[2u8; 10]); ciphertext.extend([3u8; 16]); - Ok(ciphertext) + Ok(()) }) .build()?; diff --git a/packages/aead/src/ciphertext/read_monads.rs b/packages/aead/src/ciphertext/read_monads.rs index 098c5ad7..a9820863 100644 --- a/packages/aead/src/ciphertext/read_monads.rs +++ b/packages/aead/src/ciphertext/read_monads.rs @@ -89,12 +89,16 @@ impl CiphertextAndTagReader { self, f: impl FnOnce(&mut [u8]) -> Result, ) -> Plaintext { - Plaintext(self.0.map_ok(|raw| { - let mut raw = zeroize::Zeroizing::new(raw); - let len = f(&mut raw)?; - raw.truncate(len); - Ok(core::mem::take(&mut *raw)) - })) + let mut buf = self.0; + // On an error `buf` is dropped, and so wiped, with the closure. + Plaintext( + buf.risky_try_update(|raw| { + let len = f(raw)?; + raw.truncate(len); + Ok(()) + }) + .map(|()| buf), + ) } } diff --git a/packages/aead/src/ciphertext/write_monads.rs b/packages/aead/src/ciphertext/write_monads.rs index 30df892c..b3e7004c 100644 --- a/packages/aead/src/ciphertext/write_monads.rs +++ b/packages/aead/src/ciphertext/write_monads.rs @@ -64,12 +64,19 @@ impl PlaintextWritten { Self(nonce, plaintext) } - /// Provides a closure that takes the plaintext and to which the ciphertext and tag must be written. + /// Hands the closure the plaintext buffer, which it must overwrite in + /// place with the ciphertext and then extend with the tag. + /// + /// The closure borrows the buffer, so it stays inside the `Protected` + /// throughout. A backend that fails part way through may leave + /// plaintext in it, and the buffer is wiped on that path too. pub fn accepts_ciphertext_and_tag_ok( self, - f: impl FnOnce(Vec) -> Result, E>, + f: impl FnOnce(&mut Vec) -> Result<(), E>, ) -> EncryptedWithTag { - EncryptedWithTag::new(self.0, self.1.map_ok(f)) + let Self(nonce, mut buf) = self; + // On an error `buf` is dropped, and so wiped, with the closure. + EncryptedWithTag::new(nonce, buf.risky_try_update(f).map(|()| buf)) } } diff --git a/packages/aead/src/typed.rs b/packages/aead/src/typed.rs index 6ebef3ab..a708fa59 100644 --- a/packages/aead/src/typed.rs +++ b/packages/aead/src/typed.rs @@ -1260,7 +1260,7 @@ impl Sink for ProtectedOptional { /// Move a protected value into a protected `Some`. fn some(value: Protected) -> Protected> { - value.map(Some) + value.into_some() } /// Any sink, with its result moved into `Protected` when it is done: the diff --git a/packages/aead/tests/ciphertext_builder_wipe.rs b/packages/aead/tests/ciphertext_builder_wipe.rs new file mode 100644 index 00000000..3485a5d3 --- /dev/null +++ b/packages/aead/tests/ciphertext_builder_wipe.rs @@ -0,0 +1,82 @@ +//! A `CipherTextBuilder` whose seal fails must not free the plaintext +//! buffer with the plaintext still in it. +//! +//! The builder hands the sealing closure the plaintext buffer, and a +//! backend that fails may leave plaintext there. This test installs a +//! global allocator that, while armed, counts how many secret words are +//! still in each block as it is freed. It lives in its own test binary so +//! the allocator sees nothing but this seal. + +use std::alloc::{GlobalAlloc, Layout, System}; +use std::sync::atomic::{AtomicBool, AtomicUsize, Ordering}; + +use vitaminc_aead::{CipherTextBuilder, NonceGenerator, RandomNonceGenerator}; +use vitaminc_protected::Protected; + +const SECRET: u64 = 0x5EC2_E7C0_FFEE_D00D; + +static ARMED: AtomicBool = AtomicBool::new(false); +static LEFT_IN_FREED_BLOCKS: AtomicUsize = AtomicUsize::new(0); + +struct Inspecting; + +// SAFETY: every call is forwarded to `System` unchanged. `dealloc` only +// reads the block it is about to free, which the caller still owns. +unsafe impl GlobalAlloc for Inspecting { + unsafe fn alloc(&self, layout: Layout) -> *mut u8 { + unsafe { System.alloc(layout) } + } + + unsafe fn dealloc(&self, ptr: *mut u8, layout: Layout) { + if ARMED.load(Ordering::SeqCst) { + // SAFETY: `ptr` is a live block of `layout.size()` bytes. + let block = unsafe { core::slice::from_raw_parts(ptr, layout.size()) }; + let found = block + .as_chunks::<8>() + .0 + .iter() + .filter(|w| **w == SECRET.to_ne_bytes()) + .count(); + LEFT_IN_FREED_BLOCKS.fetch_add(found, Ordering::SeqCst); + } + unsafe { System.dealloc(ptr, layout) } + } +} + +#[global_allocator] +static ALLOCATOR: Inspecting = Inspecting; + +/// Four secret words, as the bytes of a plaintext buffer. +fn plaintext() -> Vec { + SECRET.to_ne_bytes().repeat(4) +} + +// One test, so no other thread allocates while the allocator is armed. +#[test] +fn a_failed_seal_frees_no_plaintext() { + // The allocator does see the words when a block that holds them is + // freed unwiped. + let bare = plaintext(); + LEFT_IN_FREED_BLOCKS.store(0, Ordering::SeqCst); + ARMED.store(true, Ordering::SeqCst); + drop(bare); + ARMED.store(false, Ordering::SeqCst); + assert_eq!(LEFT_IN_FREED_BLOCKS.load(Ordering::SeqCst), 4); + + // A seal that fails without touching the buffer, as a backend that + // refuses its input would. + let nonce = RandomNonceGenerator::<12>::init() + .and_then(|g| g.generate()) + .expect("nonce"); + let secret = Protected::new(plaintext()); + LEFT_IN_FREED_BLOCKS.store(0, Ordering::SeqCst); + ARMED.store(true, Ordering::SeqCst); + let sealed = CipherTextBuilder::new() + .append_nonce(nonce) + .append_target_plaintext(secret) + .accepts_ciphertext_and_tag_ok(|_| Err("seal failed")) + .build(); + ARMED.store(false, Ordering::SeqCst); + assert!(sealed.is_err(), "the seal should have failed"); + assert_eq!(LEFT_IN_FREED_BLOCKS.load(Ordering::SeqCst), 0); +} diff --git a/packages/async-traits/src/lib.rs b/packages/async-traits/src/lib.rs index dd8075a9..b4644b5d 100644 --- a/packages/async-traits/src/lib.rs +++ b/packages/async-traits/src/lib.rs @@ -30,7 +30,7 @@ use vitaminc_traits::OutputSize; /// impl AsyncFixedOutput<4, Protected<[u8; 4]>> for PreparedOutput { /// type Error = std::convert::Infallible; /// async fn try_finalize_into(self, out: &mut Protected<[u8; 4]>) -> Result<(), Self::Error> { -/// out.update(|bytes| bytes.copy_from_slice(self.0.risky_ref())); +/// out.risky_update(|bytes| bytes.copy_from_slice(self.0.risky_ref())); /// Ok(()) /// } /// } diff --git a/packages/encrypt/src/cipher.rs b/packages/encrypt/src/cipher.rs index dba03674..8b01c787 100644 --- a/packages/encrypt/src/cipher.rs +++ b/packages/encrypt/src/cipher.rs @@ -96,12 +96,7 @@ where CipherTextBuilder::new() .append_nonce(nonce) .append_target_plaintext(data) - .accepts_ciphertext_and_tag_ok(|mut buf| { - cipher - .key - .seal(&nonce_bytes, aad.as_bytes(), &mut buf) - .map(|()| buf) - }) + .accepts_ciphertext_and_tag_ok(|buf| cipher.key.seal(&nonce_bytes, aad.as_bytes(), buf)) .build() } diff --git a/packages/encrypt/src/cipher_static.rs b/packages/encrypt/src/cipher_static.rs index d01cf3d8..1c4869e9 100644 --- a/packages/encrypt/src/cipher_static.rs +++ b/packages/encrypt/src/cipher_static.rs @@ -6,7 +6,6 @@ use crate::Aes256Cipher; use vitaminc_aead::hlist::{Absent, Encrypted, Entry, StaticCipher}; use vitaminc_aead::{tags, IntoAad, Unspecified}; use vitaminc_protected::{Controlled, Protected}; -use zeroize::Zeroizing; /// What an [`Absent`] seals: the null leaf, as a `None` of bytes is /// everywhere else. No separate absence derivation is needed, @@ -22,15 +21,13 @@ const NULL_LEAF: [u8; 1] = [tags::NULL]; /// could open as a present value. fn tagged_bytes(data: Protected>) -> Protected> { data.map(|bytes| { - // `map` hands the closure the plain vector and with it the duty to - // wipe it. The leaf is a fresh allocation one byte longer, so the - // caller's vector is copied rather than grown in place (a growth - // would leave the old allocation behind unwiped); wrapping it wipes - // it when the closure returns. - let bytes = Zeroizing::new(bytes); + // The leaf is a fresh allocation one byte longer, so the caller's + // vector is copied rather than grown in place (a growth would leave + // the old allocation behind unwiped). `map` only lends the vector, + // and wipes it when the closure returns. let mut leaf = Vec::with_capacity(bytes.len() + 1); leaf.push(tags::BYTES); - leaf.extend_from_slice(&bytes); + leaf.extend_from_slice(bytes); leaf }) } @@ -41,15 +38,16 @@ fn untagged_bytes(leaf: Protected>) -> Result>, Unspec if leaf.risky_ref().first() != Some(&tags::BYTES) { return Err(Unspecified); } - Ok(leaf.map(|mut bytes| { + let mut leaf = leaf; + leaf.risky_update(|bytes| { // Rotate the tag to the end and pop it, so the one byte the shorter // vector leaves in its spare capacity is the public tag and not the // last byte of the plaintext. `Protected` wipes the spare capacity // on drop as well; this keeps nothing secret there to begin with. bytes.rotate_left(1); bytes.pop(); - bytes - })) + }); + Ok(leaf) } #[cfg_attr(docsrs, doc(cfg(feature = "hlist")))] diff --git a/packages/kms/src/lib.rs b/packages/kms/src/lib.rs index 85a3cef3..6f716a8a 100644 --- a/packages/kms/src/lib.rs +++ b/packages/kms/src/lib.rs @@ -25,9 +25,8 @@ use zeroize::Zeroize; /// /// SDK errors leave the caller's output untouched. Reset finalization retains /// the buffered input on SDK error or cancellation while awaiting KMS, allowing -/// retry. On success it copies the MAC to the output, then clears the input's -/// length. This clear retains capacity and does not immediately wipe the old -/// message bytes; the protected buffer is wiped when dropped. Consuming +/// retry. On success it copies the MAC to the output, then wipes the input +/// and empties it, keeping its capacity for the next message. Consuming /// finalization drops the instance on success, failure, or cancellation. /// A cancelled request may still complete remotely. /// @@ -103,7 +102,7 @@ where { fn update(&mut self, data: &Protected) { let pref: ProtectedRef = data.as_protected_ref(); - self.input.update_with_ref(pref, |input, data| { + self.input.risky_update_with_ref(pref, |input, data| { input.extend(data.as_ref()); }); } @@ -114,9 +113,9 @@ where T: AsRef<[u8]> + Zeroize, { fn update(&mut self, data: Protected) { - self.input.update_with(data, |input, mut data| { + // `data` is wiped when `risky_update_with` drops it. + self.input.risky_update_with(data, |input, data| { input.extend_from_slice(data.as_ref()); - data.zeroize(); }); } } @@ -124,7 +123,7 @@ where impl Update for AwsKmsHmac { fn update(&mut self, data: Info) { let pref: ProtectedRef<[u8]> = data.0.as_protected_ref(); - self.input.update_with_ref(pref, |input, data| { + self.input.risky_update_with_ref(pref, |input, data| { input.extend(data); }); } @@ -132,7 +131,7 @@ impl Update for AwsKmsHmac { impl<'r, const N: usize> Update> for AwsKmsHmac { fn update(&mut self, pref: ProtectedRef<[u8]>) { - self.input.update_with_ref(pref, |input, data| { + self.input.risky_update_with_ref(pref, |input, data| { input.extend(data); }); } @@ -152,7 +151,7 @@ where let output = self.generate_mac().await?; let response = Protected::new(output.into_inner()); - out.update_with(response, |out, data| { + out.risky_update_with(response, |out, data| { out.copy_from_slice(data.as_ref()); }); @@ -167,19 +166,19 @@ where { type Error = Error; - /// Send the buffered message to KMS, write the MAC, and clear the input length. - /// SDK errors and cancellation during the request retain the input and - /// leave `out` unchanged. Success retains input capacity without immediately - /// wiping the previous message. See [`AwsKmsHmac`] for panic behavior. + /// Send the buffered message to KMS, write the MAC, and wipe and empty + /// the input. SDK errors and cancellation during the request retain the + /// input and leave `out` unchanged. Success keeps the input's capacity for + /// the next message. See [`AwsKmsHmac`] for panic behavior. async fn try_finalize_into_reset(&mut self, out: &mut C) -> Result<(), Self::Error> { let output = self.generate_mac().await?; let response = Protected::new(output.into_inner()); - out.update_with(response, |out, data| { + out.risky_update_with(response, |out, data| { out.copy_from_slice(data.as_ref()); }); - self.input.update(|input| input.clear()); + self.input.risky_update(|input| input.zeroize()); Ok(()) } @@ -224,8 +223,8 @@ mod tests { types::{KeySpec, KeyUsageType}, Client, Config, }; - use vitaminc_async_traits::AsyncFixedOutput; - use vitaminc_protected::{Controlled, Protected}; + use vitaminc_async_traits::{AsyncFixedOutput, AsyncFixedOutputReset}; + use vitaminc_protected::{AsProtectedRef, Controlled, Protected, ProtectedRef}; use vitaminc_traits::Update; fn get_config() -> Config { @@ -290,6 +289,19 @@ mod tests { Ok(()) } + /// A `Protected` passed by value, and a `ProtectedRef<[u8]>`, are + /// appended like the borrowed forms. + #[tokio::test] + async fn test_update_by_value_and_by_protected_ref() { + let mut hmac: AwsKmsHmac<32> = + AwsKmsHmac::new(get_config(), "0cce5331-13a6-437f-a477-1c8988667281"); + hmac.update(Protected::new(vec![0, 1])); + let pref: ProtectedRef<[u8]> = "ab".as_protected_ref(); + hmac.update(pref); + + assert_eq!(hmac.input.risky_unwrap(), vec![0, 1, 97, 98]); + } + #[tokio::test] async fn test_finalize() -> Result<(), Box> { let config = get_config(); @@ -303,4 +315,47 @@ mod tests { Ok(()) } + + /// The MAC lands in `out`: a finalization that returned `Ok` without + /// writing would leave it all zeros. + #[tokio::test] + async fn test_finalize_into_writes_the_mac() -> Result<(), Box> { + let client = Client::from_conf(get_config()); + let key_id = get_key_id(&client, KeySpec::Hmac256).await?; + + let mut out = Protected::new([0u8; 32]); + AwsKmsHmac::<32>::new(get_config(), key_id) + .chain(&Protected::new(vec![1, 2, 3])) + .try_finalize_into(&mut out) + .await?; + + assert_ne!(out.risky_ref(), &[0u8; 32], "the MAC was written to out"); + Ok(()) + } + + /// A reset finalization writes the MAC and leaves the input empty, so + /// the same message fed again gives the same MAC. + #[tokio::test] + async fn test_finalize_into_reset_writes_the_mac_and_empties_the_input( + ) -> Result<(), Box> { + let client = Client::from_conf(get_config()); + let key_id = get_key_id(&client, KeySpec::Hmac256).await?; + + let mut hmac = AwsKmsHmac::<32>::new(get_config(), key_id); + hmac.update(&Protected::new(vec![1, 2, 3])); + let mut first = Protected::new([0u8; 32]); + hmac.try_finalize_into_reset(&mut first).await?; + assert_ne!(first.risky_ref(), &[0u8; 32], "the MAC was written to out"); + assert!(hmac.input.risky_ref().is_empty(), "the input was emptied"); + + hmac.update(&Protected::new(vec![1, 2, 3])); + let mut second = Protected::new([0u8; 32]); + hmac.try_finalize_into_reset(&mut second).await?; + assert_eq!( + first.risky_ref(), + second.risky_ref(), + "nothing of the first message was left in the input" + ); + Ok(()) + } } diff --git a/packages/normalize/src/scalar.rs b/packages/normalize/src/scalar.rs index 7017d908..7e1a52c9 100644 --- a/packages/normalize/src/scalar.rs +++ b/packages/normalize/src/scalar.rs @@ -349,7 +349,7 @@ pub fn uuid(value: &[u8]) -> Option>> { /// sharing this one, so it stays out of this crate's public API. pub(crate) fn fixed(mut write: impl FnMut(&mut [u8; N])) -> Protected> { let mut bytes = Protected::new(vec![0u8; N]); - bytes.update(|bytes| { + bytes.risky_update(|bytes| { let array = <&mut [u8; N]>::try_from(bytes.as_mut_slice()) .expect("the vector was made with exactly N bytes"); write(array); diff --git a/packages/normalize/src/scalar_tests.rs b/packages/normalize/src/scalar_tests.rs index a93967f0..38aa9048 100644 --- a/packages/normalize/src/scalar_tests.rs +++ b/packages/normalize/src/scalar_tests.rs @@ -467,7 +467,7 @@ fn timestamps_one_past_chronos_range_are_invalid() { fn fills_exactly(bytes: &Protected>, len: usize) -> bool { Protected::new(len) .zip_ref(bytes, |len, bytes: &Vec| { - bytes.len() == len && bytes.capacity() == len + bytes.len() == *len && bytes.capacity() == *len }) .risky_unwrap() } diff --git a/packages/ore/src/order.rs b/packages/ore/src/order.rs index 60b8b696..9d8f99b9 100644 --- a/packages/ore/src/order.rs +++ b/packages/ore/src/order.rs @@ -670,7 +670,7 @@ fn make_room( return Err(NormalizeError::Reserved); } if nulls == NullOrder::First { - code.update(|code| increment(code)); + code.risky_update(|code| increment(code)); } return Ok(code); } @@ -821,7 +821,7 @@ fn variable_width(bytes: &[u8]) -> Protected> { /// to wipe those. fn fixed(mut write: impl FnMut(&mut [u8; N])) -> Protected> { let mut bytes = Protected::new(vec![0u8; N]); - bytes.update(|bytes| { + bytes.risky_update(|bytes| { let array = <&mut [u8; N]>::try_from(bytes.as_mut_slice()) .expect("the vector was made with exactly N bytes"); write(array); diff --git a/packages/ore/src/order_tests.rs b/packages/ore/src/order_tests.rs index 81cda754..617a527a 100644 --- a/packages/ore/src/order_tests.rs +++ b/packages/ore/src/order_tests.rs @@ -622,7 +622,7 @@ mod oracle { fn fills_exactly(bytes: &Protected>, len: usize) -> bool { Protected::new(len) .zip_ref(bytes, |len, bytes: &Vec| { - bytes.len() == len && bytes.capacity() == len + bytes.len() == *len && bytes.capacity() == *len }) .risky_unwrap() } diff --git a/packages/permutation/src/bitwise.rs b/packages/permutation/src/bitwise.rs index bd657321..3acae04d 100644 --- a/packages/permutation/src/bitwise.rs +++ b/packages/permutation/src/bitwise.rs @@ -27,7 +27,7 @@ macro_rules! impl_bitwise_permutable { for j in 0..$N { bits[j] = (bytes[j / 8] >> (7 - (j % 8))) & 1; } - let permuted = Zeroizing::new(permute_array(self, *bits)); + let permuted = Zeroizing::new(permute_array(self, &bits)); let mut out = [0u8; $array_size]; for j in 0..$N { out[j / 8] |= (permuted[j] & 1) << (7 - (j % 8)); diff --git a/packages/permutation/src/elementwise.rs b/packages/permutation/src/elementwise.rs index bacdd260..bea634f7 100644 --- a/packages/permutation/src/elementwise.rs +++ b/packages/permutation/src/elementwise.rs @@ -25,7 +25,7 @@ where [T; N]: IsPermutable + Zeroed, { fn permute(&self, input: [T; N]) -> [T; N] { - permute_array(self, input) + permute_array(self, &Zeroizing::new(input)) } } @@ -40,7 +40,7 @@ where } #[inline] -pub fn permute_array(key: &PermutationKey, input: [T; N]) -> [T; N] +pub fn permute_array(key: &PermutationKey, input: &[T; N]) -> [T; N] where [T; N]: IsPermutable + Zeroed, T: Zeroize + Copy + ConditionallySelectable, @@ -55,8 +55,9 @@ where // `ConditionallySelectable` so the access pattern is independent of `kv`, // preventing cache-line timing leaks of the secret key bytes. Secret // locals — including the partially-populated `out` — are wrapped in - // `Zeroizing` so they are wiped on any unwind path. - let input = Zeroizing::new(input); + // `Zeroizing` so they are wiped on any unwind path. The input is + // borrowed, so the caller keeps it where its own wipe reaches it, and no + // by-value copy is made here. let mut out: Zeroizing<[T; N]> = Zeroizing::new(Zeroed::zeroed()); for (i, k) in key.iter().enumerate() { let kv = Zeroizing::new(k.risky_unwrap()); diff --git a/packages/permutation/src/key.rs b/packages/permutation/src/key.rs index f83257a6..4fada61d 100644 --- a/packages/permutation/src/key.rs +++ b/packages/permutation/src/key.rs @@ -267,8 +267,8 @@ impl PermutationKey { where [u8; N]: IsPermutable + Zeroed, { - // `invert` borrows, so we map the inverse of the borrowed `target` - // through `permute_array` without ever copying the key. + // `invert` borrows `target`, and `map` lends the inverse to + // `permute_array`, so no bare copy of either key is made. Self(target.invert().0.map(|arr| permute_array(self, arr))) } diff --git a/packages/protected/README.md b/packages/protected/README.md index b5a12e1a..efb6ed1a 100644 --- a/packages/protected/README.md +++ b/packages/protected/README.md @@ -45,8 +45,10 @@ inner type is: unlike a move, a `Copy` value has no single owner whose drop can wipe it. Duplicating the wrapper is an explicit `clone()`, and each clone is wiped when it drops. See the `Protected` type docs for the full rationale. `risky_unwrap` is the escape hatch: it hands back the plain inner value along -with the obligation to wipe it. Combinators such as `map` show the plain value -only to their closure and return the result re-wrapped. +with the obligation to wipe it. Combinators such as `map` lend the value to +their closure by reference, wipe it when the closure returns, and return the +result re-wrapped. The `risky_update` family lends it mutably, to change it in +place; a mutable borrow can move the value out, hence the `risky_` prefix. It also provides an "opaque" implementation of the `Debug` trait so you can debug protected values without accidentally leaking their innards. @@ -119,7 +121,7 @@ use vitaminc_protected::{Controlled, Protected}; let x: Protected = Protected::new(1); let y: Protected = Protected::new(2); -let z: Protected = x.zip(y, |a, b| a + b); +let z: Protected = x.zip(y, |a, b| *a + *b); ``` If the inner type is an `Option` you can call `transpose` to swap the `Protected` and the `Option`. diff --git a/packages/protected/build.rs b/packages/protected/build.rs index 212b8343..f228130d 100644 --- a/packages/protected/build.rs +++ b/packages/protected/build.rs @@ -12,6 +12,17 @@ fn main() { // Set by `cargo kani`; declared here so the harness module is not an // unexpected cfg to an ordinary build. println!("cargo::rustc-check-cfg=cfg(kani)"); + // The stack residue tests state their guarantee for optimised builds: + // at `opt-level = 0` the compiler makes argument copies of its own that + // no wipe in this crate reaches. The level is not exposed as a `cfg`, + // and `debug_assertions` is not the same thing (a profile can turn it + // off without optimising), so it is read from the build script's + // environment here. + println!("cargo::rustc-check-cfg=cfg(vitaminc_unoptimized)"); + println!("cargo::rerun-if-env-changed=OPT_LEVEL"); + if env::var("OPT_LEVEL").as_deref() == Ok("0") { + println!("cargo::rustc-cfg=vitaminc_unoptimized"); + } let os = env::var("CARGO_CFG_TARGET_OS").unwrap_or_default(); let vendor = env::var("CARGO_CFG_TARGET_VENDOR").unwrap_or_default(); let has_limit = vendor == "apple" diff --git a/packages/protected/src/as_protected_ref.rs b/packages/protected/src/as_protected_ref.rs index 0a62d148..1acf3bb0 100644 --- a/packages/protected/src/as_protected_ref.rs +++ b/packages/protected/src/as_protected_ref.rs @@ -110,7 +110,7 @@ impl<'a, T: ?Sized> ProtectedRef<'a, T> { /// plaintext, and it can only do that through a public method. /// /// Code that receives a `ProtectedRef` could already read it, because - /// [`Controlled::update_with_ref`] hands its closure the same `&T`. This + /// [`Controlled::risky_update_with_ref`] hands its closure the same `&T`. This /// method makes that one explicit step, under the `risky_` name, rather /// than a detour through a throwaway `Protected`. /// diff --git a/packages/protected/src/controlled.rs b/packages/protected/src/controlled.rs index bf82e33e..2a081e89 100644 --- a/packages/protected/src/controlled.rs +++ b/packages/protected/src/controlled.rs @@ -6,23 +6,57 @@ use zeroize::Zeroize; /// /// This trait is sealed: downstream crates use its implementations rather than /// implementing it. [`Protected`] and the capability adapters implement it. -/// Wrappers retain their own drop/zeroization behavior until their inner value -/// is extracted. It is not a sandbox: closures and `risky_*` methods can copy, -/// expose, or retain secrets. +/// It is not a sandbox: a closure, or a `risky_*` method, can copy, expose or +/// retain what it reads. Scope restrictions are only enforced where an API +/// explicitly requires them; see [`crate::Usage`]. /// -/// # Ownership and cleanup +/// # How the combinators hand over the inner value /// -/// `map`, `map_ok`, `zip`, and `update_with` hand bare owned values to closures. -/// Those values no longer have the original wrapper's drop protection. The -/// closure must wipe any secret it discards, including on error or panic, or -/// immediately put it in a wrapper with zeroizing drop behavior. `Zeroize` -/// alone does not mean a value is wiped automatically when dropped. -/// Borrowing operations keep the wrapper, but do not prevent copies or roll -/// back mutations on panic. Scope restrictions are only enforced where an API -/// explicitly requires them; see [`crate::Usage`]. +/// `map`, `map_ok`, `zip` and `zip_ref` consume a controlled value and lend +/// its inner value to a closure as a shared reference. The closure reads the +/// value and builds a new one from it, which the combinator wraps. It never +/// owns the value, so it cannot move it anywhere: the consumed wrapper keeps +/// it, and wipes it when the combinator returns, including when the closure +/// panics. +/// +/// ``` +/// # mod vitaminc { pub mod protected { pub use vitaminc_protected::*; } } +/// use vitaminc::protected::{Controlled, Protected}; +/// let len = Protected::new(vec![1u8, 2, 3]).map(|v| v.len()); +/// assert_eq!(len.risky_unwrap(), 3); +/// ``` +/// +/// A conversion that would take the value by ownership builds its result +/// from the borrow instead. For bytes to text that is one copy, made straight +/// into the value the combinator wraps, and the error carries no bytes: +/// +/// ``` +/// # mod vitaminc { pub mod protected { pub use vitaminc_protected::*; } } +/// use vitaminc::protected::{Controlled, Protected}; +/// let text = Protected::new(vec![104u8, 105]) +/// .map_ok(|v| std::str::from_utf8(v).map(str::to_owned)) +/// .expect("valid UTF-8"); +/// assert_eq!(text.risky_unwrap(), "hi"); +/// ``` +/// +/// A borrow does not stop a closure from copying or logging what it reads. +/// What it guarantees is that the wrapper keeps ownership, so the original is +/// always wiped, and anything the closure returns is wrapped again. An error +/// returned from `map_ok` is passed through as is, so do not build one that +/// carries the value. +/// +/// # Changing the value in place +/// +/// The `risky_update` family lends the inner value mutably, to change it in +/// place, for example to decrypt a buffer. A mutable borrow can still move +/// the value out (`core::mem::take`, `core::mem::replace`), which hands the +/// closure a bare value nothing wipes. That is why these methods carry the +/// `risky_` prefix, like `risky_unwrap`: review what each closure does with +/// the value. A change the closure has already made stays if it panics; +/// there is no rollback. pub trait Controlled: ControlledPrivate + Zeroize { /// The bare value held by the wrapper, without its access restrictions. - type Inner; + type Inner: Zeroize; /// Wrap an owned value. Equivalent to [`new`](Self::new); moving a value /// here does not wipe copies or earlier stack locations held by the caller. @@ -94,8 +128,9 @@ pub trait Controlled: ControlledPrivate + Zeroize { /// Map the inner value of this [Controlled] type. /// Conceptually similar to `Option::map`. - /// The closure owns the unwrapped input and must handle its cleanup; see - /// the trait's ownership contract. The output uses the [`ReplaceT`] adapter. + /// + /// The closure borrows the inner value; see the trait docs. The output + /// uses the [`ReplaceT`] adapter. /// /// # Example /// @@ -105,27 +140,29 @@ pub trait Controlled: ControlledPrivate + Zeroize { /// # mod vitaminc { pub mod protected { pub use vitaminc_protected::*; } } /// use vitaminc::protected::{Controlled, Protected}; /// let x = Protected::new(100u8); - /// let y = x.map(|x| x + 10); + /// let y = x.map(|x| *x + 10); /// assert_eq!(y.risky_unwrap(), 110); /// ``` fn map(self, f: F) -> >::Output where Self: Sized + ReplaceT, - F: FnOnce(::Inner) -> B, + F: FnOnce(&::Inner) -> B, >::Output: Controlled, B: Zeroize, { - >::Output::init_from_inner(f(self.risky_unwrap())) + // `self` is dropped, and so wiped, when this returns or unwinds. + >::Output::init_from_inner(f(self.risky_ref())) } /// Similar to `map` but the closure returns a `Result` with the new inner value. /// The result is a `Result` with the new [Controlled] type. /// + /// The closure borrows the inner value; see the trait docs. + /// /// # Errors /// - /// Returns the closure's error unchanged. The input is consumed, and neither - /// the error nor secrets discarded by the closure are automatically wrapped - /// or wiped. See the trait's ownership contract. + /// Returns the closure's error unchanged. The input is wiped either way, + /// so the error must not carry the value. /// /// # Example /// @@ -135,22 +172,25 @@ pub trait Controlled: ControlledPrivate + Zeroize { /// # mod vitaminc { pub mod protected { pub use vitaminc_protected::*; } } /// use vitaminc::protected::{Controlled, Protected}; /// let x = Protected::new(vec![240, 159, 146, 150]); - /// let y = x.map_ok(String::from_utf8); + /// let y = x.map_ok(|v| std::str::from_utf8(v).map(str::to_owned)); /// assert!(matches!(y, Ok(_))); /// assert_eq!(y.unwrap().risky_unwrap(), "💖"); /// ``` fn map_ok(self, f: F) -> Result<>::Output, E> where Self: Sized + ReplaceT, - F: FnOnce(::Inner) -> Result, + F: FnOnce(&::Inner) -> Result, >::Output: Controlled, B: Zeroize, { - f(self.risky_unwrap()).map(>::Output::init_from_inner) + // `self` is dropped, and so wiped, when this returns or unwinds. + f(self.risky_ref()).map(>::Output::init_from_inner) } /// Zip two [Controlled] values of the same type together with a function that combines them. /// + /// The closure borrows both inner values; see the trait docs. + /// /// # Example /// /// Add two [Protected] values together. @@ -160,26 +200,29 @@ pub trait Controlled: ControlledPrivate + Zeroize { /// use vitaminc::protected::{Controlled, Protected}; /// let x = Protected::new(1); /// let y = Protected::new(2); - /// let z = x.zip(y, |x, y| x + y); + /// let z = x.zip(y, |x, y| *x + *y); /// assert_eq!(z.risky_unwrap(), 3); /// ``` /// /// The result is a plain `Protected`: adapter capabilities and usage - /// scopes are not propagated or checked. The closure owns both bare inputs - /// and is responsible for their cleanup. + /// scopes are not propagated or checked. fn zip(self, b: Other, f: F) -> Protected where Self: Sized, Other: Controlled, Out: Zeroize, - F: FnOnce(Self::Inner, Other::Inner) -> Out, + F: FnOnce(&Self::Inner, &Other::Inner) -> Out, { // TODO: Use Replace private trait - Protected::init_from_inner(f(self.risky_unwrap(), b.risky_unwrap())) + // `self` and `b` are dropped, and so wiped, when this returns or + // unwinds. + Protected::init_from_inner(f(self.risky_ref(), b.risky_ref())) } /// Like `zip` but the second argument is a reference. /// + /// The closure borrows both inner values; see the trait docs. + /// /// # Example /// /// ``` @@ -187,7 +230,7 @@ pub trait Controlled: ControlledPrivate + Zeroize { /// use vitaminc::protected::{Controlled, Protected}; /// let x = Protected::new(String::from("hello ")); /// let y = Protected::new(String::from("world")); - /// let z = x.zip_ref(&y, |x, y| x + y); + /// let z = x.zip_ref(&y, |x, y| format!("{x}{y}")); /// assert_eq!(z.risky_unwrap(), "hello world"); /// ``` /// @@ -202,14 +245,19 @@ pub trait Controlled: ControlledPrivate + Zeroize { >::Output: Controlled, Other: AsProtectedRef<'a, A>, Out: Zeroize, - F: FnOnce(Self::Inner, &A) -> Out, + F: FnOnce(&Self::Inner, &A) -> Out, { let arg: ProtectedRef<'a, A> = other.as_protected_ref(); - >::Output::init_from_inner(f(self.risky_unwrap(), arg.risky_ref())) + // `self` is dropped, and so wiped, when this returns or unwinds. + >::Output::init_from_inner(f(self.risky_ref(), arg.risky_ref())) } - /// Update the inner value in place through a mutable borrow. - /// If the closure panics, mutations already made remain; there is no rollback. + /// Change the inner value in place, through a mutable borrow. + /// + /// This is risky because the closure can move the value out of the + /// borrow (`core::mem::take`, `core::mem::replace`), and a value moved out + /// is bare. See the trait docs. If the closure panics, mutations already + /// made remain; there is no rollback. /// /// # Example /// @@ -217,7 +265,7 @@ pub trait Controlled: ControlledPrivate + Zeroize { /// # mod vitaminc { pub mod protected { pub use vitaminc_protected::*; } } /// # use vitaminc::protected::{Controlled, Protected}; /// let mut x = Protected::new([0u8; 4]); - /// x.update(|x| { + /// x.risky_update(|x| { /// x.iter_mut().for_each(|x| { /// *x += 1; /// }); @@ -225,15 +273,45 @@ pub trait Controlled: ControlledPrivate + Zeroize { /// assert_eq!(x.risky_unwrap(), [1, 1, 1, 1]); /// ``` /// - fn update(&mut self, mut f: F) + fn risky_update(&mut self, mut f: F) where F: FnMut(&mut Self::Inner), { f(self.inner_mut()); } - /// Update the inner value with another [Controlled] value. - /// The inner value of the second argument is passed to the closure. + /// Like `risky_update`, but the closure can fail. + /// + /// The error is returned as is. The value stays in the wrapper, possibly + /// part changed, and is wiped when the wrapper drops, so a caller that + /// gives up on an error can just drop it. The error must not carry the + /// value. + /// + /// # Example + /// + /// ``` + /// # mod vitaminc { pub mod protected { pub use vitaminc_protected::*; } } + /// # use vitaminc::protected::{Controlled, Protected}; + /// let mut x = Protected::new(vec![1u8, 2, 3]); + /// x.risky_try_update(|v| { + /// v.pop().ok_or("empty")?; + /// Ok::<_, &str>(()) + /// }) + /// .expect("not empty"); + /// assert_eq!(x.risky_unwrap(), [1, 2]); + /// ``` + fn risky_try_update(&mut self, f: F) -> Result<(), E> + where + F: FnOnce(&mut Self::Inner) -> Result<(), E>, + { + f(self.inner_mut()) + } + + /// Change the inner value in place, using the value of another + /// [Controlled] value, which the closure borrows and which is wiped when + /// this returns. + /// + /// This is risky for the same reason as `risky_update`. /// /// # Example /// @@ -242,28 +320,24 @@ pub trait Controlled: ControlledPrivate + Zeroize { /// use vitaminc::protected::{Controlled, Protected}; /// let mut x = Protected::new([0u8; 32]); /// let y = Protected::new([1u8; 32]); - /// x.update_with(y, |x, y| { - /// x.copy_from_slice(&y); + /// x.risky_update_with(y, |x, y| { + /// x.copy_from_slice(y); /// }); /// assert_eq!(x.risky_unwrap(), [1u8; 32]); /// ``` /// - /// No usage-scope compatibility is checked. The closure receives `other`'s - /// bare inner value and must wipe discarded secrets. Mutations to `self` - /// are not rolled back if the closure panics. - fn update_with(&mut self, other: Other, mut f: F) + /// No usage-scope compatibility is checked. Mutations to `self` are not + /// rolled back if the closure panics. + fn risky_update_with(&mut self, other: Other, mut f: F) where - F: FnMut(&mut Self::Inner, Other::Inner), + F: FnMut(&mut Self::Inner, &Other::Inner), Other: Controlled, { - // FIXME: There's a chance here that other will be dropped and not zeroized correctly - // But not all Zeroize types are ZeroizeOnDrop - we may need to yield a wrapper type that Derefs to the inner value - // Ditto for the zip method - // Either that or just make sure the caller uses zeroize() on the other value :/ - f(self.inner_mut(), other.risky_unwrap()); + // `other` is dropped, and so wiped, when this returns or unwinds. + f(self.inner_mut(), other.risky_ref()); } - /// Like `update_with` but the second argument is a reference. + /// Like `risky_update_with` but the second argument is a reference. /// /// # Example /// @@ -274,13 +348,13 @@ pub trait Controlled: ControlledPrivate + Zeroize { /// /// let mut x = Protected::new([0u8; 32]); /// let y = Protected::new([1u8; 32]); - /// x.update_with_ref(y.as_protected_ref(), |x, y| { + /// x.risky_update_with_ref(y.as_protected_ref(), |x, y| { /// x.copy_from_slice(y); /// }); /// assert_eq!(x.risky_unwrap(), [1u8; 32]); /// ``` /// - fn update_with_ref<'a, A, F>(&mut self, other: ProtectedRef<'a, A>, mut f: F) + fn risky_update_with_ref<'a, A, F>(&mut self, other: ProtectedRef<'a, A>, mut f: F) where A: ?Sized + 'a, F: FnMut(&mut Self::Inner, &A), @@ -329,6 +403,11 @@ pub trait Controlled: ControlledPrivate + Zeroize { /// negating the protections that the [Controlled] type provides. /// /// **Use with caution!** + /// + /// The returned value is bare: nothing wipes it any more. A result that + /// is not used is dropped on the spot, unwiped, so the compiler warns + /// when that happens. + #[must_use = "the bare value is dropped unwiped if it is not used"] fn risky_unwrap(self) -> Self::Inner; /// Provides a reference to the inner value. @@ -355,14 +434,14 @@ mod tests { #[test] fn test_map_homogenous_inner() { let x = Protected::new(100u8); - let y = x.map(|x| x + 10); + let y = x.map(|x| *x + 10); assert_eq!(y.risky_unwrap(), 110u8); } #[test] fn test_map_different_inner() { let x = Protected::new(100u8); - let y: Protected = x.map(u16::from); + let y: Protected = x.map(|x| u16::from(*x)); assert_eq!(y.risky_unwrap(), 100u16); } @@ -370,25 +449,25 @@ mod tests { fn test_map_adapter_homogenous_inner() { assert_eq!( Exportable::>::new(100) - .map(|x| x + 10) + .map(|x| *x + 10) .risky_unwrap(), 110u8 ); assert_eq!( Equatable::>::new(100) - .map(|x| x + 10) + .map(|x| *x + 10) .risky_unwrap(), 110u8 ); assert_eq!( Exportable::>>::new(100) - .map(|x| x + 10) + .map(|x| *x + 10) .risky_unwrap(), 110u8 ); assert_eq!( Equatable::>>::new(100) - .map(|x| x + 10) + .map(|x| *x + 10) .risky_unwrap(), 110u8 ); @@ -398,25 +477,25 @@ mod tests { fn test_map_adapter_different_inner() { assert_eq!( Exportable::>::new(100) - .map(u16::from) + .map(|x| u16::from(*x)) .risky_unwrap(), 100u16 ); assert_eq!( Equatable::>::new(100) - .map(u16::from) + .map(|x| u16::from(*x)) .risky_unwrap(), 100u16 ); assert_eq!( Exportable::>>::new(100) - .map(u16::from) + .map(|x| u16::from(*x)) .risky_unwrap(), 100u16 ); assert_eq!( Equatable::>>::new(100) - .map(u16::from) + .map(|x| u16::from(*x)) .risky_unwrap(), 100u16 ); diff --git a/packages/protected/src/equatable/mod.rs b/packages/protected/src/equatable/mod.rs index d8240537..8531d0ad 100644 --- a/packages/protected/src/equatable/mod.rs +++ b/packages/protected/src/equatable/mod.rs @@ -115,12 +115,12 @@ impl Equatable { } } -// SAFETY: `inner_ptr` returns a pointer to `self`'s live, owned inner field, and +// SAFETY: `inner_ptr_mut` returns a pointer to `self`'s live, owned inner field, and // `Equatable`'s derived `Drop` only zeroizes — satisfying `MoveInner`'s contract. unsafe impl crate::MoveInner for Equatable { type Inner = T; - fn inner_ptr(&self) -> *const T { - &self.0 + fn inner_ptr_mut(&mut self) -> *mut T { + &mut self.0 } } diff --git a/packages/protected/src/exportable/mod.rs b/packages/protected/src/exportable/mod.rs index 88cc8e57..91a0d9cd 100644 --- a/packages/protected/src/exportable/mod.rs +++ b/packages/protected/src/exportable/mod.rs @@ -73,12 +73,12 @@ impl Exportable { } } -// SAFETY: `inner_ptr` returns a pointer to `self`'s live, owned inner field, and +// SAFETY: `inner_ptr_mut` returns a pointer to `self`'s live, owned inner field, and // `Exportable`'s derived `Drop` only zeroizes — satisfying `MoveInner`'s contract. unsafe impl crate::MoveInner for Exportable { type Inner = T; - fn inner_ptr(&self) -> *const T { - &self.0 + fn inner_ptr_mut(&mut self) -> *mut T { + &mut self.0 } } diff --git a/packages/protected/src/lib.rs b/packages/protected/src/lib.rs index 330971a4..0fee995b 100644 --- a/packages/protected/src/lib.rs +++ b/packages/protected/src/lib.rs @@ -79,49 +79,64 @@ pub use vitaminc_protected_derive::{OpaqueDebug, TimingSafeEq}; /// /// # Safety /// -/// `inner_ptr` must return a valid, well-aligned pointer to the wrapper's live, -/// owned inner field (i.e. `&self.0`), and the implementing type's `Drop` must do -/// nothing but zeroize — so that [`move_inner_out`] skipping it leaks only the -/// wipe, never a resource. +/// `inner_ptr_mut` must return a valid, well-aligned pointer to the wrapper's +/// live, owned inner field (i.e. `&mut self.0`), and the implementing type's +/// `Drop` must do nothing but zeroize — so that [`move_inner_out`] skipping it +/// leaks only the wipe, never a resource. pub(crate) unsafe trait MoveInner { /// The owned inner field type. type Inner; /// Pointer to the live, owned inner field (see the trait's safety contract). - fn inner_ptr(&self) -> *const Self::Inner; + fn inner_ptr_mut(&mut self) -> *mut Self::Inner; } /// Move the single owned field out of a controlled wrapper (e.g. `Protected`) -/// **without** running its zeroizing `Drop`. +/// **without** running its zeroizing `Drop`, and wipe the slot it came from. /// -/// The inner field is bit-copied to the caller and `wrapper` is forgotten so its +/// The inner field is bit-copied to the caller, the bytes of the wrapper's +/// field are then overwritten with zeroes, and the wrapper is forgotten so its /// `Drop` never runs against the now-moved-out field (no double-free, no /// use-after-zeroize). This is the shared primitive behind /// `risky_unwrap` / `flatten` / `transpose`. /// -/// This is a *safe* function: the only soundness obligation — that `inner_ptr` -/// names the wrapper's live, owned field — is discharged by the [`MoveInner`] -/// `unsafe` trait, and the pointer is derived from `wrapper` *after* this -/// function owns it, so no pointer outlives the move that produced it. +/// This is a *safe* function: the only soundness obligation — that +/// `inner_ptr_mut` names the wrapper's live, owned field — is discharged by the +/// [`MoveInner`] `unsafe` trait, and the pointer is derived from `wrapper` +/// *after* this function owns it, so no pointer outlives the move that +/// produced it. /// -/// # Why the source slot is deliberately NOT scrubbed +/// # Why the slot is wiped bitwise, not zeroized /// /// `ptr::read` produces a **bitwise** copy. For heap-backed inners (`Vec`, /// `String`, `Box`, …) the returned value and `wrapper`'s field then alias the -/// *same* heap allocation. Scrubbing the source semantically (e.g. -/// `inner.zeroize()` against the source) would wipe the heap the returned value -/// still owns — a use-after-zeroize that hands the caller corrupted data (a -/// decrypted `Protected>` would come back all zeroes). For inline inners -/// (`[u8; N]`) the abandoned stack copy is the inherent cost of an explicit -/// move-out: `risky_unwrap` & friends transfer ownership — and the wiping -/// obligation — to the caller by contract. +/// *same* heap allocation. Zeroizing the source (`inner.zeroize()`) would wipe +/// the heap the returned value still owns, and a decrypted +/// `Protected>` would come back all zeroes. Overwriting the field's +/// own bytes instead touches only what lives inline: for a `Vec` that is its +/// pointer, length and capacity, which is harmless; for an inline inner +/// (`[u8; N]`) it is the secret itself, which would otherwise stay in the +/// abandoned stack slot. The wipe uses volatile stores and a fence, so it is +/// not removed as a dead store. It mirrors `Locked::move_in`, which wipes the +/// source slot on the way in. +/// +/// The wipe reaches the slot this function is given. A copy the caller made +/// before the call, such as an argument temporary at `opt-level = 0`, is +/// outside it. pub(crate) fn move_inner_out(wrapper: W) -> W::Inner { - // SAFETY: by `MoveInner`'s contract, `inner_ptr` points to `wrapper`'s live, - // owned inner field. It is read exactly once and `wrapper` is then forgotten, - // so the returned value is the sole owner — no double-free, and the wrapper's - // zeroizing `Drop` never wipes the moved-out value. - let inner = unsafe { core::ptr::read(wrapper.inner_ptr()) }; - core::mem::forget(wrapper); - inner + let mut wrapper = core::mem::ManuallyDrop::new(wrapper); + let slot = wrapper.inner_ptr_mut(); + // SAFETY: by `MoveInner`'s contract, `slot` points to `wrapper`'s live, + // owned inner field. It is read exactly once, and `wrapper` is never + // dropped (it is in a `ManuallyDrop` that is not taken), so the returned + // value is the sole owner — no double-free, and the wrapper's zeroizing + // `Drop` never wipes the moved-out value. The slot is then overwritten + // bitwise and never read as an `Inner` again, which is `wipe_bytes`'s + // contract. + unsafe { + let inner = core::ptr::read(slot); + locked::wipe_bytes(slot); + inner + } } /// ReplaceT is a sealed trait that is used to replace the inner value of a type. @@ -204,9 +219,7 @@ mod private { impl Lends for Equatable {} impl Lends for Exportable {} - /// Private trait that is used to hide the inner value of a Controlled type - /// as well as preventing consumers from implementing Controlled themselves. - /// Marker trait used to seal the `Controlled` trait, preventing external implementations. - /// This trait is only implemented within this crate. + /// Marker trait that seals [`Controlled`](crate::Controlled), so that it + /// is only implemented within this crate. pub trait ControlledPrivate {} } diff --git a/packages/protected/src/locked/mod.rs b/packages/protected/src/locked/mod.rs index d932521b..a6bd7869 100644 --- a/packages/protected/src/locked/mod.rs +++ b/packages/protected/src/locked/mod.rs @@ -716,7 +716,7 @@ pub(crate) fn wipe_vec(out: &mut Vec) { /// /// `slot` must point to `size_of::()` writable bytes that will never be /// read as a `T` again. -unsafe fn wipe_bytes(slot: *mut T) { +pub(crate) unsafe fn wipe_bytes(slot: *mut T) { wipe_raw(slot.cast::(), mem::size_of::()); } diff --git a/packages/protected/src/ops.rs b/packages/protected/src/ops.rs index 4ab4f951..5cbfa76c 100644 --- a/packages/protected/src/ops.rs +++ b/packages/protected/src/ops.rs @@ -3,12 +3,13 @@ use zeroize::Zeroize; use crate::{Controlled, Protected}; -impl BitXor for Protected +impl BitXor for Protected where - T: BitXor + Zeroize, - ::Output: Zeroize, + T: Zeroize, + for<'a> &'a T: BitXor<&'a T, Output = O>, + O: Zeroize, { - type Output = Protected; + type Output = Protected; fn bitxor(self, rhs: Self) -> Self::Output { self.zip(rhs, |x, y| x ^ y) diff --git a/packages/protected/src/protected/mod.rs b/packages/protected/src/protected/mod.rs index e4a7bf08..806e614d 100644 --- a/packages/protected/src/protected/mod.rs +++ b/packages/protected/src/protected/mod.rs @@ -20,9 +20,9 @@ use zeroize::{Zeroize, ZeroizeOnDrop}; /// /// [`Controlled::risky_unwrap`] is the escape hatch: it hands back the plain /// inner value, and with it the wiping obligation. Combinators such as -/// [`Controlled::map`] expose the plain value only to their closure and -/// return the result re-wrapped in a controlled type; the closure owns the -/// plain value for its own duration and anything it copies out of it. +/// [`Controlled::map`] lend the plain value to their closure by reference, +/// wipe it when the closure returns, and return the result re-wrapped in a +/// controlled type; the closure owns only what it copies out of the borrow. #[derive(Zeroize, ZeroizeOnDrop, OpaqueDebug)] pub struct Protected(pub(crate) T); @@ -36,19 +36,18 @@ impl Protected { /// /// Ownership of the (still sensitive) value transfers to the caller, which /// becomes responsible for its lifecycle. See [`crate::move_inner_out`] for - /// the shared primitive and the rationale (including why the source slot is - /// not scrubbed). + /// the shared primitive, which also wipes the slot the value leaves. fn into_inner_unchecked(self) -> T { crate::move_inner_out(self) } } -// SAFETY: `inner_ptr` returns a pointer to `self`'s live, owned inner field, and +// SAFETY: `inner_ptr_mut` returns a pointer to `self`'s live, owned inner field, and // `Protected`'s derived `Drop` only zeroizes — satisfying `MoveInner`'s contract. unsafe impl crate::MoveInner for Protected { type Inner = T; - fn inner_ptr(&self) -> *const T { - &self.0 + fn inner_ptr_mut(&mut self) -> *mut T { + &mut self.0 } } @@ -72,6 +71,22 @@ impl Protected> { } } +impl Protected { + /// Move the value into a protected `Some`, without it leaving + /// protection. The inverse of [`transpose`](Protected::transpose) for a + /// value that is present. + /// + /// ``` + /// # mod vitaminc { pub mod protected { pub use vitaminc_protected::*; } } + /// use vitaminc::protected::{Controlled, Protected}; + /// let x = Protected::new([1u8; 32]).into_some(); + /// assert_eq!(x.risky_unwrap(), Some([1u8; 32])); + /// ``` + pub fn into_some(self) -> Protected> { + Protected::new(Some(self.into_inner_unchecked())) + } +} + impl Protected> { #[inline] /// Transpose a [Protected] of `Option` into an `Option` of [Protected]. diff --git a/packages/protected/src/usage/mod.rs b/packages/protected/src/usage/mod.rs index abc48fbb..495fb223 100644 --- a/packages/protected/src/usage/mod.rs +++ b/packages/protected/src/usage/mod.rs @@ -14,7 +14,7 @@ use zeroize::{Zeroize, ZeroizeOnDrop}; /// # Current limitations /// /// Scope propagation is incomplete: mapping `Usage, S>` returns -/// an unscoped `Protected`, as does [`Controlled::zip`], and `update_with` does +/// an unscoped `Protected`, as does [`Controlled::zip`], and `risky_update_with` does /// not check scope compatibility. The `Controlled` escape hatches also remain /// available. Serialization is allowed /// whenever `T::Inner` implements [`SafeSerialize`], even when `T` is not an diff --git a/packages/protected/tests/residue_heap.rs b/packages/protected/tests/residue_heap.rs new file mode 100644 index 00000000..4d837021 --- /dev/null +++ b/packages/protected/tests/residue_heap.rs @@ -0,0 +1,201 @@ +//! What the `Controlled` combinators leave on the heap. +//! +//! The combinators lend the closure the inner value, and the wrapper they +//! consume wipes it when it drops. For a heap-backed value such as `Vec` +//! that means the buffer is zeroed before it is freed, however the closure +//! ends: by reading the value, by building a converted copy from it, or by +//! panicking. +//! +//! The check is an allocator that, while armed, scans every block being freed +//! for a planted marker before handing it to the system allocator. The scan +//! happens inside `dealloc`, while the block is still owned, so no freed +//! memory is ever read. `realloc` is left to the trait's default (allocate, +//! copy, free through this wrapper), so a block a growing buffer abandons is +//! scanned as well. +//! +//! The allocator is process-wide, so the tests that arm it take `SCAN`. +//! +//! Not run under Miri: the scan compares every byte of a block being freed, +//! and a block's spare capacity is uninitialised memory. +#![cfg(not(miri))] + +use core::cell::UnsafeCell; +use core::hint::black_box; +use std::alloc::{GlobalAlloc, Layout, System}; +use std::sync::atomic::{AtomicBool, AtomicUsize, Ordering}; +use std::sync::Mutex; +use vitaminc_protected::{Controlled, Protected}; + +const MARKER: &[u8] = b"vitaminc residue marker: heap secret bytes that must be wiped"; +const MAX_PATTERN: usize = 128; + +struct Pattern(UnsafeCell<[u8; MAX_PATTERN]>); +// Written only while disarmed, under `SCAN`; read only while armed. +unsafe impl Sync for Pattern {} + +static PATTERN: Pattern = Pattern(UnsafeCell::new([0; MAX_PATTERN])); +static PATTERN_LEN: AtomicUsize = AtomicUsize::new(0); +static ARMED: AtomicBool = AtomicBool::new(false); +static HITS: AtomicUsize = AtomicUsize::new(0); +static SCAN: Mutex<()> = Mutex::new(()); + +struct ScanningAlloc; + +unsafe fn block_holds_pattern(ptr: *const u8, size: usize) -> bool { + let len = PATTERN_LEN.load(Ordering::Acquire); + if len == 0 || size < len { + return false; + } + let pat = core::slice::from_raw_parts(PATTERN.0.get().cast::(), len); + let block = core::slice::from_raw_parts(ptr, size); + block.windows(len).any(|w| w == pat) +} + +unsafe impl GlobalAlloc for ScanningAlloc { + unsafe fn alloc(&self, layout: Layout) -> *mut u8 { + System.alloc(layout) + } + + unsafe fn alloc_zeroed(&self, layout: Layout) -> *mut u8 { + System.alloc_zeroed(layout) + } + + unsafe fn dealloc(&self, ptr: *mut u8, layout: Layout) { + if ARMED.load(Ordering::Acquire) && block_holds_pattern(ptr, layout.size()) { + HITS.fetch_add(1, Ordering::AcqRel); + } + System.dealloc(ptr, layout) + } +} + +#[global_allocator] +static GLOBAL: ScanningAlloc = ScanningAlloc; + +/// Freed blocks holding `MARKER` while `f` runs. +fn leaked_blocks(f: impl FnOnce()) -> usize { + let _guard = SCAN.lock().unwrap_or_else(|e| e.into_inner()); + assert!(MARKER.len() <= MAX_PATTERN); + // SAFETY: written while disarmed and under `SCAN`, so no `dealloc` reads + // the pattern concurrently. + unsafe { + core::slice::from_raw_parts_mut(PATTERN.0.get().cast::(), MARKER.len()) + .copy_from_slice(MARKER) + }; + PATTERN_LEN.store(MARKER.len(), Ordering::Release); + HITS.store(0, Ordering::Release); + ARMED.store(true, Ordering::Release); + let outcome = std::panic::catch_unwind(std::panic::AssertUnwindSafe(f)); + ARMED.store(false, Ordering::Release); + PATTERN_LEN.store(0, Ordering::Release); + let hits = HITS.load(Ordering::Acquire); + if let Err(payload) = outcome { + std::panic::resume_unwind(payload); + } + hits +} + +fn secret() -> Protected> { + Protected::new(black_box(MARKER.to_vec())) +} + +mod given_the_scan { + use super::*; + + #[test] + fn sees_a_vec_freed_unwiped() { + let n = leaked_blocks(|| { + let bare: Vec = secret().risky_unwrap(); + drop(black_box(bare)); + }); + assert!(n >= 1, "the scan should find the marker in the freed block"); + } +} + +mod given_a_heap_inner_the_closure_only_reads { + use super::*; + + #[test] + fn map_wipes_it_before_the_free() { + let p = secret(); + let n = leaked_blocks(|| drop(black_box(p.map(|v| v.len())))); + assert_eq!(n, 0, "map should wipe the Vec the closure dropped"); + } + + #[test] + fn map_ok_wipes_it_before_the_free() { + let p = secret(); + let n = leaked_blocks(|| { + let out: Result, ()> = p.map_ok(|v| Ok(v.len())); + drop(black_box(out)); + }); + assert_eq!(n, 0, "map_ok should wipe the Vec the closure dropped"); + } + + #[test] + fn zip_wipes_both_before_the_free() { + let (a, b) = (secret(), secret()); + let n = leaked_blocks(|| drop(black_box(a.zip(b, |a, b| a.len() + b.len())))); + assert_eq!(n, 0, "zip should wipe both Vecs the closure dropped"); + } + + #[test] + fn zip_ref_wipes_the_moved_one_before_the_free() { + let (a, b) = (secret(), secret()); + let n = leaked_blocks(|| { + drop(black_box(a.zip_ref(&b, |a, b: &Vec| a.len() + b.len()))); + drop(b); + }); + assert_eq!(n, 0, "zip_ref should wipe the Vec the closure dropped"); + } + + #[test] + fn risky_update_with_wipes_the_other_before_the_free() { + let (mut a, b) = (secret(), secret()); + let n = leaked_blocks(|| { + a.risky_update_with(b, |a, b| a.truncate(b.len() / 2)); + drop(a); + }); + assert_eq!( + n, 0, + "risky_update_with should wipe the Vec the closure dropped" + ); + } +} + +mod given_a_heap_inner_the_closure_converts { + use super::*; + + #[test] + fn the_source_and_the_converted_copy_are_both_wiped() { + let p = secret(); + let n = leaked_blocks(|| { + let out: Protected = p + .map_ok(|v| core::str::from_utf8(v).map(str::to_owned)) + .expect("the marker is ASCII"); + drop(out); + }); + assert_eq!( + n, 0, + "the source Vec and the converted String should both be wiped" + ); + } +} + +mod given_the_closure_panics { + use super::*; + + #[test] + fn the_value_is_wiped_on_unwind() { + let p = secret(); + let n = leaked_blocks(|| { + let outcome = std::panic::catch_unwind(std::panic::AssertUnwindSafe(|| { + let _: Protected = p.map(|v| { + black_box(v); + panic!("closure failed with the secret in hand") + }); + })); + assert!(outcome.is_err(), "the closure should have panicked"); + }); + assert_eq!(n, 0, "an unwinding closure should still wipe the Vec"); + } +} diff --git a/packages/protected/tests/residue_stack.rs b/packages/protected/tests/residue_stack.rs new file mode 100644 index 00000000..24ede021 --- /dev/null +++ b/packages/protected/tests/residue_stack.rs @@ -0,0 +1,259 @@ +//! What the `Controlled` combinators leave on the stack. +//! +//! Moving a value whose bytes live inline (`Protected<[u8; N]>`) copies them, +//! and the slot a move leaves behind holds the secret until something else +//! overwrites it. The combinators lend the inner value to the closure where it +//! sits, and the wrapper they consume wipes it when it drops. These tests +//! check that: each case plants a marker inside a `Protected<[u8; 32]>`, +//! runs one combinator on a fresh thread, then searches the dead stack below +//! the caller for the marker. +//! +//! The technique is the one ore.rs uses for its stack scrub tests: paint the +//! stack below the caller, run the case, then copy the region out with +//! volatile reads (so the copy itself calls nothing that would overwrite it) +//! and search it. +//! +//! The wipe reaches the wrapper the combinator is given. At `opt-level = 0` +//! the compiler makes an argument copy of its own for a by-value call, which +//! is outside it, so the zero-copy cases are ignored there and the guarantee +//! is stated for optimised builds. Run with `--release` to exercise them. +//! +//! Not run under Miri: the snapshot reads dead stack through raw pointers, +//! which is outside any allocation Miri knows about. +#![cfg(not(miri))] + +use core::hint::black_box; +use std::thread; +use vitaminc_protected::{Controlled, Protected}; + +type Array = [u8; 32]; +type Case = fn(Protected) -> usize; +const MARKER: &Array = b"vitaminc residue marker: 32 byte"; + +/// Bytes of dead stack [`after`] copies and searches. +const SCAN_BYTES: usize = 64 * 1024; + +/// Fill the stack below the caller with a marker byte, so that nothing from +/// an earlier case is mistaken for this one's residue. +#[inline(never)] +fn paint() { + let mut buf = [0xa5u8; SCAN_BYTES + 4096]; + black_box(&mut buf); +} + +/// Copy the `SCAN_BYTES` of stack below this frame into `out`, using only +/// volatile reads, so that the copy calls no function that would overwrite +/// the region first. +#[inline(never)] +fn snapshot(out: &mut [u8]) { + let marker = 0u8; + let top = black_box(&marker as *const u8); + // Scan right up to `marker`: the cases are shallow, so their residue + // sits close below this frame. This frame's own slots fall inside the + // region too, and none of them can hold the marker. + let start = top.wrapping_sub(out.len()); + for (i, slot) in out.iter_mut().enumerate() { + // SAFETY: the region is inside this thread's stack (which `after` + // sizes well beyond `SCAN_BYTES`), below the live frames, and mapped. + // It is read as plain bytes. + *slot = unsafe { core::ptr::read_volatile(start.wrapping_add(i)) }; + } +} + +/// Run `f` below a padding frame, so that its residue sits deeper than the +/// frames `snapshot` and its callees use. At `opt-level = 0` nothing in the +/// snapshot loop is inlined, and without the padding those calls overwrite +/// the first few hundred bytes of the region before they are read. +#[inline(never)] +fn run(f: F) { + let mut pad = [0u8; 4096]; + black_box(&mut pad); + f() +} + +/// The dead stack left by `f`, run on a fresh thread: `SCAN_BYTES`, ordered +/// from the deepest address up. +fn after(f: F) -> Vec { + thread::Builder::new() + .stack_size(4 * 1024 * 1024) + .spawn(move || { + let mut out = vec![0u8; SCAN_BYTES]; + paint(); + run(f); + snapshot(&mut out); + out + }) + .unwrap() + .join() + .unwrap() +} + +/// How many times `needle` occurs in `dead`. +fn copies(dead: &[u8], needle: &[u8]) -> usize { + dead.windows(needle.len()).filter(|w| *w == needle).count() +} + +/// A `Protected` holding the marker, filled in place so the only copy is +/// the one inside the wrapper. +#[inline(never)] +fn planted() -> Protected { + let mut p = Protected::new([0u8; 32]); + p.risky_update(|a| a.copy_from_slice(black_box(MARKER))); + p +} + +// Each case takes the value by value, which is the shape the combinators are +// used in and the one that makes a move. The closures read one byte so that +// the compiler cannot drop the value as unused. + +#[inline(never)] +fn by_map(p: Protected) -> usize { + p.map(|a| black_box(a)[0] as usize).risky_unwrap() +} + +#[inline(never)] +fn by_map_ok(p: Protected) -> usize { + p.map_ok(|a| Ok::<_, ()>(black_box(a)[0] as usize)) + .unwrap() + .risky_unwrap() +} + +#[inline(never)] +fn by_zip(p: Protected) -> usize { + p.zip(Protected::new(0usize), |a, n| black_box(a)[0] as usize + *n) + .risky_unwrap() +} + +#[inline(never)] +fn by_zip_ref(p: Protected) -> usize { + let other = Protected::new(0usize); + p.zip_ref(&other, |a, n: &usize| black_box(a)[0] as usize + *n) + .risky_unwrap() +} + +#[inline(never)] +fn by_risky_update_with(p: Protected) -> usize { + let mut sink = Protected::new(0usize); + sink.risky_update_with(p, |s, a| *s = black_box(a)[0] as usize); + sink.risky_unwrap() +} + +/// Positive control: `risky_unwrap` hands the bare array to the caller, +/// whose plain local is not wiped. The scan must see that copy, or it is +/// not looking at the right memory. +#[inline(never)] +fn by_risky_unwrap(p: Protected) -> usize { + let bare = p.risky_unwrap(); + black_box(&bare)[0] as usize +} + +fn residue(case: Case) -> usize { + let dead = after(move || { + assert_eq!(black_box(case(planted())), MARKER[0] as usize); + }); + copies(&dead, MARKER) +} + +mod given_the_scan { + use super::*; + + #[test] + fn sees_the_bare_array_risky_unwrap_hands_out() { + assert!( + residue(by_risky_unwrap) >= 1, + "the scan should find the plain copy the caller is left holding" + ); + } +} + +mod given_an_inline_array_moved_into_a_combinator { + use super::*; + + #[test] + #[cfg_attr( + vitaminc_unoptimized, + ignore = "the wipe reaches the compiler's own argument copy only at opt-level 1 and above" + )] + fn map_leaves_no_copy() { + assert_eq!(residue(by_map), 0, "map should leave no copy of the array"); + } + + #[test] + #[cfg_attr( + vitaminc_unoptimized, + ignore = "the wipe reaches the compiler's own argument copy only at opt-level 1 and above" + )] + fn map_ok_leaves_no_copy() { + assert_eq!( + residue(by_map_ok), + 0, + "map_ok should leave no copy of the array" + ); + } + + #[test] + #[cfg_attr( + vitaminc_unoptimized, + ignore = "the wipe reaches the compiler's own argument copy only at opt-level 1 and above" + )] + fn zip_leaves_no_copy() { + assert_eq!(residue(by_zip), 0, "zip should leave no copy of the array"); + } + + #[test] + #[cfg_attr( + vitaminc_unoptimized, + ignore = "the wipe reaches the compiler's own argument copy only at opt-level 1 and above" + )] + fn zip_ref_leaves_no_copy() { + assert_eq!( + residue(by_zip_ref), + 0, + "zip_ref should leave no copy of the array" + ); + } + + #[test] + #[cfg_attr( + vitaminc_unoptimized, + ignore = "the wipe reaches the compiler's own argument copy only at opt-level 1 and above" + )] + fn risky_update_with_leaves_no_copy() { + assert_eq!( + residue(by_risky_update_with), + 0, + "risky_update_with should leave no copy of the array" + ); + } +} + +/// Where each case leaves the marker, as offsets below the top of the scan. +/// A diagnostic for when a case fails: `cargo test --release --test +/// residue_stack -- --ignored --nocapture report`. +#[test] +#[ignore = "diagnostic: prints where each case leaves its residue"] +fn report() { + let cases: [(&str, Case); 6] = [ + ("risky_unwrap (control)", by_risky_unwrap), + ("map", by_map), + ("map_ok", by_map_ok), + ("zip", by_zip), + ("zip_ref", by_zip_ref), + ("risky_update_with", by_risky_update_with), + ]; + for (name, case) in cases { + let dead = after(move || { + assert_eq!(black_box(case(planted())), MARKER[0] as usize); + }); + let offsets: Vec = dead + .windows(MARKER.len()) + .enumerate() + .filter(|(_, w)| *w == MARKER) + .map(|(i, _)| dead.len() - i) + .collect(); + eprintln!( + "{name:24} copies={} offsets_below_top={offsets:?}", + offsets.len() + ); + } +} diff --git a/packages/protected/tests/ui/negative_space/usage_does_not_lend_a_protected_ref.rs b/packages/protected/tests/ui/negative_space/usage_does_not_lend_a_protected_ref.rs index fb9da182..c2897c6d 100644 --- a/packages/protected/tests/ui/negative_space/usage_does_not_lend_a_protected_ref.rs +++ b/packages/protected/tests/ui/negative_space/usage_does_not_lend_a_protected_ref.rs @@ -1,5 +1,5 @@ // A `Usage` scope says which code may take the value. A `ProtectedRef` would -// carry it to any consumer (`zip_ref`, `update_with_ref`, encryption), so a +// carry it to any consumer (`zip_ref`, `risky_update_with_ref`, encryption), so a // `Usage` does not lend one; `risky_ref` is the deliberate way in. use vitaminc_protected::{AsProtectedRef, Protected, ProtectedRef, Usage}; diff --git a/packages/random/src/bounded.rs b/packages/random/src/bounded.rs index 787a2f87..5bbbd989 100644 --- a/packages/random/src/bounded.rs +++ b/packages/random/src/bounded.rs @@ -113,13 +113,12 @@ impl BoundedRng> for SafeRand { /// secret; a caller whose secret bound may be zero should construct a /// [`Protected`] and use that impl, which never panics. fn next_below(&mut self, n: Protected) -> Protected { - // Check the bound before `map` unwraps it: `Controlled::map` hands - // the raw inner value to the closure, so a zero check inside the - // closure would panic with the secret already out of its wrapper and - // unwinding without being wiped. Checked here, an unwind drops `n` - // still wrapped, and `Protected`'s drop glue zeroizes it. + // Check the bound here rather than inside the closure, so the + // panic names this method's contract. Either way the value is + // wiped on unwind: here `n` is still wrapped, and inside the closure + // `Controlled::map` only lends it, and `n` is wiped when it drops. assert!(*n.risky_ref() != 0, "range must be non-zero"); - n.map(|n| below_u32(self, n)) + n.map(|n| below_u32(self, *n)) } } @@ -146,7 +145,7 @@ impl BoundedRngInclusive for SafeRand { impl BoundedRngInclusive> for SafeRand { fn next_bounded(&mut self, max: Protected) -> Protected { // `0..=max` is never empty, so no pre-check is needed here. - max.map(|max| upto_u32(self, max)) + max.map(|max| upto_u32(self, *max)) } }