spesmilo / electrum

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

WIF key with scalar outside curve order is accepted #5797

Closed tnkmt closed 4 years ago

tnkmt commented 4 years ago

I found this in the source code:

def _CKD_priv(parent_privkey: bytes, parent_chaincode: bytes,
              child_index: bytes, is_hardened_child: bool) -> Tuple[bytes, bytes]:
    try:
        keypair = ecc.ECPrivkey(parent_privkey)
    except ecc.InvalidECPointException as e:
        raise BitcoinException('Impossible xprv (not within curve order)') from e
    parent_pubkey = keypair.get_public_key_bytes(compressed=True)
    if is_hardened_child:
        data = bytes([0]) + parent_privkey + child_index
    else:
        data = parent_pubkey + child_index
    I = hmac_oneshot(parent_chaincode, data, hashlib.sha512)
    I_left = ecc.string_to_number(I[0:32])
    child_privkey = (I_left + ecc.string_to_number(parent_privkey)) % ecc.CURVE_ORDER
    if I_left >= ecc.CURVE_ORDER or child_privkey == 0:
        raise ecc.InvalidECPointException()
    child_privkey = ecc.number_to_string(child_privkey, ecc.CURVE_ORDER)
    child_chaincode = I[32:]
    return child_privkey, child_chaincode

Is this check only working when the private key is being generated from the master private key? Or it's not working at all?

The invalid Bitcoin key I used to test Electrum was 5Km2kuu7vtFDPpxywn4u3NLu8iSdrqhxWT8tUKiNTjDB7yUFUg3.

Also, I tried the "Bitaddress", "TP's Go Bitcoin Tests - Private keys" and "Bitcoin Key Compression Tool - Ian Coleman" with this invalid WIF private key and they give error message like expected.

Coinbin also have this bug.

Discussion on Reddit.

SomberNight commented 4 years ago

Yes, you can import WIF keys where the EC scalar falls outside the curve range. We take the privkey modulo the group order. This is not a bug; it's a design decision. https://github.com/spesmilo/electrum/blob/ddeb176b3da0c4fcbb6cf2b8d85dd319e2a2bf56/electrum/bitcoin.py#L610-L611

SomberNight commented 4 years ago

def _CKD_priv(parent_privkey: bytes, parent_chaincode: bytes, child_index: bytes, is_hardened_child: bool) -> Tuple[bytes, bytes]:

That method is about bip32.

tnkmt commented 4 years ago

What will happen if anyone is receiving coins to such address and then tries to spend them? Coins will be unmovable (lost)?

SomberNight commented 4 years ago

No. It will work. You can test it on testnet/regtest.

tnkmt commented 4 years ago

It make sense. Nodes do not know the private key (they "see" only the addresses, public keys and signatures).

And it's not possible in practice to detect invalid private keys just by looking at their addresses, public keys and signatures. Right?

SomberNight commented 4 years ago

Effectively, with what we are doing, some group elements have two representations; while most only have one. Your "invalid" key contains a number that is too large to be a group element; but we subtract the group order from it and use that number instead. It's as if you imported the "valid" key in the first place.

In fact if you try to export it, we will give you the "valid" one. Just try it: create an imported wallet from key 5Km2kuu7vtFDPpxywn4u3NLu8iSdrqhxWT8tUKiNTjDB7yUFUg3 and then try to export the key. It will export 5HpHagT65TZzG1PH3CSu63kCkUBpkA7sBXKmV2joANEAPgL2Hb6 instead.