Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

(CRITICAL) Incorrect derivation for certain BIP39 keys, fund loss >:( #58

Closed
a49q258k opened this issue Feb 23, 2017 · 28 comments
Closed

Comments

@a49q258k
Copy link

a49q258k commented Feb 23, 2017

Test case (by luck this is the first one I generated, thankfully I cross-referenced with other tools. Not all mnemonics / root keys trigger this bug)

mnemonic: fruit wave dwarf banana earth journey tattoo true farm silk olive fence
passphrase: banana

https://iancoleman.github.io/bip39/ derived first address: 17rxURoF96VhmkcEGCj5LNQkmN9HVhWb7F (also shared by Electrum)

Other clients derive a different address (Copay, BIP32JP, etc): 13EuKhffWkBE2KUwcbkbELZb1MpzbimJ3Y

@a49q258k
Copy link
Author

@iancoleman @galeksandrp @dangershony @chrisrico @dooglus

Alerting for attention, please reproduce. Until this is fixed, the tool should be taken offline

@iancoleman
Copy link
Owner

iancoleman commented Feb 23, 2017

Thanks for reporting this

My findings so far:

The root BIP32 key is the same for both this tool and bip32jp, so the error is happening in the derivation (as stated in the issue title)

Using bip32.org and the root BIP32 key, with BIP44 path of m/44'/0'/0'/0/0 the same address is generated as this tool - so bip32.org agrees with this tool, but not with Copay or bip32jp. This would seem to suggest an issue with a particular library.

bip32jp and bip32.org both use bitcoinjs-lib-0.1.3 whereas this tool uses bitcoinjs-lib-1.5.7 so from this it seems unlikely that the bitcoinjs-lib is the cause of this issue.

I will keep investigating and update as I find more.


Mnemonic

fruit wave dwarf banana earth journey tattoo true farm silk olive fence

Password

banana

Root key (all agree):

xprv9s21ZrQH143K25QhxbucbDDuQ4naNntJRi4KUfWT7xo4EKsHt2QJDu7KXp1A3u7Bi1j8ph3EGsZ9Xvz9dGuVrtHHs7pXeTzjuxBrCmmhgC6

Derivation path:

m/44'/0'/0'/0/0
Tool Address Library
iancoleman/bip39 17rxU... bitcoinjs-lib-1.5.7
bip32.org 17rxU... bitcoinjs-lib-0.1.3 + bip32.js
bip32jp 13EuK... bitcoinjs-lib-0.1.3 + bip32.js
bitcore.io 13EuK... bitcore
electrum 17rxU bitcoin.py

@iancoleman
Copy link
Owner

@bip32JP also alerting for attention

@iancoleman
Copy link
Owner

iancoleman commented Feb 23, 2017

It appears to be an issue with hardened derivation

m/44/0 (unhardened) gives the same addresses - 1Q1ad
m/44'/0 (hardened) gives different addresses - 1nipn vs 1PZFF

This is true for all hardened indexes, eg
m/0'/0 gives different addresses - 18DxV vs 1JtnT

Exactly where the hardening deviates is still something I'm looking into.

See #58 (comment) for reason behind the different addresses.

On a broader level, there is no reference implementation for bip32 derivation. So I'm still unsure where the issue lies and unfortunately all bip32 wallets must be treated as equally dubious at this stage. I'm also going to go through the official bip32 test vectors and see which libraries implement them.

@ralyodio
Copy link

Can you provide a simple test case? I'd like to try in the wallets I use to see if they are affected.

@chjj
Copy link

chjj commented Feb 23, 2017

Bcoin seems to agree with iancoleman/bip39:

var bcoin = require('bcoin');
var seed, key, pub, addr;
seed = bcoin.hd.Mnemonic.fromPhrase('fruit wave dwarf banana earth journey tattoo true farm silk olive fence').toSeed('banana')
key = bcoin.hd.PrivateKey.fromSeed(seed);
pub = key.derivePath("m/44'/0'/0'/0/0").publicKey;
addr = new bcoin.keyring(pub);
console.log(addr);

Output:

{ network: 'main',
  witness: false,
  nested: false,
  publicKey: '02ee29857a40f81c39181dffac6937e65473d16c6274368e337d1ede2892036ae8',
  script: null,
  program: null,
  type: 'pubkeyhash',
  address: '17rxURoF96VhmkcEGCj5LNQkmN9HVhWb7F' }

@chjj
Copy link

chjj commented Feb 23, 2017

Posting the seed derived from bcoin here for reference: 4b381541583be4423346c643850da4b320e46a87ae3d2a4e6da11eba819cd4acba45d239319ac14f863b8d5ab5a0d0c64d2e8a1e7d1457df2e5a3c51c73235be

Master xprivkey: xprv9s21ZrQH143K25QhxbucbDDuQ4naNntJRi4KUfWT7xo4EKsHt2QJDu7KXp1A3u7Bi1j8ph3EGsZ9Xvz9dGuVrtHHs7pXeTzjuxBrCmmhgC6

@iancoleman
Copy link
Owner

Can you provide a simple test case? I'd like to try in the wallets I use to see if they are affected.

@chovy

  • Enter the mnemonic fruit wave dwarf banana earth journey tattoo true farm silk olive fence
  • Enter the BIP39 password banana
  • Use BIP44 derivation for the first account
  • The first generated address may be one of 17rxU... or 13EuK..., when it should be the same for all

See #58 (comment) for a list of clients that have been tested so far.

@iancoleman
Copy link
Owner

https://medium.com/@alexberegszaszi/why-do-my-bip32-wallets-disagree-6f3254cc5846#.86inuifuq

The implementation in bitcore-lib has an issue with certain private keys. It drops the leading byte if it is a zero.

According to that post, the 17rxU... address would be correct (ie this tool, electrum, bip32.org are correct)

@braydonf
Copy link

braydonf commented Feb 23, 2017

I've extensively compared results across several libraries, and bcoin (https://github.com/bcoin-org/bcoin), bitcoinjs-lib (https://github.com/bitcoinjs/bitcoinjs-lib), libbtc (https://github.com/libbtc/libbtc), hdkeys (https://github.com/cryptocoinjs/hdkey), and many others, are all using correct BIP32 derivation.

The derivation in Copay (and anything using bitcore-lib) is incorrect. The bug is when there is a leading zero of the private key and the hash during derivation does not include the zero. The BIP32 specification states that the size of the private key is always 32 bytes before it's hashed.

FWIW: Funds will still be recoverable, however it may be cumbersome to derive both sets of private keys for recovery for those derivations affected.

@dangershony
Copy link
Contributor

@thashiznets @NicolasDorier
This is the implementation we are using in C#
https://github.com/Thashiznets/BIP39.NET
I will try to run this mnemonic on our framework.

@iancoleman
Copy link
Owner

bitcore-lib does implement the bip32 test vectors, see https://github.com/bitpay/bitcore-lib/blob/764aa6d4e9f28969192db2e8c1c82326cdbb6a6b/test/hdkeys.js#L240

So despite passing the reference test vectors, the library is still able to create invalid addresses.

I suggest that perhaps the reference test vectors may be extended to include a test that covers this particular case of dropping leading zeros.

@NicolasDorier
Copy link

NicolasDorier commented Feb 23, 2017

NBitcoin: 17rxURoF96VhmkcEGCj5LNQkmN9HVhWb7F @dangershony
I used @Thashiznets implementation, but kind of changed lots of stuff, so you might need to check also.

new Mnemonic("fruit wave dwarf banana earth journey tattoo true farm silk olive fence")
.DeriveExtKey("banana")
.Derive(new KeyPath("m/44'/0'/0'/0/0"))
.Neuter()
.PubKey
.GetAddress(Network.Main).ToString()

Output

17rxURoF96VhmkcEGCj5LNQkmN9HVhWb7F 

@jhoenicke
Copy link

Is this the same bug as bitpay/bitcore-lib#94 ?
The problem was that private key derivation didn't include leading zero bytes if the private key starts with them. I thought the bug was fixed though.

@dangershony
Copy link
Contributor

Thanks @NicolasDorier that was fast, I can confirm also my code has the same result which is based on @Thashiznets implementation

@iancoleman
Copy link
Owner

Is this the same bug as bitpay/bitcore-lib#94 ?

Yes. Any wallet using bitcore for HD wallets is affected.

The bug has been known since Feb 2016 - a long time for incompatible implementations to be out in the wild.
bitpay/bitcore-lib#47

Also appropriate for this list: bitpay/bitcore-lib#94 and bitpay/bitcore-lib#97

@realindiahotel
Copy link

@dangershony I'm getting as follows from: https://github.com/Thashiznets/BIP39.NET

banana

fruit wave dwarf banana earth journey tattoo true farm silk olive fence

4b381541583be4423346c643850da4b320e46a87ae3d2a4e6da11eba819cd4acba45d239319ac14f863b8d5ab5a0d0c64d2e8a1e7d1457df2e5a3c51c73235be

@realindiahotel
Copy link

bip39

@realindiahotel
Copy link

@dangershony and now I just saw you also tested, thanks :)

