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

[stefan0xC]

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

vaultwarden/src/api/core/accounts.rs

Lines 103 to 109 in 382e610

	if !user.password_hash.is_empty() {
	if CONFIG.is_signup_allowed(&email) {
	err!("User already exists")
	} else {
	err!("Registration not allowed or user already exists")
	}
	}

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")
}

[jjlin]

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]

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?

vaultwarden/src/api/core/accounts.rs

Lines 549 to 554 in 382e610

	if CONFIG.mail_enabled() {
	if let Some(user) = User::find_by_mail(&data.Email, &conn).await {
	if let Err(e) = mail::send_delete_account(&user.email, &user.uuid).await {
	error!("Error sending delete account email: {:#?}", e);
	}
	}

[jjlin]

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?

vaultwarden/src/api/core/accounts.rs

Lines 549 to 554 in 382e610

	if CONFIG.mail_enabled() {
	if let Some(user) = User::find_by_mail(&data.Email, &conn).await {
	if let Err(e) = mail::send_delete_account(&user.email, &user.uuid).await {
	error!("Error sending delete account email: {:#?}", e);
	}
	}

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.

[BlackDex]

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]

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

vaultwarden/src/api/core/accounts.rs

Lines 633 to 642 in 382e610

	// To prevent user enumeration, act as if the user exists.
	if CONFIG.mail_enabled() {
	// There is still a timing side channel here in that the code
	// paths that send mail take noticeably longer than ones that
	// don't. Add a randomized sleep to mitigate this somewhat.
	use rand::{rngs::SmallRng, Rng, SeedableRng};
	let mut rng = SmallRng::from_entropy();
	let delta: i32 = 100;
	let sleep_ms = (1_000 + rng.gen_range(-delta..=delta)) as u64;
	tokio::time::sleep(tokio::time::Duration::from_millis(sleep_ms)).await;

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]

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]

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.

[BlackDex]

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.