From 2b0dc0d2280b48960a3510b1dfda05a1929bb3dd Mon Sep 17 00:00:00 2001 From: Ava Chow Date: Mon, 9 Feb 2026 15:19:41 -0800 Subject: [PATCH 1/3] wallet: Disallow . and .. from wallet names Wallet names that are also paths that contain . and .. are unintuitive and can result in unexpected behavior, particularly in migration. Therefore we should disallow users from specifying wallet names that contain . and .. as path elements. --- src/wallet/wallet.cpp | 21 +++++- test/functional/wallet_migration.py | 105 +------------------------- test/functional/wallet_multiwallet.py | 17 +++++ 3 files changed, 38 insertions(+), 105 deletions(-) diff --git a/src/wallet/wallet.cpp b/src/wallet/wallet.cpp index d25c39128b2..49099bf70cd 100644 --- a/src/wallet/wallet.cpp +++ b/src/wallet/wallet.cpp @@ -2942,17 +2942,34 @@ bool CWallet::EraseAddressReceiveRequest(WalletBatch& batch, const CTxDestinatio static util::Result GetWalletPath(const std::string& name) { + const fs::path name_path = fs::PathFromString(name); + + // 'name' must be a normalized path, i.e. no . or .. except at the root + if (name_path != name_path.lexically_normal()) { + return util::Error{Untranslated("Wallet name given as a path must be normalized")}; + } + + // 'name' cannot begin with ./ or ../ + if (!name_path.empty() && (*name_path.begin() == fs::PathFromString(".") || *name_path.begin() == fs::PathFromString(".."))) { + return util::Error{Untranslated("Wallet name given as a relative path cannot begin with ./ or ../, for wallets not in the walletdir, please use an absolute path.")}; + } + + // Disallow path at root + if (name_path.has_root_path() && name_path.root_path() == name_path) { + return util::Error{Untranslated("Wallet name cannot be the root path")}; + } + // Do some checking on wallet path. It should be either a: // // 1. Path where a directory can be created. // 2. Path to an existing directory. // 3. Path to a symlink to a directory. // 4. For backwards compatibility, the name of a data file in -walletdir. - const fs::path wallet_path = fsbridge::AbsPathJoin(GetWalletDir(), fs::PathFromString(name)); + const fs::path wallet_path = fsbridge::AbsPathJoin(GetWalletDir(), name_path); fs::file_type path_type = fs::symlink_status(wallet_path).type(); if (!(path_type == fs::file_type::not_found || path_type == fs::file_type::directory || (path_type == fs::file_type::symlink && fs::is_directory(wallet_path)) || - (path_type == fs::file_type::regular && fs::PathFromString(name).filename() == fs::PathFromString(name)))) { + (path_type == fs::file_type::regular && name_path.filename() == name_path))) { return util::Error{Untranslated(strprintf( "Invalid -wallet path '%s'. -wallet path should point to a directory where wallet.dat and " "database/log.?????????? files can be stored, a location where such a directory could be created, " diff --git a/test/functional/wallet_migration.py b/test/functional/wallet_migration.py index d4489eaa1c9..e28cfd578c5 100755 --- a/test/functional/wallet_migration.py +++ b/test/functional/wallet_migration.py @@ -599,57 +599,6 @@ class WalletMigrationTest(BitcoinTestFramework): assert_equal(bals, wallet.getbalances()) - def test_wallet_with_relative_path(self): - self.log.info("Test migration of a wallet that isn't loaded, specified by a relative path") - - # Get the nearest common path of both nodes' wallet paths. - common_parent = os.path.commonpath([self.master_node.wallets_path, self.old_node.wallets_path]) - - # This test assumes that the relative path from each wallet directory to the common path is identical. - assert_equal(os.path.relpath(common_parent, start=self.master_node.wallets_path), os.path.relpath(common_parent, start=self.old_node.wallets_path)) - - wallet_name = "relative" - absolute_path = os.path.abspath(os.path.join(common_parent, wallet_name)) - relative_name = os.path.relpath(absolute_path, start=self.master_node.wallets_path) - - wallet = self.create_legacy_wallet(relative_name) - # listwalletdirs only returns wallets in the wallet directory - assert {"name": relative_name} not in wallet.listwalletdir()["wallets"] - assert relative_name in wallet.listwallets() - - default = self.master_node.get_wallet_rpc(self.default_wallet_name) - addr = wallet.getnewaddress() - txid = default.sendtoaddress(addr, 1) - self.generate(self.master_node, 1) - bals = wallet.getbalances() - bals["mine"]["nonmempool"] = Decimal('0.0') - - migrate_res, wallet = self.migrate_and_get_rpc(relative_name) - - # Check that the wallet was migrated, knows the right txid, and has the right balance. - assert wallet.gettransaction(txid) - assert_equal(bals, wallet.getbalances()) - - # The migrated wallet should not be in the wallet dir, but should be in the list of wallets. - info = wallet.getwalletinfo() - - walletdirlist = wallet.listwalletdir() - assert {"name": info["walletname"]} not in walletdirlist["wallets"] - - walletlist = wallet.listwallets() - assert info["walletname"] in walletlist - - # Check that old node can restore from the backup. - self.old_node.restorewallet("relative_restored", migrate_res['backup_path']) - wallet = self.old_node.get_wallet_rpc("relative_restored") - assert wallet.gettransaction(txid) - del bals["mine"]["nonmempool"] - assert_equal(bals, wallet.getbalances()) - - info = wallet.getwalletinfo() - assert_equal(info["descriptors"], False) - assert_equal(info["format"], "bdb") - def test_wallet_with_path(self, wallet_path): self.log.info("Test migrating a wallet with the following path/name: %s", wallet_path) # the wallet data is actually inside of path/that/ends/ @@ -1122,53 +1071,6 @@ class WalletMigrationTest(BitcoinTestFramework): # Check the wallet we tried to migrate is still BDB self.assert_is_bdb("failed") - def test_failed_migration_cleanup_relative_path(self): - self.log.info("Test that a failed migration with a relative path is cleaned up") - - # Get the nearest common path of both nodes' wallet paths. - common_parent = os.path.commonpath([self.master_node.wallets_path, self.old_node.wallets_path]) - - # This test assumes that the relative path from each wallet directory to the common path is identical. - assert_equal(os.path.relpath(common_parent, start=self.master_node.wallets_path), os.path.relpath(common_parent, start=self.old_node.wallets_path)) - - wallet_name = "relativefailure" - absolute_path = os.path.abspath(os.path.join(common_parent, wallet_name)) - relative_name = os.path.relpath(absolute_path, start=self.master_node.wallets_path) - - wallet = self.create_legacy_wallet(relative_name) - - # Make a copy of the wallet with the solvables wallet name so that we are unable - # to create the solvables wallet when migrating, thus failing to migrate - wallet.unloadwallet() - solvables_path = os.path.join(common_parent, f"{wallet_name}_solvables") - - shutil.copytree(self.old_node.wallets_path / relative_name, solvables_path) - original_shasum = sha256sum_file(os.path.join(solvables_path, "wallet.dat")) - - self.old_node.loadwallet(relative_name) - - # Add a multisig so that a solvables wallet is created - wallet.addmultisigaddress(2, [wallet.getnewaddress(), get_generate_key().pubkey]) - wallet.importaddress(get_generate_key().p2pkh_addr) - - self.old_node.unloadwallet(relative_name) - assert_raises_rpc_error(-4, "Failed to create database", self.master_node.migratewallet, relative_name) - - assert all(wallet not in self.master_node.listwallets() for wallet in [f"{wallet_name}", f"{wallet_name}_watchonly", f"{wallet_name}_solvables"]) - - assert not (self.master_node.wallets_path / f"{wallet_name}_watchonly").exists() - # Since the file in failed_solvables is one that we put there, migration shouldn't touch it - assert os.path.exists(solvables_path) - new_shasum = sha256sum_file(os.path.join(solvables_path , "wallet.dat")) - assert_equal(original_shasum, new_shasum) - - # Check the wallet we tried to migrate is still BDB - datfile = os.path.join(absolute_path, "wallet.dat") - with open(datfile, "rb") as f: - data = f.read(16) - _, _, magic = struct.unpack("QII", data) - assert_equal(magic, BTREE_MAGIC) - def test_blank(self): self.log.info("Test that a blank wallet is migrated") wallet = self.create_legacy_wallet("blank", blank=True) @@ -1687,13 +1589,11 @@ class WalletMigrationTest(BitcoinTestFramework): self.test_encrypted() self.test_nonexistent() self.test_unloaded_by_path() - self.test_wallet_with_relative_path() - self.test_wallet_with_path("path/to/mywallet/") - self.test_wallet_with_path("path/that/ends/in/..") + self.test_wallet_with_path("path/to/trailing/") + self.test_wallet_with_path("path/to/mywallet") migration_failure_cases = [ "", - "../", os.path.abspath(self.master_node.datadir_path / "absolute_path"), "normallynamedwallet" ] @@ -1708,7 +1608,6 @@ class WalletMigrationTest(BitcoinTestFramework): self.test_conflict_txs() self.test_hybrid_pubkey() self.test_failed_migration_cleanup() - self.test_failed_migration_cleanup_relative_path() self.test_avoidreuse() self.test_preserve_tx_extra_info() self.test_blank() diff --git a/test/functional/wallet_multiwallet.py b/test/functional/wallet_multiwallet.py index 5b9a23b593f..6240126ccba 100755 --- a/test/functional/wallet_multiwallet.py +++ b/test/functional/wallet_multiwallet.py @@ -87,6 +87,8 @@ class MultiWalletTest(BitcoinTestFramework): assert_equal(node.listwalletdir(), {'wallets': [{'name': self.default_wallet_name, "warnings": []}]}) + self.test_invalid_wallet_names() + # check wallet.dat is created self.stop_nodes() assert_equal(os.path.isfile(wallet_dir(node, self.default_wallet_name, self.wallet_data_filename)), True) @@ -458,6 +460,21 @@ class MultiWalletTest(BitcoinTestFramework): node.unloadwallet(wallet) self.nodes[1].loadwallet(wallet) + def test_invalid_wallet_names(self): + self.log.info("Test weird paths are not allowed as wallet names") + NON_NORMALIZED = ["bad/./path", "bad/../path", "/bad/./path", "/bad/../path", "../", "./", "./wallet", "../wallets/../wallets/wallet"] + for name in NON_NORMALIZED: + assert_raises_rpc_error(-4, "Wallet name given as a path must be normalized", self.nodes[0].createwallet, name) + + INVALID_RELPATH = ["../wallets/wallet", "..", "."] + for name in INVALID_RELPATH: + assert_raises_rpc_error(-4, "Wallet name given as a relative path cannot begin with ./ or ../", self.nodes[0].createwallet, name) + + INVALID_ROOT = ["/"] + if platform.system() == "Windows": + INVALID_ROOT.extend(["C:\\", "C:"]) + for name in INVALID_ROOT: + assert_raises_rpc_error(-4, "Wallet name cannot be the root path", self.nodes[0].createwallet, name) if __name__ == '__main__': MultiWalletTest(__file__).main() From 3d7f0e4ed5ac1fea0529fb25662cf0ea9a397cee Mon Sep 17 00:00:00 2001 From: Ava Chow Date: Wed, 29 Apr 2026 15:12:46 -0700 Subject: [PATCH 2/3] wallettool: Use GetWalletPath to determine the wallet path Instead of computing the path separately, use GetWalletPath to use the behavior and error checking of the typical wallet path computation. --- src/wallet/wallet.cpp | 2 +- src/wallet/wallet.h | 3 +++ src/wallet/wallettool.cpp | 7 ++++++- 3 files changed, 10 insertions(+), 2 deletions(-) diff --git a/src/wallet/wallet.cpp b/src/wallet/wallet.cpp index 49099bf70cd..a95d67e884a 100644 --- a/src/wallet/wallet.cpp +++ b/src/wallet/wallet.cpp @@ -2940,7 +2940,7 @@ bool CWallet::EraseAddressReceiveRequest(WalletBatch& batch, const CTxDestinatio return true; } -static util::Result GetWalletPath(const std::string& name) +util::Result GetWalletPath(const std::string& name) { const fs::path name_path = fs::PathFromString(name); diff --git a/src/wallet/wallet.h b/src/wallet/wallet.h index 9cd78258454..5385ee97116 100644 --- a/src/wallet/wallet.h +++ b/src/wallet/wallet.h @@ -1150,6 +1150,9 @@ struct MigrationResult { [[nodiscard]] util::Result MigrateLegacyToDescriptor(const std::string& wallet_name, const SecureString& passphrase, WalletContext& context); //! Requirement: The wallet provided to this function must be isolated, with no attachment to the node's context. [[nodiscard]] util::Result MigrateLegacyToDescriptor(std::shared_ptr local_wallet, const SecureString& passphrase, WalletContext& context); + +//! Determine the path that the wallet is stored in +util::Result GetWalletPath(const std::string& name); } // namespace wallet #endif // BITCOIN_WALLET_WALLET_H diff --git a/src/wallet/wallettool.cpp b/src/wallet/wallettool.cpp index 7c24c0e1c6a..d1f3a8ce982 100644 --- a/src/wallet/wallettool.cpp +++ b/src/wallet/wallettool.cpp @@ -102,7 +102,12 @@ bool ExecuteWalletToolFunc(const ArgsManager& args, const std::string& command) return false; } const std::string name = args.GetArg("-wallet", ""); - const fs::path path = fsbridge::AbsPathJoin(GetWalletDir(), fs::PathFromString(name)); + util::Result path_res = GetWalletPath(name); + if (!path_res) { + tfm::format(std::cerr, "%s\n", util::ErrorString(path_res).original); + return false; + } + const fs::path& path = *path_res; if (command == "create") { if (name.empty()) { From eed7af666b6670badc9f58d200b77dc973786c60 Mon Sep 17 00:00:00 2001 From: David Gumberg Date: Wed, 29 Apr 2026 14:35:35 -0700 Subject: [PATCH 3/3] doc: Add release note for disallowing some wallet path names --- doc/release-notes-34544.md | 7 +++++++ 1 file changed, 7 insertions(+) create mode 100644 doc/release-notes-34544.md diff --git a/doc/release-notes-34544.md b/doc/release-notes-34544.md new file mode 100644 index 00000000000..bcc3e38f3c4 --- /dev/null +++ b/doc/release-notes-34544.md @@ -0,0 +1,7 @@ +Wallet +------ + +- Wallets names that are relative paths including `..` and `.` elements, and + wallets named `/` are no longer allowed. Any users that depended on this + behavior can instead use absolute paths to their wallet or move their wallet + to a safer path.