Merge bitcoin/bitcoin#36048: util: keep wallet names literal in notification commands

db39de5601 doc: add `-walletnotify` security note (Lőrinc)
1f9dfabef6 refactor: use string views in `ReplaceAll` (Lőrinc)
469b0e59a2 util: make `ReplaceAll` literal (Lőrinc)
604d7e8fdd test: characterize walletnotify shell injection (Lőrinc)
4efaa6763a test: simplify `ReplaceAll` coverage (Lőrinc)

Pull request description:

  **Problem:** On non-Windows builds, operators can configure `-walletnotify` to run a command for wallet transactions, with `%w` replaced by the shell-escaped wallet name.
  An authenticated RPC caller allowed to create wallets can supply a name containing `$'`, request an address, and send a transaction to it.
  While replacing `%w`, `ReplaceAll()` passes the escaped wallet name to `std::regex_replace()` as replacement text.
  There, `$'` copies the command suffix into the escaped name, breaking its quote accounting and allowing shell metacharacters in the wallet name to alter the command.
  `runCommand()` passes the result to `system()`, so a suitable command template could execute additional shell commands as the node process account.
  It is not reachable over P2P or by an unauthenticated network peer.
  #25803 introduced this behavior in v24 when it replaced Boost's literal substitution with `std::regex_replace()`.

  **Fix:** Restore the literal, non-recursive contract `ReplaceAll()` had before #25803, matching every current caller's literal search and replacement text, while the wallet notification test covers a wallet name containing `$'`.

  **Related:** #35833 restricts control characters in new wallet names, while this change fixes replacement metacharacters in `ReplaceAll()`.

  This was found and disclosed responsibly by the Red Team 🟥.

ACKs for top commit:
  maflcko:
    re-ACK db39de5601 💈
  jeanpablojp:
    re-ACK db39de5601
  stickies-v:
    re-ACK db39de5601

Tree-SHA512: 0be4adecfee50cb4dab90ae3386079767694a6b1fa1d7bd1f10ef73de88707b232f1ba4975a723c465a4d34d12296d501986c657d93bd8ae0bdced16afad1b5e

doc/release-notes-36048.md

