mirror of
https://github.com/dani-garcia/vaultwarden.git
synced 2026-02-05 00:29:40 +03:00
warn users if someone tries to re-register their email address #1380
Reference in New Issue
Block a user
Delete Branch "%!s()"
Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?
Originally created by @stefan0xC on GitHub (Oct 10, 2022).
When signup is enabled it's possible to check if a user already exists for an attacker by registration which (while not practical) makes it in principle possible to enumerate users that way.
I've already made a pull request #2799 which improves the situation a bit insofar that it would get rid of the more explicit
"User already exists"error message but the security could be further improved by maybe rate-limiting the registration as well as simply sending the user a mail if someone tries to re-register their email address.So my idea would be to change the currently redundant check
382e6107fe/src/api/core/accounts.rs (L103-L109)to something like@jjlin commented on GitHub (Oct 10, 2022):
I don't think it's useful to send an email telling a user someone tried registering using their email address. There's generally no specific action they can take in response to that, and it opens up a way for someone to spam a user with these emails and/or rack up charges if that instance uses an email-sending service that bills based on email volume.
@stefan0xC commented on GitHub (Oct 10, 2022):
Well, I think this would depend on the message e.g. we could suggest to enable 2FA if they haven't or to inform the administrator if it wasn't them.
Okay, but if I want to spam someone can I not use
/#/recover-deletealready?382e6107fe/src/api/core/accounts.rs (L549-L554)@jjlin commented on GitHub (Oct 10, 2022):
I think recommending 2FA is mostly orthogonal to failed registration. That would make more sense in response to an incorrect password attempt, though I wouldn't support a notification for that either.
I feel strongly that this email notification would just be an annoyance to the user, much as getting an email for every incorrect password attempt would be. Asking the user to inform the admin just additionally creates an annoyance to the admin, as there isn't much they can do either. It would be better to have a per-user security event log that could display such events, and I believe people have opened feature requests with Bitwarden related to this, but they haven't gotten around to implementing it yet.
You had mentioned rate limiting registrations previously. There is a rate limiting mechanism already built into Vaultwarden, but it's only used for login attempts currently. I agree it could be reasonable to apply a rate limit for registrations too, though perhaps with its own configurable limit.
That's true enough, but that aspect of it isn't my primary concern. Arguably, there could be an option to disable
recover-deleteand have the admins manually delete accounts as needed, but AFAIK, no one has yet reported a real-world issue involving that. As with a lot of things, it might be only a matter of time.@BlackDex commented on GitHub (Oct 10, 2022):
I agree with @jjlin that warning them isn't something I would support either.
A rate limit would be nice maybe, especially for open environments, though, i doubt that much will have it open.
But it wouldn't hurt though to have a rate limit on either (or both) the register or recover-delete.
@stefan0xC commented on GitHub (Oct 13, 2022):
I don't see how that would annoy people as I think it is very unlikely to ever occur (and for users that try to register they might find it useful to get reminded of their account) and I think the issues you have raised could be addressed by making this feature optional but like I said in the first place this highly impractical for an attacker and I don't think this a likely attack vector either and the time and effort needed to implement this change is probably not worth it. 🤷
Do you think we should maybe add the random-delay from the password-hint
382e6107fe/src/api/core/accounts.rs (L633-L642)to register and recover-delete to mitigate user enumeration? I guess that would be a simple change that's easier to implement than rate-limiting.@BlackDex commented on GitHub (Oct 13, 2022):
If someone can't register, they should just contact the admin or the one who invited them. They needs to check what is going on.
When someone forgot or clicked a wrong button, i would want to receive a mail that i did that my self, or someone else.
Adding a random sleep to both of them isn't going to help either and will only make the process slower.
I would even argue that we cloud remove that random sleep there, and add a rate-limit to it instead.
@jjlin commented on GitHub (Oct 13, 2022):
As mentioned in the comments, the sleep is to mitigate the timing side channel, not to simulate a rate limit.
@BlackDex commented on GitHub (Oct 13, 2022):
I know, but if a rate-limit would help also. Depending on how strict it is set. But it doesn't hurt keeping it there.