Skip to content

Commit 44be3a6

Browse files
dadrusdadrus
authored
fix: Rejecting malformed ECDSA signatures (#114)
1 parent a1ae2ef commit 44be3a6

2 files changed

Lines changed: 191 additions & 28 deletions

File tree

‎asymmetric.go‎

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -254,6 +254,11 @@ func (v *ecdsaVerifier) verifyPayload(payload []byte, signature []byte) error {
254254
keyBytes++
255255
}
256256

257+
expectedLen := 2 * keyBytes //nolint:mnd
258+
if len(signature) != expectedLen {
259+
return ErrInvalidSignature
260+
}
261+
257262
r := big.NewInt(0).SetBytes(signature[:keyBytes])
258263
s := big.NewInt(0).SetBytes(signature[keyBytes:])
259264

‎asymmetric_test.go‎

Lines changed: 186 additions & 28 deletions
Original file line numberDiff line numberDiff line change
@@ -317,15 +317,80 @@ func TestECDSAverifierVerifyPayload(t *testing.T) {
317317
pkp521, err := ecdsa.GenerateKey(elliptic.P521(), rand.Reader)
318318
require.NoError(t, err)
319319

320-
for _, tc := range []struct {
321-
key *ecdsa.PrivateKey
322-
alg SignatureAlgorithm
320+
for uc, tc := range map[string]struct {
321+
key *ecdsa.PrivateKey
322+
alg SignatureAlgorithm
323+
mutateMessage func(t *testing.T, message []byte) []byte
324+
mutateSignature func(t *testing.T, signature []byte) []byte
325+
shouldError bool
323326
}{
324-
{alg: EcdsaP256Sha256, key: pkp256},
325-
{alg: EcdsaP384Sha384, key: pkp384},
326-
{alg: EcdsaP521Sha512, key: pkp521},
327+
"valid signature with P256": {
328+
alg: EcdsaP256Sha256,
329+
key: pkp256,
330+
},
331+
"valid signature with P384": {
332+
alg: EcdsaP384Sha384,
333+
key: pkp384,
334+
},
335+
"valid signature with P521": {
336+
alg: EcdsaP521Sha512,
337+
key: pkp521,
338+
},
339+
"signature is too short": {
340+
shouldError: true,
341+
alg: EcdsaP256Sha256,
342+
key: pkp256,
343+
mutateSignature: func(t *testing.T, signature []byte) []byte {
344+
t.Helper()
345+
346+
return signature[0 : len(signature)-1]
347+
},
348+
},
349+
"signature is too long": {
350+
shouldError: true,
351+
alg: EcdsaP256Sha256,
352+
key: pkp256,
353+
mutateSignature: func(t *testing.T, signature []byte) []byte {
354+
t.Helper()
355+
356+
res := make([]byte, len(signature)+1)
357+
copy(res, signature)
358+
359+
return res
360+
},
361+
},
362+
"wrong signature": {
363+
shouldError: true,
364+
alg: EcdsaP256Sha256,
365+
key: pkp256,
366+
mutateMessage: func(t *testing.T, message []byte) []byte {
367+
t.Helper()
368+
369+
message[0] ^= 0x01
370+
371+
return message
372+
},
373+
},
327374
} {
328-
t.Run(string(tc.alg), func(t *testing.T) {
375+
t.Run(uc, func(t *testing.T) {
376+
mutateMessage := tc.mutateMessage
377+
if mutateMessage == nil {
378+
mutateMessage = func(t *testing.T, message []byte) []byte {
379+
t.Helper()
380+
381+
return message
382+
}
383+
}
384+
385+
mutateSignature := tc.mutateSignature
386+
if mutateSignature == nil {
387+
mutateSignature = func(t *testing.T, signature []byte) []byte {
388+
t.Helper()
389+
390+
return signature
391+
}
392+
}
393+
329394
sig, err := newECDSASigner(tc.key, "test", tc.alg)
330395
require.NoError(t, err)
331396

@@ -336,12 +401,16 @@ func TestECDSAverifierVerifyPayload(t *testing.T) {
336401
ver, err := newECDSAVerifier(&tc.key.PublicKey, "test", tc.alg)
337402
require.NoError(t, err)
338403

339-
err = ver.verifyPayload(message, res)
340-
require.NoError(t, err)
404+
err = ver.verifyPayload(mutateMessage(t, message), mutateSignature(t, res))
405+
406+
if tc.shouldError {
407+
require.Error(t, err)
408+
require.ErrorIs(t, err, ErrInvalidSignature)
409+
410+
return
411+
}
341412

342-
err = ver.verifyPayload([]byte("test"), res)
343-
require.Error(t, err)
344-
require.ErrorIs(t, err, ErrInvalidSignature)
413+
require.NoError(t, err)
345414
})
346415
}
347416
}
@@ -404,19 +473,92 @@ func TestRSAVerifierVerifyPayload(t *testing.T) {
404473
pk4096, err := rsa.GenerateKey(rand.Reader, 4096)
405474
require.NoError(t, err)
406475

407-
for _, tc := range []struct {
408-
uc string
409-
key *rsa.PrivateKey
410-
alg SignatureAlgorithm
476+
for uc, tc := range map[string]struct {
477+
key *rsa.PrivateKey
478+
alg SignatureAlgorithm
479+
mutateMessage func(t *testing.T, message []byte) []byte
480+
mutateSignature func(t *testing.T, signature []byte) []byte
481+
shouldError bool
411482
}{
412-
{uc: "2048 key with RsaPkcs1v15Sha256", key: pk2048, alg: RsaPkcs1v15Sha256},
413-
{uc: "3072 key with RsaPkcs1v15Sha384", key: pk3072, alg: RsaPkcs1v15Sha384},
414-
{uc: "4096 key with RsaPkcs1v15Sha512", key: pk4096, alg: RsaPkcs1v15Sha512},
415-
{uc: "2048 key with RsaPssSha256", key: pk2048, alg: RsaPssSha256},
416-
{uc: "3072 key with RsaPssSha384", key: pk3072, alg: RsaPssSha384},
417-
{uc: "4096 key with RsaPssSha512", key: pk4096, alg: RsaPssSha512},
483+
"2048 key with RsaPkcs1v15Sha256": {
484+
key: pk2048,
485+
alg: RsaPkcs1v15Sha256,
486+
},
487+
"3072 key with RsaPkcs1v15Sha384": {
488+
key: pk3072,
489+
alg: RsaPkcs1v15Sha384,
490+
},
491+
"4096 key with RsaPkcs1v15Sha512": {
492+
key: pk4096,
493+
alg: RsaPkcs1v15Sha512,
494+
},
495+
"2048 key with RsaPssSha256": {
496+
key: pk2048,
497+
alg: RsaPssSha256,
498+
},
499+
"3072 key with RsaPssSha384": {
500+
key: pk3072,
501+
alg: RsaPssSha384,
502+
},
503+
"4096 key with RsaPssSha512": {
504+
key: pk4096,
505+
alg: RsaPssSha512,
506+
},
507+
"signature is too short": {
508+
shouldError: true,
509+
alg: RsaPssSha256,
510+
key: pk2048,
511+
mutateSignature: func(t *testing.T, signature []byte) []byte {
512+
t.Helper()
513+
514+
return signature[0 : len(signature)-1]
515+
},
516+
},
517+
"signature is too long": {
518+
shouldError: true,
519+
alg: RsaPssSha256,
520+
key: pk2048,
521+
mutateSignature: func(t *testing.T, signature []byte) []byte {
522+
t.Helper()
523+
524+
res := make([]byte, len(signature)+1)
525+
copy(res, signature)
526+
527+
return res
528+
},
529+
},
530+
"wrong signature": {
531+
shouldError: true,
532+
alg: RsaPssSha256,
533+
key: pk2048,
534+
mutateMessage: func(t *testing.T, message []byte) []byte {
535+
t.Helper()
536+
537+
message[0] ^= 0x01
538+
539+
return message
540+
},
541+
},
418542
} {
419-
t.Run(tc.uc, func(t *testing.T) {
543+
t.Run(uc, func(t *testing.T) {
544+
mutateMessage := tc.mutateMessage
545+
if mutateMessage == nil {
546+
mutateMessage = func(t *testing.T, message []byte) []byte {
547+
t.Helper()
548+
549+
return message
550+
}
551+
}
552+
553+
mutateSignature := tc.mutateSignature
554+
if mutateSignature == nil {
555+
mutateSignature = func(t *testing.T, signature []byte) []byte {
556+
t.Helper()
557+
558+
return signature
559+
}
560+
}
561+
420562
sig, err := newRSASigner(tc.key, "test", tc.alg)
421563
require.NoError(t, err)
422564

@@ -427,12 +569,16 @@ func TestRSAVerifierVerifyPayload(t *testing.T) {
427569
ver, err := newRSAVerifier(&tc.key.PublicKey, "test", tc.alg)
428570
require.NoError(t, err)
429571

430-
err = ver.verifyPayload(message, res)
431-
require.NoError(t, err)
572+
err = ver.verifyPayload(mutateMessage(t, message), mutateSignature(t, res))
573+
574+
if tc.shouldError {
575+
require.Error(t, err)
576+
require.ErrorIs(t, err, ErrInvalidSignature)
432577

433-
err = ver.verifyPayload([]byte("test"), res)
434-
require.Error(t, err)
435-
require.ErrorIs(t, err, ErrInvalidSignature)
578+
return
579+
}
580+
581+
require.NoError(t, err)
436582
})
437583
}
438584
}
@@ -488,10 +634,22 @@ func TestEd25519VerifierVerifyPayload(t *testing.T) {
488634
ver, err := newEd25519Verifier(pubKey, "test", sig.alg)
489635
require.NoError(t, err)
490636

637+
// valid signature
491638
err = ver.verifyPayload(message, res)
492639
require.NoError(t, err)
493640

641+
// invalid signature for the given message
494642
err = ver.verifyPayload([]byte("test"), res)
495643
require.Error(t, err)
496644
require.ErrorIs(t, err, ErrInvalidSignature)
645+
646+
// too short signature
647+
err = ver.verifyPayload(message, res[0:len(res)-1])
648+
require.Error(t, err)
649+
require.ErrorIs(t, err, ErrInvalidSignature)
650+
651+
// too long signature
652+
err = ver.verifyPayload(message, append(res, 0x00))
653+
require.Error(t, err)
654+
require.ErrorIs(t, err, ErrInvalidSignature)
497655
}

0 commit comments

Comments
 (0)