Add Security Warning while using for_user method

[dmdhrumilmistry]

Add security warning in for_user documentation to avoid introducing vulnerability in other applications.

Security Warning: validate whether user is active or not before generating token for the user. This will only create token for the user, it doesn't perform user validation whether user is active or not.

[Andrew-Chen-Wang]

Thanks for filing. cc @robd003

This is more of a documentation thing as using .for_user directly is non-standard, but because of my very own telling to everyone to just use SimpleJWT classes directly, this mistake is attributed to me. Unfortunately, it works by design and is following DRY principles and separation of duties where the validation takes place on the serializer part, which is Django standard, but I'll add some documentation to warn users.

[sevdog]

This is an issue only if used with JWTStatelessUserAuthentication/JWTTokenUserAuthentication, while JWTAuthentication class is not affected since it queries the database to check if ther user is active.

The "bad configuration" is:

whenever the Token.for_user method is used without prior user validation and JWTStatelessUserAuthentication or JWTTokenUserAuthentication (which is an alias of ther previous one) authentication class is used.

[dmdhrumilmistry]

Thanks for filing. cc @robd003

This is more of a documentation thing as using .for_user directly is non-standard, but because of my very own telling to everyone to just use SimpleJWT classes directly, this mistake is attributed to me. Unfortunately, it works by design and is following DRY principles and separation of duties where the validation takes place on the serializer part, which is Django standard, but I'll add some documentation to warn users.

I understand but there are several projects using it directly without any kind of user's active status validation which makes those projects vulnerable.

[Andrew-Chen-Wang]

I'm all for adding a logger.error and logger.critical, but outside of that is changing functionality and design principles. I'm not used to CVE reports and actionables in an open source context, but this is my understanding of general good design and consistent behavior for developers. Let me know if that's not how I should be approaching this and why.

[dmdhrumilmistry]

I'm all for adding a logger.error and logger.critical, but outside of that is changing functionality and design principles. I'm not used to CVE reports and actionables in an open source context, but this is my understanding of general good design and consistent behavior for developers. Let me know if that's not how I should be approaching this and why.

Adding logs can help users to avoid such issues. If possible let's also add more secure examples in docs so it doesn't introduce vulns in their projects.

[igorer88]

@Andrew-Chen-Wang Why don't add an exception raised if using .for_user directly?

As @dmdhrumilmistry says

there are several projects using it directly without any kind of user's active status validation which makes those projects vulnerable.

and as the vuln doc says:
CVE-2024-22513

so people doesn't have to check this manually:
if user and user.is_active:

Because this line in the vuln makes it important I think:

This feature introduces the ability to use different subclasses of the Token class, such as RefreshToken, SlidingToken, AccessToken, and UntypedToken with for_user method. However, this enhancement also expands the potential attack surface.

And it won't be shown on vuln scanners.

[doctornkz-intelas]

Hello folks. How can we go forward with this issue? Our DataDog is giving MEDIUM level for this CVE and it's hard to ignore such cases. We are safe with that, but the issue is increasing noise from our monitoring. Thank you for your effort.

[alimony]

Adding that it might be a legal requirement for some users not to have this active CVE, for compliance reasons.

[Andrew-Chen-Wang]

Why don't add an exception raised if using .for_user directly?

There are valid use cases for utilizing .for_user directly.

so people doesn't have to check this manually:
if user and user.is_active:

Some project don't use is_active flag such as for GDPR reasons where a full deletion is required.

We have a utility user validation check function, but, as noted before, the problem is utilizing the experimental JWTStatelessUserAuthentication. This means implementing the validation check would incur a database query which completely defeats the purpose of the class.

Again, this is a documentation error, that I or any willing users can make a PR for during free time.

[Andrew-Chen-Wang]

Adding that it might be a legal requirement for some users not to have this active CVE, for compliance reasons.

Definitely a valid reason that this should be ack-ed. If it's not really noticeable yet, I've been fairly disengaged due to constrained time for a solid 2 years around SimpleJWT. If anyone would like to assist with a PR (I think our RTDocs are also broken possibly?), I'm happy to take a look.

[marcofucci]

I'm happy to spend some time on this, but I need some help understanding what the real problem is.

As far as I can see, by design, we separate the logic of validating the user from creating the token, following the standard Django practice where models are kept as simple as possible, and views and serializers implement the actual business logic. That's why for_user does not check if the user is valid and instead assumes that it was already validated somewhere else. This is indeed the case, as you cannot call TokenObtainPairView or TokenRefreshView with an inactive user to generate a token.

As the documentation also states, you need to use JWTAuthentication as the DRF authentication class. Even if you manage to generate a valid token for an inactive user somehow, you would not be able to use it to access protected resources.

Changing the for_user method also doesn't solve the underlying problem that the CVE seems to refer to, as you can call for_user to generate a token for an active user and then immediately disable the user.

It also looks like JWTStatelessUserAuthentication is meant to be used in situations where you don't have a database user, so again, the authentication should happen prior to generating the token, e.g., by SSO.

I also checked a few of the projects mentioned in the CVE that are believed to be vulnerable, and they all seem to use JWTAuthentication. This means that they are not vulnerable, as the user is validated before accessing protected resources, even if somehow you manage to generate a token for an invalid user.

So the TL;DR version is that, if you follow the documentation, there are no security issues with inactive users.
I'm not sure I agree with the severity level of this CVE being medium, it should probably be info or very low.

With that being said, I think we could make the following changes:

