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

[SECURITY] Algorithm Confusion Through kid Header #235

Closed
paragonie-security opened this issue Aug 20, 2021 · 1 comment
Closed

[SECURITY] Algorithm Confusion Through kid Header #235

paragonie-security opened this issue Aug 20, 2021 · 1 comment

Comments

@paragonie-security
Copy link

First, let us note that the alg header is correctly being validated.

jwcrypto/jwcrypto/jwt.py

Lines 483 to 485 in ea36353

# Apply algs restrictions if any, before performing any operation
if self._algs:
self.token.allowed_algs = self._algs

However, if a kid header is being used by an application, jwcrypto will pass a key to the cryptographic verification routine without first asserting that the key's intended algorithm matches the token header's.

jwcrypto/jwcrypto/jwt.py

Lines 495 to 519 in ea36353

elif isinstance(key, JWKSet):
self.token.deserialize(jwt, None)
if 'kid' in self.token.jose_header:
kid_key = key.get_key(self.token.jose_header['kid'])
if not kid_key:
raise JWTMissingKey('Key ID %s not in key set'
% self.token.jose_header['kid'])
self.token.deserialize(jwt, kid_key)
else:
for k in key:
try:
self.token.deserialize(jwt, k)
self.deserializelog.append("Success")
break
except Exception as e: # pylint: disable=broad-except
keyid = k.get('kid')
if keyid is None:
keyid = k.thumbprint()
self.deserializelog.append('Key [%s] failed: [%s]' % (
keyid, repr(e)))
continue
if "Success" not in self.deserializelog:
raise JWTMissingKey('No working key found in key set')
else:
raise ValueError("Unrecognized Key Type")

jwcrypto/jwcrypto/jwk.py

Lines 1192 to 1199 in ea36353

def get_key(self, kid):
"""Gets a key from the set.
:param kid: the 'kid' key identifier.
"""
for jwk in self['keys']:
if jwk.get('kid') == kid:
return jwk
return None

This is congruent to the problem in firebase/php-jwt#351 https://seclists.org/fulldisclosure/2021/Aug/14

Note: This particular sharp edge isn't covered by the JWT Best Practices RFC.

@simo5
Copy link
Member

simo5 commented Aug 23, 2021

So first of all, this project has a clear security policy you DID NOT follow: https://github.com/latchset/jwcrypto/security/policy

Secondly, if you look at the code you are pointing at you will see that in absence of a kid we try all keys.

This is because we defer validation of key type to the algorithm handling which uses typed keys to handle cryptographic operations, via the get_op_key function:

def get_op_key(self, operation=None, arg=None):

This function returns a python cryptography object for all cases except the 'oct' type. In the 'oct' case a base64 encoded string is returned, and all calling functions try to either decode it, or use a key object. If pyca object is passed to base64_decode then the code will retuen an exception. if a byte64 encoded string is used in a EC or RSA operation then when the object is dereferenced to use a sign() or verify() method it will also throw an exception. And if a RSA key is used with an EC operation (or vice versa) then pyca will assert.

We could probably pass the expected key type to the get_op_key() function, but that would just return a nicer error message, it is not required for security.

Please do better or I will automatically close reports from you in the future, I am not here to waste time on poor research.

If you think I missed something, please provide a reproducer, this is python and json objects, it is very simply to provide me with a token, keys and 3 lines of code to test out anything (See the examples in the documentation).

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

No branches or pull requests

2 participants