From 709cb157fb74b9e0acea7f9f0f5edd22a25217e2 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?P=C3=A9ter=20Szil=C3=A1gyi?= Date: Mon, 19 Dec 2016 11:10:26 +0200 Subject: [PATCH] core, crypto: use canonical signature validation --- core/types/transaction.go | 4 ++-- core/types/transaction_signing.go | 13 +++++------ core/vm/contracts.go | 14 ++++-------- crypto/crypto.go | 13 ++++------- crypto/crypto_test.go | 38 +++++++++++++++---------------- 5 files changed, 35 insertions(+), 47 deletions(-) diff --git a/core/types/transaction.go b/core/types/transaction.go index 6baab42961..87b54ab30b 100644 --- a/core/types/transaction.go +++ b/core/types/transaction.go @@ -199,9 +199,9 @@ func (tx *Transaction) UnmarshalJSON(input []byte) error { var V byte if isProtectedV((*big.Int)(dec.V)) { - V = byte((new(big.Int).Sub((*big.Int)(dec.V), deriveChainId((*big.Int)(dec.V))).Uint64()) - 35 + 27) + V = byte((new(big.Int).Sub((*big.Int)(dec.V), deriveChainId((*big.Int)(dec.V))).Uint64()) - 35) } else { - V = byte(((*big.Int)(dec.V)).Uint64()) + V = byte(((*big.Int)(dec.V)).Uint64() - 27) } if !crypto.ValidateSignatureValues(V, (*big.Int)(dec.R), (*big.Int)(dec.S), false) { return ErrInvalidSig diff --git a/core/types/transaction_signing.go b/core/types/transaction_signing.go index caae15fb5b..f991667576 100644 --- a/core/types/transaction_signing.go +++ b/core/types/transaction_signing.go @@ -138,17 +138,16 @@ func (s EIP155Signer) PublicKey(tx *Transaction) ([]byte, error) { return nil, ErrInvalidChainId } - V := byte((new(big.Int).Sub(tx.data.V, s.chainIdMul).Uint64()) - 35 + 27) + V := byte(new(big.Int).Sub(tx.data.V, s.chainIdMul).Uint64() - 35) if !crypto.ValidateSignatureValues(V, tx.data.R, tx.data.S, true) { return nil, ErrInvalidSig } - // encode the signature in uncompressed format R, S := tx.data.R.Bytes(), tx.data.S.Bytes() sig := make([]byte, 65) copy(sig[32-len(R):32], R) copy(sig[64-len(S):64], S) - sig[64] = V - 27 + sig[64] = V // recover the public key from the signature hash := s.Hash(tx) @@ -238,7 +237,7 @@ func (hs HomesteadSigner) PublicKey(tx *Transaction) ([]byte, error) { if tx.data.V.BitLen() > 8 { return nil, ErrInvalidSig } - V := byte(tx.data.V.Uint64()) + V := byte(tx.data.V.Uint64() - 27) if !crypto.ValidateSignatureValues(V, tx.data.R, tx.data.S, true) { return nil, ErrInvalidSig } @@ -247,7 +246,7 @@ func (hs HomesteadSigner) PublicKey(tx *Transaction) ([]byte, error) { sig := make([]byte, 65) copy(sig[32-len(r):32], r) copy(sig[64-len(s):64], s) - sig[64] = V - 27 + sig[64] = V // recover the public key from the snature hash := hs.Hash(tx) @@ -308,7 +307,7 @@ func (fs FrontierSigner) PublicKey(tx *Transaction) ([]byte, error) { return nil, ErrInvalidSig } - V := byte(tx.data.V.Uint64()) + V := byte(tx.data.V.Uint64() - 27) if !crypto.ValidateSignatureValues(V, tx.data.R, tx.data.S, false) { return nil, ErrInvalidSig } @@ -317,7 +316,7 @@ func (fs FrontierSigner) PublicKey(tx *Transaction) ([]byte, error) { sig := make([]byte, 65) copy(sig[32-len(r):32], r) copy(sig[64-len(s):64], s) - sig[64] = V - 27 + sig[64] = V // recover the public key from the snature hash := fs.Hash(tx) diff --git a/core/vm/contracts.go b/core/vm/contracts.go index b45f14724b..5cd0d0bb15 100644 --- a/core/vm/contracts.go +++ b/core/vm/contracts.go @@ -89,21 +89,15 @@ func ecrecoverFunc(in []byte) []byte { r := common.BytesToBig(in[64:96]) s := common.BytesToBig(in[96:128]) - // Treat V as a 256bit integer - vbig := common.Bytes2Big(in[32:64]) - v := byte(vbig.Uint64()) + v := in[63] - 27 // tighter sig s values in homestead only apply to tx sigs - if !crypto.ValidateSignatureValues(v, r, s, false) { + if common.Bytes2Big(in[32:63]).BitLen() > 0 || !crypto.ValidateSignatureValues(v, r, s, false) { glog.V(logger.Detail).Infof("ECRECOVER error: v, r or s value invalid") return nil } - - // v needs to be at the end and normalized for libsecp256k1 - vbignormal := new(big.Int).Sub(vbig, big.NewInt(27)) - vnormal := byte(vbignormal.Uint64()) - rsv := append(in[64:128], vnormal) - pubKey, err := crypto.Ecrecover(in[:32], rsv) + // v needs to be at the end for libsecp256k1 + pubKey, err := crypto.Ecrecover(in[:32], append(in[64:128], v)) // make sure the public key is a valid one if err != nil { glog.V(logger.Detail).Infoln("ECRECOVER error: ", err) diff --git a/crypto/crypto.go b/crypto/crypto.go index bf4e5fe054..25aec0836f 100644 --- a/crypto/crypto.go +++ b/crypto/crypto.go @@ -167,25 +167,20 @@ func GenerateKey() (*ecdsa.PrivateKey, error) { return ecdsa.GenerateKey(secp256k1.S256(), rand.Reader) } +// ValidateSignatureValues verifies whether the signature values are valid with +// the given chain rules. The v value is assumed to be the canonical secp256k1 +// value of either 0 or 1. func ValidateSignatureValues(v byte, r, s *big.Int, homestead bool) bool { if r.Cmp(common.Big1) < 0 || s.Cmp(common.Big1) < 0 { return false } - vint := uint32(v) // reject upper range of s values (ECDSA malleability) // see discussion in secp256k1/libsecp256k1/include/secp256k1.h if homestead && s.Cmp(secp256k1.HalfN) > 0 { return false } // Frontier: allow s to be in full N range - if s.Cmp(secp256k1.N) >= 0 { - return false - } - if r.Cmp(secp256k1.N) < 0 && (vint == 27 || vint == 28) { - return true - } else { - return false - } + return r.Cmp(secp256k1.N) < 0 && s.Cmp(secp256k1.N) < 0 && (v == 0 || v == 1) } func SigToPub(hash, sig []byte) (*ecdsa.PublicKey, error) { diff --git a/crypto/crypto_test.go b/crypto/crypto_test.go index e3b4a6d09a..86a5823064 100644 --- a/crypto/crypto_test.go +++ b/crypto/crypto_test.go @@ -181,38 +181,38 @@ func TestValidateSignatureValues(t *testing.T) { secp256k1nMinus1 := new(big.Int).Sub(secp256k1.N, common.Big1) // correct v,r,s - check(true, 27, one, one) - check(true, 28, one, one) + check(true, 0, one, one) + check(true, 1, one, one) // incorrect v, correct r,s, - check(false, 30, one, one) - check(false, 26, one, one) + check(false, 2, one, one) + check(false, 3, one, one) // incorrect v, combinations of incorrect/correct r,s at lower limit + check(false, 2, zero, zero) + check(false, 2, zero, one) + check(false, 2, one, zero) + check(false, 2, one, one) + + // correct v for any combination of incorrect r,s check(false, 0, zero, zero) check(false, 0, zero, one) check(false, 0, one, zero) - check(false, 0, one, one) - // correct v for any combination of incorrect r,s - check(false, 27, zero, zero) - check(false, 27, zero, one) - check(false, 27, one, zero) - - check(false, 28, zero, zero) - check(false, 28, zero, one) - check(false, 28, one, zero) + check(false, 1, zero, zero) + check(false, 1, zero, one) + check(false, 1, one, zero) // correct sig with max r,s - check(true, 27, secp256k1nMinus1, secp256k1nMinus1) + check(true, 0, secp256k1nMinus1, secp256k1nMinus1) // correct v, combinations of incorrect r,s at upper limit - check(false, 27, secp256k1.N, secp256k1nMinus1) - check(false, 27, secp256k1nMinus1, secp256k1.N) - check(false, 27, secp256k1.N, secp256k1.N) + check(false, 0, secp256k1.N, secp256k1nMinus1) + check(false, 0, secp256k1nMinus1, secp256k1.N) + check(false, 0, secp256k1.N, secp256k1.N) // current callers ensures r,s cannot be negative, but let's test for that too // as crypto package could be used stand-alone - check(false, 27, minusOne, one) - check(false, 27, one, minusOne) + check(false, 0, minusOne, one) + check(false, 0, one, minusOne) } func checkhash(t *testing.T, name string, f func([]byte) []byte, msg, exp []byte) {