From 67e7ba8e1aea58fc864f9bb1fc0e56b70777185e Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Aur=C3=A8le=20Oul=C3=A8s?= Date: Thu, 15 Dec 2022 10:58:14 +0100 Subject: [PATCH 1/3] rpc: Sanitize label name in various RPCs - importprivkey - importaddress - importpubkey - listtransactions - listsinceblock - importmulti - importdescriptors --- src/wallet/rpc/backup.cpp | 10 +++++----- src/wallet/rpc/transactions.cpp | 4 ++-- 2 files changed, 7 insertions(+), 7 deletions(-) diff --git a/src/wallet/rpc/backup.cpp b/src/wallet/rpc/backup.cpp index ddf10cae155..6a0f8548f18 100644 --- a/src/wallet/rpc/backup.cpp +++ b/src/wallet/rpc/backup.cpp @@ -142,7 +142,7 @@ RPCHelpMan importprivkey() std::string strSecret = request.params[0].get_str(); std::string strLabel; if (!request.params[1].isNull()) - strLabel = request.params[1].get_str(); + strLabel = LabelFromValue(request.params[1]); // Whether to perform rescan after import if (!request.params[2].isNull()) @@ -235,7 +235,7 @@ RPCHelpMan importaddress() std::string strLabel; if (!request.params[1].isNull()) - strLabel = request.params[1].get_str(); + strLabel = LabelFromValue(request.params[1]); // Whether to perform rescan after import bool fRescan = true; @@ -428,7 +428,7 @@ RPCHelpMan importpubkey() std::string strLabel; if (!request.params[1].isNull()) - strLabel = request.params[1].get_str(); + strLabel = LabelFromValue(request.params[1]); // Whether to perform rescan after import bool fRescan = true; @@ -1163,7 +1163,7 @@ static UniValue ProcessImport(CWallet& wallet, const UniValue& data, const int64 if (internal && data.exists("label")) { throw JSONRPCError(RPC_INVALID_PARAMETER, "Internal addresses should not have a label"); } - const std::string& label = data.exists("label") ? data["label"].get_str() : ""; + const std::string& label = data.exists("label") ? LabelFromValue(data["label"]) : ""; const bool add_keypool = data.exists("keypool") ? data["keypool"].get_bool() : false; // Add to keypool only works with privkeys disabled @@ -1457,7 +1457,7 @@ static UniValue ProcessDescriptorImport(CWallet& wallet, const UniValue& data, c const std::string& descriptor = data["desc"].get_str(); const bool active = data.exists("active") ? data["active"].get_bool() : false; const bool internal = data.exists("internal") ? data["internal"].get_bool() : false; - const std::string& label = data.exists("label") ? data["label"].get_str() : ""; + const std::string& label = data.exists("label") ? LabelFromValue(data["label"]) : ""; // Parse descriptor string FlatSigningProvider keys; diff --git a/src/wallet/rpc/transactions.cpp b/src/wallet/rpc/transactions.cpp index 02a1ac5ea1e..844657a71cb 100644 --- a/src/wallet/rpc/transactions.cpp +++ b/src/wallet/rpc/transactions.cpp @@ -487,7 +487,7 @@ RPCHelpMan listtransactions() std::optional filter_label; if (!request.params[0].isNull() && request.params[0].get_str() != "*") { - filter_label = request.params[0].get_str(); + filter_label.emplace(LabelFromValue(request.params[0])); if (filter_label.value().empty()) { throw JSONRPCError(RPC_INVALID_PARAMETER, "Label argument must be a valid label name or \"*\"."); } @@ -637,7 +637,7 @@ RPCHelpMan listsinceblock() std::optional filter_label; if (!request.params[5].isNull()) { - filter_label = request.params[5].get_str(); + filter_label = LabelFromValue(request.params[5]); } int depth = height ? wallet.GetLastBlockHeight() + 1 - *height : -1; From 552b51e682b5a52d9e2fbe64e44e623451692bd3 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Aur=C3=A8le=20Oul=C3=A8s?= Date: Thu, 15 Dec 2022 11:02:32 +0100 Subject: [PATCH 2/3] refactor: Add sanity checks in LabelFromValue --- src/wallet/rpc/addresses.cpp | 12 ++++-------- src/wallet/rpc/backup.cpp | 16 +++++----------- src/wallet/rpc/transactions.cpp | 5 ++--- src/wallet/rpc/util.cpp | 5 ++++- 4 files changed, 15 insertions(+), 23 deletions(-) diff --git a/src/wallet/rpc/addresses.cpp b/src/wallet/rpc/addresses.cpp index 903a569cb9d..ec72db522db 100644 --- a/src/wallet/rpc/addresses.cpp +++ b/src/wallet/rpc/addresses.cpp @@ -43,9 +43,7 @@ RPCHelpMan getnewaddress() } // Parse the label first so we don't generate a key if there's an error - std::string label; - if (!request.params[0].isNull()) - label = LabelFromValue(request.params[0]); + const std::string label{LabelFromValue(request.params[0])}; OutputType output_type = pwallet->m_default_address_type; if (!request.params[1].isNull()) { @@ -140,7 +138,7 @@ RPCHelpMan setlabel() throw JSONRPCError(RPC_INVALID_ADDRESS_OR_KEY, "Invalid Bitcoin address"); } - std::string label = LabelFromValue(request.params[1]); + const std::string label{LabelFromValue(request.params[1])}; if (pwallet->IsMine(dest)) { pwallet->SetAddressBook(dest, label, "receive"); @@ -258,9 +256,7 @@ RPCHelpMan addmultisigaddress() LOCK2(pwallet->cs_wallet, spk_man.cs_KeyStore); - std::string label; - if (!request.params[2].isNull()) - label = LabelFromValue(request.params[2]); + const std::string label{LabelFromValue(request.params[2])}; int required = request.params[0].getInt(); @@ -662,7 +658,7 @@ RPCHelpMan getaddressesbylabel() LOCK(pwallet->cs_wallet); - std::string label = LabelFromValue(request.params[0]); + const std::string label{LabelFromValue(request.params[0])}; // Find all addresses that have the given label UniValue ret(UniValue::VOBJ); diff --git a/src/wallet/rpc/backup.cpp b/src/wallet/rpc/backup.cpp index 6a0f8548f18..76f6ce94489 100644 --- a/src/wallet/rpc/backup.cpp +++ b/src/wallet/rpc/backup.cpp @@ -140,9 +140,7 @@ RPCHelpMan importprivkey() EnsureWalletIsUnlocked(*pwallet); std::string strSecret = request.params[0].get_str(); - std::string strLabel; - if (!request.params[1].isNull()) - strLabel = LabelFromValue(request.params[1]); + const std::string strLabel{LabelFromValue(request.params[1])}; // Whether to perform rescan after import if (!request.params[2].isNull()) @@ -233,9 +231,7 @@ RPCHelpMan importaddress() EnsureLegacyScriptPubKeyMan(*pwallet, true); - std::string strLabel; - if (!request.params[1].isNull()) - strLabel = LabelFromValue(request.params[1]); + const std::string strLabel{LabelFromValue(request.params[1])}; // Whether to perform rescan after import bool fRescan = true; @@ -426,9 +422,7 @@ RPCHelpMan importpubkey() EnsureLegacyScriptPubKeyMan(*pwallet, true); - std::string strLabel; - if (!request.params[1].isNull()) - strLabel = LabelFromValue(request.params[1]); + const std::string strLabel{LabelFromValue(request.params[1])}; // Whether to perform rescan after import bool fRescan = true; @@ -1163,7 +1157,7 @@ static UniValue ProcessImport(CWallet& wallet, const UniValue& data, const int64 if (internal && data.exists("label")) { throw JSONRPCError(RPC_INVALID_PARAMETER, "Internal addresses should not have a label"); } - const std::string& label = data.exists("label") ? LabelFromValue(data["label"]) : ""; + const std::string label{LabelFromValue(data["label"])}; const bool add_keypool = data.exists("keypool") ? data["keypool"].get_bool() : false; // Add to keypool only works with privkeys disabled @@ -1457,7 +1451,7 @@ static UniValue ProcessDescriptorImport(CWallet& wallet, const UniValue& data, c const std::string& descriptor = data["desc"].get_str(); const bool active = data.exists("active") ? data["active"].get_bool() : false; const bool internal = data.exists("internal") ? data["internal"].get_bool() : false; - const std::string& label = data.exists("label") ? LabelFromValue(data["label"]) : ""; + const std::string label{LabelFromValue(data["label"])}; // Parse descriptor string FlatSigningProvider keys; diff --git a/src/wallet/rpc/transactions.cpp b/src/wallet/rpc/transactions.cpp index 844657a71cb..5579cb6c495 100644 --- a/src/wallet/rpc/transactions.cpp +++ b/src/wallet/rpc/transactions.cpp @@ -635,10 +635,9 @@ RPCHelpMan listsinceblock() bool include_removed = (request.params[3].isNull() || request.params[3].get_bool()); bool include_change = (!request.params[4].isNull() && request.params[4].get_bool()); + // Only set it if 'label' was provided. std::optional filter_label; - if (!request.params[5].isNull()) { - filter_label = LabelFromValue(request.params[5]); - } + if (!request.params[5].isNull()) filter_label.emplace(LabelFromValue(request.params[5])); int depth = height ? wallet.GetLastBlockHeight() + 1 - *height : -1; diff --git a/src/wallet/rpc/util.cpp b/src/wallet/rpc/util.cpp index 26270f23ed4..d5f62dbd775 100644 --- a/src/wallet/rpc/util.cpp +++ b/src/wallet/rpc/util.cpp @@ -132,7 +132,10 @@ const LegacyScriptPubKeyMan& EnsureConstLegacyScriptPubKeyMan(const CWallet& wal std::string LabelFromValue(const UniValue& value) { - std::string label = value.get_str(); + static const std::string empty_string; + if (value.isNull()) return empty_string; + + const std::string& label{value.get_str()}; if (label == "*") throw JSONRPCError(RPC_WALLET_INVALID_LABEL_NAME, "Invalid label name"); return label; From 65e78bda7cd23ad7b6ede63c6bf5ffe6f552c71d Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Aur=C3=A8le=20Oul=C3=A8s?= Date: Tue, 27 Sep 2022 11:10:01 +0200 Subject: [PATCH 3/3] test: Invalid label name coverage --- test/functional/wallet_labels.py | 40 ++++++++++++++++++++++++++++++++ 1 file changed, 40 insertions(+) diff --git a/test/functional/wallet_labels.py b/test/functional/wallet_labels.py index c92b6f2c2ad..8cf37163dd3 100755 --- a/test/functional/wallet_labels.py +++ b/test/functional/wallet_labels.py @@ -28,6 +28,44 @@ class WalletLabelsTest(BitcoinTestFramework): def skip_test_if_missing_module(self): self.skip_if_no_wallet() + def invalid_label_name_test(self): + node = self.nodes[0] + address = node.getnewaddress() + pubkey = node.getaddressinfo(address)['pubkey'] + rpc_calls = [ + [node.getnewaddress], + [node.setlabel, address], + [node.getaddressesbylabel], + [node.importpubkey, pubkey], + [node.addmultisigaddress, 1, [pubkey]], + [node.getreceivedbylabel], + [node.listsinceblock, node.getblockhash(0), 1, False, True, False], + ] + if self.options.descriptors: + response = node.importdescriptors([{ + 'desc': f'pkh({pubkey})', + 'label': '*', + 'timestamp': 'now', + }]) + else: + rpc_calls.extend([ + [node.importprivkey, node.dumpprivkey(address)], + [node.importaddress, address], + ]) + + response = node.importmulti([{ + 'scriptPubKey': {'address': address}, + 'label': '*', + 'timestamp': 'now', + }]) + + assert_equal(response[0]['success'], False) + assert_equal(response[0]['error']['code'], -11) + assert_equal(response[0]['error']['message'], "Invalid label name") + + for rpc_call in rpc_calls: + assert_raises_rpc_error(-11, "Invalid label name", *rpc_call, "*") + def run_test(self): # Check that there's no UTXO on the node node = self.nodes[0] @@ -138,6 +176,8 @@ class WalletLabelsTest(BitcoinTestFramework): # in the label. This is a no-op. change_label(node, labels[2].addresses[0], labels[2], labels[2]) + self.invalid_label_name_test() + if self.options.descriptors: # This is a descriptor wallet test because of segwit v1+ addresses self.log.info('Check watchonly labels')