Reject the invalid extended keys of BIP32 test vector 5 - #440
Open
DMZ22 wants to merge 1 commit into
Open
Conversation
Three BIP32 rules were not enforced when parsing an extended key, so 8 of the 18 keys that must be rejected (16 from test vector 5 plus the two bitcoinfuzz strings in the issue) were accepted instead: - the serialization prefix says private or public, and the payload has to agree; the type was being taken from the payload alone, so an xpub carrying private key data parsed happily - a key at depth zero is a master key, so its parent fingerprint must be zero - a key at depth zero is nobody's child, so its child index must be zero The prefix check goes in hparse, which is the only place that knows which prefix matched; the depth checks go in deserialize, so they also cover callers that do not come through the parse API. This also corrects tests/cmds/test_cases/ku/bip84_ltc.txt, whose input was itself a "pubkey version / prvkey mismatch" key: a Litecoin BIP84 private key serialized with the zpub version bytes. It is replaced with the correctly serialized zprv, which produces identical output.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Closes #437.
I measured it before fixing it: of the 16 keys in BIP32 test vector 5 plus the two bitcoinfuzz strings from the issue body, 8 of 18 were accepted rather than rejected. They fall into exactly three rules, all of which were missing:
xprv9s21ZrQMAtJo7…(zero depth, child index40000000)tpubD6NzVbkrYhZ4W…(tpub carrying private key data)0200…07, invalid checksum0 of 18 are accepted now, and all of test vectors 1–4 still parse and round-trip.
Where the checks go
The prefix/payload agreement check is in
hparse. That is the only place that knows which of_bip32_prv_prefix/_bip32_pub_prefixactually matched —deserializecannot do it, becauseoverride_networkcalls it with the version bytes zeroed out. It returnsNone, matching the unknown-version and bad-checksum paths right above it. Being inhparseit covers bip49 and bip84 as well as bip32.The depth invariants are in
deserialize, since they are network-agnostic and this way they also cover callers that don't go through the parse API. They raiseEncodingError, which is what this file already imports and what the neighbouring payload failures (sec_to_public_pair, the secret exponent range check) already do.That does mean rejection is signalled two different ways depending on the case, but that is pre-existing — bad checksum returns
Nonetoday while a bad SEC encoding raises. Happy to make the depth checks returnNonefromhparseinstead if you'd rather the parse API were uniform.One thing worth a look
tests/cmds/test_cases/ku/bip84_ltc.txtwas itself a test-vector-5 key. Its input decodes to version04b24746— Litecoin's BIP84 public prefix — with a payload beginning00, i.e. private key data. A private key serialized with the public version bytes, which is precisely "pubkey version / prvkey mismatch". The fixture's own comment said "encoded as zpub" while the expected output reported"private_key": "yes".I replaced it with the correctly serialized
zprvof the same key. Every other field in the expected output is byte-identical, which confirms it is the same key material — the diff is the comment, the command, and theinputfield.Verification
tests/btc/bip32_test.py,tests/parse_test.py,tests/key_validate_test.py: 23 passed before, and pass after.tests/cmds/), plus the three new tests.bip32_test.pygo 9 passed → 2 failed, so the tests do guard the change.ruff format --checkis clean on both files I touched that were clean before.ParseAPI.pyis already unformatted onmain, so I left the rest of it alone rather than reformat it in this PR.