Skip to content

Commit 3275e21

Browse files
shogo82148shogo82148
authored
Merge pull request from GHSA-rm8v-mxj3-5rmq
### Summary Decrypting AES-CBC encrypted JWE has Potential Padding Oracle Attack Vulnerability. ### Details On [v2.0.10](https://github.com/lestrrat-go/jwx/releases/tag/v2.0.10), decrypting AES-CBC encrypted JWE may return an error "failed to generate plaintext from decrypted blocks: invalid padding": https://github.com/lestrrat-go/jwx/blob/8840ffd4afc5839f591ff0e9ba9034af52b1643e/jwe/internal/aescbc/aescbc.go#L210-L213 ```go plaintext, err := unpad(buf, c.blockCipher.BlockSize()) if err != nil { return nil, fmt.Errorf(`failed to generate plaintext from decrypted blocks: %w`, err) } ``` Reporting padding error causes [Padding Oracle Attack](https://en.wikipedia.org/wiki/Padding_oracle_attack) Vulnerability. RFC 7516 JSON Web Encryption (JWE) says that we MUST NOT do this. > 11.5. Timing Attacks > To mitigate the attacks described in RFC 3218 [RFC3218], the > recipient MUST NOT distinguish between format, padding, and length > errors of encrypted keys. It is strongly recommended, in the event > of receiving an improperly formatted key, that the recipient > substitute a randomly generated CEK and proceed to the next step, to > mitigate timing attacks. In addition, the time to remove padding depends on the length of the padding. It may leak the length of the padding by Timing Attacks. https://github.com/lestrrat-go/jwx/blob/796b2a9101cf7e7cb66455e4d97f3c158ee10904/jwe/internal/aescbc/aescbc.go#L33-L66 ```go func unpad(buf []byte, n int) ([]byte, error) { lbuf := len(buf) rem := lbuf % n // First, `buf` must be a multiple of `n` if rem != 0 { return nil, fmt.Errorf("input buffer must be multiple of block size %d", n) } // Find the last byte, which is the encoded padding // i.e. 0x1 == 1 byte worth of padding last := buf[lbuf-1] // This is the number of padding bytes that we expect expected := int(last) if expected == 0 || /* we _have_ to have padding here. therefore, 0x0 is not an option */ expected > n || /* we also must make sure that we don't go over the block size (n) */ expected > lbuf /* finally, it can't be more than the buffer itself. unlikely, but could happen */ { return nil, fmt.Errorf(`invalid padding byte at the end of buffer`) } // start i = 1 because we have already established that expected == int(last) where // last = buf[lbuf-1]. // // we also don't check against lbuf-i in range, because we have established expected <= lbuf for i := 1; i < expected; i++ { if buf[lbuf-i] != last { return nil, fmt.Errorf(`invalid padding`) } } return buf[:lbuf-expected], nil } ``` To mitigate Timing Attacks, it MUST be done in constant time. ### Impact The authentication tag is verified, so it is not an immediate attack.
1 parent 796b2a9 commit 3275e21

2 files changed

Lines changed: 52 additions & 34 deletions

File tree

‎jwe/internal/aescbc/aescbc.go‎

Lines changed: 49 additions & 32 deletions
Original file line numberDiff line numberDiff line change
@@ -7,6 +7,7 @@ import (
77
"crypto/sha512"
88
"crypto/subtle"
99
"encoding/binary"
10+
"errors"
1011
"fmt"
1112
"hash"
1213
)
@@ -30,39 +31,56 @@ func pad(buf []byte, n int) []byte {
3031
return newbuf
3132
}
3233

