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

* 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 <robert@zaremba.ch>

* Missing quote

Co-authored-by: Robert Zaremba <robert@zaremba.ch>

* Apply minor suggestions from code review

Co-authored-by: Robert Zaremba <robert@zaremba.ch>

* 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 <aaron@regen.network>
Co-authored-by: Robert Zaremba <robert@zaremba.ch>
Co-authored-by: Aleksandr Bezobchuk <alexanderbez@users.noreply.github.com>

crypto/keys/internal/ecdsa/privkey.go

@@ -9,7 +9,56 @@ import (
 	"math/big"
 )
 
-// GenPrivKey generates a new secp256r1 private key. It uses operating system randomness.
+// 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)
+// 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
+// a bit shift of 1 to the right (Rsh) is equivalent
+// to division by 2, only faster.
+var p256HalfOrder = new(big.Int).Rsh(p256Order, 1)
+
+// IsSNormalized returns true for the integer sigS if sigS falls in
+// lower half of the curve order
+func IsSNormalized(sigS *big.Int) bool {
+	return sigS.Cmp(p256HalfOrder) != 1
+}
+
+// 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
+	}
+
+	return new(big.Int).Sub(p256Order, sigS)
+}
+
+// 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 *big.Int, s *big.Int) []byte {
+
+	rBytes := r.Bytes()
+	sBytes := s.Bytes()
+	sigBytes := make([]byte, 64)
+	// 0 pad the byte arrays from the left if they aren't big enough.
+	copy(sigBytes[32-len(rBytes):32], rBytes)
+	copy(sigBytes[64-len(sBytes):64], sBytes)
+	return sigBytes
+}
+
+// GenPrivKey generates a new secp256r1 private key. It uses operating
+// system randomness.
 func GenPrivKey(curve elliptic.Curve) (PrivKey, error) {
 	key, err := ecdsa.GenerateKey(curve, rand.Reader)
 	if err != nil {
@@ -38,10 +87,25 @@ func (sk *PrivKey) Bytes() []byte {
 	return bz
 }
 
-// Sign hashes and signs the message usign ECDSA. Implements SDK PrivKey interface.
+// Sign hashes and signs the message using ECDSA. Implements SDK
+// PrivKey interface.
+// NOTE: this now calls the ecdsa Sign function
+// (not method!) directly as the s value of the signature is needed to
+// low-s normalize the signature value
+// See issue: https://github.com/cosmos/cosmos-sdk/issues/9723
+// 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)
-	return sk.PrivateKey.Sign(rand.Reader, digest[:], nil)
+	r, s, err := ecdsa.Sign(rand.Reader, &sk.PrivateKey, digest[:])
+
+	if err != nil {
+		return nil, err
+	}
+
+	normS := NormalizeS(s)
+	return signatureRaw(r, normS), nil
 }
 
 // String returns a string representation of the public key based on the curveName.

crypto/keys/internal/ecdsa/privkey_internal_test.go

@@ -1,9 +1,12 @@
 package ecdsa
 
 import (
-	"testing"
-
+	"crypto/ecdsa"
+	"crypto/elliptic"
+	"crypto/sha256"
 	"github.com/tendermint/tendermint/crypto"
+	"math/big"
+	"testing"
 
 	"github.com/stretchr/testify/suite"
 )
@@ -60,6 +63,37 @@ func (suite *SKSuite) TestSign() {
 		require.False(suite.pk.VerifySignature(msg, sigCpy))
 	}
 
+	// mutate the signature by scalar neg'ing the s value
+	// to give a high-s signature, valid ECDSA but should
+	// be invalid with Cosmos signatures.
+	// code mostly copied from privkey/pubkey.go
+
+	// extract the r, s values from sig
+	r := new(big.Int).SetBytes(sig[:32])
+	low_s := new(big.Int).SetBytes(sig[32:64])
+
+	// test that NormalizeS simply returns an already
+	// normalized s
+	require.Equal(NormalizeS(low_s), low_s)
+
+	// flip the s value into high order of curve P256
+	// leave r untouched!
+	high_s := new(big.Int).Mod(new(big.Int).Neg(low_s), elliptic.P256().Params().N)
+
+	require.False(suite.pk.VerifySignature(msg, signatureRaw(r,high_s)))
+
+	// Valid signature using low_s, but too long
+	sigCpy = make([]byte, len(sig)+2)
+	copy(sigCpy, sig)
+	sigCpy[65] = byte('A')
+
+	require.False(suite.pk.VerifySignature(msg, sigCpy))
+
+	// check whether msg can be verified with same key, and high_s
+	// value using "regular" ecdsa signature
+	hash := sha256.Sum256([]byte(msg))
+	require.True(ecdsa.Verify(&suite.pk.PublicKey, hash[:], r, high_s))
+
 	// Mutate the message
 	msg[1] ^= byte(2)
 	require.False(suite.pk.VerifySignature(msg, sig))

crypto/keys/internal/ecdsa/pubkey.go

@@ -4,7 +4,6 @@ import (
 	"crypto/ecdsa"
 	"crypto/elliptic"
 	"crypto/sha256"
-	"encoding/asn1"
 	"fmt"
 	"math/big"
 
@@ -14,6 +13,16 @@ import (
 	"github.com/cosmos/cosmos-sdk/types/errors"
 )
 
+// signatureFromBytes function roughly copied from secp256k1_nocgo.go
+// Read Signature struct from R || S. Caller needs to ensure that
+// len(sigStr) == 64.
+func signatureFromBytes(sigStr []byte) *signature {
+	return &signature{
+		R: new(big.Int).SetBytes(sigStr[:32]),
+		S: new(big.Int).SetBytes(sigStr[32:64]),
+	}
+}
+
 // signature holds the r and s values of an ECDSA signature.
 type signature struct {
 	R, S *big.Int
@@ -46,9 +55,23 @@ func (pk *PubKey) Bytes() []byte {
 }
 
 // VerifySignature checks if sig is a valid ECDSA signature for msg.
+// This includes checking for low-s normalized signatures
+// 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 []byte, sig []byte) bool {
-	s := new(signature)
-	if _, err := asn1.Unmarshal(sig, s); err != nil || s == nil {
+
+	// check length for raw signature
+	// which is two 32-byte padded big.Ints
+	// concatenated
+	// NOT DER!
+
+	if len(sig) != 64 {
+		return false
+	}
+
+	s := signatureFromBytes(sig)
+	if !IsSNormalized(s.S) {
 		return false
 	}