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

Limit password resets #3344

Merged
merged 1 commit into from Jun 27, 2023
Merged

Limit password resets #3344

merged 1 commit into from Jun 27, 2023

Conversation

sunaurus
Copy link
Collaborator

@sunaurus sunaurus commented Jun 25, 2023

This PR limits password reset e-mails to 3 per 24h, which will severely reduce the impact of #3320 (which is quite a critical issue).

I believe having 3 resets per 24h should cover 99.99% of legit usage, but I can imagine some edge cases where it's not enough. In the longer term, it would be nice to make this limit a configuration option for instance admins.

@sunaurus sunaurus force-pushed the password_reset_limit branch 4 times, most recently from 2ddd77a to 10eb03a Compare June 26, 2023 11:01
@Nutomic
Copy link
Member

Nutomic commented Jun 26, 2023

Instead of writing custom logic, it should be enough to use the same rate limit wrapper which is used for registration.

https://github.com/LemmyNet/lemmy/blob/main/src/api_routes_http.rs#L272

@sunaurus
Copy link
Collaborator Author

sunaurus commented Jun 26, 2023

The built-in rate limiter is a bit limited (😛) for this case in key ways:

  1. It's IP based, not e-mail based (and it doesn't seem trivial to add support for e-mail based logic) - meaning a single e-mail address could still get spammed in a distributed attack
  2. It's very hard to configure when scaling horizontally, as each server has its own separate memory of who should be rate limited
  3. It's not persistent, so the hourly rate limit bucket cleanup (or just a server restart, for example) will allow another batch of e-mails for all potential distributed attackers again

Instance admins can work around 2. and 3. with external rate limiters (like nginx or cloudflare). 1. is harder and more expensive to work around externally, but quite simple to do in Lemmy code itself.

By the way @Nutomic, this issue is already being exploited in the wild - before I applied this patch on lemm.ee, I had one e-mail address spammed with nearly 1000 of these duplicate e-mails before I managed to detect it.

@sunaurus
Copy link
Collaborator Author

Ah, and a fourth downside of using the built-in rate limiter:
4. A malicious user on a shared IP address could permanently DOS the password reset feature for all other users on the same IP - if they persistently use up all the allowed attempts and get their IP rate limited, then no other user from that IP will ever be able to reset their password.

Copy link
Member

@dessalines dessalines left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Seems simple and good to me, thx!

I'll let @Nutomic also approve before merging.

@Nutomic Nutomic merged commit 76a4513 into LemmyNet:main Jun 27, 2023
1 check passed
@sunaurus sunaurus deleted the password_reset_limit branch June 27, 2023 11:11
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

Successfully merging this pull request may close these issues.

None yet

3 participants