From 793a10436e67473f49fa03cbac6ae9fd3bff8a14 Mon Sep 17 00:00:00 2001 From: James Maes Date: Wed, 29 Jul 2026 22:07:42 -0500 Subject: [PATCH] feat(security): zeroize long-lived engine key material on drop Wrap DatabaseEngine's storage/WORM/ledger keys, WebhookDispatcher's HMAC secret, and add_account's transient password in zeroize::Zeroizing so their backing memory is overwritten once the holding struct drops, instead of lingering as plain heap bytes for the rest of the process lifetime. Refs #199, #163 --- Cargo.lock | 2 + crates/nuncio-filter/Cargo.toml | 1 + crates/nuncio-filter/src/webhook.rs | 5 ++- crates/nuncio-store/src/db.rs | 61 ++++++++++++++++++++++++++--- crates/nunciod/Cargo.toml | 1 + crates/nunciod/src/grpc.rs | 8 +++- 6 files changed, 69 insertions(+), 9 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index ac6e67b..a9c3920 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -2561,6 +2561,7 @@ dependencies = [ "tokio", "tracing", "wiremock", + "zeroize", ] [[package]] @@ -2653,6 +2654,7 @@ dependencies = [ "tracing", "tracing-subscriber", "wiremock", + "zeroize", ] [[package]] diff --git a/crates/nuncio-filter/Cargo.toml b/crates/nuncio-filter/Cargo.toml index e65bf9e..31c98f8 100644 --- a/crates/nuncio-filter/Cargo.toml +++ b/crates/nuncio-filter/Cargo.toml @@ -25,6 +25,7 @@ async-trait.workspace = true chrono.workspace = true reqwest.workspace = true hex.workspace = true +zeroize.workspace = true [dev-dependencies] tempfile.workspace = true diff --git a/crates/nuncio-filter/src/webhook.rs b/crates/nuncio-filter/src/webhook.rs index ec25944..d25dd41 100644 --- a/crates/nuncio-filter/src/webhook.rs +++ b/crates/nuncio-filter/src/webhook.rs @@ -8,6 +8,7 @@ use serde_json::json; use sha2::Sha256; use std::time::{Duration, SystemTime, UNIX_EPOCH}; use thiserror::Error; +use zeroize::Zeroizing; type HmacSha256 = Hmac; @@ -28,7 +29,7 @@ pub enum WebhookError { /// Outbound Webhook Dispatcher executing authenticated HTTP POST requests. pub struct WebhookDispatcher { client: Client, - secret_key: String, + secret_key: Zeroizing, } impl WebhookDispatcher { @@ -42,7 +43,7 @@ impl WebhookDispatcher { Self { client, - secret_key: secret_key.into(), + secret_key: Zeroizing::new(secret_key.into()), } } diff --git a/crates/nuncio-store/src/db.rs b/crates/nuncio-store/src/db.rs index 7cc09b6..4095690 100644 --- a/crates/nuncio-store/src/db.rs +++ b/crates/nuncio-store/src/db.rs @@ -6,6 +6,7 @@ use std::path::Path; use std::str::FromStr; use tempfile::TempDir; use thiserror::Error; +use zeroize::Zeroizing; /// Raw row shape for a pending remote mutation record fetched from SQLite. type PendingMutationRow = (String, String, String, String, String, String, i64, i64); @@ -102,9 +103,9 @@ impl DatabaseError { #[derive(Clone)] pub struct DatabaseEngine { pool: SqlitePool, - storage_key: [u8; 32], - worm_key: Vec, - ledger_key: Vec, + storage_key: Zeroizing<[u8; 32]>, + worm_key: Zeroizing>, + ledger_key: Zeroizing>, } impl std::fmt::Debug for DatabaseEngine { @@ -122,7 +123,7 @@ impl std::fmt::Debug for DatabaseEngine { #[allow(clippy::type_complexity)] fn resolve_engine_keys( secrets: &crate::vault::SecretManager, -) -> Result<([u8; 32], Vec, Vec), DatabaseError> { +) -> Result<(Zeroizing<[u8; 32]>, Zeroizing>, Zeroizing>), DatabaseError> { let storage_key_bytes = secrets .get_or_create_key_bytes(crate::vault::STORAGE_KEY_ACCOUNT, 32) .map_err(|e| DatabaseError::KeyProvisioning(e.to_string()))?; @@ -135,7 +136,11 @@ fn resolve_engine_keys( let ledger_key = secrets .get_or_create_key_bytes(crate::vault::LEDGER_KEY_ACCOUNT, 32) .map_err(|e| DatabaseError::KeyProvisioning(e.to_string()))?; - Ok((storage_key, worm_key, ledger_key)) + Ok(( + Zeroizing::new(storage_key), + Zeroizing::new(worm_key), + Zeroizing::new(ledger_key), + )) } /// Reconstruct a [`nuncio_contacts::Contact`] from a `contacts` table row. @@ -3268,4 +3273,50 @@ mod tests { assert_eq!(report.record_count, 1); assert_eq!(report.first_broken_seq, Some(1)); } + + /// Proves the zeroize mechanism `Zeroizing`'s `Drop` relies on is real, not + /// decorative: explicitly invoking `Zeroize::zeroize` on key-shaped byte + /// buffers overwrites every byte with zero. This is checked directly + /// (without dropping the value) because inspecting freed heap memory would + /// require `unsafe`, which this workspace forbids. + #[test] + fn zeroize_mechanism_actually_wipes_key_bytes() { + use zeroize::Zeroize; + + let mut fixed = Zeroizing::new([0xABu8; 32]); + assert!(fixed.iter().all(|&b| b != 0), "fixture must start non-zero"); + fixed.zeroize(); + assert!( + fixed.iter().all(|&b| b == 0), + "Zeroize::zeroize must overwrite every byte of a fixed-size key buffer" + ); + + let mut variable = Zeroizing::new(vec![0xCDu8; 32]); + assert!( + variable.iter().all(|&b| b != 0), + "fixture must start non-zero" + ); + variable.zeroize(); + assert!( + variable.iter().all(|&b| b == 0), + "Zeroize::zeroize must overwrite every byte of a heap-allocated key buffer" + ); + } + + /// `DatabaseEngine`'s manual `Debug` impl must never print the raw + /// storage/WORM/ledger key bytes it now holds in `Zeroizing` wrappers, + /// even after they were introduced. + #[tokio::test] + async fn debug_impl_never_prints_raw_key_bytes() { + let (engine, _dir) = DatabaseEngine::connect_ephemeral() + .await + .expect("connect ephemeral test db"); + let debug_output = format!("{engine:?}"); + assert!( + !debug_output.contains("storage_key") + && !debug_output.contains("worm_key") + && !debug_output.contains("ledger_key"), + "Debug output must never expose key field names/values: {debug_output}" + ); + } } diff --git a/crates/nunciod/Cargo.toml b/crates/nunciod/Cargo.toml index 663ac49..a43ef22 100644 --- a/crates/nunciod/Cargo.toml +++ b/crates/nunciod/Cargo.toml @@ -33,6 +33,7 @@ async-trait.workspace = true subtle.workspace = true hex.workspace = true bytes.workspace = true +zeroize.workspace = true [dev-dependencies] nuncio-cli = { path = "../nuncio-cli" } diff --git a/crates/nunciod/src/grpc.rs b/crates/nunciod/src/grpc.rs index f5cb81a..f8379cc 100644 --- a/crates/nunciod/src/grpc.rs +++ b/crates/nunciod/src/grpc.rs @@ -511,10 +511,14 @@ impl Accounts for AccountsGrpcService { request: Request, ) -> Result, Status> { let req = request.into_inner(); + // Moved into a zeroizing wrapper immediately so the plaintext credential's + // backing memory is overwritten as soon as this function returns, rather + // than lingering as an ordinary `String` for the rest of the process. + let password = zeroize::Zeroizing::new(req.password); let proto_config = req .config .ok_or_else(|| Status::invalid_argument("config is required"))?; - if req.password.is_empty() { + if password.is_empty() { return Err(Status::invalid_argument("password is required")); } @@ -538,7 +542,7 @@ impl Accounts for AccountsGrpcService { // The original persistence error is always what the caller sees -- // a rollback failure is reported alongside it, never in place of it. self.secrets - .set_secret(&config.keyring_secret_key, &req.password) + .set_secret(&config.keyring_secret_key, &password) .map_err(|e| Status::internal(format!("failed to store credential in vault: {e}")))?; if let Err(e) = self.db.save_account(&config).await { -- 2.54.0