Stable release for CVE-2020-26160 fix

[agiammar]

The CVE-2020-26160 vulnerability is fixed in the preview v4.0.0-preview1
Are there any plans to make the fix part of a stable release, either V4 or V3?

[ewilde]

We are maintaining a fixed version of this library https://github.com/form3tech-oss/jwt-go 

CVE fixed: https://github.com/form3tech-oss/jwt-go/releases/tag/v3.2.1

If that is of interest

[JohnStarich]

@dgrijalva Would you be open to cherry-picking the CVE fix from v4 onto a patch release of v3 – at least until repo ownership and v4 are sorted out?

[SVilgelm]

same issue #422

[JohnStarich]

I disagree about it being the same. Certainly is related though, thanks for the link!

This issue is not about how to fix the problem; it’s about getting the existing fix from v4 out to users in a stable release. I think ideally in v3, given that the new version is still in preview.

[SVilgelm]

agree

[petherin]

We issue tokens using v3, so all our tokens have aud as string not []string. If we upgrade to v4 to get the vulnerability fix, then aud will be []string in generated tokens and all our services consuming those tokens will break. To track down and make them all expect aud as []string is a difficult maintenance job with potentially disastrous authentication issues.

Can a future release be backwards-compatible so generated tokens have an aud of string or []string, maybe a flag that allows the user of the library to choose?

[tomqwpl]

Yes, we also have the same problem. A naive swap to the form3tech-oss version of this library causes JWT tokens generated by dex not to verify as the Audience field has changed to a []string and so the JWT token doesn't parse using StandardClaims.

[tomqwpl]

As I understand it, and looking at the code, the CVE only affects MapClaims, and not StandardClaims. If you use a StandardClaims and the token has an audience array rather than a string, parsing the JWT just fails, so you don't get into the situation described by the CVE.
The spec allows for aud to be a string or an array. StandardClaims originally chose to map this as a single string. It would therefore fail to parse a JWT if used with a token that had an array of aud values. Fine. Now, that situation has been reversed and that now (both 4.0 here and the form3tech-oss version) use an array for Audience in StandardClaims which means that it now only works for tokens that contain an array claim for audience. That's got nothing to do with the CVE, both are valid, so the change is unnecessary (and annoying) from the point of view of simply fixing CVE. The CVE only affects you if you're using MapClaims.

[SVilgelm]

@tommyo you are not correct, the aud in v4 support both, string and []string:

jwt-go/claim_strings.go

Line 38 in c661858

