search

spesmilo / electrum

Electrum Bitcoin Wallet
https://electrum.org
MIT License
7.44k stars 3.09k forks source link

Electrum misinterprets master extended key (depth 0) as child extended key with depth 3 #3671

Closed nym-zone closed 6 years ago

nym-zone commented 6 years ago

As briefly explained in my commit log at nym-zone/easyseed@c912169:

Expected behaviour

Wallet created from an “xprv”/“yprv”/“zprv” extended master private key matches wallet restored from BIP 39 mnemonic phrase plus appropriate BIP 32 derivation path.

Actual behaviour

Wallet restored from an “xprv”/“yprv”/“zprv” extended master private key generates/contains addresses different from that restored from the mnemonic as aforesaid. This may cause user loss of funds: If a user creates a wallet from an extended master private key, and uses the matching BIP 39 mnemonic phrase as a backup, then the backup will apparently be useless for restoring the wallet.

Example Test Data

The following “steps to reproduce” use this test vector, generated with easyseed:

Entropy:
50416ff29fdf0d7c1cdf7b076132c1c7

Mnemonic:
expect arena witness distance valid safe inflict urge alter another race moment

Seed:
c641850638402a5afb56e3252c1b4db2f4b15f3ae3e470335a2732baafd5704a6591d6a0103ca58011c020b82df665bf28d95b188f356436b581620cf6b1a57e

### Multiple formats of extended private key for use with Electrum.
### USE ONLY ONE OF THESE!

Old-style address (“1-Address”):
xprv9s21ZrQH143K2XmdMqcWE29L75KLystP5qeMrRh7NJZdKvdRykoR6wNVohA7yUeaRtK2vni2ybAAY7mt8QyAmJDt6EF7f7DhbHFNMit7keL

Segwit backward-compatible P2WPKH-nested-in-P2SH (“3-Address”):
yprvABrGsX5C9janspxkCCQ8S7EqH3TnvVsszxAadpazkJwWP2SfEQxyj12dpu7hyPJVqXRqgGJbSFWiRQPSr7PBZXuUxZwYF23Bs1K1kJTne9J

Segwit Bech32 New/Future Format (“Bravo Charlie One”):
zprvAWgYBBk7JR8Gj89s2ZBkeCLLT1cEs7sNv4goRDUt8KKPS8FtV58YM4gmr75HyHxRFAYeRju9tusGJh11ZooCMmb5pudxpvrg8jNf8rQJz5U

These can be reproduced with the following command, as of nym-zone/easyseed@c912169:

echo 50416ff29fdf0d7c1cdf7b076132c1c7 | xxd -p -r | \
    easyseed -E -A -b 128 -k -

Other test vectors were tried by me, from the set of official test vectors listed in BIP 39. E.g.:

Mnemonic: “hamster diagram private dutch cause delay private meat slide toddler razor book happy fancy gospel tennis maple dilemma loan word shrug inflict delay length”

Passphrase (Electrum “Extra Words”): “TREZOR”

Extended master private key:

xprv9s21ZrQH143K2XTAhys3pMNcGn261Fi5Ta2Pw8PwaVPhg3D8DWkzWQwjTJfskj8ofb81i9NP2cUNKxwjueJHHMQAnxtivTA75uUFqPFeWzk

Steps to Reproduce

  1. Create a wallet “test” in Electrum, using the “xprv”, “yprv”, or “zprv” extended master private key.

  2. Create another wallet “test_verify” in Electrum, using the BIP 39 mnemonic with the appropriate derivation path:

    • For “xprv”: m/44'/0'/0'
    • For “yprv”: m/49'/0'/0'
    • For “zprv”: m/84'/0'/0' (N.b. that I did not expect this to work, because BIP 84 was first published as a draft eleven days ago, on 2017-12-28. It creates a wallet full of old-style “1” addresses, rather than Bech32 “Bravo Charlie” addresses. The other two given derivation paths should certainly work.)

  3. Compare the addresses in the “test” wallet with those in the “test_verify” wallet.

Version Information

Tested with Electrum version 3.0.4.

Additional Information

I do not see how this could be a bug in my software. It is remotely possible that I could make a stupendously moronic mistake; but then, my software would not pass the test vectors as it does every make check (including “hamster diagram private dutch...” and dozens of other in English and Japanese). I tried to look through the Electrum codebase for some cause; but it is an unfamiliar codebase in an unfamiliar language, and I didn’t see anything obvious at a glance/grep through the BIP39/BIP32 parts of lib/keystore.py and lib/bitcoin.py.

After entering a BIP 39 mnemonic phrase into Electrum and clicking for the next screen, a bizarre small, blank window popped up. It did not go away, even when the Electrum process exited. I observe this on v3.0.4; I did not observe it on v3.0.3 or previous versions of Electrum, in which I have tested BIP 39 mnemonic entry.

To be clear: I strongly disagree with the nonstandard “xprv”/“yprv”/“zprv”/etc. scheme, which abuses BIP 32 serialization version numbers to convey information which should be contained in the derivation path. The same problem exists with “tpub”/“tprv” in the spec, insofar as testnet addresses also have their own derivation paths. An extended master private key (or public key) is not tied to only one type of address. However, I preliminarily implemented this in the hope of helping users adopt Segwit with the most popular userfriendly light client—for the good of users (lower fees), and for the good of the network.

ecdsa commented 6 years ago

you derived xprv/yprv/zprv at the root level. this is not how this spec works.

nym-zone commented 6 years ago

@ecdsa:

you derived xprv/yprv/zprv at the root level. this is not how this spec works.

To which spec do you refer by “this” spec? Electrum’s? Is it written down somewhere, outside the Python code?

My code perfectly implements BIP 39, and the pertinent portion of BIP 32 for master extended private key generation and serialization. It passes all English and Japanese test vectors, including xprv generation, referenced in the BIP 39 standard; thus, those are also derived at the root level (depth byte 0x00).

If Electrum follows a different spec, I would appreciate a reference I could use to implement it properly. Should the “depth” byte be set to 0x01, and the child number set to 0x8000002c (xprv), 0x80000031 (yprv), 0x80000054 (zprv)? Or...? If so, what should the parent’s fingerprint be set to when there is no parent?

FWIW, I searched the Electrum issues for pertinent keywords before I opened a new issue—and just now, again. Also, obviously I found the place in the Electrum code which contains the magic numbers for yprv and zprv; had a comment there explained or referred to a spec, I would have followed it. It is not as if I didn’t do my homework before writing an implementation, nor as before opening an issue.

ecdsa commented 6 years ago

we will release a BIP shortly in the meantime please read http://docs.electrum.org/en/latest/seedphrase.html#list-of-reserved-numbers

nym-zone commented 6 years ago

@ecdsa:

we will release a BIP shortly in the meantime please read http://docs.electrum.org/en/latest/seedphrase.html#list-of-reserved-numbers

Thanks for your reply. But this only lists the same magic numbers as I found in Electrum’s lib/bitcoin.py, and added to my code yesterday.

If I did not have those version numbers, then I would not have obtained “yprv” and “zprv” from the base58check encoding. I did not just slap those on, you know.

Now as for xprv, please try Electrum against the BIP 39 test vectors. My software passes. But with the official test vectors, Electrum 3.0.4 gives different results for (0) restoring wallet from xprv, versus (1) restoring wallet from BIP 39 mnemonic plus passphrase. I quoted one in my original comment on this issue (mnemonic: “hamster diagram private dutch...”, passphrase: “TREZOR”; versus xprv xprv9s21ZrQH143...). I tried this in Electrum; did you?

The input I feed to to base58check is laid out as such:

That passes the test vectors.

dabura667 commented 6 years ago

I think @ecdsa is saying you need to use m/44'/0'/0' xprv and not m/ xprv to recover, just like the xpub.

So your zprv should be derived:

  1. get root zprv
  2. derive m/84'/0'/0'
  3. export that node (depth:3 index: 0x80000000) as a zprv base58check.
dabura667 commented 6 years ago

yup. I was able to get the same addresses by:

> var btc = require('bitcoinjs-lib')
// import your root xprv you gave
> var hd = btc.HDNode.fromBase58('xprv9s21ZrQH143K2XmdMqcWE29L75KLystP5qeMrRh7NJZdKvdRykoR6wNVohA7yUeaRtK2vni2ybAAY7mt8QyAmJDt6EF7f7DhbHFNMit7keL')
// derive m/44'/0'/0' and export as base58check
> hd.deriveHardened(44).deriveHardened(0).deriveHardened(0).toBase58()
'xprv9z7KYqivhqD6FHfsDLbFEPDfWFt6xHzV1j3fG4rHoV4w2ppyd74aY8UwywJb8eqxAoJ7NqiDp1dCsYzicgnGPkhmhbLZkMnWhMwUxa23Uah'

And recovering from that.

nym-zone commented 6 years ago

I think @ecdsa is saying you need to use m/44'/0'/0' xprv and not m/ xprv to recover, just like the xpub.

So your zprv should be derived:

  1. get root zprv
  2. derive m/84'/0'/0'
  3. export that node (depth:3 index: 0x80000000) as a zprv base58check.

Thank you, @dabura667. I will try adding code for this later, if I want to support the Electrum use cases I’d intended.

The question then is why Electrum will accept a master extended private key with depth 0, and neither reject it, nor ask the derivation path and follow it (as it does from a BIP39 mnemonic). It will simply spit out entirely different addresses. This is a bug.

Neither considering my code nor the new Segwit yprv/zprv code:

You wrote whilst I was experimenting with Electrum, and gathering precise reproduction details and screenshots using a test vector from Trezor’s vectors.json. If Electrum will accept both of these and yet generate totally different addresses, then that’s a bug.

Following are the exact steps I followed, starting with an empty .electrum directory, with results and screenshots.

xprv from vectors.json

  1. Wallet name: “from_xprv”
  2. [radio button] Standard Wallet
  3. [radio button] Use public or private keys
  4. Enter the following from Trezor’s vectors.json: 
    xprv9s21ZrQH143K2XTAhys3pMNcGn261Fi5Ta2Pw8PwaVPhg3D8DWkzWQwjTJfskj8ofb81i9NP2cUNKxwjueJHHMQAnxtivTA75uUFqPFeWzk
  5. Wallet password: leave blank

Wallet addresses, starting from the first:

1ChUYDT2hHsn3uzfSKiZQYFRwTFkc1Nucn
15zPySjyph5wE5W2kbnPKWzTjdgBHeQXre
19EpKxqjoYn4CzFhgmfXQxJygSP5J7iyVC
1Q23FLhZ6h7TtJ6bfLS9GNQi59Qxmr3DP6
1GQ92yy3iR2crF3WDiX9hdU8V7bNfddS1N
[...]

electrum_from_xprv

Seed phrase from vectors.json

  1. File→New/Restore
  2. Wallet name: “from_bip39”
  3. [radio button] Standard wallet
  4. [radio button] I already have a seed
  5. Options button: Check both boxes, “Extend this seed with custom words” (for passphrase) and “BIP39 seed”
  6. Enter seed phrase: “hamster diagram private dutch cause delay private meat slide toddler razor book happy fancy gospel tennis maple dilemma loan word shrug inflict delay length” (checksum: ok)
  7. Seed extension: “TREZOR”
  8. Derivation: leave default m/44'/0'/0'
  9. Wallet password: leave blank

Wallet addresses, starting from the first:

1JocbdmrSq7UnydGJjVX83HjTTbvyUSm61
1J4Us3ijqVBPKjDYJCRvXRCwyx7RSk45RY
1KYk1sAgKh3sJzN8wExqAJRY5TG88jGDRP
1BHNRChgpTGUWa55Aqodw9kjnukaTLHPpV
1PF2YfmJvirVkcXx8G66LH6p19qd29qQAb
[...]

electrum_from_bip39

Suggested fix

Detect a master extended key (public or private) with depth 0, and interpose a dialogue asking the derivation path (just as the dialogue it uses for BIP39 phrases).

nym-zone commented 6 years ago

@dabura667, thank you for the information you provided. I confirmed on my end that the xprv you derived with bitcoinjs-lib generates the same addresses as obtained from BIP39 mnemonic restoration:

1Ei7ffqmSxzKk1tWV8oAKiktrKEnY7xD4Z
1PLpmMCb3c6dMKparSPaWru5HzQ54sih7E
1MencAr37B32cQG3cRvd4mLEEvXVdQfDyW
1G5UFJbHw8TGevuSBR74NKZTy4sFQwa2AR
1PT9LQtofGhP77PdQ5cbc5pc92cLfKARud
[...]

Now I know affirmatively which depth of key Electrum is expecting; so I can code to that. But if Electrum accepts the master extended key (depth byte 0x00, index 0x00000000) and simply gives the wrong addresses, then that is still a bug.

montvid commented 6 years ago

@ecdsa Thanks for the heads up about writing the BIP. I hope it will be up as soon as possible, please inform the general audience so we can push for a better standard than bip-39. Even the bitcoin core devs agree that bip-39 should be not implemented.

leafcutterant commented 6 years ago

I agree, this is still a bug.

I can enter the

when restoring a wallet from what Electrum calls "master private key" and they are all accepted, generating totally different wallets.

From trial it looks like Electrum refers to the second (Account Extended Private Key).

(I used the nomenclature from Ian Coleman's implementation.)