From 0cd9273fd959c6742574259d026039f7da0309a2 Mon Sep 17 00:00:00 2001 From: "Wladimir J. van der Laan" Date: Tue, 7 Mar 2017 09:50:41 +0100 Subject: [PATCH] rpc: Prevent `dumpwallet` from overwriting files Prevent arbitrary files from being overwritten. There have been reports that users have overwritten wallet files this way. It may also avoid other security issues. Fixes #9934. Adds mention to release notes and adds a test. --- doc/release-notes.md | 3 +++ src/wallet/rpcdump.cpp | 14 ++++++++++++-- test/functional/wallet-dump.py | 5 ++++- 3 files changed, 19 insertions(+), 3 deletions(-) diff --git a/doc/release-notes.md b/doc/release-notes.md index 04fb0f333..4ecca7897 100644 --- a/doc/release-notes.md +++ b/doc/release-notes.md @@ -83,6 +83,9 @@ Low-level RPC changes * `getwalletinfo` * `getmininginfo` +- `dumpwallet` no longer allows overwriting files. This is a security measure + as well as prevents dangerous user mistakes. + Credits ======= diff --git a/src/wallet/rpcdump.cpp b/src/wallet/rpcdump.cpp index 9539cc9f4..1123fd6db 100644 --- a/src/wallet/rpcdump.cpp +++ b/src/wallet/rpcdump.cpp @@ -600,7 +600,7 @@ UniValue dumpwallet(const JSONRPCRequest& request) if (request.fHelp || request.params.size() != 1) throw std::runtime_error( "dumpwallet \"filename\"\n" - "\nDumps all wallet keys in a human-readable format.\n" + "\nDumps all wallet keys in a human-readable format to a server-side file. This does not allow overwriting existing files.\n" "\nArguments:\n" "1. \"filename\" (string, required) The filename with path (either absolute or relative to bitcoind)\n" "\nResult:\n" @@ -616,9 +616,19 @@ UniValue dumpwallet(const JSONRPCRequest& request) EnsureWalletIsUnlocked(pwallet); - std::ofstream file; boost::filesystem::path filepath = request.params[0].get_str(); filepath = boost::filesystem::absolute(filepath); + + /* Prevent arbitrary files from being overwritten. There have been reports + * that users have overwritten wallet files this way: + * https://github.com/bitcoin/bitcoin/issues/9934 + * It may also avoid other security issues. + */ + if (boost::filesystem::exists(filepath)) { + throw JSONRPCError(RPC_INVALID_PARAMETER, filepath.string() + " already exists. If you are sure this is what you want, move it out of the way first"); + } + + std::ofstream file; file.open(filepath.string().c_str()); if (!file.is_open()) throw JSONRPCError(RPC_INVALID_PARAMETER, "Cannot open wallet dump file"); diff --git a/test/functional/wallet-dump.py b/test/functional/wallet-dump.py index e4757c8c0..12db95e5d 100755 --- a/test/functional/wallet-dump.py +++ b/test/functional/wallet-dump.py @@ -7,7 +7,7 @@ import os from test_framework.test_framework import BitcoinTestFramework -from test_framework.util import assert_equal +from test_framework.util import (assert_equal, assert_raises_jsonrpc) def read_dump(file_name, addrs, hd_master_addr_old): @@ -105,5 +105,8 @@ class WalletDumpTest(BitcoinTestFramework): assert_equal(found_addr_chg, 90*2 + 50) # old reserve keys are marked as change now assert_equal(found_addr_rsv, 90*2) + # Overwriting should fail + assert_raises_jsonrpc(-8, "already exists", self.nodes[0].dumpwallet, tmpdir + "/node0/wallet.unencrypted.dump") + if __name__ == '__main__': WalletDumpTest().main ()