search

bitpay / bitcore-lib

A pure and powerful JavaScript Bitcoin library
https://bitcore.io/
Other
611 stars 1.03k forks source link

BIP32 private derivation produces invalid results in certain cases #94

Open axic opened 7 years ago

axic commented 7 years ago

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: https://github.com/MetaMask/metamask-plugin/issues/640

coder5876 commented 7 years ago

Related to https://github.com/bitpay/bitcore-lib/issues/47

axic commented 7 years ago

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 commented 7 years ago

Indeed, the toString() method of bn.js includes an option to serialize with padding, since 3.1.2: https://github.com/indutny/bn.js/commit/49dd63826efeddaf519f2ba53e40b90694f24260

axic commented 7 years ago

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

braydonf commented 7 years ago

@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 commented 7 years ago

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 commented 7 years ago

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 commented 7 years ago

Okay, I have a PR with a patch: https://github.com/bitpay/bitcore-lib/pull/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 commented 7 years ago

Is this issue still open?

dabura667 commented 7 years ago

@Levino Apparently so: https://github.com/bitpay/bitcore-lib/blob/master/lib/hdprivatekey.js#L183

levino commented 7 years ago

@braydonf Is this not someone "critical" and should be fixed asap? Or am I mistaken on the severity?

dabura667 commented 7 years ago

@Levino Review #97

rickw commented 7 years ago

This still isn't fixed? I had to stop using my TREZOR because they use this library.

EDIT: and anyone creating a key now won't work after it's fixed.

braydonf commented 7 years ago

@Levino, yes it's critical in the sense that any new implementations should be using correct derivation. There are other libraries that can be used for new implementations, such as https://github.com/bcoin-org/bcoin, https://github.com/bitcoinjs/bitcoinjs-lib and https://github.com/cryptocoinjs/hdkey (FWIW both hdkeys and bcoin use libsecp256k1 bindings and are much faster for hd derivation).

Existing implementations will need to have a strategy for detecting and recovering any affected keys, and why #97 includes deriveNonCompliantChild for this purpose.

@rickw Trezor doesn't use bitcore-lib for keys, this is handled by their hardware. From what I remember, they use Insight/bitcore-node as their source of the block chain, and is not affected.

rickw commented 7 years ago

@braydonf when I couldn't regenerate my wallet from the seed phrase they told me this was the problem. And when I checked my private key separately it has this bug in it. (Leading Zero) TREZOR told me when this is fixed then I can regenerate my wallet.

dabura667 commented 7 years ago

@rickw You can regenerate your TREZOR seed using any non-bitcore-lib app.

Mycelium is one of them, so is Electrum.

The problem is that Copay and bitcore-lib based wallets derive different wallets in 1 / 256 cases. You were unlucky, but it has nothing to do with Trezor.

Your Trezor will work fine (the private key generation is all embedded in the Trezor device) the only thing that won't work is if you try to restore your Trezor seed in Copay or any bitcore-lib based wallet / recovery app.

If you need to restore your wallet right now for some reason you can use Electrum, however, just as if you inserted the phrase into Copay, inserting the phrase into ANY app will defeat the purpose of Trezor (keeping your seed untouched by any online machine or malware)

rickw commented 7 years ago

@dabura667 thanks, most of that I know and when I was trying to restore the wallet they were using the bitcore-lib, so they told me. I have everything handled. I had originally looked at a patch but those months ago I thought I read in one of the threads that it was handled. Since I stopped using TREZOR and bitcore personally I was ok. I was just surprised when I got the email today that his wasn't fixed. My apologies for sounding harsh.