#35833 log: prevent user input from injecting fake log lines
https://github.com/bitcoin/bitcoin/pull/35833 · · +75/-18 in 9 files, 3 commits · labels: Utils/log/libs
Goal
- Prevent untrusted input from forging fake lines or misleading banners in debug.log
- Protect node operators relying on log integrity from RPC or external daemon injection
This pull request updates the core logging utility to escape embedded newline characters as '\x0a', preventing untrusted input from injecting forged log lines into debug.log. It strips only the conventional trailing newline from log messages before escaping, introduces a 'SplitLines' string helper, and updates legitimate multiline logging call sites to emit each line as an independent log entry.
Problem: Restricted RPC callers or external sources supplying wallet names or daemon status strings can supply newline characters that Bitcoin Core's logging framework previously preserved. This allows untrusted input to forge fake timestamps, warning banners, or block connection messages in node logs.
Category: Utilities (logging, arguments, libraries) (#30 of 66)
P3 · security
- P3 because it fixes an established log injection vulnerability from untrusted inputs
- Improves diagnostic integrity for node operators without fixing an active crash or consensus flaw
P3 because this fixes an established log injection vulnerability by preventing untrusted runtime input from forging fake node log lines. As noted in the description, 'Restricted RPC users and callers of createwallet or restorewallet can inject newlines... making forged lines look like node messages'. This improves logging robustness and operator diagnostic integrity, but does not address an actively exploited crash or consensus vulnerability, making review worthwhile and deferrable.
Membership: Modifies BCLog::Logger in src/logging.cpp and adds util::SplitLines in src/util/string.h
Factors: security/stability 2, bug 1, performance 0, user value 1, leverage 0
Reviewability: Ready
- Ready to review
- CI is passing and author addressed requested architectural cleanups
The PR is in a clean state with passing CI, and the author incorporated the architectural changes requested by reviewers.
Author status: active; promptly addressed reviewer feedback and force-pushed the suggested cleanups
Resolved concerns:
- Rejecting newlines wholesale would break existing multiline logs such as exceptions and transaction dumps (raised by maflcko; resolved by introducing SplitLines)
- Sanitizing strings at RPC call sites stripped non-newline characters and produced misleading logs (raised by ryanofsky; resolved by switching to central newline escaping in Logger::Format)
- Restricting wallet names to fix log injection exceeded the scope of a logging change (raised by ryanofsky, polespinasa; resolved by dropping wallet name restrictions)
Agreement: Strong
- Strong consensus on central newline escaping after resolving earlier approach and scope objections
- Supports central escaping over per-site sanitizing to cover all log sources (ryanofsky, davidgumberg)
- Verified injection paths are blocked by manual testing (polespinasa)
- Concept approval without stated reasons (achow101)
Strong consensus on central newline escaping; previous approach and scope objections were resolved to reviewer satisfaction
Reviewers agreed on the need to prevent log line injection, and the author adopted ryanofsky's design for central escaping alongside polespinasa's feedback.
- ryanofsky approved the updated escaping implementation on 2026-09-15
- polespinasa tested and re-ACKed on 2026-09-15
Objections:
| Reviewer | Kind | Harm | Status | Blocking | Author replied | Quote |
|---|---|---|---|---|---|---|
| maflcko | correctness | Wholesale blocking of newlines breaks existing multiline logs like exceptions and wallet commit transactions | resolved | no | yes | 2026-08-05: 'Sure, but this will silently break logs, as explained above.' Settled: 2026-09-15: Author implemented SplitLines for intentional multiline logs following ryanofsky's suggestion. |
| ryanofsky | approach | Using SanitizeString on RPC inputs strips valid characters and is fragile compared to global log escaping | resolved | yes | yes | 2026-09-14: 'It doesn't seem ideal to use SanitizeString string here because this strips characters other than newlines... Would be better to just escape newlines with \n in log messages' Settled: 2026-09-15: 'Thanks @ryanofsky, rebased and replaced RPC-specific sanitization with global newline escaping' |
| ryanofsky | scope | Restricting wallet names to prevent log injection adds out-of-scope wallet validation logic | resolved | no | yes | 2026-09-15: 'I do think it would be a little better to drop the two wallet commits here... and only keep the logging commits' Settled: 2026-09-15: 'dropped the wallet-name restrictions, and kept the RPC and wallet log-injection regressions' |
Support:
- ryanofsky: Favors central escaping in Logger::Format as it comprehensively secures all logging sources including Tor and I2P
- polespinasa: Verified testing of wallet name and RPC log behaviors across revisions
- davidgumberg: Noted external daemon strings need newline protection across the board
- achow101: ACK ed4eb51e9fc6f62975e272e96b22e0a6b64d3205 [not substantive]
Participants: Crypt-iQ (question), maflcko (support), davidgumberg (support), achow101 (support), polespinasa (support), ryanofsky (objection)
State derived from the lists: substantive support, no open objection (ryanofsky, polespinasa, davidgumberg)
Review verdicts (DrahtBot): 0 (+3)
- Stale ACK: achow101, polespinasa, ryanofsky
Files
48 lines under test/bench/ci.
- src/test/util_tests.cpp +24/-2
- src/logging.cpp +9/-7
- test/functional/rpc_whitelist.py +14/-2
- src/util/string.h +11/-0
- src/noui.cpp +5/-4
- test/functional/wallet_createwallet.py +6/-0
- src/util/exception.cpp +2/-1
- src/wallet/wallet.cpp +2/-1
- src/wallet/walletdb.cpp +2/-1
Card
PR #35833 updates the core logging framework to escape embedded newlines as '\x0a', preventing untrusted input from forging arbitrary log lines in debug.log. It solves a log-injection vulnerability exposed via RPC methods, wallet names, and external daemon messages without breaking existing multiline diagnostic messages. Review consensus is strong, with the author adopting an architecture suggested by ryanofsky and resolving all prior concerns regarding scope and multiline log readability.