highperfocused/bitcoin
1
0
Fork 0
mirror of https://github.com/bitcoin/bitcoin.git synced 2026-06-12 07:39:41 +02:00
Code Issues Packages Projects Releases Wiki Activity

Merge bitcoin/bitcoin#34544: wallet: Disallow wallet names that are paths including .. and . elements

Browse Source 
eed7af666b doc: Add release note for disallowing some wallet path names (David Gumberg)
3d7f0e4ed5 wallettool: Use GetWalletPath to determine the wallet path (Ava Chow)
2b0dc0d228 wallet: Disallow . and .. from wallet names (Ava Chow)

Pull request description:

  Wallet names including `..` and `.` are unintuitive and can lead to various issues, see #34497

  This disallows creating or loading wallets that have any path elements that are `..` or `.`, including any present in an absolute path. This does not disallow relative paths altogether but rather limits them to only being subdirectories of the wallets directory.

ACKs for top commit:
  davidgumberg:
    crACK eed7af666b
  w0xlt:
    ACK eed7af666b
  arejula27:
    ACK eed7af666b

Tree-SHA512: bec5e54369061eb630d9afb94701badce09e8beb63686cf714016466fc01653d4841030fc10fcd14d44e6b1022c0994cb32253d80533807704d9e11eda2423ff
This commit is contained in:
merge-script
2026-05-02 10:58:15 +02:00
parent db98e357d3 eed7af666b
commit b1ff4773ba
6 changed files with 55 additions and 107 deletions
Download Patch File Download Diff File Expand all files Collapse all files

7
doc/release-notes-34544.md Normal file
View File

@@ -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.

23
src/wallet/wallet.cpp
View File

@@ -2940,19 +2940,36 @@ bool CWallet::EraseAddressReceiveRequest(WalletBatch& batch, const CTxDestinatio
return true;
}
static util::Result<fs::path> GetWalletPath(const std::string& name)
util::Result<fs::path> 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, "

3
src/wallet/wallet.h
View File

@@ -1144,6 +1144,9 @@ struct MigrationResult {
[[nodiscard]] util::Result<MigrationResult> 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<MigrationResult> MigrateLegacyToDescriptor(std::shared_ptr<CWallet> local_wallet, const SecureString& passphrase, WalletContext& context);
//! Determine the path that the wallet is stored in
util::Result<fs::path> GetWalletPath(const std::string& name);
} // namespace wallet
#endif // BITCOIN_WALLET_WALLET_H

7
src/wallet/wallettool.cpp
View File

@@ -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<fs::path> 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()) {

105
test/functional/wallet_migration.py
View File

@@ -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()

17
test/functional/wallet_multiwallet.py
View File

@@ -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()