Skip to content

Commit

Permalink
Merge pull request from GHSA-rm8v-mxj3-5rmq
Browse files Browse the repository at this point in the history
### 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.
  • Loading branch information
shogo82148 authored Jun 14, 2023
1 parent 796b2a9 commit 3275e21
Show file tree
Hide file tree
Showing 2 changed files with 52 additions and 34 deletions.
81 changes: 49 additions & 32 deletions jwe/internal/aescbc/aescbc.go
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@ import (
"crypto/sha512"
"crypto/subtle"
"encoding/binary"
"errors"
"fmt"
"hash"
)
Expand All @@ -30,39 +31,56 @@ func pad(buf []byte, n int) []byte {
return newbuf
}

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)
// ref. https://github.com/golang/go/blob/c3db64c0f45e8f2d75c5b59401e0fc925701b6f4/src/crypto/tls/conn.go#L279-L324
//
// extractPadding returns, in constant time, the length of the padding to remove
// from the end of payload. It also returns a byte which is equal to 255 if the
// padding was valid and 0 otherwise. See RFC 2246, Section 6.2.3.2.
func extractPadding(payload []byte) (toRemove int, good byte) {
if len(payload) < 1 {
return 0, 0
}

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

// This is the number of padding bytes that we expect
expected := int(last)
// The maximum possible padding length plus the actual length field
toCheck := 256
// The length of the padded data is public, so we can use an if here
if toCheck > len(payload) {
toCheck = len(payload)
}

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`)
for i := 1; i <= toCheck; i++ {
t := uint(paddingLen) - uint(i)
// if i <= paddingLen then the MSB of t is zero
mask := byte(int32(^t) >> 31)
b := payload[len(payload)-i]
good &^= mask&paddingLen ^ mask&b
}

// start i = 1 because we have already established that expected == int(last) where
// last = buf[lbuf-1].
// We AND together the bits of good and replicate the result across
// all the bits.
good &= good << 4
good &= good << 2
good &= good << 1
good = uint8(int8(good) >> 7)

// Zero the padding length on error. This ensures any unchecked bytes
// are included in the MAC. Otherwise, an attacker that could
// distinguish MAC failures from padding failures could mount an attack
// similar to POODLE in SSL 3.0: given a good ciphertext that uses a
// full block's worth of padding, replace the final block with another
// block. If the MAC check passed but the padding check failed, the
// last byte of that block decrypted to the block size.
//
// 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`)
}
}
// See also macAndPaddingGood logic below.
paddingLen &= good

return buf[:lbuf-expected], nil
toRemove = int(paddingLen)
return
}

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

if subtle.ConstantTimeCompare(expectedTag, tag) != 1 {
return nil, fmt.Errorf(`invalid ciphertext (tag mismatch)`)
}

cbc := cipher.NewCBCDecrypter(c.blockCipher, nonce)
buf := make([]byte, tagOffset)
cbc.CryptBlocks(buf, ciphertext)

plaintext, err := unpad(buf, c.blockCipher.BlockSize())
if err != nil {
return nil, fmt.Errorf(`failed to generate plaintext from decrypted blocks: %w`, err)
toRemove, good := extractPadding(buf)
cmp := subtle.ConstantTimeCompare(expectedTag, tag) & int(good)
if cmp != 1 {
return nil, errors.New(`invalid ciphertext`)
}

plaintext := buf[:len(buf)-toRemove]
ret := ensureSize(dst, len(plaintext))
out := ret[len(dst):]
copy(out, plaintext)
Expand Down
5 changes: 3 additions & 2 deletions jwe/internal/aescbc/aescbc_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -71,10 +71,11 @@ func TestPad(t *testing.T) {
return
}

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

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

0 comments on commit 3275e21

Please sign in to comment.