BIP32 private derivation produces invalid results in certain cases

[axic]

It seems that it happens when the previous steps' private key ended up with a leading zero, because that is dropped when crafting the input for the HMAC.

The affected line: https://github.com/bitpay/bitcore-lib/blob/master/lib/hdprivatekey.js#L183

For a detailed discussion see: MetaMask/metamask-extension#640

[coder5876]

Related to #47

[axic]

The private key is stored using a wrapper over bn.js here: https://github.com/bitpay/bitcore-lib/blob/master/lib/hdprivatekey.js#L193

And the said wrapper's serialisation method (https://github.com/bitpay/bitcore-lib/blob/master/lib/crypto/bn.js#L71) must have this issue.

I'm wondering though why this wrapper isn't kept up to date since July 2015. bn.js was improved greatly early this year and is able to perform all these tasks, without issues.

[braydonf]

Indeed, the toString() method of bn.js includes an option to serialize with padding, since 3.1.2: indutny/bn.js@49dd638

[axic]

@braydonf actually I would suggest using toArrayLike(Buffer, 32, 'be') to avoid the need of back and forth hex conversion.

[braydonf]

@axic Thanks for reporting.

BIP032 states serialization to be 32 bytes as input, and with leading zero for 33 bytes: https://github.com/bitcoin/bips/blob/master/bip-0032.mediawiki#child-key-derivation-ckd-functions

[braydonf]

I've created a 1.0.0 development branch for collaboration on changes that change the API:
https://github.com/bitpay/bitcore-lib/tree/1.0.0

[axic]

BIP032 states serialization to be 32 bytes as input, and with leading zero for 33 bytes:

There's a leading zero prepended in every case during private key derivation, however the bug here is that if the private key has a leading zero, that is omitted.

The format is: [uint8: zero][uint256: private key][uint32: path index]

e.g. 000062b830267819c737684672a04f93cbad7774093d7c365d375d3a1dda12e18280000000 is wrongly stored as 0062b830267819c737684672a04f93cbad7774093d7c365d375d3a1dda12e18280000000

[braydonf]

Okay, I have a PR with a patch: #97

It could be useful to have this test case be a part of the BIP itself, and to cross reference with other tests with other libraries.

[levino]

Is this issue still open?