Repository: lestrrat-go/jwx
Pull request #938 (2023-06-14 08:01:27)

Bump golang.org/x/crypto from 0.9.0 to 0.10.0
Bumps [golang.org/x/crypto](https://github.com/golang/crypto) from 0.9.0 to 0.10.0.
<details>
<summary>Commits</summary>
<ul>
<li><a href="https://github.com/golang/crypto/commit/8e447d8cc585b0089d1938b8747264783295e65f"><code>8e447d8</code></a> go.mod: update golang.org/x dependencies</li>
<li><a href="https://github.com/golang/crypto/commit/22c99979527e39b0c665cb2ed3c91e30739b69e6"><code>22c9997</code></a> go.mod: tell x repo tagging to ignore dep on net</li>
<li><a href="https://github.com/golang/crypto/commit/d0b316056be80e452fdae998581d0a807b8df0a8"><code>d0b3160</code></a> x509roots: add new module</li>
<li><a href="https://github.com/golang/crypto/commit/16222386f4de802a3c27c1714b0bcc28c0fd5397"><code>1622238</code></a> Add support for &quot;<a href="mailto:hmac-sha2-512-etm@openssh.com">hmac-sha2-512-etm@openssh.com</a>&quot;</li>
<li>See full diff in <a href="https://github.com/golang/crypto/compare/v0.9.0...v0.10.0">compare view</a></li>
</ul>
</details>
<br />


[![Dependabot compatibility score](https://dependabot-badges.githubapp.com/badges/compatibility_score?dependency-name=golang.org/x/crypto&package-manager=go_modules&previous-version=0.9.0&new-version=0.10.0)](https://docs.github.com/en/github/managing-security-vulnerabilities/about-dependabot-security-updates#about-compatibility-scores)

Dependabot will resolve any conflicts with this PR as long as you don't alter it yourself. You can also trigger a rebase manually by commenting `@dependabot rebase`.

[//]: # (dependabot-automerge-start)
[//]: # (dependabot-automerge-end)

---

<details>
<summary>Dependabot commands and options</summary>
<br />

You can trigger Dependabot actions by commenting on this PR:
- `@dependabot rebase` will rebase this PR
- `@dependabot recreate` will recreate this PR, overwriting any edits that have been made to it
- `@dependabot merge` will merge this PR after your CI passes on it
- `@dependabot squash and merge` will squash and merge this PR after your CI passes on it
- `@dependabot cancel merge` will cancel a previously requested merge and block automerging
- `@dependabot reopen` will reopen this PR if it is closed
- `@dependabot close` will close this PR and stop Dependabot recreating it. You can achieve the same result by closing it manually
- `@dependabot ignore this major version` will close this PR and stop Dependabot creating any more for this major version (unless you reopen the PR or upgrade to it yourself)
- `@dependabot ignore this minor version` will close this PR and stop Dependabot creating any more for this minor version (unless you reopen the PR or upgrade to it yourself)
- `@dependabot ignore this dependency` will close this PR and stop Dependabot creating any more for this dependency (unless you reopen the PR or upgrade to it yourself)


</details>


Patch:
diff --git a/Changes b/Changes
index 9f414a62..17f73ddd 100644
--- a/Changes
+++ b/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
diff --git a/deps.bzl b/deps.bzl
index 4ecdfb6e..c484c00b 100644
--- a/deps.bzl
+++ b/deps.bzl
@@ -122,8 +122,8 @@ def go_dependencies():
         name = "org_golang_x_crypto",
         build_file_proto_mode = "disable_global",
         importpath = "golang.org/x/crypto",
-        sum = "h1:LF6fAI+IutBocDJ2OT0Q1g8plpYljMZ4+lty+dsqw3g=",
-        version = "v0.9.0",
+        sum = "h1:LKqV2xt9+kDzSTfOhx4FrkEBcMrAgHSYgzywV9zcGmM=",
+        version = "v0.10.0",
     )
     go_repository(
         name = "org_golang_x_mod",
@@ -152,23 +152,23 @@ def go_dependencies():
         name = "org_golang_x_sys",
         build_file_proto_mode = "disable_global",
         importpath = "golang.org/x/sys",
-        sum = "h1:EBmGv8NaZBZTWvrbjNoL6HVt+IVy3QDQpJs7VRIw3tU=",
-        version = "v0.8.0",
+        sum = "h1:KS/R3tvhPqvJvwcKfnBHJwwthS11LRhmM5D59eEXa0s=",
+        version = "v0.9.0",
     )
     go_repository(
         name = "org_golang_x_term",
         build_file_proto_mode = "disable_global",
         importpath = "golang.org/x/term",
-        sum = "h1:n5xxQn2i3PC0yLAbjTpNT85q/Kgzcr2gIoX9OrJUols=",
-        version = "v0.8.0",
+        sum = "h1:GRRCnKYhdQrD8kfRAdQ6Zcw1P0OcELxGLKJvtjVMZ28=",
+        version = "v0.9.0",
     )
 
     go_repository(
         name = "org_golang_x_text",
         build_file_proto_mode = "disable_global",
         importpath = "golang.org/x/text",
-        sum = "h1:2sjJmO8cDvYveuX97RDLsxlyUxLl+GHoLxBiRdHllBE=",
-        version = "v0.9.0",
+        sum = "h1:UpjohKhiEgNc0CSauXmwYftY1+LlaC75SJwh0SgCX58=",
+        version = "v0.10.0",
     )
     go_repository(
         name = "org_golang_x_tools",
diff --git a/go.mod b/go.mod
index 6ebd4603..d23bf7da 100644
--- a/go.mod
+++ b/go.mod
@@ -11,5 +11,5 @@ require (
 	github.com/lestrrat-go/option v1.0.1
 	github.com/segmentio/asm v1.2.0
 	github.com/stretchr/testify v1.8.4
-	golang.org/x/crypto v0.9.0
+	golang.org/x/crypto v0.10.0
 )
diff --git a/go.sum b/go.sum
index d61bcef1..2248ad87 100644
--- a/go.sum
+++ b/go.sum
@@ -32,8 +32,8 @@ github.com/stretchr/testify v1.8.4/go.mod h1:sz/lmYIOXD/1dqDmKjjqLyZ2RngseejIcXl
 github.com/yuin/goldmark v1.4.13/go.mod h1:6yULJ656Px+3vBD8DxQVa3kxgyrAnzto9xy5taEt/CY=
 golang.org/x/crypto v0.0.0-20190308221718-c2843e01d9a2/go.mod h1:djNgcEr1/C05ACkg1iLfiJU5Ep61QUkGW8qpdssI0+w=
 golang.org/x/crypto v0.0.0-20210921155107-089bfa567519/go.mod h1:GvvjBRRGRdwPK5ydBHafDWAxML/pGHZbMvKqRZ5+Abc=
-golang.org/x/crypto v0.9.0 h1:LF6fAI+IutBocDJ2OT0Q1g8plpYljMZ4+lty+dsqw3g=
-golang.org/x/crypto v0.9.0/go.mod h1:yrmDGqONDYtNj3tH8X9dzUun2m2lzPa9ngI6/RUPGR0=
+golang.org/x/crypto v0.10.0 h1:LKqV2xt9+kDzSTfOhx4FrkEBcMrAgHSYgzywV9zcGmM=
+golang.org/x/crypto v0.10.0/go.mod h1:o4eNf7Ede1fv+hwOwZsTHl9EsPFO6q6ZvYR8vYfY45I=
 golang.org/x/mod v0.6.0-dev.0.20220419223038-86c51ed26bb4/go.mod h1:jJ57K6gSWd91VN4djpZkiMVwK6gcyfeH4XE8wZrZaV4=
 golang.org/x/mod v0.8.0/go.mod h1:iBbtSCu2XBx23ZKBPSOrRkjjQPZFPuis4dIYUhu/chs=
 golang.org/x/net v0.0.0-20190620200207-3b0461eec859/go.mod h1:z5CRVTTTmAJ677TzLLGU+0bjPO0LkuOLi4/5GtJWs/s=
@@ -51,17 +51,20 @@ golang.org/x/sys v0.0.0-20220412211240-33da011f77ad/go.mod h1:oPkhp1MJrh7nUepCBc
 golang.org/x/sys v0.0.0-20220520151302-bc2c85ada10a/go.mod h1:oPkhp1MJrh7nUepCBck5+mAzfO9JrbApNNgaTdGDITg=
 golang.org/x/sys v0.0.0-20220722155257-8c9f86f7a55f/go.mod h1:oPkhp1MJrh7nUepCBck5+mAzfO9JrbApNNgaTdGDITg=
 golang.org/x/sys v0.5.0/go.mod h1:oPkhp1MJrh7nUepCBck5+mAzfO9JrbApNNgaTdGDITg=
-golang.org/x/sys v0.8.0 h1:EBmGv8NaZBZTWvrbjNoL6HVt+IVy3QDQpJs7VRIw3tU=
 golang.org/x/sys v0.8.0/go.mod h1:oPkhp1MJrh7nUepCBck5+mAzfO9JrbApNNgaTdGDITg=
+golang.org/x/sys v0.9.0 h1:KS/R3tvhPqvJvwcKfnBHJwwthS11LRhmM5D59eEXa0s=
+golang.org/x/sys v0.9.0/go.mod h1:oPkhp1MJrh7nUepCBck5+mAzfO9JrbApNNgaTdGDITg=
 golang.org/x/term v0.0.0-20201126162022-7de9c90e9dd1/go.mod h1:bj7SfCRtBDWHUb9snDiAeCFNEtKQo2Wmx5Cou7ajbmo=
 golang.org/x/term v0.0.0-20210927222741-03fcf44c2211/go.mod h1:jbD1KX2456YbFQfuXm/mYQcufACuNUgVhRMnK/tPxf8=
 golang.org/x/term v0.5.0/go.mod h1:jMB1sMXY+tzblOD4FWmEbocvup2/aLOaQEp7JmGp78k=
 golang.org/x/term v0.8.0/go.mod h1:xPskH00ivmX89bAKVGSKKtLOWNx2+17Eiy94tnKShWo=
+golang.org/x/term v0.9.0/go.mod h1:M6DEAAIenWoTxdKrOltXcmDY3rSplQUkrvaDU5FcQyo=
 golang.org/x/text v0.3.0/go.mod h1:NqM8EUOU14njkJ3fqMW+pc6Ldnwhi/IjpwHt7yyuwOQ=
 golang.org/x/text v0.3.3/go.mod h1:5Zoc/QRtKVWzQhOtBMvqHzDpF6irO9z98xDceosuGiQ=
 golang.org/x/text v0.3.7/go.mod h1:u+2+/6zg+i71rQMx5EYifcz6MCKuco9NR6JIITiCfzQ=
 golang.org/x/text v0.7.0/go.mod h1:mrYo+phRRbMaCq/xk9113O4dZlRixOauAjOtrjsXDZ8=
 golang.org/x/text v0.9.0/go.mod h1:e1OnstbJyHTd6l/uOt8jFFHp6TRDWZR/bV3emEE/zU8=
+golang.org/x/text v0.10.0/go.mod h1:TvPlkZtksWOMsz7fbANvkp4WM8x/WCo/om8BMLbz+aE=
 golang.org/x/tools v0.0.0-20180917221912-90fa682c2a6e/go.mod h1:n7NCudcB/nEzxVGmLbDWY5pfWTLqBcC2KZ6jyYvM4mQ=
 golang.org/x/tools v0.0.0-20191119224855-298f0cb1881e/go.mod h1:b+2E5dAYhXwXZwtnZ6UAqBI28+e2cm9otk0dWdXHAEo=
 golang.org/x/tools v0.1.12/go.mod h1:hNGJHUnrk76NpqgfD5Aqm5Crs+Hm0VOH/i9J2+nxYbc=
diff --git a/jwe/internal/aescbc/aescbc.go b/jwe/internal/aescbc/aescbc.go
index d38245ff..e1061412 100644
--- a/jwe/internal/aescbc/aescbc.go
+++ b/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)


Test patch:
diff --git a/jwe/internal/aescbc/aescbc_test.go b/jwe/internal/aescbc/aescbc_test.go
index 1b131258..533141ed 100644
--- a/jwe/internal/aescbc/aescbc_test.go
+++ b/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