util: Don't allow Base58 decoding of non-Base58 strings. Add Base58 tests. #17721

pull practicalswift wants to merge 2 commits into bitcoin:master from practicalswift:less-liberal-base58-decoding changing 4 files +32 −1

practicalswift commented at 11:13 AM on December 11, 2019: contributor

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

in src/util/string.h:40 in 419fada5c4 outdated

  33 | @@ -31,4 +34,9 @@ inline std::string Join(const std::vector<std::string>& list, const std::string&
  34 |      return Join(list, separator, [](const std::string& i) { return i; });
  35 |  }
  36 |  
  37 | +NODISCARD inline bool ValidAsCString(const std::string& str) noexcept

laanwj commented at 11:18 AM on December 11, 2019:

Please add doc comment

practicalswift commented at 11:42 AM on December 11, 2019:

Added!

laanwj commented at 11:20 AM on December 11, 2019: member

The ValidAsCString function could also be used in ParsePrechecks in util/strencodings.cpp

Concept ACK, handling embedded \0 is a good thing
DrahtBot added the label Tests on Dec 11, 2019
DrahtBot added the label Utils/log/libs on Dec 11, 2019
practicalswift renamed this:
~~util: Don't allow Base58 decoding of non-Base58 strings. Add base58 tests.~~
util: Don't allow Base58 decoding of non-Base58 strings. Add Base58 tests.
on Dec 11, 2019
practicalswift force-pushed on Dec 11, 2019
practicalswift commented at 11:42 AM on December 11, 2019: contributor

@laanwj Done! Please re-review :)
DrahtBot commented at 5:29 PM on December 11, 2019: member


The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.



Conflicts

Reviewers, this pull request conflicts with the following ones:
- #17229 (tests: Add fuzzing harnesses for various Base{32,58,64} and hex related functions by practicalswift)
If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first.
DrahtBot added the label Needs rebase on Dec 12, 2019
practicalswift force-pushed on Dec 12, 2019
tests: Add tests for base58-decoding of std::string:s containing non-base58 characters ff7a999226
util: Don't allow base58-decoding of std::string:s containing non-base58 characters d945c6f5e6
practicalswift force-pushed on Dec 12, 2019
DrahtBot removed the label Needs rebase on Dec 12, 2019

MarcoFalke commented at 6:49 PM on December 12, 2019: member

ACK d945c6f5e6f61b6e289ac7da6834c18f1b677b0f 🚓

<details><summary>Show signature and timestamp</summary>

Signature:

Timestamp of file with hash 715c326e53e59aa78e26d17ed651e7efc2a70b43763dca45dca48f34fa22d05a -

</details>

laanwj commented at 10:15 AM on December 13, 2019: member

ACK d945c6f5e6f61b6e289ac7da6834c18f1b677b0f
laanwj referenced this in commit 995b6c83e1 on Dec 13, 2019
laanwj merged this on Dec 13, 2019
laanwj closed this on Dec 13, 2019
sidhujag referenced this in commit db2f0b32fa on Dec 13, 2019
jasonbcox referenced this in commit bd611af6fa on Oct 28, 2020
sidhujag referenced this in commit b4bf670575 on Nov 10, 2020
practicalswift deleted the branch on Apr 10, 2021
kittywhiskers referenced this in commit c8e14a3e41 on May 25, 2021
UdjinM6 referenced this in commit 50f95ca817 on May 28, 2021
furszy referenced this in commit 170beab56c on Jul 1, 2021
random-zebra referenced this in commit b4751e10ce on Aug 11, 2021
gades referenced this in commit 38cae596eb on Apr 26, 2022
DrahtBot locked this on Aug 16, 2022

Contributors

Labels

Linked (view graph)

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

util: Don't allow Base58 decoding of non-Base58 strings. Add Base58 tests. #17721

Conflicts