wallettool: do not use fs::remove_all in createfromdump cleanup
What changed, and why it matters
This patch fixes a cleanup routine in Bitcoin Core's wallet tool. Previously, when creating a wallet from a dump file failed, the tool used a broad 'delete everything in this directory' command. The change makes it delete only the specific files that belong to the wallet it just created, reducing the risk of accidentally wiping unrelated files or directories.
Treat as a security-hardening fix and include in release notes. Users running prior versions should avoid passing untrusted or empty wallet names to bitcoin-wallet createfromdump, and should back up any data in the wallets directory before using the tool.
Security signals we found
Potential path-deletion vulnerability: fs::remove_all on a user-influenced path could remove more than intended
Switch from recursive directory removal to targeted file removal
New regression test for empty wallet name case
Command-line wallet tool surface (bitcoin-wallet createfromdump)
Evidence from the diff
In src/wallet/dump.cpp, CreateFromDump previously called fs::remove_all(wallet_path) on failure. The patch collects the wallet’s actual database files via wallet->GetDatabase().Files() and, if a wallet name was provided, the wallet_path itself, then removes each with fs::remove() instead. A new functional test checks that a failed createfromdump with an empty wallet name does not delete the entire wallets directory or an unrelated wallet.dat inside it.
Changed components
src/wallet/dump.cppbitcoin-wallet toolcreatefromdump commandInspect captured patch +13 / −1
diff --git a/src/wallet/dump.cpp b/src/wallet/dump.cpp
index c63b95b5..eb6fa0e4 100644
--- a/src/wallet/dump.cpp
+++ b/src/wallet/dump.cpp
@@ -276,11 +276,17 @@ bool CreateFromDump(const ArgsManager& args, const std::string& name, const fs::
dump_file.close();
}
+ // On failure, gather the paths to remove
+ std::vector<fs::path> paths_to_remove = wallet->GetDatabase().Files();
+ if (!name.empty()) paths_to_remove.push_back(wallet_path);
+
wallet.reset(); // The pointer deleter will close the wallet for us.
// Remove the wallet dir if we have a failure
if (!ret) {
- fs::remove_all(wallet_path);
+ for (const auto& p : paths_to_remove) {
+ fs::remove(p);
+ }
}
return ret;
diff --git a/test/functional/tool_wallet.py b/test/functional/tool_wallet.py
index 786ef627..44ac1da9 100755
--- a/test/functional/tool_wallet.py
+++ b/test/functional/tool_wallet.py
@@ -319,6 +319,12 @@ class ToolWalletTest(BitcoinTestFramework):
self.write_dump(dump_data, bad_sum_wallet_dump)
self.assert_raises_tool_error('Error: Checksum is not the correct size', '-wallet=badload', '-dumpfile={}'.format(bad_sum_wallet_dump), 'createfromdump')
assert not (self.nodes[0].wallets_path / "badload").is_dir()
+ self.assert_raises_tool_error('Error: Checksum is not the correct size', '-wallet=', '-dumpfile={}'.format(bad_sum_wallet_dump), 'createfromdump')
+ assert self.nodes[0].wallets_path.exists()
+ assert not (self.nodes[0].wallets_path / "wallet.dat").exists()
+
+ self.log.info('Checking createfromdump with an unnamed wallet')
+ self.do_tool_createfromdump("", "wallet.dump")
def test_chainless_conflicts(self):
self.log.info("Test wallet tool when wallet contains conflicting transactions")
Why this scored 59/100
Community notes
Notes can correct, qualify, or add evidence to the AI analysis. Every note shown here has been validated by a human moderator.
The AI analysis stands alone for now. Submit a note if you can add evidence or important context.