Skip to content

Add bounds checks before base58 decoding #17511

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Merged
merged 2 commits into from
Dec 12, 2019
Merged

Add bounds checks before base58 decoding #17511

merged 2 commits into from
Dec 12, 2019

Conversation

sipa
Copy link
Member

@sipa sipa commented Nov 18, 2019

Fixes #17501.

maflcko
maflcko reviewed Nov 19, 2019
View reviewed changes
@practicalswift
Copy link
Contributor

Concept ACK

Thanks for fixing this!

@sipa sipa force-pushed the 201911_boundedbase58 branch from 1dfd33b to bfa56d8 Compare November 19, 2019 06:50
@laanwj
Copy link
Member

laanwj commented Nov 19, 2019 •
edited
Loading

I still stand by what I said in #17501 (comment)

I would say it's not the encoder/decoder responsibility to check input sizes. Good generic code works for any input size.
But of course, the application side (e.g. address parsing routines) could have a check to see if inputs are reasonable. They have that knowledge.

Checking input sizes is not the responsibility of the decoding code, but of application specific code.

Never mind, you're doing this. I was surprised at the complexity of the DecodeBase58Size function, that it ignores a potentially infinite amount of spaces and ones. Don't we have tighter constraints on address lengths and such? I guess apparently not!

Concept ACK.

@sipa
Copy link
Member Author

sipa commented Nov 19, 2019

@laanwj Hmm, I think there is a better solution than this. We do have upper bounds on the encoded data as well, which would be fine. Alternatively, we could just make DecodeBase58(Check) take an additional argument for the max size on the encoded data.

@laanwj
Copy link
Member

laanwj commented Nov 19, 2019 •
edited
Loading

Ah yes, the number of '0' is not really unbounded, because they are counted and signify the padding. It basically just ignores an infinite amount of spaces around it. That's fine.

I think this solution is OK!

Alternatively, we could just make DecodeBase58(Check) take an additional argument for the max size on the encoded data.

I kind of like having a separate function to estimate the size. However, this would make it easier to ensure consistency between the functions, I guess.

laanwj
laanwj reviewed Nov 19, 2019
View reviewed changes
laanwj
laanwj reviewed Nov 19, 2019
View reviewed changes
@practicalswift practicalswift mentioned this pull request Nov 19, 2019
Closed
@sipa
Pass a maximum output length to DecodeBase58 and DecodeBase58Check
2bcf1fc 
Also remove a needless loop in DecodeBase58 to prune zeroes in the base256
output of the conversion. The number of zeroes is implied by keeping track
explicitly of the length during the loop.
@sipa sipa force-pushed the 201911_boundedbase58 branch from bfa56d8 to ff60592 Compare November 19, 2019 23:40
@Sjors
Copy link
Member

Sjors commented Nov 20, 2019

Note that there are various places, e.g. PSBT parsing, that call DecodeBase64 in strencodings directly, bypassing these new bounds checks.

@laanwj
Copy link
Member

laanwj commented Nov 20, 2019 •
edited
Loading

@Sjors DecodeBase64 doesn't have this problem, its runtime is linear on the input size.

@practicalswift
Copy link
Contributor

@sipa

I think we should drop the default for max_ret_len since the default of std::numeric_limits<int>::max() is not a good or "natural" choice for most callers.

I think it would be preferable to not have a default for max_ret_len to make sure callers make an informed decision regarding max_ret_len.

Makes sense?

@sipa sipa force-pushed the 201911_boundedbase58 branch from ff60592 to 5909bcd Compare December 6, 2019 00:32
@sipa
Copy link
Member Author

sipa commented Dec 6, 2019

@practicalswift Done.

@laanwj
Copy link
Member

laanwj commented Dec 6, 2019 •
edited
Loading

code review ACK 5909bcd
(definitely agree that this is easier to review than the size estimation as separate function)

@practicalswift
Copy link
Contributor

practicalswift commented Dec 6, 2019 •
edited
Loading