@bip32JP
Copy link

bip32JP commented Feb 23, 2017

Will fix later tonight.

Perhaps we should print a warning / show an alert when the derivation needs to pad from now on...

@bip32JP
Copy link

bip32JP commented Feb 23, 2017

Bip32JP is goal post moving fix implemented.

It's not a permanent fix, as I am on the go and only have iPhone to edit girhub, so difficult.

I have confirmed 17rx... on my public site.

@dangershony
Copy link
Contributor

@Thashiznets we should thank you for deving bip39, you are welcome to join the Blockchain C# community on stratisplatform.slack.com

@bip32JP
Copy link

bip32JP commented Feb 23, 2017

BIP32JP now has more permanent fix in place.

  1. Asks user if they want to use old buggy code and if they don't know what that means say no. (I assume most users just click cancel to any pop up they don't understand or the x mark)
  2. global variable (only persists until the tab visits another site) stores which derivation they want to use.
  3. I ran tests, and it looks like on average the bug occurs around 1 / 256 hard key derivations (makes sense)

So in theory, any wallet that makes 256 accounts in Copay will likely have one account that is wrong compared to other implementations.

@vogelito
Copy link

Does this affect derivation as per BIP45?

iancoleman pushed a commit to iancoleman/bips that referenced this issue Feb 23, 2017
These additional test vectors will ensure all future implementations are
interoperable.
See iancoleman/bip39#58
and bitpay/bitcore-lib#47
@realindiahotel
Copy link

@dangershony sounds good cheers! Send me an invite to thashiznets@yahoo.com.au and I'll join in :D

sstone added a commit to ACINQ/bitcoin-lib that referenced this issue Feb 27, 2017
hypo-test added a commit to hypo-test/BitcoinBips-713993353 that referenced this issue Nov 25, 2018
These additional test vectors will ensure all future implementations are
interoperable.
See iancoleman/bip39#58
and bitpay/bitcore-lib#47
gitsucker added a commit to gitsucker/better-Bitcoin-improvement-proposals that referenced this issue Nov 25, 2018
These additional test vectors will ensure all future implementations are
interoperable.
See iancoleman/bip39#58
and bitpay/bitcore-lib#47
@s1r-mar71n
Copy link

can this bug happen aswell without passphrase?

@jhoenicke
Copy link

@s1r-mar71n Yes, this was a bug in bip32 derivation and completely independent of bip39.

@Sintayew4
Copy link

#58

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

16 participants