@@ -0,0 +1,8 @@
+Wallet
+------
+
+* On non-Windows systems, an authenticated RPC caller allowed to create wallets
+  could execute arbitrary commands as the node process account when
+  `-walletnotify` was configured, by crafting a wallet name with regex
+  replacement characters. Wallet notification placeholder replacement now
+  treats wallet names literally. (#36048)

src/test/util_tests.cpp

@@ -300,17 +300,20 @@ BOOST_AUTO_TEST_CASE(util_Join)
 BOOST_AUTO_TEST_CASE(util_ReplaceAll)
 {
     const std::string original("A test \"%s\" string '%s'.");
-    auto test_replaceall = [&original](const std::string& search, const std::string& substitute, const std::string& expected) {
-        auto test = original;
+    auto test_replaceall{[](std::string test, std::string_view search, std::string_view substitute, std::string_view expected) {
         ReplaceAll(test, search, substitute);
         BOOST_CHECK_EQUAL(test, expected);
-    };
+    }};
 
-    test_replaceall("", "foo", original);
-    test_replaceall(original, "foo", "foo");
-    test_replaceall("%s", "foo", "A test \"foo\" string 'foo'.");
-    test_replaceall("\"", "foo", "A test foo%sfoo string '%s'.");
-    test_replaceall("'", "foo", "A test \"%s\" string foo%sfoo.");
+    test_replaceall(original, "", "foo", original);
+    test_replaceall(original, "missing", "foo", original);
+    test_replaceall(original, original, "foo", "foo");
+    test_replaceall(original, "%s", "foo", "A test \"foo\" string 'foo'.");
+    test_replaceall(original, "\"", "foo", "A test foo%sfoo string '%s'.");
+    test_replaceall(original, "'", "foo", "A test \"%s\" string foo%sfoo.");
+    test_replaceall("a.b", ".", "x", "axb");
+    test_replaceall("%w and %w", "%w", "$&$`$'$1$$", "$&$`$'$1$$ and $&$`$'$1$$");
+    test_replaceall("x", "x", "xx", "xx");
 }
 
 BOOST_AUTO_TEST_CASE(util_TrimString)

src/util/string.cpp

@@ -6,15 +6,27 @@
 
 #include <iterator>
 #include <memory>
-#include <regex>
 #include <stdexcept>
 #include <string>
+#include <string_view>
 
 namespace util {
-void ReplaceAll(std::string& in_out, const std::string& search, const std::string& substitute)
+void ReplaceAll(std::string& in_out, std::string_view search, std::string_view substitute)
 {
     if (search.empty()) return;
-    in_out = std::regex_replace(in_out, std::regex(search), substitute);
+    auto pos{in_out.find(search)};
+    if (pos == std::string::npos) return;
+
+    // Build separately because repeated std::string::replace() calls move the remaining suffix when sizes differ
+    std::string result;
+    result.reserve(in_out.size());
+    std::string::size_type start{0};
+    for (; pos != std::string::npos; pos = in_out.find(search, start)) {
+        result.append(in_out, start, pos - start).append(substitute);
+        start = pos + search.size();
+    }
+    result.append(in_out, start);
+    in_out.swap(result);
 }
 
 LineReader::LineReader(std::string_view str, size_t max_line_length)

src/util/string.h

@@ -98,7 +98,8 @@ struct ConstevalFormatString {
     consteval ConstevalFormatString(const char* str) : fmt{str} { detail::CheckNumFormatSpecifiers<num_params>(fmt); }
 };
 
-void ReplaceAll(std::string& in_out, const std::string& search, const std::string& substitute);
+/// Replace every non-overlapping occurrence of `search` with `substitute`, treating both literally; the replacement text is not searched again.
+void ReplaceAll(std::string& in_out, std::string_view search, std::string_view substitute);
 
 /** Split a string on any char found in separators, returning a vector.
  *

test/functional/feature_notifications.py

@@ -42,6 +42,7 @@ class NotificationsTest(BitcoinTestFramework):
         self.num_nodes = 2
         self.setup_clean_chain = True
         self.uses_wallet = None
+        self.noban_tx_relay = True
 
     def setup_network(self):
         self.wallet = ''.join(chr(i) for i in range(FILE_CHAR_START, FILE_CHAR_END) if chr(i) not in FILE_CHARS_DISALLOWED)
@@ -175,6 +176,19 @@ class NotificationsTest(BitcoinTestFramework):
             self.expect_wallet_notify([(bump2, blockheight2, blockhash2), (tx2, -1, UNCONFIRMED_HASH_STRING)])
             assert_equal(self.nodes[1].gettransaction(bump2)["confirmations"], 1)
 
+            if platform.system() != 'Windows':
+                self.log.info("test -walletnotify replacement metacharacters in wallet name")
+                self.nodes[1].unloadwallet(self.wallet)
+                command_marker = os.path.join(self.options.tmpdir, "walletnotify_injected")
+                # The previous regex replacement expanded `$'` to the command suffix, breaking the shell-escaped wallet name's quote accounting
+                wallet_name = self.nodes[1].createwallet(f"$'$'; echo Pwned > {os.path.basename(command_marker)}; #")["name"]
+                txid = self.nodes[0].sendtoaddress(self.nodes[1].get_wallet_rpc(wallet_name).getnewaddress(), 1)
+                self.sync_mempools()
+                notify_path = os.path.join(self.walletnotify_dir, notify_outputname(wallet_name, txid))
+                self.wait_until(lambda: os.path.exists(command_marker) or os.path.exists(notify_path), timeout=10)
+                assert not os.path.exists(command_marker)
+                assert os.path.exists(notify_path)
+
         self.log.info("test -alertnotify with large work invalid chain")
         # create a bunch of invalid blocks
         tip = self.nodes[0].getbestblockhash()