From 4efaa6763a7eb3614b78fa09e676fe0630c34678 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?L=C5=91rinc?= Date: Thu, 20 Aug 2026 12:37:17 -0700 Subject: [PATCH 1/5] test: simplify `ReplaceAll` coverage Let each case provide its input so strings outside the original fixture can use the same table without separate temporary variables. --- src/test/util_tests.cpp | 15 +++++++-------- 1 file changed, 7 insertions(+), 8 deletions(-) diff --git a/src/test/util_tests.cpp b/src/test/util_tests.cpp index 90510e3dec0..a2125c4ae1c 100644 --- a/src/test/util_tests.cpp +++ b/src/test/util_tests.cpp @@ -300,17 +300,16 @@ 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, const std::string& search, const std::string& substitute, const std::string& 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, 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."); } BOOST_AUTO_TEST_CASE(util_TrimString) From 604d7e8fdd95d22203120ca15d98d89f8668c240 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?L=C5=91rinc?= Date: Sat, 15 Aug 2026 12:10:07 -0700 Subject: [PATCH 2/5] test: characterize walletnotify shell injection `-walletnotify` shell-escapes wallet names before substituting `%w` into the configured command. `ReplaceAll()` uses `%w` as the regex pattern and the escaped wallet name as replacement text, where `$'` copies the command suffix into the escaped name and allows its shell metacharacters to alter the command. Record the command execution, missing notification file, regex pattern matching, replacement expansion, and non-recursive replacement. --- src/test/util_tests.cpp | 4 ++++ test/functional/feature_notifications.py | 14 ++++++++++++++ 2 files changed, 18 insertions(+) diff --git a/src/test/util_tests.cpp b/src/test/util_tests.cpp index a2125c4ae1c..2ce1ef6463e 100644 --- a/src/test/util_tests.cpp +++ b/src/test/util_tests.cpp @@ -306,10 +306,14 @@ BOOST_AUTO_TEST_CASE(util_ReplaceAll) }}; 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", "xxx"); // TODO: Search text must remain literal. + test_replaceall("%w and %w", "%w", "$&$`$'$1$$", "%w and %w$ and %w and $"); // TODO: Replacement bytes must remain literal. + test_replaceall("x", "x", "xx", "xx"); } BOOST_AUTO_TEST_CASE(util_TrimString) diff --git a/test/functional/feature_notifications.py b/test/functional/feature_notifications.py index e983d5a2b83..fb573d3af98 100755 --- a/test/functional/feature_notifications.py +++ b/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 os.path.exists(command_marker) # TODO: Wallet names must not inject shell commands. + assert not os.path.exists(notify_path) # TODO: Wallet names must remain literal in notification paths. + self.log.info("test -alertnotify with large work invalid chain") # create a bunch of invalid blocks tip = self.nodes[0].getbestblockhash() From 469b0e59a29ac48364c20268feeb4477b8ec9192 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?L=C5=91rinc?= Date: Sat, 15 Aug 2026 12:11:51 -0700 Subject: [PATCH 3/5] util: make `ReplaceAll` literal `ReplaceAll()` substitutes fixed tokens in notification commands and other strings. PR #25803 replaced the Boost helper with `std::regex_replace()`, treating searches as regular expressions and substitutes as replacement-format syntax. Restore literal, non-recursive replacement so callers match fixed tokens and preserve replacement bytes exactly, while avoiding a new string when the search text is absent. Co-authored-by: Rob Hamilton <6456095+Rob1Ham@users.noreply.github.com> --- src/test/util_tests.cpp | 4 ++-- src/util/string.cpp | 15 +++++++++++++-- src/util/string.h | 1 + test/functional/feature_notifications.py | 4 ++-- 4 files changed, 18 insertions(+), 6 deletions(-) diff --git a/src/test/util_tests.cpp b/src/test/util_tests.cpp index 2ce1ef6463e..a7ba38b6d04 100644 --- a/src/test/util_tests.cpp +++ b/src/test/util_tests.cpp @@ -311,8 +311,8 @@ BOOST_AUTO_TEST_CASE(util_ReplaceAll) 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", "xxx"); // TODO: Search text must remain literal. - test_replaceall("%w and %w", "%w", "$&$`$'$1$$", "%w and %w$ and %w and $"); // TODO: Replacement bytes must remain literal. + test_replaceall("a.b", ".", "x", "axb"); + test_replaceall("%w and %w", "%w", "$&$`$'$1$$", "$&$`$'$1$$ and $&$`$'$1$$"); test_replaceall("x", "x", "xx", "xx"); } diff --git a/src/util/string.cpp b/src/util/string.cpp index d9d59ef5717..c197076715d 100644 --- a/src/util/string.cpp +++ b/src/util/string.cpp @@ -6,7 +6,6 @@ #include #include -#include #include #include @@ -14,7 +13,19 @@ namespace util { void ReplaceAll(std::string& in_out, const std::string& search, const std::string& 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) diff --git a/src/util/string.h b/src/util/string.h index c984960a5d6..0963c12d698 100644 --- a/src/util/string.h +++ b/src/util/string.h @@ -98,6 +98,7 @@ struct ConstevalFormatString { consteval ConstevalFormatString(const char* str) : fmt{str} { detail::CheckNumFormatSpecifiers(fmt); } }; +/// 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, const std::string& search, const std::string& substitute); /** Split a string on any char found in separators, returning a vector. diff --git a/test/functional/feature_notifications.py b/test/functional/feature_notifications.py index fb573d3af98..11d22356895 100755 --- a/test/functional/feature_notifications.py +++ b/test/functional/feature_notifications.py @@ -186,8 +186,8 @@ class NotificationsTest(BitcoinTestFramework): 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 os.path.exists(command_marker) # TODO: Wallet names must not inject shell commands. - assert not os.path.exists(notify_path) # TODO: Wallet names must remain literal in notification paths. + 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 From 1f9dfabef64121c5ea030f5dbaaaf2a6af706eef Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?L=C5=91rinc?= Date: Thu, 20 Aug 2026 13:11:34 -0700 Subject: [PATCH 4/5] refactor: use string views in `ReplaceAll` PR #25803 changed these parameters to `const std::string&` for `std::regex_replace()`. The literal implementation no longer needs owned strings, so restore the original `std::string_view` interface. --- src/test/util_tests.cpp | 2 +- src/util/string.cpp | 3 ++- src/util/string.h | 2 +- 3 files changed, 4 insertions(+), 3 deletions(-) diff --git a/src/test/util_tests.cpp b/src/test/util_tests.cpp index a7ba38b6d04..413236f4552 100644 --- a/src/test/util_tests.cpp +++ b/src/test/util_tests.cpp @@ -300,7 +300,7 @@ BOOST_AUTO_TEST_CASE(util_Join) BOOST_AUTO_TEST_CASE(util_ReplaceAll) { const std::string original("A test \"%s\" string '%s'."); - auto test_replaceall{[](std::string test, const std::string& search, const std::string& substitute, const std::string& expected) { + 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); }}; diff --git a/src/util/string.cpp b/src/util/string.cpp index c197076715d..3c84a487e95 100644 --- a/src/util/string.cpp +++ b/src/util/string.cpp @@ -8,9 +8,10 @@ #include #include #include +#include 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; auto pos{in_out.find(search)}; diff --git a/src/util/string.h b/src/util/string.h index 0963c12d698..265bf987e50 100644 --- a/src/util/string.h +++ b/src/util/string.h @@ -99,7 +99,7 @@ struct ConstevalFormatString { }; /// 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, const std::string& search, const std::string& substitute); +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. * From db39de5601094dc3f0b15ce4759e1b88025403c2 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?L=C5=91rinc?= Date: Tue, 1 Sep 2026 11:30:27 -0700 Subject: [PATCH 5/5] doc: add `-walletnotify` security note Co-authored-by: maflcko <6399679+maflcko@users.noreply.github.com> --- doc/release-notes-36048.md | 8 ++++++++ 1 file changed, 8 insertions(+) create mode 100644 doc/release-notes-36048.md diff --git a/doc/release-notes-36048.md b/doc/release-notes-36048.md new file mode 100644 index 00000000000..dc912207707 --- /dev/null +++ b/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)