Skip to content

Commit

Permalink
Merge #17511: Add bounds checks before base58 decoding
Browse files Browse the repository at this point in the history 
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
  • Loading branch information
@laanwj
laanwj committed Dec 12, 2019
2 parents 3f1966e + 5909bcd commit 3914e87
Show file tree
Hide file tree
Showing 5 changed files with 41 additions and 22 deletions.
20 changes: 11 additions & 9 deletions src/base58.cpp
View file
Expand Up @@ -11,6 +11,8 @@
#include <assert.h>
#include <string.h>

#include <limits>

/** All alphanumeric characters except for "0", "I", "O", and "l" */
static const char* pszBase58 = "123456789ABCDEFGHJKLMNPQRSTUVWXYZabcdefghijkmnopqrstuvwxyz";
static const int8_t mapBase58[256] = {
Expand All @@ -32,7 +34,7 @@ static const int8_t mapBase58[256] = {
-1,-1,-1,-1,-1,-1,-1,-1, -1,-1,-1,-1,-1,-1,-1,-1,
};

bool DecodeBase58(const char* psz, std::vector<unsigned char>& vch)
bool DecodeBase58(const char* psz, std::vector<unsigned char>& vch, int max_ret_len)
{
// Skip leading spaces.
while (*psz && IsSpace(*psz))
Expand All @@ -42,6 +44,7 @@ bool DecodeBase58(const char* psz, std::vector<unsigned char>& vch)
int length = 0;
while (*psz == '1') {
zeroes++;
if (zeroes > max_ret_len) return false;
psz++;
}
// Allocate enough space in big-endian base256 representation.
Expand All @@ -62,6 +65,7 @@ bool DecodeBase58(const char* psz, std::vector<unsigned char>& vch)
}
assert(carry == 0);
length = i;
if (length + zeroes > max_ret_len) return false;
psz++;
}
// Skip trailing spaces.
Expand All @@ -71,8 +75,6 @@ bool DecodeBase58(const char* psz, std::vector<unsigned char>& vch)
return false;
// Skip leading zeroes in b256.
std::vector<unsigned char>::iterator it = b256.begin() + (size - length);
while (it != b256.end() && *it == 0)
it++;
// Copy result into output vector.
vch.reserve(zeroes + (b256.end() - it));
vch.assign(zeroes, 0x00);
Expand Down Expand Up @@ -126,9 +128,9 @@ std::string EncodeBase58(const std::vector<unsigned char>& vch)
return EncodeBase58(vch.data(), vch.data() + vch.size());
}

bool DecodeBase58(const std::string& str, std::vector<unsigned char>& vchRet)
bool DecodeBase58(const std::string& str, std::vector<unsigned char>& vchRet, int max_ret_len)
{
return DecodeBase58(str.c_str(), vchRet);
return DecodeBase58(str.c_str(), vchRet, max_ret_len);
}

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

