tekul/jose-jwt

decode trusts header info

Closed this issue · 3 comments

There is a critical vulnerability in most jwt implementations

https://auth0.com/blog/2015/03/31/critical-vulnerabilities-in-json-web-token-libraries/

The decode function looks vulnerable as well, because the set of decode keys is decided based on untrusted header information.

tekul commented

Thanks for the heads up.

The intention is that the caller should check whether the header matches the expected algorithms and strengths on decoding, which is why the decode function returns the additional information (the JwtContent type, rather than just the claims. I should make this clearer in the documentation. If you pattern match on the result, you can reject a JWT with the "none" algorithm, because it will match Unsecured claims. Here's a similar example using the decodeClaims function:

https://github.com/tekul/broch/blob/2c94c0910826132cfd014f6880106f3c86da303d/Broch/OAuth2/ClientAuth.hs#L74

Since the header type makes it clear the token is unsecured, it is rejected. I think additionally the decode function should return an error if keys are supplied and the algorithm is "none", as the article suggests. The "none" algorithm makes code more complicated in general and I wish they'd just remove it from the specs.

I think the other case it mentions, where the algorithm is switched, making the verifier use an RSA public key as an HMAC key should be prevented by the type-safety of the JWKs used. It isn't possible to use an RSA JWK with HMAC.

What do you think?

tekul commented

After thinking about this a bit more, for all my current use cases I know the algorithm up front (e.g. an OpenID Connect client registers with a specific algorithm, or a default is used), so it makes sense to supply this information to the decode function, rather than checking it externally. I'll introduce a JwtEncoding type, containing the algorithm information, which can be passed to both encode and decode. This will also save checking the enc value separately which is messy.

tekul commented

I've changed the encode and decode API as described in the previous comment. It's optional with the decode function, which takes a Maybe JwtEncoding, but the function will also now only decode Unsecured JWTs if the algorithm is explicitly supplied, otherwise it returns an error.