Security issue with UserManager.CreateAsync
Closed this issue · 24 comments
The latest code in the repository for the UserManager.CreateAsync method is as follows...
public virtual async Task<IdentityResult> CreateAsync(TUser user, string password)
{
ThrowIfDisposed();
var passwordStore = GetPasswordStore();
if (user == null)
{
throw new ArgumentNullException(nameof(user));
}
if (password == null)
{
throw new ArgumentNullException(nameof(password));
}
var result = await UpdatePasswordHash(passwordStore, user, password);
if (!result.Succeeded)
{
return result;
}
return await CreateAsync(user);
}This is a security issue because the UpdatePasswordHash method occurs before CreateAsync. This allows for the changing of an existing user's password.
It also doesn't make sense to update a password before creating a user.
My guess is this is a way to validate the password and if that is the case a call to PasswordValidator.ValidateAsync would be better.
UpdatePasswordHash is really user.PasswordHash = hash it doesn't commit anything to the database, its just updating the user entity, Create is what actually commits to the database.
I'm not sure I understand the issue around changing user's password, the user entity should not exist so updating it should have no effect until its Created
No, on line #2307 there is this...
await passwordStore.SetPasswordHashAsync(user, hash, CancellationToken);
That causes the password store (ie. database) to set the password.
The issue is with creating a new user when an existing user exists. For example, username is unique and if "JohnDoe" is already registered and another user attempts to register with the same username the passwordStore.SetPasswordHashAsync will update the password for JohnDoe #1 and now JohnDoe #2 can sign in and see JohnDoe #1's account.
So I guess the scenario you describe might exists if you get an existing user from the database, and pass it into Create. But you shouldn't do that and is almost certainly an error in the app to call Create this way.
And even so, the default EF UserStore implementation should throw when you try to reinsert an existing user. It would fail in SaveChanges since it would generate an insert from the Context.Add, see: https://github.com/aspnet/Identity/blob/dev/src/Microsoft.AspNetCore.Identity.EntityFrameworkCore/UserStore.cs#L339
Yeah it will fail but at that point the password has already been updated.
In EF this might be ok if you don't do SaveChanges until the very end but I am not using EF. I am implementing my own store to use stored procedures. I am not "getting" a user from the database.
The password store should not be called before a user is created.
There's a implicit contract of Unit of Work between the manager and stores. Changes should not be persisted outside of Update/Create/Delete. You will likely run into all sorts of issues if this is not true for your store. That is a fundamental assumption for how the UserManager works. You will run into this issue in many other places.
EF has an explicit UOW pattern unless the developer intentionally circumvents the convention and uses a stored procedure. This works well because EF (by design) directs developers to make changes and then commit those changes. Identity has no parallel structure.
The methods on UserManager do not promote a UOW pattern nor do they signal that a UOW pattern should be followed.
This issue can resolved with a few tweaks and I do believe this is an issue that should be addressed. I'm happy to submit a pull request.
For users of EF this (I agree with you) should never happen because of the UOW pattern enforced by EF but for those of us that implement a custom data store then the methods should be atomic.
An alternative would be to pass a transaction scope into every method so that the operations can be added to a transaction but the current interfaces do not support that. More evidence that Identity is not UOW centric.
It might be easy to fix this method, but the pattern for many methods is to use store methods to manipulate the state of the user, validation is done as a final step before Update is called. If your store atomically updates the user on each call, there's no way to roll back the changes.
See the common Update method, which does validation, updates two normalized fields, then 'saves'.
https://github.com/aspnet/Identity/blob/dev/src/Microsoft.AspNetCore.Identity/UserManager.cs#L2415
Early on we struggled with this issue, as its definitely a hidden assumption/contract that the stores must follow or you run into issues like you are seeing. Perhaps the documentation should call this out better.
Thanks for the example.
The example has issues as well.
The call to UpdateNormalizedUserNameAsync makes a call to Store.SetNormalizedUserNameAsync
The call to UpdateNormalizedEmailAsync makes a call to store.SetNormalizedEmailAsync
I was under the impression that a call to a Store would be used to persist changes especially void methods. There are many calls to validators such as the UserValidator or PasswordValidator and those return IdentityResult values. There are also calls to PasswordHasher and a string is returned. These all make sense and I wouldn't expect any of them to save data to a store.
From what I am understanding from our conversation (and please correct me if I am wrong) is that the UserManager is working as it was designed. The UserManager relies on the EF stores and a custom store that uses stored procedures is not the intended use case.
Stored procedures could work if every call to UserManager was wrapped in a transaction scope.
Yeah this is the intended design the managers are responsible for calling various store methods which basically map to get/set on properties, eventually they will call one of the methods that are supposed to commit (Update/Create/Delete).
The key takeaway is that stores must not persist in any other method as that would likely leave the data in a inconsistent state like you are seeing. Any store that works with this assumption is supported.
Ok, well I really wish I knew that because I would not have chosen this framework. I selected Identity because I didn't want to reinvent security and I thought choosing Identity would be a very secure choice. I also liked the fact that I could implement custom stores because my company does not allow EF; I can only use stored procedures. I thought the core framework was solid and that by implementing custom stores I could have the best of both worlds.
Unfortunately the implicit UOW pattern bit me and badly. This issue caused a security hole where users could essentially hijack another user's account and this isn't a site for my kid's soccer team.
With the proliferation of EF I may be in the minority but this is a big big flaw IMO. I like some of the utility features of this framework but I will not use UserManager in the future because stuff like this is how you end up in the news for data breaches.
Thanks for the dialog.
Yeah we probably should call out the UOW pattern in the documentation for custom stores a bit more clearly (cc @Rick-Anderson @blowdart ) Maybe your transaction scope idea will work...
UserManager uses the UOW pattern which has implications on how you store data. Data stores must not persist in any other method as that could leave the data in a inconsistent state.
Need to flesh out any other method - do you mean in the scope of the work you can only call one method that stores data (like UpdateNormalizedUserNameAsync or UpdateNormalizedEmailAsync ?
If the UOW pattern is used then perhaps there should be an obvious method that is called to persist the changes.
It would be helpful if there was a SaveChanges or something similar that clearly points out the UOW pattern is used. The UOW pattern states that a single method is used to save all of the units of work and the current code base violates that because an array of methods commit the units of work.
https://martinfowler.com/eaaCatalog/unitOfWork.html
Implementing the Repository and Unit of Work Patterns
Yeah we shouldn't call it the UOW pattern since its not exactly that, best to just be explicit and say something to the effect of: "Stores should only persist changes in the CreateAsync/UpdateAsync/DeleteAsync methods of base Store interface, any other store method should only manipulate the entity without persisting the changes."
I think that's a good idea
Where can I add that info other than Intro to Identity?
I think it should be added there and I think it should be a warning in every page that references any of the store methods. If you can add something to the intellisense comments I think that would be good too.
This should really go in the custom store documentation, it's not really important for people who are consuming only the public identity APIs, but its very important custom store implementers to understand.
custom store documentation
@HaoK where is that?
@HaoK Custom store docs are being scheduled. See dotnet/AspNetCore.Docs#1162
This:
"Stores should only persist changes in the CreateAsync/UpdateAsync/DeleteAsync methods of base Store interface, any other store method should only manipulate the entity without persisting the changes."
is particularly unintuitive, because the signature of all the mutators in this interface returns a Task. Which, to the implementer, looks like the framework expects IO to be performed within the mutators. Would be clearer if only Create/Update/Delete returned Tasks.
Agree @tempywritescode
Also naming an interface I....Store makes me think to save/read something into anything. For what is the SetPasswordHashAsync needed? To make a IUser<TKey>.PasswordHash = passwordHash;? That implicits to have property PasswordHash in my IUser implementation (maybe a bad idea - also ever not needed to hold this hash in a object - IMHO).
Intuitive i thought that implementing a function of I....Store saving something into my datastore or to get something out of my datastore per function call. But it seems it is not true...tooks me hours to finally figure that out and find this thread. :-/
regards
Implicit TransactionScope may help achieve consistency seamlessly, regardless of the UserManager operations order.
https://docs.microsoft.com/en-us/dotnet/api/system.transactions.transactionscope?view=netcore-2.0
I can't see the following in the documentation :
Yeah this is the intended design the managers are responsible for calling various store methods which basically map to get/set on properties, eventually they will call one of the methods that are supposed to commit (Update/Create/Delete).
Am I missing something?
I'm probably not finding the right pages.