From 9824db4f0917a3a88b4e1c742a5b09a7dd0116e0 Mon Sep 17 00:00:00 2001 From: Karolin Varner Date: Tue, 2 Jan 2024 19:57:51 +0100 Subject: [PATCH] fix: Migrate away from lazy_static in favor of thread_local MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The new secret memory pool was causing CI failures in the fuzzing code, due to the fuzzer compiling its binaries with memory sanitizer support. https://doc.rust-lang.org/beta/unstable-book/compiler-flags/sanitizer.html Using lazy_static was – intentionally – introducing a memory leak, but the LeakSanitizer detected this and raised an error. Now by using thread_local we are calling the destructors and so – while still being a memory leak in practice – the LeakSanitizer no longer detects this behaviour as an error. Alternatively we could have used a known-leaks list with the leak-sanitizer, but this would have increased the complexity of the build setup. Finally, this was likely triggered with the migration to memsec, because libsodium circumvents the malloc/free calls, relying on direct calls to MMAP. --- Cargo.lock | 140 ++++++++++++++++++++++++++++++--- Cargo.toml | 1 - secret-memory/Cargo.toml | 1 - secret-memory/src/secret.rs | 151 ++++++++++++++++++++++++++++-------- 4 files changed, 248 insertions(+), 45 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index 918c4cd..28b7eab 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -188,7 +188,7 @@ dependencies = [ "regex", "rustc-hash", "shlex", - "syn", + "syn 2.0.39", "which", ] @@ -256,6 +256,12 @@ version = "1.0.0" source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "baf1de4339761588bc0619e3cbc0120ee582ebb74b53b4efbf79117bd2da40fd" +[[package]] +name = "cfg_aliases" +version = "0.1.1" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "fd16c4719339c4530435d38e511904438d07cce7950afa3718a84ac36c10e89e" + [[package]] name = "ciborium" version = "0.2.1" @@ -337,7 +343,7 @@ dependencies = [ "heck", "proc-macro2", "quote", - "syn", + "syn 2.0.39", ] [[package]] @@ -471,7 +477,7 @@ checksum = "67e77553c4162a157adbf834ebae5b415acbecbeafc7a74b0e886657506a7611" dependencies = [ "proc-macro2", "quote", - "syn", + "syn 2.0.39", ] [[package]] @@ -523,7 +529,7 @@ checksum = "d4029edd3e734da6fe05b6cd7bd2960760a616bd2ddd0d59a0124746d6272af0" dependencies = [ "cfg-if", "libc", - "redox_syscall", + "redox_syscall 0.3.5", "windows-sys 0.48.0", ] @@ -662,6 +668,15 @@ dependencies = [ "hashbrown 0.14.3", ] +[[package]] +name = "instant" +version = "0.1.12" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "7a5bbe824c507c5da5956355e86a746d82e0e1464f65d862cc5e71da70e94b2c" +dependencies = [ + "cfg-if", +] + [[package]] name = "is-terminal" version = "0.4.9" @@ -792,6 +807,16 @@ version = "0.4.12" source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "c4cd1a83af159aa67994778be9070f0ae1bd732942279cabb14f86f986a21456" +[[package]] +name = "lock_api" +version = "0.4.11" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "3c168f8615b12bc01f9c17e2eb0cc07dcae1940121185446edc3744920e8ef45" +dependencies = [ + "autocfg", + "scopeguard", +] + [[package]] name = "log" version = "0.4.20" @@ -915,6 +940,31 @@ version = "6.6.1" source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "e2355d85b9a3786f481747ced0e0ff2ba35213a1f9bd406ed906554d7af805a1" +[[package]] +name = "parking_lot" +version = "0.11.2" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "7d17b78036a60663b797adeaee46f5c9dfebb86948d1255007a1d6be0271ff99" +dependencies = [ + "instant", + "lock_api", + "parking_lot_core", +] + +[[package]] +name = "parking_lot_core" +version = "0.8.6" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "60a2cfe6f0ad2bfc16aefa463b497d5c7a5ecd44a23efa72aa342d90177356dc" +dependencies = [ + "cfg-if", + "instant", + "libc", + "redox_syscall 0.2.16", + "smallvec", + "winapi", +] + [[package]] name = "paste" version = "1.0.14" @@ -980,7 +1030,7 @@ source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "ae005bd773ab59b4725093fd7df83fd7892f7d8eafb48dbd7de6e024e4215f9d" dependencies = [ "proc-macro2", - "syn", + "syn 2.0.39", ] [[package]] @@ -1060,6 +1110,15 @@ dependencies = [ "crossbeam-utils", ] +[[package]] +name = "redox_syscall" +version = "0.2.16" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "fb5a58c1855b4b6819d59012155603f0b22ad30cad752600aadfcb695265519a" +dependencies = [ + "bitflags 1.3.2", +] + [[package]] name = "redox_syscall" version = "0.3.5" @@ -1213,7 +1272,6 @@ dependencies = [ "allocator-api2", "allocator-api2-tests", "anyhow", - "lazy_static", "libsodium-sys-stable", "log", "memsec", @@ -1221,6 +1279,7 @@ dependencies = [ "rosenpass-sodium", "rosenpass-to", "rosenpass-util", + "static_init", "zeroize", ] @@ -1346,7 +1405,7 @@ checksum = "43576ca501357b9b071ac53cdc7da8ef0cbd9493d8df094cd821777ea6e894d3" dependencies = [ "proc-macro2", "quote", - "syn", + "syn 2.0.39", ] [[package]] @@ -1375,6 +1434,12 @@ version = "1.2.0" source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "a7cee0529a6d40f580e7a5e6c495c8fbfe21b7b52795ed4bb5e62cdf92bc6380" +[[package]] +name = "smallvec" +version = "1.11.2" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "4dccd0940a2dcdf68d092b8cbab7dc0ad8fa938bf95787e1b916b0e3d0e8e970" + [[package]] name = "spin" version = "0.9.8" @@ -1400,12 +1465,51 @@ version = "1.1.0" source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "a2eb9349b6444b326872e140eb1cf5e7c522154d69e7a0ffb0fb81c06b37543f" +[[package]] +name = "static_init" +version = "1.0.3" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "8a2a1c578e98c1c16fc3b8ec1328f7659a500737d7a0c6d625e73e830ff9c1f6" +dependencies = [ + "bitflags 1.3.2", + "cfg_aliases", + "libc", + "parking_lot", + "parking_lot_core", + "static_init_macro", + "winapi", +] + +[[package]] +name = "static_init_macro" +version = "1.0.2" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "70a2595fc3aa78f2d0e45dd425b22282dd863273761cc77780914b2cf3003acf" +dependencies = [ + "cfg_aliases", + "memchr", + "proc-macro2", + "quote", + "syn 1.0.109", +] + [[package]] name = "strsim" version = "0.10.0" source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "73473c0e59e6d5812c5dfe2a064a6444949f089e20eec9a2e5506596494e4623" +[[package]] +name = "syn" +version = "1.0.109" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "72b64191b275b66ffe2469e8af2c1cfe3bafa67b529ead792a6d0160888b4237" +dependencies = [ + "proc-macro2", + "quote", + "unicode-ident", +] + [[package]] name = "syn" version = "2.0.39" @@ -1466,7 +1570,7 @@ checksum = "266b2e40bc00e5a6c09c3584011e08b06f123c00362c92b975ba9843aaaa14b8" dependencies = [ "proc-macro2", "quote", - "syn", + "syn 2.0.39", ] [[package]] @@ -1636,7 +1740,7 @@ dependencies = [ "once_cell", "proc-macro2", "quote", - "syn", + "syn 2.0.39", "wasm-bindgen-shared", ] @@ -1658,7 +1762,7 @@ checksum = "c5353b8dab669f5e10f5bd76df26a9360c748f054f862ff5f3f8aae0c7fb3907" dependencies = [ "proc-macro2", "quote", - "syn", + "syn 2.0.39", "wasm-bindgen-backend", "wasm-bindgen-shared", ] @@ -1961,7 +2065,7 @@ checksum = "c2f140bda219a26ccc0cdb03dba58af72590c53b22642577d88a927bc5c87d6b" dependencies = [ "proc-macro2", "quote", - "syn", + "syn 2.0.39", ] [[package]] @@ -1969,6 +2073,20 @@ name = "zeroize" version = "1.7.0" source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "525b4ec142c6b68a2d10f01f7bbf6755599ca3f81ea53b8431b7dd348f5fdb2d" +dependencies = [ + "zeroize_derive", +] + +[[package]] +name = "zeroize_derive" +version = "1.4.2" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "ce36e65b0d2999d2aafac989fb249189a141aee1f53c612c1f37d72631959f69" +dependencies = [ + "proc-macro2", + "quote", + "syn 2.0.39", +] [[package]] name = "zip" diff --git a/Cargo.toml b/Cargo.toml index 96337f8..43a401b 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -42,7 +42,6 @@ doc-comment = "0.3.3" base64 = "0.21.5" zeroize = "1.7.0" memoffset = "0.9.0" -lazy_static = "1.4.0" thiserror = "1.0.50" paste = "1.0.14" env_logger = "0.10.1" diff --git a/secret-memory/Cargo.toml b/secret-memory/Cargo.toml index ebb4d19..3f56f3d 100644 --- a/secret-memory/Cargo.toml +++ b/secret-memory/Cargo.toml @@ -15,7 +15,6 @@ rosenpass-to = { workspace = true } rosenpass-sodium = { workspace = true } rosenpass-util = { workspace = true } libsodium-sys-stable = { workspace = true } -lazy_static = { workspace = true } zeroize = { workspace = true } rand = { workspace = true } memsec = { workspace = true } diff --git a/secret-memory/src/secret.rs b/secret-memory/src/secret.rs index 74764fe..ab859c3 100644 --- a/secret-memory/src/secret.rs +++ b/secret-memory/src/secret.rs @@ -1,15 +1,17 @@ -use std::{collections::HashMap, convert::TryInto, fmt, path::Path, sync::Mutex}; +use std::cell::RefCell; +use std::collections::HashMap; +use std::convert::TryInto; +use std::fmt; +use std::ops::{Deref, DerefMut}; +use std::path::Path; use anyhow::Context; -use lazy_static::lazy_static; use rand::{Fill as Randomize, Rng}; use zeroize::{Zeroize, ZeroizeOnDrop}; -use rosenpass_util::{ - b64::b64_reader, - file::{fopen_r, LoadValue, LoadValueB64, ReadExactToEnd}, - functional::mutating, -}; +use rosenpass_util::b64::b64_reader; +use rosenpass_util::file::{fopen_r, LoadValue, LoadValueB64, ReadExactToEnd}; +use rosenpass_util::functional::mutating; use crate::alloc::{secret_box, SecretBox, SecretVec}; use crate::file::StoreSecret; @@ -17,8 +19,78 @@ use crate::file::StoreSecret; // This might become a problem in library usage; it's effectively a memory // leak which probably isn't a problem right now because most memory will // be reused… -lazy_static! { - static ref SECRET_CACHE: Mutex = Mutex::new(SecretMemoryPool::new()); +thread_local! { + static SECRET_CACHE: RefCell = RefCell::new(SecretMemoryPool::new()); +} + +fn with_secret_memory_pool(mut f: Fn) -> R +where + Fn: FnMut(Option<&mut SecretMemoryPool>) -> R, +{ + // This acquires the SECRET_CACHE + SECRET_CACHE + .try_with(|cell| { + // And acquires the inner reference + cell.try_borrow_mut() + .as_deref_mut() + // To call the given function + .map(|pool| f(Some(pool))) + .ok() + }) + .ok() + .flatten() + // Failing that, the given function is called with None + .unwrap_or_else(|| f(None)) +} + +// Wrapper around SecretBox that applies automatic zeroization +#[derive(Debug)] +struct ZeroizingSecretBox(Option>); + +impl ZeroizingSecretBox { + fn new(boxed: T) -> Self { + ZeroizingSecretBox(Some(secret_box(boxed))) + } +} + +impl ZeroizingSecretBox { + fn from_secret_box(inner: SecretBox) -> Self { + Self(Some(inner)) + } + + fn take(mut self) -> SecretBox { + self.0.take().unwrap() + } +} + +impl ZeroizeOnDrop for ZeroizingSecretBox {} +impl Zeroize for ZeroizingSecretBox { + fn zeroize(&mut self) { + if let Some(inner) = &mut self.0 { + let inner: &mut SecretBox = inner; // type annotation + inner.zeroize() + } + } +} + +impl Drop for ZeroizingSecretBox { + fn drop(&mut self) { + self.zeroize() + } +} + +impl Deref for ZeroizingSecretBox { + type Target = T; + + fn deref(&self) -> &T { + &self.0.as_ref().unwrap() + } +} + +impl DerefMut for ZeroizingSecretBox { + fn deref_mut(&mut self) -> &mut T { + self.0.as_mut().unwrap() + } } /// Pool that stores secret memory allocations @@ -31,7 +103,7 @@ lazy_static! { /// [libsodium documentation](https://libsodium.gitbook.io/doc/memory_management#guarded-heap-allocations) #[derive(Debug)] // TODO check on Debug derive, is that clever struct SecretMemoryPool { - pool: HashMap>>, + pool: HashMap>>, } impl SecretMemoryPool { @@ -44,33 +116,37 @@ impl SecretMemoryPool { } /// Return secret back to the pool for future re-use - pub fn release(&mut self, mut sec: SecretBox<[u8; N]>) { + pub fn release(&mut self, mut sec: ZeroizingSecretBox<[u8; N]>) { sec.zeroize(); // This conversion sequence is weird but at least it guarantees // that the heap allocation is preserved according to the docs - let sec: SecretVec = sec.into(); + let sec: SecretVec = sec.take().into(); let sec: SecretBox<[u8]> = sec.into(); - self.pool.entry(N).or_default().push(sec); + self.pool + .entry(N) + .or_default() + .push(ZeroizingSecretBox::from_secret_box(sec)); } /// Take protected memory from the pool, allocating new one if no suitable /// chunk is found in the inventory. /// /// The secret is guaranteed to be full of nullbytes - pub fn take(&mut self) -> SecretBox<[u8; N]> { + pub fn take(&mut self) -> ZeroizingSecretBox<[u8; N]> { let entry = self.pool.entry(N).or_default(); - match entry.pop() { + let inner = match entry.pop() { None => secret_box([0u8; N]), - Some(sec) => sec.try_into().unwrap(), - } + Some(sec) => sec.take().try_into().unwrap(), + }; + ZeroizingSecretBox::from_secret_box(inner) } } /// Storeage for a secret backed by [rosenpass_sodium::alloc::Alloc] pub struct Secret { - storage: Option>, + storage: Option>, } impl Secret { @@ -84,9 +160,12 @@ impl Secret { pub fn zero() -> Self { // Using [SecretMemoryPool] here because this operation is expensive, // yet it is used in hot loops - Self { - storage: Some(SECRET_CACHE.lock().unwrap().take()), - } + let buf = with_secret_memory_pool(|pool| { + pool.map(|p| p.take()) + .unwrap_or_else(|| ZeroizingSecretBox::new([0u8; N])) + }); + + Self { storage: Some(buf) } } /// Returns a new [Secret] that is randomized @@ -101,7 +180,7 @@ impl Secret { /// Borrows the data pub fn secret(&self) -> &[u8; N] { - self.storage.as_ref().unwrap() + &self.storage.as_ref().unwrap() } /// Borrows the data mutably @@ -110,13 +189,6 @@ impl Secret { } } -impl ZeroizeOnDrop for Secret {} -impl Zeroize for Secret { - fn zeroize(&mut self) { - self.secret_mut().zeroize(); - } -} - impl Randomize for Secret { fn try_fill(&mut self, rng: &mut R) -> Result<(), rand::Error> { // Zeroize self first just to make sure the barriers from the zeroize create take @@ -127,11 +199,26 @@ impl Randomize for Secret { } } +impl ZeroizeOnDrop for Secret {} +impl Zeroize for Secret { + fn zeroize(&mut self) { + if let Some(inner) = &mut self.storage { + inner.zeroize() + } + } +} + impl Drop for Secret { fn drop(&mut self) { - self.storage - .take() - .map(|sec| SECRET_CACHE.lock().unwrap().release(sec)); + with_secret_memory_pool(|pool| { + if let Some((pool, secret)) = pool.zip(self.storage.take()) { + pool.release(secret); + } + }); + + // This should be unnecessary: The pool has one item – the inner secret – which + // zeroizes itself on drop. Calling it should not do any harm though… + self.zeroize() } }