From ac01dbee63e31712d5574ba10f1992a47a917f66 Mon Sep 17 00:00:00 2001 From: "Wladimir J. van der Laan" Date: Fri, 1 Feb 2019 13:39:43 +0100 Subject: [PATCH] Merge #15235: Do not import private keys to wallets with private keys disabled e6c58d3b014ab8ef5cca4be68764af4b79685fcb Do not import private keys to wallets with private keys disabled (Andrew Chow) b5c5021b644731d14a6ef04961320a99466f035a Refactor importwallet to extract data from the file and then import (Andrew Chow) 1f77f6754ce724493b0cb084ae0b35107d58605f tests: unify RPC argument to cli argument conversion and handle dicts and lists (Andrew Chow) Pull request description: Fixes a bug where private keys could be imported to wallets with private keys disabled. Now every RPC which can import private keys checks for whether the wallet has private keys are disabled and errors if it is. Also added an belt-and-suspenders check to `AddKeyPubkeyWithDB` to have it assert that the wallet has private keys enabled. Tree-SHA512: 5cd04febce9aa2bd9bfd02f312c6ff8705e37278cae59efd3895f6d6e2f1b477aefd297e2dd0860791bdd3d4f3cad8eb1a404f8f3d4e2035b91314ad2c1028ae dash changes --- src/wallet/rpcdump.cpp | 101 +++++++++++++------ src/wallet/rpcwallet.cpp | 4 + src/wallet/wallet.cpp | 3 + test/functional/test_framework/test_node.py | 12 ++- test/functional/wallet_disableprivatekeys.py | 11 ++ 5 files changed, 98 insertions(+), 33 deletions(-) diff --git a/src/wallet/rpcdump.cpp b/src/wallet/rpcdump.cpp index c35b16bb6e..cfb0885d79 100644 --- a/src/wallet/rpcdump.cpp +++ b/src/wallet/rpcdump.cpp @@ -109,6 +109,9 @@ UniValue importprivkey(const JSONRPCRequest& request) + HelpExampleRpc("importprivkey", "\"mykey\", \"testing\", false") ); + if (pwallet->IsWalletFlagSet(WALLET_FLAG_DISABLE_PRIVATE_KEYS)) { + throw JSONRPCError(RPC_WALLET_ERROR, "Cannot import private keys to a wallet with private keys disabled"); + } WalletRescanReserver reserver(pwallet); bool fRescan = true; @@ -525,8 +528,10 @@ UniValue importwallet(const JSONRPCRequest& request) // Use uiInterface.ShowProgress instead of pwallet.ShowProgress because pwallet.ShowProgress has a cancel button tied to AbortRescan which // we don't want for this progress bar showing the import progress. uiInterface.ShowProgress does not have a cancel button. uiInterface.ShowProgress(strprintf("%s " + _("Importing..."), pwallet->GetDisplayName()), 0, false); // show progress dialog in GUI + std::vector> keys; + std::vector> scripts; while (file.good()) { - uiInterface.ShowProgress("", std::max(1, std::min(99, (int)(((double)file.tellg() / (double)nFilesize) * 100))), false); + uiInterface.ShowProgress("", std::max(1, std::min(50, (int)(((double)file.tellg() / (double)nFilesize) * 100))), false); std::string line; std::getline(file, line); if (line.empty() || line[0] == '#') @@ -538,13 +543,6 @@ UniValue importwallet(const JSONRPCRequest& request) continue; CKey key = DecodeSecret(vstr[0]); if (key.IsValid()) { - CPubKey pubkey = key.GetPubKey(); - assert(key.VerifyPubKey(pubkey)); - CKeyID keyid = pubkey.GetID(); - if (pwallet->HaveKey(keyid)) { - pwallet->WalletLogPrintf("Skipping import of %s (key already present)\n", EncodeDestination(keyid)); - continue; - } int64_t nTime = DecodeDumpTime(vstr[1]); std::string strLabel; bool fLabel = true; @@ -560,36 +558,67 @@ UniValue importwallet(const JSONRPCRequest& request) fLabel = true; } } - pwallet->WalletLogPrintf("Importing %s...\n", EncodeDestination(keyid)); - if (!pwallet->AddKeyPubKey(key, pubkey)) { - fGood = false; - continue; - } - pwallet->mapKeyMetadata[keyid].nCreateTime = nTime; - if (fLabel) - pwallet->SetAddressBook(keyid, strLabel, "receive"); - nTimeBegin = std::min(nTimeBegin, nTime); + keys.push_back(std::make_tuple(key, nTime, fLabel, strLabel)); } else if(IsHex(vstr[0])) { std::vector vData(ParseHex(vstr[0])); CScript script = CScript(vData.begin(), vData.end()); - CScriptID id(script); - if (pwallet->HaveCScript(id)) { - pwallet->WalletLogPrintf("Skipping import of %s (script already present)\n", vstr[0]); - continue; - } - if(!pwallet->AddCScript(script)) { - pwallet->WalletLogPrintf("Error importing script %s\n", vstr[0]); - fGood = false; - continue; - } int64_t birth_time = DecodeDumpTime(vstr[1]); - if (birth_time > 0) { - pwallet->m_script_metadata[id].nCreateTime = birth_time; - nTimeBegin = std::min(nTimeBegin, birth_time); - } + scripts.push_back(std::pair(script, birth_time)); } } file.close(); + // We now know whether we are importing private keys, so we can error if private keys are disabled + if (keys.size() > 0 && pwallet->IsWalletFlagSet(WALLET_FLAG_DISABLE_PRIVATE_KEYS)) { + uiInterface.ShowProgress("", 100, false); // hide progress dialog in GUI + throw JSONRPCError(RPC_WALLET_ERROR, "Importing wallets is disabled when private keys are disabled"); + } + double total = (double)(keys.size() + scripts.size()); + double progress = 0; + for (const auto& key_tuple : keys) { + uiInterface.ShowProgress("", std::max(50, std::min(75, (int)((progress / total) * 100) + 50)), false); + const CKey& key = std::get<0>(key_tuple); + int64_t time = std::get<1>(key_tuple); + bool has_label = std::get<2>(key_tuple); + std::string label = std::get<3>(key_tuple); + + CPubKey pubkey = key.GetPubKey(); + assert(key.VerifyPubKey(pubkey)); + CKeyID keyid = pubkey.GetID(); + if (pwallet->HaveKey(keyid)) { + pwallet->WalletLogPrintf("Skipping import of %s (key already present)\n", EncodeDestination(keyid)); + continue; + } + pwallet->WalletLogPrintf("Importing %s...\n", EncodeDestination(keyid)); + if (!pwallet->AddKeyPubKey(key, pubkey)) { + fGood = false; + continue; + } + pwallet->mapKeyMetadata[keyid].nCreateTime = time; + if (has_label) + pwallet->SetAddressBook(keyid, label, "receive"); + nTimeBegin = std::min(nTimeBegin, time); + progress++; + } + for (const auto& script_pair : scripts) { + uiInterface.ShowProgress("", std::max(50, std::min(75, (int)((progress / total) * 100) + 50)), false); + const CScript& script = script_pair.first; + int64_t time = script_pair.second; + CScriptID id(script); + if (pwallet->HaveCScript(id)) { + pwallet->WalletLogPrintf("Skipping import of %s (script already present)\n", HexStr(script)); + continue; + } + if(!pwallet->AddCScript(script)) { + pwallet->WalletLogPrintf("Error importing script %s\n", HexStr(script)); + fGood = false; + continue; + } + if (time > 0) { + pwallet->m_script_metadata[id].nCreateTime = time; + nTimeBegin = std::min(nTimeBegin, time); + } + progress++; + } uiInterface.ShowProgress("", 100, false); // hide progress dialog in GUI pwallet->UpdateTimeFirstKey(nTimeBegin); } @@ -629,6 +658,10 @@ UniValue importelectrumwallet(const JSONRPCRequest& request) if (fPruneMode) throw JSONRPCError(RPC_WALLET_ERROR, "Importing wallets is disabled in pruned mode"); + if (pwallet->IsWalletFlagSet(WALLET_FLAG_DISABLE_PRIVATE_KEYS)) { + throw JSONRPCError(RPC_WALLET_ERROR, "Error: Private keys are disabled for this wallet"); + } + LOCK2(cs_main, pwallet->cs_wallet); EnsureWalletIsUnlocked(pwallet); @@ -1020,6 +1053,12 @@ static UniValue ProcessImport(CWallet * const pwallet, const UniValue& data, con const std::string& output = isScript ? scriptPubKey.get_str() : scriptPubKey["address"].get_str(); // Parse the output. + // If private keys are disabled, abort if private keys are being imported + if (pwallet->IsWalletFlagSet(WALLET_FLAG_DISABLE_PRIVATE_KEYS) && !keys.isNull()) { + throw JSONRPCError(RPC_WALLET_ERROR, "Cannot import private keys to a wallet with private keys disabled"); + } + + // Generate the script and destination for the scriptPubKey provided CScript script; CTxDestination dest; diff --git a/src/wallet/rpcwallet.cpp b/src/wallet/rpcwallet.cpp index 526a968382..aa339bc1c2 100644 --- a/src/wallet/rpcwallet.cpp +++ b/src/wallet/rpcwallet.cpp @@ -3181,6 +3181,10 @@ static UniValue upgradetohd(const JSONRPCRequest& request) throw JSONRPCError(RPC_WALLET_ERROR, "Cannot downgrade wallet"); } + if (pwallet->IsWalletFlagSet(WALLET_FLAG_DISABLE_PRIVATE_KEYS)) { + throw JSONRPCError(RPC_WALLET_ERROR, "Error: Private keys are disabled for this wallet"); + } + bool prev_encrypted = pwallet->IsCrypted(); SecureString secureWalletPassphrase; diff --git a/src/wallet/wallet.cpp b/src/wallet/wallet.cpp index 268eeb239a..a36e8faa12 100644 --- a/src/wallet/wallet.cpp +++ b/src/wallet/wallet.cpp @@ -359,6 +359,9 @@ bool CWallet::AddKeyPubKeyWithDB(WalletBatch &batch, const CKey& secret, const C { AssertLockHeld(cs_wallet); // mapKeyMetadata + // Make sure we aren't adding private keys to private key disabled wallets + assert(!IsWalletFlagSet(WALLET_FLAG_DISABLE_PRIVATE_KEYS)); + // CCryptoKeyStore has no concept of wallet databases, but calls AddCryptedKey // which is overridden below. To avoid flushes, the database handle is // tunneled through to it. diff --git a/test/functional/test_framework/test_node.py b/test/functional/test_framework/test_node.py index 9df8229e9e..5592c20632 100755 --- a/test/functional/test_framework/test_node.py +++ b/test/functional/test_framework/test_node.py @@ -375,6 +375,14 @@ class TestNodeCLIAttr: def get_request(self, *args, **kwargs): return lambda: self(*args, **kwargs) +def arg_to_cli(arg): + if isinstance(arg, bool): + return str(arg).lower() + elif isinstance(arg, dict) or isinstance(arg, list): + return json.dumps(arg) + else: + return str(arg) + class TestNodeCLI(): """Interface to dash-cli for an individual node""" @@ -406,8 +414,8 @@ class TestNodeCLI(): def send_cli(self, command=None, *args, **kwargs): """Run dash-cli command. Deserializes returned string as python object.""" - pos_args = [str(arg).lower() if type(arg) is bool else str(arg) for arg in args] - named_args = [str(key) + "=" + str(value) for (key, value) in kwargs.items()] + pos_args = [arg_to_cli(arg) for arg in args] + named_args = [str(key) + "=" + arg_to_cli(value) for (key, value) in kwargs.items()] assert not (pos_args and named_args), "Cannot use positional arguments and named arguments in the same dash-cli call" p_args = [self.binary, "-datadir=" + self.datadir] + self.options if named_args: diff --git a/test/functional/wallet_disableprivatekeys.py b/test/functional/wallet_disableprivatekeys.py index 0ba2cfe9be..1ac3bedc14 100755 --- a/test/functional/wallet_disableprivatekeys.py +++ b/test/functional/wallet_disableprivatekeys.py @@ -7,6 +7,7 @@ from test_framework.test_framework import BitcoinTestFramework from test_framework.util import ( + assert_equal, assert_raises_rpc_error, ) @@ -28,5 +29,15 @@ class DisablePrivateKeysTest(BitcoinTestFramework): assert_raises_rpc_error(-4,"Error: Private keys are disabled for this wallet", w1.getrawchangeaddress) w1.importpubkey(w2.getaddressinfo(w2.getnewaddress())['pubkey']) + self.log.info('Test that private keys cannot be imported') + addr = w2.getnewaddress('') + privkey = w2.dumpprivkey(addr) + assert_raises_rpc_error(-4, 'Cannot import private keys to a wallet with private keys disabled', w1.importprivkey, privkey) + result = w1.importmulti([{'scriptPubKey': {'address': addr}, 'timestamp': 'now', 'keys': [privkey]}]) + assert(not result[0]['success']) + assert('warning' not in result[0]) + assert_equal(result[0]['error']['code'], -4) + assert_equal(result[0]['error']['message'], 'Cannot import private keys to a wallet with private keys disabled') + if __name__ == '__main__': DisablePrivateKeysTest().main()