-
Notifications
You must be signed in to change notification settings - Fork 1.5k
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
Comments
@iancoleman @galeksandrp @dangershony @chrisrico @dooglus Alerting for attention, please reproduce. Until this is fixed, the tool should be taken offline |
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 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
Password
Root key (all agree):
Derivation path:
|
@bip32JP also alerting for attention |
This is true for all hardened indexes, eg
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. |
Can you provide a simple test case? I'd like to try in the wallets I use to see if they are affected. |
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' } |
Posting the seed derived from bcoin here for reference: Master xprivkey: |
See #58 (comment) for a list of clients that have been tested so far. |
https://medium.com/@alexberegszaszi/why-do-my-bip32-wallets-disagree-6f3254cc5846#.86inuifuq
According to that post, the |
I've extensively compared results across several libraries, and 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. |
@thashiznets @NicolasDorier |
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. |
NBitcoin: 17rxURoF96VhmkcEGCj5LNQkmN9HVhWb7F @dangershony
Output
|
Is this the same bug as bitpay/bitcore-lib#94 ? |
Thanks @NicolasDorier that was fast, I can confirm also my code has the same result which is based on @Thashiznets implementation |
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. Also appropriate for this list: bitpay/bitcore-lib#94 and bitpay/bitcore-lib#97 |
@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 |
@dangershony and now I just saw you also tested, thanks :) |
Will fix later tonight. Perhaps we should print a warning / show an alert when the derivation needs to pad from now on... |
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. |
@Thashiznets we should thank you for deving bip39, you are welcome to join the Blockchain C# community on stratisplatform.slack.com |
BIP32JP now has more permanent fix in place.
So in theory, any wallet that makes 256 accounts in Copay will likely have one account that is wrong compared to other implementations. |
Does this affect derivation as per BIP45? |
These additional test vectors will ensure all future implementations are interoperable. See iancoleman/bip39#58 and bitpay/bitcore-lib#47
@dangershony sounds good cheers! Send me an invite to thashiznets@yahoo.com.au and I'll join in :D |
These additional test vectors will ensure all future implementations are interoperable. See iancoleman/bip39#58 and bitpay/bitcore-lib#47
These additional test vectors will ensure all future implementations are interoperable. See iancoleman/bip39#58 and bitpay/bitcore-lib#47
can this bug happen aswell without passphrase? |
@s1r-mar71n Yes, this was a bug in bip32 derivation and completely independent of bip39. |
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
The text was updated successfully, but these errors were encountered: