log: prevent user input from injecting fake log lines #35833

l0rinc commented at 12:02 AM on July 29, 2026: contributor

Problem: Restricted RPC users and callers of createwallet or restorewallet can inject newlines through rejected methods or wallet names, and global log escaping preserves them, making forged lines look like node messages.

Fix: Escape embedded newlines in log messages.

<details> <summary>Manual reproducer</summary>

DATADIR="$(mktemp -d /tmp/bitcoin-log-injection.XXXXXX)"
M="$(date -u +%Y-%m-%dT%H:%M:%SZ) ERROR: ConnectTip: ConnectBlock 0000000000000000deadbeefdeadbeefdeadbeefdeadbeefdeadbeefdeadbeef failed, bad-txns-inputs-missingorspent"
cmake -B build >/dev/null 2>&1 && cmake --build build -j >/dev/null 2>&1
build/bin/bitcoind -regtest -daemonwait -datadir="$DATADIR" -rpcwhitelist=__cookie__:getblock,stop >/dev/null 2>&1
build/bin/bitcoin-cli -regtest -datadir="$DATADIR" $'getblock\n'"$M" >/dev/null 2>&1; killall bitcoind >/dev/null
echo; grep -E 'ConnectTip|not allowed' "$DATADIR/regtest/debug.log"

Before

2026-07-28T23:18:57Z [warning] RPC User __cookie__ not allowed to call method getblock
2026-07-28T23:18:55Z ERROR: ConnectTip: ConnectBlock 0000000000000000deadbeefdeadbeefdeadbeefdeadbeefdeadbeefdeadbeef failed, bad-txns-inputs-missingorspent

After

2026-07-28T23:19:45Z [warning] RPC User __cookie__ not allowed to call method getblock\x0a2026-07-28T23:19:41Z ERROR: ConnectTip: ConnectBlock 0000000000000000deadbeefdeadbeefdeadbeefdeadbeefdeadbeefdeadbeef failed, bad-txns-inputs-missingorspent

</details>

DrahtBot commented at 12:02 AM on July 29, 2026: contributor

The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

Code Coverage & Benchmarks

For details see: https://corecheck.dev/bitcoin/bitcoin/pulls/35833.

Reviews

See the guideline and AI policy for information on the review process.

Type	Reviewers
ACK	ryanofsky, w0xlt, achow101
Stale ACK	polespinasa

If your review is incorrectly listed, please copy-paste <code></code> into the comment that the bot should ignore.

Conflicts

No conflicts as of last run.

DrahtBot added the label CI failed on Jul 29, 2026

Crypt-iQ commented at 2:51 AM on July 29, 2026: contributor

Just posting mostly as a curiosity to see if you looked at whether the logging interface is vulnerable to format string attacks. I don't think it is, but would be interested in knowing if so. Though I'm not aware of anywhere in the p2p interface that can arbitrarily log blobs.

maflcko commented at 5:58 AM on July 29, 2026: member

I wonder if non-trailing b"\n" can be wholesale rejected by the log framework, unless they are opt-in for the IIRC single log line that "needs" them?

l0rinc force-pushed on Jul 29, 2026

DrahtBot removed the label CI failed on Jul 29, 2026

l0rinc commented at 9:25 PM on July 29, 2026: contributor

If someone does a followup that addresses that for the whole log framework, please ping me. Here I meant to cover the two exceptional cases I found of user-provided data that escapes sanitization.

davidgumberg commented at 10:15 PM on August 4, 2026: contributor

We also print arbitrary stuff from the tor control server:

+1 to just blocking newlines in all logging, the tor control server can log control sequences except for those disallowed by LogEscapeMessage

https://github.com/bitcoin/bitcoin/blob/17c5e33e9c5418fb0240f5d4210c87f54b88cdda/src/torcontrol.cpp#L672

and from the i2p daemon:

https://github.com/bitcoin/bitcoin/blob/17c5e33e9c5418fb0240f5d4210c87f54b88cdda/src/i2p.cpp#L273-L275

Both of these can be remote in some setups (but are trusted i.e. if they are compromised you are in big trouble)

On top of newlines, there are some (obscure i believe) terminal setups where 8-bit control sequences > 127 can be printed which allows e.g. clipboard injection.

Maybe LogEscapeMessage like this:

diff --git a/src/logging.cpp b/src/logging.cpp
index bbe5f043ed..39e9493749 100644
--- a/src/logging.cpp
+++ b/src/logging.cpp
@@ -338,7 +338,9 @@ namespace BCLog {
         std::string ret;
         for (char ch_in : str) {
             uint8_t ch = (uint8_t)ch_in;
-            if ((ch >= 32 || ch == '\n') && ch != '\x7f') {
+            if (ch == '\n') {
+                continue;
+            } else if (ch >= 32 && ch < 127) {
                 ret += ch_in;
             } else {
                 ret += strprintf("\\x%02x", ch);
@@ -427,7 +429,7 @@ std::string BCLog::Logger::Format(const util::log::Entry& entry) const
     result += GetLogPrefix(static_cast<LogFlags>(entry.category), entry.level);
     result += LogEscapeMessage(entry.message);

-    if (!result.ends_with('\n')) result += '\n';
+    result += '\n';
     return result;
 }

maflcko commented at 8:01 AM on August 5, 2026: member

Maybe LogEscapeMessage like this:

Sure, but this will silently break logs, as explained above. Ref:

# git grep --extended-regexp '(WalletLogPrintf\("CommitTransaction:\\n|LogWarning\("\\n)'
src/util/exception.cpp:    LogWarning("\n\n************************\n%s", message);
src/wallet/wallet.cpp:    WalletLogPrintf("CommitTransaction:\n%s\n", util::RemoveSuffixView(tx->ToString(), "\n"));

They will need to be fixed, or opt-in with an option, as explained above.

maflcko commented at 8:04 AM on August 5, 2026: member

My recommendation would be to go ahead with this pull first, then rework the log framework in a separate pull.

l0rinc commented at 6:27 PM on August 5, 2026: contributor

I agree with Marco that the logging framework should be reworked separately, I’d like to keep this PR focused on the identified newline injection paths and handle broader filtering in follow-ups. And we should probably be careful about escaping every byte above 0x7f, since that could also affect valid UTF-8, including translated log messages.

achow101 commented at 11:52 PM on August 7, 2026: member

ACK ed4eb51e9fc6f62975e272e96b22e0a6b64d3205

polespinasa commented at 11:11 AM on August 26, 2026: member

tACK ed4eb51e9fc6f62975e272e96b22e0a6b64d3205

<details> <summary>manual test</summary>

$ ./build/bin/bitcoin-cli -regtest createwallet invalid\nwallet
{
  "name": "invalidnwallet"
}
sliv3r@sliv3r-tuxedo:~/Documentos/Projectes/BitcoinCore/bitcoin$ ./build/bin/bitcoin-cli -regtest createwallet "invalid\nwallet"
{
  "name": "invalid\\nwallet"
}
sliv3r@sliv3r-tuxedo:~/Documentos/Projectes/BitcoinCore/bitcoin$ ./build/bin/bitcoin-cli -regtest createwallet $'invalid\nwallet'
error code: -8
error message:
Wallet name cannot contain control characters

<\details>

sedited referenced this in commit b811aeabad on Sep 2, 2026

in src/httprpc.cpp:118 in 6ed8e2af39

 114 | @@ -115,7 +115,7 @@ UniValue ExecuteHTTPRPC(const UniValue& valRequest, JSONRPCRequest& jreq, HTTPSt
 115 |          } else if (valRequest.isObject()) {
 116 |              jreq.parse(valRequest);
 117 |              if (user_has_whitelist && !g_rpc_whitelist[jreq.authUser].contains(jreq.strMethod)) {
 118 | -                LogWarning("RPC User %s not allowed to call method %s", jreq.authUser, jreq.strMethod);
 119 | +                LogWarning("RPC User %s not allowed to call method %s", jreq.authUser, SanitizeString(jreq.strMethod));

ryanofsky commented at 7:35 PM on September 14, 2026:

In commit "test: characterize rejected RPC method logs" (9d5fb22f1d499267a1deda6320a417c2b7d03530)

It doesn't seem ideal to use SanitizeString string here because this strips characters other than newlines, and produce confusing / misleading warnings, and could make it appear that whitelisted methods were being rejected in the logs.

It would be good (in a followup) to revert this change and simply escape newlines with \n in log messages, just like we escape all other control characters.

l0rinc commented at 7:45 AM on September 15, 2026:

Did something similar

ryanofsky commented at 8:00 PM on September 14, 2026: contributor

Code review ACK ed4eb51e9fc6f62975e272e96b22e0a6b64d3205, but the fixes here seem messy and fragile. Would be better to just escape newlines with \n in log messages like we escape other control characters as others have suggested. Since there are only 3 places in the code using multiline log messages, this should be pretty easy:

--- a/src/logging.cpp
+++ b/src/logging.cpp
@@ -330,15 +330,17 @@ namespace BCLog {
     /** Belts and suspenders: make sure outgoing log messages don't contain
      * potentially suspicious characters, such as terminal control codes.
      *
-     * This escapes control characters except newline ('\n') in C syntax.
-     * It escapes instead of removes them to still allow for troubleshooting
-     * issues where they accidentally end up in strings.
+     * This escapes control characters, including newline ('\n'), in C
+     * syntax, so a message always occupies exactly one line and data that
+     * ends up in a message can't forge additional log lines. It escapes
+     * instead of removes them to still allow for troubleshooting issues
+     * where they accidentally end up in strings.
      */
     std::string LogEscapeMessage(std::string_view str) {
         std::string ret;
         for (char ch_in : str) {
             uint8_t ch = (uint8_t)ch_in;
-            if ((ch >= 32 || ch == '\n') && ch != '\x7f') {
+            if (ch >= 32 && ch != '\x7f') {
                 ret += ch_in;
             } else {
                 ret += strprintf("\\x%02x", ch);
@@ -425,9 +427,10 @@ std::string BCLog::Logger::Format(const util::log::Entry& entry) const
     }
 
     result += GetLogPrefix(static_cast<LogFlags>(entry.category), entry.level);
-    result += LogEscapeMessage(entry.message);
-
-    if (!result.ends_with('\n')) result += '\n';
+    // Many callers still terminate messages with '\n'. Drop that one trailing
+    // newline so it isn't escaped, then terminate the line unconditionally.
+    result += LogEscapeMessage(util::RemoveSuffixView(entry.message, "\n"));
+    result += '\n';
     return result;
 }
 
--- a/src/noui.cpp
+++ b/src/noui.cpp
@@ -8,6 +8,7 @@
 #include <node/interface_ui.h>
 #include <util/btcsignals.h>
 #include <util/log.h>
+#include <util/string.h>
 #include <util/translation.h>
 
 #include <string>
@@ -26,18 +27,18 @@ void noui_ThreadSafeMessageBox(const bilingual_str& message, unsigned int style)
     switch (style) {
     case CClientUIInterface::MSG_ERROR:
         strCaption = "Error: ";
-        if (!fSecure) LogError("%s\n", message.original);
+        if (!fSecure) SplitLines(message.original, [](std::string_view line) { LogError("%s", line); });
         break;
     case CClientUIInterface::MSG_WARNING:
         strCaption = "Warning: ";
-        if (!fSecure) LogWarning("%s\n", message.original);
+        if (!fSecure) SplitLines(message.original, [](std::string_view line) { LogWarning("%s", line); });
         break;
     case CClientUIInterface::MSG_INFORMATION:
         strCaption = "Information: ";
-        if (!fSecure) LogInfo("%s\n", message.original);
+        if (!fSecure) SplitLines(message.original, [](std::string_view line) { LogInfo("%s", line); });
         break;
     default:
-        if (!fSecure) LogInfo("%s%s\n", strCaption, message.original);
+        if (!fSecure) SplitLines(message.original, [&](std::string_view line) { LogInfo("%s%s", strCaption, line); });
     }
 
     tfm::format(std::cerr, "%s%s\n", strCaption, message.original);
--- a/src/test/util_tests.cpp
+++ b/src/test/util_tests.cpp
@@ -1398,8 +1398,10 @@ BOOST_AUTO_TEST_CASE(test_LogEscapeMessage)
 {
     // ASCII and UTF-8 must pass through unaltered.
     BOOST_CHECK_EQUAL(BCLog::LogEscapeMessage("Valid log message貓"), "Valid log message貓");
-    // Newlines must pass through unaltered.
-    BOOST_CHECK_EQUAL(BCLog::LogEscapeMessage("Message\n with newlines\n"), "Message\n with newlines\n");
+    // Newlines are escaped like other control characters, so a message can't
+    // span (or forge) log lines. Logger::Format strips the conventional
+    // trailing newline before escaping.
+    BOOST_CHECK_EQUAL(BCLog::LogEscapeMessage("Message\n with newlines\n"), R"(Message\x0a with newlines\x0a)");
     // Other control characters are escaped in C syntax.
     BOOST_CHECK_EQUAL(BCLog::LogEscapeMessage("\x01\x7f Corrupted log message\x0d"), R"(\x01\x7f Corrupted log message\x0d)");
     // Embedded NULL characters are escaped too.
--- a/src/util/exception.cpp
+++ b/src/util/exception.cpp
@@ -7,6 +7,7 @@
 
 #include <tinyformat.h>
 #include <util/log.h>
+#include <util/string.h>
 
 #include <exception>
 #include <iostream>
@@ -36,6 +37,6 @@ static std::string FormatException(const std::exception* pex, std::string_view t
 void PrintExceptionContinue(const std::exception* pex, std::string_view thread_name)
 {
     std::string message = FormatException(pex, thread_name);
-    LogWarning("\n\n************************\n%s", message);
+    SplitLines(strprintf("\n\n************************\n%s", message), [](std::string_view line) { LogWarning("%s", line); });
     tfm::format(std::cerr, "\n\n************************\n%s\n", message);
 }
--- a/src/util/string.h
+++ b/src/util/string.h
@@ -160,6 +160,23 @@ std::vector<T> Split(std::span<const char> sp LIFETIMEBOUND, char sep, bool incl
     return Split<std::string>(str, separators);
 }
 
+/**
+ * Call fn(line) for each newline-separated line of str, without the newline.
+ * An empty str produces one empty line; a trailing newline does not produce
+ * an extra one. Intended for logging multi-line text one log message per line,
+ * since a log message cannot contain a newline (see LogEscapeMessage).
+ */
+template <typename Fn>
+void SplitLines(std::string_view str, Fn&& fn)
+{
+    do {
+        const size_t pos{str.find('\n')};
+        fn(str.substr(0, pos));
+        if (pos == std::string_view::npos) break;
+        str.remove_prefix(pos + 1);
+    } while (!str.empty());
+}
+
 [[nodiscard]] inline std::string_view TrimStringView(std::string_view str LIFETIMEBOUND, std::string_view pattern = " \f\n\r\t\v")
 {
     std::string::size_type front = str.find_first_not_of(pattern);
--- a/src/wallet/wallet.cpp
+++ b/src/wallet/wallet.cpp
@@ -2127,7 +2127,7 @@ void CWallet::CommitTransaction(
 )
 {
     LOCK(cs_wallet);
-    WalletLogPrintf("CommitTransaction:\n%s\n", util::RemoveSuffixView(tx->ToString(), "\n"));
+    SplitLines(strprintf("CommitTransaction:\n%s", tx->ToString()), [&](std::string_view line) { WalletLogPrintf("%s\n", line); });
 
     // Add tx to wallet, because if it has change it's also ours,
     // otherwise just for transaction history.

</details>

l0rinc force-pushed on Sep 15, 2026

l0rinc commented at 7:45 AM on September 15, 2026: contributor

Thanks @ryanofsky, rebased and replaced RPC-specific sanitization with global newline escaping, preserving printable method characters. Intentional multiline logs use the new SplitLines (rewritten slightly and covered with tests).

polespinasa commented at 9:19 AM on September 15, 2026: member

re-ACK aae82ddcc52a2f87a73077d150f9dd95ef7e572d

DrahtBot requested review from ryanofsky on Sep 15, 2026

DrahtBot requested review from achow101 on Sep 15, 2026

in src/util/exception.cpp:41 in aae82ddcc5

  35 | @@ -35,7 +36,10 @@ static std::string FormatException(const std::exception* pex, std::string_view t
  36 |  
  37 |  void PrintExceptionContinue(const std::exception* pex, std::string_view thread_name)
  38 |  {
  39 | +    LogWarning("");
  40 | +    LogWarning("");
  41 | +    LogWarning("************************");

ryanofsky commented at 1:36 PM on September 15, 2026:

In commit "log: escape newlines in messages" (aae82ddcc52a2f87a73077d150f9dd95ef7e572d)

Might be a little better to drop these separating lines for simplicity. If PrintExceptionContinue is called it seems reasonable to just log the exception as a normal warning. No strong opinion though (and the patch I posted did have these).

in src/util/string.h:188 in aae82ddcc5 outdated

 182 | @@ -183,6 +183,17 @@ std::vector<T> Split(std::span<const char> sp LIFETIMEBOUND, char sep, bool incl
 183 |      return str;
 184 |  }
 185 |  
 186 | +//! Visit each line without its newline, ignoring one trailing newline. Empty input yields one empty line.
 187 | +template <typename Fn>
 188 | +void SplitLines(std::string_view str, Fn&& fn)

ryanofsky commented at 2:01 PM on September 15, 2026:

In commit "log: escape newlines in messages" (aae82ddcc52a2f87a73077d150f9dd95ef7e572d)

Searching code and looking for \x0a in test log output showed one more place where I think it makes sense to use SplitLines:

Main one is walletdb.cpp:501 which can show multline messages from walletdb.cpp:781. It would seem good to use SplitLines there for readability since the message is not shown elsewhere.

Other places where SplitLines could be used but I think it's is better to not are:

common/init.cpp:90 which prints a multiline warning, but IMO better not to add SplitLines there because normally this text is an InitError and users have to explicitly set an option to downgrade it to a warning, so seems good for it to take up less space.
ipc/capnp/protocol.cpp:43 where multliline messages might have been logged before and will now use \x0a. But in this case I think it would be worse to use SplitLines, and \x0a is an improvement for safety reasons and grepping.
qt/bitcoin.cpp:187 and src/dbwrapper.cpp:105, and in tor/i2p prints as pointed out earlier, but these all seem like cases that should not use SplitLines because single lines are expected and output isn't necessarily trusted.

l0rinc commented at 9:21 PM on September 15, 2026:

Applied it to https://github.com/bitcoin/bitcoin/blob/57721f8074c2e8ccfe4c6d10114d4683dca58118/src/wallet/walletdb.cpp#L501 but kept https://github.com/bitcoin/bitcoin/blob/24efc8ecb758ee3146b0fba357f339a287527b3f/src/wallet/walletdb.cpp#L782 (let me know if you think we should do this as well, this one seemed less cleanly applicable.

ryanofsky commented at 2:52 PM on September 18, 2026:

(let me know if you think we should do this as well, this one seemed less cleanly applicable.

Thanks! Current code looks good, and matches my suggestion. I wasn't suggesting calling SpiltLines in walletdb.cpp:781. That wouldn't make sense because it is a strprintf, call not a log statement. I was only saying walletdb.cpp:501 should use SplitLines because walletdb.cpp:781 generates strings containing newlines.

ryanofsky approved

ryanofsky commented at 2:32 PM on September 15, 2026: contributor

Code review ACK aae82ddcc52a2f87a73077d150f9dd95ef7e572d. Thanks for picking up the escaping approach, this seems simpler than the per-site sanitizing and it covers more cases like the tor/i2p cases @davidgumberg mentioned as well.

I do think it would be a little better to drop the two wallet commits here (316599d3e7d0bcd5e9e7f12e7fa4a3cd57f736f2, c4ef1c0e3d7434c1b274f0eeb2e9fe1e5a55e8cb) and only keep the logging commits (f984c21cd1a8672105c90890c99ecfa7f17ca376, aae82ddcc52a2f87a73077d150f9dd95ef7e572d), just because the logging commits are sufficient to fix the reported problem, and restricting wallet names seems out of scope and something that deserves a wallet PR. But also fine to keep the wallet commits, I know they already received ACKs including from achow.

polespinasa commented at 2:40 PM on September 15, 2026: member

restricting wallet names seems out of scope and something that deserves a wallet PR.

This might be a good idea, there's already a PR trying to restrict some wallet names, which could do that job if the author wants to: #35768

I would be happy doing it here too tho

l0rinc force-pushed on Sep 15, 2026

l0rinc renamed this:
~~rpc,wallet: prevent user input injecting fake log lines~~
log: prevent user input from injecting fake log lines
on Sep 15, 2026

DrahtBot added the label Utils/log/libs on Sep 15, 2026

l0rinc commented at 9:18 PM on September 15, 2026: contributor

Thanks, updated to focus on logging, dropped the wallet-name restrictions, and kept the RPC and wallet log-injection regressions. Wallet names can still contain newlines on platforms that allow them, but restricting them for paths and UIs can be handled independently. The manual reproducer still shows the RPC injection path, but the solution is more general now.

in test/functional/wallet_createwallet.py:59 in 33abd9cf79

  55 | @@ -55,6 +56,11 @@ def run_test(self):
  56 |          assert_raises_rpc_error(-4, "Passphrase provided but private keys are disabled. A passphrase is only used to encrypt private keys, so cannot be used for wallets with private keys disabled.",
  57 |              self.nodes[0].createwallet, wallet_name='w0', disable_private_keys=True, passphrase="passphrase")
  58 |          assert_raises_rpc_error(-8, "Wallet name cannot be empty", self.nodes[0].createwallet, "")
  59 | +        if platform.system() != 'Windows':  # Windows disallows newlines in filenames

ryanofsky commented at 2:55 PM on September 18, 2026:

In commit "test: characterize RPC and wallet input logs" (33abd9cf79d36854495f8d9dc614d944a194bea9)

Not important, but it would seem better to check that windows disallows this instead of skipping the test on windows, more like:

wallet_name = "w0\ninvalid"
if platform.system() != 'Windows':
    with node.assert_debug_log([f"[{wallet_name}]".replace("\n", "\\x0a")]):
        assert_equal(node.createwallet(wallet_name)["name"], wallet_name)
    node.unloadwallet(wallet_name)
else:
    # Windows disallows newlines  in filenames
    assert_raises_rpc_error(None, None, node.createwallet, wallet_name)

in test/functional/rpc_whitelist.py:120 in 33abd9cf79

 112 | @@ -110,6 +113,15 @@ def run_test(self):
 113 |          self.test_users_permissions()
 114 |          self.test_rpcwhitelistdefault_permissions(1, 403)
 115 |  
 116 | +    def test_rejected_method_logging(self):
 117 | +        """Test logging a method rejected by the RPC whitelist."""
 118 | +        self.log.info(f"[{self.users[0][0]}]: Testing rejected method logging")
 119 | +        forged_log_line = "ERROR: ConnectTip: ConnectBlock 0000000000000000deadbeefdeadbeefdeadbeefdeadbeefdeadbeefdeadbeef failed, bad-txns-inputs-missingorspent"
 120 | +        for rejected_method in [f"getblock\n{forged_log_line}", "getblock&"]:

ryanofsky commented at 3:20 PM on September 18, 2026:

In commit "test: characterize RPC and wallet input logs" (33abd9cf79d36854495f8d9dc614d944a194bea9)

Testing getblock& is reasonable, but I think would be confusing to anyone looking at this code knowing why this case is here or why the behavior is good. Not important, but would suggest adding more special characters and a comment

# Test special characters to ensure full method name is logged, not a
# SanitizeString()-mangled one which could be misleading
for rejected_method in [f"getblock\n{forged_log_line}", 'getblock!"#$%&\'*+<=>[\\]^`{|}~']:

ryanofsky commented at 3:21 PM on September 18, 2026: contributor

Code review ACK 54c71d72e842a7286b81db3275368a5f43a8a7df. Thanks for the updates!

Please ignore very minor comments below unless this needs to be updated for another reason.

DrahtBot requested review from polespinasa on Sep 18, 2026

l0rinc force-pushed on Sep 18, 2026

l0rinc commented at 6:59 PM on September 18, 2026: contributor

Thanks @ryanofsky, I liked both ideas, so I added the Windows and extra-character coverage.

ryanofsky approved

ryanofsky commented at 4:48 PM on September 21, 2026: contributor

Code review ACK 0321f720124e271931d934c7da455da9e99013c2. Just extending tests a little to cover more special character & windows behavior as suggested (thanks!)

w0xlt commented at 8:42 PM on September 26, 2026: contributor

I think this route still fails: an RPC user creates a wallet named w5\n<fake line> with load_on_startup=true. If that path is missing at the next startup (e.g. under a /tmp cleared on reboot), VerifyWallets warns with the path embedded in the message, and debug.log shows:

[init] [noui.cpp:34] [operator()] [warning] Skipping -wallet path that doesn't exist. Failed to load database path '<datadir>/wallets/w5
[init] [noui.cpp:34] [operator()] [warning] ERROR: ConnectTip: ConnectBlock 0000…deadbeef failed, bad-txns-inputs-missingorspent'. Path does not exist.

The timestamp and [warning] prefix are real, but the rest of the second line comes from the wallet name. This route was still blocked at aae82dd, because c4ef1c0e3d rejected control characters in wallet names. It opened in 54c71d7, when that commit was dropped but the noui split stayed.

Suggested fix:

diff --git a/src/noui.cpp b/src/noui.cpp
index 7c93f36f2c..6a222ff055 100644
--- a/src/noui.cpp
+++ b/src/noui.cpp
@@ -8,7 +8,6 @@
 #include <node/interface_ui.h>
 #include <util/btcsignals.h>
 #include <util/log.h>
-#include <util/string.h>
 #include <util/translation.h>
 
 #include <string>
@@ -27,18 +26,18 @@ void noui_ThreadSafeMessageBox(const bilingual_str& message, unsigned int style)
     switch (style) {
     case CClientUIInterface::MSG_ERROR:
         strCaption = "Error: ";
-        if (!fSecure) util::SplitLines(message.original, [](auto line) { LogError("%s", line); });
+        if (!fSecure) LogError("%s", message.original);
         break;
     case CClientUIInterface::MSG_WARNING:
         strCaption = "Warning: ";
-        if (!fSecure) util::SplitLines(message.original, [](auto line) { LogWarning("%s", line); });
+        if (!fSecure) LogWarning("%s", message.original);
         break;
     case CClientUIInterface::MSG_INFORMATION:
         strCaption = "Information: ";
-        if (!fSecure) util::SplitLines(message.original, [](auto line) { LogInfo("%s", line); });
+        if (!fSecure) LogInfo("%s", message.original);
         break;
     default:
-        if (!fSecure) util::SplitLines(message.original, [&](auto line) { LogInfo("%s%s", strCaption, line); });
+        if (!fSecure) LogInfo("%s%s", strCaption, message.original);
     }
 
     tfm::format(std::cerr, "%s%s\n", strCaption, message.original);
diff --git a/test/functional/wallet_startup.py b/test/functional/wallet_startup.py
index ed07bb044a..0417e78702 100755
--- a/test/functional/wallet_startup.py
+++ b/test/functional/wallet_startup.py
@@ -7,6 +7,7 @@
 Verify that a bitcoind node can maintain list of wallets loading on startup
 """
 import os
+import platform
 import shutil
 import stat
 import uuid
@@ -93,6 +94,27 @@ class WalletStartupTest(BitcoinTestFramework):
         self.start_node(0)
         assert_equal(set(node.listwallets()), {'w2', 'w3'})
 
+    def test_startup_warning_log_injection(self, node):
+        self.log.info("Test that a wallet name can't forge log lines through startup warnings")
+        if platform.system() == 'Windows':
+            self.log.info("Skipping: Windows disallows newlines in filenames")
+            return
+        forged_log_line = "ERROR: ConnectTip: ConnectBlock 0000000000000000deadbeefdeadbeefdeadbeefdeadbeefdeadbeefdeadbeef failed, bad-txns-inputs-missingorspent"
+        wallet_name = f"w5\n{forged_log_line}"
+        # An RPC user can persist an arbitrary wallet name in settings.json
+        assert_equal(node.createwallet(wallet_name=wallet_name, load_on_startup=True)["name"], wallet_name)
+        self.stop_node(0)
+
+        # If the wallet path disappears (e.g. under a /tmp cleared on reboot),
+        # startup warns with the wallet path embedded in the message
+        wallet_path = node.wallets_path / wallet_name
+        shutil.rmtree(wallet_path)
+        warning = f"Skipping -wallet path that doesn't exist. Failed to load database path '{wallet_path}'. Path does not exist."
+        with node.assert_debug_log([warning.replace("\n", "\\x0a")], unexpected_msgs=[f"[warning] {forged_log_line}"]):
+            self.start_node(0)
+        # Console output bypasses the logger's escaping
+        self.stop_node(0, expected_stderr=f"Warning: {warning}")
+
     def run_test(self):
         self.log.info('Should start without any wallets')
         assert_equal(self.nodes[0].listwallets(), [])
@@ -124,6 +146,7 @@ class WalletStartupTest(BitcoinTestFramework):
 
         self.test_load_unwritable_wallet(self.nodes[0])
         self.test_disabled_settings(self.nodes[0])
+        self.test_startup_warning_log_injection(self.nodes[0])
 
 if __name__ == '__main__':
     WalletStartupTest(__file__).main()

</details>

ryanofsky commented at 1:30 AM on September 27, 2026: contributor

re: #35833 (comment)

I think this route still fails

Nice find. The actual bugfix commit 0321f720124e271931d934c7da455da9e99013c2 does fully fix the original problem and prevents unescaped newlines from being output by log statements, making log functions safe to use with untrusted input.

But the preceding commit 5dff09c040c0c63c56d9794e35c96dde4822ae4e introducing SplitLines creates a smaller-scale version of the original problem because SplitLines is not safe to use with untrusted input. Would suggest fixing this by just dropping commit 5dff09c040c0c63c56d9794e35c96dde4822ae4e and simplifying the PR. SplitLines was intended to make log output look a little nicer in some cases by preserving newlines, but I think your example shows it is not actually safe in practice and not worth keeping.

l0rinc force-pushed on Sep 28, 2026

DrahtBot added the label CI failed on Sep 28, 2026

DrahtBot commented at 1:38 AM on September 28, 2026: contributor

🚧 At least one of the CI tasks failed. Task lint: https://github.com/bitcoin/bitcoin/actions/runs/36364312910/job/108747569383 LLM reason (✨ experimental): CI failed because the lint check for unsafe shutil.rmtree() usage in test/functional/wallet_startup.py (check rmtree) reported a violation.

<details><summary>Hints</summary>

Try to run the tests locally, according to the documentation. However, a CI failure may still happen due to a number of reasons, for example:

Possibly due to a silent merge conflict (the changes in this pull request being incompatible with the current code in the target branch). If so, make sure to rebase on the latest commit of the target branch.
A sanitizer issue, which can only be found by compiling with the sanitizer and running the affected test.
An intermittent issue.

Leave a comment here, if you need help tracking down a confusing failure.

</details>

test: characterize RPC and wallet input logs

Co-authored-by: Ryan Ofsky <ryan@ofsky.org>
Co-authored-by: w0xlt <94266259+w0xlt@users.noreply.github.com>

960dcdb625

log: escape newlines in messages

Escape embedded newlines as \x0a in the logger so untrusted input cannot forge additional log lines. Preserve printable method characters, UTF-8, and wallet naming behavior.

Remove one conventional trailing newline before escaping and append the log terminator centrally.

Cover rejected RPC methods, wallet creation, and the startup warning for a persisted wallet path that has disappeared.

Co-authored-by: Ryan Ofsky <ryan@ofsky.org>
Co-authored-by: w0xlt <94266259+w0xlt@users.noreply.github.com>

d93d366e20

l0rinc force-pushed on Sep 28, 2026

l0rinc commented at 2:37 AM on September 28, 2026: contributor

Dropped SplitLines as suggested, added @w0xlt’s wallet startup regression, rebased, and updated Russ’s coauthor email to ryan@ofsky.org.

DrahtBot removed the label CI failed on Sep 28, 2026

ryanofsky approved

ryanofsky commented at 8:32 PM on September 29, 2026: contributor

Code review ACK d93d366e204e95e873ef0b89425f84a520f8fabd. Fix is minimal now and tests have been expanded to ensure that newlines can't sneak into log messages through InitWarning or InitError calls by testing the load_on_startup case w0xlt reported #35833 (comment).

Suggestion: maybe drop sentence "Authorization still uses the original method string, and wallet naming and loading behavior remain unchanged." from the PR description. It seems confusing unless you know the previous history of the PR, because you wouldn't expect a logging change to affect authorization or wallet naming behavior.

w0xlt commented at 9:46 PM on September 29, 2026: contributor

ACK d93d366e204e95e873ef0b89425f84a520f8fabd

achow101 commented at 9:56 PM on September 30, 2026: member

ACK d93d366e204e95e873ef0b89425f84a520f8fabd

achow101 merged this on Sep 30, 2026

achow101 closed this on Sep 30, 2026