From 9064d73bf8b22f6b9449f72fc6559f25ca45b1fc Mon Sep 17 00:00:00 2001 From: Simon Date: Mon, 9 Jan 2017 21:25:42 -0800 Subject: [PATCH 1/2] Fixes #1497 ZCA-009 by restricting data exporting to user defined folder. Previously the RPC interface allowed z_exportwallet, backupwallet and dumpwallet to write data to an arbitrary filename. ZCA-009 demonstrates how this is vulnerable. The resolution is to only allow data to written when the -exportdir has been configured. Also filenames are restricted to alphanumeric characters. --- src/init.cpp | 1 + src/test/rpc_wallet_tests.cpp | 22 ++++++++++++++++------ src/util.cpp | 20 ++++++++++++++++++++ src/util.h | 1 + src/utilstrencodings.cpp | 16 ++++++++++++++++ src/utilstrencodings.h | 1 + src/wallet/rpcdump.cpp | 29 +++++++++++++++++++++++++---- src/wallet/rpcwallet.cpp | 31 ++++++++++++++++++++++++------- 8 files changed, 104 insertions(+), 17 deletions(-) diff --git a/src/init.cpp b/src/init.cpp index 53f6fb832..8a2398be1 100644 --- a/src/init.cpp +++ b/src/init.cpp @@ -289,6 +289,7 @@ std::string HelpMessage(HelpMessageMode mode) #endif } strUsage += HelpMessageOpt("-datadir=", _("Specify data directory")); + strUsage += HelpMessageOpt("-exportdir=", _("Specify directory to be used when exporting data")); strUsage += HelpMessageOpt("-dbcache=", strprintf(_("Set database cache size in megabytes (%d to %d, default: %d)"), nMinDbCache, nMaxDbCache, nDefaultDbCache)); strUsage += HelpMessageOpt("-loadblock=", _("Imports blocks from external blk000??.dat file") + " " + _("on startup")); strUsage += HelpMessageOpt("-maxorphantx=", strprintf(_("Keep at most unconnectable transactions in memory (default: %u)"), DEFAULT_MAX_ORPHAN_TRANSACTIONS)); diff --git a/src/test/rpc_wallet_tests.cpp b/src/test/rpc_wallet_tests.cpp index 8471ebebc..d0ee27aea 100644 --- a/src/test/rpc_wallet_tests.cpp +++ b/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::filesystem::path temp = boost::filesystem::temp_directory_path() / - boost::filesystem::unique_path(); - const std::string path = temp.native(); + BOOST_CHECK_THROW(CallRPC(string("z_exportwallet invalid!*/_chars.txt")), runtime_error); + + BOOST_CHECK_NO_THROW(CallRPC(string("z_exportwallet ") + tmpfilename.string())); - 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()); diff --git a/src/util.cpp b/src/util.cpp index b667acc5e..ec8f90445 100644 --- a/src/util.cpp +++ b/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; diff --git a/src/util.h b/src/util.h index ef3a347fa..56c7ea39b 100644 --- a/src/util.h +++ b/src/util.h @@ -134,6 +134,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) { diff --git a/src/utilstrencodings.cpp b/src/utilstrencodings.cpp index c15bddc6f..646536f1e 100644 --- a/src/utilstrencodings.cpp +++ b/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, diff --git a/src/utilstrencodings.h b/src/utilstrencodings.h index b0edd8b54..e08de362b 100644 --- a/src/utilstrencodings.h +++ b/src/utilstrencodings.h @@ -22,6 +22,7 @@ /** This is needed because the foreach macro can't get over the comma in pair */ #define PAIRTYPE(t1, t2) std::pair +std::string SanitizeFilename(const std::string& str); std::string SanitizeString(const std::string& str); std::vector ParseHex(const char* psz); std::vector ParseHex(const std::string& str); diff --git a/src/wallet/rpcdump.cpp b/src/wallet/rpcdump.cpp index 789bd4537..07d81bce5 100644 --- a/src/wallet/rpcdump.cpp +++ b/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(); } diff --git a/src/wallet/rpcwallet.cpp b/src/wallet/rpcwallet.cpp index 6b0f5f528..770f5826c 100644 --- a/src/wallet/rpcwallet.cpp +++ b/src/wallet/rpcwallet.cpp @@ -1766,21 +1766,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\"") - + HelpExampleRpc("backupwallet", "\"backup.dat\"") + + HelpExampleCli("backupwallet", "\"backupdata\"") + + HelpExampleRpc("backupwallet", "\"backupdata\"") ); LOCK2(cs_main, pwalletMain->cs_wallet); - string strDest = params[0].get_str(); - if (!BackupWallet(*pwalletMain, strDest)) + 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 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(); } From bab89e35c737e62f309c5c1d93f1a446f6af56dd Mon Sep 17 00:00:00 2001 From: Simon Date: Wed, 18 Jan 2017 10:05:49 -0800 Subject: [PATCH 2/2] Update walletbackup.py qa test to use -exportdir option --- qa/rpc-tests/walletbackup.py | 31 ++++++++++++++++++------------- 1 file changed, 18 insertions(+), 13 deletions(-) diff --git a/qa/rpc-tests/walletbackup.py b/qa/rpc-tests/walletbackup.py index e71065781..5f3eb6fcc 100755 --- a/qa/rpc-tests/walletbackup.py +++ b/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].dumpwallet(tmpdir + "/node0/wallet.dump") - self.nodes[1].backupwallet(tmpdir + "/node1/wallet.bak") - self.nodes[1].dumpwallet(tmpdir + "/node1/wallet.dump") - self.nodes[2].backupwallet(tmpdir + "/node2/wallet.bak") - self.nodes[2].dumpwallet(tmpdir + "/node2/wallet.dump") + self.nodes[0].backupwallet("walletbak") + self.nodes[0].dumpwallet("walletdump") + self.nodes[1].backupwallet("walletbak") + self.nodes[1].dumpwallet("walletdump") + self.nodes[2].backupwallet("walletbak") + 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 + "/node1/wallet.bak", tmpdir + "/node1/regtest/wallet.dat") - shutil.copyfile(tmpdir + "/node2/wallet.bak", tmpdir + "/node2/regtest/wallet.dat") + shutil.copyfile(tmpdir + "/node0/walletbak", tmpdir + "/node0/regtest/wallet.dat") + shutil.copyfile(tmpdir + "/node1/walletbak", tmpdir + "/node1/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[1].importwallet(tmpdir + "/node1/wallet.dump") - self.nodes[2].importwallet(tmpdir + "/node2/wallet.dump") + self.nodes[0].importwallet(tmpdir + "/node0/walletdump") + self.nodes[1].importwallet(tmpdir + "/node1/walletdump") + self.nodes[2].importwallet(tmpdir + "/node2/walletdump") sync_blocks(self.nodes)