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

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

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");

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