DecodeBase58(const std::string& str, …) is a bit too liberal when decoding? #17718

When fuzzing the base58 functions I noticed that DecodeBase58(const std::string& str, …) is perhaps a bit too liberal in what input it accepts when decoding.

I naïvely assumed that DecodeBase58(const std::string& str, …) would return false when passing it a str which contains at least one character that is not any of the base58 characters (all alphanumeric characters except for "0", "I", "O", and "l") or the whitespace characters.

That is not the case:

• str contains good → DecodeBase58(const std::string& str, …) == true (as expected)
• str contains bad0IOl → DecodeBase58(const std::string& str, …) == false (as expected)
• str contains good\x00bad0IOl → DecodeBase58(const std::string& str, …) == true(!)

The reason is that DecodeBase58(const std::string& str, …) calls DecodeBase58(const char* psz, …) which will only consider the string up to the first \x00.

The same applies also to DecodeBase58Check(const std::string& str, …), DecodeBase32(const std::string& str, …) and DecodeBase64(const std::string& str, …).

Also ParseMoney(const std::string&, …) and ParseHex(const std::string& …).

This is a list of functions that might be worth checking for potential problems introduced by "\0-stuffing".

I'll take a look :)

The candidate list was generated using git grep -E '(\(const std::string *&|[a-zA-Z0-9_]\(const (unsigned |)char *\*)' and then filtered manually.

src/arith_uint256.cpp:template void base_uint<256>::SetHex(const char*);
src/arith_uint256.cpp:template void base_uint<256>::SetHex(const std::string&);

src/arith_uint256.cpp:void base_uint<BITS>::SetHex(const char* psz)
src/arith_uint256.cpp:void base_uint<BITS>::SetHex(const std::string& str)

src/base58.cpp:bool DecodeBase58(const char* psz, std::vector<unsigned char>& vch)
src/base58.cpp:bool DecodeBase58(const std::string& str, std::vector<unsigned char>& vchRet)

src/base58.cpp:bool DecodeBase58Check(const char* psz, std::vector<unsigned char>& vchRet)
src/base58.cpp:bool DecodeBase58Check(const std::string& str, std::vector<unsigned char>& vchRet)

src/uint256.cpp:template void base_blob<160>::SetHex(const char*);
src/uint256.cpp:template void base_blob<160>::SetHex(const std::string&);

src/uint256.cpp:template void base_blob<256>::SetHex(const char*);
src/uint256.cpp:template void base_blob<256>::SetHex(const std::string&);

src/uint256.cpp:void base_blob<BITS>::SetHex(const char* psz)
src/uint256.cpp:void base_blob<BITS>::SetHex(const std::string& str)

src/uint256.h:inline uint256 uint256S(const char *str)
src/uint256.h:inline uint256 uint256S(const std::string& str)

src/util/moneystr.cpp:bool ParseMoney(const char* pszIn, CAmount& nRet)
src/util/moneystr.cpp:bool ParseMoney(const std::string& str, CAmount& nRet)

src/util/strencodings.cpp:int64_t atoi64(const char* psz)
src/util/strencodings.cpp:int64_t atoi64(const std::string& str)

src/util/strencodings.cpp:std::vector<unsigned char> DecodeBase32(const char* p, bool* pf_invalid)
src/util/strencodings.cpp:std::string DecodeBase32(const std::string& str, bool* pf_invalid)

src/util/strencodings.cpp:std::vector<unsigned char> DecodeBase64(const char* p, bool* pf_invalid)
src/util/strencodings.cpp:std::string DecodeBase64(const std::string& str, bool* pf_invalid)

src/util/strencodings.cpp:std::string EncodeBase32(const unsigned char* pch, size_t len)
src/util/strencodings.cpp:std::string EncodeBase32(const std::string& str)

src/util/strencodings.cpp:std::string EncodeBase64(const unsigned char* pch, size_t len)
src/util/strencodings.cpp:std::string EncodeBase64(const std::string& str)

src/util/strencodings.cpp:std::vector<unsigned char> ParseHex(const char* psz)
src/util/strencodings.cpp:std::vector<unsigned char> ParseHex(const std::string& str)

The thing has always been that a lot of those don't even have a mechanism to report parse errors. Handling \0 would just be the icing on the cake so to say… (I had a similar idea in #17385 to get rid of some of these, but I think it's just too deep a rabbit hole)