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) diff --git a/src/test/util_tests.cpp b/src/test/util_tests.cpp index 90510e3dec0..413236f4552 100644 --- a/src/test/util_tests.cpp +++ b/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) diff --git a/src/util/string.cpp b/src/util/string.cpp index d9d59ef5717..3c84a487e95 100644 --- a/src/util/string.cpp +++ b/src/util/string.cpp @@ -6,15 +6,27 @@ #include #include -#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; - 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..265bf987e50 100644 --- a/src/util/string.h +++ b/src/util/string.h @@ -98,7 +98,8 @@ struct ConstevalFormatString { consteval ConstevalFormatString(const char* str) : fmt{str} { detail::CheckNumFormatSpecifiers(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. * diff --git a/test/functional/feature_notifications.py b/test/functional/feature_notifications.py index e983d5a2b83..11d22356895 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 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()