cryptonomex / graphene

MIT License
1.05k stars 337 forks source link

Review use of libsecp256k1 contexts #131

Closed pmconrad closed 7 years ago

pmconrad commented 9 years ago

I stumbled across this https://github.com/bitcoin/secp256k1/pull/254#issuecomment-118074138 :

That said, your question makes it clear that the intended use isn't clear. That should definitely be clarified in the header file and perhaps a README. I guess it should list this:

  • Do not create/destroy contexts for each call. Their creation is far slower than any library call, and part of their purpose is to reuse precomputed values across calls.
  • If you perform signing, you should call context_randomize after initialization of the context, and perhaps on a regularly basis (daily?).
  • When calling context_randomize pass in external entropy (perhaps from the operating system, perhaps from user input).
  • You are responsible for locking. After initialization, only context_randomize requires exclusive locked, but the caller is responsible for not calling any other functions that use the context during randomization.
  • If you do both validation and signing, you can either use a shared context for both, or separate contexts. One reason for using separate contexts if that for signing, calling context_randomize requires an exclusive lock on the context. If you need to do validation at the same time, it may be useful to separate them, so validation is not blocked by context randomization.

Apparently we need to call context_randomize regularly (for signing), with proper synchronization.

bytemaster commented 9 years ago

This deserves some major brownie pts, pmconrad, please post a BTS account name.

pmconrad commented 9 years ago

Well, since I coded some of it I don't feel like I really deserve it, but... please send them to "pmc", and thanks!

arhag commented 9 years ago

Wait, libsecp256k1 supports deterministic signing.

Derandomized DSA (via RFC6979 or with a caller provided function.)

Here is RFC6979 by the way: https://tools.ietf.org/html/rfc6979

So is that extra entropy necessary at all or is it just an optional bonus? I don't understand why context_randomize should be necessary at all?

Edit: Apparently, this (https://github.com/bitcoin/secp256k1/commit/d2275795ff22a6f4738869f5528fbbb61738aa48) is why it is needed. It is to "reduce exposure to potential power/EMI sidechannels". Crazy stuff. Of course, it isn't a threat at all if the attacker isn't physically near the signing device.

theoreticalbts commented 8 years ago

@bytemaster what is the status of this ticket? Is this a real problem, or can I close it?

vikramrajkumar commented 7 years ago

This issue was moved to bitshares/bitshares-2#36