mirrors/bitcoin
1
0
Fork 0
mirror of https://github.com/bitcoin/bitcoin.git synced 2025-02-21 14:34:49 +01:00
Code Issues Projects Releases Wiki Activity

Merge bitcoin/bitcoin#25642: Don't wrap around when deriving an extended key at a too large depth

Browse source 
fb9faffae3 extended keys: fail to derive too large depth instead of wrapping around (Antoine Poinsot)
8dc6670ce1 descriptor: don't assert success of extended key derivation (Antoine Poinsot)
50cfc9e761 (pubk)key: mark Derive() as nodiscard (Antoine Poinsot)
0ca258a5ac descriptor: never ignore the return value when deriving an extended key (Antoine Poinsot)
d3599c22bd 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 fb9faffae3
  instagibbs:
    utACK  fb9faffae3

Tree-SHA512: 9f75c23572ce847239bd15e5497df2960b6bd63c61ea72347959d968b5c4c9a4bfeee284e76bdcd7bacbf9eeb70feee85ffd3e316f353ca6eca30e93aafad343
This commit is contained in:
Andrew Chow 2022-08-10 14:09:53 -04:00
commit 93999a5fbe
No known key found for this signature in database
GPG key ID: 17565732E08E5E41
8 changed files with 42 additions and 16 deletions
Show stats Download patch file Download diff file Expand all files Collapse all files

1
src/key.cpp
View file

@ -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);

4
src/key.h
View file

@ -146,7 +146,7 @@ public:
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);
};

1
src/pubkey.cpp
View file

@ -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);

4
src/pubkey.h
View file

@ -218,7 +218,7 @@ public:
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

11
src/script/descriptor.cpp
View file

@ -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 @@ public:
}
} 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 @@ public:
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;
}

18
src/test/bip32_tests.cpp
View file

@ -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()

4
src/test/descriptor_tests.cpp
View file

@ -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;

15
src/wallet/scriptpubkeyman.cpp
View file

@ -1080,6 +1080,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
@ -1097,11 +1104,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 {
@ -1109,7 +1116,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);
@ -1117,7 +1124,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);