bool DecodeBase58Check(const char* psz, std::vector<unsigned char>& vchRet)
bool DecodeBase58Check(const char* psz, std::vector<unsigned char>& vchRet, int max_ret_len)
{
if (!DecodeBase58(psz, vchRet) ||
if (!DecodeBase58(psz, vchRet, max_ret_len > std::numeric_limits<int>::max() - 4 ? std::numeric_limits<int>::max() : max_ret_len + 4) ||
(vchRet.size() < 4)) {
vchRet.clear();
return false;
Expand All @@ -157,7 +159,7 @@ bool DecodeBase58Check(const char* psz, std::vector<unsigned char>& vchRet)
return true;
}

bool DecodeBase58Check(const std::string& str, std::vector<unsigned char>& vchRet)
bool DecodeBase58Check(const std::string& str, std::vector<unsigned char>& vchRet, int max_ret)
{
return DecodeBase58Check(str.c_str(), vchRet);
return DecodeBase58Check(str.c_str(), vchRet, max_ret);
}
8 changes: 4 additions & 4 deletions src/base58.h
View file
Expand Up @@ -35,13 +35,13 @@ std::string EncodeBase58(const std::vector<unsigned char>& vch);
* return true if decoding is successful.
* psz cannot be nullptr.
*/
NODISCARD bool DecodeBase58(const char* psz, std::vector<unsigned char>& vchRet);
NODISCARD bool DecodeBase58(const char* psz, std::vector<unsigned char>& vchRet, int max_ret_len);

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

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

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

#endif // BITCOIN_BASE58_H
2 changes: 1 addition & 1 deletion src/bench/base58.cpp
View file
Expand Up @@ -47,7 +47,7 @@ static void Base58Decode(benchmark::State& state)
const char* addr = "17VZNX1SN5NtKa8UQFxwQbFeFc3iqRYhem";
std::vector<unsigned char> vch;
while (state.KeepRunning()) {
(void) DecodeBase58(addr, vch);
(void) DecodeBase58(addr, vch, 64);
}
}

Expand Down
8 changes: 4 additions & 4 deletions src/key_io.cpp
View file
Expand Up @@ -73,7 +73,7 @@ CTxDestination DecodeDestination(const std::string& str, const CChainParams& par
{
std::vector<unsigned char> data;
uint160 hash;
if (DecodeBase58Check(str, data)) {
if (DecodeBase58Check(str, data, 21)) {
// base58-encoded Bitcoin addresses.
// Public-key-hash-addresses have version 0 (or 111 testnet).
// The data vector contains RIPEMD160(SHA256(pubkey)), where pubkey is the serialized public key.
Expand Down Expand Up @@ -133,7 +133,7 @@ CKey DecodeSecret(const std::string& str)
{
CKey key;
std::vector<unsigned char> data;
if (DecodeBase58Check(str, data)) {
if (DecodeBase58Check(str, data, 34)) {
const std::vector<unsigned char>& privkey_prefix = Params().Base58Prefix(CChainParams::SECRET_KEY);
if ((data.size() == 32 + privkey_prefix.size() || (data.size() == 33 + privkey_prefix.size() && data.back() == 1)) &&
std::equal(privkey_prefix.begin(), privkey_prefix.end(), data.begin())) {
Expand Down Expand Up @@ -164,7 +164,7 @@ CExtPubKey DecodeExtPubKey(const std::string& str)
{
CExtPubKey key;
std::vector<unsigned char> data;
if (DecodeBase58Check(str, data)) {
if (DecodeBase58Check(str, data, 78)) {
const std::vector<unsigned char>& prefix = Params().Base58Prefix(CChainParams::EXT_PUBLIC_KEY);
if (data.size() == BIP32_EXTKEY_SIZE + prefix.size() && std::equal(prefix.begin(), prefix.end(), data.begin())) {
key.Decode(data.data() + prefix.size());
Expand All @@ -187,7 +187,7 @@ CExtKey DecodeExtKey(const std::string& str)
{
CExtKey key;
std::vector<unsigned char> data;
if (DecodeBase58Check(str, data)) {
if (DecodeBase58Check(str, data, 78)) {
const std::vector<unsigned char>& prefix = Params().Base58Prefix(CChainParams::EXT_SECRET_KEY);
if (data.size() == BIP32_EXTKEY_SIZE + prefix.size() && std::equal(prefix.begin(), prefix.end(), data.begin())) {
key.Decode(data.data() + prefix.size());
Expand Down
25 changes: 21 additions & 4 deletions src/test/base58_tests.cpp
View file
Expand Up @@ -7,6 +7,7 @@
#include <base58.h>
#include <test/util/setup_common.h>
#include <util/strencodings.h>
#include <util/vector.h>

#include <univalue.h>

Expand Down Expand Up @@ -53,17 +54,33 @@ BOOST_AUTO_TEST_CASE(base58_DecodeBase58)
}
std::vector<unsigned char> expected = ParseHex(test[0].get_str());
std::string base58string = test[1].get_str();
BOOST_CHECK_MESSAGE(DecodeBase58(base58string, result), strTest);
BOOST_CHECK_MESSAGE(DecodeBase58(base58string, result, 256), strTest);
BOOST_CHECK_MESSAGE(result.size() == expected.size() && std::equal(result.begin(), result.end(), expected.begin()), strTest);
}

BOOST_CHECK(!DecodeBase58("invalid", result));
BOOST_CHECK(!DecodeBase58("invalid", result, 100));

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

BOOST_AUTO_TEST_CASE(base58_random_encode_decode)
{
for (int n = 0; n < 1000; ++n) {
unsigned int len = 1 + InsecureRandBits(8);
unsigned int zeroes = InsecureRandBool() ? InsecureRandRange(len + 1) : 0;
auto data = Cat(std::vector<unsigned char>(zeroes, '\000'), g_insecure_rand_ctx.randbytes(len - zeroes));
auto encoded = EncodeBase58Check(data);
std::vector<unsigned char> decoded;
auto ok_too_small = DecodeBase58Check(encoded, decoded, InsecureRandRange(len));
BOOST_CHECK(!ok_too_small);
auto ok = DecodeBase58Check(encoded, decoded, len + InsecureRandRange(257 - len));
BOOST_CHECK(ok);
BOOST_CHECK(data == decoded);
}
}

BOOST_AUTO_TEST_SUITE_END()

0 comments on commit 3914e87

Please sign in to comment.