Skip to content

Commit

Permalink
Merge bitcoin#27727: rpc: Fix invalid bech32 address handling
Browse files Browse the repository at this point in the history 
eeee55f 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 bitcoin#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 eeee55f
  brunoerg:
    crACK eeee55f

Tree-SHA512: c8639ee49e2a54b740b72d66bc4a40352dd553a6e3220dea9f94e48e33124f21f597a2817cb405d0a4c88d21df1013c0a4877a01370a2d326aa2cff1f9c381a8
  • Loading branch information
@achow101
achow101 committed May 24, 2023
2 parents 51c0507 + eeee55f commit a13f374
Show file tree
Hide file tree
Showing 4 changed files with 217 additions and 6 deletions.
13 changes: 10 additions & 3 deletions src/key_io.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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);
Expand Down Expand Up @@ -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();
}

Expand All @@ -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();
}

Expand All @@ -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();
}
}

Expand Down
6 changes: 3 additions & 3 deletions test/functional/rpc_invalid_address_message.py
View file
Original file line number Diff line number Diff line change
Expand Up @@ -63,12 +63,12 @@ def check_invalid(self, addr, error_str, error_locations=None):

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])
Expand Down Expand Up @@ -105,7 +105,7 @@ def test_validateaddress(self):
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)
Expand Down
203 changes: 203 additions & 0 deletions test/functional/rpc_validateaddress.py
View file
Original file line number Diff line number Diff line change
@@ -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()
1 change: 1 addition & 0 deletions test/functional/test_runner.py
View file
Original file line number Diff line number Diff line change
Expand Up @@ -171,6 +171,7 @@
'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',
Expand Down

0 comments on commit a13f374

Please sign in to comment.