Skip to content

Commit 3914e87

Browse files
laanwjlaanwj
committed
Merge #17511: Add bounds checks before base58 decoding
5909bcd Add bounds checks in key_io before DecodeBase58Check (Pieter Wuille) 2bcf1fc Pass a maximum output length to DecodeBase58 and DecodeBase58Check (Pieter Wuille) Pull request description: Fixes #17501. ACKs for top commit: laanwj: code review ACK 5909bcd practicalswift: ACK 5909bcd -- code looks correct Tree-SHA512: 4807f4a9508dee9c0f1ad63f56f70f4ec4e6b7e35eb91322a525e3da3828521a41de9b8338a6bf67250803660b480d95fd02ce6b2fe79c4c88bc19b54f9d8889
2 parents 3f1966e + 5909bcd commit 3914e87

File tree

5 files changed

+41
-22
lines changed

5 files changed

+41
-22
lines changed

‎src/base58.cpp

Lines changed: 11 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -11,6 +11,8 @@
1111
#include <assert.h>
1212
#include <string.h>
1313

14+
#include <limits>
15+
1416
/** All alphanumeric characters except for "0", "I", "O", and "l" */
1517
static const char* pszBase58 = "123456789ABCDEFGHJKLMNPQRSTUVWXYZabcdefghijkmnopqrstuvwxyz";
1618
static const int8_t mapBase58[256] = {
@@ -32,7 +34,7 @@ static const int8_t mapBase58[256] = {
3234
-1,-1,-1,-1,-1,-1,-1,-1, -1,-1,-1,-1,-1,-1,-1,-1,
3335
};
3436

35-
bool DecodeBase58(const char* psz, std::vector<unsigned char>& vch)
37+
bool DecodeBase58(const char* psz, std::vector<unsigned char>& vch, int max_ret_len)
3638
{
3739
// Skip leading spaces.
3840
while (*psz && IsSpace(*psz))
@@ -42,6 +44,7 @@ bool DecodeBase58(const char* psz, std::vector<unsigned char>& vch)
4244
int length = 0;
4345
while (*psz == '1') {
4446
zeroes++;
47+
if (zeroes > max_ret_len) return false;
4548
psz++;
4649
}
4750
// Allocate enough space in big-endian base256 representation.
@@ -62,6 +65,7 @@ bool DecodeBase58(const char* psz, std::vector<unsigned char>& vch)
6265
}
6366
assert(carry == 0);
6467
length = i;
68+
if (length + zeroes > max_ret_len) return false;
6569
psz++;
6670
}
6771
// Skip trailing spaces.
@@ -71,8 +75,6 @@ bool DecodeBase58(const char* psz, std::vector<unsigned char>& vch)
7175
return false;
7276
// Skip leading zeroes in b256.
7377
std::vector<unsigned char>::iterator it = b256.begin() + (size - length);
74-
while (it != b256.end() && *it == 0)
75-
it++;
7678
// Copy result into output vector.
7779
vch.reserve(zeroes + (b256.end() - it));
7880
vch.assign(zeroes, 0x00);
@@ -126,9 +128,9 @@ std::string EncodeBase58(const std::vector<unsigned char>& vch)
126128
return EncodeBase58(vch.data(), vch.data() + vch.size());
127129
}
128130

129-
bool DecodeBase58(const std::string& str, std::vector<unsigned char>& vchRet)
131+
bool DecodeBase58(const std::string& str, std::vector<unsigned char>& vchRet, int max_ret_len)
130132
{
131-
return DecodeBase58(str.c_str(), vchRet);
133+
return DecodeBase58(str.c_str(), vchRet, max_ret_len);
132134
}
133135

134136
std::string EncodeBase58Check(const std::vector<unsigned char>& vchIn)
@@ -140,9 +142,9 @@ std::string EncodeBase58Check(const std::vector<unsigned char>& vchIn)
140142
return EncodeBase58(vch);
141143
}
142144

