Merge bitcoin/bitcoin#27727: rpc: Fix invalid bech32 address handling

eeee55f9288740747b6e8d806ce8177fd92635cf rpc: Fix invalid bech32 handling (MarcoFalke)

Pull request description:

  Currently the handling of invalid bech32(m) addresses over RPC has many issues:

  * No error for invalid addresses is reported, leading to internal bugs via `CHECK_NONFATAL`, see https://github.com/bitcoin/bitcoin/issues/27723
  * The error messages use "data size" (the meaning of which is unclear to the user, because the witness program data and bech32 section data are related but different) when they mean "program size"

  Fix all issues. Also, use the BIP 173 and BIP 350 test vectors.

ACKs for top commit:
  achow101:
    ACK eeee55f9288740747b6e8d806ce8177fd92635cf
  brunoerg:
    crACK eeee55f9288740747b6e8d806ce8177fd92635cf

Tree-SHA512: c8639ee49e2a54b740b72d66bc4a40352dd553a6e3220dea9f94e48e33124f21f597a2817cb405d0a4c88d21df1013c0a4877a01370a2d326aa2cff1f9c381a8

4 files changed, 217 insertions, 6 deletions

diff --git a/src/key_io.cpp b/src/key_io.cpp
index 4659a59544..33499b0d23 100644
--- a/src/key_io.cpp
+++ b/src/key_io.cpp
@@ -124,7 +124,11 @@ CTxDestination DecodeDestination(const std::string& str, const CChainParams& par
 
     data.clear();
     const auto dec = bech32::Decode(str);
-    if ((dec.encoding == bech32::Encoding::BECH32 || dec.encoding == bech32::Encoding::BECH32M) && dec.data.size() > 0) {
+    if (dec.encoding == bech32::Encoding::BECH32 || dec.encoding == bech32::Encoding::BECH32M) {
+        if (dec.data.empty()) {
+            error_str = "Empty Bech32 data section";
+            return CNoDestination();
+        }
         // Bech32 decoding
         if (dec.hrp != params.Bech32HRP()) {
             error_str = strprintf("Invalid or unsupported prefix for Segwit (Bech32) address (expected %s, got %s).", params.Bech32HRP(), dec.hrp);
@@ -158,7 +162,7 @@ CTxDestination DecodeDestination(const std::string& str, const CChainParams& par
                     }
                 }
 
-                error_str = "Invalid Bech32 v0 address data size";
+                error_str = strprintf("Invalid Bech32 v0 address program size (%s byte), per BIP141", data.size());
                 return CNoDestination();
             }
 
@@ -175,7 +179,7 @@ CTxDestination DecodeDestination(const std::string& str, const CChainParams& par
             }
 
             if (data.size() < 2 || data.size() > BECH32_WITNESS_PROG_MAX_LEN) {
-                error_str = "Invalid Bech32 address data size";
+                error_str = strprintf("Invalid Bech32 address program size (%s byte)", data.size());
                 return CNoDestination();
             }
 
@@ -184,6 +188,9 @@ CTxDestination DecodeDestination(const std::string& str, const CChainParams& par
             std::copy(data.begin(), data.end(), unk.program);
             unk.length = data.size();
             return unk;
+        } else {
+            error_str = strprintf("Invalid padding in Bech32 data section");
+            return CNoDestination();
         }
     }
 
diff --git a/test/functional/rpc_invalid_address_message.py b/test/functional/rpc_invalid_address_message.py
index fd282a9bc1..59b45bbb9b 100755
--- a/test/functional/rpc_invalid_address_message.py
+++ b/test/functional/rpc_invalid_address_message.py
@@ -63,12 +63,12 @@ class InvalidAddressErrorMessageTest(BitcoinTestFramework):
 
     def test_validateaddress(self):
         # Invalid Bech32
-        self.check_invalid(BECH32_INVALID_SIZE, 'Invalid Bech32 address data size')
+        self.check_invalid(BECH32_INVALID_SIZE, "Invalid Bech32 address program size (41 byte)")
         self.check_invalid(BECH32_INVALID_PREFIX, 'Invalid or unsupported Segwit (Bech32) or Base58 encoding.')
         self.check_invalid(BECH32_INVALID_BECH32, 'Version 1+ witness address must use Bech32m checksum')
         self.check_invalid(BECH32_INVALID_BECH32M, 'Version 0 witness address must use Bech32 checksum')
         self.check_invalid(BECH32_INVALID_VERSION, 'Invalid Bech32 address witness version')
-        self.check_invalid(BECH32_INVALID_V0_SIZE, 'Invalid Bech32 v0 address data size')
+        self.check_invalid(BECH32_INVALID_V0_SIZE, "Invalid Bech32 v0 address program size (21 byte), per BIP141")
         self.check_invalid(BECH32_TOO_LONG, 'Bech32 string too long', list(range(90, 108)))
         self.check_invalid(BECH32_ONE_ERROR, 'Invalid Bech32 checksum', [9])
         self.check_invalid(BECH32_TWO_ERRORS, 'Invalid Bech32 checksum', [22, 43])
@@ -105,7 +105,7 @@ class InvalidAddressErrorMessageTest(BitcoinTestFramework):
     def test_getaddressinfo(self):
         node = self.nodes[0]
 
-        assert_raises_rpc_error(-5, "Invalid Bech32 address data size", node.getaddressinfo, BECH32_INVALID_SIZE)
+        assert_raises_rpc_error(-5, "Invalid Bech32 address program size (41 byte)", node.getaddressinfo, BECH32_INVALID_SIZE)
         assert_raises_rpc_error(-5, "Invalid or unsupported Segwit (Bech32) or Base58 encoding.", node.getaddressinfo, BECH32_INVALID_PREFIX)
         assert_raises_rpc_error(-5, "Invalid or unsupported Base58-encoded address.", node.getaddressinfo, BASE58_INVALID_PREFIX)
         assert_raises_rpc_error(-5, "Invalid or unsupported Segwit (Bech32) or Base58 encoding.", node.getaddressinfo, INVALID_ADDRESS)
diff --git a/test/functional/rpc_validateaddress.py b/test/functional/rpc_validateaddress.py
new file mode 100755
index 0000000000..2ce6b35d04
--- /dev/null
+++ b/test/functional/rpc_validateaddress.py
@@ -0,0 +1,203 @@
+#!/usr/bin/env python3
+# Copyright (c) 2023 The Bitcoin Core developers
+# Distributed under the MIT software license, see the accompanying
+# file COPYING or http://www.opensource.org/licenses/mit-license.php.
+"""Test validateaddress for main chain"""
+
+from test_framework.test_framework import BitcoinTestFramework
+
+from test_framework.util import assert_equal
+
+INVALID_DATA = [
+    # BIP 173
+    (
+        "tc1qw508d6qejxtdg4y5r3zarvary0c5xw7kg3g4ty",
+        "Invalid or unsupported Segwit (Bech32) or Base58 encoding.",  # Invalid hrp
+        [],
+    ),
+    ("bc1qw508d6qejxtdg4y5r3zarvary0c5xw7kv8f3t5", "Invalid Bech32 checksum", [41]),
+    (
+        "BC13W508D6QEJXTDG4Y5R3ZARVARY0C5XW7KN40WF2",
+        "Version 1+ witness address must use Bech32m checksum",
+        [],
+    ),
+    (
+        "bc1rw5uspcuh",
+        "Version 1+ witness address must use Bech32m checksum",  # Invalid program length
+        [],
+    ),
+    (
+        "bc10w508d6qejxtdg4y5r3zarvary0c5xw7kw508d6qejxtdg4y5r3zarvary0c5xw7kw5rljs90",
+        "Version 1+ witness address must use Bech32m checksum",  # Invalid program length
+        [],
+    ),
+    (
+        "BC1QR508D6QEJXTDG4Y5R3ZARVARYV98GJ9P",
+        "Invalid Bech32 v0 address program size (16 byte), per BIP141",
+        [],
+    ),
+    (
+        "tb1qrp33g0q5c5txsp9arysrx4k6zdkfs4nce4xj0gdcccefvpysxf3q0sL5k7",
+        "Invalid or unsupported Segwit (Bech32) or Base58 encoding.",  # tb1, Mixed case
+        [],
+    ),
+    (
+        "BC1QW508D6QEJXTDG4Y5R3ZARVARY0C5XW7KV8F3t4",
+        "Invalid character or mixed case",  # bc1, Mixed case, not in BIP 173 test vectors
+        [40],
+    ),
+    (
+        "bc1zw508d6qejxtdg4y5r3zarvaryvqyzf3du",
+        "Version 1+ witness address must use Bech32m checksum",  # Wrong padding
+        [],
+    ),
+    (
+        "tb1qrp33g0q5c5txsp9arysrx4k6zdkfs4nce4xj0gdcccefvpysxf3pjxtptv",
+        "Invalid or unsupported Segwit (Bech32) or Base58 encoding.",  # tb1, Non-zero padding in 8-to-5 conversion
+        [],
+    ),
+    ("bc1gmk9yu", "Empty Bech32 data section", []),
+    # BIP 350
+    (
+        "tc1p0xlxvlhemja6c4dqv22uapctqupfhlxm9h8z3k2e72q4k9hcz7vq5zuyut",
+        "Invalid or unsupported Segwit (Bech32) or Base58 encoding.",  # Invalid human-readable part
+        [],
+    ),
+    (
+        "bc1p0xlxvlhemja6c4dqv22uapctqupfhlxm9h8z3k2e72q4k9hcz7vqh2y7hd",
+        "Version 1+ witness address must use Bech32m checksum",  # Invalid checksum (Bech32 instead of Bech32m)
+        [],
+    ),
+    (
+        "tb1z0xlxvlhemja6c4dqv22uapctqupfhlxm9h8z3k2e72q4k9hcz7vqglt7rf",
+        "Invalid or unsupported Segwit (Bech32) or Base58 encoding.",  # tb1, Invalid checksum (Bech32 instead of Bech32m)
+        [],
+    ),
+    (
+        "BC1S0XLXVLHEMJA6C4DQV22UAPCTQUPFHLXM9H8Z3K2E72Q4K9HCZ7VQ54WELL",
+        "Version 1+ witness address must use Bech32m checksum",  # Invalid checksum (Bech32 instead of Bech32m)
+        [],
+    ),
+    (
+        "bc1qw508d6qejxtdg4y5r3zarvary0c5xw7kemeawh",
+        "Version 0 witness address must use Bech32 checksum",  # Invalid checksum (Bech32m instead of Bech32)
+        [],
+    ),
+    (
+        "tb1q0xlxvlhemja6c4dqv22uapctqupfhlxm9h8z3k2e72q4k9hcz7vq24jc47",
+        "Invalid or unsupported Segwit (Bech32) or Base58 encoding.",  # tb1, Invalid checksum (Bech32m instead of Bech32)
+        [],
+    ),
+    (
+        "bc1p38j9r5y49hruaue7wxjce0updqjuyyx0kh56v8s25huc6995vvpql3jow4",
+        "Invalid Base 32 character",  # Invalid character in checksum
+        [59],
+    ),
+    (
+        "BC130XLXVLHEMJA6C4DQV22UAPCTQUPFHLXM9H8Z3K2E72Q4K9HCZ7VQ7ZWS8R",
+        "Invalid Bech32 address witness version",
+        [],
+    ),
+    ("bc1pw5dgrnzv", "Invalid Bech32 address program size (1 byte)", []),
+    (
+        "bc1p0xlxvlhemja6c4dqv22uapctqupfhlxm9h8z3k2e72q4k9hcz7v8n0nx0muaewav253zgeav",
+        "Invalid Bech32 address program size (41 byte)",
+        [],
+    ),
+    (
+        "BC1QR508D6QEJXTDG4Y5R3ZARVARYV98GJ9P",
+        "Invalid Bech32 v0 address program size (16 byte), per BIP141",
+        [],
+    ),
+    (
+        "tb1p0xlxvlhemja6c4dqv22uapctqupfhlxm9h8z3k2e72q4k9hcz7vq47Zagq",
+        "Invalid or unsupported Segwit (Bech32) or Base58 encoding.",  # tb1, Mixed case
+        [],
+    ),
+    (
+        "bc1p0xlxvlhemja6c4dqv22uapctqupfhlxm9h8z3k2e72q4k9hcz7v07qwwzcrf",
+        "Invalid padding in Bech32 data section",  # zero padding of more than 4 bits
+        [],
+    ),
+    (
+        "tb1p0xlxvlhemja6c4dqv22uapctqupfhlxm9h8z3k2e72q4k9hcz7vpggkg4j",
+        "Invalid or unsupported Segwit (Bech32) or Base58 encoding.",  # tb1, Non-zero padding in 8-to-5 conversion
+        [],
+    ),
+    ("bc1gmk9yu", "Empty Bech32 data section", []),
+]
+VALID_DATA = [
+    # BIP 350
+    (
+        "BC1QW508D6QEJXTDG4Y5R3ZARVARY0C5XW7KV8F3T4",
+        "0014751e76e8199196d454941c45d1b3a323f1433bd6",
+    ),
+    # (
+    #   "tb1qrp33g0q5c5txsp9arysrx4k6zdkfs4nce4xj0gdcccefvpysxf3q0sl5k7",
+    #   "00201863143c14c5166804bd19203356da136c985678cd4d27a1b8c6329604903262",
+    # ),
+    (
+        "bc1qrp33g0q5c5txsp9arysrx4k6zdkfs4nce4xj0gdcccefvpysxf3qccfmv3",
+        "00201863143c14c5166804bd19203356da136c985678cd4d27a1b8c6329604903262",
+    ),
+    (
+        "bc1pw508d6qejxtdg4y5r3zarvary0c5xw7kw508d6qejxtdg4y5r3zarvary0c5xw7kt5nd6y",
+        "5128751e76e8199196d454941c45d1b3a323f1433bd6751e76e8199196d454941c45d1b3a323f1433bd6",
+    ),
+    ("BC1SW50QGDZ25J", "6002751e"),
+    ("bc1zw508d6qejxtdg4y5r3zarvaryvaxxpcs", "5210751e76e8199196d454941c45d1b3a323"),
+    # (
+    #   "tb1qqqqqp399et2xygdj5xreqhjjvcmzhxw4aywxecjdzew6hylgvsesrxh6hy",
+    #   "0020000000c4a5cad46221b2a187905e5266362b99d5e91c6ce24d165dab93e86433",
+    # ),
+    (
+        "bc1qqqqqp399et2xygdj5xreqhjjvcmzhxw4aywxecjdzew6hylgvses5wp4dt",
+        "0020000000c4a5cad46221b2a187905e5266362b99d5e91c6ce24d165dab93e86433",
+    ),
+    # (
+    #   "tb1pqqqqp399et2xygdj5xreqhjjvcmzhxw4aywxecjdzew6hylgvsesf3hn0c",
+    #   "5120000000c4a5cad46221b2a187905e5266362b99d5e91c6ce24d165dab93e86433",
+    # ),
+    (
+        "bc1pqqqqp399et2xygdj5xreqhjjvcmzhxw4aywxecjdzew6hylgvses7epu4h",
+        "5120000000c4a5cad46221b2a187905e5266362b99d5e91c6ce24d165dab93e86433",
+    ),
+    (
+        "bc1p0xlxvlhemja6c4dqv22uapctqupfhlxm9h8z3k2e72q4k9hcz7vqzk5jj0",
+        "512079be667ef9dcbbac55a06295ce870b07029bfcdb2dce28d959f2815b16f81798",
+    ),
+]
+
+
+class ValidateAddressMainTest(BitcoinTestFramework):
+    def set_test_params(self):
+        self.setup_clean_chain = True
+        self.chain = ""  # main
+        self.num_nodes = 1
+        self.extra_args = [["-prune=899"]] * self.num_nodes
+
+    def check_valid(self, addr, spk):
+        info = self.nodes[0].validateaddress(addr)
+        assert_equal(info["isvalid"], True)
+        assert_equal(info["scriptPubKey"], spk)
+        assert "error" not in info
+        assert "error_locations" not in info
+
+    def check_invalid(self, addr, error_str, error_locations):
+        res = self.nodes[0].validateaddress(addr)
+        assert_equal(res["isvalid"], False)
+        assert_equal(res["error"], error_str)
+        assert_equal(res["error_locations"], error_locations)
+
+    def test_validateaddress(self):
+        for (addr, error, locs) in INVALID_DATA:
+            self.check_invalid(addr, error, locs)
+        for (addr, spk) in VALID_DATA:
+            self.check_valid(addr, spk)
+
+    def run_test(self):
+        self.test_validateaddress()
+
+
+if __name__ == "__main__":
+    ValidateAddressMainTest().main()
diff --git a/test/functional/test_runner.py b/test/functional/test_runner.py
index 3dc22ccde3..a6f1bbae7d 100755
--- a/test/functional/test_runner.py
+++ b/test/functional/test_runner.py
@@ -171,6 +171,7 @@ BASE_SCRIPTS = [
     'wallet_fast_rescan.py --descriptors',
     'interface_zmq.py',
     'rpc_invalid_address_message.py',
+    'rpc_validateaddress.py',
     'interface_bitcoin_cli.py --legacy-wallet',
     'interface_bitcoin_cli.py --descriptors',
     'feature_bind_extra.py',