ACK 5909bcd -- code looks correct

FWIW there is a base58 fuzzer in #17229 (currently at zero concept ACKs :)) which would be nice to have in the tree in addition to the unit tests to get further testing.

@DrahtBot
Copy link
Contributor

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

Conflicts

Reviewers, this pull request conflicts with the following ones:

  • #17721 (util: Don't allow Base58 decoding of non-Base58 strings. Add Base58 tests. by practicalswift)
  • #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.

laanwj added a commit that referenced this pull request Dec 12, 2019
@laanwj
Merge #17511: Add bounds checks before base58 decoding
3914e87 
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
@laanwj laanwj merged commit 5909bcd into bitcoin:master Dec 12, 2019
sidhujag pushed a commit to syscoin/syscoin that referenced this pull request Dec 12, 2019
@laanwj
Merge bitcoin#17511: Add bounds checks before base58 decoding
16c91db 
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 bitcoin#17501.

ACKs for top commit:
  laanwj:
    code review ACK 5909bcd
  practicalswift:
    ACK 5909bcd -- code looks correct

Tree-SHA512: 4807f4a9508dee9c0f1ad63f56f70f4ec4e6b7e35eb91322a525e3da3828521a41de9b8338a6bf67250803660b480d95fd02ce6b2fe79c4c88bc19b54f9d8889
@MarkLTZ MarkLTZ mentioned this pull request Apr 4, 2020
Closed
jasonbcox pushed a commit to Bitcoin-ABC/bitcoin-abc that referenced this pull request Oct 28, 2020
@sipa @Fabcien
Add bounds checks before base58 decoding
cf70ca7 
Summary: Backport of core [[bitcoin/bitcoin#17511 | PR17511]].

Test Plan:
  ninja all check-all

Reviewers: #bitcoin_abc, deadalnix

Reviewed By: #bitcoin_abc, deadalnix

Differential Revision: https://reviews.bitcoinabc.org/D8153
sidhujag pushed a commit to syscoin-core/syscoin that referenced this pull request Nov 10, 2020
@laanwj
Merge bitcoin#17511: Add bounds checks before base58 decoding
5a7c40f 
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 bitcoin#17501.

ACKs for top commit:
  laanwj:
    code review ACK 5909bcd
  practicalswift:
    ACK 5909bcd -- code looks correct

Tree-SHA512: 4807f4a9508dee9c0f1ad63f56f70f4ec4e6b7e35eb91322a525e3da3828521a41de9b8338a6bf67250803660b480d95fd02ce6b2fe79c4c88bc19b54f9d8889
@maflcko maflcko mentioned this pull request May 12, 2021
Merged
@furszy furszy mentioned this pull request Jun 7, 2021
Merged
@furszy furszy mentioned this pull request Jun 9, 2021
Merged
furszy added a commit to PIVX-Project/PIVX that referenced this pull request Jul 1, 2021
@furszy
Merge #2414: Finish address encoding cleanup
170beab 
a470c34 Backport attributes.h and connect it to base58.h functions only (practicalswift)
0247f6f util: Don't allow base58-decoding of std::string:s containing non-base58 characters (practicalswift)
70c480c tests: Add tests for base58-decoding of std::string:s containing non-base58 characters (practicalswift)
9d481be Add bounds checks in key_io before DecodeBase58Check (Pieter Wuille)
eac71b5 Finish Encode/Decode destination functions move from base58 to key_io. (furszy)
2e9376c Pass a maximum output length to DecodeBase58 and DecodeBase58Check (Pieter Wuille)
c93e19f Clean duplicate usage of DecodeSecret & EncodeSecret. (furszy)
4d4160e Stop using CBase58Data for ext keys (furszy)
e861cda Backport string ToUpper and ToLower. (furszy)
f6c2872 util: Add Join helper to join a list of strings (MarcoFalke)
32c1e42 Add tests for util/vector.h's Cat and Vector (Pieter Wuille)
dc42563 Add some general std::vector utility functions (Pieter Wuille)

Pull request description:

  Decoupled from #2411 Tor's v3 addr support, built on top of #2359.

  This PR finishes the address encoding cleanup, removing the `CBase58`, `CBitcoinSecret`, `CBitcoinExtKey`, and `CBitcoinExtPubKey` classes, in favor of using the KeyIO::Encode/Decode functions. Furthermore, all PIVX-specific address logic is moved to key_io.{h,cpp}, leaving base58.{h,cpp} as a pure utility that implements the base58 encoding/decoding logic.
  Plus, includes some general utility functions for std::vector and std::string.

  Adaptation of the following PRs:

  *  bitcoin#11372.
  * bitcoin#16670. (without faebf62)
  *  bitcoin#16889.
  *  bitcoin#17511.
  *  bitcoin#17721.

ACKs for top commit:
  random-zebra:
    rebase utACK a470c34
  Fuzzbawls:
    ACK a470c34

Tree-SHA512: 7a3e1ea0f86c7dab960a5761a666dc7eb291d749e1e9cc24583eec2d6114ca47bc6b9ad50c1c7ff2ecba7f3f60100ce7c0ee8522dc3a2f29d6d79cb052187e0d
random-zebra added a commit to PIVX-Project/PIVX that referenced this pull request Aug 11, 2021
@random-zebra
Merge #2411: Road to Tor v3 support (BIP155)
b4751e1 
ecde04a [Consensus] Bump Active Protocol version to 70923 for v5.3 (random-zebra)
b63e4f5 Consensus: Add v5.3 enforcement height for testnet. (furszy)
f44be94 Only relay IPv4, IPv6, Tor addresses (Pieter Wuille)
015298c fix: tor: Call event_base_loopbreak from the event's callback (furszy)
34ff7a8 Consensus: Add mnb ADDRv2 guard. (furszy)
b4515dc GUI: Present v3 onion addresses properly in MNs list. (furszy)
337d43d tests: don't export in6addr_loopback (Vasil Dimov)
2cde8e0 GUI: Do not show the tor v3 onion address in the topbar. (furszy)
0b5f406 Doc: update tor.md with latest upstream information. (furszy)
89df7f2 addrman: ensure old versions don't parse peers.dat (Vasil Dimov)
bb90c5c test: add getnetworkinfo network name regression tests (Jon Atack)
d8e01b5 rpc: update GetNetworksInfo() to not return unsupported networks (Jon Atack)
57fc7b0 net: update GetNetworkName() with all enum Network cases (Jon Atack)
647d60b tests: Modify rpc_bind to conform to bitcoin#14532 behaviour. (Carl Dong)
d4d6729 Allow running rpc_bind.py --nonloopback test without IPv6 (Kristaps Kaupe)
4a034d8 test: Add rpc_bind test to default-run tests (Wladimir J. van der Laan)
61a08af [tests] bind functional test nodes to 127.0.0.1  Prevents OSX firewall (Sjors Provoost)
6a4f1e0 test: Add basic addr relay test (furszy)
78aa61c net: Make addr relay mockable (furszy)
ba954ca Send and require SENDADDRV2 before VERACK (Pieter Wuille)
61c2ed4 Bump net protocol version + don't send 'sendaddrv2' to pre-70923 software (furszy)
ccd508a tor: make a TORv3 hidden service instead of TORv2 (Vasil Dimov)
6da9a14 net: advertise support for ADDRv2 via new message (furszy)
e58d5d0 Migrate to test_large_inv() to Misbehaving logging. (furszy)
d496b64 [QA] fix mininode CAddress ser/deser (Jonas Schnelli)
cec9567 net: CAddress & CAddrMan: (un)serialize as ADDRv2 Change the serialization of `CAddrMan` to serialize its addresses in ADDRv2/BIP155 format by default. Introduce a new `CAddrMan` format version (3). (furszy)
b8c1dda streams update: get rid of nType and nVersion. (furszy)
3eaa273 Support bypassing range check in ReadCompactSize (Pieter Wuille)
a237ba4 net: recognize TORv3/I2P/CJDNS networks (Vasil Dimov)
8e50853 util: make EncodeBase32 consume Spans (Sebastian Falbesoner)
1f67e30 net: CNetAddr: add support to (un)serialize as ADDRv2 (Vasil Dimov)
2455420 test: move HasReason so it can be reused (furszy)
d41adb4 util: move HasPrefix() so it can be reused (Vasil Dimov)
f6f86af Unroll Keccak-f implementation (Pieter Wuille)
45222e6 Implement keccak-f[1600] and SHA3-256 (Pieter Wuille)
08ad06d net: change CNetAddr::ip to have flexible size (furszy)
3337219 net: improve encapsulation of CNetAddr. (furszy)
910d5c4 test: Do not instantiate CAddrDB for static call (Hennadii Stepanov)
6b607ef Drop IsLimited in favor of IsReachable (Ben Woosley)
a40711b IsReachable is the inverse of IsLimited (DRY). Includes unit tests (marcaiaf)
8839828 net: don't accept non-left-contiguous netmasks (Vasil Dimov)
5d7f864 rpcbind: Warn about exposing RPC to untrusted networks (Luke Dashjr)
2a6abd8 CNetAddr: Add IsBindAny method to check for INADDR_ANY (Luke Dashjr)
4fdfa45 net: Always default rpcbind to localhost, never "all interfaces" (Luke Dashjr)
31064a8 net: Minor accumulated cleanups (furszy)
9f9c871 tests: Avoid using C-style NUL-terminated strings as arguments (practicalswift)
f6c52a3 tests: Add tests to make sure lookup methods fail on std::string parameters with embedded NUL characters (practicalswift)
a751b9b net: Avoid using C-style NUL-terminated strings as arguments in the netbase interface (furszy)
f30869d test: add IsRFC2544 tests (Mark Tyneway)
ed5abe1 Net: Proper CService deserialization + GetIn6Addr return false if addr isn't an IPv6 addr (furszy)
86d73fb net: save the network type explicitly in CNetAddr (Vasil Dimov)
ad57dfc net: document `enum Network` (Vasil Dimov)
cb160de netaddress: Update CNetAddr for ORCHIDv2 (Carl Dong)
c3c04e4 net: Better misbehaving logging (furszy)
3660487 net: Use C++11 member initialization in protocol (Marco)
082baa3 refactor: Drop unused CBufferedFile::Seek() (Hennadii Stepanov)
e2d776a util: CBufferedFile fixes (Larry Ruane)
6921f42 streams: backport OverrideStream class (furszy)

Pull request description:

  Conjunction of a large number of back ports, updates and refactorings that made with the final goal of implementing v3 Onion addresses support (BIP155 https://github.com/bitcoin/bips/blob/master/bip-0155.mediawiki) before the tor v2 addresses EOL, scheduled, by the Tor project, for (1) July 15th: v2 addr support removal from the code base, and (2) October 15th: v2 addr network disable, where **every peer in our network running under Tor will loose the connection and drop the network**.

  As BIP155 describes, this is introducing a new P2P message to gossip longer node addresses over the P2P network. This is required to support new-generation Onion addresses, I2P, and potentially other networks that have longer endpoint addresses than fit in the 128 bits of the current addr message.

  In order to achieve the end goal, had to:
  1.  Create Span class and push it up to latest Bitcoin implementation.
  2.  Update the whole serialization framework and every object using it up to latest Bitcoin implementation (3-4 years ahead of what we currently are in master).
  3.  Update the address manager implementing ASN-based bucketing of the network nodes.
  4.  Update and refactor the netAddress and address manager tests to latest Bitcoin implementation (4 years ahead of what we currently are in master).
  5.  Several util string, vector, encodings, parsing, hashing backports and more..

  Important note:
  This PR it is not meant to be merged as a standalone PR, will decouple smaller ones moving on. Adding on each sub-PR its own description isolated from this big monster.

  Second note:
  This is still a **work-in-progress**, not ready for testing yet. I'm probably missing to mention few PRs that have already adapted to our sources. Just making it public so can decouple the changes, we can start merging them and i can continue working a bit more confortable (rebase a +170 commits separate branch is not fun..).

  ### List of back ported and adapted PRs:

  Span and Serialization:
  ----------------
  *  bitcoin#12886.
  *  bitcoin#12916.
  *  bitcoin#13558.
  *  bitcoin#13697. (Only Span's commit 29943a9)
  *  bitcoin#17850.
  *  bitcoin#17896.
  *  bitcoin#12752.
  *  bitcoin#16577.
  *  bitcoin#16670. (without faebf62)
  *  bitcoin#17957.
  *  bitcoin#18021.
  *  bitcoin#18087.
  *  bitcoin#18112 (only from 353f376 that we don't support).
  *  bitcoin#18167.
  *  bitcoin#18317.
  *  bitcoin#18591 (only Span's commit 0fbde48)
  *  bitcoin#18468.
  *  bitcoin#19020.
  *  bitcoin#19032.
  *  bitcoin#19367.
  *  bitcoin#19387.

  Net, NetAddress and AddrMan:
  ----------------

  *  bitcoin#7932.
  *  bitcoin#10756.
  *  bitcoin#10765.
  *  bitcoin#12218.
  *  bitcoin#12855.
  *  bitcoin#13532.
  *  bitcoin#13575.
  *  bitcoin#13815.
  *  bitcoin#14532.
  *  bitcoin#15051.
  *  bitcoin#15138.
  *  bitcoin#15689.
  *  bitcoin#16702.
  *  bitcoin#17243.
  *  bitcoin#17345.
  *  bitcoin#17754.
  *  bitcoin#17758.
  *  bitcoin#17812.
  *  bitcoin#18023.
  *  bitcoin#18454.
  *  bitcoin#18512.
  *  bitcoin#19314.
  *  bitcoin#19687

  Keys and Addresses encoding:
  ----------------
  * bitcoin#11372.
  * bitcoin#17511.
  * bitcoin#17721.

  Util:
  ----------------
  * bitcoin#9140.
  * bitcoin#16577.
  * bitcoin#16889.
  * bitcoin#19593.

  Bench:
  ----------------
  * bitcoin#16299.

  BIP155:
  ----------------
  *  bitcoin#19351.
  *  bitcoin#19360.
  *  bitcoin#19534.
  *  bitcoin#19628.
  *  bitcoin#19841.
  *  bitcoin#19845.
  *  bitcoin#19954.
  *  bitcoin#19991 (pending).
  *  bitcoin#19845.
  *  bitcoin#20000 (pending).
  *  bitcoin#20120.
  *  bitcoin#20284.
  *  bitcoin#20564.
  *  bitcoin#21157 (pending).
  *  bitcoin#21564 (pending).
  *  Fully removed v2 onion addr support.
  *  Add hardcoded seeds.
  *  Add release-notes, changes to files.md and every needed documentation.

  I'm currently working on the PRs marked as "pending", this isn't over, but I'm pretty pretty close :). What a long road..

ACKs for top commit:
  random-zebra:
    utACK ecde04a
  Fuzzbawls:
    ACK ecde04a

Tree-SHA512: 82c95fbda76fce63f96d8a9af7fa9a89cb1e1b302b7891e27118a6103af0be23606bf202c7332fa61908205e6b6351764e2ec23d753f1e2484028f57c2e8b51a
@bitcoin bitcoin locked as resolved and limited conversation to collaborators Dec 16, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@laanwj laanwj laanwj left review comments

+1 more reviewer

@maflcko maflcko maflcko left review comments

Reviewers whose approvals may not affect merge requirements
Assignees
No one assigned
Labels
Tests Utils/log/libs
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Base58 decoding is done without checking that the input size is reasonable
6 participants
@sipa @practicalswift @laanwj @Sjors @DrahtBot @maflcko