Merge bitcoin/bitcoin#26186: rpc: Sanitize label name in various RPCs with tests

65e78bda7c test: Invalid label name coverage (Aurèle Oulès)
552b51e682 refactor: Add sanity checks in LabelFromValue (Aurèle Oulès)
67e7ba8e1a rpc: Sanitize label name in various RPCs (Aurèle Oulès)

Pull request description:

  The following RPCs did not sanitize the optional label name:
  - importprivkey
  - importaddress
  - importpubkey
  - importmulti
  - importdescriptors
  - listsinceblock

  Thus is was possible to import an address with a label `*` which should not be possible.
  The wildcard label is used for backwards compatibility in the `listtransactions` rpc.
  I added test coverage for these RPCs.

ACKs for top commit:
  ajtowns:
    ACK 65e78bda7c
  achow101:
    ACK 65e78bda7c
  furszy:
    diff ACK 65e78bd
  stickies-v:
    re-ACK 65e78bda7c
  theStack:
    re-ACK 65e78bda7c

Tree-SHA512: ad99f2824d4cfae352166b76da4ca0069b7c2eccf81aaa0654be25bbb3c6e5d6b005d93960f3f4154155f80e12be2d0cebd5529922ae3d2a36ee4eed82440b31

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<int>();
 
@@ -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);

src/wallet/rpc/backup.cpp

@@ -156,9 +156,7 @@ RPCHelpMan importprivkey()
         EnsureWalletIsUnlocked(*pwallet);
 
         std::string strSecret = request.params[0].get_str();
-        std::string strLabel;
-        if (!request.params[1].isNull())
-            strLabel = request.params[1].get_str();
+        const std::string strLabel{LabelFromValue(request.params[1])};
 
         // Whether to perform rescan after import
         if (!request.params[2].isNull())
@@ -249,9 +247,7 @@ RPCHelpMan importaddress()
 
     EnsureLegacyScriptPubKeyMan(*pwallet, true);
 
-    std::string strLabel;
-    if (!request.params[1].isNull())
-        strLabel = request.params[1].get_str();
+    const std::string strLabel{LabelFromValue(request.params[1])};
 
     // Whether to perform rescan after import
     bool fRescan = true;
@@ -442,9 +438,7 @@ RPCHelpMan importpubkey()
 
     EnsureLegacyScriptPubKeyMan(*pwallet, true);
 
-    std::string strLabel;
-    if (!request.params[1].isNull())
-        strLabel = request.params[1].get_str();
+    const std::string strLabel{LabelFromValue(request.params[1])};
 
     // Whether to perform rescan after import
     bool fRescan = true;
@@ -1170,7 +1164,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{LabelFromValue(data["label"])};
         const bool add_keypool = data.exists("keypool") ? data["keypool"].get_bool() : false;
 
         // Add to keypool only works with privkeys disabled
@@ -1464,7 +1458,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{LabelFromValue(data["label"])};
 
         // Parse descriptor string
         FlatSigningProvider keys;

src/wallet/rpc/transactions.cpp

@@ -486,7 +486,7 @@ RPCHelpMan listtransactions()
 
     std::optional<std::string> 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 \"*\".");
         }
@@ -634,10 +634,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<std::string> filter_label;
-    if (!request.params[5].isNull()) {
-        filter_label = request.params[5].get_str();
-    }
+    if (!request.params[5].isNull()) filter_label.emplace(LabelFromValue(request.params[5]));
 
     int depth = height ? wallet.GetLastBlockHeight() + 1 - *height : -1;
 

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;

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')