From 475b9d9c8b1fbbf9888b41efff53a1007864e57c Mon Sep 17 00:00:00 2001 From: coreyphillips Date: Wed, 26 Aug 2026 15:59:05 -0400 Subject: [PATCH 1/4] fix(trezor): redact secrets before forwarding transport debug logs The log_debug stream handed to native apps is produced by trezor-connect-rs, so its contents change with every dependency bump and cannot be assumed free of key material. Consumer-side regex scrubbing was shown not to be a security boundary (synonymdev/bitkit-android#1067). Sanitize at the source instead: the callback adapter now runs both tag and message through a redaction pass that blanks labeled secrets, bare extended keys, PSBTs and long hex/base64 runs, and caps each string's length. Counts, flags and error strings still pass through so the diagnostics stay useful. No FFI signature change. --- src/modules/trezor/callbacks.rs | 6 + src/modules/trezor/implementation.rs | 10 +- src/modules/trezor/log_sanitizer.rs | 204 +++++++++++++++++++++++++++ src/modules/trezor/mod.rs | 5 + src/modules/trezor/tests.rs | 171 ++++++++++++++++++++++ 5 files changed, 394 insertions(+), 2 deletions(-) create mode 100644 src/modules/trezor/log_sanitizer.rs diff --git a/src/modules/trezor/callbacks.rs b/src/modules/trezor/callbacks.rs index c80d39a3..0e0329c3 100644 --- a/src/modules/trezor/callbacks.rs +++ b/src/modules/trezor/callbacks.rs @@ -175,6 +175,12 @@ pub trait TrezorTransportCallback: Send + Sync { /// debug UI (e.g., TrezorDebugLog on Android) so they are visible /// alongside the Kotlin-level logs. /// + /// Both arguments are redacted by bitkit-core before this is called: + /// credentials, keys, PSBTs, raw frame hex and anything else that looks + /// like key material are replaced with ``, and each string is + /// length-capped. Consumers do not need their own scrubbing pass. See + /// `log_sanitizer` for the exact policy. + /// /// # Arguments /// * `tag` - Short tag identifying the subsystem (e.g., "HANDSHAKE", "THP") /// * `message` - Human-readable debug message diff --git a/src/modules/trezor/implementation.rs b/src/modules/trezor/implementation.rs index ea97d89b..7ee7c998 100644 --- a/src/modules/trezor/implementation.rs +++ b/src/modules/trezor/implementation.rs @@ -307,9 +307,15 @@ impl TransportCallback for CallbackAdapter { self.callback.load_thp_credential(device_id.to_string()) } + /// Forward transport diagnostics to the native debug UI. + /// + /// The upstream stream is not trusted to be secret-free — its contents + /// belong to trezor-connect-rs and change with every bump — so everything + /// is redacted here, at the last point bitkit-core controls, rather than + /// by the consumer after the fact. fn log_debug(&self, tag: &str, message: &str) { - self.callback - .log_debug(tag.to_string(), message.to_string()); + let (tag, message) = super::log_sanitizer::sanitize_debug_log(tag, message); + self.callback.log_debug(tag, message); } } diff --git a/src/modules/trezor/log_sanitizer.rs b/src/modules/trezor/log_sanitizer.rs new file mode 100644 index 00000000..7a36c0b1 --- /dev/null +++ b/src/modules/trezor/log_sanitizer.rs @@ -0,0 +1,204 @@ +//! Redaction of Trezor transport debug output. +//! +//! `TransportCallback::log_debug` forwards diagnostics produced by +//! trezor-connect-rs to the native apps. That stream is owned by the +//! dependency and its exact contents change on every bump, so bitkit-core +//! cannot assume it is free of key material — and consumer-side regex +//! scrubbing is not a security boundary (see synonymdev/bitkit-android#1067). +//! +//! Everything crossing the FFI boundary therefore goes through +//! [`sanitize_debug_log`] first. The policy is deliberately conservative: +//! anything that looks like a secret is replaced with a stable `` +//! placeholder, and only values that are provably harmless (booleans, counts, +//! byte lengths, `None`) survive under a sensitive label. Diagnostics that +//! carry no value at all — subsystem tags, state names, error strings — pass +//! through untouched. + +use lazy_regex::{lazy_regex, Lazy}; +use regex::{Captures, Regex}; + +/// Placeholder substituted for any redacted value. +const REDACTED: &str = ""; + +/// Maximum length of a forwarded tag, in characters. +const MAX_TAG_CHARS: usize = 32; + +/// Maximum length of a forwarded message, in characters. +/// +/// Caps the per-chunk BLE spam that otherwise floods the native debug buffer, +/// and bounds anything the passes below failed to recognise as a secret. +const MAX_MESSAGE_CHARS: usize = 512; + +/// Key fragments whose value is never forwarded, whatever it looks like. +/// +/// A PIN and a pairing code are short integers, so the "counts are harmless" +/// exemption below must not apply to them. +const ALWAYS_SENSITIVE_KEY_FRAGMENTS: &[&str] = &[ + "entropy", + "mnemonic", + "pairingcode", + "passphrase", + "password", + "pin", + "privkey", + "secret", + "seed", +]; + +/// Key fragments whose value is forwarded only when it is provably harmless +/// (see [`is_harmless_value`]) — these labels are usually attached to sizes +/// and flags worth keeping, such as `has_credentials=true`. +const SENSITIVE_KEY_FRAGMENTS: &[&str] = &[ + "ciphertext", + "credential", + "key", + "nonce", + "payload", + "plaintext", + "psbt", + "salt", + "session", + "signature", + "token", + "transaction", + "xprv", + "xpub", +]; + +/// Sanitize a `(tag, message)` pair before it is handed to the native +/// `log_debug` callback. +/// +/// Returns owned strings ready for the FFI call. +pub fn sanitize_debug_log(tag: &str, message: &str) -> (String, String) { + ( + truncate(&sanitize(tag), MAX_TAG_CHARS), + truncate(&sanitize(message), MAX_MESSAGE_CHARS), + ) +} + +/// Apply every redaction pass to a single line of debug output. +fn sanitize(text: &str) -> String { + let text = redact_labeled_values(text); + redact_bare_secrets(&text) +} + +/// Redact `key=value`, `key: value` and `"key": value` pairs whose key names a +/// secret, unless the value is self-evidently harmless. +fn redact_labeled_values(text: &str) -> String { + static LABELED_VALUE: Lazy = lazy_regex!( + r#"(?x) + (?P[A-Za-z_][A-Za-z0-9_.\-]*) # label, optionally JSON-quoted + "? + \s*[:=]\s* + (?P + "[^"]*" # quoted string + | \[[^\]]*\] # array + | \{[^}]*\} # object + | [^\s,;)\]}"]+ # bare token + )"# + ); + + LABELED_VALUE + .replace_all(text, |caps: &Captures| { + let whole = &caps[0]; + let key = &caps["key"]; + let value = &caps["value"]; + let separator = &whole[key.len()..whole.len() - value.len()]; + + if is_always_sensitive_key(key) { + return format!("{}{}{}", key, separator, placeholder_for(value)); + } + if !is_sensitive_key(key) || is_harmless_value(value) { + return whole.to_string(); + } + // `credential: host_key=32bytes` — the captured "value" is itself a + // labeled pair, so descend into it instead of blanking the lot. + if is_labeled_pair(value) { + return format!("{}{}{}", key, separator, redact_labeled_values(value)); + } + format!("{}{}{}", key, separator, placeholder_for(value)) + }) + .into_owned() +} + +/// Redact secrets that carry no label at all: extended keys, base64 PSBTs, +/// long base64 blobs and long hex runs (serialized transactions, raw frames). +fn redact_bare_secrets(text: &str) -> String { + // A base64-encoded PSBT always starts with the `psbt\xff` magic. + static PSBT: Lazy = lazy_regex!(r"\bcHNidP[A-Za-z0-9+/]+=*"); + // xpub/xprv and the ypub/zpub/tpub/upub/vpub variants, mainnet or testnet. + static EXTENDED_KEY: Lazy = + lazy_regex!(r"\b[xyztuvXYZTUV](?:pub|prv)[1-9A-HJ-NP-Za-km-z]{40,}"); + // 16 bytes or more of contiguous hex: frame payloads, txids, serialized txs. + static LONG_HEX: Lazy = lazy_regex!(r"\b[0-9a-fA-F]{32,}\b"); + // Any other long unbroken base64 run — serialized credentials and the like. + static LONG_BASE64: Lazy = lazy_regex!(r"[A-Za-z0-9+/]{64,}=*"); + + let text = PSBT.replace_all(text, REDACTED); + let text = EXTENDED_KEY.replace_all(&text, REDACTED); + let text = LONG_HEX.replace_all(&text, REDACTED); + LONG_BASE64.replace_all(&text, REDACTED).into_owned() +} + +/// Whether a label suggests its value is key material. +fn is_sensitive_key(key: &str) -> bool { + matches_fragment(key, SENSITIVE_KEY_FRAGMENTS) || is_always_sensitive_key(key) +} + +/// Whether a label is one whose value is redacted unconditionally. +fn is_always_sensitive_key(key: &str) -> bool { + matches_fragment(key, ALWAYS_SENSITIVE_KEY_FRAGMENTS) +} + +/// Fold a label down to its letters and digits, then look for any fragment in +/// it — so `host_static_key`, `hostStaticKey` and `"host-static-key"` all hit +/// `key`. Over-matching here only costs diagnostic detail; under-matching leaks. +fn matches_fragment(key: &str, fragments: &[&str]) -> bool { + let normalized: String = key + .chars() + .filter(|c| c.is_ascii_alphanumeric()) + .map(|c| c.to_ascii_lowercase()) + .collect(); + fragments + .iter() + .any(|fragment| normalized.contains(fragment)) +} + +/// Whether a captured value is itself a `key=value` pair. +fn is_labeled_pair(value: &str) -> bool { + static LABELED_PAIR: Lazy = lazy_regex!(r#"^[A-Za-z_][A-Za-z0-9_.\-]*"?\s*[:=]"#); + LABELED_PAIR.is_match(value) +} + +/// Whether a value is safe to forward even under a sensitive label. +/// +/// Only counts, sizes, booleans and absence qualify — these are the +/// diagnostics worth keeping (`has_credentials=true`, `payload: 48 bytes`). +fn is_harmless_value(value: &str) -> bool { + static COUNT_OR_SIZE: Lazy = + lazy_regex!(r"(?i)^-?[0-9]+\s*(b|kb|mb|bit|bits|byte|bytes|char|chars|ms|s)?$"); + + matches!( + value.to_ascii_lowercase().as_str(), + "true" | "false" | "none" | "null" | "nil" | "n/a" | "unknown" | r#""""# | "[]" | "{}" + ) || COUNT_OR_SIZE.is_match(value) +} + +/// Build a placeholder that preserves the shape of the value it replaces, so +/// quoted fields stay quoted and arrays stay arrays. +fn placeholder_for(value: &str) -> String { + match value.as_bytes().first() { + Some(b'"') => format!("\"{}\"", REDACTED), + Some(b'[') => format!("[{}]", REDACTED), + Some(b'{') => format!("{{{}}}", REDACTED), + _ => REDACTED.to_string(), + } +} + +/// Truncate to `max_chars` on a character boundary, marking the cut. +fn truncate(text: &str, max_chars: usize) -> String { + match text.char_indices().nth(max_chars) { + Some((byte_index, _)) => format!("{}…", &text[..byte_index]), + None => text.to_string(), + } +} diff --git a/src/modules/trezor/mod.rs b/src/modules/trezor/mod.rs index 2fc271e0..2e9d1501 100644 --- a/src/modules/trezor/mod.rs +++ b/src/modules/trezor/mod.rs @@ -7,6 +7,11 @@ pub mod account_info; mod callbacks; mod errors; mod implementation; +/// Only the callback transport forwards debug output to a consumer, and that +/// transport is mobile-only. Compiled under `test` as well so the redaction +/// rules can be exercised on the host. +#[cfg(any(target_os = "android", target_os = "ios", test))] +pub(crate) mod log_sanitizer; #[cfg(test)] mod tests; mod types; diff --git a/src/modules/trezor/tests.rs b/src/modules/trezor/tests.rs index 961e99cb..98a46c70 100644 --- a/src/modules/trezor/tests.rs +++ b/src/modules/trezor/tests.rs @@ -938,4 +938,175 @@ mod tests { let _ = adapter.on_passphrase_request(true); assert_eq!(*mock.last_passphrase_on_device.lock().unwrap(), Some(true)); } + + // ======================================================================== + // Debug Log Sanitizer Tests + // ======================================================================== + + mod log_sanitizer { + use crate::modules::trezor::log_sanitizer::sanitize_debug_log; + + /// Secrets that must never survive a round trip through the sanitizer. + /// Deliberately literal — the regression test below greps for them. + const TEST_CREDENTIAL: &str = + "9f86d081884c7d659a2feaa0c55ad015a3bf4f1b2b0b822cd15d6c15b0f00a08"; + const TEST_XPUB: &str = "xpub6ERApfZwUNrhLCkDtcHTcxd75RbzS1ed54G1LkBUHQVHQKqhMkhgbmJbZRkrgZw4koxb5JaHWkY4ALHY2grBGRjaDMzQLcgJvLJuZZvRcEL"; + const TEST_PSBT: &str = "cHNidP8BAHUCAAAAASaBcTce3/KF6Tet7qSze3gADAVmy7OtZGQXE8pCFxv2AAAAAAD+////AtPf9QUAAAAAGXapFQ=="; + const TEST_FRAME_HEX: &str = + "042000ff0a20d3f1e5c7b9a84206ff1e2d3c4b5a69788796a5b4c3d2e1f00112233445566778899"; + + fn sanitized(message: &str) -> String { + let (_, message) = sanitize_debug_log("THP", message); + message + } + + #[test] + fn test_labeled_credential_is_redacted() { + let output = sanitized(&format!("Loaded credential={}", TEST_CREDENTIAL)); + assert_eq!(output, "Loaded credential="); + } + + #[test] + fn test_labeled_psbt_is_redacted() { + let output = sanitized(&format!("signing psbt={}", TEST_PSBT)); + assert_eq!(output, "signing psbt="); + } + + #[test] + fn test_unexpected_secret_label_is_redacted() { + // The point of matching on key fragments: labels nobody enumerated + // up front, like `thp_credential` or `master_key`, are still caught. + let output = sanitized(&format!( + "thp_credential={} master_key={}", + TEST_CREDENTIAL, TEST_XPUB + )); + assert_eq!(output, "thp_credential= master_key="); + } + + #[test] + fn test_json_string_and_array_values_are_redacted() { + let output = sanitized(&format!( + r#"{{"host_static_key": "{}", "credential": [1, 2, 3]}}"#, + TEST_CREDENTIAL + )); + assert_eq!( + output, + r#"{"host_static_key": "", "credential": []}"# + ); + } + + #[test] + fn test_bare_xpub_is_redacted() { + let output = sanitized(&format!("account descriptor {} at m/84'/0'/0'", TEST_XPUB)); + assert_eq!(output, "account descriptor at m/84'/0'/0'"); + } + + #[test] + fn test_bare_frame_hex_is_redacted() { + let output = sanitized(&format!("wrote frame {}", TEST_FRAME_HEX)); + assert_eq!(output, "wrote frame "); + } + + #[test] + fn test_bare_psbt_is_redacted() { + let output = sanitized(&format!("tx {}", TEST_PSBT)); + assert_eq!(output, "tx "); + } + + #[test] + fn test_connection_state_passes_through() { + let message = "trezor_state=1 (0=needs pairing, 1=paired, 2=autoconnect)"; + assert_eq!(sanitized(message), message); + } + + #[test] + fn test_error_codes_pass_through() { + let message = "Attempt 2 FAILED: THP Error: DecryptionFailed (error_code: 17)"; + assert_eq!(sanitized(message), message); + } + + #[test] + fn test_byte_lengths_and_booleans_pass_through() { + // Sensitive labels carrying only a count or a flag are the + // diagnostics worth keeping, so they must survive redaction. + let message = "Completion payload: 48 bytes (credential_sent=true)"; + assert_eq!(sanitized(message), message); + + let message = "try_to_unlock=false, has_credentials=true"; + assert_eq!(sanitized(message), message); + + let message = "Parsed credential: host_key=32bytes, credential=139bytes"; + assert_eq!(sanitized(message), message); + } + + #[test] + fn test_short_hex_metadata_passes_through() { + let message = "Channel allocated: a1b2"; + assert_eq!(sanitized(message), message); + } + + #[test] + fn test_tag_passes_through() { + let (tag, _) = sanitize_debug_log("HANDSHAKE", "Creating THP session..."); + assert_eq!(tag, "HANDSHAKE"); + } + + #[test] + fn test_long_message_is_truncated() { + let output = sanitized(&"chunk ".repeat(200)); + assert!(output.ends_with("…")); + assert!(output.chars().count() < 530); + } + + #[test] + fn test_multibyte_message_truncation_does_not_panic() { + let output = sanitized(&"é".repeat(1000)); + assert!(output.ends_with("…")); + } + + #[test] + fn test_no_fixture_secret_survives_sanitization() { + let fixtures = [ + format!("credential={}", TEST_CREDENTIAL), + format!( + "Stored credential {} for ble:AA:BB:CC:DD:EE:FF", + TEST_CREDENTIAL + ), + format!(r#"{{"credential":"{}"}}"#, TEST_CREDENTIAL), + format!("thp_credential={}", TEST_CREDENTIAL), + format!("xpub={}", TEST_XPUB), + format!("derived {} for account 0", TEST_XPUB), + format!("psbt={}", TEST_PSBT), + format!("Signing {}", TEST_PSBT), + format!("frame={}", TEST_FRAME_HEX), + format!("<< {}", TEST_FRAME_HEX), + "passphrase=hunter2 pin=1234 mnemonic=[a, b, c]".to_string(), + ]; + + for fixture in fixtures { + for (tag, message) in [ + sanitize_debug_log("THP", &fixture), + sanitize_debug_log(&fixture, "THP"), + ] { + let output = format!("{} {}", tag, message); + for secret in [ + TEST_CREDENTIAL, + TEST_XPUB, + TEST_PSBT, + TEST_FRAME_HEX, + "hunter2", + "1234", + ] { + assert!( + !output.contains(secret), + "leaked {:?} from fixture {:?}: {:?}", + secret, + fixture, + output + ); + } + } + } + } + } } From 2a01a0ddec989c59aaad207ddc5bb39712d72dd3 Mon Sep 17 00:00:00 2001 From: coreyphillips Date: Wed, 26 Aug 2026 16:15:50 -0400 Subject: [PATCH 2/4] fix(trezor): close three gaps in debug log redaction Multi-word values (mnemonics, unquoted passphrases) kept every word after the first, since the value pattern stops at whitespace; a redaction now absorbs trailing words up to the next delimiter or key=value pair. Bare integers passed through under sensitive labels, so token=1234567890 was forwarded intact. A number is now only harmless when it carries a unit or its label names a count or length. Byte dumps a debug formatter split into groups ([04, 20, 00, ff, ...], 04:20:00:ff:..., [4, 32, 0, 255, ...]) bypassed the contiguous-hex pass and are now redacted too. --- src/modules/trezor/log_sanitizer.rs | 119 ++++++++++++++++++++++------ src/modules/trezor/tests.rs | 54 +++++++++++++ 2 files changed, 149 insertions(+), 24 deletions(-) diff --git a/src/modules/trezor/log_sanitizer.rs b/src/modules/trezor/log_sanitizer.rs index 7a36c0b1..50a164c8 100644 --- a/src/modules/trezor/log_sanitizer.rs +++ b/src/modules/trezor/log_sanitizer.rs @@ -15,7 +15,7 @@ //! through untouched. use lazy_regex::{lazy_regex, Lazy}; -use regex::{Captures, Regex}; +use regex::Regex; /// Placeholder substituted for any redacted value. const REDACTED: &str = ""; @@ -41,10 +41,17 @@ const ALWAYS_SENSITIVE_KEY_FRAGMENTS: &[&str] = &[ "password", "pin", "privkey", + "recovery", "secret", "seed", ]; +/// Key fragments that make a bare integer provably a count or a size. +/// +/// Without one of these, an integer under a sensitive label is just as likely +/// to be a numeric token or code as it is to be a length. +const COUNT_KEY_FRAGMENTS: &[&str] = &["count", "index", "len", "num", "offset", "size"]; + /// Key fragments whose value is forwarded only when it is provably harmless /// (see [`is_harmless_value`]) — these labels are usually attached to sizes /// and flags worth keeping, such as `has_credentials=true`. @@ -94,35 +101,82 @@ fn redact_labeled_values(text: &str) -> String { "[^"]*" # quoted string | \[[^\]]*\] # array | \{[^}]*\} # object + # count with a spaced-out unit + | -?[0-9]+\s*(?i:bytes|byte|bits|bit|chars|char|kb|mb|ms|b|s)\b | [^\s,;)\]}"]+ # bare token )"# ); - LABELED_VALUE - .replace_all(text, |caps: &Captures| { - let whole = &caps[0]; - let key = &caps["key"]; - let value = &caps["value"]; - let separator = &whole[key.len()..whole.len() - value.len()]; + let mut out = String::with_capacity(text.len()); + let mut cursor = 0; - if is_always_sensitive_key(key) { - return format!("{}{}{}", key, separator, placeholder_for(value)); - } - if !is_sensitive_key(key) || is_harmless_value(value) { - return whole.to_string(); + for caps in LABELED_VALUE.captures_iter(text) { + let whole = caps.get(0).expect("group 0 always matches"); + if whole.start() < cursor { + // Swallowed by the multi-word redaction of an earlier pair. + continue; + } + let key = &caps["key"]; + let value = &caps["value"]; + let separator = &whole.as_str()[key.len()..whole.len() - value.len()]; + + out.push_str(&text[cursor..whole.start()]); + cursor = whole.end(); + + if !is_always_sensitive_key(key) { + if !is_sensitive_key(key) || is_harmless_value(key, value) { + out.push_str(whole.as_str()); + continue; } - // `credential: host_key=32bytes` — the captured "value" is itself a - // labeled pair, so descend into it instead of blanking the lot. if is_labeled_pair(value) { - return format!("{}{}{}", key, separator, redact_labeled_values(value)); + // `credential: host_key=32bytes` — the captured "value" is itself + // a labeled pair, so descend into it instead of blanking the lot. + out.push_str(key); + out.push_str(separator); + out.push_str(&redact_labeled_values(value)); + continue; } - format!("{}{}{}", key, separator, placeholder_for(value)) - }) - .into_owned() + } + + out.push_str(key); + out.push_str(separator); + out.push_str(&placeholder_for(value)); + cursor = end_of_multiword_value(text, value, cursor); + } + + out.push_str(&text[cursor..]); + out +} + +/// End of a redacted value, extended past the words the capture left behind. +/// +/// An unquoted secret can be several words long — `mnemonic=abandon ability +/// able …` — and the value pattern stops at the first space, so everything +/// after it would otherwise be forwarded verbatim. Trailing words are absorbed +/// up to the next structural delimiter or `key=value` pair, which keeps +/// unrelated diagnostics on the same line intact. +fn end_of_multiword_value(text: &str, value: &str, end: usize) -> usize { + static TRAILING_WORD: Lazy = lazy_regex!(r#"^[\t ]+[^\s,;:=(){}\[\]"]+"#); + + // Quoted, bracketed and braced values are already delimited. + if matches!(value.as_bytes().first(), Some(b'"' | b'[' | b'{')) { + return end; + } + + let mut end = end; + while let Some(word) = TRAILING_WORD.find(&text[end..]) { + let next = end + word.end(); + if matches!(text.as_bytes().get(next), Some(b':' | b'=')) { + break; // that word labels a value of its own + } + end = next; + } + end } /// Redact secrets that carry no label at all: extended keys, base64 PSBTs, -/// long base64 blobs and long hex runs (serialized transactions, raw frames). +/// long base64 blobs, and raw frames however they were formatted — one hex run +/// or byte groups a debug formatter split apart. fn redact_bare_secrets(text: &str) -> String { // A base64-encoded PSBT always starts with the `psbt\xff` magic. static PSBT: Lazy = lazy_regex!(r"\bcHNidP[A-Za-z0-9+/]+=*"); @@ -131,12 +185,20 @@ fn redact_bare_secrets(text: &str) -> String { lazy_regex!(r"\b[xyztuvXYZTUV](?:pub|prv)[1-9A-HJ-NP-Za-km-z]{40,}"); // 16 bytes or more of contiguous hex: frame payloads, txids, serialized txs. static LONG_HEX: Lazy = lazy_regex!(r"\b[0-9a-fA-F]{32,}\b"); + // The same payloads once a debug formatter has split them into groups: + // `04, 20, 00, ff, …`, `04 20 00 ff …`, `04:20:00:ff:…`. + static GROUPED_HEX: Lazy = + lazy_regex!(r"(?i)\b[0-9a-f]{2}(?:[\s,:_-]+[0-9a-f]{2}){7,}\b"); + // `[4, 32, 0, 255, …]` — Rust's `Debug` for a slice of bytes. + static BYTE_ARRAY: Lazy = lazy_regex!(r"\[\s*[0-9]{1,3}(?:\s*,\s*[0-9]{1,3}){7,}\s*\]"); // Any other long unbroken base64 run — serialized credentials and the like. static LONG_BASE64: Lazy = lazy_regex!(r"[A-Za-z0-9+/]{64,}=*"); let text = PSBT.replace_all(text, REDACTED); let text = EXTENDED_KEY.replace_all(&text, REDACTED); let text = LONG_HEX.replace_all(&text, REDACTED); + let text = GROUPED_HEX.replace_all(&text, REDACTED); + let text = BYTE_ARRAY.replace_all(&text, format!("[{}]", REDACTED)); LONG_BASE64.replace_all(&text, REDACTED).into_owned() } @@ -174,14 +236,23 @@ fn is_labeled_pair(value: &str) -> bool { /// /// Only counts, sizes, booleans and absence qualify — these are the /// diagnostics worth keeping (`has_credentials=true`, `payload: 48 bytes`). -fn is_harmless_value(value: &str) -> bool { - static COUNT_OR_SIZE: Lazy = - lazy_regex!(r"(?i)^-?[0-9]+\s*(b|kb|mb|bit|bits|byte|bytes|char|chars|ms|s)?$"); +/// +/// A number is only a count when it says so: either it carries a unit, or the +/// label names a length. A bare integer under any other sensitive label is +/// just as likely to be a numeric token or a code, so it is redacted. +fn is_harmless_value(key: &str, value: &str) -> bool { + static SIZE: Lazy = + lazy_regex!(r"(?i)^-?[0-9]+\s*(b|kb|mb|bit|bits|byte|bytes|char|chars|ms|s)$"); + static BARE_NUMBER: Lazy = lazy_regex!(r"^-?[0-9]+$"); - matches!( + if matches!( value.to_ascii_lowercase().as_str(), "true" | "false" | "none" | "null" | "nil" | "n/a" | "unknown" | r#""""# | "[]" | "{}" - ) || COUNT_OR_SIZE.is_match(value) + ) { + return true; + } + SIZE.is_match(value) + || (BARE_NUMBER.is_match(value) && matches_fragment(key, COUNT_KEY_FRAGMENTS)) } /// Build a placeholder that preserves the shape of the value it replaces, so diff --git a/src/modules/trezor/tests.rs b/src/modules/trezor/tests.rs index 98a46c70..0248811f 100644 --- a/src/modules/trezor/tests.rs +++ b/src/modules/trezor/tests.rs @@ -954,6 +954,9 @@ mod tests { const TEST_PSBT: &str = "cHNidP8BAHUCAAAAASaBcTce3/KF6Tet7qSze3gADAVmy7OtZGQXE8pCFxv2AAAAAAD+////AtPf9QUAAAAAGXapFQ=="; const TEST_FRAME_HEX: &str = "042000ff0a20d3f1e5c7b9a84206ff1e2d3c4b5a69788796a5b4c3d2e1f00112233445566778899"; + const TEST_FRAME_GROUPS: &str = "04, 20, 00, ff, 0a, 20, d3, f1, e5, c7, b9, a8"; + const TEST_MNEMONIC: &str = + "abandon ability able about above absent absorb abstract absurd abuse access accident"; fn sanitized(message: &str) -> String { let (_, message) = sanitize_debug_log("THP", message); @@ -1007,6 +1010,47 @@ mod tests { assert_eq!(output, "wrote frame "); } + #[test] + fn test_grouped_frame_bytes_are_redacted() { + // Debug formatters split a frame into byte groups, which the + // contiguous-hex pass alone does not recognise. + let output = sanitized( + "wrote frame [04, 20, 00, ff, 0a, 20, d3, f1, e5, c7, b9, a8, 42, 06, ff, 1e]", + ); + assert_eq!(output, "wrote frame []"); + + let output = sanitized("<< 04 20 00 ff 0a 20 d3 f1 e5 c7 b9 a8"); + assert_eq!(output, "<< "); + + let output = sanitized("read [4, 32, 0, 255, 10, 32, 211, 241, 229, 199]"); + assert_eq!(output, "read []"); + } + + #[test] + fn test_multiword_secret_is_redacted_in_full() { + let output = + sanitized("mnemonic=abandon ability able about above absent absorb abstract abuse"); + assert_eq!(output, "mnemonic="); + + // Neighbouring diagnostics still survive. + let output = sanitized("passphrase=correct horse battery staple, state=paired"); + assert_eq!(output, "passphrase=, state=paired"); + + let output = sanitized("passphrase=correct horse battery device=trezor"); + assert_eq!(output, "passphrase= device=trezor"); + } + + #[test] + fn test_bare_numeric_secret_is_redacted() { + // A number is only a count when it carries a unit or the label + // names a length — `token=1234567890` is neither. + let output = sanitized("token=1234567890 session_id=4815162342"); + assert_eq!(output, "token= session_id="); + + let output = sanitized("credential_count=3, key_len=32"); + assert_eq!(output, "credential_count=3, key_len=32"); + } + #[test] fn test_bare_psbt_is_redacted() { let output = sanitized(&format!("tx {}", TEST_PSBT)); @@ -1080,7 +1124,12 @@ mod tests { format!("Signing {}", TEST_PSBT), format!("frame={}", TEST_FRAME_HEX), format!("<< {}", TEST_FRAME_HEX), + format!("frame=[{}]", TEST_FRAME_GROUPS), + format!("<< {}", TEST_FRAME_GROUPS), + format!("mnemonic={}", TEST_MNEMONIC), + format!("recovery: {} (12 words)", TEST_MNEMONIC), "passphrase=hunter2 pin=1234 mnemonic=[a, b, c]".to_string(), + "session_token=4815162342".to_string(), ]; for fixture in fixtures { @@ -1096,6 +1145,11 @@ mod tests { TEST_FRAME_HEX, "hunter2", "1234", + "4815162342", + // Tails, so a redaction that only covers the first + // word or the first byte group still fails the test. + "ability", + "d3, f1", ] { assert!( !output.contains(secret), From 538ce38a34b41d228d9b87703ac7989204ce1b4f Mon Sep 17 00:00:00 2001 From: coreyphillips Date: Wed, 26 Aug 2026 16:42:12 -0400 Subject: [PATCH 3/4] fix(trezor): close nested and multi-word leaks in log redaction Sensitive labels forwarded their value whenever it happened to look like another labeled pair, so `token=user:hunter2` crossed log_debug intact: only descend into a value that is itself a sensitive pair. Four more shapes the flat parser let through: - a secret nested under an innocuous label (`context={"token":"hunter2"}`) was forwarded whole; non-sensitive labels now recurse into their value - a two-word label (`seed phrase:`) could not be spanned by the key pattern, so the up-to-two words preceding a label now count toward it - an escaped quote ended a quoted value early, leaking the tail - a spaced-out separator (`pin = 1234`) was absorbed as a trailing word of the previous secret, leaving its own value unredacted --- src/modules/trezor/log_sanitizer.rs | 66 +++++++++++++++++++++++++---- src/modules/trezor/tests.rs | 62 +++++++++++++++++++++++++++ 2 files changed, 119 insertions(+), 9 deletions(-) diff --git a/src/modules/trezor/log_sanitizer.rs b/src/modules/trezor/log_sanitizer.rs index 50a164c8..add922a9 100644 --- a/src/modules/trezor/log_sanitizer.rs +++ b/src/modules/trezor/log_sanitizer.rs @@ -98,7 +98,7 @@ fn redact_labeled_values(text: &str) -> String { "? \s*[:=]\s* (?P - "[^"]*" # quoted string + "(?:[^"\\]|\\.)*" # quoted string, escapes included | \[[^\]]*\] # array | \{[^}]*\} # object # count with a spaced-out unit @@ -119,16 +119,27 @@ fn redact_labeled_values(text: &str) -> String { let key = &caps["key"]; let value = &caps["value"]; let separator = &whole.as_str()[key.len()..whole.len() - value.len()]; + // `seed phrase: …` is one label written as two words, and the pattern + // above can only capture the last of them. + let label = format!("{}{}", preceding_words(text, whole.start()), key); out.push_str(&text[cursor..whole.start()]); cursor = whole.end(); - if !is_always_sensitive_key(key) { - if !is_sensitive_key(key) || is_harmless_value(key, value) { + if !is_always_sensitive_key(&label) { + if !is_sensitive_key(&label) { + // The label is innocuous, but the value can still nest a pair + // that is not — `context={"token": "hunter2"}`. + out.push_str(key); + out.push_str(separator); + out.push_str(&redact_nested_values(value)); + continue; + } + if is_harmless_value(&label, value) { out.push_str(whole.as_str()); continue; } - if is_labeled_pair(value) { + if is_sensitive_labeled_pair(value) { // `credential: host_key=32bytes` — the captured "value" is itself // a labeled pair, so descend into it instead of blanking the lot. out.push_str(key); @@ -148,6 +159,37 @@ fn redact_labeled_values(text: &str) -> String { out } +/// The one or two plain words written immediately before a label. +/// +/// Anything but a bare word — punctuation, a digit, a redacted value — ends the +/// run, so this only ever picks up words that read as part of the label itself. +fn preceding_words(text: &str, start: usize) -> &str { + static PRECEDING_WORDS: Lazy = lazy_regex!(r"(?:[A-Za-z_][A-Za-z0-9_.\-]*[\t ]+){1,2}$"); + PRECEDING_WORDS + .find(&text[..start]) + .map_or("", |words| words.as_str()) +} + +/// Redact inside the value of a label that is not itself sensitive. +/// +/// Structured values are descended into, because the parser above sees the +/// stream as flat text and would otherwise forward a nested `{"token": …}` +/// whole. Recursion terminates because every descent drops at least the +/// delimiters or the leading `key=`. +fn redact_nested_values(value: &str) -> String { + static LABELED_PAIR: Lazy = lazy_regex!(r#"^[A-Za-z_][A-Za-z0-9_.\-]*"?\s*[:=]\s*\S"#); + + let (open, close) = match value.as_bytes().first() { + Some(b'"') => ('"', '"'), + Some(b'[') => ('[', ']'), + Some(b'{') => ('{', '}'), + _ if LABELED_PAIR.is_match(value) => return redact_labeled_values(value), + _ => return value.to_string(), + }; + let inner = &value[1..value.len() - 1]; + format!("{}{}{}", open, redact_labeled_values(inner), close) +} + /// End of a redacted value, extended past the words the capture left behind. /// /// An unquoted secret can be several words long — `mnemonic=abandon ability @@ -166,7 +208,10 @@ fn end_of_multiword_value(text: &str, value: &str, end: usize) -> usize { let mut end = end; while let Some(word) = TRAILING_WORD.find(&text[end..]) { let next = end + word.end(); - if matches!(text.as_bytes().get(next), Some(b':' | b'=')) { + // `passphrase=hunter2 pin = 1234` — the separator may be spaced out, and + // absorbing `pin` would leave its value behind unredacted. + let rest = text[next..].trim_start_matches([' ', '\t']); + if rest.starts_with([':', '=']) { break; // that word labels a value of its own } end = next; @@ -226,10 +271,13 @@ fn matches_fragment(key: &str, fragments: &[&str]) -> bool { .any(|fragment| normalized.contains(fragment)) } -/// Whether a captured value is itself a `key=value` pair. -fn is_labeled_pair(value: &str) -> bool { - static LABELED_PAIR: Lazy = lazy_regex!(r#"^[A-Za-z_][A-Za-z0-9_.\-]*"?\s*[:=]"#); - LABELED_PAIR.is_match(value) +/// Whether a captured value starts with a sensitive `key=value` pair. +fn is_sensitive_labeled_pair(value: &str) -> bool { + static LABELED_PAIR: Lazy = + lazy_regex!(r#"^(?P[A-Za-z_][A-Za-z0-9_.\-]*)"?\s*[:=]\s*\S"#); + LABELED_PAIR + .captures(value) + .is_some_and(|captures| is_sensitive_key(&captures["key"])) } /// Whether a value is safe to forward even under a sensitive label. diff --git a/src/modules/trezor/tests.rs b/src/modules/trezor/tests.rs index 0248811f..c901adfd 100644 --- a/src/modules/trezor/tests.rs +++ b/src/modules/trezor/tests.rs @@ -969,6 +969,60 @@ mod tests { assert_eq!(output, "Loaded credential="); } + #[test] + fn test_labeled_pair_like_secret_is_redacted() { + for (message, expected) in [ + ("credential=SGVsbG8gV29ybGQ=", "credential="), + ( + "host_static_key=c2VjcmV0a2V5bWF0ZXJpYWwxMjM0NTY3ODkw=", + "host_static_key=", + ), + ("session_id=trezor:abcd1234", "session_id="), + ("token=user:hunter2", "token="), + ] { + assert_eq!(sanitized(message), expected); + } + } + + #[test] + fn test_nested_secret_under_innocuous_label_is_redacted() { + for (message, expected) in [ + ( + r#"context={"token":"hunter2"}"#, + r#"context={"token":""}"#, + ), + ( + "request=[passphrase=hunter2]", + "request=[passphrase=]", + ), + ("detail=\"pin=1234\"", "detail=\"pin=\""), + ("state=token:hunter2", "state=token:"), + ] { + assert_eq!(sanitized(message), expected); + } + } + + #[test] + fn test_two_word_label_is_redacted() { + let output = sanitized(&format!("seed phrase: {}", TEST_MNEMONIC)); + assert_eq!(output, "seed phrase: "); + + let output = sanitized("wallet passphrase: correct horse battery"); + assert_eq!(output, "wallet passphrase: "); + } + + #[test] + fn test_spaced_separator_after_multiword_value_is_redacted() { + let output = sanitized("passphrase=hunter2 pin = 1234"); + assert_eq!(output, "passphrase= pin = "); + } + + #[test] + fn test_escaped_quote_does_not_end_a_quoted_secret() { + let output = sanitized(r#"passphrase="hunter\"tail""#); + assert_eq!(output, r#"passphrase="""#); + } + #[test] fn test_labeled_psbt_is_redacted() { let output = sanitized(&format!("signing psbt={}", TEST_PSBT)); @@ -1130,6 +1184,14 @@ mod tests { format!("recovery: {} (12 words)", TEST_MNEMONIC), "passphrase=hunter2 pin=1234 mnemonic=[a, b, c]".to_string(), "session_token=4815162342".to_string(), + "passphrase=hunter2 pin = 1234".to_string(), + r#"passphrase="hunter2\"1234""#.to_string(), + format!( + r#"context={{"passphrase":"hunter2","seed":"{}"}}"#, + TEST_MNEMONIC + ), + format!("seed phrase: {}", TEST_MNEMONIC), + format!("recovery seed = {} pin = 1234", TEST_MNEMONIC), ]; for fixture in fixtures { From d9418fde064ee36a25881728e1348e2dbc6d76b3 Mon Sep 17 00:00:00 2001 From: coreyphillips Date: Fri, 28 Aug 2026 14:31:52 -0400 Subject: [PATCH 4/4] fix(trezor): close two gaps in the debug log sanitizer An unclosed quote made the labeled-value pattern fail to match at all, so `passphrase="hunter2` was forwarded verbatim. The pattern now accepts a quote upstream left open, and the delimiter check no longer mistakes the last character of such a value for its closing quote. Redaction descends into nested values, once per link in a chain like `a=b=c=...`, which a long enough line drove off the stack and aborted the process. Truncation now runs before redaction rather than after, and descent stops at a fixed depth and blanks the rest. Condense the comments added by this branch to the reasoning that is not already in the code. --- src/modules/trezor/callbacks.rs | 8 +- src/modules/trezor/implementation.rs | 8 +- src/modules/trezor/log_sanitizer.rs | 196 +++++++++++++-------------- src/modules/trezor/mod.rs | 5 +- src/modules/trezor/tests.rs | 53 +++++--- 5 files changed, 138 insertions(+), 132 deletions(-) diff --git a/src/modules/trezor/callbacks.rs b/src/modules/trezor/callbacks.rs index 0e0329c3..01be7067 100644 --- a/src/modules/trezor/callbacks.rs +++ b/src/modules/trezor/callbacks.rs @@ -175,11 +175,9 @@ pub trait TrezorTransportCallback: Send + Sync { /// debug UI (e.g., TrezorDebugLog on Android) so they are visible /// alongside the Kotlin-level logs. /// - /// Both arguments are redacted by bitkit-core before this is called: - /// credentials, keys, PSBTs, raw frame hex and anything else that looks - /// like key material are replaced with ``, and each string is - /// length-capped. Consumers do not need their own scrubbing pass. See - /// `log_sanitizer` for the exact policy. + /// Both arguments arrive redacted and length-capped: anything resembling + /// key material is already ``, so consumers need no scrubbing + /// pass of their own. See `log_sanitizer` for the policy. /// /// # Arguments /// * `tag` - Short tag identifying the subsystem (e.g., "HANDSHAKE", "THP") diff --git a/src/modules/trezor/implementation.rs b/src/modules/trezor/implementation.rs index 7ee7c998..ac06c985 100644 --- a/src/modules/trezor/implementation.rs +++ b/src/modules/trezor/implementation.rs @@ -307,12 +307,8 @@ impl TransportCallback for CallbackAdapter { self.callback.load_thp_credential(device_id.to_string()) } - /// Forward transport diagnostics to the native debug UI. - /// - /// The upstream stream is not trusted to be secret-free — its contents - /// belong to trezor-connect-rs and change with every bump — so everything - /// is redacted here, at the last point bitkit-core controls, rather than - /// by the consumer after the fact. + /// Redacts here, the last point bitkit-core controls, because the upstream + /// stream is not trusted to be secret-free. fn log_debug(&self, tag: &str, message: &str) { let (tag, message) = super::log_sanitizer::sanitize_debug_log(tag, message); self.callback.log_debug(tag, message); diff --git a/src/modules/trezor/log_sanitizer.rs b/src/modules/trezor/log_sanitizer.rs index add922a9..904a45a1 100644 --- a/src/modules/trezor/log_sanitizer.rs +++ b/src/modules/trezor/log_sanitizer.rs @@ -1,38 +1,35 @@ //! Redaction of Trezor transport debug output. //! //! `TransportCallback::log_debug` forwards diagnostics produced by -//! trezor-connect-rs to the native apps. That stream is owned by the -//! dependency and its exact contents change on every bump, so bitkit-core -//! cannot assume it is free of key material — and consumer-side regex -//! scrubbing is not a security boundary (see synonymdev/bitkit-android#1067). +//! trezor-connect-rs. That stream belongs to the dependency and its contents +//! change on every bump, so it cannot be assumed free of key material, and +//! consumer-side scrubbing is not a security boundary (see +//! synonymdev/bitkit-android#1067). //! -//! Everything crossing the FFI boundary therefore goes through -//! [`sanitize_debug_log`] first. The policy is deliberately conservative: -//! anything that looks like a secret is replaced with a stable `` -//! placeholder, and only values that are provably harmless (booleans, counts, -//! byte lengths, `None`) survive under a sensitive label. Diagnostics that -//! carry no value at all — subsystem tags, state names, error strings — pass -//! through untouched. +//! [`sanitize_debug_log`] therefore runs over everything before it crosses the +//! FFI boundary. Anything resembling a secret becomes ``; only +//! provably harmless values (booleans, counts, byte lengths, `None`) survive +//! under a sensitive label. Value-free diagnostics such as tags, state names +//! and error strings pass through untouched. use lazy_regex::{lazy_regex, Lazy}; use regex::Regex; -/// Placeholder substituted for any redacted value. const REDACTED: &str = ""; -/// Maximum length of a forwarded tag, in characters. const MAX_TAG_CHARS: usize = 32; -/// Maximum length of a forwarded message, in characters. -/// -/// Caps the per-chunk BLE spam that otherwise floods the native debug buffer, -/// and bounds anything the passes below failed to recognise as a secret. +/// Caps the per-chunk BLE spam that floods the native debug buffer, and bounds +/// anything the passes below fail to recognise as a secret. const MAX_MESSAGE_CHARS: usize = 512; -/// Key fragments whose value is never forwarded, whatever it looks like. -/// -/// A PIN and a pairing code are short integers, so the "counts are harmless" -/// exemption below must not apply to them. +/// How far redaction descends into nested values before it stops looking and +/// blanks the rest. Real diagnostics nest a level or two; a long chain of them +/// is only ever a way to drive this module off the stack. +const MAX_NESTING_DEPTH: usize = 8; + +/// Key fragments whose value is never forwarded, whatever it looks like: a PIN +/// or pairing code is a short integer, so the count exemption must not reach it. const ALWAYS_SENSITIVE_KEY_FRAGMENTS: &[&str] = &[ "entropy", "mnemonic", @@ -46,15 +43,12 @@ const ALWAYS_SENSITIVE_KEY_FRAGMENTS: &[&str] = &[ "seed", ]; -/// Key fragments that make a bare integer provably a count or a size. -/// -/// Without one of these, an integer under a sensitive label is just as likely -/// to be a numeric token or code as it is to be a length. +/// Key fragments that make a bare integer provably a count or a size rather +/// than a numeric token. const COUNT_KEY_FRAGMENTS: &[&str] = &["count", "index", "len", "num", "offset", "size"]; -/// Key fragments whose value is forwarded only when it is provably harmless -/// (see [`is_harmless_value`]) — these labels are usually attached to sizes -/// and flags worth keeping, such as `has_credentials=true`. +/// Key fragments whose value is forwarded only when [`is_harmless_value`] +/// clears it, keeping diagnostics like `has_credentials=true`. const SENSITIVE_KEY_FRAGMENTS: &[&str] = &[ "ciphertext", "credential", @@ -75,23 +69,21 @@ const SENSITIVE_KEY_FRAGMENTS: &[&str] = &[ /// Sanitize a `(tag, message)` pair before it is handed to the native /// `log_debug` callback. /// -/// Returns owned strings ready for the FFI call. +/// Truncation runs first so the redaction passes only ever see a bounded input. pub fn sanitize_debug_log(tag: &str, message: &str) -> (String, String) { ( - truncate(&sanitize(tag), MAX_TAG_CHARS), - truncate(&sanitize(message), MAX_MESSAGE_CHARS), + sanitize(&truncate(tag, MAX_TAG_CHARS)), + sanitize(&truncate(message, MAX_MESSAGE_CHARS)), ) } -/// Apply every redaction pass to a single line of debug output. fn sanitize(text: &str) -> String { - let text = redact_labeled_values(text); - redact_bare_secrets(&text) + redact_bare_secrets(&redact_labeled_values(text, 0)) } /// Redact `key=value`, `key: value` and `"key": value` pairs whose key names a /// secret, unless the value is self-evidently harmless. -fn redact_labeled_values(text: &str) -> String { +fn redact_labeled_values(text: &str, depth: usize) -> String { static LABELED_VALUE: Lazy = lazy_regex!( r#"(?x) (?P[A-Za-z_][A-Za-z0-9_.\-]*) # label, optionally JSON-quoted @@ -99,6 +91,7 @@ fn redact_labeled_values(text: &str) -> String { \s*[:=]\s* (?P "(?:[^"\\]|\\.)*" # quoted string, escapes included + | "(?:[^"\\]|\\.)* # ... or one upstream left unclosed | \[[^\]]*\] # array | \{[^}]*\} # object # count with a spaced-out unit @@ -113,13 +106,12 @@ fn redact_labeled_values(text: &str) -> String { for caps in LABELED_VALUE.captures_iter(text) { let whole = caps.get(0).expect("group 0 always matches"); if whole.start() < cursor { - // Swallowed by the multi-word redaction of an earlier pair. - continue; + continue; // swallowed by the multi-word redaction of an earlier pair } let key = &caps["key"]; let value = &caps["value"]; let separator = &whole.as_str()[key.len()..whole.len() - value.len()]; - // `seed phrase: …` is one label written as two words, and the pattern + // `seed phrase: ...` is one label written as two words, and the pattern // above can only capture the last of them. let label = format!("{}{}", preceding_words(text, whole.start()), key); @@ -128,23 +120,23 @@ fn redact_labeled_values(text: &str) -> String { if !is_always_sensitive_key(&label) { if !is_sensitive_key(&label) { - // The label is innocuous, but the value can still nest a pair - // that is not — `context={"token": "hunter2"}`. + // Innocuous label, but the value can still nest a pair that is + // not: `context={"token": "hunter2"}`. out.push_str(key); out.push_str(separator); - out.push_str(&redact_nested_values(value)); + out.push_str(&redact_nested_values(value, depth)); continue; } if is_harmless_value(&label, value) { out.push_str(whole.as_str()); continue; } - if is_sensitive_labeled_pair(value) { - // `credential: host_key=32bytes` — the captured "value" is itself - // a labeled pair, so descend into it instead of blanking the lot. + if depth < MAX_NESTING_DEPTH && is_sensitive_labeled_pair(value) { + // `credential: host_key=32bytes`, so descend into the inner + // pair instead of blanking the lot. out.push_str(key); out.push_str(separator); - out.push_str(&redact_labeled_values(value)); + out.push_str(&redact_labeled_values(value, depth + 1)); continue; } } @@ -159,10 +151,8 @@ fn redact_labeled_values(text: &str) -> String { out } -/// The one or two plain words written immediately before a label. -/// -/// Anything but a bare word — punctuation, a digit, a redacted value — ends the -/// run, so this only ever picks up words that read as part of the label itself. +/// The one or two plain words written immediately before a label. Anything but +/// a bare word ends the run, so only words reading as part of the label count. fn preceding_words(text: &str, start: usize) -> &str { static PRECEDING_WORDS: Lazy = lazy_regex!(r"(?:[A-Za-z_][A-Za-z0-9_.\-]*[\t ]+){1,2}$"); PRECEDING_WORDS @@ -170,58 +160,71 @@ fn preceding_words(text: &str, start: usize) -> &str { .map_or("", |words| words.as_str()) } -/// Redact inside the value of a label that is not itself sensitive. -/// -/// Structured values are descended into, because the parser above sees the -/// stream as flat text and would otherwise forward a nested `{"token": …}` -/// whole. Recursion terminates because every descent drops at least the -/// delimiters or the leading `key=`. -fn redact_nested_values(value: &str) -> String { +/// Redact inside the value of a label that is not itself sensitive: the parser +/// above sees flat text and would otherwise forward a nested `{"token": ...}` +/// whole. Past [`MAX_NESTING_DEPTH`] the value is blanked instead of descended +/// into, so an adversarial chain of pairs cannot exhaust the stack. +fn redact_nested_values(value: &str, depth: usize) -> String { static LABELED_PAIR: Lazy = lazy_regex!(r#"^[A-Za-z_][A-Za-z0-9_.\-]*"?\s*[:=]\s*\S"#); - let (open, close) = match value.as_bytes().first() { - Some(b'"') => ('"', '"'), - Some(b'[') => ('[', ']'), - Some(b'{') => ('{', '}'), - _ if LABELED_PAIR.is_match(value) => return redact_labeled_values(value), - _ => return value.to_string(), + if depth >= MAX_NESTING_DEPTH { + return placeholder_for(value); + } + match delimiters(value) { + Some((open, close)) => format!( + "{}{}{}", + open, + redact_labeled_values(&value[1..value.len() - 1], depth + 1), + close + ), + None if LABELED_PAIR.is_match(value) => redact_labeled_values(value, depth + 1), + None => value.to_string(), + } +} + +/// The delimiter pair enclosing a value, if it is enclosed at all. An unclosed +/// quote is not, and must not have its last character mistaken for one. +fn delimiters(value: &str) -> Option<(char, char)> { + let bytes = value.as_bytes(); + let (open, close) = match bytes.first()? { + b'"' => ('"', '"'), + b'[' => ('[', ']'), + b'{' => ('{', '}'), + _ => return None, }; - let inner = &value[1..value.len() - 1]; - format!("{}{}{}", open, redact_labeled_values(inner), close) + (value.len() >= 2 && bytes[value.len() - 1] == close as u8).then_some((open, close)) } /// End of a redacted value, extended past the words the capture left behind. /// -/// An unquoted secret can be several words long — `mnemonic=abandon ability -/// able …` — and the value pattern stops at the first space, so everything -/// after it would otherwise be forwarded verbatim. Trailing words are absorbed -/// up to the next structural delimiter or `key=value` pair, which keeps -/// unrelated diagnostics on the same line intact. +/// An unquoted secret can be several words long (`mnemonic=abandon ability +/// ...`) and the value pattern stops at the first space. Trailing words are +/// absorbed up to the next delimiter or `key=value` pair, so unrelated +/// diagnostics on the same line stay intact. fn end_of_multiword_value(text: &str, value: &str, end: usize) -> usize { static TRAILING_WORD: Lazy = lazy_regex!(r#"^[\t ]+[^\s,;:=(){}\[\]"]+"#); - // Quoted, bracketed and braced values are already delimited. - if matches!(value.as_bytes().first(), Some(b'"' | b'[' | b'{')) { + if delimiters(value).is_some() { return end; } let mut end = end; while let Some(word) = TRAILING_WORD.find(&text[end..]) { let next = end + word.end(); - // `passphrase=hunter2 pin = 1234` — the separator may be spaced out, and - // absorbing `pin` would leave its value behind unredacted. - let rest = text[next..].trim_start_matches([' ', '\t']); - if rest.starts_with([':', '=']) { - break; // that word labels a value of its own + // `passphrase=hunter2 pin = 1234`: the separator may be spaced out, and + // absorbing `pin` would leave its own value behind unredacted. + if text[next..] + .trim_start_matches([' ', '\t']) + .starts_with([':', '=']) + { + break; } end = next; } end } -/// Redact secrets that carry no label at all: extended keys, base64 PSBTs, -/// long base64 blobs, and raw frames however they were formatted — one hex run -/// or byte groups a debug formatter split apart. +/// Redact secrets carrying no label at all. fn redact_bare_secrets(text: &str) -> String { // A base64-encoded PSBT always starts with the `psbt\xff` magic. static PSBT: Lazy = lazy_regex!(r"\bcHNidP[A-Za-z0-9+/]+=*"); @@ -231,12 +234,12 @@ fn redact_bare_secrets(text: &str) -> String { // 16 bytes or more of contiguous hex: frame payloads, txids, serialized txs. static LONG_HEX: Lazy = lazy_regex!(r"\b[0-9a-fA-F]{32,}\b"); // The same payloads once a debug formatter has split them into groups: - // `04, 20, 00, ff, …`, `04 20 00 ff …`, `04:20:00:ff:…`. + // `04, 20, 00, ff, ...`, `04 20 00 ff ...`, `04:20:00:ff:...`. static GROUPED_HEX: Lazy = lazy_regex!(r"(?i)\b[0-9a-f]{2}(?:[\s,:_-]+[0-9a-f]{2}){7,}\b"); - // `[4, 32, 0, 255, …]` — Rust's `Debug` for a slice of bytes. + // `[4, 32, 0, 255, ...]`, Rust's `Debug` for a slice of bytes. static BYTE_ARRAY: Lazy = lazy_regex!(r"\[\s*[0-9]{1,3}(?:\s*,\s*[0-9]{1,3}){7,}\s*\]"); - // Any other long unbroken base64 run — serialized credentials and the like. + // Any other long unbroken base64 run: serialized credentials and the like. static LONG_BASE64: Lazy = lazy_regex!(r"[A-Za-z0-9+/]{64,}=*"); let text = PSBT.replace_all(text, REDACTED); @@ -247,19 +250,17 @@ fn redact_bare_secrets(text: &str) -> String { LONG_BASE64.replace_all(&text, REDACTED).into_owned() } -/// Whether a label suggests its value is key material. fn is_sensitive_key(key: &str) -> bool { matches_fragment(key, SENSITIVE_KEY_FRAGMENTS) || is_always_sensitive_key(key) } -/// Whether a label is one whose value is redacted unconditionally. fn is_always_sensitive_key(key: &str) -> bool { matches_fragment(key, ALWAYS_SENSITIVE_KEY_FRAGMENTS) } /// Fold a label down to its letters and digits, then look for any fragment in -/// it — so `host_static_key`, `hostStaticKey` and `"host-static-key"` all hit -/// `key`. Over-matching here only costs diagnostic detail; under-matching leaks. +/// it, so `host_static_key`, `hostStaticKey` and `"host-static-key"` all hit +/// `key`. Over-matching only costs diagnostic detail; under-matching leaks. fn matches_fragment(key: &str, fragments: &[&str]) -> bool { let normalized: String = key .chars() @@ -271,7 +272,6 @@ fn matches_fragment(key: &str, fragments: &[&str]) -> bool { .any(|fragment| normalized.contains(fragment)) } -/// Whether a captured value starts with a sensitive `key=value` pair. fn is_sensitive_labeled_pair(value: &str) -> bool { static LABELED_PAIR: Lazy = lazy_regex!(r#"^(?P[A-Za-z_][A-Za-z0-9_.\-]*)"?\s*[:=]\s*\S"#); @@ -282,12 +282,9 @@ fn is_sensitive_labeled_pair(value: &str) -> bool { /// Whether a value is safe to forward even under a sensitive label. /// -/// Only counts, sizes, booleans and absence qualify — these are the -/// diagnostics worth keeping (`has_credentials=true`, `payload: 48 bytes`). -/// -/// A number is only a count when it says so: either it carries a unit, or the -/// label names a length. A bare integer under any other sensitive label is -/// just as likely to be a numeric token or a code, so it is redacted. +/// A number only counts when it says so, by carrying a unit or by its label +/// naming a length. A bare integer under any other sensitive label is as +/// likely to be a numeric token or a code, so it is redacted. fn is_harmless_value(key: &str, value: &str) -> bool { static SIZE: Lazy = lazy_regex!(r"(?i)^-?[0-9]+\s*(b|kb|mb|bit|bits|byte|bytes|char|chars|ms|s)$"); @@ -303,18 +300,15 @@ fn is_harmless_value(key: &str, value: &str) -> bool { || (BARE_NUMBER.is_match(value) && matches_fragment(key, COUNT_KEY_FRAGMENTS)) } -/// Build a placeholder that preserves the shape of the value it replaces, so -/// quoted fields stay quoted and arrays stay arrays. +/// A placeholder preserving the shape of the value it replaces, so quoted +/// fields stay quoted and arrays stay arrays. fn placeholder_for(value: &str) -> String { - match value.as_bytes().first() { - Some(b'"') => format!("\"{}\"", REDACTED), - Some(b'[') => format!("[{}]", REDACTED), - Some(b'{') => format!("{{{}}}", REDACTED), - _ => REDACTED.to_string(), + match delimiters(value) { + Some((open, close)) => format!("{}{}{}", open, REDACTED, close), + None => REDACTED.to_string(), } } -/// Truncate to `max_chars` on a character boundary, marking the cut. fn truncate(text: &str, max_chars: usize) -> String { match text.char_indices().nth(max_chars) { Some((byte_index, _)) => format!("{}…", &text[..byte_index]), diff --git a/src/modules/trezor/mod.rs b/src/modules/trezor/mod.rs index 2e9d1501..2e1f71c4 100644 --- a/src/modules/trezor/mod.rs +++ b/src/modules/trezor/mod.rs @@ -7,9 +7,8 @@ pub mod account_info; mod callbacks; mod errors; mod implementation; -/// Only the callback transport forwards debug output to a consumer, and that -/// transport is mobile-only. Compiled under `test` as well so the redaction -/// rules can be exercised on the host. +/// Mobile-only, like the callback transport that is its only caller. Also +/// built under `test` so the redaction rules can be exercised on the host. #[cfg(any(target_os = "android", target_os = "ios", test))] pub(crate) mod log_sanitizer; #[cfg(test)] diff --git a/src/modules/trezor/tests.rs b/src/modules/trezor/tests.rs index c901adfd..431b2d56 100644 --- a/src/modules/trezor/tests.rs +++ b/src/modules/trezor/tests.rs @@ -939,15 +939,10 @@ mod tests { assert_eq!(*mock.last_passphrase_on_device.lock().unwrap(), Some(true)); } - // ======================================================================== - // Debug Log Sanitizer Tests - // ======================================================================== - mod log_sanitizer { use crate::modules::trezor::log_sanitizer::sanitize_debug_log; - /// Secrets that must never survive a round trip through the sanitizer. - /// Deliberately literal — the regression test below greps for them. + /// Literal fixtures, so the regression test below can grep for them. const TEST_CREDENTIAL: &str = "9f86d081884c7d659a2feaa0c55ad015a3bf4f1b2b0b822cd15d6c15b0f00a08"; const TEST_XPUB: &str = "xpub6ERApfZwUNrhLCkDtcHTcxd75RbzS1ed54G1LkBUHQVHQKqhMkhgbmJbZRkrgZw4koxb5JaHWkY4ALHY2grBGRjaDMzQLcgJvLJuZZvRcEL"; @@ -1023,6 +1018,20 @@ mod tests { assert_eq!(output, r#"passphrase="""#); } + #[test] + fn test_unclosed_quote_does_not_defeat_redaction() { + // An unclosed quote used to match no value at all, forwarding the + // whole pair verbatim. + assert_eq!(sanitized(r#"passphrase="hunter2"#), "passphrase="); + assert_eq!( + sanitized(&format!(r#"{{"credential": "{}"#, TEST_CREDENTIAL)), + r#"{"credential": "# + ); + // An innocuous value must not lose its last character to a closing + // delimiter that was never there. + assert_eq!(sanitized(r#"state="paired"#), r#"state="paired"#); + } + #[test] fn test_labeled_psbt_is_redacted() { let output = sanitized(&format!("signing psbt={}", TEST_PSBT)); @@ -1031,8 +1040,7 @@ mod tests { #[test] fn test_unexpected_secret_label_is_redacted() { - // The point of matching on key fragments: labels nobody enumerated - // up front, like `thp_credential` or `master_key`, are still caught. + // Matching on key fragments catches labels nobody enumerated. let output = sanitized(&format!( "thp_credential={} master_key={}", TEST_CREDENTIAL, TEST_XPUB @@ -1096,8 +1104,6 @@ mod tests { #[test] fn test_bare_numeric_secret_is_redacted() { - // A number is only a count when it carries a unit or the label - // names a length — `token=1234567890` is neither. let output = sanitized("token=1234567890 session_id=4815162342"); assert_eq!(output, "token= session_id="); @@ -1125,8 +1131,8 @@ mod tests { #[test] fn test_byte_lengths_and_booleans_pass_through() { - // Sensitive labels carrying only a count or a flag are the - // diagnostics worth keeping, so they must survive redaction. + // Counts and flags under a sensitive label are the diagnostics + // worth keeping, so they must survive redaction. let message = "Completion payload: 48 bytes (credential_sent=true)"; assert_eq!(sanitized(message), message); @@ -1152,14 +1158,26 @@ mod tests { #[test] fn test_long_message_is_truncated() { let output = sanitized(&"chunk ".repeat(200)); - assert!(output.ends_with("…")); + assert!(output.ends_with("\u{2026}")); assert!(output.chars().count() < 530); } #[test] fn test_multibyte_message_truncation_does_not_panic() { - let output = sanitized(&"é".repeat(1000)); - assert!(output.ends_with("…")); + let output = sanitized(&"\u{e9}".repeat(1000)); + assert!(output.ends_with("\u{2026}")); + } + + #[test] + fn test_deeply_chained_pairs_do_not_exhaust_the_stack() { + // Redaction descends into nested values, so an unbounded input + // used to recurse once per link and abort the process. Run on a + // thread with a small stack to catch a regression on mobile. + let worker = std::thread::Builder::new() + .stack_size(256 * 1024) + .spawn(|| sanitized(&("a=".repeat(20_000) + "1"))) + .expect("spawn"); + assert!(worker.join().expect("no stack overflow").len() < 600); } #[test] @@ -1186,6 +1204,7 @@ mod tests { "session_token=4815162342".to_string(), "passphrase=hunter2 pin = 1234".to_string(), r#"passphrase="hunter2\"1234""#.to_string(), + r#"passphrase="hunter2"#.to_string(), format!( r#"context={{"passphrase":"hunter2","seed":"{}"}}"#, TEST_MNEMONIC @@ -1208,8 +1227,8 @@ mod tests { "hunter2", "1234", "4815162342", - // Tails, so a redaction that only covers the first - // word or the first byte group still fails the test. + // Tails, so a redaction covering only the first word or + // byte group still fails the test. "ability", "d3, f1", ] {