fix: Migrate away from lazy_static in favor of thread_local

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.

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 }

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<SecretMemoryPool> = Mutex::new(SecretMemoryPool::new());
+thread_local! {
+    static SECRET_CACHE: RefCell<SecretMemoryPool> = RefCell::new(SecretMemoryPool::new());
+}
+
+fn with_secret_memory_pool<Fn, R>(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<T: Zeroize + ?Sized>(Option<SecretBox<T>>);
+
+impl<T: Zeroize> ZeroizingSecretBox<T> {
+    fn new(boxed: T) -> Self {
+        ZeroizingSecretBox(Some(secret_box(boxed)))
+    }
+}
+
+impl<T: Zeroize + ?Sized> ZeroizingSecretBox<T> {
+    fn from_secret_box(inner: SecretBox<T>) -> Self {
+        Self(Some(inner))
+    }
+
+    fn take(mut self) -> SecretBox<T> {
+        self.0.take().unwrap()
+    }
+}
+
+impl<T: Zeroize + ?Sized> ZeroizeOnDrop for ZeroizingSecretBox<T> {}
+impl<T: Zeroize + ?Sized> Zeroize for ZeroizingSecretBox<T> {
+    fn zeroize(&mut self) {
+        if let Some(inner) = &mut self.0 {
+            let inner: &mut SecretBox<T> = inner; // type annotation
+            inner.zeroize()
+        }
+    }
+}
+
+impl<T: Zeroize + ?Sized> Drop for ZeroizingSecretBox<T> {
+    fn drop(&mut self) {
+        self.zeroize()
+    }
+}
+
+impl<T: Zeroize + ?Sized> Deref for ZeroizingSecretBox<T> {
+    type Target = T;
+
+    fn deref(&self) -> &T {
+        &self.0.as_ref().unwrap()
+    }
+}
+
+impl<T: Zeroize + ?Sized> DerefMut for ZeroizingSecretBox<T> {
+    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<usize, Vec<SecretBox<[u8]>>>,
+    pool: HashMap<usize, Vec<ZeroizingSecretBox<[u8]>>>,
 }
 
 impl SecretMemoryPool {
@@ -44,33 +116,37 @@ impl SecretMemoryPool {
     }
 
     /// Return secret back to the pool for future re-use
-    pub fn release<const N: usize>(&mut self, mut sec: SecretBox<[u8; N]>) {
+    pub fn release<const N: usize>(&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<u8> = sec.into();
+        let sec: SecretVec<u8> = 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<const N: usize>(&mut self) -> SecretBox<[u8; N]> {
+    pub fn take<const N: usize>(&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<const N: usize> {
-    storage: Option<SecretBox<[u8; N]>>,
+    storage: Option<ZeroizingSecretBox<[u8; N]>>,
 }
 
 impl<const N: usize> Secret<N> {
@@ -84,9 +160,12 @@ impl<const N: usize> Secret<N> {
     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<const N: usize> Secret<N> {
 
     /// 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<const N: usize> Secret<N> {
     }
 }
 
-impl<const N: usize> ZeroizeOnDrop for Secret<N> {}
-impl<const N: usize> Zeroize for Secret<N> {
-    fn zeroize(&mut self) {
-        self.secret_mut().zeroize();
-    }
-}
-
 impl<const N: usize> Randomize for Secret<N> {
     fn try_fill<R: Rng + ?Sized>(&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<const N: usize> Randomize for Secret<N> {
     }
 }
 
+impl<const N: usize> ZeroizeOnDrop for Secret<N> {}
+impl<const N: usize> Zeroize for Secret<N> {
+    fn zeroize(&mut self) {
+        if let Some(inner) = &mut self.storage {
+            inner.zeroize()
+        }
+    }
+}
+
 impl<const N: usize> Drop for Secret<N> {
     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()
     }
 }