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

JWS signature verification broken when algorithms is a plain string #308

Open
vivienm opened this issue Jan 16, 2023 · 2 comments
Open

JWS signature verification broken when algorithms is a plain string #308

vivienm opened this issue Jan 16, 2023 · 2 comments

Comments

@vivienm
Copy link

vivienm commented Jan 16, 2023

Hello!

I'm having a small issue with functions jwt.decode and jws.verify in a corner-case scenario.

I'm playing with a handcrafted, invalid JWT where the "alg" header has been set to true (not allowed to share, sorry!):

$ echo "$JWT" | cut -d '.' -f 1 | base64 -d
{"alg":true,"typ":"JWT"}

When trying to decode or verify it, it fails with a TypeError. I'd have expected a JWSError instead:

>>> jws.verify(os.environ["JWT"], "", algorithms="HS256")
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
  File ".../jose/jwt.py", line 142, in decode
    payload = jws.verify(token, key, algorithms, verify=verify_signature)
  File ".../jose/jws.py", line 73, in verify
    _verify_signature(signing_input, header, signature, key, algorithms)
  File ".../jose/jws.py", line 256, in _verify_signature
    if algorithms is not None and alg not in algorithms:
TypeError: 'in <string>' requires string as left operand, not bool

This happens when the argument algorithms is a string. With a list, this seems to behave normally:

>>> jws.verify(os.environ["JWT"], "", algorithms=["HS256"])
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
  File ".../jose/jws.py", line 73, in verify
    _verify_signature(signing_input, header, signature, key, algorithms)
  File ".../jose/jws.py", line 257, in _verify_signature
    raise JWSError("The specified alg value is not allowed")
jose.exceptions.JWSError: The specified alg value is not allowed

Still, according to docs, both strings and lists of strings are valid types for algorithms:

python-jose/jose/jws.py

Lines 48 to 55 in 96474ec

def verify(token, key, algorithms, verify=True):
"""Verifies a JWS string's signature.
Args:
token (str): A signed JWS to be verified.
key (str or dict): A key to attempt to verify the payload with. Can be
individual JWK or JWK set.
algorithms (str or list): Valid algorithms that should be used to verify the JWS.

I believe the root issue is here:

python-jose/jose/jws.py

Lines 250 to 257 in 96474ec

def _verify_signature(signing_input, header, signature, key="", algorithms=None):
alg = header.get("alg")
if not alg:
raise JWSError("No algorithm was specified in the JWS header.")
if algorithms is not None and alg not in algorithms:
raise JWSError("The specified alg value is not allowed")

algorithms may be a plain string here (I have not seen any list normalization before this code is reached). So the expression alg not in algorithms tries to assess whether a boolean (in my scenario) belongs to a string, and fails with an unexpected TypeError.

Even in a normal scenario, with alg being a string, I'm not sure this code is correct either. If algorithms is a plain string, the test will assess whether alg is a substring of algorithms. This looks wrong: its both too permissive and differs from the list behavior.

I'm not familiar with this project, but I believe a fix may look like

     if algorithms is not None:
         if isinstance(algorithms, str):
             algorithms=[algorithms]
         if alg not in algorithms:
             raise JWSError("The specified alg value is not allowed") 
@almartmart
Copy link

The point here might be that "alg" header is expecting to contain a string rather than a boolean. Same with the string value, as it is expecting a valid algorithm. Expecting a proper behaviour on a library might require a proper usage of it, so I don't think this code needs to be added. It may just be a good contribution to the Documentation.

@vivienm
Copy link
Author

vivienm commented Jan 19, 2023

I beg to differ: JWTs are typically sent by clients, and should be considered as untrusted input. I agree the JWT is invalid, but I think a proper exception should be raised, not a generic, undocumented one.

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