This is the code for changing a password in the default AccountController scaffolded in MVC 4:
// ChangePassword will throw an exception rather
//than return false in certain failure scenarios.
bool changePasswordSucceeded;
try
{
string userName = User.Identity.Name;
changePasswordSucceeded = WebSecurity.ChangePassword(userName,
model.OldPassword,
model.NewPassword);
}
catch (Exception)
{
changePasswordSucceeded = false;
}
if (changePasswordSucceeded)
{
return RedirectToAction("Manage", new { Message = ManageMessageId.ChangePasswordSuccess });
}
else
{
ModelState.AddModelError("",
"The current password is incorrect or the new password is invalid.");
}
My issue is that the message is unclear. If the current password is incorrect then that's fine but if the new password is invalid I'd like to present a better message to the user telling them what's wrong and I'd like to understand the "failure scenarios" better so that I can tailor the message.
The documentation here is specific about the exceptions and I don't think these should be swallowed in the Action and reported as invalid passwords.
So why is the "new password is invalid" a possibility here? Can I remove it if I'm only using the SimpleMembershipProvider without OAuth in my application?
Edit: NB This Data Annotation also appears in the "RegisterModel" class so there is a password validity check here too
[StringLength(100,
ErrorMessage = "The {0} must be at least {2} characters long.",
MinimumLength = 6)]
This is now a long answer, so I have edited it into sections. I am not a security expert compared to a user like rook, and even he says not to trust his security answers, or anybody else's. Test and understand it yourself, always. Any web developers reading this should read the OWASP top ten web security flaws, use online guidance such as Trustworthy Computing and, of course, Mr. Bruce Schneier. Security is complicated and too big for any one of us, so use frameworks which do the work according to best practice when you can. Above all, keep it all in perspective.
See below for more detail on these summary answers:
Think about it from a security perspective:
The machine tells me
"current password incorrect"
Now I put in try again with what I think your current password is:
"new password invalid"
I now know your password, but you don't know I do as I haven't changed anything.
Footnote: I got the first set of logic wrong, deleted it, edited it and reposted, but hey - I'm not Bruce
There are many combinations of this attack, but it's just like email harvesting from a bad login system. Think what happens if I get two different messages when I try to "recover password" using an email address:
then I can easily find out valid member emails from your site for a targeted phishing attack on that user, or just build a list of valid email addresses for spam.
A more user friendly, but equally secure error message for our password change in both cases would be:
"The current password is incorrect or the new password is invalid. Remember that your password is case sensitive, and new passwords must have [64 letters/5 numbers/4 characters/3 names of greek gods]"
To stay sane, keep this in the context of your application use, but remember you have a responsibility in a bigger eco-system, and users share passwords between websites, whether you like it or not.
Regarding the question part about when ChangePassword returns false:
Essentially WebSecurity.ChangePassword
:
In summary, it returns false
if:
UserId
no longer exists in the database; orchangePasswordSucceeded = false
if any other exception happens).newPassword
validity check within the call to ChangePassword
So in theory, if you have your validation attributes in place correctly, and ignoring the edge cases, it can only return false
if we have an edge case (the user has been deleted, between the Get and Post, or we can't hit the database, or the current password is invalid).
This all holds true even in the case that attackers bypass the UI in the browser (which they will) as the Action itself calls IsValid
on the model.
There is a serious disclaimer here: I don't spend my life working on security. I follow secure-by-design principles (I keep up with the OWASP top ten project, for instance) and believe I have a good awareness of the most important security principles. I may therefore have got some of this logic wrong.
If the client side "password requirements" validation is not there - what happens? The user has to wait for the Post to return to find out what happened.
If the client side "password requirements" validation is there, does it weaken our interface? I don't believe so.:
MVC doesn't, AFAIK, do either of these things, so that is where this starts to fall apart.
It also falls apart because we return a different error and message from the server due to the Model.IsValid
test inside the action, which enforces the new password validation and the different error message for this failure. Therefore, in its current implementation, the single error message approach is flawed.
So would I get change the way this is all done? In the hope that I would improve the other parts in time, I probably wouldn't, other than changing the return message to be a bit longer and more informative.
This is a personal opinion, it could be argued the other way
The detail:
Membership.GetUser(userName, true).ChangePassword(...)
. In the case of an ArgumentException see docs it will return false
.ChangePassword
on the actual provider (e.g. SimpleMembershipProvider.ChangePassword
), and will return false
if that fails.The actual provider implementation gets complex. For SimpleMembershipProvider
, for example,
ArgumentExceptions
(bubbling up to the catch
and return false
in MembershipUser.ChangePassword
) if the password/username is empty, null, too long etc.UserId
from the database (returns false if fails),PasswordChangedDate
for you while it goesMembershipUser