Repository: bitcoin/bitcoin (language: cpp)

Issue:
pubkey.cpp:368:18: runtime error: implicit conversion from type 'int' of value 256 (32-bit, signed) to type 'unsigned char' changed the value to 0 (8-bit, unsigned)
Steps to reproduce:

* Build with `integer` sanitizer
* Start: `UBSAN_OPTIONS="suppressions=$(pwd)/test/sanitizer_suppressions/ubsan:print_stacktrace=1:halt_on_error=1:report_error_type=1" ./src/qt/bitcoin-qt -chain=main`
* Enter `getdescriptorinfo "sh(multi(1,xprv9uPDJpEQgRQfDcW7BkF7eTya6RPxXeJCqCJGHuCJ4GiRVLzkTXBAJMu2qaMWPrS7AANYqdq6vcBcBUdJCVVFceUvJFjaPdGZ2y9WACViL4L/5/5/5/2/8/5/8/8/8/2/2/5/8/5/2/5/85/58/5/2/5/5/5/5/2/58/2/5/8/0000/00/8/5/2/001/1/246/8/501/46/8/5/8/5/000/8/52/5/8/5/2/8/2/5/8/5/246/8/5/2/8/2/5/8/5/2/5/8/8/2/5/5/5/58/88/2/6/8/5/2/8/2/5/8/2/588/5/2/5/8/5/2/8/5/1/5/4/5/5/5/2/6/8/5/2/8/2/5/8/5/2/5/58/58/2/5/8/58/588/2/52/5/5/5/58/8/2/6/8/5/2/8/2/5/8/2/588/5/2/5/8/5/2/8/5/15/58/58/2/6/8/5/2/8/2/5/8/5/2/5/8/58/2/5/58/58/588/2/5/2/8/5/8/5/4/5/58/588/2/6/8/5/2/8/2/5/8/2/8/5/2/8/2/5/8/5/2/5/58/58/2/5/5/5/58/588/2/6/8/5/2/8/2/5/8/2/8/588/2/6/8/5/2/8/2/5/5/8/5/2/0/8/5/2/00/4/5/5/588/2/6/8/5/2/8/25/5/8/2/5/58/58/8/4/1,[00000000]xprv9uPDJpEQgRQfDcW7BkF7eTya6RPxXeJCqCJGHuCJ4GiRVLzkTXBAJMu2qaMWPrS7AANYqdq6vcBcBUdJCVVFceUvJFjaPdGZ2y9WACViL4L/58)0)"` into the console


Discussion / hints:

Fix patch:
diff --git a/src/key.cpp b/src/key.cpp
index 9b0971a2dd205..199808505d1b9 100644
--- a/src/key.cpp
+++ b/src/key.cpp
@@ -333,6 +333,7 @@ bool CKey::Derive(CKey& keyChild, ChainCode &ccChild, unsigned int nChild, const
 }
 
 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);
diff --git a/src/key.h b/src/key.h
index 12d03778a0e9b..e9b78ebd44899 100644
--- a/src/key.h
+++ b/src/key.h
@@ -146,7 +146,7 @@ class CKey
     bool SignSchnorr(const uint256& hash, Span<unsigned char> sig, const uint256* merkle_root, const uint256& aux) 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.
@@ -176,7 +176,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);
 };
diff --git a/src/pubkey.cpp b/src/pubkey.cpp
index 324f681a0a307..0b94868d1fb2b 100644
--- a/src/pubkey.cpp
+++ b/src/pubkey.cpp
@@ -365,6 +365,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);
diff --git a/src/pubkey.h b/src/pubkey.h
index 463efe1b003c3..0485a38f7215d 100644
--- a/src/pubkey.h
+++ b/src/pubkey.h
@@ -218,7 +218,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;
 };
 
 class XOnlyPubKey
