From cf132bca1154b434a3340c7c69c73eb9834dd993 Mon Sep 17 00:00:00 2001 From: Karolin Varner Date: Thu, 30 Nov 2023 12:46:13 +0100 Subject: [PATCH] chore: Move rest of coloring.rs into secret-memory crate Also removes the StoreSecret trait from cli.rs as it was redundant. --- Cargo.lock | 3 +- fuzz/Cargo.toml | 1 + fuzz/fuzz_targets/handle_msg.rs | 2 +- rosenpass/Cargo.toml | 1 - rosenpass/src/cli.rs | 16 +-- rosenpass/src/lib.rs | 1 - rosenpass/src/prftree.rs | 2 +- rosenpass/src/protocol.rs | 3 +- secret-memory/Cargo.toml | 2 + secret-memory/src/file.rs | 7 ++ secret-memory/src/lib.rs | 6 +- .../src/secret.rs | 111 ++++++++---------- 12 files changed, 72 insertions(+), 83 deletions(-) create mode 100644 secret-memory/src/file.rs rename rosenpass/src/coloring.rs => secret-memory/src/secret.rs (95%) diff --git a/Cargo.lock b/Cargo.lock index 0afaded..4e6870b 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -1064,7 +1064,6 @@ dependencies = [ "clap 4.4.8", "criterion", "env_logger", - "lazy_static", "libsodium-sys-stable", "log", "memoffset", @@ -1122,6 +1121,8 @@ name = "rosenpass-secret-memory" version = "0.1.0" dependencies = [ "anyhow", + "lazy_static", + "libsodium-sys-stable", "rosenpass-sodium", "rosenpass-to", "rosenpass-util", diff --git a/fuzz/Cargo.toml b/fuzz/Cargo.toml index 9fb6f66..345f553 100644 --- a/fuzz/Cargo.toml +++ b/fuzz/Cargo.toml @@ -11,6 +11,7 @@ cargo-fuzz = true arbitrary = { workspace = true } libfuzzer-sys = { workspace = true } stacker = { workspace = true } +rosenpass-secret-memory = { workspace = true } rosenpass-sodium = { workspace = true } rosenpass-ciphers = { workspace = true } rosenpass-to = { workspace = true } diff --git a/fuzz/fuzz_targets/handle_msg.rs b/fuzz/fuzz_targets/handle_msg.rs index 2d202d9..83d3533 100644 --- a/fuzz/fuzz_targets/handle_msg.rs +++ b/fuzz/fuzz_targets/handle_msg.rs @@ -3,8 +3,8 @@ extern crate rosenpass; use libfuzzer_sys::fuzz_target; -use rosenpass::coloring::Secret; use rosenpass::protocol::CryptoServer; +use rosenpass_secret_memory::Secret; use rosenpass_sodium::init as sodium_init; fuzz_target!(|rx_buf: &[u8]| { diff --git a/rosenpass/Cargo.toml b/rosenpass/Cargo.toml index 49b7253..f8a0591 100644 --- a/rosenpass/Cargo.toml +++ b/rosenpass/Cargo.toml @@ -25,7 +25,6 @@ static_assertions = { workspace = true } memoffset = { workspace = true } libsodium-sys-stable = { workspace = true } oqs-sys = { workspace = true } -lazy_static = { workspace = true } thiserror = { workspace = true } paste = { workspace = true } log = { workspace = true } diff --git a/rosenpass/src/cli.rs b/rosenpass/src/cli.rs index 2e888f5..ddb9ad3 100644 --- a/rosenpass/src/cli.rs +++ b/rosenpass/src/cli.rs @@ -1,13 +1,12 @@ use anyhow::{bail, ensure}; use clap::Parser; +use rosenpass_secret_memory::file::StoreSecret; use rosenpass_util::file::{LoadValue, LoadValueB64}; -use std::path::{Path, PathBuf}; +use std::path::PathBuf; use crate::app_server; use crate::app_server::AppServer; use crate::{ - // app_server::{AppServer, LoadValue, LoadValueB64}, - coloring::Secret, pqkem::{StaticKEM, KEM}, protocol::{SPk, SSk, SymKey}, }; @@ -248,14 +247,3 @@ impl Cli { srv.event_loop() } } - -trait StoreSecret { - fn store_secret>(&self, path: P) -> anyhow::Result<()>; -} - -impl StoreSecret for Secret { - fn store_secret>(&self, path: P) -> anyhow::Result<()> { - std::fs::write(path, self.secret())?; - Ok(()) - } -} diff --git a/rosenpass/src/lib.rs b/rosenpass/src/lib.rs index b223400..ee49bb6 100644 --- a/rosenpass/src/lib.rs +++ b/rosenpass/src/lib.rs @@ -1,4 +1,3 @@ -pub mod coloring; #[rustfmt::skip] pub mod labeled_prf; pub mod app_server; diff --git a/rosenpass/src/prftree.rs b/rosenpass/src/prftree.rs index f3956f1..289646c 100644 --- a/rosenpass/src/prftree.rs +++ b/rosenpass/src/prftree.rs @@ -1,5 +1,5 @@ //! Implementation of the tree-like structure used for the label derivation in [labeled_prf](crate::labeled_prf) -use crate::coloring::Secret; +use rosenpass_secret_memory::Secret; use anyhow::Result; use rosenpass_ciphers::{hash, KEY_LEN}; diff --git a/rosenpass/src/protocol.rs b/rosenpass/src/protocol.rs index 29926a7..0207f0a 100644 --- a/rosenpass/src/protocol.rs +++ b/rosenpass/src/protocol.rs @@ -68,7 +68,6 @@ //! ``` use crate::{ - coloring::*, labeled_prf as lprf, msgs::*, pqkem::*, @@ -76,7 +75,7 @@ use crate::{ }; use anyhow::{bail, ensure, Context, Result}; use rosenpass_ciphers::{aead, xaead, KEY_LEN}; -use rosenpass_secret_memory::Public; +use rosenpass_secret_memory::{Public, Secret}; use rosenpass_util::{cat, mem::cpy_min, ord::max_usize, time::Timebase}; use std::collections::hash_map::{ Entry::{Occupied, Vacant}, diff --git a/secret-memory/Cargo.toml b/secret-memory/Cargo.toml index 0220ffd..48d5ddb 100644 --- a/secret-memory/Cargo.toml +++ b/secret-memory/Cargo.toml @@ -14,3 +14,5 @@ anyhow = { workspace = true } rosenpass-to = { workspace = true } rosenpass-sodium = { workspace = true } rosenpass-util = { workspace = true } +libsodium-sys-stable = { workspace = true } +lazy_static = { workspace = true } diff --git a/secret-memory/src/file.rs b/secret-memory/src/file.rs new file mode 100644 index 0000000..f67bb52 --- /dev/null +++ b/secret-memory/src/file.rs @@ -0,0 +1,7 @@ +use std::path::Path; + +pub trait StoreSecret { + type Error; + + fn store_secret>(&self, path: P) -> Result<(), Self::Error>; +} diff --git a/secret-memory/src/lib.rs b/secret-memory/src/lib.rs index fae4d3f..3e57f54 100644 --- a/secret-memory/src/lib.rs +++ b/secret-memory/src/lib.rs @@ -1,4 +1,8 @@ +pub mod debug; +pub mod file; + mod public; pub use crate::public::Public; -pub mod debug; +mod secret; +pub use crate::secret::Secret; diff --git a/rosenpass/src/coloring.rs b/secret-memory/src/secret.rs similarity index 95% rename from rosenpass/src/coloring.rs rename to secret-memory/src/secret.rs index 85cddad..97983dc 100644 --- a/rosenpass/src/coloring.rs +++ b/secret-memory/src/secret.rs @@ -3,15 +3,17 @@ use lazy_static::lazy_static; use libsodium_sys as libsodium; use rosenpass_util::{ b64::b64_reader, - file::{fopen_r, LoadValue, LoadValueB64, ReadExactToEnd, StoreValue}, + file::{fopen_r, LoadValue, LoadValueB64, ReadExactToEnd}, functional::mutating, }; -use std::result::Result; + use std::{ collections::HashMap, convert::TryInto, fmt, os::raw::c_void, path::Path, ptr::null_mut, sync::Mutex, }; +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… @@ -28,7 +30,7 @@ lazy_static! { /// Further information about the protection in place can be found in in the /// [libsodium documentation](https://libsodium.gitbook.io/doc/memory_management#guarded-heap-allocations) #[derive(Debug)] // TODO check on Debug derive, is that clever -pub struct SecretMemoryPool { +struct SecretMemoryPool { pool: HashMap>, } @@ -44,6 +46,7 @@ impl SecretMemoryPool { /// Return secrete back to the pool for future re-use /// /// This consumes the [Secret], but its memory is re-used. + #[allow(dead_code)] pub fn release(&mut self, mut s: Secret) { unsafe { self.release_by_ref(&mut s); @@ -201,6 +204,50 @@ impl fmt::Debug for Secret { } } +impl LoadValue for Secret { + type Error = anyhow::Error; + + fn load>(path: P) -> anyhow::Result { + let mut v = Self::random(); + let p = path.as_ref(); + fopen_r(p)? + .read_exact_to_end(v.secret_mut()) + .with_context(|| format!("Could not load file {p:?}"))?; + Ok(v) + } +} + +impl LoadValueB64 for Secret { + type Error = anyhow::Error; + + fn load_b64>(path: P) -> anyhow::Result { + use std::io::Read; + + let mut v = Self::random(); + let p = path.as_ref(); + // This might leave some fragments of the secret on the stack; + // in practice this is likely not a problem because the stack likely + // will be overwritten by something else soon but this is not exactly + // guaranteed. It would be possible to remedy this, but since the secret + // data will linger in the Linux page cache anyways with the current + // implementation, going to great length to erase the secret here is + // not worth it right now. + b64_reader(&mut fopen_r(p)?) + .read_exact(v.secret_mut()) + .with_context(|| format!("Could not load base64 file {p:?}"))?; + Ok(v) + } +} + +impl StoreSecret for Secret { + type Error = anyhow::Error; + + fn store_secret>(&self, path: P) -> anyhow::Result<()> { + std::fs::write(path, self.secret())?; + Ok(()) + } +} + #[cfg(test)] mod test { use super::*; @@ -274,61 +321,3 @@ mod test { assert_eq!(new_secret.secret(), &[0; N]); } } - -trait StoreSecret { - type Error; - - fn store_secret>(&self, path: P) -> Result<(), Self::Error>; -} - -impl StoreSecret for T { - type Error = ::Error; - - fn store_secret>(&self, path: P) -> Result<(), Self::Error> { - self.store(path) - } -} - -impl LoadValue for Secret { - type Error = anyhow::Error; - - fn load>(path: P) -> anyhow::Result { - let mut v = Self::random(); - let p = path.as_ref(); - fopen_r(p)? - .read_exact_to_end(v.secret_mut()) - .with_context(|| format!("Could not load file {p:?}"))?; - Ok(v) - } -} - -impl LoadValueB64 for Secret { - type Error = anyhow::Error; - - fn load_b64>(path: P) -> anyhow::Result { - use std::io::Read; - - let mut v = Self::random(); - let p = path.as_ref(); - // This might leave some fragments of the secret on the stack; - // in practice this is likely not a problem because the stack likely - // will be overwritten by something else soon but this is not exactly - // guaranteed. It would be possible to remedy this, but since the secret - // data will linger in the Linux page cache anyways with the current - // implementation, going to great length to erase the secret here is - // not worth it right now. - b64_reader(&mut fopen_r(p)?) - .read_exact(v.secret_mut()) - .with_context(|| format!("Could not load base64 file {p:?}"))?; - Ok(v) - } -} - -impl StoreSecret for Secret { - type Error = anyhow::Error; - - fn store_secret>(&self, path: P) -> anyhow::Result<()> { - std::fs::write(path, self.secret())?; - Ok(()) - } -}