1. Change the documentation to warn users that if they call for_user manually, they need to validate the user separately first (e.g., use a serializer provided by djangorestframework-simplejwt or do so manually).
2. We could potentially add a log warning in for_user if the user is inactive so that devs are aware of it if they have missed the previous point.
3. Potentially, I noticed that the TokenVerifyView doesn't authenticate the user, so the endpoint could return True even if the user is invalid. This could be misleading. Maybe we could change the related serializer. I'm not sure why we need this verify view, as any dummy DRF View with JWTAuthentication would be enough to validate a token.

@dmdhrumilmistry could you shed some light on this please as I might have missed something?
If this is correct, could we try change the severity level of this CVE?

[dmdhrumilmistry]

I'm happy to spend some time on this, but I need some help understanding what the real problem is.

As far as I can see, by design, we separate the logic of validating the user from creating the token, following the standard Django practice where models are kept as simple as possible, and views and serializers implement the actual business logic. That's why for_user does not check if the user is valid and instead assumes that it was already validated somewhere else. This is indeed the case, as you cannot call TokenObtainPairView or TokenRefreshView with an inactive user to generate a token.

As the documentation also states, you need to use JWTAuthentication as the DRF authentication class. Even if you manage to generate a valid token for an inactive user somehow, you would not be able to use it to access protected resources.

Changing the for_user method also doesn't solve the underlying problem that the CVE seems to refer to, as you can call for_user to generate a token for an active user and then immediately disable the user.

It also looks like JWTStatelessUserAuthentication is meant to be used in situations where you don't have a database user, so again, the authentication should happen prior to generating the token, e.g., by SSO.

I also checked a few of the projects mentioned in the CVE that are believed to be vulnerable, and they all seem to use JWTAuthentication. This means that they are not vulnerable, as the user is validated before accessing protected resources, even if somehow you manage to generate a token for an invalid user.

So the TL;DR version is that, if you follow the documentation, there are no security issues with inactive users. I'm not sure I agree with the severity level of this CVE being medium, it should probably be info or very low.

With that being said, I think we could make the following changes:

1. Change the documentation to warn users that if they call `for_user` manually, they need to validate the user separately first (e.g., use a serializer provided by djangorestframework-simplejwt or do so manually).

2. We could potentially add a log warning in `for_user` if the user is inactive so that devs are aware of it if they have missed the previous point.

3. Potentially, I noticed that the `TokenVerifyView` doesn't authenticate the user, so the endpoint could return True even if the user is invalid. This could be misleading. Maybe we could change the related serializer. I'm not sure why we need this verify view, as any dummy DRF View with `JWTAuthentication` would be enough to validate a token.

@dmdhrumilmistry could you shed some light on this please as I might have missed something? If this is correct, could we try change the severity level of this CVE?

CVE will only affect the projects that aren't following best practices and writing their custom authenticators without validating user status. The suggested changes seem valid to me.

I haven't assigned any severity in NIST database, ratings were provided by the analysts.

I've submitted an update in the GitHub advisory to mark vuln as low severity.

github/advisory-database#4582

[marcofucci]

Thanks, @dmdhrumilmistry and please keep us updated!

To rectify my previous summary, it looks like only TokenObtainPairSerializer and TokenObtainSlidingSerializer check for user.is_active, while TokenRefreshSerializer and TokenRefreshSlidingSerializer do not. This doesn't change the overall security impact, as even if it seems like you can refresh a token related to an inactive user (if you managed to obtain a token somehow), you still can't use it to access protected resources.

However, it would be beneficial to change these two serializers to behave like the TokenObtain*Serializers for consistency and a better user experience.

Therefore, I would add one more item to the list of improvements:

4. Update TokenRefreshSerializer and TokenRefreshSlidingSerializer to return an error if the user is inactive, mirroring what TokenObtainPairSerializer and TokenObtainSlidingSerializer do.

As the list above is not required for the change to the CVE severity score, I think it's not an imminent priority so we could wait a bit longer to see if anyone has anything else to add before we start working on it.

[doctornkz-intelas]

Thank you @marcofucci and @dmdhrumilmistry for your effort. We know how sometimes mentally tough is to manage open-source projects. Thank you for your contribution.
The explanation by @marcofucci related to the Django way is totally clear. One method shouldn't be fully responsible for other contexts inside other methods or external integrations. It would be a mess around the code. I am very sorry about applying the pressure here and there.
If we see another way to solve our issues with CVE monitoring systems (like DataDog, Docker, name it, scanners) instead of dramatically changing the code - I am personally ok with that.

[dmdhrumilmistry]

Thanks, @dmdhrumilmistry and please keep us updated!

To rectify my previous summary, it looks like only TokenObtainPairSerializer and TokenObtainSlidingSerializer check for user.is_active, while TokenRefreshSerializer and TokenRefreshSlidingSerializer do not. This doesn't change the overall security impact, as even if it seems like you can refresh a token related to an inactive user (if you managed to obtain a token somehow), you still can't use it to access protected resources.

However, it would be beneficial to change these two serializers to behave like the TokenObtain*Serializers for consistency and a better user experience.

Therefore, I would add one more item to the list of improvements:

4. Update `TokenRefreshSerializer` and `TokenRefreshSlidingSerializer` to return an error if the user is inactive, mirroring what `TokenObtainPairSerializer` and `TokenObtainSlidingSerializer` do.

As the list above is not required for the change to the CVE severity score, I think it's not an imminent priority so we could wait a bit longer to see if anyone has anything else to add before we start working on it.

The severity has been changed to low in GHSA Database