@@ -327,7 +327,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;
 };
 
 /** Users of this module must hold an ECCVerifyHandle. The constructor and
diff --git a/src/script/descriptor.cpp b/src/script/descriptor.cpp
index 34a4da74f8bec..9bdffad89b7d8 100644
--- a/src/script/descriptor.cpp
+++ b/src/script/descriptor.cpp
@@ -328,7 +328,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;
             }
@@ -388,14 +388,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;
@@ -498,8 +497,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;
     }
diff --git a/src/wallet/scriptpubkeyman.cpp b/src/wallet/scriptpubkeyman.cpp
index bba31dfe0ba15..3391d56b15f73 100644
--- a/src/wallet/scriptpubkeyman.cpp
+++ b/src/wallet/scriptpubkeyman.cpp
@@ -1084,6 +1084,13 @@ CPubKey LegacyScriptPubKeyMan::GenerateNewKey(WalletBatch &batch, CHDChain& hd_c
     return pubkey;
 }
 
+//! 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 LegacyScriptPubKeyMan::DeriveNewChildKey(WalletBatch &batch, CKeyMetadata& metadata, CKey& secret, CHDChain& hd_chain, bool internal)
 {
     // for now we use a fixed keypath scheme of m/0'/0'/k
@@ -1101,11 +1108,11 @@ void LegacyScriptPubKeyMan::DeriveNewChildKey(WalletBatch &batch, CKeyMetadata&
 
     // derive m/0'
     // use hardened derivation (child keys >= 0x80000000 are hardened after bip32)
-    masterKey.Derive(accountKey, BIP32_HARDENED_KEY_LIMIT);
+    DeriveExtKey(masterKey, BIP32_HARDENED_KEY_LIMIT, accountKey);
 
     // derive m/0'/0' (external chain) OR m/0'/1' (internal chain)
     assert(internal ? m_storage.CanSupportFeature(FEATURE_HD_SPLIT) : true);
-    accountKey.Derive(chainChildKey, BIP32_HARDENED_KEY_LIMIT+(internal ? 1 : 0));
+    DeriveExtKey(accountKey, BIP32_HARDENED_KEY_LIMIT+(internal ? 1 : 0), chainChildKey);
 
     // derive child key at next index, skip keys already known to the wallet
     do {
@@ -1113,7 +1120,7 @@ void LegacyScriptPubKeyMan::DeriveNewChildKey(WalletBatch &batch, CKeyMetadata&
         // childIndex | BIP32_HARDENED_KEY_LIMIT = derive childIndex in hardened child-index-range
         // example: 1 | BIP32_HARDENED_KEY_LIMIT == 0x80000001 == 2147483649
         if (internal) {
-            chainChildKey.Derive(childKey, hd_chain.nInternalChainCounter | BIP32_HARDENED_KEY_LIMIT);
+            DeriveExtKey(chainChildKey, hd_chain.nInternalChainCounter | BIP32_HARDENED_KEY_LIMIT, childKey);
             metadata.hdKeypath = "m/0'/1'/" + ToString(hd_chain.nInternalChainCounter) + "'";
             metadata.key_origin.path.push_back(0 | BIP32_HARDENED_KEY_LIMIT);
             metadata.key_origin.path.push_back(1 | BIP32_HARDENED_KEY_LIMIT);
@@ -1121,7 +1128,7 @@ void LegacyScriptPubKeyMan::DeriveNewChildKey(WalletBatch &batch, CKeyMetadata&
             hd_chain.nInternalChainCounter++;
         }
         else {
-            chainChildKey.Derive(childKey, hd_chain.nExternalChainCounter | BIP32_HARDENED_KEY_LIMIT);
+            DeriveExtKey(chainChildKey, hd_chain.nExternalChainCounter | BIP32_HARDENED_KEY_LIMIT, childKey);
             metadata.hdKeypath = "m/0'/0'/" + ToString(hd_chain.nExternalChainCounter) + "'";
             metadata.key_origin.path.push_back(0 | BIP32_HARDENED_KEY_LIMIT);
             metadata.key_origin.path.push_back(0 | BIP32_HARDENED_KEY_LIMIT);


Test patch:
diff --git a/src/test/bip32_tests.cpp b/src/test/bip32_tests.cpp
index 64cc9242392da..75b29ae0aaf86 100644
--- a/src/test/bip32_tests.cpp
+++ b/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()
diff --git a/src/test/descriptor_tests.cpp b/src/test/descriptor_tests.cpp
index a8c666079d3bd..8a17472cdac96 100644
--- a/src/test/descriptor_tests.cpp
+++ b/src/test/descriptor_tests.cpp
@@ -233,7 +233,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);
                 }
                 int count_pks = 0;
@@ -265,7 +265,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);
                 }
                 int count_pks = 0;