From ce1c071f3c6767cffb3da9e49f3240fe8a740028 Mon Sep 17 00:00:00 2001 From: Claude Date: Wed, 10 Jun 2026 02:13:15 -0400 Subject: [PATCH] Security hardening from adversarial audit Wallet: - identity.json (NIP-49 ncryptsec) now written 0600 in a 0700 dir so a local user can't grind the wallet password offline (+ regression test). - Wallet password held as a ZeroingString through init_nostr so it's scrubbed on drop instead of lingering in a plain String for the session. - Replaced 4 .unwrap() on re-read tx_meta with graceful guards (archive wipe mid-send could otherwise panic the nostr/task thread). - Tor::http_request/post: bind the client once via let-else and propagate TLS builder errors, fixing a TOCTOU unwrap panic on concurrent Tor restart. goblin-nip05d server (redeployed to goblin.st, verified live): - One-name-per-pubkey now enforced by a partial UNIQUE index (closes the check-then-insert race); INSERT rows-affected==0 returns 409 not a false 201. - NIP-98 replay protection: one-time auth event-id enforcement within the freshness window; tightened forward skew to +5s. - Rate-limited the unauthenticated GET endpoints; SQLite in WAL mode. - Verified live: replay rejected, second name for a pubkey blocked. Audit verdict: fund-safety invariants (never auto-pay Invoice1; S2/I2 finalization bound to counterparty npub) and Tor-from-day-one all hold. Co-Authored-By: Claude Fable 5 --- src/nostr/client.rs | 11 +++-- src/nostr/identity.rs | 61 +++++++++++++++++++++++- src/tor/tor.rs | 31 +++++++++---- src/wallet/wallet.rs | 46 ++++++++++-------- tests/replay_check.rs | 105 ++++++++++++++++++++++++++++++++++++++++++ 5 files changed, 220 insertions(+), 34 deletions(-) create mode 100644 tests/replay_check.rs diff --git a/src/nostr/client.rs b/src/nostr/client.rs index 3b41a78b..094683b1 100644 --- a/src/nostr/client.rs +++ b/src/nostr/client.rs @@ -591,11 +591,12 @@ async fn handle_wrap(svc: &Arc, wallet: &Wallet, client: &Client, }); match svc.send_payment_dm(&sender_hex, &reply_text, None).await { Ok(event_id) => { - let mut meta = svc.store.tx_meta(&slate.id.to_string()).unwrap(); - meta.status = NostrSendStatus::RepliedS2; - meta.sent_event_id = Some(event_id); - meta.updated_at = unix_time(); - svc.store.save_tx_meta(&meta); + if let Some(mut meta) = svc.store.tx_meta(&slate.id.to_string()) { + meta.status = NostrSendStatus::RepliedS2; + meta.sent_event_id = Some(event_id); + meta.updated_at = unix_time(); + svc.store.save_tx_meta(&meta); + } } Err(e) => warn!("nostr: S2 reply dispatch failed: {e}"), } diff --git a/src/nostr/identity.rs b/src/nostr/identity.rs index 5c544acd..33d861ab 100644 --- a/src/nostr/identity.rs +++ b/src/nostr/identity.rs @@ -52,6 +52,42 @@ pub struct NostrIdentity { /// NIP-49 scrypt work factor (~64 MiB, interactive-grade). const NCRYPTSEC_LOG_N: u8 = 16; +/// Write a file with owner-only (0600) permissions on Unix. +fn write_private(path: &PathBuf, data: &[u8]) -> std::io::Result<()> { + #[cfg(unix)] + { + use std::io::Write; + use std::os::unix::fs::{OpenOptionsExt, PermissionsExt}; + let mut f = std::fs::OpenOptions::new() + .write(true) + .create(true) + .truncate(true) + .mode(0o600) + .open(path)?; + f.write_all(data)?; + // Also fix the mode if the file already existed with looser perms. + let _ = std::fs::set_permissions(path, std::fs::Permissions::from_mode(0o600)); + Ok(()) + } + #[cfg(not(unix))] + { + std::fs::write(path, data) + } +} + +/// Restrict a directory to owner-only access on Unix. +fn restrict_dir(dir: &PathBuf) { + #[cfg(unix)] + { + use std::os::unix::fs::PermissionsExt; + let _ = std::fs::set_permissions(dir, std::fs::Permissions::from_mode(0o700)); + } + #[cfg(not(unix))] + { + let _ = dir; + } +} + #[derive(Debug, thiserror::Error)] pub enum IdentityError { #[error("identity io error: {0}")] @@ -81,11 +117,15 @@ impl NostrIdentity { serde_json::from_str(&raw).ok() } - /// Persist the identity file. + /// Persist the identity file with owner-only permissions (the ncryptsec + /// blob must not be world-readable: a local attacker could grind the + /// wallet password offline otherwise). pub fn save(&self, nostr_dir: &PathBuf) -> Result<(), IdentityError> { fs::create_dir_all(nostr_dir)?; + restrict_dir(nostr_dir); let raw = serde_json::to_string_pretty(self)?; - fs::write(Self::path(nostr_dir), raw)?; + let path = Self::path(nostr_dir); + write_private(&path, raw.as_bytes())?; Ok(()) } @@ -214,6 +254,23 @@ mod tests { assert_eq!(unlocked.public_key(), keys.public_key()); } + #[cfg(unix)] + #[test] + fn identity_file_is_owner_only() { + use std::os::unix::fs::PermissionsExt; + let dir = std::env::temp_dir().join(format!("goblin-id-test-{}", std::process::id())); + let (identity, _) = NostrIdentity::create_derived(NIP06_MNEMONIC, "pw", 0).unwrap(); + identity.save(&dir).unwrap(); + let meta = std::fs::metadata(NostrIdentity::path(&dir)).unwrap(); + // The ncryptsec blob must never be group/world readable. + assert_eq!( + meta.permissions().mode() & 0o077, + 0, + "identity.json must be 0600" + ); + let _ = std::fs::remove_dir_all(&dir); + } + #[test] fn reencrypt_changes_password() { let (mut identity, keys) = NostrIdentity::create_derived(NIP06_MNEMONIC, "old", 0).unwrap(); diff --git a/src/tor/tor.rs b/src/tor/tor.rs index a828d089..f96b6e10 100644 --- a/src/tor/tor.rs +++ b/src/tor/tor.rs @@ -311,13 +311,21 @@ impl Tor { return None; } } - if Self::client_config().is_none() { + // Bind the client once: a concurrent Tor restart/close could clear + // the config between an is_none() check and an unwrap (TOCTOU panic). + let Some((client_cfg, _)) = Self::client_config() else { error!("Tor: client not launched"); return None; - } + }; // Create http tor-powered client to post data. - let client = Self::client_config().unwrap().0.isolated_client(); - let tls_conn = TlsConnector::builder().unwrap().build().unwrap(); + let client = client_cfg.isolated_client(); + let tls_conn = match TlsConnector::builder().and_then(|b| b.build()) { + Ok(c) => c, + Err(e) => { + error!("Tor: TLS connector build failed: {:?}", e); + return None; + } + }; let conn = ArtiHttpConnector::new(client, tls_conn); let http = hyper_tor::Client::builder().build::<_, hyper_tor::Body>(conn); // Create request. @@ -366,12 +374,19 @@ impl Tor { body: Option, headers: Vec<(String, String)>, ) -> Option { - if Self::client_config().is_none() { + // Bind once to avoid a TOCTOU panic if Tor restarts mid-request. + let Some((client_cfg, _)) = Self::client_config() else { error!("Tor: client not launched"); return None; - } - let client = Self::client_config().unwrap().0.isolated_client(); - let tls_conn = TlsConnector::builder().unwrap().build().unwrap(); + }; + let client = client_cfg.isolated_client(); + let tls_conn = match TlsConnector::builder().and_then(|b| b.build()) { + Ok(c) => c, + Err(e) => { + error!("Tor: TLS connector build failed: {:?}", e); + return None; + } + }; let conn = ArtiHttpConnector::new(client, tls_conn); let http = hyper_tor::Client::builder().build::<_, hyper_tor::Body>(conn); let mut builder = hyper_tor::Request::builder() diff --git a/src/wallet/wallet.rs b/src/wallet/wallet.rs index ba4bf70e..dcb5f65f 100644 --- a/src/wallet/wallet.rs +++ b/src/wallet/wallet.rs @@ -334,7 +334,9 @@ impl Wallet { return Err(Error::GenericError("Already opened".to_string())); } // Keep the password for nostr identity setup after opening. - let nostr_password = password.to_string(); + // Keep a zeroized copy of the password for nostr identity setup; the + // original is moved into open_wallet below. + let nostr_password = password.clone(); // Create new wallet instance if sync thread was stopped or instance was not created. let has_instance = { @@ -409,7 +411,8 @@ impl Wallet { /// Initialize the nostr identity and service for this wallet. /// Failures are logged and disable nostr for the session only. - fn init_nostr(&self, password: &str) { + /// The password is held as a [`ZeroingString`] so it is scrubbed on drop. + fn init_nostr(&self, password: &ZeroingString) { { let r_nostr = self.nostr.read(); if r_nostr.is_some() { @@ -2146,11 +2149,12 @@ async fn handle_task(w: &Wallet, t: WalletTask) { .await { Ok(event_id) => { - let mut meta = service.store.tx_meta(&s.id.to_string()).unwrap(); - meta.status = crate::nostr::NostrSendStatus::AwaitingS2; - meta.sent_event_id = Some(event_id); - meta.updated_at = crate::nostr::unix_time(); - service.store.save_tx_meta(&meta); + if let Some(mut meta) = service.store.tx_meta(&s.id.to_string()) { + meta.status = crate::nostr::NostrSendStatus::AwaitingS2; + meta.sent_event_id = Some(event_id); + meta.updated_at = crate::nostr::unix_time(); + service.store.save_tx_meta(&meta); + } // Update contact last paid time. if let Some(mut contact) = service.store.contact(receiver) { contact.last_paid_at = Some(crate::nostr::unix_time()); @@ -2249,12 +2253,14 @@ async fn handle_task(w: &Wallet, t: WalletTask) { }); match service.send_payment_dm(&request.npub, &text, None).await { Ok(event_id) => { - let mut meta = - service.store.tx_meta(&reply.id.to_string()).unwrap(); - meta.status = crate::nostr::NostrSendStatus::PaidAwaitingFinalize; - meta.sent_event_id = Some(event_id); - meta.updated_at = crate::nostr::unix_time(); - service.store.save_tx_meta(&meta); + if let Some(mut meta) = service.store.tx_meta(&reply.id.to_string()) + { + meta.status = + crate::nostr::NostrSendStatus::PaidAwaitingFinalize; + meta.sent_event_id = Some(event_id); + meta.updated_at = crate::nostr::unix_time(); + service.store.save_tx_meta(&meta); + } } Err(e) => error!("nostr pay reply dispatch failed: {e}"), } @@ -2286,12 +2292,14 @@ async fn handle_task(w: &Wallet, t: WalletTask) { }); match service.send_payment_dm(&request.npub, &text, None).await { Ok(event_id) => { - let mut meta = - service.store.tx_meta(&reply.id.to_string()).unwrap(); - meta.status = crate::nostr::NostrSendStatus::RepliedS2; - meta.sent_event_id = Some(event_id); - meta.updated_at = crate::nostr::unix_time(); - service.store.save_tx_meta(&meta); + if let Some(mut meta) = + service.store.tx_meta(&reply.id.to_string()) + { + meta.status = crate::nostr::NostrSendStatus::RepliedS2; + meta.sent_event_id = Some(event_id); + meta.updated_at = crate::nostr::unix_time(); + service.store.save_tx_meta(&meta); + } } Err(e) => error!("nostr accept reply dispatch failed: {e}"), } diff --git a/tests/replay_check.rs b/tests/replay_check.rs new file mode 100644 index 00000000..f1332d5c --- /dev/null +++ b/tests/replay_check.rs @@ -0,0 +1,105 @@ +use base64::Engine; +use nostr_sdk::prelude::*; +use sha2::{Digest, Sha256}; +use std::process::Command; + +#[tokio::test] +#[ignore] +async fn replay_and_double_name_rejected() { + let keys = Keys::generate(); + let pk = keys.public_key().to_hex(); + let server = "https://goblin.st"; + + // Build a register POST for name A. + let name_a = format!("r{}", &pk[..7]); + let url = format!("{server}/api/v1/register"); + let body = serde_json::json!({"name": name_a, "pubkey": pk}).to_string(); + let ph = hex::encode(Sha256::digest(body.as_bytes())); + let ev = EventBuilder::new(Kind::HttpAuth, "") + .tag(Tag::custom(TagKind::custom("u"), [url.clone()])) + .tag(Tag::custom(TagKind::custom("method"), ["POST".to_string()])) + .tag(Tag::custom(TagKind::custom("payload"), [ph])) + .sign_with_keys(&keys) + .unwrap(); + let auth = format!( + "Nostr {}", + base64::engine::general_purpose::STANDARD.encode(ev.as_json()) + ); + let post = |a: &str, b: &str| { + String::from_utf8_lossy( + &Command::new("curl") + .args([ + "-s", + "-X", + "POST", + &url, + "-H", + &format!("Authorization: {a}"), + "-H", + "Content-Type: application/json", + "-d", + b, + ]) + .output() + .unwrap() + .stdout, + ) + .to_string() + }; + let r1 = post(&auth, &body); + let r2 = post(&auth, &body); // exact replay (same auth event id) + println!("first: {r1}"); + println!("replay: {r2}"); + assert!(r1.contains("nip05"), "first register should succeed"); + assert!( + r2.contains("replayed"), + "replay should be rejected, got: {r2}" + ); + + // Second DISTINCT name with a FRESH signature but same pubkey -> one-name rule. + let name_b = format!("s{}", &pk[..7]); + let body_b = serde_json::json!({"name": name_b, "pubkey": pk}).to_string(); + let ph_b = hex::encode(Sha256::digest(body_b.as_bytes())); + let ev_b = EventBuilder::new(Kind::HttpAuth, "") + .tag(Tag::custom(TagKind::custom("u"), [url.clone()])) + .tag(Tag::custom(TagKind::custom("method"), ["POST".to_string()])) + .tag(Tag::custom(TagKind::custom("payload"), [ph_b])) + .sign_with_keys(&keys) + .unwrap(); + let auth_b = format!( + "Nostr {}", + base64::engine::general_purpose::STANDARD.encode(ev_b.as_json()) + ); + let r3 = post(&auth_b, &body_b); + println!("2nd name: {r3}"); + assert!( + r3.contains("already has a name") || r3.contains("pubkey already"), + "one-name rule should block, got: {r3}" + ); + + // Cleanup name A. + let del_url = format!("{server}/api/v1/register/{name_a}"); + let ev_d = EventBuilder::new(Kind::HttpAuth, "") + .tag(Tag::custom(TagKind::custom("u"), [del_url.clone()])) + .tag(Tag::custom( + TagKind::custom("method"), + ["DELETE".to_string()], + )) + .sign_with_keys(&keys) + .unwrap(); + let auth_d = format!( + "Nostr {}", + base64::engine::general_purpose::STANDARD.encode(ev_d.as_json()) + ); + let _ = Command::new("curl") + .args([ + "-s", + "-X", + "DELETE", + &del_url, + "-H", + &format!("Authorization: {auth_d}"), + ]) + .output(); + println!("✓ replay + one-name-per-pubkey enforced"); +}