Auto merge of #2006 - bitcartel:1497_destination_path_validation_when_exporting, r=bitcartel

Fixes #1497 ZCA-009 by restricting export to a user defined folder and sanitizing filenames

qa/rpc-tests/walletbackup.py

@@ -47,8 +47,13 @@ class WalletBackupTest(BitcoinTestFramework):
 
     # This mirrors how the network was setup in the bash test
     def setup_network(self, split=False):
+        # -exportdir option means we must provide a valid path to the destination folder for wallet backups
+        ed0 = "-exportdir=" + self.options.tmpdir + "/node0"
+        ed1 = "-exportdir=" + self.options.tmpdir + "/node1"
+        ed2 = "-exportdir=" + self.options.tmpdir + "/node2"
+
         # nodes 1, 2,3 are spenders, let's give them a keypool=100
-        extra_args = [["-keypool=100"], ["-keypool=100"], ["-keypool=100"], []]
+        extra_args = [["-keypool=100", ed0], ["-keypool=100", ed1], ["-keypool=100", ed2], []]
         self.nodes = start_nodes(4, self.options.tmpdir, extra_args)
         connect_nodes(self.nodes[0], 3)
         connect_nodes(self.nodes[1], 3)
@@ -122,12 +127,12 @@ class WalletBackupTest(BitcoinTestFramework):
 
         logging.info("Backing up")
         tmpdir = self.options.tmpdir
-        self.nodes[0].backupwallet(tmpdir + "/node0/wallet.bak")
+        self.nodes[0].backupwallet("walletbak")
-        self.nodes[0].dumpwallet(tmpdir + "/node0/wallet.dump")
+        self.nodes[0].dumpwallet("walletdump")
-        self.nodes[1].backupwallet(tmpdir + "/node1/wallet.bak")
+        self.nodes[1].backupwallet("walletbak")
-        self.nodes[1].dumpwallet(tmpdir + "/node1/wallet.dump")
+        self.nodes[1].dumpwallet("walletdump")
-        self.nodes[2].backupwallet(tmpdir + "/node2/wallet.bak")
+        self.nodes[2].backupwallet("walletbak")
-        self.nodes[2].dumpwallet(tmpdir + "/node2/wallet.dump")
+        self.nodes[2].dumpwallet("walletdump")
 
         logging.info("More transactions")
         for i in range(5):
@@ -159,9 +164,9 @@ class WalletBackupTest(BitcoinTestFramework):
         shutil.rmtree(self.options.tmpdir + "/node2/regtest/chainstate")
 
         # Restore wallets from backup
-        shutil.copyfile(tmpdir + "/node0/wallet.bak", tmpdir + "/node0/regtest/wallet.dat")
+        shutil.copyfile(tmpdir + "/node0/walletbak", tmpdir + "/node0/regtest/wallet.dat")
-        shutil.copyfile(tmpdir + "/node1/wallet.bak", tmpdir + "/node1/regtest/wallet.dat")
+        shutil.copyfile(tmpdir + "/node1/walletbak", tmpdir + "/node1/regtest/wallet.dat")
-        shutil.copyfile(tmpdir + "/node2/wallet.bak", tmpdir + "/node2/regtest/wallet.dat")
+        shutil.copyfile(tmpdir + "/node2/walletbak", tmpdir + "/node2/regtest/wallet.dat")
 
         logging.info("Re-starting nodes")
         self.start_three()
@@ -185,9 +190,9 @@ class WalletBackupTest(BitcoinTestFramework):
         assert_equal(self.nodes[1].getbalance(), 0)
         assert_equal(self.nodes[2].getbalance(), 0)
 
