feat: Low-s normalization for ecdsa secp256r1 signing (cosmos#9738) (c…

…osmos#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 <[email protected]>

* Missing quote

Co-authored-by: Robert Zaremba <[email protected]>

* Apply minor suggestions from code review

Co-authored-by: Robert Zaremba <[email protected]>

* 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 <[email protected]>
Co-authored-by: Robert Zaremba <[email protected]>
Co-authored-by: Aleksandr Bezobchuk <[email protected]>
(cherry picked from commit aa37ae9)

Co-authored-by: John Kemp <[email protected]>

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
 	}

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)

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