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

Library does not automatically verify that the token hasn't been tampered with #784

Closed
Ichicoro opened this issue Feb 12, 2024 · 8 comments

Comments

@Ichicoro
Copy link

I have to report a (quite big) issue/vulnerability, assuming it's not misconfiguration on our end 😅

It seems that the library doesn't do verification of the JWT's signature. I tampered with one token's payload and used it as my Bearer token (without correctly regenerating the signature, I just left it the same as it was before) and the library validated my forged token. I even tried having different SIGNING_KEY and VERIFYING_KEY and the Django application worked flawlessly, so there definitely is no verification of the validity of the token.

I fixed this by overriding our authentication class with a custom-made one that inherits JWTCookieAuthentication

import jwt
from dj_rest_auth.app_settings import api_settings
from dj_rest_auth.jwt_auth import JWTCookieAuthentication
from django.conf import settings
from rest_framework.exceptions import AuthenticationFailed
from rest_framework_simplejwt import settings as jwt_settings


class FixedJWTCookieAuthentication(JWTCookieAuthentication):
    def authenticate(self, request):
        cookie_name = api_settings.JWT_AUTH_COOKIE
        header = self.get_header(request)
        if header is None:
            if cookie_name:
                raw_token = request.COOKIES.get(cookie_name)
                # True at your own risk
                if api_settings.JWT_AUTH_COOKIE_ENFORCE_CSRF_ON_UNAUTHENTICATED:
                    self.enforce_csrf(request)
                elif raw_token is not None and api_settings.JWT_AUTH_COOKIE_USE_CSRF:
                    self.enforce_csrf(request)
            else:
                return None
        else:
            raw_token = self.get_raw_token(header)

        if raw_token is None:
            return None

        verifying_key = jwt_settings.api_settings.user_settings.get("VERIFYING_KEY")
        if verifying_key is None:
            verifying_key = jwt_settings.api_settings.user_settings.get("SIGNING_KEY")
        if verifying_key is None:
            verifying_key = settings.SECRET_KEY

        try:
            validated_token = jwt.decode(raw_token, verifying_key, algorithms=["HS256"])
        except jwt.InvalidSignatureError:
            raise AuthenticationFailed("Invalid JWT signature")
        except jwt.ExpiredSignatureError:
            raise AuthenticationFailed("JWT signature has expired")
        except jwt.DecodeError:
            raise AuthenticationFailed("Error decoding JWT")
        except:  # noqa E722
            raise AuthenticationFailed("Invalid JWT token")

        return self.get_user(validated_token), validated_token
@robd003
Copy link

robd003 commented Mar 19, 2024

@Andrew-Chen-Wang @aqeelat Can we get a response to this? Seems like a glaring security issue.

GHSA-5vcc-86wm-547q
https://github.com/dmdhrumilmistry/CVEs/tree/main/CVE-2024-22513

@Andrew-Chen-Wang
Copy link
Member

well shit

@Andrew-Chen-Wang
Copy link
Member

@robd003 thanks for pinging me. let's start the the filed CVE in your comment (which I'll repost in the dup issue):

re: GHSA-5vcc-86wm-547q there's another issue that'll track that: #779


re: this issue, thanks for filing @Ichicoro

This is surprising. I'm not entirely sure what the difference is between your Authentication class and the current one?

The current one runs:

  1. get_validated_token which creates the token class
  2. the constructor of the token class grabs the token backend then runs .decode
  3. The decoder method runs validated_token = jwt.decode(raw_token, verifying_key, algorithms=["HS256"])

I have two questions:

  1. Did you in any way modify your authentication class or use a derivative when pen testing?
  2. How did you modify your token payload?

@chickahoona
Copy link

Sorry if I am stepping in yet @Andrew-Chen-Wang analysis seems to be spot on.

For others to follow:

There is no try catch block in between that would catch / hide one of the errors...

There are of course options why its not workign for @Ichicoro, for example if he overwrote e.g. AUTH_TOKEN_CLASSES and is not calling there the .decode in the constructor or is using a different backend that doesn't validate things.

@Ichicoro could you elaborate on that. Andrew won't be able to fix things if he cannot reproduce things.

@stephendwolff
Copy link

I was having a look into this, and i noticed the JWTCookieAuthentication is part of dj_rest_auth rather than this project. @Ichicoro why did you use this instead of the simplejwt JWTAuthentication class?

@Andrew-Chen-Wang
Copy link
Member

@stephendwolff dj_rest_auth is designed for SPAs like standalone React. dj_rest_auth utilizes SimpleJWT under the hood. There are multiple possibilities this is happening such as a improper implementation of the actual cookie handoff (though I doubt since it seems OP tested this via an external tool) or misconfiguration as noted by chickahoona.

I'll keep this open for a bit to allow OP to follow up with more code configuration, but I'll have to mark this as invalid as it doesn't seem possible. I definitely think simplejwt blacklist could have an effect as there are a ton of variables (in production, not locally) that have unknown side effects, but this wasn't noted.

@stephendwolff
Copy link

@Andrew-Chen-Wang ah ok - thanks for the explanation. I was trying to work out if it was applicable for an API i'm working on for an app (Flutter i think) - i think probably not.

@Andrew-Chen-Wang
Copy link
Member

Closing this with no response from OP and improbability of this happening with OOTB SimpleJWT. Feel free to re-open with follow-up details.

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

No branches or pull requests

5 participants