warn users if someone tries to re-register their email address #1380

Closed
opened 2026-02-05 00:45:51 +03:00 by OVERLORD · 8 comments
Owner

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

if !user.password_hash.is_empty() {
    if CONFIG.mail_enabled() {
        mail::send_registration_attempt_warning(&user.email, &ip, ...).await;
    }
    err!("Registration not allowed or user already exists")
}
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](https://www.hacksplaining.com/prevention/user-enumeration) 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 https://github.com/dani-garcia/vaultwarden/blob/382e6107fe79c0828c7efeb1e05b81cf2a0f2572/src/api/core/accounts.rs#L103-L109 to something like ```rust if !user.password_hash.is_empty() { if CONFIG.mail_enabled() { mail::send_registration_attempt_warning(&user.email, &ip, ...).await; } err!("Registration not allowed or user already exists") } ```
OVERLORD added the enhancementlow priority labels 2026-02-05 00:45:51 +03:00
Author
Owner

@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.

@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.
Author
Owner

@stefan0xC commented on GitHub (Oct 10, 2022):

There's generally no specific action they can take in response to that,

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.

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.

Okay, but if I want to spam someone can I not use /#/recover-delete already?
382e6107fe/src/api/core/accounts.rs (L549-L554)

@stefan0xC commented on GitHub (Oct 10, 2022): > There's generally no specific action they can take in response to that, 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. > 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. Okay, but if I want to spam someone can I not use `/#/recover-delete` already? https://github.com/dani-garcia/vaultwarden/blob/382e6107fe79c0828c7efeb1e05b81cf2a0f2572/src/api/core/accounts.rs#L549-L554
Author
Owner

@jjlin commented on GitHub (Oct 10, 2022):

There's generally no specific action they can take in response to that,

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.

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.

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.

Okay, but if I want to spam someone can I not use /#/recover-delete already?

382e6107fe/src/api/core/accounts.rs (L549-L554)

That's true enough, but that aspect of it isn't my primary concern. Arguably, there could be an option to disable recover-delete and 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.

@jjlin commented on GitHub (Oct 10, 2022): > > There's generally no specific action they can take in response to that, > > 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. 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. > > 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. > > Okay, but if I want to spam someone can I not use `/#/recover-delete` already? > > https://github.com/dani-garcia/vaultwarden/blob/382e6107fe79c0828c7efeb1e05b81cf2a0f2572/src/api/core/accounts.rs#L549-L554 That's true enough, but that aspect of it isn't my primary concern. Arguably, there could be an option to disable `recover-delete` and 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.
Author
Owner

@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.

@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.
Author
Owner

@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.

@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. :shrug: Do you think we should maybe add the random-delay from the password-hint https://github.com/dani-garcia/vaultwarden/blob/382e6107fe79c0828c7efeb1e05b81cf2a0f2572/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.
Author
Owner

@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.

@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.
Author
Owner

@jjlin commented on GitHub (Oct 13, 2022):

I would even argue that we cloud remove that random sleep there, and add a rate-limit to it instead.

As mentioned in the comments, the sleep is to mitigate the timing side channel, not to simulate a rate limit.

@jjlin commented on GitHub (Oct 13, 2022): > I would even argue that we cloud remove that random sleep there, and add a rate-limit to it instead. As mentioned in the comments, the sleep is to mitigate the timing side channel, not to simulate a rate limit.
Author
Owner

@BlackDex commented on GitHub (Oct 13, 2022):

I would even argue that we cloud remove that random sleep there, and add a rate-limit to it instead.

As mentioned in the comments, the sleep is to mitigate the timing side channel, not to simulate a rate limit.

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.

@BlackDex commented on GitHub (Oct 13, 2022): > > I would even argue that we cloud remove that random sleep there, and add a rate-limit to it instead. > > As mentioned in the comments, the sleep is to mitigate the timing side channel, not to simulate a rate limit. 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.
Sign in to join this conversation.
1 Participants
Notifications
Due Date
No due date set.
Dependencies

No dependencies set.

Reference: starred/vaultwarden#1380