PointyCastle / pointycastle

Moved into the Bouncy Castle project: https://github.com/bcgit/pc-dart
MIT License
270 stars 76 forks source link

Support lower-s form conversion for ECDSA signatures #215

Open cheeseandcereal opened 4 years ago

cheeseandcereal commented 4 years ago

Because of the prevalence of libsecp256k1 with bitcoin/ethereum, more and more people are using those libraries for ecdsa. When verifying signatures, libsecp256k1 enforces a signature's s to be in lower-s form to essentially help protect signature malleability.

See the comment in its include header for more info:

https://github.com/bitcoin-core/secp256k1/blob/d644dda5c9dbdecee52d1aa259235510fdc2d4ee/include/secp256k1.h#L483-L513

Because of this, I would propose a feature be added that either converts the s value of ECSignatures to their lower-s form automatically upon signature creation (if necessary), or at least provide a public api function to do so for easier compatibility with creating signatures that can be consistently verified by these libraries.

Here's some pseudocode that would do the latter option:

ECSignature normalizeECSignature(ECSignature currentSignature, ECDomainParameters curveParams) {
  BigInt normalizedS = currentSignature.s;
  if (normalizedS.compareTo(curveParams.n >> 1) > 0) {
    normalizedS = curveParams.n - normalizedS;
  }
  return ECSignature(currentSignature.r, normalizedS);
}
stevenroose commented 4 years ago

Sure, that makes sense. I would have a method on ECSignature called normalize() that just normalizes the signature so no new signature has to be created.

cheeseandcereal commented 4 years ago

An ECSignature doesn't contain data about its ECDomainParameters which are required for doing this calculation. Would a better alternative be providing an optional normalize boolean when actually creating the signature? Or should a normalize() function be added to ECSignature which simply requires taking in an ECDomainParameters object? (Or as a 3rd alternative, simply add a reference to the ECDomainParameters on the ECSignature object itself when creating it, so it can have that information already available to call a normalize() function with no additional params)

stevenroose commented 4 years ago

That's a good point. I would not add a ref to the signature for sure.

Of the other two options, it seems like we could do both. normalize(ECDomainParameters) seems fine, but a bool to normalize from the start seems better. They can both exist, though.

Btw, I'm about to move this project to a new repo, so keep your PR branch around in case you'd have to re-do.