High severity issue in github.com/dgrijalva/jwt-go

[sagikazarmark]

There is an unpatched, high severity issue in the aforementioned JWT package: dgrijalva/jwt-go#428

Unfortunately, it looks like the author completely abandoned the package.

There is a maintained fork that fixes the issue, but we decided to switch to gopkg.in/square/go-jose.v2

A lot of users seem to prefer this package now.

I might provide a PR later, I just wanted to raise awareness that the current JWT implementation relies on a library with a security risk.

It's not a security issue with the library itself and can easily be fixed by users on their end by using a fork, so public disclosure should probably be ok. Feel free to delete the issue if you disagree @peterbourgon

[JimFicarra]

If you're referring to the fork at github.com/form3tech-oss/jwt-go, there's a bug in the code that doesn't properly handle the audience claim. The RFC specifies the AUD claim can either be a string or an array of strings.

The code in the fork assumes it's an array of string and any AUD claim with a string (e.g. Azure AD id tokens) will cause a panic.

[sagikazarmark]

Thanks for pointing that out, I didn't know that. I guess we are better off with go-jose then.

[peterbourgon]

Happy to take a PR which migrates to the other package.

[nathanejohnson]

dgrijalva/jwt-go#422

If you look at the bottom, it appears to be fixed in the v4 version, which has been tagged "preview" for a bit but that's what we're using internally for what it's worth.

[aldas]

This vulnerability is real problem when you are reusing token signing keys for different applications and have coded explicit rule that aud is optional. If aud is checked as required field then there is no vulnerability, only a bug (claims are considered invalid). See dgrijalva/jwt-go#422 (comment)

But does it even make sense in security perspective to have a rule - it is OK when aud field is missing from token and if aud is in token we will check if it matches with our value. Even without this vulnerability you have a problem - If attacker gets their hands on VALID token without aud this check passes.

[peterbourgon]

Is there an outcome here? What should we be doing?

[JimFicarra]

This weekend I started replacing the dgrijalva/jwt-go libary usage with the square/go-jose library to see how this would pan-out. This will be a breaking change to use kit/auth/jwt since the existing NewSigner and NewParser funcs take parameters that are specific to the dgrijalva library. They have to be replaced with the equivalent go-jose library parameters to keep close to the intent of the two funcs.

We can go that route or try to make those funcs a bit more generic so that the implementation returned in the endpoint.Endpoint can be more easily replaced/maintained in the future.

Thoughts?

[aldas]

is it jwt-go problem when developer writes in their application aud claim check that is optional and that check produces same result as aud would not exist at certain condition? if ok := StandardClaims.VerifyAudience("https://mysite.com", false) Anyone who wrote aud check as required is safe. see

I mean - when it makes sense to optionally check that kind of info?

[sagikazarmark]

Sadly, go-jose seems to have its own issues: dunglas/mercure#394 (comment)

Vulnerabilities is one problem. Maintenance is another. The currently used jwt library is not maintained.

Sending a PR is on my todolist, but didn't have the time so far.

[townsymush]

So after digging and having the same issue. I think the solution here will be to use https://github.com/golang-jwt/jwt. It's a maintained fork from dgrijalva/jwt-go#428

[raymclee]

So after digging and having the same issue. I think the solution here will be to use https://github.com/golang-jwt/jwt. It's a maintained fork from dgrijalva/jwt-go#428

I tried to use https://github.com/golang-jwt/jwt. but I got the following error

cannot use keys (type func(*"github.com/golang-jwt/jwt".Token) (interface {}, error)) as type "github.com/dgrijalva/jwt-go".Keyfunc in argument to "github.com/go-kit/kit/auth/jwt".NewParser

[andig]

Because go-kit hasn‘t made the switch yet.

[vovinacci]

Submitted #1172 to switch to https://github.com/golang-jwt/jwt. Kept things very simple. Hope that works and it's possible to do a hotfix release if all is fine and PR is merged.

[sagikazarmark]

Not sure a hotfix is the right approach as this is a breaking change.

[vovinacci]

Ah, indeed that's a breaking change, I was too fast, sorry for that.