From 2ec542f04db5182301a971fb5ae49f90ccd754d0 Mon Sep 17 00:00:00 2001 From: Benjamen Keroack Date: Wed, 7 Oct 2026 16:06:47 -0600 Subject: [PATCH 1/2] tor: read the SAFECOOKIE cookie file with a size bound, regular files only The control port names the cookie file in its PROTOCOLINFO reply, and satd read it with fs::read, to end of file and without checking what it was. Bitcoin Core reads it with ReadBinaryFile(cookiefile, TOR_COOKIE_SIZE) (src/torcontrol.cpp:616), which never reads more than 32 bytes. satd now opens the file non-blocking, refuses anything that is not a regular file, and reads at most 33 bytes, so /dev/zero or a FIFO named as the cookie fails authentication at once instead of filling memory or blocking startup in open(2). A file longer than 32 bytes is still refused, as before. Co-Authored-By: Claude Opus 5.5 (1M context) --- node/src/net/tor.rs | 244 ++++++++++++++++++++++++++++++++++++++++++-- 1 file changed, 233 insertions(+), 11 deletions(-) diff --git a/node/src/net/tor.rs b/node/src/net/tor.rs index 27e0b2f15..28f57ed67 100644 --- a/node/src/net/tor.rs +++ b/node/src/net/tor.rs @@ -148,20 +148,21 @@ impl TorController { // control port is trusted" model (same as Bitcoin Core): the bytes are // only fed into the HMAC, never sent or logged, and the SERVERHASH check // below fails unless the port already knows the file — i.e. it is the - // real Tor. No confidentiality leak results. - let cookie = std::fs::read(cookie_path).map_err(|e| { - format!( - "Tor SAFECOOKIE: cannot read cookie file {}: {} (run satd as a user that can read \ - it, or set CookieAuthFileGroupReadable 1 in torrc)", - cookie_path, e - ) - })?; + // real Tor. No confidentiality leak results. The read itself is + // bounded (see `read_cookie_file`), so a path such as /dev/zero or a + // FIFO cannot exhaust memory or stall startup. + let cookie = read_cookie_file(cookie_path)?; if cookie.len() != COOKIE_LEN { + // At most COOKIE_LEN + 1 bytes were read, so a longer read means + // "longer than the cookie", not its exact size. + let size = if cookie.len() > COOKIE_LEN { + format!("more than {COOKIE_LEN}") + } else { + cookie.len().to_string() + }; return Err(format!( "Tor SAFECOOKIE: cookie file {} is {} bytes, expected {}", - cookie_path, - cookie.len(), - COOKIE_LEN + cookie_path, size, COOKIE_LEN )); } @@ -357,6 +358,51 @@ impl TorController { } } +/// Open the SAFECOOKIE cookie file the control port named and read at most +/// `COOKIE_LEN + 1` bytes of it. +/// +/// Bitcoin Core reads the cookie with `ReadBinaryFile(cookiefile, +/// TOR_COOKIE_SIZE)` (`torcontrol.cpp`), which never reads more than 32 bytes. +/// The path comes from the control port, so nothing guarantees it names a +/// small regular file: a read of `/dev/zero` to end of file never ends, and +/// opening a FIFO for reading waits for a writer. The file is opened +/// non-blocking and must be a regular file. One byte past the cookie length is +/// read so that a longer file is refused by the caller's length check instead +/// of being truncated to a cookie. +fn read_cookie_file(cookie_path: &str) -> Result, String> { + let unreadable = |e: std::io::Error| { + format!( + "Tor SAFECOOKIE: cannot read cookie file {}: {} (run satd as a user that can read \ + it, or set CookieAuthFileGroupReadable 1 in torrc)", + cookie_path, e + ) + }; + let mut options = std::fs::OpenOptions::new(); + options.read(true); + #[cfg(unix)] + { + use std::os::unix::fs::OpenOptionsExt; + options.custom_flags(libc::O_NONBLOCK); + } + let file = options.open(cookie_path).map_err(unreadable)?; + let metadata = file.metadata().map_err(unreadable)?; + if !metadata.is_file() { + return Err(format!( + "Tor SAFECOOKIE: cookie file {} is not a regular file", + cookie_path + )); + } + read_cookie_bytes(file).map_err(unreadable) +} + +/// Read at most `COOKIE_LEN + 1` bytes from `reader`. +fn read_cookie_bytes(reader: impl std::io::Read) -> std::io::Result> { + use std::io::Read as _; + let mut cookie = Vec::with_capacity(COOKIE_LEN + 1); + reader.take(COOKIE_LEN as u64 + 1).read_to_end(&mut cookie)?; + Ok(cookie) +} + /// HMAC-SHA256(key, msg). fn hmac(key: &[u8], msg: &[u8]) -> Vec { let mut mac = HmacSha256::new_from_slice(key).expect("HMAC accepts any key length"); @@ -584,4 +630,180 @@ mod tests { "client leaked a proof to an unverified server" ); } + + /// A control port that offers SAFECOOKIE and names `cookie_path` as the + /// cookie file, then waits for the client's next line. Returns that line + /// (empty if the client hung up without sending one). + async fn fake_tor_naming_cookie(listener: TcpListener, cookie_path: String) -> String { + let (stream, _) = listener.accept().await.unwrap(); + let (rh, mut wh) = tokio::io::split(stream); + let mut reader = BufReader::new(rh); + let mut l = String::new(); + reader.read_line(&mut l).await.unwrap(); + assert!(l.starts_with("PROTOCOLINFO"), "got: {l}"); + wh.write_all( + format!( + "250-PROTOCOLINFO 1\r\n250-AUTH METHODS=SAFECOOKIE COOKIEFILE=\"{cookie_path}\"\r\n250 OK\r\n" + ) + .as_bytes(), + ) + .await + .unwrap(); + let mut next = String::new(); + let _ = reader.read_line(&mut next).await; + next + } + + /// Connect to a fake control port naming `cookie_path` and authenticate, + /// in a task of its own so the caller can time it out. + fn spawn_authenticate(addr: String) -> tokio::task::JoinHandle> { + tokio::spawn(async move { + let mut ctrl = TorController::connect(&addr).await?; + ctrl.authenticate(None).await + }) + } + + #[cfg(unix)] + #[test] + fn safecookie_refuses_a_fifo_cookie_path() { + // Opening a FIFO for reading waits for a writer. The cookie path comes + // from the control port, so naming a FIFO must not stall startup: the + // file has to be refused as not a regular file, promptly. + let dir = tempfile::tempdir().unwrap(); + let fifo = dir.path().join("cookie.fifo"); + let c_path = std::ffi::CString::new(fifo.to_str().unwrap()).unwrap(); + // SAFETY: `c_path` is a valid NUL-terminated path for the call. + assert_eq!(unsafe { libc::mkfifo(c_path.as_ptr(), 0o600) }, 0, "mkfifo"); + + // The client runs on a runtime of its own in its own thread: if it + // blocks in open(2), this thread's timeout still fires. + let cookie_path = fifo.to_str().unwrap().to_string(); + let (tx, rx) = std::sync::mpsc::channel(); + let client = std::thread::spawn(move || { + let rt = tokio::runtime::Builder::new_current_thread() + .enable_all() + .build() + .unwrap(); + let outcome = rt.block_on(async move { + let listener = TcpListener::bind("127.0.0.1:0").await.unwrap(); + let addr = listener.local_addr().unwrap().to_string(); + let server = tokio::spawn(fake_tor_naming_cookie(listener, cookie_path)); + let auth = async { + let mut ctrl = TorController::connect(&addr).await?; + ctrl.authenticate(None).await + } + .await; + (auth, server.await.unwrap()) + }); + let _ = tx.send(outcome); + }); + + match rx.recv_timeout(std::time::Duration::from_secs(10)) { + Ok((auth, next)) => { + client.join().unwrap(); + let err = auth.unwrap_err(); + assert!(err.contains("not a regular file"), "got: {err}"); + assert!( + !next.starts_with("AUTHCHALLENGE"), + "client went on to AUTHCHALLENGE with a refused cookie: {next}" + ); + } + Err(_) => { + // Release the reader blocked in open(2) so its thread ends, + // then fail. O_NONBLOCK: never wait here for a reader. + use std::os::unix::fs::OpenOptionsExt; + drop( + std::fs::OpenOptions::new() + .write(true) + .custom_flags(libc::O_NONBLOCK) + .open(&fifo), + ); + let _ = client.join(); + panic!("authenticate blocked on a FIFO named as the Tor cookie file"); + } + } + } + + #[cfg(unix)] + #[tokio::test] + async fn safecookie_refuses_a_device_cookie_path() { + // A character device such as /dev/zero never reaches end of file, so a + // read to EOF would not end. It is not a regular file, so it is refused + // before any read. + let listener = TcpListener::bind("127.0.0.1:0").await.unwrap(); + let addr = listener.local_addr().unwrap().to_string(); + let server = tokio::spawn(fake_tor_naming_cookie(listener, "/dev/zero".to_string())); + let auth = spawn_authenticate(addr); + let err = tokio::time::timeout(std::time::Duration::from_secs(10), auth) + .await + .expect("authenticate did not return for a /dev/zero cookie path") + .unwrap() + .unwrap_err(); + assert!(err.contains("not a regular file"), "got: {err}"); + let _ = tokio::time::timeout(std::time::Duration::from_secs(10), server).await; + } + + #[tokio::test] + async fn safecookie_refuses_a_cookie_file_longer_than_32_bytes() { + // Tor writes exactly 32 bytes. A longer file is refused rather than + // truncated to its first 32 bytes. + let dir = tempfile::tempdir().unwrap(); + let path = dir.path().join("control.authcookie"); + std::fs::write(&path, [7u8; 64]).unwrap(); + + let listener = TcpListener::bind("127.0.0.1:0").await.unwrap(); + let addr = listener.local_addr().unwrap().to_string(); + let server = tokio::spawn(fake_tor_naming_cookie( + listener, + path.to_str().unwrap().to_string(), + )); + let err = tokio::time::timeout(std::time::Duration::from_secs(10), spawn_authenticate(addr)) + .await + .expect("authenticate timed out") + .unwrap() + .unwrap_err(); + assert!(err.contains("is more than 32 bytes"), "got: {err}"); + let _ = tokio::time::timeout(std::time::Duration::from_secs(10), server).await; + } + + /// A reader with `len` bytes available that counts how many it served. + struct CountingReader { + remaining: usize, + served: std::rc::Rc>, + } + + impl std::io::Read for CountingReader { + fn read(&mut self, buf: &mut [u8]) -> std::io::Result { + let n = buf.len().min(self.remaining); + buf[..n].fill(0xab); + self.remaining -= n; + self.served.set(self.served.get() + n); + Ok(n) + } + } + + #[test] + fn cookie_read_stops_one_byte_past_the_cookie_length() { + let served = std::rc::Rc::new(std::cell::Cell::new(0)); + let reader = CountingReader { + remaining: 1 << 20, + served: served.clone(), + }; + let cookie = read_cookie_bytes(reader).unwrap(); + assert_eq!(cookie.len(), COOKIE_LEN + 1); + assert!( + served.get() <= COOKIE_LEN + 1, + "read {} bytes from a 1 MiB source; the cookie read must stop at {}", + served.get(), + COOKIE_LEN + 1 + ); + + // An exact 32-byte source reads as the cookie. + let served = std::rc::Rc::new(std::cell::Cell::new(0)); + let reader = CountingReader { + remaining: COOKIE_LEN, + served, + }; + assert_eq!(read_cookie_bytes(reader).unwrap().len(), COOKIE_LEN); + } } From 90afa399c524e8b3149ec272e78338212c3c2112 Mon Sep 17 00:00:00 2001 From: Benjamen Keroack Date: Wed, 7 Oct 2026 17:08:46 -0600 Subject: [PATCH 2/2] Read bitcoin.conf comments as Core does; keep request dumps and secrets out of logs bitcoin.conf: satd skipped only lines that start with `#`, so a comment after a value became part of it. `txindex=1 # c` left txindex off, `rpcport=8332 # c` fell back to the default port, and `rpcpassword=pw # c` set the password to the whole string, all without a warning. Bitcoin Core's GetConfigOptions (src/common/config.cpp:41-62) drops everything from the first `#`, trims " \t\r\n", and refuses an rpcpassword line holding a `#` ("using # in rpcpassword can be ambiguous and should be avoided"). satd's parser and sat-cli's now do the same, with the same message. Trace logging: under -loglevel=trace (or RUST_LOG=trace) jsonrpsee-server logged every HTTP request with its headers, the Authorization header included, before satd's auth layer ran (jsonrpsee-server 0.26 src/server.rs:1026), and jsonrpsee-core logs call parameters; tungstenite logs WebSocket payloads. Core logs no credentials or parameters at any level. A global filter layer now drops TRACE output from the jsonrpsee and tungstenite targets whatever the reloadable filter allows; their DEBUG output is unchanged. Debug: Config, RpcAuthEntry, the reorg webhook target and the satd-auth operator credentials (behind RpcAuth) derived Debug over their secrets. Their Debug output now names them without the password, cookie token, rpcauth salt and tag, or webhook secret. Nothing printed them today. Co-Authored-By: Claude Opus 5.5 (1M context) --- docs/manual/src/config-reference.md | 7 +- docs/manual/src/configuration.md | 4 +- sat-cli/src/conf.rs | 74 ++++++-- sat-cli/src/main.rs | 2 +- satd-auth/src/operator.rs | 63 ++++++- satd/src/config.rs | 99 ++++++++++- satd/src/config_localharden_tests.rs | 244 +++++++++++++++++++++++++++ satd/src/main.rs | 7 +- satd/src/reload.rs | 12 +- satd/tests/local_hardening.rs | 55 ++++++ 10 files changed, 536 insertions(+), 31 deletions(-) create mode 100644 satd/src/config_localharden_tests.rs create mode 100644 satd/tests/local_hardening.rs diff --git a/docs/manual/src/config-reference.md b/docs/manual/src/config-reference.md index a01521beb..863266362 100644 --- a/docs/manual/src/config-reference.md +++ b/docs/manual/src/config-reference.md @@ -23,6 +23,11 @@ track Bitcoin Core v30. - **Resolution order.** `-conf=` if given, else `/bitcoin.conf`, else `/satd.conf`. Flags override file values. +- **Comments.** As in Core, `#` starts a comment anywhere on a line: + `txindex=1 # keep it` sets `txindex` to `1`. What is left of the line is + trimmed of spaces, tabs and line endings. An `rpcpassword` line that + contains a `#` is refused at load (`using # in rpcpassword can be ambiguous + and should be avoided`), as Core refuses it. - **Key disposition.** Each config-file key gets one of four treatments: 1. **Honored.** satd implements it. This is the common case. 2. **Skipped with a warning.** A recognized Core v30 option satd does not @@ -120,7 +125,7 @@ startup error. | `logsourcelocations` | off | restart | core | Prepend source `file:line` to each log line. | | `debug` | none | hot | core | Enable debug logging for a category (repeatable; bare/`all`/`1` = everything). satd adds `stratum`, the Stratum server's per-miner lines (see [Verifying a miner](stratum.md#verifying-a-miner)). | | `debugexclude` | none | hot | core | Disable debug logging for a category `debug` would otherwise enable. | -| `loglevel` | info | hot | core | Global verbosity (`trace`/`debug`/`info`/`warn`/`error`) or a per-category override (`net:debug`). Maps onto satd's `tracing` filter: a bare level sets the default for targets without an override, and does not lower a more specific `-debug`/`RUST_LOG` directive (`-debug=net -loglevel=error` still logs `net` at debug). A `category:level` pair overrides that subsystem. | +| `loglevel` | info | hot | core | Global verbosity (`trace`/`debug`/`info`/`warn`/`error`) or a per-category override (`net:debug`). Maps onto satd's `tracing` filter: a bare level sets the default for targets without an override, and does not lower a more specific `-debug`/`RUST_LOG` directive (`-debug=net -loglevel=error` still logs `net` at debug). A `category:level` pair overrides that subsystem. The JSON-RPC and WebSocket libraries' TRACE output (`jsonrpsee*`, `tungstenite*` targets), which prints request headers and call parameters, is never logged, whatever the level; their DEBUG output is. | | `allowignoredconf` | off | restart | core | Suppress startup warnings about `includeconf` files satd had to ignore. | | `maxshutdownsecs` | 30 | hot | satd | Graceful-shutdown budget (seconds), shared by the `shutdownnotify` hook (at most half of it), the wait for the block connector to stop (at most half of what is left) and the final flush. A connector still in a block at the end of its share costs the clean-shutdown marker but not the flush; a flush that overruns the budget force-exits without the marker. | diff --git a/docs/manual/src/configuration.md b/docs/manual/src/configuration.md index 4739a0cc7..7bd0311be 100644 --- a/docs/manual/src/configuration.md +++ b/docs/manual/src/configuration.md @@ -94,7 +94,9 @@ sat-cli -datadir=/path/to/datadir getblockcount `rpccookiefile` come from the command line first, then the active chain's section (`[main]`, `[test]`, `[testnet4]`, `[signet]`, `[regtest]`), then the top level of the file; the first value in a scope wins. A top-level - `rpcport` applies on every chain, as it does for satd itself. + `rpcport` applies on every chain, as it does for satd itself. The file is + read with satd's rules: `#` starts a comment anywhere on a line, and an + `rpcpassword` line holding a `#` is an error. - **Which credentials.** A non-empty `rpcpassword` from either source means user/password authentication, with `rpcuser` looked up the same way; otherwise the cookie file, whose relative path is under the chain's data diff --git a/sat-cli/src/conf.rs b/sat-cli/src/conf.rs index e5d5111d8..f5b6fba69 100644 --- a/sat-cli/src/conf.rs +++ b/sat-cli/src/conf.rs @@ -103,14 +103,21 @@ pub struct ConfFile { } impl ConfFile { - pub fn parse(content: &str) -> Self { + /// Parse the file as bitcoin-cli does, with Bitcoin Core's + /// GetConfigOptions (src/common/config.cpp): a `#` anywhere on a line + /// starts a comment, what is left is trimmed of spaces, tabs, CRs and + /// LFs, and an `rpcpassword` line holding a `#` is an error. + pub fn parse(content: &str) -> Result { + let trim = |s: &str| s.trim_matches([' ', '\t', '\r', '\n']).to_string(); let mut file = ConfFile::default(); let mut section: Option = None; - for line in content.lines() { - // Whole-line comments only, as the daemon parses them: a `#` - // later on a line is part of the value (a password may hold one). - let line = line.trim(); - if line.is_empty() || line.starts_with('#') { + for (idx, line) in content.lines().enumerate() { + let (line, used_hash) = match line.find('#') { + Some(pos) => (&line[..pos], true), + None => (line, false), + }; + let line = trim(line); + if line.is_empty() { continue; } if line.starts_with('[') && line.ends_with(']') { @@ -118,25 +125,34 @@ impl ConfFile { continue; } let (key, value) = match line.split_once('=') { - Some((k, v)) => (k.trim(), v.trim()), - None => (line, "1"), + Some((k, v)) => (trim(k), trim(v)), + None => (line.clone(), "1".to_string()), }; + if used_hash && key.contains("rpcpassword") { + return Err(format!( + "parse error on line {}, using # in rpcpassword can be ambiguous and \ + should be avoided", + idx + 1 + )); + } let map = match §ion { Some(s) => file.sections.entry(s.clone()).or_default(), None => &mut file.global, }; - map.entry(key.to_string()).or_default().push(value.to_string()); + map.entry(key).or_default().push(value); } - file + Ok(file) } /// Read `path`. A missing file is not an error (`Ok(None)`); an - /// unreadable one is, when the caller named it with `-conf`. - pub fn read(path: &Path) -> std::io::Result> { + /// unreadable or unparseable one is. + pub fn read(path: &Path) -> Result, String> { match std::fs::read_to_string(path) { - Ok(s) => Ok(Some(Self::parse(&s))), + Ok(s) => Self::parse(&s).map(Some).map_err(|e| { + format!("Error reading configuration file {}: {e}", path.display()) + }), Err(e) if e.kind() == std::io::ErrorKind::NotFound => Ok(None), - Err(e) => Err(e), + Err(e) => Err(format!("cannot read {}: {e}", path.display())), } } @@ -344,7 +360,7 @@ mod tests { use super::*; fn resolve_str(cli: &CliConn, body: &str) -> Result { - let conf = ConfFile::parse(body); + let conf = ConfFile::parse(body)?; resolve(cli, Some(&conf), Path::new("/d"), PathBuf::from("/d/bitcoin.conf")) } @@ -489,10 +505,34 @@ mod tests { #[test] fn comments_and_bare_keys() { - let body = "# a comment\nregtest\nrpcport=9\nrpcpassword=a#b\n"; + let body = "# a comment\nregtest\nrpcport=9\nrpcpassword=ab\n"; let c = resolve_str(&CliConn::default(), body).unwrap(); assert_eq!((c.chain, c.port), (Chain::Regtest, 9)); - assert_eq!(c.creds, Creds::UserPass(String::new(), "a#b".into())); + assert_eq!(c.creds, Creds::UserPass(String::new(), "ab".into())); + } + + /// bitcoin-cli reads the file with Bitcoin Core's GetConfigOptions + /// (src/common/config.cpp): a `#` anywhere starts a comment, and the rest + /// is trimmed of spaces, tabs, CRs and LFs. + #[test] + fn an_inline_comment_is_not_part_of_the_value() { + let body = "regtest=1 # lab node\n[regtest] # this chain\nrpcport=19000\t# port\n\ + rpcuser=u #user\nrpcpassword=p\n"; + let c = resolve_str(&CliConn::default(), body).unwrap(); + assert_eq!((c.chain, c.port), (Chain::Regtest, 19000)); + assert_eq!(c.creds, Creds::UserPass("u".into(), "p".into())); + } + + /// Core refuses an `rpcpassword` line holding a `#`, as the daemon does. + #[test] + fn a_hash_on_an_rpcpassword_line_is_refused() { + for body in ["rpcpassword=a#b\n", "rpcuser=u\n[main]\nrpcpassword=p # old\n"] { + let err = resolve_str(&CliConn::default(), body).unwrap_err(); + assert!( + err.contains("using # in rpcpassword can be ambiguous and should be avoided"), + "{body:?}: {err}" + ); + } } #[test] diff --git a/sat-cli/src/main.rs b/sat-cli/src/main.rs index 03c38844c..480f45e7a 100644 --- a/sat-cli/src/main.rs +++ b/sat-cli/src/main.rs @@ -818,7 +818,7 @@ async fn main() { } Ok(None) => None, Err(e) => { - eprintln!("error: cannot read {}: {e}", conf_path.display()); + eprintln!("error: {e}"); std::process::exit(1); } }; diff --git a/satd-auth/src/operator.rs b/satd-auth/src/operator.rs index 36164a326..584b3e3e3 100644 --- a/satd-auth/src/operator.rs +++ b/satd-auth/src/operator.rs @@ -24,7 +24,7 @@ fn ct_eq(a: &[u8], b: &[u8]) -> bool { } /// The auto-generated cookie credential (`__cookie__:`). -#[derive(Debug, Clone)] +#[derive(Clone)] pub struct CookieCredential { /// Where the cookie file lives (for cleanup on shutdown). pub path: PathBuf, @@ -34,7 +34,7 @@ pub struct CookieCredential { } /// A `-rpcuser`/`-rpcpassword` pair. -#[derive(Debug, Clone)] +#[derive(Clone)] pub struct UserPassCredential { /// Username. pub username: String, @@ -45,7 +45,7 @@ pub struct UserPassCredential { /// One parsed Bitcoin-Core `-rpcauth=user:salt$hash` entry. `salt`'s ASCII bytes /// are the HMAC key (Core's `rpcauth.py` does `salt.encode('utf-8')`); `hash` is /// the expected 32-byte tag. -#[derive(Debug, Clone)] +#[derive(Clone)] pub struct RpcAuthCredential { /// Username. pub username: String, @@ -55,6 +55,34 @@ pub struct RpcAuthCredential { pub hash: Vec, } +// The credentials' `Debug` output names each credential but never shows its +// secret: the cookie token, the password, or the rpcauth salt and tag (which +// together allow offline password guessing). `OperatorCreds` and the +// `RpcAuth` handle that holds it derive `Debug` over these. +impl std::fmt::Debug for CookieCredential { + fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { + f.debug_struct("CookieCredential") + .field("path", &self.path) + .finish_non_exhaustive() + } +} + +impl std::fmt::Debug for UserPassCredential { + fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { + f.debug_struct("UserPassCredential") + .field("username", &self.username) + .finish_non_exhaustive() + } +} + +impl std::fmt::Debug for RpcAuthCredential { + fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { + f.debug_struct("RpcAuthCredential") + .field("username", &self.username) + .finish_non_exhaustive() + } +} + /// Zero or more operator credentials of the three Core-compatible kinds. A /// request authenticates the operator if **any** credential matches the /// presented `user:pass` (Core's behaviour: cookie, `-rpcuser`/`-rpcpassword`, @@ -225,4 +253,33 @@ mod tests { assert_eq!(c.matching_user("alice", "tok"), None); assert_eq!(c.matching_user("bob", "secret"), None); } + + #[test] + fn debug_output_names_credentials_without_their_secrets() { + let hash = vec![0xa5u8; 32]; + let c = OperatorCreds { + cookie: Some(CookieCredential { + path: PathBuf::from("/tmp/x.cookie"), + token: "CookieTokenMarker".into(), + }), + userpass: vec![UserPassCredential { + username: "alice".into(), + password: "PasswordMarker".into(), + }], + rpcauth: vec![RpcAuthCredential { + username: "bob".into(), + salt: "SaltMarker".into(), + hash: hash.clone(), + }], + }; + for shown in [format!("{c:?}"), format!("{c:#?}")] { + for secret in ["CookieTokenMarker", "PasswordMarker", &format!("{hash:?}")] { + assert!(!shown.contains(secret), "Debug shows {secret}: {shown}"); + } + // What identifies a credential is still there. + for name in ["x.cookie", "alice", "bob"] { + assert!(shown.contains(name), "Debug lost {name}: {shown}"); + } + } + } } diff --git a/satd/src/config.rs b/satd/src/config.rs index ff36d9e19..c093fb278 100644 --- a/satd/src/config.rs +++ b/satd/src/config.rs @@ -292,7 +292,7 @@ impl std::str::FromStr for EsploraAuthMode { /// don't allow `:` in the username because Core's rpcauth.py forbids it /// too (Basic-auth header is `user:pass`, so a `:` in the user breaks /// the decode). -#[derive(Debug, Clone)] +#[derive(Clone)] pub struct RpcAuthEntry { pub username: String, /// The salt EXACTLY as written in the config line. Core's @@ -305,6 +305,16 @@ pub struct RpcAuthEntry { pub hash: Vec, } +/// The salt and the tag together let anyone holding them test password +/// guesses offline, so `Debug` shows the user only. +impl std::fmt::Debug for RpcAuthEntry { + fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { + f.debug_struct("RpcAuthEntry") + .field("username", &self.username) + .finish_non_exhaustive() + } +} + impl RpcAuthEntry { pub fn parse(s: &str) -> Result { let raw = s.trim(); @@ -408,7 +418,10 @@ pub struct ProfileDefaults { } /// Resolved node configuration after merging CLI args, config file, and defaults. -#[derive(Debug, Clone)] +/// +/// `Debug` is written by hand (see the impl below): the struct holds RPC, Tor, +/// Esplora and webhook secrets. +#[derive(Clone)] pub struct Config { pub network: Network, pub datadir: PathBuf, @@ -1425,6 +1438,19 @@ pub struct Config { pub pending_notes: Vec, } +/// `Config` holds `rpcpassword`, `rpcauth`, `torpassword`, `esplorauserpass` +/// and `reorgwebhooksecret`, so a derived `Debug` would print them wherever a +/// config reached a `{:?}` or a panic message. This one names the network +/// and the data directory and nothing else. +impl std::fmt::Debug for Config { + fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { + f.debug_struct("Config") + .field("network", &self.network) + .field("datadir", &self.datadir) + .finish_non_exhaustive() + } +} + /// A deferred operator-facing note emitted during config load. Its /// `level` selects the tracing macro in `main.rs`. #[derive(Debug, Clone)] @@ -8077,6 +8103,13 @@ pub fn filter_unsupported_core_cli_args( Ok((kept, warnings)) } +/// Trim a `bitcoin.conf` line, key or value the way Bitcoin Core does +/// (`TrimString(str, " \t\r\n")` in src/common/config.cpp): spaces, tabs, +/// CRs and LFs only, not every Unicode whitespace character. +fn trim_conf_whitespace(s: &str) -> &str { + s.trim_matches([' ', '\t', '\r', '\n']) +} + /// Parsed bitcoin.conf file. #[derive(Debug, Default)] pub struct ConfigFile { @@ -8106,8 +8139,15 @@ impl ConfigFile { for (idx, line) in content.lines().enumerate() { let line_no = idx + 1; - let trimmed = line.trim(); - if trimmed.is_empty() || trimmed.starts_with('#') { + // Bitcoin Core's GetConfigOptions (src/common/config.cpp): a `#` + // anywhere on a line starts a comment, and what is left is + // trimmed of spaces, tabs, CRs and LFs. + let (line, used_hash) = match line.find('#') { + Some(pos) => (&line[..pos], true), + None => (line, false), + }; + let trimmed = trim_conf_whitespace(line); + if trimmed.is_empty() { continue; } @@ -8120,13 +8160,23 @@ impl ConfigFile { // Key=value or bare key let (key, value) = if let Some(eq_pos) = trimmed.find('=') { - let k = trimmed[..eq_pos].trim().to_string(); - let v = trimmed[eq_pos + 1..].trim().to_string(); + let k = trim_conf_whitespace(&trimmed[..eq_pos]).to_string(); + let v = trim_conf_whitespace(&trimmed[eq_pos + 1..]).to_string(); (k, v) } else { (trimmed.to_string(), "1".to_string()) }; + // Core refuses a `#` on an rpcpassword line rather than guess + // whether it starts a comment or belongs to the password. Its + // check is a substring match on the key, as here. + if used_hash && key.contains("rpcpassword") { + return Err(format!( + "Error reading configuration file: parse error on line {line_no}, using # \ + in rpcpassword can be ambiguous and should be avoided" + )); + } + // Accept Bitcoin Core's hyphenated `reindex-chainstate` config // spelling — Core's own conf-file key is hyphenated, so a // migrated bitcoin.conf carries this form. satd's canonical key @@ -8899,6 +8949,39 @@ pub fn build_env_filter(config: &Config) -> tracing_subscriber::EnvFilter { env_filter } +/// Target prefixes of dependency crates whose TRACE output prints raw +/// requests. jsonrpsee-server logs each HTTP request with its headers, +/// `Authorization` included, before satd's auth layer has run +/// (jsonrpsee-server 0.26 `src/server.rs:1026`), and jsonrpsee-core logs call +/// parameters, raw request bodies and responses (`src/server/rpc_module.rs:310, +/// 356, 409`). tungstenite logs every WebSocket frame with its payload +/// (`src/protocol/frame/mod.rs:228, 257`). Bitcoin Core logs no credentials +/// and no RPC parameters at any level. +const REQUEST_DUMP_TARGET_PREFIXES: [&str; 2] = ["jsonrpsee", "tungstenite"]; + +/// Whether `meta` is TRACE output from one of [`REQUEST_DUMP_TARGET_PREFIXES`]. +fn is_request_dump(meta: &tracing::Metadata<'_>) -> bool { + *meta.level() == tracing::Level::TRACE + && REQUEST_DUMP_TARGET_PREFIXES + .iter() + .any(|prefix| meta.target().starts_with(prefix)) +} + +/// A global filter layer that drops [`is_request_dump`] spans and events. Their +/// DEBUG and higher output still follows the [`build_env_filter`] filter. +/// +/// It is a separate layer rather than an `EnvFilter` directive because +/// `EnvFilter` lets the most specific directive win: a `RUST_LOG` naming a +/// longer target, such as `jsonrpsee_core::server=trace`, would override a +/// `jsonrpsee_core=debug` cap. No `-loglevel`, `-debug`, `logging` call or +/// `RUST_LOG` value can turn this layer off. +pub fn request_dump_guard() -> impl tracing_subscriber::Layer { + tracing_subscriber::filter::FilterFn::new(|meta: &tracing::Metadata<'_>| !is_request_dump(meta)) + // Allows any level itself, so the EnvFilter's hint still sets the + // global maximum level. + .with_max_level_hint(tracing_subscriber::filter::LevelFilter::TRACE) +} + /// Normalize a Bitcoin Core `-loglevel` severity to a `tracing` level. /// Core accepts `trace|debug|info|warning|error`; `tracing` spells the /// warning level `warn`. Returns `None` for an unrecognized level. @@ -9316,6 +9399,10 @@ fn check_money_range(rate: u64, raw: &str) -> Result { Ok(rate) } +#[cfg(test)] +#[path = "config_localharden_tests.rs"] +mod localharden_tests; + #[cfg(test)] mod tests { use super::*; diff --git a/satd/src/config_localharden_tests.rs b/satd/src/config_localharden_tests.rs new file mode 100644 index 000000000..8280da9ab --- /dev/null +++ b/satd/src/config_localharden_tests.rs @@ -0,0 +1,244 @@ +//! `bitcoin.conf` comment handling and secret-free `Debug` output. + +use super::*; + +/// Parse `body` as the `--conf` file of a regtest node with its datadir in +/// `dir`, plus `extra` command-line flags. +fn config_from(dir: &tempfile::TempDir, body: &str, extra: &[&str]) -> Result { + let conf = dir.path().join("comments.conf"); + std::fs::write(&conf, body).unwrap(); + let mut args = vec![ + "satd", + "--regtest", + "--datadir", + dir.path().to_str().unwrap(), + "--conf", + conf.to_str().unwrap(), + ]; + args.extend_from_slice(extra); + Config::from_cli(CliArgs::try_parse_from(args).unwrap()) +} + +fn global(cf: &ConfigFile, key: &str) -> Vec { + cf.global.get(key).cloned().unwrap_or_default() +} + +/// Bitcoin Core's GetConfigOptions (src/common/config.cpp) drops everything +/// from the first `#` on a line, wherever it is, before it reads the line. +#[test] +fn an_inline_comment_is_not_part_of_the_value() { + let cf = ConfigFile::parse( + "txindex=1 # keep the transaction index\n\ + rpcport=8332\t# the RPC port\n\ + uacomment=abc#def\n\ + includeconf=extra.conf # more settings\n\ + # a whole-line comment\n\ + \x20 # an indented comment\n\ + [test] # the testnet3 section\n\ + rpcport=18332 #x\n", + ) + .unwrap(); + assert_eq!(global(&cf, "txindex"), ["1"]); + assert_eq!(global(&cf, "rpcport"), ["8332"]); + assert_eq!(global(&cf, "uacomment"), ["abc"]); + assert_eq!(global(&cf, "includeconf"), ["extra.conf"]); + assert_eq!( + cf.sections.get("test").and_then(|s| s.get("rpcport")).cloned(), + Some(vec!["18332".to_string()]), + "a section header followed by a comment still opens the section" + ); +} + +/// The values a node actually runs with: before, `rpcport=18555 # c` failed +/// to parse as a port and the default was used without a word. +#[test] +fn inline_comments_reach_the_running_config_as_core_reads_them() { + let dir = tempfile::tempdir().expect("tempdir"); + let cfg = config_from( + &dir, + "rpcuser=alice # the operator\nrpcpassword=pw\n[regtest] # this chain\n\ + rpcport=18555 # not the default\n", + &[], + ) + .unwrap(); + assert_eq!(cfg.rpcport, 18555); + assert_eq!(cfg.rpcuser.as_deref(), Some("alice")); +} + +/// Core refuses an `rpcpassword` line that holds a `#` at all, rather than +/// guess whether it starts a comment or belongs to the password +/// (src/common/config.cpp GetConfigOptions; the caller adds the +/// "Error reading configuration file: " prefix, src/common/init.cpp). +#[test] +fn a_hash_on_an_rpcpassword_line_is_refused_with_cores_message() { + let cases = [ + ("rpcpassword=pw # old password\n", 1), + ("rpcpassword=a#b\n", 1), + ("server=1\n[regtest]\nrpcpassword=#\n", 3), + ]; + for (body, line) in cases { + let err = ConfigFile::parse(body).expect_err(body); + assert_eq!( + err, + format!( + "Error reading configuration file: parse error on line {line}, using # in \ + rpcpassword can be ambiguous and should be avoided" + ), + "{body:?}" + ); + } + + // satd reads a bare key as `key=1`, where Core refuses any line without + // `=`; with a `#` on it, a bare `rpcpassword` is refused too. + assert!(ConfigFile::parse("rpcpassword # no value\n").is_err()); + + // A commented-out password line is only a comment, and a `#` on any + // other key is a comment, as in Core. + let cf = ConfigFile::parse("# rpcpassword=old\nrpcpassword=pw\nrpcuser=bob#x\n").unwrap(); + assert_eq!(global(&cf, "rpcpassword"), ["pw"]); + assert_eq!(global(&cf, "rpcuser"), ["bob"]); + + // The running config refuses it the same way. + let dir = tempfile::tempdir().expect("tempdir"); + let err = config_from(&dir, "rpcuser=u\nrpcpassword=pw # c\n", &[]).unwrap_err(); + assert!(err.contains("using # in rpcpassword"), "got: {err}"); +} + +/// Core trims only spaces, tabs, CRs and LFs around a line, a key and a value +/// (`TrimString(str, " \t\r\n")`), so a vertical tab or a form feed stays in +/// the value. +#[test] +fn whitespace_is_trimmed_as_core_trims_it() { + let cf = ConfigFile::parse(" \t rpcuser \t=\t alice \t\r\nuacomment=x\u{b}\nrpcbind=\u{c}y\n").unwrap(); + assert_eq!(global(&cf, "rpcuser"), ["alice"]); + assert_eq!(global(&cf, "uacomment"), ["x\u{b}"]); + assert_eq!(global(&cf, "rpcbind"), ["\u{c}y"]); +} + +/// `Config` holds the RPC, Tor, Esplora and webhook secrets. Its `Debug` +/// output must not, so a stray `{:?}` or a panic message cannot write them to +/// the log. +#[test] +fn config_debug_output_carries_no_secrets() { + let dir = tempfile::tempdir().expect("tempdir"); + let cfg = config_from( + &dir, + "", + &[ + "--rpcuser=dbguser", + "--rpcpassword=RpcPasswordMarker", + "--rpcauth=authuser:0123456789abcdef$\ + 9383f6d244049af54e59a84188e2f2b1e58ff20de019156bc0c430ff8ae4c7a3", + "--torpassword=TorPasswordMarker", + "--esplorauserpass=esplora:EsploraPasswordMarker", + "--reorg-webhook=http://127.0.0.1:9/hook", + "--reorg-webhook-secret=WebhookSecretMarker", + ], + ) + .unwrap(); + assert_eq!(cfg.rpcpassword.as_deref(), Some("RpcPasswordMarker")); + let shown = format!("{cfg:?}"); + for secret in [ + "RpcPasswordMarker", + "TorPasswordMarker", + "EsploraPasswordMarker", + "WebhookSecretMarker", + "9383f6d2", + ] { + assert!(!shown.contains(secret), "Config Debug shows {secret}: {shown}"); + } + let shown_alt = format!("{cfg:#?}"); + assert!(!shown_alt.contains("RpcPasswordMarker"), "{shown_alt}"); + assert!(shown.contains("Regtest"), "Config Debug still names the network: {shown}"); + + let entry = &cfg.rpcauth[0]; + let shown = format!("{entry:?}"); + assert!(shown.contains("authuser"), "{shown}"); + assert!(!shown.contains(&format!("{:?}", entry.hash)), "rpcauth hash shown: {shown}"); + + let hook = crate::reload::webhook_target_from(&cfg).unwrap(); + let shown = format!("{hook:?}"); + assert!(shown.contains("127.0.0.1:9/hook"), "{shown}"); + assert!(!shown.contains("WebhookSecretMarker"), "webhook secret shown: {shown}"); +} + +/// A callsite for metadata built by hand: the filters only read the metadata +/// they are handed, never the callsite's. +struct TestCallsite; + +impl tracing::callsite::Callsite for TestCallsite { + fn set_interest(&self, _: tracing::subscriber::Interest) {} + fn metadata(&self) -> &tracing::Metadata<'_> { + unreachable!("filters read the metadata they are handed") + } +} + +static TEST_CALLSITE: TestCallsite = TestCallsite; + +/// Whether satd's log stack, with `filter` as its reloadable filter and +/// [`request_dump_guard`] beside it as `main` installs them, writes an event +/// at `level` from `target`. The subscriber is asked directly, so no global +/// dispatcher or callsite cache is involved. +fn would_log(filter: tracing_subscriber::EnvFilter, target: &str, level: tracing::Level) -> bool { + use tracing::Subscriber as _; + use tracing_subscriber::layer::SubscriberExt as _; + let meta = tracing::Metadata::new( + "event", + target, + level, + None, + None, + None, + tracing::field::FieldSet::new(&[], tracing::callsite::Identifier(&TEST_CALLSITE)), + tracing::metadata::Kind::EVENT, + ); + tracing_subscriber::registry() + .with(filter) + .with(request_dump_guard()) + .enabled(&meta) +} + +/// jsonrpsee's TRACE output prints the HTTP `Authorization` header and the +/// call parameters; tungstenite's prints WebSocket payloads. Trace logging +/// must not turn either on, while their DEBUG output and satd's own TRACE +/// output still get through. +#[test] +fn trace_logging_leaves_out_the_request_dumps() { + use tracing::Level; + let dumps = [ + "jsonrpsee-server", + "jsonrpsee_server::server", + "jsonrpsee-core", + "jsonrpsee_core::server::rpc_module", + "jsonrpsee", + "tungstenite::protocol::frame", + ]; + + let cfg = Config::from_cli( + CliArgs::try_parse_from(["satd", "--regtest", "--loglevel=trace"]).unwrap(), + ) + .unwrap(); + for target in dumps { + assert!( + !would_log(build_env_filter(&cfg), target, Level::TRACE), + "{target} TRACE is logged under -loglevel=trace" + ); + assert!( + would_log(build_env_filter(&cfg), target, Level::DEBUG), + "{target} DEBUG must still be logged under -loglevel=trace" + ); + } + assert!(would_log(build_env_filter(&cfg), "node::rpc::server", Level::TRACE)); + + // Directives naming the targets, more specifically than the guard's + // prefixes, cannot turn them back on. + let explicit = "trace,jsonrpsee-server=trace,jsonrpsee_core::server::rpc_module=trace,\ + tungstenite::protocol=trace"; + for target in dumps { + assert!( + !would_log(tracing_subscriber::EnvFilter::new(explicit), target, Level::TRACE), + "{target} TRACE is logged when a directive names it" + ); + } + assert!(would_log(tracing_subscriber::EnvFilter::new(explicit), "node::net", Level::TRACE)); +} diff --git a/satd/src/main.rs b/satd/src/main.rs index 351211de6..7db0f52e1 100644 --- a/satd/src/main.rs +++ b/satd/src/main.rs @@ -585,7 +585,12 @@ async fn run() -> Option> use tracing_subscriber::Layer as _; use tracing_subscriber::layer::SubscriberExt; use tracing_subscriber::util::SubscriberInitExt; - let registry = tracing_subscriber::registry().with(filter_layer); + // `request_dump_guard` keeps the RPC and WebSocket libraries' TRACE + // output, which prints request headers and parameters, out of the + // log whatever the filter above allows. + let registry = tracing_subscriber::registry() + .with(filter_layer) + .with(config::request_dump_guard()); // Only colorize when stdout is an actual terminal and the operator // hasn't opted out via NO_COLOR (https://no-color.org). Under journald // / a pipe, `is_terminal()` is false, so we don't leak ANSI escapes diff --git a/satd/src/reload.rs b/satd/src/reload.rs index a03b65be9..adf0da920 100644 --- a/satd/src/reload.rs +++ b/satd/src/reload.rs @@ -69,12 +69,22 @@ impl LogReloadHandle { /// [`SharedWebhook`] so a SIGHUP reload can change the URL/secret — or turn the /// webhook on/off — without restarting the dispatcher task. `None` means "no /// webhook configured"; the dispatcher then drains and drops records. -#[derive(Clone, Debug)] +#[derive(Clone)] pub struct WebhookTarget { pub url: String, pub secret: Option, } +/// `secret` is the webhook's HMAC key; `Debug` says only whether one is set. +impl std::fmt::Debug for WebhookTarget { + fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { + f.debug_struct("WebhookTarget") + .field("url", &self.url) + .field("secret", &self.secret.as_ref().map(|_| "")) + .finish() + } +} + /// Shared, reloadable reorg-webhook target. The dispatcher (spawned once at /// startup regardless of whether a webhook is configured) snapshots this per /// record; the reload path swaps its contents. diff --git a/satd/tests/local_hardening.rs b/satd/tests/local_hardening.rs new file mode 100644 index 000000000..e01ab97c7 --- /dev/null +++ b/satd/tests/local_hardening.rs @@ -0,0 +1,55 @@ +//! What satd writes to its log when trace logging is on. +//! +//! Bitcoin Core logs neither RPC credentials nor RPC parameters at any log +//! level. satd's RPC server is jsonrpsee, whose TRACE output prints each HTTP +//! request with its headers (the `Authorization` header included) and each +//! call's parameters, so `-loglevel=trace` must not let that output through. + +mod common; + +use base64::Engine as _; +use common::TestNode; +use serde_json::json; + +#[test] +fn trace_logging_never_writes_rpc_credentials_or_parameters() { + let user = "traceloguser"; + let pass = "TraceLogPasswordMarker"; + let param = "TraceLogParamMarker"; + let user_arg = format!("--rpcuser={user}"); + let pass_arg = format!("--rpcpassword={pass}"); + let mut node = TestNode::start(&["--loglevel=trace", &user_arg, &pass_arg]); + // The harness's RPC calls authenticate with `cookie` as `user:pass`. + node.cookie = format!("{user}:{pass}"); + + node.rpc_ok("getblockcount", vec![]); + node.rpc_ok("echo", vec![json!(param)]); + node.stop(); + + let log = std::fs::read_to_string(&node.stderr_log).expect("read the node's log"); + // jsonrpsee's own DEBUG lines must still be in the log, or the absence + // checks below could pass because its output was off altogether. (Before + // the TRACE cap, these same arguments put every request's headers in the + // log.) + assert!( + log.contains("Accepting new connection"), + "jsonrpsee-server's debug output is missing; only its TRACE output should be left out" + ); + + let basic = base64::engine::general_purpose::STANDARD.encode(format!("{user}:{pass}")); + for (what, secret) in [ + ("the Basic credential", basic.as_str()), + ("the password", pass), + ("an RPC parameter", param), + ] { + assert!( + !log.contains(secret), + "the trace log contains {what} ({secret}): {}", + log.lines() + .filter(|l| l.contains(secret)) + .take(3) + .collect::>() + .join("\n") + ); + } +}