Vulnerabilities in JSON Web Token

[wunki]

Hi,

I believe some of the vulnerabilities mentioned in this article are also applicable to this library:

Critical vulnerabilities in JSON Web Token libraries

Think it would be good to add an argument to the Parse function which tells it which algorithm to use.

[Acconut]

I was just about to open an issue with the same topic. As far as I can tell, jwt-go is vulnerable to the HMAC-spoofing. Hopefully this will be fixed soon.

[rmullinnix]

You can restrict the key today in the user provided lookup function. The jwt.Token is passed into the lookup function. You can check the signing method and only return the key if it's valid for the signing method. This would allow you to prevent the RSA public key being used with a HMAC algorithm.

[dgrijalva]

Adding it to Parse is probably the wrong place. Adding it as a return value to Keyfunc makes more sense, since it still requires the same thing of the developer, but allows for handling multiple types of keys. Remember that, before parse, you don't know what kind of key it's claiming to be.

[dgrijalva]

There are two simple ways to protect yourself from this attack today:

// in your Keyfunc, check the alg
        if _, ok := t.Method.(*jwt.SigningMethodRSA); !ok {
            return nil, fmt.Errorf("Unexpected signing method: %v", t.Header["alg"])
        }

OR, you can simply return an rsa.PrivateKey instead of []byte.

[dgrijalva]

IMO, the safest solution here would be to have jwt.SigningMethodRSA stop accepting []byte and require parsed keys.

• Requiring signing method as an argument to Parse means that, if the developer actually supports multiple algs, they have to implement their own parsing or do two passes.
• Requiring signing method be returned from Keyfunc can be simply bypassed by returning t.Header["alg"]
• Requiring specific key types is the most explicit solution to the actual issue (mistaken identity).

[dgrijalva]

Updated README:
c48cfd5

[russellhaering]

It feels like what is really needed is for the verification method to be chosen based on the type of key returned from Keyfunc rather than based on what the token specifies. We can even continue to support returning []byte from Keyfunc, we just need to parse it before selecting a signing method. If there is a mismatch between the supplied key and the method specified in the JWT it should result in a validation failure.

Edit: on second thought this wouldn't work.

[dgrijalva]

I'd be interested in a PR to that effect. I think it's easier said than one. The alg property contains both the method and the width. If an rsa.PrivateKey is returned, we know we want jwt.SigningMethodRSA, but we don't know if it's RS256, RS512, etc.

I also don't really love the idea of trying to parse a []byte 27 different ways to figure out what's inside.

It's also worth looking at what the token specifies. If the token specifies one alg and we can infer a different one is valid, I'd still think we should reject that token.

[dgrijalva]

There's already a helper method to parse out RSA keys from PEM data. It's not that much extra code. In fact, I know some developer prefer this as it allows them to keep the parsed data cached and not add that overhead to every request.

[dgrijalva]

The thing that makes returning the expectation awkward is that there's no easy way to pass type information around. There are a few potential ways to do this:

• Have the developer return the concrete instance of the alg. This requires implementors to parse the full alg string unless they have a single type they're expecting.
• Have the developer return some other contstant (string?). They need to be different from what's in alg to void a straight passthrough and they need to contain both alg and width data. Inelegant.
• What if we did something like this:

// in the library 
type Expectation reflect.Type
const (
  ExpectedSigningMethodRSA = reflect.TypeOf(SigningMethodRS256)
  // etc
)

type Keyfunc func(*Token) (interface{}, Expectation, error)

[russellhaering]

Can you clarify the concern about passthrough? You want the API to discourage people from simply returning the contents of the alg field?

I actually prefer that because as far as I can tell it allows me to take an algorithm specified in a JSON Web Keys and return it directly.

For example, see: https://www.googleapis.com/oauth2/v2/certs

If I understand your concern correctly, I see what you're getting at but it seems like the kind of problem we can solve with docs.

[russellhaering]

On the other hand I like your suggestion, but:

1. Instead of using reflect.Type I'd just do a normal enum with iota.
2. For use cases like JSON web keys it would be useful to a lookup or map (whose docs are elaborately festooned with warnings) that returns one of those based on an alg string.

[dgrijalva]

Yes.

The implementation today is perfectly safe if you remember to check the type. We could just solve the issue with documentation. I've already updated the README.

If we're going to change the API, it should not require extensive documentation to follow correctly.

[dgrijalva]

I used reflect.Type because it saves table lookups later. It stores exactly the information we care about and it's directly comparable using the reflect library. Since they are predefined constants, users of the library shouldn't care about the type.

Additionally, the supported algs are extensible. A developer can implement their own if they type they care about is not supported. Using integer identifiers makes collisions possible external alg implementations.

[dgrijalva]

Any solution here is going to be inconvenient for someone. Security always is.

[rmullinnix]

As a developer, I don't need the full token to find the key. I end up looking for the "kid" in the header and then the Method. With those two items, I can find the key and ensure it's valid for the algorithm. If I want to use the same key for multiple algorithms, it's up to me. Returning an expectation from Keyfunc seems to put the burden on the library as opposed to the developer who should be in control of the keys.

[dgrijalva]

That's a fair point. There is a trade-off between coddling developers and providing enough guidance to use the library safely. Some things can be done with documentation. Some things should be in the API.

[Acconut]

@dgrijalva Checking the signing method in the keyfunc solves my concerns. Thanks for the quick responses 👍

[dgrijalva]

One big swing we can take here is to drop support for passing []byte to the rsa types. It doesn't completely eliminate the issue, but it reduces the likelihood a user will accidentally pass the raw bytes of a serialized public key to the hsa decoder (which is the main issue here).

[dgrijalva]

It's also more explicit. We already expose the helper methods used to decode those things, so it shouldn't add a huge burden.

[bbigras]

Any progress on this issue?

[dgrijalva]

Lots of progress in the 3.0 branch, which is pretty close to ready.

For the current version, as long as you're verifying the signing method matches your expectations, everything is fine.

[bbigras]

Excellent. Thanks.

[supafoundation]

Any updates on this? Thanks

[elithrar]

@scorpnode

• Today: validate that the signature you are using is the same signature the token is using
• In the future: track the progress of the 3.0 milestone / issue - 3.0 #75

[dgrijalva]

There's another option:

If you create an instance of Parser, you can set the valid signing methods before parsing your token.

[dgrijalva]

3.0 (#77) is landing today or tomorrow. It drops support for []byte keys for the RSA signing methods. This should close the HSA/RSA issue, but it still couldn't hurt to validate the signing method yourself.

[dgrijalva]

Resolved by #77