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 Issue]: Auth JWT does not have expiration time and is sent in the URL to all requests. #3499

Closed
4 tasks done
samwightt opened this issue Jul 5, 2023 · 7 comments
Labels
area: security bug Something isn't working

Comments

@samwightt
Copy link

samwightt commented Jul 5, 2023

Requirements

  • Is this a bug report? For questions or discussions use https://lemmy.ml/c/lemmy_support
  • Did you check to see if this issue already exists?
  • Is this only a single bug? Do not put multiple bugs in one issue.
  • Is this a backend issue? Use the lemmy-ui repo for UI / frontend issues.

Summary

Lemmy authenticates users with a JWT access token. This JWT does not expire and is only revocable if the user does an event like changing their password. This is insecure and does not follow current best practices for access tokens or for JWTs. To fix this:

  • Add some sort of session revocation mechanism.
  • Move the JWT from a URL param to a header.
  • Give JWTs a short expiration time and have frontends perform a refresh with a refresh token. Have the refresh token rotate on every refresh call.

Steps to Reproduce

Lemmy's API authenticates users using the login operation, which returns a JWT. This JWT is then passed on to all subsequent API calls in the auth param, which is sent in the URL for GET requests. While the JWT uses a good encryption algorithm, it does not contain an expiration time. There are no explicit ways for a user to revoke a particular session or JWT, but all JWTs can be revoked by doing an event like resetting your password. The JWT functions like an access token.

This breaks multiple security best practices around bearer tokens and JWTs, namely:

This is a security vulnerability. If any user JWT is stolen, that JWT will remain active until the user changes their password. Attackers will have access to a user's account for an unlimited amount of time.

Technical Details

No logs.

Version

All Lemmy versions

Lemmy Instance URL

No response

@samwightt samwightt added the bug Something isn't working label Jul 5, 2023
@samwightt
Copy link
Author

Also I may be wrong on this, I read the Claims code but there could be something I'm missing here. But on lemmy.blahaj.zone, I'm seeing a JWT sent both in the cookie and as a URL parameter.

@0xAnansi
Copy link

0xAnansi commented Jul 6, 2023

I went a bit deeper on one of your point here: #3364

@abhibeckert
Copy link

JWTs should have a short expiration time (ex. 1 hour)

Doesn't that mean you'd be logged out after one hour?

@tkroes
Copy link

tkroes commented Jul 10, 2023

That's what the refresh token is for.

@NinekoTheCat
Copy link

Please add security label to this issue

@abhibeckert
Copy link

That's what the refresh token is for.

Refresh tokens are mainly to reduce server load by avoiding the need to verify the auth token on every request.

While there can be edge cases where they help - I'm of the opinion that's more than offset by increased complexity and risk of a bad/buggy implementation, etc etc. Refresh tokens increase your attack surface.

Having said that, verifying auth tokens once an hour (by checking the refresh token) is certainly better than never verifying them, which seems to be what lemmy is doing now?

And yeah, move the tokens into a HTTP header. Preferably the cookie header - where you can take advantage of the httpOnly and secure flags.

@Nutomic
Copy link
Member

Nutomic commented Sep 28, 2023

#3946 moves auth tokens into header/cookie.

@Nutomic Nutomic closed this as completed Sep 28, 2023
Michad added a commit to Michad/lemmony that referenced this issue Jan 7, 2024
LemmyNet/lemmy#3499 removes support for JWT via URL.  This went out in 0.19. Now should be sent in via auth header. Left in old methods for backwards compatibility. with 0.18. I believe this fixes jheidecker#20
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area: security bug Something isn't working
Projects
None yet
Development

No branches or pull requests

7 participants