-
Notifications
You must be signed in to change notification settings - Fork 1.3k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Possibility of Reintroducing HS256/RSA256 Type Confusion (CVE-2021-46743) #351
Comments
This aims to provide backwards compatibility by guessing the algorithm for a key based on the key's contents, but it will likely fail in corner cases. If this is merged, users **SHOULD** be explicit about the algorithms they're using. i.e. Instead of $keyAsString, pass in (new JWTKey($keyAsString, 'ES384'))
This aims to provide backwards compatibility by guessing the algorithm for a key based on the key's contents, but it will likely fail in corner cases. If this is merged, users **SHOULD** be explicit about the algorithms they're using. i.e. Instead of $keyAsString, pass in (new JWTKey($keyAsString, 'ES384'))
#352 contains a first draft pull request that may address this in a mostly backwards-compatible manner, but there's some thorny corner cases where it might fail (especially trying to distinguish between EdDSA vs HS256). |
Attached is a sample vulnerable application and proof of concept for this issue. |
If anyone needs an immediate, short-term mitigation for this issue, see https://github.com/paragonie/php-jwt-guard |
@paragonie-security can't thank you enough for your work on this. I'll try to get your PR merged shortly (and the documentation updated) and a major version out which breaks BC and requires the more secure alg/key paired format |
This is a follow-up to the HS256/RS256 Type Confusion attack against the JWT protocol.
Now, firebase/php-jwt attempts to side-step this risk by forcing the user to hard-code the algorithms they wish to support.
php-jwt/src/JWT.php
Lines 103 to 108 in d2113d9
If
$key
is an array, and$header
contains akid
field, the key used to verify a token is determined by thekid
header.php-jwt/src/JWT.php
Lines 114 to 123 in d2113d9
Reintroducing the Vulnerability
EDIT: 2021-08-11 - This example is a bit misleading. See the attached Proof of Concept file instead. php-jwt-poc.zip
Let's say you're a service that wants to check
HS256
tokens against one key type andRS256
tokens against another. YourHS256
key has{"kid":"gandalf0"}
, while yourRS256
public key has{"kid":"legolas1"}
.You might call php-jwt like so:
If anyone ever sets up JWT like this:
Congratulations! you've just reintroduced the critical vulnerability in your usage of the app.
All you have to do is set
{"alg":"HS256","kid":"legolas1"}
and use the SHA256 hash of the RSA public key as an HMAC key, and you can mint tokens all day long.Another Way To Setup This Vulnerability
Let's say you have two different endpoints that only each accept one JWT signature algorithm.
/oauth
only allowsRS256
tokens/rpc
only allowsHS256
tokensThis is clearly a safer usage of the firebase/php-jwt library than the previous canned example.
Suppose you have a universal array that your application uses that maps keys--as is common with PHP frameworks.
In this setup, once again, you've introduced a critical vulnerability into your application.
All an attacker needs to do is target your
/rpc
endpoint and swap the Key ID fromgandalf0
tolegolas1
and they can mint tokens.What's going on here?
The fundamental problem is that the keys passed to firebase/php-jwt are just strings. This flies in the face of cryptography engineering best practices: A key should always be considered to be the raw key material alongside its parameter choices.
Is this a security vulnerability?
This is not a vulnerability in the firebase/php-jwt library. It is, however, a very sharp edge that an unsuspecting developer could cut themselves on.
Cryptography should be easy to use, hard to misuse, and secure by default.
Whether the JOSE authors want to acknowledge it or not, what they published was a cryptographic protocol--one that fails to live up to these tenets. It's worth noting that PASETO mitigates this in its specification, so library authors don't have to even worry about it.
Any application that uses this library in the way described above has a critical vulnerability, so it may be prudent to publish a security advisory and/or obtain a CVE identifier.Update: This was assigned CVE-2021-46743The good news is: This can be easily fixed.
The bad news is: It constitutes a backwards compatibility break.
How to Fix This Library
If you were to update the API to require keys to be a
Keyring
object, which maps a string KeyID (kid
) to aJWTKey
object--and thatJWTKey
object had a hard-coded algorithm that it could be used with--then this issue would be easily avoided.Pseudocode
Edited to clarify the value of a security advisory and/or CVE to ensure users of this library remain safe against attack.
The text was updated successfully, but these errors were encountered: