merge for v2 (#939)

* 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.

Co-authored-by: ICHINOSE Shogo <shogo82148@gmail.com>

* Update Changes

---------

Co-authored-by: ICHINOSE Shogo <shogo82148@gmail.com>

Changes

@@ -4,6 +4,16 @@ Changes
 v2 has many incompatibilities with v1. To see the full list of differences between
 v1 and v2, please read the Changes-v2.md file (https://github.com/lestrrat-go/jwx/blob/develop/v2/Changes-v2.md)
 
+v2.0.11 - 14 Jun 2023
+[Security]
+  * Potential Padding Oracle Attack Vulnerability and Timing Attack Vulnerability 
+    for JWE AES-CBC encrypted payloads affecting all v2 releases up to v2.0.10,
+    all v1 releases up to v1.2.25, and all v0 releases up to v0.9.2 have been reported by
+    @shogo82148.
+
+    Please note that v0 versions will NOT receive fixes.
+    This release fixes these vulnerabilities for the v2 series.
+
 v2.0.10 - 12 Jun 2023
 [New Features]
   * [jwe] (EXPERIMENTAL) Added `jwe.KeyEncrypter` and `jwe.KeyDecrypter` interfaces

jwe/internal/aescbc/aescbc.go

@@ -7,6 +7,7 @@ import (
 	"crypto/sha512"
 	"crypto/subtle"
 	"encoding/binary"
+	"errors"
 	"fmt"
 	"hash"
 )
@@ -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 {
@@ -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)

jwe/internal/aescbc/aescbc_test.go

@@ -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