-        self.nodes[0].importwallet(tmpdir + "/node0/wallet.dump")
+        self.nodes[0].importwallet(tmpdir + "/node0/walletdump")
-        self.nodes[1].importwallet(tmpdir + "/node1/wallet.dump")
+        self.nodes[1].importwallet(tmpdir + "/node1/walletdump")
-        self.nodes[2].importwallet(tmpdir + "/node2/wallet.dump")
+        self.nodes[2].importwallet(tmpdir + "/node2/walletdump")
 
         sync_blocks(self.nodes)
 

src/init.cpp

@@ -289,6 +289,7 @@ std::string HelpMessage(HelpMessageMode mode)
 #endif
     }
     strUsage += HelpMessageOpt("-datadir=<dir>", _("Specify data directory"));
+    strUsage += HelpMessageOpt("-exportdir=<dir>", _("Specify directory to be used when exporting data"));
     strUsage += HelpMessageOpt("-dbcache=<n>", strprintf(_("Set database cache size in megabytes (%d to %d, default: %d)"), nMinDbCache, nMaxDbCache, nDefaultDbCache));
     strUsage += HelpMessageOpt("-loadblock=<file>", _("Imports blocks from external blk000??.dat file") + " " + _("on startup"));
     strUsage += HelpMessageOpt("-maxorphantx=<n>", strprintf(_("Keep at most <n> unconnectable transactions in memory (default: %u)"), DEFAULT_MAX_ORPHAN_TRANSACTIONS));

src/test/rpc_wallet_tests.cpp

@@ -358,16 +358,26 @@ BOOST_AUTO_TEST_CASE(rpc_wallet_z_exportwallet)
     pwalletMain->GetPaymentAddresses(addrs);
     BOOST_CHECK(addrs.size()==1);
     
+    // Set up paths
+    boost::filesystem::path tmppath = boost::filesystem::temp_directory_path();
+    boost::filesystem::path tmpfilename = boost::filesystem::unique_path("%%%%%%%%");
+    boost::filesystem::path exportfilepath = tmppath / tmpfilename;
+
+    // export will fail since exportdir is not set
+    BOOST_CHECK_THROW(CallRPC(string("z_exportwallet ") + tmpfilename.string()), runtime_error);
+
+    // set exportdir
+    mapArgs["-exportdir"] = tmppath.native();
+
+    // run some tests
     BOOST_CHECK_THROW(CallRPC("z_exportwallet"), runtime_error);
 
     BOOST_CHECK_THROW(CallRPC("z_exportwallet toomany args"), runtime_error);
 
-   
+    BOOST_CHECK_THROW(CallRPC(string("z_exportwallet invalid!*/_chars.txt")), runtime_error);
-    boost::filesystem::path temp = boost::filesystem::temp_directory_path() /
+
-            boost::filesystem::unique_path();
+    BOOST_CHECK_NO_THROW(CallRPC(string("z_exportwallet ") + tmpfilename.string()));
-    const std::string path = temp.native();
 
-    BOOST_CHECK_NO_THROW(CallRPC(string("z_exportwallet ") + path));
 
     auto addr = paymentAddress.Get();
     libzcash::SpendingKey key;
@@ -382,7 +392,7 @@ BOOST_AUTO_TEST_CASE(rpc_wallet_z_exportwallet)
     EnsureWalletIsUnlocked();
 
     ifstream file;
-    file.open(path.c_str(), std::ios::in | std::ios::ate);
+    file.open(exportfilepath.string().c_str(), std::ios::in | std::ios::ate);
     BOOST_CHECK(file.is_open());
     bool fVerified = false;
     int64_t nFilesize = std::max((int64_t)1, (int64_t)file.tellg());

src/util.cpp

@@ -484,6 +484,26 @@ const boost::filesystem::path &ZC_GetParamsDir()
     return path;
 }
 
