Skip to content

Commit 93999a5

Browse files
achow101achow101
committed
Merge bitcoin/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 #25751. ACKs for top commit: achow101: re-ACK fb9faff instagibbs: utACK bitcoin/bitcoin@fb9faff Tree-SHA512: 9f75c23572ce847239bd15e5497df2960b6bd63c61ea72347959d968b5c4c9a4bfeee284e76bdcd7bacbf9eeb70feee85ffd3e316f353ca6eca30e93aafad343
2 parents 251c535 + fb9faff commit 93999a5

File tree

8 files changed

+42
-16
lines changed

8 files changed

+42
-16
lines changed

‎src/key.cpp

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -333,6 +333,7 @@ bool CKey::Derive(CKey& keyChild, ChainCode &ccChild, unsigned int nChild, const
333333
}
334334

335335
bool CExtKey::Derive(CExtKey &out, unsigned int _nChild) const {
336+
if (nDepth == std::numeric_limits<unsigned char>::max()) return false;
336337
out.nDepth = nDepth + 1;
337338
CKeyID id = key.GetPubKey().GetID();
338339
memcpy(out.vchFingerprint, &id, 4);

‎src/key.h

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -146,7 +146,7 @@ class CKey
146146
bool SignSchnorr(const uint256& hash, Span<unsigned char> sig, const uint256* merkle_root, const uint256& aux) const;
147147

148148
//! Derive BIP32 child key.
149-
bool Derive(CKey& keyChild, ChainCode &ccChild, unsigned int nChild, const ChainCode& cc) const;
149+
[[nodiscard]] bool Derive(CKey& keyChild, ChainCode &ccChild, unsigned int nChild, const ChainCode& cc) const;
150150

151151
/**
152152
* Verify thoroughly whether a private key and a public key match.
@@ -176,7 +176,7 @@ struct CExtKey {
176176

177177
void Encode(unsigned char code[BIP32_EXTKEY_SIZE]) const;
178178
void Decode(const unsigned char code[BIP32_EXTKEY_SIZE]);
179-
bool Derive(CExtKey& out, unsigned int nChild) const;
179+
[[nodiscard]] bool Derive(CExtKey& out, unsigned int nChild) const;
180180
CExtPubKey Neuter() const;
181181
void SetSeed(Span<const std::byte> seed);
182182
};

‎src/pubkey.cpp

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -365,6 +365,7 @@ void CExtPubKey::DecodeWithVersion(const unsigned char code[BIP32_EXTKEY_WITH_VE
365365
}
366366

367367
bool CExtPubKey::Derive(CExtPubKey &out, unsigned int _nChild) const {
368+
if (nDepth == std::numeric_limits<unsigned char>::max()) return false;
368369
out.nDepth = nDepth + 1;
369370
CKeyID id = pubkey.GetID();
370371
memcpy(out.vchFingerprint, &id, 4);

‎src/pubkey.h

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -218,7 +218,7 @@ class CPubKey
218218
bool Decompress();
219219

220220
//! Derive BIP32 child pubkey.
221-
bool Derive(CPubKey& pubkeyChild, ChainCode &ccChild, unsigned int nChild, const ChainCode& cc) const;
221+
[[nodiscard]] bool Derive(CPubKey& pubkeyChild, ChainCode &ccChild, unsigned int nChild, const ChainCode& cc) const;
222222
};
223223

224224
class XOnlyPubKey
@@ -327,7 +327,7 @@ struct CExtPubKey {
327327
void Decode(const unsigned char code[BIP32_EXTKEY_SIZE]);
328328
void EncodeWithVersion(unsigned char code[BIP32_EXTKEY_WITH_VERSION_SIZE]) const;
329329
void DecodeWithVersion(const unsigned char code[BIP32_EXTKEY_WITH_VERSION_SIZE]);
330-
bool Derive(CExtPubKey& out, unsigned int nChild) const;
330+
[[nodiscard]] bool Derive(CExtPubKey& out, unsigned int nChild) const;
331331
};
332332

333333
/** Users of this module must hold an ECCVerifyHandle. The constructor and

‎src/script/descriptor.cpp

Lines changed: 5 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -328,7 +328,7 @@ class BIP32PubkeyProvider final : public PubkeyProvider
328328
{
329329
if (!GetExtKey(arg, xprv)) return false;
330330
for (auto entry : m_path) {
331-
xprv.Derive(xprv, entry);
331+
if (!xprv.Derive(xprv, entry)) return false;
332332
if (entry >> 31) {
333333
last_hardened = xprv;
334334
}
@@ -388,14 +388,13 @@ class BIP32PubkeyProvider final : public PubkeyProvider
388388
}
389389
} else {
390390
for (auto entry : m_path) {
391-
der = parent_extkey.Derive(parent_extkey, entry);
392-
assert(der);
391+
if (!parent_extkey.Derive(parent_extkey, entry)) return false;
393392
}
394393
final_extkey = parent_extkey;
395394
if (m_derive == DeriveType::UNHARDENED) der = parent_extkey.Derive(final_extkey, pos);
396395
assert(m_derive != DeriveType::HARDENED);
397396
}
398-
assert(der);
397+
if (!der) return false;
399398

400399
final_info_out = final_info_out_tmp;
401400
key_out = final_extkey.pubkey;
@@ -498,8 +497,8 @@ class BIP32PubkeyProvider final : public PubkeyProvider
498497
CExtKey extkey;
499498
CExtKey dummy;
500499
if (!GetDerivedExtKey(arg, extkey, dummy)) return false;
501-
if (m_derive == DeriveType::UNHARDENED) extkey.Derive(extkey, pos);
502-
if (m_derive == DeriveType::HARDENED) extkey.Derive(extkey, pos | 0x80000000UL);
500+
if (m_derive == DeriveType::UNHARDENED && !extkey.Derive(extkey, pos)) return false;
501+
if (m_derive == DeriveType::HARDENED && !extkey.Derive(extkey, pos | 0x80000000UL)) return false;
503502
key = extkey.key;
504503
return true;
505504
}

‎src/test/bip32_tests.cpp

Lines changed: 18 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -184,4 +184,22 @@ BOOST_AUTO_TEST_CASE(bip32_test5) {
184184
}
185185
}
186186

187+
BOOST_AUTO_TEST_CASE(bip32_max_depth) {
188+
CExtKey key_parent{DecodeExtKey(test1.vDerive[0].prv)}, key_child;
189+
CExtPubKey pubkey_parent{DecodeExtPubKey(test1.vDerive[0].pub)}, pubkey_child;
190+
191+
// We can derive up to the 255th depth..
192+
for (auto i = 0; i++ < 255;) {
193+
BOOST_CHECK(key_parent.Derive(key_child, 0));
194+
std::swap(key_parent, key_child);
195+
BOOST_CHECK(pubkey_parent.Derive(pubkey_child, 0));
196+
std::swap(pubkey_parent, pubkey_child);
197+
}
198+
199+
// But trying to derive a non-existent 256th depth will fail!
200+
BOOST_CHECK(key_parent.nDepth == 255 && pubkey_parent.nDepth == 255);
201+
BOOST_CHECK(!key_parent.Derive(key_child, 0));
202+
BOOST_CHECK(!pubkey_parent.Derive(pubkey_child, 0));
203+
}
204+
187205
BOOST_AUTO_TEST_SUITE_END()

‎src/test/descriptor_tests.cpp

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -233,7 +233,7 @@ void DoCheck(const std::string& prv, const std::string& pub, const std::string&
233233
for (const auto& xpub_pair : parent_xpub_cache) {
234234
const CExtPubKey& xpub = xpub_pair.second;
235235
CExtPubKey der;
236-
xpub.Derive(der, i);
236+
BOOST_CHECK(xpub.Derive(der, i));
237237
pubkeys.insert(der.pubkey);
238238
}
239239
int count_pks = 0;
@@ -265,7 +265,7 @@ void DoCheck(const std::string& prv, const std::string& pub, const std::string&
265265
const CExtPubKey& xpub = xpub_pair.second;
266266
pubkeys.insert(xpub.pubkey);
267267
CExtPubKey der;
268-
xpub.Derive(der, i);
268+
BOOST_CHECK(xpub.Derive(der, i));
269269
pubkeys.insert(der.pubkey);
270270
}
271271
int count_pks = 0;

‎src/wallet/scriptpubkeyman.cpp

Lines changed: 11 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -1080,6 +1080,13 @@ CPubKey LegacyScriptPubKeyMan::GenerateNewKey(WalletBatch &batch, CHDChain& hd_c
10801080
return pubkey;
10811081
}
10821082

1083+
//! Try to derive an extended key, throw if it fails.
1084+
static void DeriveExtKey(CExtKey& key_in, unsigned int index, CExtKey& key_out) {
1085+
if (!key_in.Derive(key_out, index)) {
1086+
throw std::runtime_error("Could not derive extended key");
1087+
}
1088+
}
1089+
10831090
void LegacyScriptPubKeyMan::DeriveNewChildKey(WalletBatch &batch, CKeyMetadata& metadata, CKey& secret, CHDChain& hd_chain, bool internal)
10841091
{
10851092
// for now we use a fixed keypath scheme of m/0'/0'/k
@@ -1097,27 +1104,27 @@ void LegacyScriptPubKeyMan::DeriveNewChildKey(WalletBatch &batch, CKeyMetadata&
10971104

10981105
// derive m/0'
10991106
// use hardened derivation (child keys >= 0x80000000 are hardened after bip32)
1100-
masterKey.Derive(accountKey, BIP32_HARDENED_KEY_LIMIT);
1107+
DeriveExtKey(masterKey, BIP32_HARDENED_KEY_LIMIT, accountKey);
11011108

11021109
// derive m/0'/0' (external chain) OR m/0'/1' (internal chain)
11031110
assert(internal ? m_storage.CanSupportFeature(FEATURE_HD_SPLIT) : true);
1104-
accountKey.Derive(chainChildKey, BIP32_HARDENED_KEY_LIMIT+(internal ? 1 : 0));
1111+
DeriveExtKey(accountKey, BIP32_HARDENED_KEY_LIMIT+(internal ? 1 : 0), chainChildKey);
11051112

11061113
// derive child key at next index, skip keys already known to the wallet
11071114
do {
11081115
// always derive hardened keys
11091116
// childIndex | BIP32_HARDENED_KEY_LIMIT = derive childIndex in hardened child-index-range
11101117
// example: 1 | BIP32_HARDENED_KEY_LIMIT == 0x80000001 == 2147483649
11111118
if (internal) {
1112-
chainChildKey.Derive(childKey, hd_chain.nInternalChainCounter | BIP32_HARDENED_KEY_LIMIT);
1119+
DeriveExtKey(chainChildKey, hd_chain.nInternalChainCounter | BIP32_HARDENED_KEY_LIMIT, childKey);
11131120
metadata.hdKeypath = "m/0'/1'/" + ToString(hd_chain.nInternalChainCounter) + "'";
11141121
metadata.key_origin.path.push_back(0 | BIP32_HARDENED_KEY_LIMIT);
11151122
metadata.key_origin.path.push_back(1 | BIP32_HARDENED_KEY_LIMIT);
11161123
metadata.key_origin.path.push_back(hd_chain.nInternalChainCounter | BIP32_HARDENED_KEY_LIMIT);
11171124
hd_chain.nInternalChainCounter++;
11181125
}
11191126
else {
1120-
chainChildKey.Derive(childKey, hd_chain.nExternalChainCounter | BIP32_HARDENED_KEY_LIMIT);
1127+
DeriveExtKey(chainChildKey, hd_chain.nExternalChainCounter | BIP32_HARDENED_KEY_LIMIT, childKey);
11211128
metadata.hdKeypath = "m/0'/0'/" + ToString(hd_chain.nExternalChainCounter) + "'";
11221129
metadata.key_origin.path.push_back(0 | BIP32_HARDENED_KEY_LIMIT);
11231130
metadata.key_origin.path.push_back(0 | BIP32_HARDENED_KEY_LIMIT);

0 commit comments

Comments
 (0)