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 <[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)

# Conflicts:
#	crypto/keys/internal/ecdsa/privkey.go
#	crypto/keys/internal/ecdsa/privkey_internal_test.go
#	crypto/keys/internal/ecdsa/pubkey.go

crypto/keys/internal/ecdsa/privkey.go

@@ -0,0 +1,133 @@
+package ecdsa
+
+import (
+	"crypto/ecdsa"
+	"crypto/elliptic"
+	"crypto/rand"
+	"crypto/sha256"
+	"fmt"
+	"math/big"
+)
+
+// 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 {
+		return PrivKey{}, err
+	}
+	return PrivKey{*key}, nil
+}
+
+type PrivKey struct {
+	ecdsa.PrivateKey
+}
+
+// PubKey returns ECDSA public key associated with this private key.
+func (sk *PrivKey) PubKey() PubKey {
+	return PubKey{sk.PublicKey, nil}
+}
+
+// Bytes serialize the private key using big-endian.
+func (sk *PrivKey) Bytes() []byte {
+	if sk == nil {
+		return nil
+	}
+	fieldSize := (sk.Curve.Params().BitSize + 7) / 8
+	bz := make([]byte, fieldSize)
+	sk.D.FillBytes(bz)
+	return bz
+}
+
+// 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)
+	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.
+func (sk *PrivKey) String(name string) string {
+	return name + "{-}"
+}
+
+// MarshalTo implements proto.Marshaler interface.
+func (sk *PrivKey) MarshalTo(dAtA []byte) (int, error) {
+	bz := sk.Bytes()
+	copy(dAtA, bz)
+	return len(bz), nil
+}
+
+// Unmarshal implements proto.Marshaler interface.
+func (sk *PrivKey) Unmarshal(bz []byte, curve elliptic.Curve, expectedSize int) error {
+	if len(bz) != expectedSize {
+		return fmt.Errorf("wrong ECDSA SK bytes, expecting %d bytes", expectedSize)
+	}
+
+	sk.Curve = curve
+	sk.D = new(big.Int).SetBytes(bz)
+	sk.X, sk.Y = curve.ScalarBaseMult(bz)
+	return nil
+}

crypto/keys/internal/ecdsa/privkey_internal_test.go

@@ -0,0 +1,100 @@
+package ecdsa
+
+import (
+	"crypto/ecdsa"
+	"crypto/elliptic"
+	"crypto/sha256"
+	"github.com/tendermint/tendermint/crypto"
+	"math/big"
+	"testing"
+
+	"github.com/stretchr/testify/suite"
+)
+
+func TestSKSuite(t *testing.T) {
+	suite.Run(t, new(SKSuite))
+}
+
+type SKSuite struct{ CommonSuite }
+
+func (suite *SKSuite) TestString() {
+	const prefix = "abc"
+	suite.Require().Equal(prefix+"{-}", suite.sk.String(prefix))
+}
+
+func (suite *SKSuite) TestPubKey() {
+	pk := suite.sk.PubKey()
+	suite.True(suite.sk.PublicKey.Equal(&pk.PublicKey))
+}
+
+func (suite *SKSuite) TestBytes() {
+	bz := suite.sk.Bytes()
+	suite.Len(bz, 32)
+	var sk *PrivKey
+	suite.Nil(sk.Bytes())
+}
+
+func (suite *SKSuite) TestMarshal() {
+	require := suite.Require()
+	const size = 32
+
+	var buffer = make([]byte, size)
+	suite.sk.MarshalTo(buffer)
+
+	var sk = new(PrivKey)
+	err := sk.Unmarshal(buffer, secp256r1, size)
+	require.NoError(err)
+	require.True(sk.Equal(&suite.sk.PrivateKey))
+}
+
+func (suite *SKSuite) TestSign() {
+	require := suite.Require()
+
+	msg := crypto.CRandBytes(1000)
+	sig, err := suite.sk.Sign(msg)
+	require.NoError(err)
+	sigCpy := make([]byte, len(sig))
+	copy(sigCpy, sig)
+	require.True(suite.pk.VerifySignature(msg, sigCpy))
+
+	// Mutate the signature
+	for i := range sig {
+		sigCpy[i] ^= byte(i + 1)
+		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

@@ -0,0 +1,108 @@
+package ecdsa
+
+import (
+	"crypto/ecdsa"
+	"crypto/elliptic"
+	"crypto/sha256"
+	"fmt"
+	"math/big"
+
+	tmcrypto "github.com/tendermint/tendermint/crypto"
+
+	"github.com/cosmos/cosmos-sdk/types/address"
+	"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
+}
+
+type PubKey struct {
+	ecdsa.PublicKey
+
+	// cache
+	address tmcrypto.Address
+}
+
+// Address gets the address associated with a pubkey. If no address exists, it returns a newly created ADR-28 address
+// for ECDSA keys.
+// protoName is a concrete proto structure id.
+func (pk *PubKey) Address(protoName string) tmcrypto.Address {
+	if pk.address == nil {
+		pk.address = address.Hash(protoName, pk.Bytes())
+	}
+	return pk.address
+}
+
+// Bytes returns the byte representation of the public key using a compressed form
+// specified in section 4.3.6 of ANSI X9.62 with first byte being the curve type.
+func (pk *PubKey) Bytes() []byte {
+	if pk == nil {
+		return nil
+	}
+	return elliptic.MarshalCompressed(pk.Curve, pk.X, pk.Y)
+}
+
+// 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 {
+
+	// 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
+	}
+
+	h := sha256.Sum256(msg)
+	return ecdsa.Verify(&pk.PublicKey, h[:], s.R, s.S)
+}
+
+// String returns a string representation of the public key based on the curveName.
+func (pk *PubKey) String(curveName string) string {
+	return fmt.Sprintf("%s{%X}", curveName, pk.Bytes())
+}
+
+// **** Proto Marshaler ****
+
+// MarshalTo implements proto.Marshaler interface.
+func (pk *PubKey) MarshalTo(dAtA []byte) (int, error) {
+	bz := pk.Bytes()
+	copy(dAtA, bz)
+	return len(bz), nil
+}
+
+// Unmarshal implements proto.Marshaler interface.
+func (pk *PubKey) Unmarshal(bz []byte, curve elliptic.Curve, expectedSize int) error {
+	if len(bz) != expectedSize {
+		return errors.Wrapf(errors.ErrInvalidPubKey, "wrong ECDSA PK bytes, expecting %d bytes, got %d", expectedSize, len(bz))
+	}
+	cpk := ecdsa.PublicKey{Curve: curve}
+	cpk.X, cpk.Y = elliptic.UnmarshalCompressed(curve, bz)
+	if cpk.X == nil || cpk.Y == nil {
+		return errors.Wrapf(errors.ErrInvalidPubKey, "wrong ECDSA PK bytes, unknown curve type: %d", bz[0])
+	}
+	pk.PublicKey = cpk
+	return nil
+}