Skip to content
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

Gracefully Prevent Keys from Ever Being Used for the Wrong Header-Specified Algorithm #352

Closed
wants to merge 2 commits into from

Conversation

paragonie-security
Copy link

@paragonie-security paragonie-security commented Aug 4, 2021

Proposed Fix for #351

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'))
if (\is_array($key) || $key instanceof \ArrayAccess) {
/** @var Keyring|JWTKey $key */
$key = self::getKeyType($key, $allowed_algs);
if ($key instanceof Keyring) {

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why hard-code this class? It means I can't implement my own key database or just use an array of key objects. There is verification below that the key type is as expected.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Feel free to swap it out for an interface in your own implementation. This is just a proof-of-concept fix.

// See issue #351
throw new UnexpectedValueException('Incorrect key for this algorithm');
}
if (!static::verify("$headb64.$bodyb64", $sig, $key->getKeyMaterial(), $header->alg)) {

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For better BC, just cast the key to string?

@@ -121,9 +126,16 @@ public static function decode($jwt, $key, array $allowed_algs = array())
throw new UnexpectedValueException('"kid" empty, unable to lookup correct key');
}
}
if (!($key instanceof JWTKey)) {

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For BC, why not construct the object here if the key is a string and guess the alg?

*/
public function isValidForAlg($headerAlg)
{
return JWT::constantTimeEquals($this->alg, $headerAlg);

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why do you need const time comparison here? The alg is generally known or picked from a short list.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Consistency.

src/Keys/JWTKey.php Show resolved Hide resolved

// Check the signature
if (!static::verify("$headb64.$bodyb64", $sig, $key, $header->alg)) {
if (!$key->isValidForAlg($header->alg)) {

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It seems like the alg guessing code should allow multiple? e.g. if it's an RSA key type all the signature lengths should pass here. The way it looks now, if RS512 and RS256 are supported by the endpoint, and a bare string key is passed in, a request with a header alg RS256 fails?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We don't feel particularly strongly about the safety and correctness of this guessing code. We'd feel much safer if a major version bump with a small risk of BC breaks was elected instead.

Copy link
Collaborator

@bshaffer bshaffer left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The changes here causes our tests to fail in 6 different places. I think this is a significant enough number to indicate to me at least that these changes represent too large of a BC break for the current version.

I am in support of creating a major version release with these changes, and requiring the keys to represent a Key or KeyRing object instead of allowing a key without an algorithm. I think this is the better approach, rather than break a significant number of users with this change.

Thanks so much for your work on this!

public static function guessAlgFromKeyMaterial($keyMaterial, array $candidates = array())
{
$length = JWT::safeStrlen($keyMaterial);
if ($length >= 720) {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For the tests you've provided, this does not pass with the generated RSA public key. The public key is of length 272. Is this a bug or is the guessing expected to be that brittle?

it could be because the tests are generating a key with only 1024 bytes. When I change this to 4096, the tests for RSA pass.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This was a canned guess at key size. 1024-bit RSA is a bit dodgy; 3072-bit and higher is recommended.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The other thing we could do is parse the base64-encoded string and inspect the ASN.1 header. That's probably a bit more reliable. I'll send a superseding pull request that does that instead.

}
return 'RS256';
}
} elseif ($length >= 220) {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same issue here - the EC key generates with length 215, and so the tests for EC keys fail.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What curve are you using?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The test you provided!

Comment on lines +47 to +50
$this->assertSame(
'HS384',
JWTKey::guessAlgFromKeyMaterial($misc, array('HS512'))
);
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This test also fails, I'm not sure why

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks like a pasto. Line 48 should be HS512 when compared with the lines below.

@bshaffer
Copy link
Collaborator

bshaffer commented Nov 3, 2021

@paragonie-security I've decided to avoid trying to guess the algorithm, and instead make providing an algorithm supported in v5 and required in v6. Please see my PRs #365 (v5) and #364 (v6).

I think providing a quick and easy way for users to upgrade is preferable to the almost certain breakage of existing implementations if we try detecting the algorithm. Please let me know what you think, and I would love a review of the above PRs as well, if it isn't too much trouble!!

@paragonie-security
Copy link
Author

Yes. We agree that algorithm guessing is dangerous. That's why we eschewed it in the wrapper library that addresses the issue.

This PR was always intended as a proof of concept for a fix, in case you were at a loss for how to tackle the changes. We're happy to review your PRs in favor of this one.

@bshaffer
Copy link
Collaborator

bshaffer commented Nov 3, 2021

@paragonie-security Thank you so much for all of your help with this! I will await your review of my other PRs before merging.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants