sbuss / bitmerchant

Bitcoin merchant tools
MIT License
82 stars 36 forks source link

Check for rare corner case where private key is > 2**256 - 2**32 - 2v9 - 2**8 - 2**7 - 2**6 - 2**4 - 1 #33

Closed mflaxman closed 10 years ago

mflaxman commented 10 years ago

While very unlikely, I don't see this check anywhere in the code?

http://bitcoin.stackexchange.com/questions/3609/can-an-sha256-hash-be-used-as-an-ecdsa-private-key

sbuss commented 10 years ago

The linked thread actually has the wrong value for the maximum valid private key. The correct maximum value can be found here: https://en.bitcoin.it/wiki/Private_key#Range_of_valid_private_keys

Because I rely on the python ECDSA library for the elliptic curve math, it verifies that we're not using an invalid private key:

In [1]: from bitmerchant.wallet.keys import PrivateKey

In [2]: h = "FFFF FFFF FFFF FFFF FFFF FFFF FFFF FFFE BAAE DCE6 AF48 A03B BFD2 5E8C D036 4141".replace(" ", "")

In [3]: max_valid = long(h, 16) - 1

In [4]: k = PrivateKey(max_valid)

In [5]: k = PrivateKey(max_valid + 1)
---------------------------------------------------------------------------
AssertionError                            Traceback (most recent call last)
<ipython-input-5-6455c9cec7b9> in <module>()
----> 1 k = PrivateKey(max_valid + 1)

/Users/sbuss/workspace/bitmerchant/bitmerchant/bitmerchant/wallet/keys.pyc in __init__(self, secret_exponent, network, *args, **kwargs)
     54         super(PrivateKey, self).__init__(network=network, *args, **kwargs)
     55         self._private_key = SigningKey.from_secret_exponent(
---> 56             secret_exponent, curve=SECP256k1)
     57 
     58     def get_key(self):

/Users/sbuss/envs/bitmerchant/lib/python2.7/site-packages/ecdsa/keys.pyc in from_secret_exponent(klass, secexp, curve, hashfunc)
    133         self.baselen = curve.baselen
    134         n = curve.order
--> 135         assert 1 <= secexp < n
    136         pubkey_point = curve.generator*secexp
    137         pubkey = ecdsa.Public_key(curve.generator, pubkey_point)

AssertionError: 

In [6]: max_valid
Out[6]: 115792089237316195423570985008687907852837564279074904382605163141518161494336L

I think it's better to explicitly throw an error than to catch it or silently mod the user input by the maximum value to be in the valid range.

It is possible for a user to give a master secret that results in an invalid key, but that is exceedingly rare and will throw an exception anyway.

I didn't implement a similar check in the BIP32 child derivation method (see https://github.com/bitcoin/bips/blob/master/bip-0032.mediawiki#child-key-derivation-ckd-functions). The probability of deriving an invalid child is so small that it's basically impossible. However, after thinking about this more, it might be possible for an invalid derivation to result in a valid key... I'm going to have to try and work this out. I should probably just implement that check, though.

mflaxman commented 10 years ago

+1 to everything, I definitely agree that failing loudly is the way to go.

This is a good example of where bitmerchant's organization and easy-to-read code shine.