+// Return the user specified export directory.  Create directory if it doesn't exist.
+// If user did not set option, return an empty path.
+// If there is a filesystem problem, throw an exception.
+const boost::filesystem::path GetExportDir()
+{
+    namespace fs = boost::filesystem;
+    fs::path path;
+    if (mapArgs.count("-exportdir")) {
+        path = fs::system_complete(mapArgs["-exportdir"]);
+        if (fs::exists(path) && !fs::is_directory(path)) {
+            throw std::runtime_error(strprintf("The -exportdir '%s' already exists and is not a directory", path.string()));
+        }
+        if (!fs::exists(path) && !fs::create_directories(path)) {
+            throw std::runtime_error(strprintf("Failed to create directory at -exportdir '%s'", path.string()));
+        }
+    }
+    return path;
+}
+
+
 const boost::filesystem::path &GetDataDir(bool fNetSpecific)
 {
     namespace fs = boost::filesystem;

src/util.h

@@ -138,6 +138,7 @@ boost::filesystem::path GetSpecialFolderPath(int nFolder, bool fCreate = true);
 boost::filesystem::path GetTempPath();
 void ShrinkDebugFile();
 void runCommand(std::string strCommand);
+const boost::filesystem::path GetExportDir();
 
 inline bool IsSwitchChar(char c)
 {

src/utilstrencodings.cpp

@@ -30,6 +30,22 @@ string SanitizeString(const string& str)
     return strResult;
 }
 
+string SanitizeFilename(const string& str)
+{
+    /**
+     * safeChars chosen to restrict filename, keeping it simple to avoid cross-platform issues.
+     * http://stackoverflow.com/a/2306003
+     */
+    static string safeChars("abcdefghijklmnopqrstuvwxyzABCDEFGHIJKLMNOPQRSTUVWXYZ01234567890");
+    string strResult;
+    for (std::string::size_type i = 0; i < str.size(); i++)
+    {
+        if (safeChars.find(str[i]) != std::string::npos)
+            strResult.push_back(str[i]);
+    }
+    return strResult;
+}
+
 const signed char p_util_hexdigit[256] =
 { -1,-1,-1,-1,-1,-1,-1,-1,-1,-1,-1,-1,-1,-1,-1,-1,
   -1,-1,-1,-1,-1,-1,-1,-1,-1,-1,-1,-1,-1,-1,-1,-1,

src/utilstrencodings.h

@@ -22,6 +22,7 @@
 /** This is needed because the foreach macro can't get over the comma in pair<t1, t2> */
 #define PAIRTYPE(t1, t2)    std::pair<t1, t2>
 
+std::string SanitizeFilename(const std::string& str);
 std::string SanitizeString(const std::string& str);
 std::vector<unsigned char> ParseHex(const char* psz);
 std::vector<unsigned char> ParseHex(const std::string& str);

src/wallet/rpcdump.cpp

@@ -430,7 +430,9 @@ Value z_exportwallet(const Array& params, bool fHelp)
             "z_exportwallet \"filename\"\n"
             "\nExports all wallet keys, for taddr and zaddr, in a human-readable format.\n"
             "\nArguments:\n"
-            "1. \"filename\"    (string, required) The filename\n"
+            "1. \"filename\"    (string, required) The filename, saved in folder set by zcashd -exportdir option\n"
+            "\nResult:\n"
+            "\"path\"           (string) The full path of the destination file\n"
             "\nExamples:\n"
             + HelpExampleCli("z_exportwallet", "\"test\"")
             + HelpExampleRpc("z_exportwallet", "\"test\"")
@@ -449,7 +451,9 @@ Value dumpwallet(const Array& params, bool fHelp)
             "dumpwallet \"filename\"\n"
             "\nDumps taddr wallet keys in a human-readable format.\n"
             "\nArguments:\n"
-            "1. \"filename\"    (string, required) The filename\n"
+            "1. \"filename\"    (string, required) The filename, saved in folder set by zcashd -exportdir option\n"
+            "\nResult:\n"
+            "\"path\"           (string) The full path of the destination file\n"
             "\nExamples:\n"
             + HelpExampleCli("dumpwallet", "\"test\"")
             + HelpExampleRpc("dumpwallet", "\"test\"")
@@ -464,8 +468,24 @@ Value dumpwallet_impl(const Array& params, bool fHelp, bool fDumpZKeys)
 
     EnsureWalletIsUnlocked();
 
+    boost::filesystem::path exportdir;
+    try {
+        exportdir = GetExportDir();
+    } catch (const std::runtime_error& e) {
+        throw JSONRPCError(RPC_INTERNAL_ERROR, e.what());
+    }
+    if (exportdir.empty()) {
+        throw JSONRPCError(RPC_WALLET_ERROR, "Cannot export wallet until the zcashd -exportdir option has been set");
+    }
+    std::string unclean = params[0].get_str();
+    std::string clean = SanitizeFilename(unclean);
+    if (clean.compare(unclean) != 0) {
+        throw JSONRPCError(RPC_WALLET_ERROR, strprintf("Filename is invalid as only alphanumeric characters are allowed.  Try '%s' instead.", clean));
+    }
+    boost::filesystem::path exportfilepath = exportdir / clean;
+
     ofstream file;
-    file.open(params[0].get_str().c_str());
+    file.open(exportfilepath.string().c_str());
     if (!file.is_open())
         throw JSONRPCError(RPC_INVALID_PARAMETER, "Cannot open wallet dump file");
 
@@ -523,7 +543,8 @@ Value dumpwallet_impl(const Array& params, bool fHelp, bool fDumpZKeys)
 
     file << "# End of dump\n";
     file.close();
-    return Value::null;
+
+    return exportfilepath.string();
 }
 
 

src/wallet/rpcwallet.cpp

@@ -1769,21 +1769,38 @@ Value backupwallet(const Array& params, bool fHelp)
     if (fHelp || params.size() != 1)
         throw runtime_error(
             "backupwallet \"destination\"\n"
-            "\nSafely copies wallet.dat to destination, which can be a directory or a path with filename.\n"
+            "\nSafely copies wallet.dat to destination filename\n"
             "\nArguments:\n"
-            "1. \"destination\"   (string) The destination directory or file\n"
+            "1. \"destination\"   (string, required) The destination filename, saved in the directory set by -exportdir option.\n"
+            "\nResult:\n"
+            "\"path\"             (string) The full path of the destination file\n"
             "\nExamples:\n"
-            + HelpExampleCli("backupwallet", "\"backup.dat\"")
+            + HelpExampleCli("backupwallet", "\"backupdata\"")
-            + HelpExampleRpc("backupwallet", "\"backup.dat\"")
+            + HelpExampleRpc("backupwallet", "\"backupdata\"")
         );
 
     LOCK2(cs_main, pwalletMain->cs_wallet);
 
-    string strDest = params[0].get_str();
+    boost::filesystem::path exportdir;
-    if (!BackupWallet(*pwalletMain, strDest))
+    try {
+        exportdir = GetExportDir();
+    } catch (const std::runtime_error& e) {
+        throw JSONRPCError(RPC_INTERNAL_ERROR, e.what());
+    }
+    if (exportdir.empty()) {
+        throw JSONRPCError(RPC_WALLET_ERROR, "Cannot backup wallet until the -exportdir option has been set");
+    }
+    std::string unclean = params[0].get_str();
+    std::string clean = SanitizeFilename(unclean);
+    if (clean.compare(unclean) != 0) {
+        throw JSONRPCError(RPC_WALLET_ERROR, strprintf("Filename is invalid as only alphanumeric characters are allowed.  Try '%s' instead.", clean));
+    }
+    boost::filesystem::path exportfilepath = exportdir / clean;
+
+    if (!BackupWallet(*pwalletMain, exportfilepath.string()))
         throw JSONRPCError(RPC_WALLET_ERROR, "Error: Wallet backup failed!");
 
-    return Value::null;
+    return exportfilepath.string();
 }