Don't wrap around when deriving an extended key at a too large depth #25642
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
Is the wrap actually harmful? In any case, current code will never do this, right? |
|
It is harmful in the sense that this behaviour is not specified by BIP32. We could accept an invalid descriptor or xpub that another software would refuse (or worse, treat differently).
Well, it was triggered by current code. |
|
ACK 84f86dc |
In some cases we asserted it succeeded, in others we were just ignoring it
It might already fail, and we'll add another failure case.
This issue was reported to me by Marco Falke, and found with the descriptor_parse fuzz target.
84f86dc to
fb9faff
Compare
|
re-ACK fb9faff |
|
Hadn't occurred to me this implicit limit due to the encoding of xpub/xprv. utACK fb9faff |
sidhujag
pushed a commit
to syscoin/syscoin
that referenced
this pull request
Aug 11, 2022
…at a too large depth fb9faff extended keys: fail to derive too large depth instead of wrapping around (Antoine Poinsot) 8dc6670 descriptor: don't assert success of extended key derivation (Antoine Poinsot) 50cfc9e (pubk)key: mark Derive() as nodiscard (Antoine Poinsot) 0ca258a descriptor: never ignore the return value when deriving an extended key (Antoine Poinsot) d3599c2 spkman: don't ignore the return value when deriving an extended key (Antoine Poinsot) Pull request description: We would previously silently wrap the derived child's depth back to `0`. Instead, explicitly fail when trying to derive an impossible depth, and handle the error in callers. An extended fuzzing corpus of `descriptor_parse` triggered this behaviour, which was reported by MarcoFalke. Fixes bitcoin#25751. ACKs for top commit: achow101: re-ACK fb9faff instagibbs: utACK bitcoin@fb9faff Tree-SHA512: 9f75c23572ce847239bd15e5497df2960b6bd63c61ea72347959d968b5c4c9a4bfeee284e76bdcd7bacbf9eeb70feee85ffd3e316f353ca6eca30e93aafad343
This was referenced Aug 17, 2022
Closed
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
We would previously silently wrap the derived child's depth back to
0. Instead, explicitly fail when trying to derive an impossible depth, and handle the error in callers.An extended fuzzing corpus of
descriptor_parsetriggered this behaviour, which was reported by MarcoFalke.Fixes #25751.