From 100e8a75bf5d8196c005331bd8f2ed42ada6d8d0 Mon Sep 17 00:00:00 2001 From: Sebastian Falbesoner Date: Thu, 24 Aug 2023 15:20:55 +0200 Subject: rpc: check and throw specific pubkey parsing errors in `HexToPubKey` In the helper `HexToPubKey`, check for three different causes of legacy public key parsing errors (in this order): - pubkey is not a hex string - pubkey doesn't have a valid length (33 or 65 bytes) [NEW] - pubkey is cryptographically invalid, i.e. not on curve (`IsFullyValid` check) and throw a specific error message for each one. Note that the error code is identical for all of them (-5), so this doesn't break RPC API compatibility. The helper is currently used for the RPCs `createmultisig` and `addmultisigaddress`. The length checks can be removed from the call-sites and error message checks in the functional tests are adapted. --- test/functional/rpc_rawtransaction.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) (limited to 'test') diff --git a/test/functional/rpc_rawtransaction.py b/test/functional/rpc_rawtransaction.py index c12865b5e3..e5d7cea135 100755 --- a/test/functional/rpc_rawtransaction.py +++ b/test/functional/rpc_rawtransaction.py @@ -491,11 +491,11 @@ class RawTransactionsTest(BitcoinTestFramework): addr2Obj = self.nodes[2].getaddressinfo(addr2) # Tests for createmultisig and addmultisigaddress - assert_raises_rpc_error(-5, "Invalid public key", self.nodes[0].createmultisig, 1, ["01020304"]) + assert_raises_rpc_error(-5, 'Pubkey "01020304" must have a length of either 33 or 65 bytes', self.nodes[0].createmultisig, 1, ["01020304"]) # createmultisig can only take public keys self.nodes[0].createmultisig(2, [addr1Obj['pubkey'], addr2Obj['pubkey']]) # addmultisigaddress can take both pubkeys and addresses so long as they are in the wallet, which is tested here - assert_raises_rpc_error(-5, "Invalid public key", self.nodes[0].createmultisig, 2, [addr1Obj['pubkey'], addr1]) + assert_raises_rpc_error(-5, f'Pubkey "{addr1}" must be a hex string', self.nodes[0].createmultisig, 2, [addr1Obj['pubkey'], addr1]) mSigObj = self.nodes[2].addmultisigaddress(2, [addr1Obj['pubkey'], addr1])['address'] -- cgit v1.2.3 From c740b154d193b91ca42f18759098d3fef6eaab05 Mon Sep 17 00:00:00 2001 From: Sebastian Falbesoner Date: Thu, 24 Aug 2023 16:05:44 +0200 Subject: rpc: use `HexToPubKey` helper for all legacy pubkey-parsing RPCs This deduplicates code and leads to more consistent and detailed error messages. Affected are legacy import RPCs (`importpubkey`, `importmulti`) and other ones where solving data can be provided (`fundrawtransaction`, `walletcreatefundedpsbt`, `send`, `sendall`). --- test/functional/wallet_basic.py | 7 ++++--- test/functional/wallet_fundrawtransaction.py | 4 ++-- 2 files changed, 6 insertions(+), 5 deletions(-) (limited to 'test') diff --git a/test/functional/wallet_basic.py b/test/functional/wallet_basic.py index f798eee365..dbb78e8d35 100755 --- a/test/functional/wallet_basic.py +++ b/test/functional/wallet_basic.py @@ -459,10 +459,11 @@ class WalletTest(BitcoinTestFramework): assert_raises_rpc_error(-5, "Invalid Bitcoin address or script", self.nodes[0].importaddress, "invalid") # This will raise an exception for attempting to import a pubkey that isn't in hex - assert_raises_rpc_error(-5, "Pubkey must be a hex string", self.nodes[0].importpubkey, "not hex") + assert_raises_rpc_error(-5, 'Pubkey "not hex" must be a hex string', self.nodes[0].importpubkey, "not hex") - # This will raise an exception for importing an invalid pubkey - assert_raises_rpc_error(-5, "Pubkey is not a valid public key", self.nodes[0].importpubkey, "5361746f736869204e616b616d6f746f") + # This will raise an exception for importing a pubkey with invalid length + too_short_pubkey = "5361746f736869204e616b616d6f746f" + assert_raises_rpc_error(-5, f'Pubkey "{too_short_pubkey}" must have a length of either 33 or 65 bytes', self.nodes[0].importpubkey, too_short_pubkey) # Bech32m addresses cannot be imported into a legacy wallet assert_raises_rpc_error(-5, "Bech32m addresses cannot be imported into legacy wallets", self.nodes[0].importaddress, "bcrt1p0xlxvlhemja6c4dqv22uapctqupfhlxm9h8z3k2e72q4k9hcz7vqc8gma6") diff --git a/test/functional/wallet_fundrawtransaction.py b/test/functional/wallet_fundrawtransaction.py index d886a59ac1..083da3d0b7 100755 --- a/test/functional/wallet_fundrawtransaction.py +++ b/test/functional/wallet_fundrawtransaction.py @@ -1055,8 +1055,8 @@ class RawTransactionsTest(BitcoinTestFramework): assert_raises_rpc_error(-4, "Not solvable pre-selected input COutPoint(%s, %s)" % (ext_utxo["txid"][0:10], ext_utxo["vout"]), wallet.fundrawtransaction, raw_tx) # Error conditions - assert_raises_rpc_error(-5, "'not a pubkey' is not hex", wallet.fundrawtransaction, raw_tx, solving_data={"pubkeys":["not a pubkey"]}) - assert_raises_rpc_error(-5, "'01234567890a0b0c0d0e0f' is not a valid public key", wallet.fundrawtransaction, raw_tx, solving_data={"pubkeys":["01234567890a0b0c0d0e0f"]}) + assert_raises_rpc_error(-5, 'Pubkey "not a pubkey" must be a hex string', wallet.fundrawtransaction, raw_tx, solving_data={"pubkeys":["not a pubkey"]}) + assert_raises_rpc_error(-5, 'Pubkey "01234567890a0b0c0d0e0f" must have a length of either 33 or 65 bytes', wallet.fundrawtransaction, raw_tx, solving_data={"pubkeys":["01234567890a0b0c0d0e0f"]}) assert_raises_rpc_error(-5, "'not a script' is not hex", wallet.fundrawtransaction, raw_tx, solving_data={"scripts":["not a script"]}) assert_raises_rpc_error(-8, "Unable to parse descriptor 'not a descriptor'", wallet.fundrawtransaction, raw_tx, solving_data={"descriptors":["not a descriptor"]}) assert_raises_rpc_error(-8, "Invalid parameter, missing vout key", wallet.fundrawtransaction, raw_tx, input_weights=[{"txid": ext_utxo["txid"]}]) -- cgit v1.2.3 From 98570fe29bb08d7edc48011aa6b9731c6ab4ed2e Mon Sep 17 00:00:00 2001 From: Sebastian Falbesoner Date: Thu, 24 Aug 2023 16:46:36 +0200 Subject: test: add coverage for parsing cryptographically invalid pubkeys --- test/functional/wallet_basic.py | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) (limited to 'test') diff --git a/test/functional/wallet_basic.py b/test/functional/wallet_basic.py index dbb78e8d35..3daec4dbd1 100755 --- a/test/functional/wallet_basic.py +++ b/test/functional/wallet_basic.py @@ -461,9 +461,11 @@ class WalletTest(BitcoinTestFramework): # This will raise an exception for attempting to import a pubkey that isn't in hex assert_raises_rpc_error(-5, 'Pubkey "not hex" must be a hex string', self.nodes[0].importpubkey, "not hex") - # This will raise an exception for importing a pubkey with invalid length + # This will raise exceptions for importing a pubkeys with invalid length / invalid coordinates too_short_pubkey = "5361746f736869204e616b616d6f746f" assert_raises_rpc_error(-5, f'Pubkey "{too_short_pubkey}" must have a length of either 33 or 65 bytes', self.nodes[0].importpubkey, too_short_pubkey) + not_on_curve_pubkey = bytes([4] + [0]*64).hex() # pubkey with coordinates (0,0) is not on curve + assert_raises_rpc_error(-5, f'Pubkey "{not_on_curve_pubkey}" must be cryptographically valid', self.nodes[0].importpubkey, not_on_curve_pubkey) # Bech32m addresses cannot be imported into a legacy wallet assert_raises_rpc_error(-5, "Bech32m addresses cannot be imported into legacy wallets", self.nodes[0].importaddress, "bcrt1p0xlxvlhemja6c4dqv22uapctqupfhlxm9h8z3k2e72q4k9hcz7vqc8gma6") -- cgit v1.2.3