diff options
author | Wladimir J. van der Laan <laanwj@gmail.com> | 2017-03-07 09:50:41 +0100 |
---|---|---|
committer | MarcoFalke <falke.marco@gmail.com> | 2017-10-04 15:11:24 +0200 |
commit | a43be5bcdb27a68abe9bb5fec57185a1b6652479 (patch) | |
tree | 41a6954250a13e353d05f1bcddcc122382a9bcef | |
parent | b6c0209aaf372a21090c0a3eed973022af14ece0 (diff) | |
download | bitcoin-a43be5bcdb27a68abe9bb5fec57185a1b6652479.tar.xz |
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.
Github-Pull: #9937
Rebased-From: 0cd9273fd959c6742574259d026039f7da0309a2
-rw-r--r-- | doc/release-notes.md | 3 | ||||
-rw-r--r-- | src/wallet/rpcdump.cpp | 14 | ||||
-rwxr-xr-x | 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 ef8de316a5..0235e1c606 100644 --- a/doc/release-notes.md +++ b/doc/release-notes.md @@ -65,6 +65,9 @@ Notable changes 0.15.1 Change log ================= +- `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 67c6d9ec64..e56485e296 100644 --- a/src/wallet/rpcdump.cpp +++ b/src/wallet/rpcdump.cpp @@ -595,7 +595,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" @@ -611,9 +611,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 569cc46e6c..016bd95925 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): @@ -108,5 +108,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 () |