From dfe73b7b10f1be3e1d8d999573c0057bc5a34cc3 Mon Sep 17 00:00:00 2001 From: "mergify[bot]" <37929162+mergify[bot]@users.noreply.github.com> Date: Tue, 27 Jul 2021 18:52:31 -0400 Subject: [PATCH] feat: Low-s normalization for ecdsa secp256r1 signing (#9738) (#9793) * added low-s normalization to ecdsa secp256r1 signing * go fmt fixes * removed else block as golint required * implement raw signature encoding for secp256r1 * move the creation of signature to after the check for sig string length * fake commit to re-run checks? (move the creation of signature to after the check for sig string length) * added a signature test for high s signature that requires sig validation to fail after the valid signature was mutated by extracting and scalar negating its s value * reordered code to prevent mutated message from being used in sig verify * added test for successful high_s signature with the ecdsa portion of the publicKey * Remove comment for self-explanatory code. Co-authored-by: Robert Zaremba * Missing quote Co-authored-by: Robert Zaremba * Apply minor suggestions from code review Co-authored-by: Robert Zaremba * normalize comments for godoc * refactored p256Order functions as private vars * Div -> Rsh optimizing time for division * resolve two code coverage issues; fix some small review issues * test using private signatureRaw function instead of copying code. Added tests to improve code coverage Co-authored-by: Aaron Craelius Co-authored-by: Robert Zaremba Co-authored-by: Aleksandr Bezobchuk (cherry picked from commit aa37ae9e748f69646b1ee2ad8b006361a4f99f95) Co-authored-by: John Kemp --- crypto/keys/internal/ecdsa/privkey.go | 14 ++++++++++++-- .../keys/internal/ecdsa/privkey_internal_test.go | 14 +++++++------- crypto/keys/internal/ecdsa/pubkey.go | 3 ++- 3 files changed, 21 insertions(+), 10 deletions(-) diff --git a/crypto/keys/internal/ecdsa/privkey.go b/crypto/keys/internal/ecdsa/privkey.go index 1d642ab9a498..088d85bf2f53 100644 --- a/crypto/keys/internal/ecdsa/privkey.go +++ b/crypto/keys/internal/ecdsa/privkey.go @@ -12,7 +12,13 @@ import ( // p256Order returns the curve order for the secp256r1 curve // NOTE: this is specific to the secp256r1/P256 curve, // and not taken from the domain params for the key itself -// (which would be a more generic approach for all EC). +// (which would be a more generic approach for all EC) +// In *here* we don't need to do it as a method on the key +// since this code is only called for secp256r1 +// if called on a key: +// func (sk PrivKey) pCurveOrder() *.big.Int { +// return sk.Curve.Params().N +// } var p256Order = elliptic.P256().Params().N // p256HalfOrder returns half the curve order @@ -29,6 +35,7 @@ func IsSNormalized(sigS *big.Int) bool { // NormalizeS will invert the s value if not already in the lower half // of curve order value func NormalizeS(sigS *big.Int) *big.Int { + if IsSNormalized(sigS) { return sigS } @@ -39,7 +46,8 @@ func NormalizeS(sigS *big.Int) *big.Int { // signatureRaw will serialize signature to R || S. // R, S are padded to 32 bytes respectively. // code roughly copied from secp256k1_nocgo.go -func signatureRaw(r, s *big.Int) []byte { +func signatureRaw(r *big.Int, s *big.Int) []byte { + rBytes := r.Bytes() sBytes := s.Bytes() sigBytes := make([]byte, 64) @@ -88,8 +96,10 @@ func (sk *PrivKey) Bytes() []byte { // It then raw encodes the signature as two fixed width 32-byte values // concatenated, reusing the code copied from secp256k1_nocgo.go func (sk *PrivKey) Sign(msg []byte) ([]byte, error) { + digest := sha256.Sum256(msg) r, s, err := ecdsa.Sign(rand.Reader, &sk.PrivateKey, digest[:]) + if err != nil { return nil, err } diff --git a/crypto/keys/internal/ecdsa/privkey_internal_test.go b/crypto/keys/internal/ecdsa/privkey_internal_test.go index 4dced3c98909..12bdaa45d726 100644 --- a/crypto/keys/internal/ecdsa/privkey_internal_test.go +++ b/crypto/keys/internal/ecdsa/privkey_internal_test.go @@ -4,10 +4,10 @@ import ( "crypto/ecdsa" "crypto/elliptic" "crypto/sha256" + "github.com/tendermint/tendermint/crypto" "math/big" "testing" - "github.com/cometbft/cometbft/crypto" "github.com/stretchr/testify/suite" ) @@ -71,17 +71,17 @@ func (suite *SKSuite) TestSign() { // extract the r, s values from sig r := new(big.Int).SetBytes(sig[:32]) - lowS := new(big.Int).SetBytes(sig[32:64]) + low_s := new(big.Int).SetBytes(sig[32:64]) // test that NormalizeS simply returns an already // normalized s - require.Equal(NormalizeS(lowS), lowS) + require.Equal(NormalizeS(low_s), low_s) // flip the s value into high order of curve P256 // leave r untouched! - highS := new(big.Int).Mod(new(big.Int).Neg(lowS), elliptic.P256().Params().N) + high_s := new(big.Int).Mod(new(big.Int).Neg(low_s), elliptic.P256().Params().N) - require.False(suite.pk.VerifySignature(msg, signatureRaw(r, highS))) + require.False(suite.pk.VerifySignature(msg, signatureRaw(r,high_s))) // Valid signature using low_s, but too long sigCpy = make([]byte, len(sig)+2) @@ -92,8 +92,8 @@ func (suite *SKSuite) TestSign() { // check whether msg can be verified with same key, and high_s // value using "regular" ecdsa signature - hash := sha256.Sum256(msg) - require.True(ecdsa.Verify(&suite.pk.PublicKey, hash[:], r, highS)) + hash := sha256.Sum256([]byte(msg)) + require.True(ecdsa.Verify(&suite.pk.PublicKey, hash[:], r, high_s)) // Mutate the message msg[1] ^= byte(2) diff --git a/crypto/keys/internal/ecdsa/pubkey.go b/crypto/keys/internal/ecdsa/pubkey.go index 1141baebec02..2743b4554c16 100644 --- a/crypto/keys/internal/ecdsa/pubkey.go +++ b/crypto/keys/internal/ecdsa/pubkey.go @@ -61,7 +61,8 @@ func (pk *PubKey) Bytes() []byte { // where the s integer component of the signature is in the // lower half of the curve order // 7/21/21 - expects raw encoded signature (fixed-width 64-bytes, R || S) -func (pk *PubKey) VerifySignature(msg, sig []byte) bool { +func (pk *PubKey) VerifySignature(msg []byte, sig []byte) bool { + // check length for raw signature // which is two 32-byte padded big.Ints // concatenated