added test vector #5 for invalid extended keys #921

pull fametrano wants to merge 2 commits into bitcoin:master from fametrano:patch-4 changing 1 files +20 −0

fametrano commented at 12:58 AM on May 16, 2020: contributor

The BIP32 specification lacks test vectors for invalid extended keys that should not be parsed as valid. Such test vectors are proposed here.
fametrano renamed this:
~~added invalid extended keys vectors~~
added test vector for invalid extended keys
on May 16, 2020
fametrano renamed this:
~~added test vector for invalid extended keys~~
added test vectors for invalid extended keys
on May 16, 2020
junderw commented at 9:57 AM on May 17, 2020: contributor
good point.

I might also add some derivation tests that test specifically for leading zeroes in the private key for hardened derivation.

So many bigInt libraries in many languages convert to byte arrays with varying lengths and people forget that the ser_key needs to be 32 bytes.

For example: btcsuite btcutil ExtendedKey will not derive the following correctly as per this Pull Request: https://github.com/btcsuite/btcutil/pull/161
```
Root m:
xprv9s21ZrQH143K2PjtEr9foXN58DmBJu3nHmSnzXwvbqQiKqWZSsGPCNG1Je5qVeBF8fGGffBHAFu4jqXLkaHW4AbnByYDCfGKB4pxpyTTFRn
m/0H
xprv9v7bi4PiA35i9dhjDptXMMn46wLrFx37xorBaJhYzvjYUFC4fAFmPv1thGA9BinMyqdh6YnDAfasQdCBpYKUkW548fmeG1ZPkXLy5KLwkUY
```
Would be one such example.
luke-jr added the label Proposed BIP modification on Jun 1, 2020
luke-jr assigned sipa on Jun 1, 2020
luke-jr commented at 7:27 PM on June 1, 2020: member

@sipa
fametrano commented at 11:00 AM on June 2, 2020: contributor

I might also add some derivation tests that test specifically for leading zeroes in the private key for hardened derivation.

that is already covered by the test vector 3 added in April 2017
fametrano commented at 2:34 PM on August 19, 2020: contributor

If it helps, a test vector is available here, as part of the btclib test suite, namely tested here
fametrano cross-referenced this on Aug 19, 2020 from issue Derive child keys from an ExtendedKey by achow101
klim0v commented at 3:57 PM on October 19, 2020: none

https://github.com/bitcoin/bips/pull/905
fametrano renamed this:
~~added test vectors for invalid extended keys~~
added test vector #4 for invalid extended keys
on Nov 15, 2020
fametrano commented at 9:41 AM on November 15, 2020: contributor

#905

Unless I am missing something, a library not failing at test vector #3 would not fail at this new proposed case: e.g. see btclib-org/btclib@0cab434
fametrano commented at 9:59 AM on November 15, 2020: contributor

is there anything I could do to help this PR being accepted?

For a json file of the invalid keys see in the btclib library https://github.com/btclib-org/btclib/blob/master/btclib/tests/test_data/bip32_invalid_keys.json
sipa commented at 7:57 PM on November 16, 2020: member

The tests for "zero parent fingerprint with non-zero depth" are wrong. There is no reason why fingerprint 0 implies it's a root.

added invalid extended keys vectors

The BIP32 specification lacks test vectors for invalid extended keys that should not be parsed as valid. Such test vectors are proposed here.

ee2e059820

fametrano force-pushed on Nov 17, 2020
fametrano commented at 6:19 AM on November 17, 2020: contributor

The tests for "zero parent fingerprint with non-zero depth" are wrong. There is no reason why fingerprint 0 implies it's a root.

My bad! I've amended the code and squashed all commits so far.
fametrano commented at 4:07 PM on August 8, 2021: contributor

any chance for this to be accepted?
MarcoFalke commented at 11:12 AM on August 23, 2021: member

Needs rebase?
luke-jr commented at 4:08 PM on August 23, 2021: member

Rebasing isn't needed assuming the vector # can just be bumped to 5.

Just needs @sipa's approval
Merge branch 'master' into patch-4 f9a81b7791
fametrano renamed this:
~~added test vector #4 for invalid extended keys~~
added test vector #5 for invalid extended keys
on Aug 25, 2021
fametrano commented at 8:56 PM on August 25, 2021: contributor

Rebased, solving the vector # conflict: if/when approved, this PR can be just squashed&merged
sipa cross-referenced this on Aug 30, 2021 from issue Stricter BIP32 decoding and test vector 5 by sipa
sipa commented at 4:36 PM on August 30, 2021: member

ACK.

See implementation for Bitcoin Core here: https://github.com/bitcoin/bitcoin/pull/22836
luke-jr merged this on Aug 30, 2021
luke-jr closed this on Aug 30, 2021
benthecarman cross-referenced this on Aug 30, 2021 from issue Add new invalid BIP 32 test vectors by benthecarman
benthecarman commented at 7:58 PM on August 30, 2021: contributor

ACK

this found a bug in bitcoin-s

https://github.com/bitcoin-s/bitcoin-s/pull/3634
fametrano deleted the branch on Aug 31, 2021
darosior cross-referenced this on Sep 1, 2021 from issue More deserialization sanity checks, BIP32 test vector 5 by darosior
darosior commented at 9:08 AM on September 1, 2021: member

ACK

Implemented in python-bip32, which was too lax on parsing. https://github.com/darosior/python-bip32/pull/17
fanquake referenced this in commit 01fa1481f9 on Sep 2, 2021
sidhujag referenced this in commit 8fdd2cb327 on Sep 2, 2021
kristapsk cross-referenced this on Nov 3, 2021 from issue BIP32 test vector 5 should be added to tests and checked by kristapsk
kristapsk cross-referenced this on Feb 17, 2022 from issue Stricter BIP32 decoding and test vector 5 by kristapsk
kristapsk referenced this in commit 83918a971d on Mar 2, 2022

Contributors

Labels

Proposed BIP modification

Linked (view graph)

#905 add more tests for leading zero