ASP.NET Core Identity samples and Identity.UI reveal the user exists via password reset

[Eirenarch]

Is there an existing issue for this?

• [X] I have searched the existing issues

Describe the bug

I might be missing something but it seems to me that both the MVC Sample of ASP.NET Core Identity and the Identity.UI project contain a bug that allows someone to find if an email is registered in the system. Both contain this code with an actual comment about revealing if the user exists

var user = await _userManager.FindByEmailAsync(model.Email);
if (user == null)
{
    // Don't reveal that the user does not exist
    return RedirectToAction(nameof(AccountController.ResetPasswordConfirmation), "Account");
}
var result = await _userManager.ResetPasswordAsync(user, model.Code, model.Password);
if (result.Succeeded)
{
    return RedirectToAction(nameof(AccountController.ResetPasswordConfirmation), "Account");
}
AddErrors(result);
return View();

However I can input an email with a random code and if I get redirected to the password confirmation error I know the account does not exist if I get an error about wrong code I know the account exists. This code seems to do nothing useful while making the whole thing annoying to the user (if they mistype the email they are told they changed the password successfully). On a side note is there a reason why the email input in a text box rather than sent in the URL?

I believe the code should be changed to something along the lines of

if (user is null)
{
    // Don't reveal that the user does not exist
    ModelState.AddModelError(String.Empty, new IdentityErrorDescriber().InvalidToken().Description);
    return View();
}

Expected Behavior

I expect the error for wrong code and for account which does not exist to be the same

Steps To Reproduce

1. Create an ASP.NET project with Identity.UI or with MVC (old templates or the code in the samples in the identity repo - https://github.com/dotnet/aspnetcore/blob/main/src/Identity/samples/IdentitySample.Mvc/Controllers/AccountController.cs )

2. Go to the password reset page with a random code in the URL for example /Account/ResetPassword?code=test

3. Input existing user email and submit - you get invalid token error

4. Input non-existing user email and submit - you are told password was reset successfully

.NET Version

net6.0 and any version with Identity

[blowdart]

Thank you for the report. We've had this one a few times. The thing is you can argue that the registration page does the same thing, it'll give a different result based on whether an email is registered or not.

We've never felt any pressing urge to address the reset password route because there are other routes to discovery. We'd look at a pull request though.

As an aside, for any project that has issue templates with a security.md defined an entry appears when filing an issue which links to their security policy;

Please consider following the instructions set out in a security policy rather than file issues directly, as some projects would rather issues with potential security implications are reported privately.

[ghost]

Hi @Eirenarch. We have added the "Needs: Author Feedback" label to this issue, which indicates that we have an open question for you before we can take further action. This issue will be closed automatically in 7 days if we do not hear back from you by then - please feel free to re-open it if you come back to this issue after that time.

[Eirenarch]

Thank you for the report. We've had this one a few times. The thing is you can argue that the registration page does the same thing, it'll give a different result based on whether an email is registered or not.

We've never felt any pressing urge to address the reset password route because there are other routes to discovery. We'd look at a pull request though.

This makes sense but this particular behavior is also bad user experience especially coupled with the fact that the user needs to type the email and might get it wrong. It would be OK to say that not hiding if an email is registered is a non-goal and removing this code but now we have the worst of both worlds - the info is leaked AND user experience is degraded.

Please consider following the instructions set out in a security policy rather than file issues directly, as some projects would rather issues with potential security implications are reported privately.

I considered using the security vulnerability template but I didn't think this important enough for this path which I presume is more urgent

To provide a pull request there must be a decision about the desired behavior. Is not leaking the registration a non-goal or is it a goal. One way it could be a goal despite the registration page is that a user can change the registration flow but keep all the rest. Alternatively the registration flow might be changed but that would make it more complex and less user friendly.

[blowdart]

Partly the typing is due to the way identity is configured by default, where the email address and the username are the same. This decision continues to haunt us, as when folks turn that coupling off the assumptions made in the templates break which some find unexpected and are a little taken aback that customization must now take place.

The coupling here also explains the hiding of emails not being a goal, because it's not exactly an email at times, it's the username. I'm not sure having the username entered is a better experience though, as usernames are public knowledge sometimes (and we're back to leaking via registration again), requiring an email address seems to be the better approach.

So ... I'll toss it back again :) Would usernames be better for password reset or emails? Do the templates need to change to take a decoupling into account, or is it ok to require folks to customize once they turn off the default configuration? I'm genuinely interested in your take here. I can be persuaded either way about what the desired behaviour for reset should be, and as planning for 8 starts now is a good time to make an argument, and then we can decide if a PR fits or not.

[Eirenarch]

Here is my suggestion and please let me know if there is a security issue that I am not aware of.

1. Forgotten Password sends a link where the URL contains the user id (not email, not username) and the code

2. The form only contains inputs for password and confirm password, no email, no username. The userId from the URL is used to identify the user.

This approach is more user-friendly because it has less inputs and does not leak emails (or usernames). The decision if emails should stay hidden is irrelevant to this approach and can be made separately. Even if it is decided that leaking registration doesn't matter it is still an improvement because the user experience has improved.

Again I'd like to point out that the current codebase is the worst option because it leaks the registration while containing an explicit code and comment saying that it doesn't. I was hesitant to change it for a long time before I posted this issue and while I was writing it I wasn't sure I wasn't missing something.

On the other topics

1. I think hiding emails should be supported by Identity the question is if it should be in the default templates. I am not sure what the answer is here

2. Username being email by default is a good thing in my opinion. I think this is the correct decision in the majority of cases

3. The registration can be changed to send an email when you want to register rather than create the account outright but this is more complex and arguably less user-friendly but it will prevent email leaks while keeping username and email the same

[blowdart]

Thanks :) We'll chat about this in triage and consider what we change for 8.0

[ghost]

Thanks for contacting us.

We're moving this issue to the .NET 8 Planning milestone for future evaluation / consideration. We would like to keep this around to collect more feedback, which can help us with prioritizing this work. We will re-evaluate this issue, during our next planning meeting(s). If we later determine, that the issue has no community involvement, or it's very rare and low-impact issue, we will close it - so that the team can focus on more important and high impact issues. To learn more about what to expect next and how this issue will be handled you can read more about our triage process here.