33-
func unpad(buf []byte, n int) ([]byte, error) {
34-
lbuf := len(buf)
35-
rem := lbuf % n
36-
37-
// First, `buf` must be a multiple of `n`
38-
if rem != 0 {
39-
return nil, fmt.Errorf("input buffer must be multiple of block size %d", n)
34+
// ref. https://github.com/golang/go/blob/c3db64c0f45e8f2d75c5b59401e0fc925701b6f4/src/crypto/tls/conn.go#L279-L324
35+
//
36+
// extractPadding returns, in constant time, the length of the padding to remove
37+
// from the end of payload. It also returns a byte which is equal to 255 if the
38+
// padding was valid and 0 otherwise. See RFC 2246, Section 6.2.3.2.
39+
func extractPadding(payload []byte) (toRemove int, good byte) {
40+
if len(payload) < 1 {
41+
return 0, 0
4042
}
4143

42-
// Find the last byte, which is the encoded padding
43-
// i.e. 0x1 == 1 byte worth of padding
44-
last := buf[lbuf-1]
44+
paddingLen := payload[len(payload)-1]
45+
t := uint(len(payload)) - uint(paddingLen)
46+
// if len(payload) > paddingLen then the MSB of t is zero
47+
good = byte(int32(^t) >> 31)
4548

46-
// This is the number of padding bytes that we expect
47-
expected := int(last)
49+
// The maximum possible padding length plus the actual length field
50+
toCheck := 256
51+
// The length of the padded data is public, so we can use an if here
52+
if toCheck > len(payload) {
53+
toCheck = len(payload)
54+
}
4855

49-
if expected == 0 || /* we _have_ to have padding here. therefore, 0x0 is not an option */
50-
expected > n || /* we also must make sure that we don't go over the block size (n) */
51-
expected > lbuf /* finally, it can't be more than the buffer itself. unlikely, but could happen */ {
52-
return nil, fmt.Errorf(`invalid padding byte at the end of buffer`)
56+
for i := 1; i <= toCheck; i++ {
57+
t := uint(paddingLen) - uint(i)
58+
// if i <= paddingLen then the MSB of t is zero
59+
mask := byte(int32(^t) >> 31)
60+
b := payload[len(payload)-i]
61+
good &^= mask&paddingLen ^ mask&b
5362
}
5463

55-
// start i = 1 because we have already established that expected == int(last) where
56-
// last = buf[lbuf-1].
64+
// We AND together the bits of good and replicate the result across
65+
// all the bits.
66+
good &= good << 4
67+
good &= good << 2
68+
good &= good << 1
69+
good = uint8(int8(good) >> 7)
70+
71+
// Zero the padding length on error. This ensures any unchecked bytes
72+
// are included in the MAC. Otherwise, an attacker that could
73+
// distinguish MAC failures from padding failures could mount an attack
74+
// similar to POODLE in SSL 3.0: given a good ciphertext that uses a
75+
// full block's worth of padding, replace the final block with another
76+
// block. If the MAC check passed but the padding check failed, the
77+
// last byte of that block decrypted to the block size.
5778
//
58-
// we also don't check against lbuf-i in range, because we have established expected <= lbuf
59-
for i := 1; i < expected; i++ {
60-
if buf[lbuf-i] != last {
61-
return nil, fmt.Errorf(`invalid padding`)
62-
}
63-
}
79+
// See also macAndPaddingGood logic below.
80+
paddingLen &= good
6481

65-
return buf[:lbuf-expected], nil
82+
toRemove = int(paddingLen)
83+
return
6684
}
6785

6886
type Hmac struct {
@@ -199,18 +217,17 @@ func (c Hmac) Open(dst, nonce, ciphertext, data []byte) ([]byte, error) {
199217
return nil, fmt.Errorf(`failed to compute auth tag: %w`, err)
200218
}
201219

202-
if subtle.ConstantTimeCompare(expectedTag, tag) != 1 {
203-
return nil, fmt.Errorf(`invalid ciphertext (tag mismatch)`)
204-
}
205-
206220
cbc := cipher.NewCBCDecrypter(c.blockCipher, nonce)
207221
buf := make([]byte, tagOffset)
208222
cbc.CryptBlocks(buf, ciphertext)
209223

210-
plaintext, err := unpad(buf, c.blockCipher.BlockSize())
211-
if err != nil {
212-
return nil, fmt.Errorf(`failed to generate plaintext from decrypted blocks: %w`, err)
224+
toRemove, good := extractPadding(buf)
225+
cmp := subtle.ConstantTimeCompare(expectedTag, tag) & int(good)
226+
if cmp != 1 {
227+
return nil, errors.New(`invalid ciphertext`)
213228
}
229+
230+
plaintext := buf[:len(buf)-toRemove]
214231
ret := ensureSize(dst, len(plaintext))
215232
out := ret[len(dst):]
216233
copy(out, plaintext)

‎jwe/internal/aescbc/aescbc_test.go‎

Lines changed: 3 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -71,10 +71,11 @@ func TestPad(t *testing.T) {
7171
return
7272
}
7373

74-
pb, err := unpad(pb, 16)
75-
if !assert.NoError(t, err, "Unpad return successfully") {
74+
toRemove, good := extractPadding(pb)
75+
if !assert.Equal(t, 1, int(good)&1, "padding should be good") {
7676
return
7777
}
78+
pb = pb[:len(pb)-toRemove]
7879

7980
if !assert.Len(t, pb, i, "Unpad should result in len = %d", i) {
8081
return

0 commit comments

Comments
 (0)