Skip to content

Commit 0e59876

Browse files
jshajsha
authored
Merge commit from fork
* cipher: fix panic on KeyUnwrap of too-short slice * jwe: don't call KeyUnwrap on empty (encrypted) key Also don't call `aead.decrypt` on an empty key. * test: make asymmetric_test more precise These two test cases were passing a nil recipient, and checking for "any error" instead of a specific error, which meant that introducing a nil recipient check in `decryptKey` caused the test to stop testing what it meant to test, but continue passing. Now we check for a specific error. * test: TestKeyUnwrapShort * jwe: TestEmptyEncryptedKey * test: add `shorten` and `empty` corruptors
1 parent ddffdbc commit 0e59876

7 files changed

Lines changed: 182 additions & 16 deletions

File tree

‎asymmetric.go‎

Lines changed: 9 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -414,6 +414,9 @@ func (ctx ecKeyGenerator) genKey() ([]byte, rawHeader, error) {
414414

415415
// Decrypt the given payload and return the content encryption key.
416416
func (ctx ecDecrypterSigner) decryptKey(headers rawHeader, recipient *recipientInfo, generator keyGenerator) ([]byte, error) {
417+
if recipient == nil {
418+
return nil, errors.New("go-jose/go-jose: missing recipient")
419+
}
417420
epk, err := headers.getEPK()
418421
if err != nil {
419422
return nil, errors.New("go-jose/go-jose: invalid epk header")
@@ -461,13 +464,18 @@ func (ctx ecDecrypterSigner) decryptKey(headers rawHeader, recipient *recipientI
461464
return nil, ErrUnsupportedAlgorithm
462465
}
463466

467+
encryptedKey := recipient.encryptedKey
468+
if len(encryptedKey) == 0 {
469+
return nil, errors.New("go-jose/go-jose: missing JWE Encrypted Key")
470+
}
471+
464472
key := deriveKey(string(algorithm), keySize)
465473
block, err := aes.NewCipher(key)
466474
if err != nil {
467475
return nil, err
468476
}
469477

470-
return josecipher.KeyUnwrap(block, recipient.encryptedKey)
478+
return josecipher.KeyUnwrap(block, encryptedKey)
471479
}
472480

473481
func (ctx edDecrypterSigner) signPayload(payload []byte, alg SignatureAlgorithm) (Signature, error) {

‎asymmetric_test.go‎

Lines changed: 30 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -22,6 +22,7 @@ import (
2222
"crypto/rand"
2323
"crypto/rsa"
2424
"errors"
25+
"github.com/go-jose/go-jose/v4/json"
2526
"io"
2627
"testing"
2728
)
@@ -163,24 +164,35 @@ func TestInvalidECDecrypt(t *testing.T) {
163164

164165
generator := randomKeyGenerator{size: 16}
165166

167+
recipient := recipientInfo{
168+
// decryptKey will error out before the contents here matter
169+
encryptedKey: []byte("not used"),
170+
}
166171
// Missing epk header
167172
headers := rawHeader{}
168173

169174
if err := headers.set(headerAlgorithm, ECDH_ES); err != nil {
170175
t.Fatal(err)
171176
}
172177

173-
if _, err := dec.decryptKey(headers, nil, generator); err == nil {
178+
want := "go-jose/go-jose: missing epk header"
179+
_, err := dec.decryptKey(headers, &recipient, generator)
180+
if err == nil {
174181
t.Error("ec decrypter accepted object with missing epk header")
182+
} else if err.Error() != want {
183+
t.Errorf("decryptKey with missing epk header: got %q, want %q", err, want)
175184
}
176185

177186
// Invalid epk header
178-
if err := headers.set(headerEPK, &JSONWebKey{}); err == nil {
179-
t.Fatal("epk header should be invalid")
180-
}
187+
invalid := json.RawMessage("invalid")
188+
headers["epk"] = &invalid
181189

182-
if _, err := dec.decryptKey(headers, nil, generator); err == nil {
190+
want = "go-jose/go-jose: invalid epk header"
191+
_, err = dec.decryptKey(headers, &recipient, generator)
192+
if err == nil {
183193
t.Error("ec decrypter accepted object with invalid epk header")
194+
} else if err.Error() != want {
195+
t.Errorf("decryptKey with invalid epk header: got %q, want %q", err, want)
184196
}
185197
}
186198

@@ -367,6 +379,12 @@ func TestInvalidECPublicKey(t *testing.T) {
367379
D: fromBase64Int("0_NxaRPUMQoAJt50Gz8YiTr8gRTwyEaCumd-MToTmIo"),
368380
}
369381

382+
recipient := recipientInfo{
383+
// encryptedKey must be non-empty to pass initial checks, but the actual
384+
// bytes don't matter because we'll error out before using them.
385+
encryptedKey: []byte("not used"),
386+
}
387+
370388
headers := rawHeader{}
371389

372390
if err := headers.set(headerAlgorithm, ECDH_ES); err != nil {
@@ -381,9 +399,15 @@ func TestInvalidECPublicKey(t *testing.T) {
381399
privateKey: ecTestKey256,
382400
}
383401

384-
if _, err := dec.decryptKey(headers, nil, randomKeyGenerator{size: 16}); err == nil {
402+
_, err := dec.decryptKey(headers, &recipient, randomKeyGenerator{size: 16})
403+
if err == nil {
385404
t.Fatal("decrypter accepted JWS with invalid ECDH public key")
386405
}
406+
407+
want := "go-jose/go-jose: invalid epk header"
408+
if err.Error() != want {
409+
t.Errorf("decryptKey with invalid ECDH public key: got %q, want %q", err, want)
410+
}
387411
}
388412

389413
func TestInvalidAlgorithmEC(t *testing.T) {

‎cipher/key_wrap.go‎

Lines changed: 9 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -66,12 +66,20 @@ func KeyWrap(block cipher.Block, cek []byte) ([]byte, error) {
6666
}
6767

6868
// KeyUnwrap implements NIST key unwrapping; it unwraps a content encryption key (cek) with the given block cipher.
69+
//
70+
// https://datatracker.ietf.org/doc/html/rfc7518#section-4.4
71+
// https://datatracker.ietf.org/doc/html/rfc7518#section-4.6
72+
// https://datatracker.ietf.org/doc/html/rfc7518#section-4.8
6973
func KeyUnwrap(block cipher.Block, ciphertext []byte) ([]byte, error) {
74+
n := (len(ciphertext) / 8) - 1
75+
if n <= 0 {
76+
return nil, errors.New("go-jose/go-jose: JWE Encrypted Key too short")
77+
}
78+
7079
if len(ciphertext)%8 != 0 {
7180
return nil, errors.New("go-jose/go-jose: key wrap input must be 8 byte blocks")
7281
}
7382

74-
n := (len(ciphertext) / 8) - 1
7583
r := make([][]byte, n)
7684

7785
for i := range r {

‎cipher/key_wrap_test.go‎

Lines changed: 41 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -23,6 +23,47 @@ import (
2323
"testing"
2424
)
2525

26+
func TestKeyUnwrapShort(t *testing.T) {
27+
// Test vectors from: http://csrc.nist.gov/groups/ST/toolkit/documents/kms/key-wrap.pdf
28+
kek0, _ := hex.DecodeString("000102030405060708090A0B0C0D0E0F")
29+
block0, _ := aes.NewCipher(kek0)
30+
want := "go-jose/go-jose: JWE Encrypted Key too short"
31+
_, err := KeyUnwrap(block0, nil)
32+
if err == nil {
33+
t.Error("KeyUnwrap(_, nil): got nil, want error")
34+
} else if err.Error() != want {
35+
t.Errorf("KeyUnwrap(_, nil): got %q, want %q", err, want)
36+
}
37+
38+
input := []byte{}
39+
_, err = KeyUnwrap(block0, []byte{})
40+
if err == nil {
41+
t.Errorf("KeyUnwrap(_, %q): got nil, want error", input)
42+
} else if err.Error() != want {
43+
t.Errorf("KeyUnwrap(_, %q): got %q, want %q", input, err, want)
44+
}
45+
46+
for n := 0; n < 16; n++ {
47+
input := bytes.Repeat([]byte("a"), n)
48+
_, err = KeyUnwrap(block0, input)
49+
if err == nil {
50+
t.Errorf("KeyUnwrap(_, %q): got nil, want error", input)
51+
} else if err.Error() != want {
52+
t.Errorf("KeyUnwrap(_, nil): got %q, want %q", err, want)
53+
}
54+
}
55+
56+
input = bytes.Repeat([]byte("a"), 17)
57+
want = "go-jose/go-jose: key wrap input must be 8 byte blocks"
58+
_, err = KeyUnwrap(block0, input)
59+
if err == nil {
60+
t.Errorf("KeyUnwrap(_, %q): got nil, want error", input)
61+
} else if err.Error() != want {
62+
t.Errorf("KeyUnwrap(_, %q): got %q, want %q", input, err, want)
63+
}
64+
65+
}
66+
2667
func TestAesKeyWrap(t *testing.T) {
2768
// Test vectors from: http://csrc.nist.gov/groups/ST/toolkit/documents/kms/key-wrap.pdf
2869
kek0, _ := hex.DecodeString("000102030405060708090A0B0C0D0E0F")

‎crypter_test.go‎

Lines changed: 26 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -191,6 +191,8 @@ func TestRoundtripsJWECorrupted(t *testing.T) {
191191
func(obj *JSONWebEncryption) (string, error) { return obj.FullSerialize(), nil },
192192
}
193193

194+
// corrupter functions return true to skip (i.e. no change was made so no error is expected),
195+
// or false to do the test and expect an error.
194196
bitflip := func(slice []byte) bool {
195197
if len(slice) > 0 {
196198
slice[0] ^= 0xFF
@@ -199,6 +201,22 @@ func TestRoundtripsJWECorrupted(t *testing.T) {
199201
return true
200202
}
201203

204+
shorten := func(slice *[]byte) bool {
205+
if len(*slice) > 0 {
206+
*slice = (*slice)[:len(*slice)-1]
207+
return false
208+
}
209+
return true
210+
}
211+
212+
empty := func(slice *[]byte) bool {
213+
if len(*slice) > 0 {
214+
*slice = nil
215+
return false
216+
}
217+
return true
218+
}
219+
202220
corrupters := []func(*JSONWebEncryption) bool{
203221
func(obj *JSONWebEncryption) bool {
204222
// Set invalid ciphertext
@@ -216,6 +234,14 @@ func TestRoundtripsJWECorrupted(t *testing.T) {
216234
// Mess with encrypted key
217235
return bitflip(obj.recipients[0].encryptedKey)
218236
},
237+
func(obj *JSONWebEncryption) bool {
238+
// Remove encrypted key
239+
return empty(&obj.recipients[0].encryptedKey)
240+
},
241+
func(obj *JSONWebEncryption) bool {
242+
// Shorten encrypted key
243+
return shorten(&obj.recipients[0].encryptedKey)
244+
},
219245
func(obj *JSONWebEncryption) bool {
220246
// Mess with GCM-KW auth tag
221247
tag, _ := obj.protected.getTag()

‎jwe_test.go‎

Lines changed: 49 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -718,6 +718,55 @@ func TestJWEWithNullAlg(t *testing.T) {
718718
}
719719
}
720720

721+
func TestEmptyEncryptedKey(t *testing.T) {
722+
// These inputs use key wrapping with an empty wrapped key.
723+
// All fields except the unprotected header are empty; in particular "JWE Encrypted Key" is empty.
724+
serializedCompact := `eyJhbGciOiJQQkVTMi1IUzUxMitBMjU2S1ciLCJjdHkiOiJhcHBsaWNhdGlvbi9qd2sranNvbiIsImVuYyI6IkEyNTZHQ00iLCJwMmMiOjIxMDAwMCwicDJzIjoiY000YyJ9....`
725+
serializedJSON := `{"unprotected":{"alg":"PBES2-HS512+A256KW","cty":"application/jwk+json","enc":"A256GCM","p2c":210000,"p2s":"cM4c"}}`
726+
acceptedAlgs := []KeyAlgorithm{PBES2_HS512_A256KW}
727+
acceptedContentAlgs := []ContentEncryption{A256GCM}
728+
item, err := ParseEncrypted(serializedCompact, acceptedAlgs, acceptedContentAlgs)
729+
if err != nil {
730+
t.Fatalf("ParseEncrypted(%q): %s", serializedCompact, err)
731+
}
732+
733+
secret := []byte("valid-key-used-for-decryption")
734+
735+
// Note: we check this "want" value to distinguish from other error cases, but future refactorings to give
736+
// a more useful error message would be okay.
737+
want := "go-jose/go-jose: error in cryptographic primitive"
738+
_, err = item.Decrypt(secret)
739+
if err == nil {
740+
t.Errorf("Decrypt() after ParseEncrypted() with empty encrypted key should fail")
741+
} else if err.Error() != want {
742+
t.Errorf("Decrypt() after ParseEncrypted() with empty encrypted key: got %q, want %q", err, want)
743+
}
744+
745+
item, err = ParseEncryptedCompact(serializedCompact, acceptedAlgs, acceptedContentAlgs)
746+
if err != nil {
747+
t.Fatalf("ParseEncryptedCompact(%q): %s", serializedCompact, err)
748+
}
749+
750+
_, err = item.Decrypt(secret)
751+
if err == nil {
752+
t.Errorf("Decrypt() after ParseEncryptedCompact() with empty encrypted key should fail")
753+
} else if err.Error() != want {
754+
t.Errorf("Decrypt() after ParseEncryptedCompact() with empty encrypted key: got %q, want %q", err, want)
755+
}
756+
757+
item, err = ParseEncryptedJSON(serializedJSON, acceptedAlgs, acceptedContentAlgs)
758+
if err != nil {
759+
t.Fatalf("ParseEncryptedJSON(%q): %s", serializedJSON, err)
760+
}
761+
762+
_, err = item.Decrypt(secret)
763+
if err == nil {
764+
t.Errorf("Decrypt() after ParseEncryptedJSON() with empty encrypted key should fail")
765+
} else if err.Error() != want {
766+
t.Errorf("Decrypt() after ParseEncryptedJSON() with empty encrypted key: got %q, want %q", err, want)
767+
}
768+
}
769+
721770
func BenchmarkParseEncryptedCompat(b *testing.B) {
722771
msg := "eyJhbGciOiJSU0EtT0FFUCIsImVuYyI6IkExMjhHQ00ifQ.dGVzdA.dGVzdA.dGVzdA.dGVzdA"
723772
for range b.N {

‎symmetric.go‎

Lines changed: 18 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -366,11 +366,21 @@ func (ctx *symmetricKeyCipher) encryptKey(cek []byte, alg KeyAlgorithm) (recipie
366366

367367
// Decrypt the content encryption key.
368368
func (ctx *symmetricKeyCipher) decryptKey(headers rawHeader, recipient *recipientInfo, generator keyGenerator) ([]byte, error) {
369-
switch headers.getAlgorithm() {
370-
case DIRECT:
371-
cek := make([]byte, len(ctx.key))
372-
copy(cek, ctx.key)
373-
return cek, nil
369+
if recipient == nil {
370+
return nil, fmt.Errorf("go-jose/go-jose: missing recipient")
371+
}
372+
373+
alg := headers.getAlgorithm()
374+
if alg == DIRECT {
375+
return bytes.Clone(ctx.key), nil
376+
}
377+
378+
encryptedKey := recipient.encryptedKey
379+
if len(encryptedKey) == 0 {
380+
return nil, fmt.Errorf("go-jose/go-jose: missing JWE Encrypted Key")
381+
}
382+
383+
switch alg {
374384
case A128GCMKW, A192GCMKW, A256GCMKW:
375385
aead := newAESGCM(len(ctx.key))
376386

@@ -385,7 +395,7 @@ func (ctx *symmetricKeyCipher) decryptKey(headers rawHeader, recipient *recipien
385395

386396
parts := &aeadParts{
387397
iv: iv.bytes(),
388-
ciphertext: recipient.encryptedKey,
398+
ciphertext: encryptedKey,
389399
tag: tag.bytes(),
390400
}
391401

@@ -401,7 +411,7 @@ func (ctx *symmetricKeyCipher) decryptKey(headers rawHeader, recipient *recipien
401411
return nil, err
402412
}
403413

404-
cek, err := josecipher.KeyUnwrap(block, recipient.encryptedKey)
414+
cek, err := josecipher.KeyUnwrap(block, encryptedKey)
405415
if err != nil {
406416
return nil, err
407417
}
@@ -445,7 +455,7 @@ func (ctx *symmetricKeyCipher) decryptKey(headers rawHeader, recipient *recipien
445455
return nil, err
446456
}
447457

448-
cek, err := josecipher.KeyUnwrap(block, recipient.encryptedKey)
458+
cek, err := josecipher.KeyUnwrap(block, encryptedKey)
449459
if err != nil {
450460
return nil, err
451461
}

0 commit comments

Comments
 (0)