Don’t wrap around when deriving an extended key at a too large depth #25642

1. 
   darosior commented at 10:26 am on July 19, 2022: member

   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 #25751.

2. spkman: don't ignore the return value when deriving an extended key d3599c22bd
3. MarcoFalke added this to the milestone 24.0 on Jul 19, 2022
4. 
   luke-jr commented at 2:23 am on July 27, 2022: member

   Is the wrap actually harmful?

   In any case, current code will never do this, right?

5. 
   darosior commented at 10:15 am on July 27, 2022: member

   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).

   In any case, current code will never do this, right?

   Well, it was triggered by current code.

6. 
   achow101 commented at 8:16 pm on August 2, 2022: member

   ACK 84f86dcc1d9ff6f3161c9567068216d7b27d3b39
7. in src/key.cpp:335 in 84f86dcc1d outdated

   332@@ -333,6 +333,7 @@ bool CKey::Derive(CKey& keyChild, ChainCode &ccChild, unsigned int nChild, const
   333 }
   334 
   335 bool CExtKey::Derive(CExtKey &out, unsigned int _nChild) const {
   

   ---

   ---

   

   MarcoFalke commented at 5:49 am on August 3, 2022:

   Maybe mark as [[nodiscard]] in the header?

   ---

   

   darosior commented at 9:36 am on August 4, 2022:

   Done, and fixed two missing occurrences (in the descriptor tests) this revealed. Thanks!
8. 
   MarcoFalke commented at 5:49 am on August 3, 2022: member

   left a nit
9. descriptor: never ignore the return value when deriving an extended key

   In some cases we asserted it succeeded, in others we were just ignoring it

   0ca258a5ac
10. (pubk)key: mark Derive() as nodiscard 50cfc9e761
11. descriptor: don't assert success of extended key derivation

    It might already fail, and we'll add another failure case.

    8dc6670ce1
12. extended keys: fail to derive too large depth instead of wrapping around

    This issue was reported to me by Marco Falke, and found with the
    descriptor_parse fuzz target.

    fb9faffae3
13. darosior force-pushed on Aug 4, 2022
14. fanquake requested review from achow101 on Aug 6, 2022
15. 
    achow101 commented at 8:42 pm on August 9, 2022: member

    re-ACK fb9faffae3a26b8aed8b671864ba679747163019
16. fanquake requested review from instagibbs on Aug 9, 2022
17. 
    instagibbs commented at 1:53 pm on August 10, 2022: member

    Hadn’t occurred to me this implicit limit due to the encoding of xpub/xprv.

    utACK https://github.com/bitcoin/bitcoin/pull/25642/commits/fb9faffae3a26b8aed8b671864ba679747163019

18. achow101 merged this on Aug 10, 2022
19. achow101 closed this on Aug 10, 2022

    ---

20. darosior deleted the branch on Aug 10, 2022
21. sidhujag referenced this in commit 0f6dfd11c3 on Aug 11, 2022
22. bitcoin locked this on Aug 10, 2023

Contributors
darosior luke-jr achow101 MarcoFalke instagibbs

Review Requested
achow101 instagibbs

Milestone
24.0