Skip to content

Commit

Permalink
Signer.Sender does not validate whole EDCSA signature parameters (0…
Browse files Browse the repository at this point in the history 
…xPolygon#1207)

* Validate V as big.NewInt

* Comments fix
  • Loading branch information
@goran-ethernal
goran-ethernal authored Feb 9, 2023
1 parent ce854b1 commit 8aba53f
Show file tree
Hide file tree
Showing 4 changed files with 53 additions and 44 deletions.
5 changes: 3 additions & 2 deletions crypto/crypto.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -26,6 +26,7 @@ var S256 = btcec.S256()
var (
secp256k1N, _ = new(big.Int).SetString("fffffffffffffffffffffffffffffffebaaedce6af48a03bbfd25e8cd0364141", 16)
secp256k1NHalf = new(big.Int).Div(secp256k1N, big.NewInt(2))
zero = big.NewInt(0)
one = big.NewInt(1)

ErrInvalidBLSSignature = errors.New("invalid BLS Signature")
Expand All @@ -51,7 +52,7 @@ type KeccakState interface {
}

// ValidateSignatureValues checks if the signature values are correct
func ValidateSignatureValues(v byte, r, s *big.Int, isHomestead bool) bool {
func ValidateSignatureValues(v, r, s *big.Int, isHomestead bool) bool {
// r & s must not be nil
if r == nil || s == nil {
return false
Expand All @@ -63,7 +64,7 @@ func ValidateSignatureValues(v byte, r, s *big.Int, isHomestead bool) bool {
}

// v must be 0 or 1
if v > 1 {
if v.Cmp(zero) == -1 || v.Cmp(one) == 1 {
return false
}

Expand Down
82 changes: 45 additions & 37 deletions crypto/crypto_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -153,127 +153,135 @@ func TestValidateSignatureValues(t *testing.T) {
cases := []struct {
homestead bool
name string
v byte
v *big.Int
r *big.Int
s *big.Int
res bool
}{
// correct v, r, s
{
name: "should be valid if v is 0 and r & s are in range",
homestead: true, v: 0, r: one, s: one, res: true,
homestead: true, v: zero, r: one, s: one, res: true,
},
{
name: "should be valid if v is 1 and r & s are in range",
homestead: true, v: 1, r: one, s: one, res: true,
homestead: true, v: one, r: one, s: one, res: true,
},
// incorrect v, correct r, s.
{
name: "should be invalid if v is out of range",
homestead: true, v: 2, r: one, s: one, res: false,
homestead: true, v: two, r: one, s: one, res: false,
},
{
name: "should be invalid if v is out of range",
homestead: true, v: big.NewInt(-10), r: one, s: one, res: false,
},
{
name: "should be invalid if v is out of range",
homestead: true, v: big.NewInt(10), r: one, s: one, res: false,
},
// incorrect v, incorrect/correct r, s.
{
name: "should be invalid if v & r & s are out of range",
homestead: true, v: 2, r: zero, s: zero, res: false,
homestead: true, v: two, r: zero, s: zero, res: false,
},
{
name: "should be invalid if v & r are out of range",
homestead: true, v: 2, r: zero, s: one, res: false,
homestead: true, v: two, r: zero, s: one, res: false,
},
{
name: "should be invalid if v & s are out of range",
homestead: true, v: 2, r: one, s: zero, res: false,
homestead: true, v: two, r: one, s: zero, res: false,
},
// correct v, incorrent r, s
{
name: "should be invalid if r & s are nil",
homestead: true, v: 0, r: nil, s: nil, res: false,
homestead: true, v: zero, r: nil, s: nil, res: false,
},
{
name: "should be invalid if r is nil",
homestead: true, v: 0, r: nil, s: one, res: false,
homestead: true, v: zero, r: nil, s: one, res: false,
},
{
name: "should be invalid if s is nil",
homestead: true, v: 0, r: one, s: nil, res: false,
homestead: true, v: zero, r: one, s: nil, res: false,
},
{
name: "should be invalid if r & s are negative",
homestead: true, v: 0, r: minusOne, s: minusOne, res: false,
homestead: true, v: zero, r: minusOne, s: minusOne, res: false,
},
{
name: "should be invalid if r is negative",
homestead: true, v: 0, r: minusOne, s: one, res: false,
homestead: true, v: zero, r: minusOne, s: one, res: false,
},
{
name: "should be invalid if s is negative",
homestead: true, v: 0, r: one, s: minusOne, res: false,
homestead: true, v: zero, r: one, s: minusOne, res: false,
},
{
name: "should be invalid if r & s are out of range",
homestead: true, v: 0, r: zero, s: zero, res: false,
homestead: true, v: zero, r: zero, s: zero, res: false,
},
{
name: "should be invalid if r is out of range (v = 0)",
homestead: true, v: 0, r: zero, s: one, res: false,
homestead: true, v: zero, r: zero, s: one, res: false,
},
{
name: "should be invalid if s is out of range (v = 0)",
homestead: true, v: 0, r: one, s: zero, res: false,
homestead: true, v: zero, r: one, s: zero, res: false,
},
{
name: "should be invalid if r & s are out of range (v = 1)",
homestead: true, v: 1, r: zero, s: zero, res: false,
homestead: true, v: one, r: zero, s: zero, res: false,
},
{
name: "should be invalid if r is out of range (v = 1)",
homestead: true, v: 1, r: zero, s: one, res: false,
homestead: true, v: one, r: zero, s: one, res: false,
},
{
name: "should be invalid if s is out of range (v = 1)",
homestead: true, v: 1, r: one, s: zero, res: false,
homestead: true, v: one, r: one, s: zero, res: false,
},
// incorrect r, s max limit (Frontier)
{
name: "should be invalid if r & s equal to secp256k1N in Frontier",
homestead: false, v: 0, r: limit, s: limit, res: false,
homestead: false, v: zero, r: limit, s: limit, res: false,
},
{
name: "should be invalid if r equals to secp256k1N in Frontier",
homestead: false, v: 0, r: limit, s: limitMinus1, res: false,
homestead: false, v: zero, r: limit, s: limitMinus1, res: false,
},
{
name: "should be invalid if s equals to secp256k1N in Frontier",
homestead: false, v: 0, r: limitMinus1, s: limit, res: false,
homestead: false, v: zero, r: limitMinus1, s: limit, res: false,
},
// incorrect r, s max limit (Homestead)
{
name: "should be invalid if r & s equal to secp256k1N in Homestead",
homestead: true, v: 0, r: limit, s: limit, res: false,
homestead: true, v: zero, r: limit, s: limit, res: false,
},
{
name: "should be invalid if r equals to secp256k1N in Homestead",
homestead: true, v: 0, r: limit, s: limitMinus1, res: false,
homestead: true, v: zero, r: limit, s: limitMinus1, res: false,
},
{
name: "should be invalid if s equals to secp256k1N in Homestead",
homestead: true, v: 0, r: limitMinus1, s: limit, res: false,
homestead: true, v: zero, r: limitMinus1, s: limit, res: false,
},
// frontier v, r, s max limit (Frontier)
{
name: "should be valid if r & s equal to secp256k1N - 1 in Frontier",
homestead: false, v: 0, r: limitMinus1, s: limitMinus1, res: true,
homestead: false, v: zero, r: limitMinus1, s: limitMinus1, res: true,
},
// incorrect v, r, s max limit (Homestead)
{
name: "should be invalid if r & s equal to secp256k1N - 1 in Homestead",
homestead: true, v: 0, r: limitMinus1, s: limitMinus1, res: false,
homestead: true, v: zero, r: limitMinus1, s: limitMinus1, res: false,
},
// correct v, r, s max limit (Homestead)
{
name: "should be valid if r equals to secp256k1N - 1 and s equals to secp256k1N/2",
homestead: true, v: 0, r: limitMinus1, s: halfLimit, res: true,
homestead: true, v: zero, r: limitMinus1, s: halfLimit, res: true,
},
// edge cases
// Previously ValidateSignatureValues uses bytes.Compare to compare r and s with upper limit
Expand All @@ -283,39 +291,39 @@ func TestValidateSignatureValues(t *testing.T) {
// > 0x01fffffffffffffffffffffffffffffffebaaedce6af48a03bbfd25e8cd0364141 (double value)
{
name: "should be invalid if r & s equal to 2 * secp256k1N in Frontier",
homestead: false, v: 0, r: doubleLimit, s: doubleLimit, res: false,
homestead: false, v: zero, r: doubleLimit, s: doubleLimit, res: false,
},
{
name: "should be invalid if r equals to 2 * secp256k1N in Frontier",
homestead: false, v: 0, r: doubleLimit, s: one, res: false,
homestead: false, v: zero, r: doubleLimit, s: one, res: false,
},
{
name: "should be invalid if s equals to 2 * secp256k1N in Frontier",
homestead: false, v: 0, r: one, s: doubleLimit, res: false,
homestead: false, v: zero, r: one, s: doubleLimit, res: false,
},
{
name: "should be invalid if r equals to 2 * secp256k1N and s equal to secp256k1N",
homestead: true, v: 0, r: doubleLimit, s: limit, res: false,
homestead: true, v: zero, r: doubleLimit, s: limit, res: false,
},
{
name: "should be invalid if r equals to 2 * secp256k1N in Frontier",
homestead: true, v: 0, r: doubleLimit, s: one, res: false,
homestead: true, v: zero, r: doubleLimit, s: one, res: false,
},
{
name: "should be invalid if s equals to secp256k1N in Frontier",
homestead: true, v: 0, r: one, s: limit, res: false,
homestead: true, v: zero, r: one, s: limit, res: false,
},
{
name: "should be valid if r & s equal to small value",
homestead: false, v: 0, r: smallValue, s: smallValue, res: true,
homestead: false, v: zero, r: smallValue, s: smallValue, res: true,
},
{
name: "should be valid if r equals to smallValue in Frontier",
homestead: false, v: 0, r: smallValue, s: one, res: true,
homestead: false, v: zero, r: smallValue, s: one, res: true,
},
{
name: "should be valid if s equals to smallValue in Frontier",
homestead: false, v: 0, r: one, s: smallValue, res: true,
homestead: false, v: zero, r: one, s: smallValue, res: true,
},
}

Expand Down
8 changes: 4 additions & 4 deletions crypto/txsigner.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -98,7 +98,7 @@ func (f *FrontierSigner) Sender(tx *types.Transaction) (types.Address, error) {

refV.Sub(refV, big27)

sig, err := encodeSignature(tx.R, tx.S, byte(refV.Int64()), f.isHomestead)
sig, err := encodeSignature(tx.R, tx.S, refV, f.isHomestead)
if err != nil {
return types.Address{}, err
}
Expand Down Expand Up @@ -181,7 +181,7 @@ func (e *EIP155Signer) Sender(tx *types.Transaction) (types.Address, error) {
bigV.Sub(bigV, mulOperand)
bigV.Sub(bigV, big35)

sig, err := encodeSignature(tx.R, tx.S, byte(bigV.Int64()), e.isHomestead)
sig, err := encodeSignature(tx.R, tx.S, bigV, e.isHomestead)
if err != nil {
return types.Address{}, err
}
Expand Down Expand Up @@ -230,15 +230,15 @@ func (e *EIP155Signer) CalculateV(parity byte) []byte {
}

// encodeSignature generates a signature value based on the R, S and V value
func encodeSignature(R, S *big.Int, V byte, isHomestead bool) ([]byte, error) {
func encodeSignature(R, S, V *big.Int, isHomestead bool) ([]byte, error) {
if !ValidateSignatureValues(V, R, S, isHomestead) {
return nil, fmt.Errorf("invalid txn signature")
}

sig := make([]byte, 65)
copy(sig[32-len(R.Bytes()):32], R.Bytes())
copy(sig[64-len(S.Bytes()):64], S.Bytes())
sig[64] = V
sig[64] = byte(V.Int64()) // here is safe to convert it since ValidateSignatureValues will validate the v value

return sig, nil
}
2 changes: 1 addition & 1 deletion state/runtime/precompiled/base.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -35,7 +35,7 @@ func (e *ecrecover) run(input []byte, caller types.Address, _ runtime.Host) ([]b
r := big.NewInt(0).SetBytes(input[64:96])
s := big.NewInt(0).SetBytes(input[96:128])

if !crypto.ValidateSignatureValues(v, r, s, false) {
if !crypto.ValidateSignatureValues(new(big.Int).SetBytes([]byte{v}), r, s, false) {
return nil, nil
}

Expand Down

0 comments on commit 8aba53f

Please sign in to comment.