Merge #9937: rpc: Prevent dumpwallet from overwriting files

0cd9273 rpc: Prevent `dumpwallet` from overwriting files (Wladimir J. van der Laan)

Pull request description:

  Prevent arbitrary files from being overwritten by `dumpwallet`. 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.

Tree-SHA512: 268c98636d40924d793b55a685a0b419bafd834ad369edaec08227ebe26ed4470ddea73008d1c4beb10ea445db1b0bb8e3546ba8fc2d1a411ebd4a0de8ce9120

doc/release-notes.md

@@ -109,6 +109,9 @@ See detailed [set of changes](https://github.com/dashpay/dash/compare/v0.14.0.2.
 - [`4286dde49`](https://github.com/dashpay/dash/commit/4286dde49) Remove support for InstantSend locked gobject collaterals (#3019)
 - [`788d42dbc`](https://github.com/dashpay/dash/commit/788d42dbc) Bump version to 0.14.0.3 and copy release notes (#3053)
 
+- `dumpwallet` no longer allows overwriting files. This is a security measure
+  as well as prevents dangerous user mistakes.
+
 Credits
 =======
 

src/wallet/rpcdump.cpp

@@ -774,7 +774,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\n"
             "\nExamples:\n"
@@ -786,8 +786,20 @@ UniValue dumpwallet(const JSONRPCRequest& request)
 
     EnsureWalletIsUnlocked(pwallet);
 
+    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(request.params[0].get_str().c_str());
+    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

@@ -6,7 +6,7 @@
 import sys
 
 from test_framework.test_framework import BitcoinTestFramework
-from test_framework.util import *
+from test_framework.util import (assert_equal, assert_raises_jsonrpc)
 
 
 def read_dump(file_name, addrs, hd_master_addr_old):
@@ -111,5 +111,8 @@ class WalletDumpTest(BitcoinTestFramework):
         # assert_equal(found_addr_chg, 180 + 50)  # old reserve keys are marked as change now
         assert_equal(found_addr_rsv, 180)  # keypool size
 
+        # Overwriting should fail
+        assert_raises_jsonrpc(-8, "already exists", self.nodes[0].dumpwallet, tmpdir + "/node0/wallet.unencrypted.dump")
+
 if __name__ == '__main__':
     WalletDumpTest().main ()