Skip to content

Commit aa26659

Browse files
brendanjryanbrendanjryan
authored
Merge pull request #3 from tempoxyz/brendanryan-safe-encode
adds guards for overflowing r/s values
2 parents 5cebb3f + 3860f40 commit aa26659

File tree

5 files changed

+245
-25
lines changed

5 files changed

+245
-25
lines changed

‎pkg/signer/errors.go‎

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -6,4 +6,7 @@ import "errors"
66
var (
77
// ErrInvalidPrivateKey is returned when a private key cannot be decoded or parsed.
88
ErrInvalidPrivateKey = errors.New("invalid private key")
9+
10+
// ErrInvalidSignature is returned when a signature has invalid components.
11+
ErrInvalidSignature = errors.New("invalid signature")
912
)

‎pkg/signer/signer.go‎

Lines changed: 20 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -81,8 +81,28 @@ func (s *Signer) SignData(data []byte) (*Signature, error) {
8181
return s.Sign(hash)
8282
}
8383

84+
// maxScalarBytes is the maximum byte length for secp256k1 scalar values (R and S).
85+
// Scalars must fit within 32 bytes (256 bits) to be valid signature components.
86+
const maxScalarBytes = 32
87+
8488
// RecoverAddress recovers the address that signed the given hash with the given signature.
8589
func RecoverAddress(hash common.Hash, sig *Signature) (common.Address, error) {
90+
if sig == nil {
91+
return common.Address{}, fmt.Errorf("%w: signature is nil", ErrInvalidSignature)
92+
}
93+
if sig.R == nil || sig.S == nil {
94+
return common.Address{}, fmt.Errorf("%w: R or S is nil", ErrInvalidSignature)
95+
}
96+
97+
rBytes := sig.R.Bytes()
98+
if len(rBytes) > maxScalarBytes {
99+
return common.Address{}, fmt.Errorf("%w: R exceeds %d bytes (got %d)", ErrInvalidSignature, maxScalarBytes, len(rBytes))
100+
}
101+
sBytes := sig.S.Bytes()
102+
if len(sBytes) > maxScalarBytes {
103+
return common.Address{}, fmt.Errorf("%w: S exceeds %d bytes (got %d)", ErrInvalidSignature, maxScalarBytes, len(sBytes))
104+
}
105+
86106
sigBytes := make([]byte, 65)
87107
sig.R.FillBytes(sigBytes[0:32])
88108
sig.S.FillBytes(sigBytes[32:64])

‎pkg/signer/signer_test.go‎

Lines changed: 70 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1,8 +1,11 @@
11
package signer
22

33
import (
4+
"errors"
5+
"math/big"
46
"testing"
57

8+
"github.com/ethereum/go-ethereum/common"
69
"github.com/ethereum/go-ethereum/crypto"
710
"github.com/stretchr/testify/assert"
811
)
@@ -114,3 +117,70 @@ func TestRecoverAddress(t *testing.T) {
114117

115118
assert.Equal(t, sgn.Address(), recoveredAddr)
116119
}
120+
121+
func TestRecoverAddress_InvalidSignatures(t *testing.T) {
122+
big33Bytes := new(big.Int).Lsh(big.NewInt(1), 256) // 2^256 requires 33 bytes
123+
124+
tests := []struct {
125+
name string
126+
sig *Signature
127+
wantErr bool
128+
wantErrStr string
129+
}{
130+
{
131+
name: "nil signature",
132+
sig: nil,
133+
wantErr: true,
134+
wantErrStr: "signature is nil",
135+
},
136+
{
137+
name: "nil R",
138+
sig: &Signature{R: nil, S: big.NewInt(1), YParity: 0},
139+
wantErr: true,
140+
wantErrStr: "nil",
141+
},
142+
{
143+
name: "nil S",
144+
sig: &Signature{R: big.NewInt(1), S: nil, YParity: 0},
145+
wantErr: true,
146+
wantErrStr: "nil",
147+
},
148+
{
149+
name: "nil R and S",
150+
sig: &Signature{R: nil, S: nil, YParity: 0},
151+
wantErr: true,
152+
wantErrStr: "nil",
153+
},
154+
{
155+
name: "oversized R (33 bytes)",
156+
sig: &Signature{R: big33Bytes, S: big.NewInt(1), YParity: 0},
157+
wantErr: true,
158+
wantErrStr: "R exceeds",
159+
},
160+
{
161+
name: "oversized S (33 bytes)",
162+
sig: &Signature{R: big.NewInt(1), S: big33Bytes, YParity: 0},
163+
wantErr: true,
164+
wantErrStr: "S exceeds",
165+
},
166+
{
167+
name: "oversized R and S",
168+
sig: &Signature{R: big33Bytes, S: big33Bytes, YParity: 0},
169+
wantErr: true,
170+
wantErrStr: "R exceeds",
171+
},
172+
}
173+
174+
for _, tt := range tests {
175+
t.Run(tt.name, func(t *testing.T) {
176+
_, err := RecoverAddress(common.Hash{}, tt.sig)
177+
if tt.wantErr {
178+
assert.Error(t, err)
179+
assert.True(t, errors.Is(err, ErrInvalidSignature))
180+
assert.Contains(t, err.Error(), tt.wantErrStr)
181+
} else {
182+
assert.NoError(t, err)
183+
}
184+
})
185+
}
186+
}

‎pkg/transaction/deserialize.go‎

Lines changed: 13 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -269,6 +269,10 @@ func decodeAccessList(accessListRaw []interface{}) (AccessList, error) {
269269
return accessList, nil
270270
}
271271

272+
// maxSignatureScalarBytes is the maximum byte length for secp256k1 signature scalars (R and S).
273+
// Valid signature components must fit within 32 bytes (256 bits).
274+
const maxSignatureScalarBytes = 32
275+
272276
// decodeSignature decodes a signature tuple [yParity, r, s].
273277
func decodeSignature(sigTuple []interface{}) (*signer.Signature, error) {
274278
if len(sigTuple) != 3 {
@@ -294,13 +298,22 @@ func decodeSignature(sigTuple []interface{}) (*signer.Signature, error) {
294298
if !ok {
295299
return nil, fmt.Errorf("r is not bytes")
296300
}
301+
// Validate R size to prevent DoS via oversized signature components.
302+
// Oversized values would cause a panic in RecoverAddress when using FillBytes.
303+
if len(rBytes) > maxSignatureScalarBytes {
304+
return nil, fmt.Errorf("r exceeds maximum size: got %d bytes, max %d", len(rBytes), maxSignatureScalarBytes)
305+
}
297306
r := new(big.Int).SetBytes(rBytes)
298307

299308
// Field 2: s
300309
sBytes, ok := sigTuple[2].([]byte)
301310
if !ok {
302311
return nil, fmt.Errorf("s is not bytes")
303312
}
313+
// Validate S size to prevent DoS via oversized signature components.
314+
if len(sBytes) > maxSignatureScalarBytes {
315+
return nil, fmt.Errorf("s exceeds maximum size: got %d bytes, max %d", len(sBytes), maxSignatureScalarBytes)
316+
}
304317
s := new(big.Int).SetBytes(sBytes)
305318

306319
return signer.NewSignature(r, s, yParity), nil

‎pkg/transaction/serialization_test.go‎

Lines changed: 139 additions & 25 deletions
Original file line numberDiff line numberDiff line change
@@ -1,11 +1,13 @@
11
package transaction
22

33
import (
4+
"encoding/hex"
45
"math/big"
56
"strings"
67
"testing"
78

89
"github.com/ethereum/go-ethereum/common"
10+
"github.com/ethereum/go-ethereum/rlp"
911
"github.com/google/go-cmp/cmp"
1012
"github.com/stretchr/testify/assert"
1113
"github.com/tempoxyz/tempo-go/pkg/signer"
@@ -331,45 +333,84 @@ func TestDecodeAccessList(t *testing.T) {
331333
}
332334

333335
func TestDecodeSignature(t *testing.T) {
336+
big33Bytes := new(big.Int).Lsh(big.NewInt(1), 256)
337+
max32Bytes := new(big.Int).Sub(new(big.Int).Lsh(big.NewInt(1), 256), big.NewInt(1))
338+
334339
tests := []struct {
335-
name string
336-
input []interface{}
337-
want *signer.Signature
338-
wantErr bool
340+
name string
341+
r []byte
342+
s []byte
343+
yParity byte
344+
want *signer.Signature
345+
wantErr bool
346+
wantErrStr string
339347
}{
340348
{
341-
name: "valid signature",
342-
input: []interface{}{
343-
[]byte{0},
344-
big.NewInt(12345).Bytes(),
345-
big.NewInt(67890).Bytes(),
346-
},
347-
want: signer.NewSignature(big.NewInt(12345), big.NewInt(67890), 0),
349+
name: "valid signature",
350+
r: big.NewInt(12345).Bytes(),
351+
s: big.NewInt(67890).Bytes(),
352+
yParity: 0,
353+
want: signer.NewSignature(big.NewInt(12345), big.NewInt(67890), 0),
348354
},
349355
{
350-
name: "signature with yParity = 1",
351-
input: []interface{}{
352-
[]byte{1},
353-
big.NewInt(12345).Bytes(),
354-
big.NewInt(67890).Bytes(),
355-
},
356-
want: signer.NewSignature(big.NewInt(12345), big.NewInt(67890), 1),
356+
name: "signature with yParity = 1",
357+
r: big.NewInt(12345).Bytes(),
358+
s: big.NewInt(67890).Bytes(),
359+
yParity: 1,
360+
want: signer.NewSignature(big.NewInt(12345), big.NewInt(67890), 1),
357361
},
358362
{
359-
name: "invalid - wrong length",
360-
input: []interface{}{
361-
[]byte{0},
362-
big.NewInt(12345).Bytes(),
363-
},
364-
wantErr: true,
363+
name: "empty R and S",
364+
r: []byte{},
365+
s: []byte{},
366+
yParity: 0,
367+
want: signer.NewSignature(big.NewInt(0), big.NewInt(0), 0),
368+
},
369+
{
370+
name: "max valid size (32 bytes)",
371+
r: max32Bytes.Bytes(),
372+
s: max32Bytes.Bytes(),
373+
yParity: 0,
374+
want: signer.NewSignature(max32Bytes, max32Bytes, 0),
375+
},
376+
{
377+
name: "oversized R (33 bytes)",
378+
r: big33Bytes.Bytes(),
379+
s: big.NewInt(1).Bytes(),
380+
yParity: 0,
381+
wantErr: true,
382+
wantErrStr: "r exceeds maximum size",
383+
},
384+
{
385+
name: "oversized S (33 bytes)",
386+
r: big.NewInt(1).Bytes(),
387+
s: big33Bytes.Bytes(),
388+
yParity: 0,
389+
wantErr: true,
390+
wantErrStr: "s exceeds maximum size",
391+
},
392+
{
393+
name: "oversized R and S",
394+
r: big33Bytes.Bytes(),
395+
s: big33Bytes.Bytes(),
396+
yParity: 0,
397+
wantErr: true,
398+
wantErrStr: "r exceeds maximum size",
365399
},
366400
}
367401

368402
for _, tt := range tests {
369403
t.Run(tt.name, func(t *testing.T) {
370-
got, err := decodeSignature(tt.input)
404+
input := []interface{}{
405+
[]byte{tt.yParity},
406+
tt.r,
407+
tt.s,
408+
}
409+
got, err := decodeSignature(input)
371410
if tt.wantErr {
372411
assert.Error(t, err)
412+
assert.Nil(t, got)
413+
assert.Contains(t, err.Error(), tt.wantErrStr)
373414
return
374415
}
375416
assert.NoError(t, err)
@@ -380,6 +421,16 @@ func TestDecodeSignature(t *testing.T) {
380421
}
381422
}
382423

424+
func TestDecodeSignature_InvalidTupleLength(t *testing.T) {
425+
input := []interface{}{
426+
[]byte{0},
427+
big.NewInt(12345).Bytes(),
428+
}
429+
got, err := decodeSignature(input)
430+
assert.Error(t, err)
431+
assert.Nil(t, got)
432+
}
433+
383434
func TestRoundtrip(t *testing.T) {
384435
tests := []struct {
385436
name string
@@ -568,6 +619,69 @@ func TestRoundtripWithOptions(t *testing.T) {
568619
})
569620
}
570621

622+
func TestDeserialize_OversizedSignature(t *testing.T) {
623+
big33Bytes := new(big.Int).Lsh(big.NewInt(1), 256) // 33 bytes
624+
625+
tests := []struct {
626+
name string
627+
r []byte
628+
s []byte
629+
wantErrStr string
630+
}{
631+
{
632+
name: "oversized R in fee payer signature",
633+
r: big33Bytes.Bytes(),
634+
s: big.NewInt(1).Bytes(),
635+
wantErrStr: "r exceeds maximum size",
636+
},
637+
{
638+
name: "oversized S in fee payer signature",
639+
r: big.NewInt(1).Bytes(),
640+
s: big33Bytes.Bytes(),
641+
wantErrStr: "s exceeds maximum size",
642+
},
643+
}
644+
645+
for _, tt := range tests {
646+
t.Run(tt.name, func(t *testing.T) {
647+
rlpList := []interface{}{
648+
big.NewInt(42424).Bytes(), // chainId
649+
big.NewInt(1000000).Bytes(), // maxPriorityFeePerGas
650+
big.NewInt(2000000).Bytes(), // maxFeePerGas
651+
big.NewInt(21000).Bytes(), // gas
652+
[]interface{}{ // calls
653+
[]interface{}{
654+
common.HexToAddress("0x1234567890123456789012345678901234567890").Bytes(),
655+
big.NewInt(1000000).Bytes(),
656+
[]byte{},
657+
},
658+
},
659+
[]interface{}{}, // accessList
660+
[]byte{}, // nonceKey
661+
big.NewInt(1).Bytes(), // nonce
662+
[]byte{}, // validBefore
663+
[]byte{}, // validAfter
664+
[]byte{}, // feeToken
665+
[]interface{}{ // fee payer signature
666+
[]byte{0},
667+
tt.r,
668+
tt.s,
669+
},
670+
[]interface{}{}, // authorizationList
671+
}
672+
673+
rlpBytes, err := rlp.EncodeToBytes(rlpList)
674+
assert.NoError(t, err)
675+
676+
serialized := "0x76" + hex.EncodeToString(rlpBytes)
677+
678+
_, err = Deserialize(serialized)
679+
assert.Error(t, err)
680+
assert.Contains(t, err.Error(), tt.wantErrStr)
681+
})
682+
}
683+
}
684+
571685
// Helper functions
572686

573687
func hexToBigInt(s string) *big.Int {

0 commit comments

Comments
 (0)