From aa37ae9e748f69646b1ee2ad8b006361a4f99f95 Mon Sep 17 00:00:00 2001 From: John Kemp Date: Tue, 27 Jul 2021 14:14:05 -0400 Subject: [PATCH] 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 * 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 --- crypto/keys/internal/ecdsa/privkey.go | 70 ++++++++++++++++++- .../internal/ecdsa/privkey_internal_test.go | 38 +++++++++- crypto/keys/internal/ecdsa/pubkey.go | 29 +++++++- 3 files changed, 129 insertions(+), 8 deletions(-) diff --git a/crypto/keys/internal/ecdsa/privkey.go b/crypto/keys/internal/ecdsa/privkey.go index 0930285b04a1..088d85bf2f53 100644 --- a/crypto/keys/internal/ecdsa/privkey.go +++ b/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. diff --git a/crypto/keys/internal/ecdsa/privkey_internal_test.go b/crypto/keys/internal/ecdsa/privkey_internal_test.go index cee214dbd6db..1eae718f698f 100644 --- a/crypto/keys/internal/ecdsa/privkey_internal_test.go +++ b/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)) diff --git a/crypto/keys/internal/ecdsa/pubkey.go b/crypto/keys/internal/ecdsa/pubkey.go index 7961b93d3e21..75fbd8b8826d 100644 --- a/crypto/keys/internal/ecdsa/pubkey.go +++ b/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 }