Merge bitcoin#25642: Don't wrap around when deriving an extended key 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

Author: achow101

src/key.cpp

@@ -341,6 +341,7 @@ ECDHSecret CKey::ComputeBIP324ECDHSecret(const EllSwiftPubKey& their_ellswift, c
 }
 
 bool CExtKey::Derive(CExtKey &out, unsigned int _nChild) const {
+    if (nDepth == std::numeric_limits<unsigned char>::max()) return false;
     out.nDepth = nDepth + 1;
     CKeyID id = key.GetPubKey().GetID();
     memcpy(out.vchFingerprint, &id, 4);

src/key.h

@@ -135,7 +135,7 @@ class CKey
     bool SignCompact(const uint256& hash, std::vector<unsigned char>& vchSig) const;
 
     //! Derive BIP32 child key.
-    bool Derive(CKey& keyChild, ChainCode &ccChild, unsigned int nChild, const ChainCode& cc) const;
+    [[nodiscard]] bool Derive(CKey& keyChild, ChainCode &ccChild, unsigned int nChild, const ChainCode& cc) const;
 
     /**
      * Verify thoroughly whether a private key and a public key match.
@@ -186,7 +186,7 @@ struct CExtKey {
 
     void Encode(unsigned char code[BIP32_EXTKEY_SIZE]) const;
     void Decode(const unsigned char code[BIP32_EXTKEY_SIZE]);
-    bool Derive(CExtKey& out, unsigned int nChild) const;
+    [[nodiscard]] bool Derive(CExtKey& out, unsigned int nChild) const;
     CExtPubKey Neuter() const;
     void SetSeed(Span<const std::byte> seed);
 };

src/pubkey.cpp

@@ -309,6 +309,7 @@ void CExtPubKey::DecodeWithVersion(const unsigned char code[BIP32_EXTKEY_WITH_VE
 }
 
 bool CExtPubKey::Derive(CExtPubKey &out, unsigned int _nChild) const {
+    if (nDepth == std::numeric_limits<unsigned char>::max()) return false;
     out.nDepth = nDepth + 1;
     CKeyID id = pubkey.GetID();
     memcpy(out.vchFingerprint, &id, 4);

src/pubkey.h

@@ -208,7 +208,7 @@ class CPubKey
     bool Decompress();
 
     //! Derive BIP32 child pubkey.
-    bool Derive(CPubKey& pubkeyChild, ChainCode &ccChild, unsigned int nChild, const ChainCode& cc) const;
+    [[nodiscard]] bool Derive(CPubKey& pubkeyChild, ChainCode &ccChild, unsigned int nChild, const ChainCode& cc) const;
 };
 
 /** An ElligatorSwift-encoded public key. */
@@ -281,7 +281,7 @@ struct CExtPubKey {
     void Decode(const unsigned char code[BIP32_EXTKEY_SIZE]);
     void EncodeWithVersion(unsigned char code[BIP32_EXTKEY_WITH_VERSION_SIZE]) const;
     void DecodeWithVersion(const unsigned char code[BIP32_EXTKEY_WITH_VERSION_SIZE]);
-    bool Derive(CExtPubKey& out, unsigned int nChild) const;
+    [[nodiscard]] bool Derive(CExtPubKey& out, unsigned int nChild) const;
 
     void Serialize(CSizeComputer& s) const
     {

src/script/descriptor.cpp

@@ -304,7 +304,7 @@ class BIP32PubkeyProvider final : public PubkeyProvider
     {
         if (!GetExtKey(arg, xprv)) return false;
         for (auto entry : m_path) {
-            xprv.Derive(xprv, entry);
+            if (!xprv.Derive(xprv, entry)) return false;
             if (entry >> 31) {
                 last_hardened = xprv;
             }
@@ -364,14 +364,13 @@ class BIP32PubkeyProvider final : public PubkeyProvider
             }
         } else {
             for (auto entry : m_path) {
-                der = parent_extkey.Derive(parent_extkey, entry);
-                assert(der);
+                if (!parent_extkey.Derive(parent_extkey, entry)) return false;
             }
             final_extkey = parent_extkey;
             if (m_derive == DeriveType::UNHARDENED) der = parent_extkey.Derive(final_extkey, pos);
             assert(m_derive != DeriveType::HARDENED);
         }
-        assert(der);
+        if (!der) return false;
 
         final_info_out = final_info_out_tmp;
         key_out = final_extkey.pubkey;
@@ -474,8 +473,8 @@ class BIP32PubkeyProvider final : public PubkeyProvider
         CExtKey extkey;
         CExtKey dummy;
         if (!GetDerivedExtKey(arg, extkey, dummy)) return false;
-        if (m_derive == DeriveType::UNHARDENED) extkey.Derive(extkey, pos);
-        if (m_derive == DeriveType::HARDENED) extkey.Derive(extkey, pos | 0x80000000UL);
+        if (m_derive == DeriveType::UNHARDENED && !extkey.Derive(extkey, pos)) return false;
+        if (m_derive == DeriveType::HARDENED && !extkey.Derive(extkey, pos | 0x80000000UL)) return false;
         key = extkey.key;
         return true;
     }

src/test/bip32_tests.cpp

@@ -184,4 +184,22 @@ BOOST_AUTO_TEST_CASE(bip32_test5) {
     }
 }
 
+BOOST_AUTO_TEST_CASE(bip32_max_depth) {
+    CExtKey key_parent{DecodeExtKey(test1.vDerive[0].prv)}, key_child;
+    CExtPubKey pubkey_parent{DecodeExtPubKey(test1.vDerive[0].pub)}, pubkey_child;
+
+    // We can derive up to the 255th depth..
+    for (auto i = 0; i++ < 255;) {
+        BOOST_CHECK(key_parent.Derive(key_child, 0));
+        std::swap(key_parent, key_child);
+        BOOST_CHECK(pubkey_parent.Derive(pubkey_child, 0));
+        std::swap(pubkey_parent, pubkey_child);
+    }
+
+    // But trying to derive a non-existent 256th depth will fail!
+    BOOST_CHECK(key_parent.nDepth == 255 && pubkey_parent.nDepth == 255);
+    BOOST_CHECK(!key_parent.Derive(key_child, 0));
+    BOOST_CHECK(!pubkey_parent.Derive(pubkey_child, 0));
+}
+
 BOOST_AUTO_TEST_SUITE_END()

src/test/descriptor_tests.cpp

@@ -180,7 +180,7 @@ void DoCheck(const std::string& prv, const std::string& pub, const std::string&
                 for (const auto& xpub_pair : parent_xpub_cache) {
                     const CExtPubKey& xpub = xpub_pair.second;
                     CExtPubKey der;
-                    xpub.Derive(der, i);
+                    BOOST_CHECK(xpub.Derive(der, i));
                     pubkeys.insert(der.pubkey);
                 }
                 for (const auto& origin_pair : script_provider_cached.origins) {
@@ -203,7 +203,7 @@ void DoCheck(const std::string& prv, const std::string& pub, const std::string&
                     const CExtPubKey& xpub = xpub_pair.second;
                     pubkeys.insert(xpub.pubkey);
                     CExtPubKey der;
-                    xpub.Derive(der, i);
+                    BOOST_CHECK(xpub.Derive(der, i));
                     pubkeys.insert(der.pubkey);
                 }
                 for (const auto& origin_pair : script_provider_cached.origins) {

src/wallet/hdchain.cpp

@@ -129,6 +129,14 @@ uint256 CHDChain::GetSeedHash()
     return Hash(vchSeed);
 }
 
+//! Try to derive an extended key, throw if it fails.
+static void DeriveExtKey(CExtKey& key_in, unsigned int index, CExtKey& key_out)
+{
+    if (!key_in.Derive(key_out, index)) {
+        throw std::runtime_error("Could not derive extended key");
+    }
+}
+
 void CHDChain::DeriveChildExtKey(uint32_t nAccountIndex, bool fInternal, uint32_t nChildIndex, CExtKey& extKeyRet, KeyOriginInfo& key_origin)
 {
     LOCK(cs);
@@ -146,15 +154,15 @@ void CHDChain::DeriveChildExtKey(uint32_t nAccountIndex, bool fInternal, uint32_
     // (keys >= 0x80000000 are hardened after bip32)
 
     // derive m/purpose'
-    masterKey.Derive(purposeKey, 44 | 0x80000000);
+    DeriveExtKey(masterKey, 44 | 0x80000000, purposeKey);
     // derive m/purpose'/coin_type'
-    purposeKey.Derive(cointypeKey, Params().ExtCoinType() | 0x80000000);
+    DeriveExtKey(purposeKey, Params().ExtCoinType() | 0x80000000, cointypeKey);
     // derive m/purpose'/coin_type'/account'
-    cointypeKey.Derive(accountKey, nAccountIndex | 0x80000000);
+    DeriveExtKey(cointypeKey, nAccountIndex | 0x80000000, accountKey);
     // derive m/purpose'/coin_type'/account'/change
-    accountKey.Derive(changeKey, fInternal ? 1 : 0);
+    DeriveExtKey(accountKey, fInternal ? 1 : 0, changeKey);
     // derive m/purpose'/coin_type'/account'/change/address_index
-    changeKey.Derive(extKeyRet, nChildIndex);
+    DeriveExtKey(changeKey, nChildIndex, extKeyRet);
 
 #ifdef ENABLE_WALLET
     // We should never ever update an already existing key_origin here