fix(trezor): redact secrets in log_debug transport output (#125) - #144
Open
coreyphillips wants to merge 3 commits into
Open
fix(trezor): redact secrets in log_debug transport output (#125)#144coreyphillips wants to merge 3 commits into
coreyphillips wants to merge 3 commits into
Conversation
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.
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.
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
Collaborator
Author
|
Not signed off: the last round of fixes was pushed but has not been reviewed. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Closes #125
Redact secrets from Trezor transport debug output in core, before the log_debug callback crosses the FFI boundary.
CallbackAdapter::log_debugforwarded the trezor-connect-rs diagnostic stream verbatim to the native callback. That stream is owned by the dependency, so its contents are only ever known for the pinned version; in 0.4.0 it is mostly state names and byte counts, but nothing stops a bump from adding credential blobs or frame dumps. The only control was consumer-side regex scrubbing, which bitkit-android#1067 showed is not a security boundary: unexpected labels, bare values and JSON arrays all slip past it.What changed
src/modules/trezor/log_sanitizer.rs:sanitize_debug_log(tag, message)redacts labeledkey=value/"key": valuepairs whose key names a secret, replacing the value with a shape-preserving<redacted>placeholder.redact_bare_secretscatches unlabeled material: extended keys (xpub/xprv and variants), base64 PSBTs, hex runs of 16+ bytes, and long base64 blobs.is_harmless_valuelets booleans, counts, byte lengths andNonethrough under a sensitive label, sohas_credentials=trueandpayload: 48 bytesstill read as before;ALWAYS_SENSITIVE_KEY_FRAGMENTS(pin, passphrase, mnemonic, ...) is exempt from that carve-out because those secrets can themselves be short integers.CallbackAdapter::log_debuginimplementation.rsnow sanitizes both arguments before invoking the native callback; the signature is unchanged.log_debugtrait doc incallbacks.rsstates the redaction guarantee so consumers can drop their own scrubbing.How to test
cargo test modules::trezor, 75 pass, 15 of them new undertests.rs::log_sanitizer.test_no_fixture_secret_survives_sanitizationis the regression guard: it pushes each fixture (credential=, psbt=, bare xpub, frame hex, thp_credential/master_key, passphrase/pin/mnemonic) through as both tag and message and asserts no known secret appears in the output.test_connection_state_passes_through,test_error_codes_pass_throughandtest_byte_lengths_and_booleans_pass_throughpin the diagnostics that must survive.cargo clippy --all-targets, no new warnings (the trezor warnings shown are pre-existing).rustfmt --edition 2021 --check src/modules/trezor/*.rs, clean.cargo check --target aarch64-apple-ios --lib, compiles the mobile-only callback adapter, which is the actual call site.cargo testis 508 pass / 11 fail; all 11 failures are blocktank tests calling api.stag.blocktank.to and are network-dependent, unrelated to this change.