143-
bool DecodeBase58Check(const char* psz, std::vector<unsigned char>& vchRet)
145+
bool DecodeBase58Check(const char* psz, std::vector<unsigned char>& vchRet, int max_ret_len)
144146
{
145-
if (!DecodeBase58(psz, vchRet) ||
147+
if (!DecodeBase58(psz, vchRet, max_ret_len > std::numeric_limits<int>::max() - 4 ? std::numeric_limits<int>::max() : max_ret_len + 4) ||
146148
(vchRet.size() < 4)) {
147149
vchRet.clear();
148150
return false;
@@ -157,7 +159,7 @@ bool DecodeBase58Check(const char* psz, std::vector<unsigned char>& vchRet)
157159
return true;
158160
}
159161

160-
bool DecodeBase58Check(const std::string& str, std::vector<unsigned char>& vchRet)
162+
bool DecodeBase58Check(const std::string& str, std::vector<unsigned char>& vchRet, int max_ret)
161163
{
162-
return DecodeBase58Check(str.c_str(), vchRet);
164+
return DecodeBase58Check(str.c_str(), vchRet, max_ret);
163165
}

‎src/base58.h

Lines changed: 4 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -35,13 +35,13 @@ std::string EncodeBase58(const std::vector<unsigned char>& vch);
3535
* return true if decoding is successful.
3636
* psz cannot be nullptr.
3737
*/
38-
NODISCARD bool DecodeBase58(const char* psz, std::vector<unsigned char>& vchRet);
38+
NODISCARD bool DecodeBase58(const char* psz, std::vector<unsigned char>& vchRet, int max_ret_len);
3939

4040
/**
4141
* Decode a base58-encoded string (str) into a byte vector (vchRet).
4242
* return true if decoding is successful.
4343
*/
44-
NODISCARD bool DecodeBase58(const std::string& str, std::vector<unsigned char>& vchRet);
44+
NODISCARD bool DecodeBase58(const std::string& str, std::vector<unsigned char>& vchRet, int max_ret_len);
4545

4646
/**
4747
* Encode a byte vector into a base58-encoded string, including checksum
@@ -52,12 +52,12 @@ std::string EncodeBase58Check(const std::vector<unsigned char>& vchIn);
5252
* Decode a base58-encoded string (psz) that includes a checksum into a byte
5353
* vector (vchRet), return true if decoding is successful
5454
*/
55-
NODISCARD bool DecodeBase58Check(const char* psz, std::vector<unsigned char>& vchRet);
55+
NODISCARD bool DecodeBase58Check(const char* psz, std::vector<unsigned char>& vchRet, int max_ret_len);
5656

5757
/**
5858
* Decode a base58-encoded string (str) that includes a checksum into a byte
5959
* vector (vchRet), return true if decoding is successful
6060
*/
61-
NODISCARD bool DecodeBase58Check(const std::string& str, std::vector<unsigned char>& vchRet);
61+
NODISCARD bool DecodeBase58Check(const std::string& str, std::vector<unsigned char>& vchRet, int max_ret_len);
6262

6363
#endif // BITCOIN_BASE58_H

‎src/bench/base58.cpp

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -47,7 +47,7 @@ static void Base58Decode(benchmark::State& state)
4747
const char* addr = "17VZNX1SN5NtKa8UQFxwQbFeFc3iqRYhem";
4848
std::vector<unsigned char> vch;
4949
while (state.KeepRunning()) {
50-
(void) DecodeBase58(addr, vch);
50+
(void) DecodeBase58(addr, vch, 64);
5151
}
5252
}
5353

‎src/key_io.cpp

Lines changed: 4 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -73,7 +73,7 @@ CTxDestination DecodeDestination(const std::string& str, const CChainParams& par
7373
{
7474
std::vector<unsigned char> data;
7575
uint160 hash;
76-
if (DecodeBase58Check(str, data)) {
76+
if (DecodeBase58Check(str, data, 21)) {
7777
// base58-encoded Bitcoin addresses.
7878
// Public-key-hash-addresses have version 0 (or 111 testnet).
7979
// The data vector contains RIPEMD160(SHA256(pubkey)), where pubkey is the serialized public key.
@@ -133,7 +133,7 @@ CKey DecodeSecret(const std::string& str)
133133
{
134134
CKey key;
135135
std::vector<unsigned char> data;
136-
if (DecodeBase58Check(str, data)) {
136+
if (DecodeBase58Check(str, data, 34)) {
137137
const std::vector<unsigned char>& privkey_prefix = Params().Base58Prefix(CChainParams::SECRET_KEY);
138138
if ((data.size() == 32 + privkey_prefix.size() || (data.size() == 33 + privkey_prefix.size() && data.back() == 1)) &&
139139
std::equal(privkey_prefix.begin(), privkey_prefix.end(), data.begin())) {
@@ -164,7 +164,7 @@ CExtPubKey DecodeExtPubKey(const std::string& str)
164164
{
165165
CExtPubKey key;
166166
std::vector<unsigned char> data;
167-
if (DecodeBase58Check(str, data)) {
167+
if (DecodeBase58Check(str, data, 78)) {
168168
const std::vector<unsigned char>& prefix = Params().Base58Prefix(CChainParams::EXT_PUBLIC_KEY);
169169
if (data.size() == BIP32_EXTKEY_SIZE + prefix.size() && std::equal(prefix.begin(), prefix.end(), data.begin())) {
170170
key.Decode(data.data() + prefix.size());
@@ -187,7 +187,7 @@ CExtKey DecodeExtKey(const std::string& str)
187187
{
188188
CExtKey key;
189189
std::vector<unsigned char> data;
190-
if (DecodeBase58Check(str, data)) {
190+
if (DecodeBase58Check(str, data, 78)) {
191191
const std::vector<unsigned char>& prefix = Params().Base58Prefix(CChainParams::EXT_SECRET_KEY);
192192
if (data.size() == BIP32_EXTKEY_SIZE + prefix.size() && std::equal(prefix.begin(), prefix.end(), data.begin())) {
193193
key.Decode(data.data() + prefix.size());

‎src/test/base58_tests.cpp

Lines changed: 21 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -7,6 +7,7 @@
77
#include <base58.h>
88
#include <test/util/setup_common.h>
99
#include <util/strencodings.h>
10+
#include <util/vector.h>
1011

1112
#include <univalue.h>
1213

@@ -53,17 +54,33 @@ BOOST_AUTO_TEST_CASE(base58_DecodeBase58)
5354
}
5455
std::vector<unsigned char> expected = ParseHex(test[0].get_str());
5556
std::string base58string = test[1].get_str();
56-
BOOST_CHECK_MESSAGE(DecodeBase58(base58string, result), strTest);
57+
BOOST_CHECK_MESSAGE(DecodeBase58(base58string, result, 256), strTest);
5758
BOOST_CHECK_MESSAGE(result.size() == expected.size() && std::equal(result.begin(), result.end(), expected.begin()), strTest);
5859
}
5960

60-
BOOST_CHECK(!DecodeBase58("invalid", result));
61+
BOOST_CHECK(!DecodeBase58("invalid", result, 100));
6162

6263
// check that DecodeBase58 skips whitespace, but still fails with unexpected non-whitespace at the end.
63-
BOOST_CHECK(!DecodeBase58(" \t\n\v\f\r skip \r\f\v\n\t a", result));
64-
BOOST_CHECK( DecodeBase58(" \t\n\v\f\r skip \r\f\v\n\t ", result));
64+
BOOST_CHECK(!DecodeBase58(" \t\n\v\f\r skip \r\f\v\n\t a", result, 3));
65+
BOOST_CHECK( DecodeBase58(" \t\n\v\f\r skip \r\f\v\n\t ", result, 3));
6566
std::vector<unsigned char> expected = ParseHex("971a55");
6667
BOOST_CHECK_EQUAL_COLLECTIONS(result.begin(), result.end(), expected.begin(), expected.end());
6768
}
6869

70+
BOOST_AUTO_TEST_CASE(base58_random_encode_decode)
71+
{
72+
for (int n = 0; n < 1000; ++n) {
73+
unsigned int len = 1 + InsecureRandBits(8);
74+
unsigned int zeroes = InsecureRandBool() ? InsecureRandRange(len + 1) : 0;
75+
auto data = Cat(std::vector<unsigned char>(zeroes, '\000'), g_insecure_rand_ctx.randbytes(len - zeroes));
76+
auto encoded = EncodeBase58Check(data);
77+
std::vector<unsigned char> decoded;
78+
auto ok_too_small = DecodeBase58Check(encoded, decoded, InsecureRandRange(len));
79+
BOOST_CHECK(!ok_too_small);
80+
auto ok = DecodeBase58Check(encoded, decoded, len + InsecureRandRange(257 - len));
81+
BOOST_CHECK(ok);
82+
BOOST_CHECK(data == decoded);
83+
}
84+
}
85+
6986
BOOST_AUTO_TEST_SUITE_END()

0 commit comments

Comments
 (0)