Skip to content
New issue

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

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

Already on GitHub? # to your account

is_prime=True and is_prime=False produce the same output #51

Closed
shea256 opened this issue Sep 9, 2015 · 5 comments · Fixed by #52
Closed

is_prime=True and is_prime=False produce the same output #51

shea256 opened this issue Sep 9, 2015 · 5 comments · Fixed by #52

Comments

@shea256
Copy link

shea256 commented Sep 9, 2015

The following two operations give me the same output:

>>> wallet.get_child(0, is_prime=True)
>>> wallet.get_child(0, is_prime=False)

I would expect a different key for a hardened (prime) key and an unhardened one.

Is there something I'm doing wrong here or is this a bug in the library?

@sbuss
Copy link
Owner

sbuss commented Sep 9, 2015

Oh, wow, this is quite a bug. I will have a fix for this out later tonight, along with instructions stating how to recover from it if you are affected by it. The short of it is: caching was busted and was returning some invalid keys, but no coins are lost because we can just regenerate any bad keys.

@shea256
Copy link
Author

shea256 commented Sep 9, 2015

Yeah, ha, scared me a bit when I noticed it.

And sounds good.

@shea256
Copy link
Author

shea256 commented Sep 9, 2015

To clarify, are hardened keys the default here, or are unhardened keys?

If it's the latter, then:

  1. this could result in privacy issues, as someone can go up the chain when they shouldn't be able to
  2. this could result in the leak of the master for users who have shared their private key for a particular sub-account and assumed that the master private key was safe

If it's the former then it would seem that someone simply wouldn't be able to walk up the public chain.

@sbuss
Copy link
Owner

sbuss commented Sep 10, 2015

@shea256 The question of prime/not-prime defaults is separate from the caching bug you found:

First: the prime/not-prime (aka hardened/not-hardened) children:

The default derivation, when calling with w.get_child(0) is non-prime, following the BIP32 spec:

Each extended key has 231 normal child keys, and 231 hardened child keys. Each of these child keys has an index. The normal child keys use indices 0 through 231-1. The hardened child keys use indices 231 through 232-1.

You ask:

To clarify, are hardened keys the default here, or are unhardened keys?

If it's the latter, then:

  • this could result in privacy issues, as someone can go up the chain when they shouldn't be able to
  • this could result in the leak of the master for users who have shared their private key for a particular sub-account and assumed that the master private key was safe

Yes, that's unfortunately true, and those issues are present in BIP32. Bitmerchant's bip32.Wallet class exposes the low-level get_child method as well as the more friendly create_new_address_for_user. It's assumed that if you are calling the low-level get_child then you are familiar with the risks of using BIP32. The higher level create_new_address_for_user assumes you don't have any particularly deep knowledge about the spec and want it to Just Work TM.

It should be noted that, create_new_address_for_user defaults to giving you the non-prime child's public copy. So, while it uses non-hardened derivation, it doesn't contain the private key which leads to the leak of the parent's private key. This was due to the BIP32 spec - you can't create a public prime child of a public parent. Since the intended use case is "generate a pubkey for a user to pay into", that was the only option.

I will totally admit that none of this is clear, and results in behavior that is possibly dangerous, but that's the way the BIP is written.

I could deprecate this method and switch to something a little more explicit. What do you think of this:

  1. get_payment_address_for_user(uid) should return a non-prime child public copy, suitable for receiving a payment. Requires the parent's public key.
  2. get_wallet_for_user(uid), which should return a prime child private copy, suitable for giving to the user to control. Requires the parent's private key.

As for the reported bug:

The behavior you're seeing is due to bad memoization code (written by me, unfortunately). That did not properly calculate cache keys. Calling get_child in this order:

>>> wallet.get_child(0, is_prime=True)
>>> wallet.get_child(0, is_prime=False)

results in both children being prime. Yet, calling get_child in this order:

>>> wallet.get_child(0, is_prime=False)
>>> wallet.get_child(0, is_prime=True)

results in both children being non-prime.

I reiterate - this may seem serious, but the common use case is generating a non-prime public key for use as a payment address, so it's unlikely that users will have called both in quick succession and been exposed to the bug.

In fact, for this to be exploitable all of these must be true:

  1. Somebody figured this out in private before you reported it here
  2. You expose your master public key to the public
  3. You call get_child directly, and you give the public and private keys of child wallets to users
  4. You call get_child(n, is_prime=False) and get_child(n, is_prime=True)
    1. in that order
    2. in the same python process
    3. on the same wallet object
    4. with the intention of only giving the prime child to the user
  5. The attacker was malicious

If your use of bitmerchant doesn't exactly match this pattern, then you're not subject to stolen coins. It's possible that some coins have been misplaced, but I'll be writing recovery instructions tonight to release with the fix.

@sbuss sbuss mentioned this issue Sep 10, 2015
@sbuss sbuss closed this as completed in #52 Sep 10, 2015
@shea256
Copy link
Author

shea256 commented Sep 10, 2015

Awesome, thanks for putting in the fix!

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

Successfully merging a pull request may close this issue.

2 participants