Repository: lestrrat-go/jwx
Pull request #499 (2021-11-13 15:16:24)

Fix verification bypass when passed a raw JSON object with specific attributes
if the payload passed to `jwt.Parse` is a JSON object *looks* like JWT, e.g. has an `"aud"` field, but otherwise has fields that does not verify, e.g. `"exp"` is invalid, the current code silently bypasses verification.

codecov-commenter: # [Codecov](https://codecov.io/gh/lestrrat-go/jwx/pull/499?src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=lestrrat-go) Report
> Merging [#499](https://codecov.io/gh/lestrrat-go/jwx/pull/499?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=lestrrat-go) (cda1555) into [main](https://codecov.io/gh/lestrrat-go/jwx/commit/2d6f618063e7e69471443624c5724a3dcfe28efa?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=lestrrat-go) (2d6f618) will **increase** coverage by `0.05%`.
> The diff coverage is `100.00%`.

[![Impacted file tree graph](https://codecov.io/gh/lestrrat-go/jwx/pull/499/graphs/tree.svg?width=650&height=150&src=pr&token=kU2RSDosPt&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=lestrrat-go)](https://codecov.io/gh/lestrrat-go/jwx/pull/499?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=lestrrat-go)

```diff
@@            Coverage Diff             @@
##             main     #499      +/-   ##
==========================================
+ Coverage   72.56%   72.61%   +0.05%     
==========================================
  Files          80       79       -1     
  Lines        9221     9221              
==========================================
+ Hits         6691     6696       +5     
+ Misses       1725     1722       -3     
+ Partials      805      803       -2     
```


| [Impacted Files](https://codecov.io/gh/lestrrat-go/jwx/pull/499?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=lestrrat-go) | Coverage Δ | |
|---|---|---|
| [jwt/jwt.go](https://codecov.io/gh/lestrrat-go/jwx/pull/499/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=lestrrat-go#diff-and0L2p3dC5nbw==) | `75.00% <100.00%> (+2.61%)` | :arrow_up: |
| [internal/json/goccy.go](https://codecov.io/gh/lestrrat-go/jwx/pull/499/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=lestrrat-go#diff-aW50ZXJuYWwvanNvbi9nb2NjeS5nbw==) | | |
| [jws/message.go](https://codecov.io/gh/lestrrat-go/jwx/pull/499/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=lestrrat-go#diff-andzL21lc3NhZ2UuZ28=) | `81.73% <0.00%> (+0.91%)` | :arrow_up: |

------

[Continue to review full report at Codecov](https://codecov.io/gh/lestrrat-go/jwx/pull/499?src=pr&el=continue&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=lestrrat-go).
> **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=lestrrat-go)
> `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
> Powered by [Codecov](https://codecov.io/gh/lestrrat-go/jwx/pull/499?src=pr&el=footer&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=lestrrat-go). Last update [2d6f618...cda1555](https://codecov.io/gh/lestrrat-go/jwx/pull/499?src=pr&el=lastupdated&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=lestrrat-go). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=lestrrat-go).


Patch:
diff --git a/Changes b/Changes
index 3d4366e2..874f7093 100644
--- a/Changes
+++ b/Changes
@@ -1,6 +1,13 @@
 Changes
 =======
 
+v1.2.11 UNRELEASED
+[Security Fix]
+  * It was reported that since v1.2.6, it was possible to craft
+    a special JSON object to bypass JWT verification via `jwt.Parse`.
+    If you relied on this module to perform all the verification,
+    upgrade is strongly recommended.
+
 v1.2.10 09 Nov 2021
 [Bug fixes]
   * Parsing OpenID claims were not working for some fields.
diff --git a/jwt/jwt.go b/jwt/jwt.go
index be7b252a..183d7e72 100644
--- a/jwt/jwt.go
+++ b/jwt/jwt.go
@@ -164,6 +164,7 @@ func verifyJWS(ctx *parseCtx, payload []byte) ([]byte, int, error) {
 		return verifyJWSWithKeySet(ctx, payload)
 	}
 
+	// We can't proceed without verification parameters
 	vp := ctx.verifyParams
 	if vp == nil {
 		return nil, _JwsVerifySkipped, nil
@@ -182,10 +183,12 @@ func verifyJWSWithKeySet(ctx *parseCtx, payload []byte) ([]byte, int, error) {
 	if ks == nil { // the caller should have checked ctx.keySet || ctx.keySetProvider
 		if p := ctx.keySetProvider; p != nil {
 			// "trust" the payload, and parse it so that the provider can do its thing
+			ctx.skipVerification = true
 			tok, err := parse(ctx, msg.Payload())
 			if err != nil {
 				return nil, _JwsVerifyInvalid, err
 			}
+			ctx.skipVerification = false
 
 			v, err := p.KeySetFrom(tok)
 			if err != nil {
@@ -319,6 +322,16 @@ OUTER:
 					return nil, errors.Errorf(`expected nested encrypted/signed payload, got raw JWT`)
 				}
 			}
+
+			if i == 0 {
+				// We were NOT enveloped in other formats
+				if !ctx.skipVerification {
+					if _, _, err := verifyJWS(ctx, payload); err != nil {
+						return nil, err
+					}
+				}
+			}
+
 			break OUTER
 		case jwx.UnknownFormat:
 			// "Unknown" may include invalid JWTs, for example, those who lack "aud"
@@ -326,6 +339,15 @@ OUTER:
 			if ctx.pedantic {
 				return nil, errors.Errorf(`invalid JWT`)
 			}
+
+			if i == 0 {
+				// We were NOT enveloped in other formats
+				if !ctx.skipVerification {
+					if _, _, err := verifyJWS(ctx, payload); err != nil {
+						return nil, err
+					}
+				}
+			}
 			break OUTER
 		case jwx.JWS:
 			// Food for thought: This is going to break if you have multiple layers of


Test patch:
diff --git a/jwt/jwt_test.go b/jwt/jwt_test.go
index ffafa44a..6b793fd6 100644
--- a/jwt/jwt_test.go
+++ b/jwt/jwt_test.go
@@ -4,12 +4,15 @@ import (
 	"bytes"
 	"context"
 	"crypto/ecdsa"
+	"crypto/rand"
+	"crypto/rsa"
 	"encoding/base64"
 	"fmt"
 	"io/ioutil"
 	"net/http"
 	"net/http/httptest"
 	"net/url"
+	"strconv"
 	"strings"
 	"sync"
 	"testing"
@@ -1332,3 +1335,54 @@ func TestGH430(t *testing.T) {
 		return
 	}
 }
+
+func TestBenHigginsByPassRegression(t *testing.T) {
+	key, err := rsa.GenerateKey(rand.Reader, 2048)
+	if err != nil {
+		panic(err)
+	}
+	// Test if an access token JSON payload parses when provided directly
+	//
+	// The JSON below is slightly modified example payload from:
+	// https://docs.aws.amazon.com/cognito/latest/developerguide/amazon-cognito-user-pools-using-the-access-token.html
+
+	// Case 1: add "aud", and adjust exp to be valid
+	// Case 2: do not add "aud", adjust exp
+
+	exp := strconv.Itoa(int(time.Now().Unix()) + 1000)
+	const tmpl = `{%s
+    "sub": "aaaaaaaa-bbbb-cccc-dddd-eeeeeeeeeeee",
+    "device_key": "aaaaaaaa-bbbb-cccc-dddd-eeeeeeeeeeee",
+    "cognito:groups": ["admin"],
+    "token_use": "access",
+    "scope": "aws.cognito.signin.user.admin",
+    "auth_time": 1562190524,
+    "iss": "https://cognito-idp.us-west-2.amazonaws.com/us-west-2_example",
+    "exp": %s,
+    "iat": 1562190524,
+    "origin_jti": "aaaaaaaa-bbbb-cccc-dddd-eeeeeeeeeeee",
+    "jti": "aaaaaaaa-bbbb-cccc-dddd-eeeeeeeeeeee",
+    "client_id": "57cbishk4j24pabc1234567890",
+    "username": "janedoe@example.com"
+  }`
+
+	testcases := [][]byte{
+		[]byte(fmt.Sprintf(tmpl, `"aud": ["test"],`, exp)),
+		[]byte(fmt.Sprintf(tmpl, ``, exp)),
+	}
+
+	for _, tc := range testcases {
+		for _, pedantic := range []bool{true, false} {
+			_, err = jwt.Parse(
+				tc,
+				jwt.WithValidate(true),
+				jwt.WithPedantic(pedantic),
+				jwt.WithVerify(jwa.RS256, &key.PublicKey),
+			)
+			t.Logf("%s", err)
+			if !assert.Error(t, err, `jwt.Parse should fail`) {
+				return
+			}
+		}
+	}
+}