func (c *ClaimStrings) UnmarshalJSON(data []byte) error {

The go representation uses []string, but the json can be either just a string or []string.

[tomqwpl]

Ah, that makes sense. I hadn't spotted that, well I hadn't looked for it to be honest. I did wonder whether it might though, to avoid the incompatibility.
So it's just the form3tech-oss version that suffers from the issue. For our use, we don't use MapClaims, so as far as I can tell aren't affected by the CVE anyway. It would be nice to have a stable release of this repository with the fix in though, as it avoids having to add a CVE ignore line to loads of repositories, or adding replace lines to the go mod files in a bunch of repositories.

[petherin]

In the scenario where we have several services using dgrijalva/jwt-go, we would like to upgrade dgrijalva/jwt-go in one of them to v4 (which would start sending out tokens where its StandardClaims.aud is []string) but still have the other services using dgrijalva/jwt-go v.3.2.0 continue to successfully consume those tokens. At the moment they would break if StandardClaims.aud is []string.

So in v4 it would be useful to decide whether using StandardClaims generates tokens whose aud is either []string or a string, to accommodate this overlap of some services having v4 and others v3.

[aldas]

I have fundamental question about this critical bug.

jwt-go before 4.0.0-preview1 allows attackers to bypass intended access restrictions in situations with []string{} for m["aud"] (which is allowed by the specification). Because the type assertion fails, "" is the value of aud. This is a security problem if the JWT token is presented to a service that lacks its own audience check.

Aud checking code is

func (m MapClaims) VerifyAudience(cmp string, req bool) bool {
	aud, _ := m["aud"].(string)
	return verifyAud(aud, cmp, req)
}

func verifyAud(aud string, cmp string, required bool) bool {
	if aud == "" {
		return !required
	}
	if subtle.ConstantTimeCompare([]byte(aud), []byte(cmp)) != 0 {
		return true
	} else {
		return false
	}
}

VerifyAudience is not called anywhere in library code. This is totally up to developer to implement.

Now examples:

func TestVerifyAud(t *testing.T) {
	var testCases = []struct {
		name  string
		token string
	}{
		{
			name:  "aud is missing",
			token: `{}`,
		},
		{
			name:  "aud is array",
			token: `{"aud": ["site"]}`,
		},
	}

	for _, tc := range testCases {
		t.Run(tc.name, func(t *testing.T) {
			var claims jwt.MapClaims
			if err := json.Unmarshal([]byte(tc.token), &claims); err != nil { // <-- aud is missing
				t.Fatal(err)
			}

			// If I set VerifyAudience to OPTIONAL why should it not PASS?
			optionalAudResult := claims.VerifyAudience("mysite.com", false) // <--- require set to false causes check to pass when aud is empty
			if !optionalAudResult {
				t.Fatalf("we have empty aud and set our verify check to optional but result was false - meaning verify had INCORRECT result")
			}

			requiredlAudResult := claims.VerifyAudience("mysite.com", true) // <--- require set to true
			if requiredlAudResult {
				t.Fatalf("we have empty aud and set our verify check to REQUIRED but result was true - meaning verify had INCORRECT result")
			}
		})
	}
}

If VerifyAudience second argument is to to true - meaning aud value is REQUIRED then it will never give us true when aud is array or empty in token.

Only way to have critical bug is to set VerifyAudience to OPTIONAL. But why on earth would you set this check to optional. This would be same as writing

if password == "mysecret" || password == "" {
  login()
}

Is it library fault when user codes one part of token validation to be optional?

NB: to fake empty aud values you need to have token signed with valid key - if attacker has valid key why they would need to manipulate aud values? If you are crossusing token singing keys for different sites and have set aud check optional - is not it more like your problem?

Fundamental question - it is really critical bug?

[aldas]

just poiting out that you can not even marshal array to jwt.StandardClaim struct

func TestStandardClaims_VerifyAud(t *testing.T) {
	var testCases = []struct {
		name  string
		token string
	}{
		{
			name:  "aud is missing",
			token: `{}`,
		},
		{
			name:  "aud is array",
			token: `{"aud": ["site"]}`,
		},
	}

	for _, tc := range testCases {
		t.Run(tc.name, func(t *testing.T) {
			var claims jwt.StandardClaims
			if err := json.Unmarshal([]byte(tc.token), &claims); err != nil { // <-- aud is missing
				t.Fatal(err)
			}

			// If I set VerifyAudience to OPTIONAL why should it not PASS?
			optionalAudResult := claims.VerifyAudience("mysite.com", false) // <--- require set to false causes check to pass when aud is empty
			if !optionalAudResult {
				t.Fatalf("we have empty aud and set our verify check to optional but result was false - meaning verify had INCORRECT result")
			}

			requiredlAudResult := claims.VerifyAudience("mysite.com", true) // <--- require set to true
			if requiredlAudResult {
				t.Fatalf("we have empty aud and set our verify check to REQUIRED but result was true - meaning verify had INCORRECT result")
			}
		})
	}
}

it will fail with

 main_test.go:100: json: cannot unmarshal array into Go struct field StandardClaims.aud of type string

[aldas]

why not just fix MapClaims.VerifyAudience with something like that if optional case is problematic

func (m MapClaims) VerifyAudience(cmp string, req bool) bool {
	rawAud, exists := m["aud"]
	if !exists {
		return !req
	}

	switch aud := rawAud.(type) {
	case string:
		return verifyAud(aud, cmp, req)
	case []string:
		for _, a := range aud {
			if verifyAud(a, cmp, req) {
				return true
			}
		}
		return !req && len(aud) == 0 // optional and empty is OK (verified)
	default: // not supported types are always incorrect
		return false
	}
}

and release v3.2.1 patch version. not need to have breaking changes at least if MapClaims is concerned.

@dgrijalva ?

[oxisto]

We have just released a v3.2.1 version of our forked/cloned community maintained version at https://github.com/golang-jwt/jwt/releases/tag/v3.2.1