diff options
author | Wladimir J. van der Laan <laanwj@protonmail.com> | 2019-12-13 11:15:18 +0100 |
---|---|---|
committer | Wladimir J. van der Laan <laanwj@protonmail.com> | 2019-12-13 11:15:28 +0100 |
commit | 995b6c83e1ae23bb32bf1b9d19635036b92f54ef (patch) | |
tree | 88cbcf8c6f16bfe750efb637984e56157a8731aa | |
parent | d4b335c60af46eb631beb38a9d6c4d55e4c0ffc3 (diff) | |
parent | d945c6f5e6f61b6e289ac7da6834c18f1b677b0f (diff) | |
download | bitcoin-995b6c83e1ae23bb32bf1b9d19635036b92f54ef.tar.xz |
Merge #17721: util: Don't allow Base58 decoding of non-Base58 strings. Add Base58 tests.
d945c6f5e6f61b6e289ac7da6834c18f1b677b0f util: Don't allow base58-decoding of std::string:s containing non-base58 characters (practicalswift)
ff7a9992263f5a19f73097c86068b6150d213c23 tests: Add tests for base58-decoding of std::string:s containing non-base58 characters (practicalswift)
Pull request description:
Don't allow Base58 decoding of non-Base58 strings. Add Base58 tests.
Fixes #17718.
Added tests before the Base58 decoding patch:
```
$ make check
…
test/base58_tests.cpp(62): error: in "base58_tests/base58_DecodeBase58":
check !DecodeBase58(std::string("\0invalid", 8), result) has failed
test/base58_tests.cpp(67): error: in "base58_tests/base58_DecodeBase58":
check !DecodeBase58(std::string("good\0bad0IOl", 12), result) has failed
test/base58_tests.cpp(76): error: in "base58_tests/base58_DecodeBase58":
check !DecodeBase58Check(std::string("3vQB7B6MrGQZaxCuFg4oh\00IOl", 26), result) has failed
*** 3 failures are detected in the test module "Bitcoin Core Test Suite"
…
$ echo $?
1
```
Added tests before the Base58 decoding patch:
```
$ make check
…
OK
…
$ echo $?
0
```
ACKs for top commit:
MarcoFalke:
ACK d945c6f5e6f61b6e289ac7da6834c18f1b677b0f 🚓
laanwj:
ACK d945c6f5e6f61b6e289ac7da6834c18f1b677b0f
Tree-SHA512: 78fee3a18718c9cfbf2e4b26daaf8f24b4deca00475b7b254fec7f8be740f8898c696d9cd0eaa7c50bca55909b9dff3b516b6fe4db92dc132dcc0a1c5e3d61af
-rw-r--r-- | src/base58.cpp | 7 | ||||
-rw-r--r-- | src/test/base58_tests.cpp | 12 | ||||
-rw-r--r-- | src/util/strencodings.cpp | 3 | ||||
-rw-r--r-- | src/util/string.h | 11 |
4 files changed, 32 insertions, 1 deletions
diff --git a/src/base58.cpp b/src/base58.cpp index a0149fb641..17d3f86ba8 100644 --- a/src/base58.cpp +++ b/src/base58.cpp @@ -7,6 +7,7 @@ #include <hash.h> #include <uint256.h> #include <util/strencodings.h> +#include <util/string.h> #include <assert.h> #include <string.h> @@ -130,6 +131,9 @@ std::string EncodeBase58(const std::vector<unsigned char>& vch) bool DecodeBase58(const std::string& str, std::vector<unsigned char>& vchRet, int max_ret_len) { + if (!ValidAsCString(str)) { + return false; + } return DecodeBase58(str.c_str(), vchRet, max_ret_len); } @@ -161,5 +165,8 @@ bool DecodeBase58Check(const char* psz, std::vector<unsigned char>& vchRet, int bool DecodeBase58Check(const std::string& str, std::vector<unsigned char>& vchRet, int max_ret) { + if (!ValidAsCString(str)) { + return false; + } return DecodeBase58Check(str.c_str(), vchRet, max_ret); } diff --git a/src/test/base58_tests.cpp b/src/test/base58_tests.cpp index 96fdf8c86d..57559fa687 100644 --- a/src/test/base58_tests.cpp +++ b/src/test/base58_tests.cpp @@ -59,12 +59,24 @@ BOOST_AUTO_TEST_CASE(base58_DecodeBase58) } BOOST_CHECK(!DecodeBase58("invalid", result, 100)); + BOOST_CHECK(!DecodeBase58(std::string("invalid"), result, 100)); + BOOST_CHECK(!DecodeBase58(std::string("\0invalid", 8), result, 100)); + + BOOST_CHECK(DecodeBase58(std::string("good", 4), result, 100)); + BOOST_CHECK(!DecodeBase58(std::string("bad0IOl", 7), result, 100)); + BOOST_CHECK(!DecodeBase58(std::string("goodbad0IOl", 11), result, 100)); + BOOST_CHECK(!DecodeBase58(std::string("good\0bad0IOl", 12), 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, 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_CHECK(DecodeBase58Check(std::string("3vQB7B6MrGQZaxCuFg4oh", 21), result, 100)); + BOOST_CHECK(!DecodeBase58Check(std::string("3vQB7B6MrGQZaxCuFg4oi", 21), result, 100)); + BOOST_CHECK(!DecodeBase58Check(std::string("3vQB7B6MrGQZaxCuFg4oh0IOl", 25), result, 100)); + BOOST_CHECK(!DecodeBase58Check(std::string("3vQB7B6MrGQZaxCuFg4oh\00IOl", 26), result, 100)); } BOOST_AUTO_TEST_CASE(base58_random_encode_decode) diff --git a/src/util/strencodings.cpp b/src/util/strencodings.cpp index 46042f5634..8f2d05f03b 100644 --- a/src/util/strencodings.cpp +++ b/src/util/strencodings.cpp @@ -4,6 +4,7 @@ // file COPYING or http://www.opensource.org/licenses/mit-license.php. #include <util/strencodings.h> +#include <util/string.h> #include <tinyformat.h> @@ -269,7 +270,7 @@ NODISCARD static bool ParsePrechecks(const std::string& str) return false; if (str.size() >= 1 && (IsSpace(str[0]) || IsSpace(str[str.size()-1]))) // No padding allowed return false; - if (str.size() != strlen(str.c_str())) // No embedded NUL characters allowed + if (!ValidAsCString(str)) // No embedded NUL characters allowed return false; return true; } diff --git a/src/util/string.h b/src/util/string.h index 76a83a4949..c6fa08e5b3 100644 --- a/src/util/string.h +++ b/src/util/string.h @@ -5,6 +5,9 @@ #ifndef BITCOIN_UTIL_STRING_H #define BITCOIN_UTIL_STRING_H +#include <attributes.h> + +#include <cstring> #include <string> #include <vector> @@ -31,4 +34,12 @@ inline std::string Join(const std::vector<std::string>& list, const std::string& return Join(list, separator, [](const std::string& i) { return i; }); } +/** + * Check if a string does not contain any embedded NUL (\0) characters + */ +NODISCARD inline bool ValidAsCString(const std::string& str) noexcept +{ + return str.size() == strlen(str.c_str()); +} + #endif // BITCOIN_UTIL_STRENCODINGS_H |