BIP32: CExtKey::SetSeed missing validation of seed bit-length #35308

Current behaviour

CExtKey::SetSeed does not check the seed bit-length constraints defined in BIP32

Generate a seed byte sequence S of a chosen length (between 128 and 512 bits; 256 bits is advised) from a (P)RNG.

I don't see a reason for not adhering to the spec in this case. Weak seeds used for master key generation can & have been exploited.

Found using the bitcoinfuzz tool.

Expected behaviour

CExtKey::SetSeed should provide bit-length (or byte-length) checks for the parameter seed e.g.,

void CExtKey::SetSeed(std::span<const std::byte> seed)
{
    if (seed.size() < 16) return; // or throw
    ...
}

Steps to reproduce

Added a testcase in key_tests.cpp and ran the tests:

BOOST_AUTO_TEST_CASE(extkey_setseed_empty)
{
    CExtKey key;
    
    // Test with 0 bytes
    std::vector<std::byte> empty{};
    key.SetSeed(empty);

    // Test with 1 byte
    std::vector<std::byte> one{std::byte{0x01}};
    key.SetSeed(one);
    
}

Running 8 test cases...

*** No errors detected

How did you obtain Bitcoin Core

Compiled from source

What version of Bitcoin Core are you using?

master@ed1795a

Operating system and version

Ubuntu 24.04.3 LTS

The reference about exploiting weak seeds is behind an authentication wall. Could you provide a public link for it?

edit: I found it: https://is.muni.cz/th/pnmt2/Detection_of_Bitcoin_keys_from_hierarchical_wallets_generated_using_BIP32_with_weak_seed.pdf

The attacks in the cited paper are about seeds with weak entropy content, not short seeds, so length alone is a poor proxy for the vulnerability described.

That said, if a length check is added for spec compliance, I believe the correct approach would be to add Assert(seed.size() >= 16 && seed.size() <= 64), since all production callers already pass a validated 32-byte seed it should not be a problem.

The attacks in the cited paper are about seeds with weak entropy content, not short seeds, so length alone is a poor proxy for the vulnerability described.

I would argue that being short qualifies seeds as weak, meaning they are easier to brute force.

I would argue that being short qualifies seeds as weak, meaning they are easier to brute force.

Yes, but a short seed can still be stronger than a big seed filled by only 0 bits. What I meant is that just checking seed length alone there would be for BIP32 spec compliance and not for preventing weak seeds, because you need to verify entropy quality before generation, and Core already does that on startup Random_SanityCheck.

Does this really need fixing? We only ever call this in production with a CKey, which has a defined size. When hooking this up to bitcoinfuzz the same data structure could just be used too, no?

Does this really need fixing? We only ever call this in production with a CKey, which has a defined size.

I'd say that adding this check or at least documenting this constraint is better than depending on all future callers to implicitly know this. The change is trivial and will not affect the current uses as they already pass valid bitlengths as you said. It's more about robustness and future-proofing.

When hooking this up to bitcoinfuzz the same data structure could just be used too, no?

I think that would defeat the purpose. In bitcoinfuzz, we want to use the same fuzzer-provided bytes for key generation across libraries and there are currently no length limitations on the input bytes we pass. The only exception is if a given library deems the seed too long or short, then the case is skipped for that library. I think it can stay as is.

I think it is important to also note that Core isn't the only one missing this check.

I reported this issue in several Bitcoin libraries and most seem to address it, maybe we can get inspired there:

https://github.com/rust-bitcoin/rust-bitcoin/issues/6199

https://github.com/bitcoin-s/bitcoin-s/issues/6350

https://github.com/MetacoSA/NBitcoin/issues/1301

https://github.com/bitcoinj/bitcoinj/issues/4209

https://github.com/diybitcoinhardware/embit/issues/127

https://github.com/richardkiss/pycoin/issues/438