I'm not sure how this is supposed to work, but it doesn't quite make sense to me.
We are writing a mobile client (Flutter) which talks to the AspNetZero backend. The user may want (or need) to reset their password - in which case, a UI is displayed to trigger that flow - sending the user an email with a password reset code.
Clearly, one options is for the user to click the "reset link" in the email - which will launch a browser, and reset their password. However, the reset code is also displayed in the email. Our user is sitting on a screen in the app, waiting for the reset code (see screenshot)
When the user supplies the reset code, and password, then the API is called to reset the password. Of course, this fails with "reset code has expired" error, because of this code in the backend (which was introduced 3 years ago in this commit
If the user chose to click the link in the email, the backend would have populated the "ExpireDate" from the encrypted "c" payload. But, because we're not doing that - we want to let the user enter the reset code, we need to pass the expire date ourselves - which seems entirely redundant.
In which case I ask - what is the point of the server checking the ExpireDate, if it's a value that can be passed/spoofed by the user anyway?
I dont think there is much you can do with your current design, because you're not saving the expiration date - you NEED it to be passed.
Anyway, the key point, is that the expiration can be bypassed using postman - just supply the userid, resetcode... etc, and a valid ExpireDate. It isn't really protecting the server from allowing expired reset codes to be used.
Correct me if I'm wrong, but shouldnt you be storing the expiredate in the database user record, alongside the reset code? I get it's safe in the "c" (encrypted reset payload).. that's totally fine - but like I said - you need to also allow the user to type in their reset code - otherwise why is it even in the email?
So, long story short, by feeling is that this is a security issue (ExpireDate can be bypassed), and a usability bug (unable to reset password through the API using a known resetCode, without passing a bogus and pointless "expireDate").
4 Answer(s)
-
0
Hi,
Even if you pass
ExpireDate, Framework will callResolveParametersmethod of ResetPasswordInput, when it is passed from client to server. So, you or any other client shouldn't be able to overwriteExpireDatevalue.In your case, you should use same encryption algorithm on your Flutter app and pass
cparameter like we are doing in expire link. In that case, server will resolve all parameters from your encryptedcparameter.Markdown is supportedCopy & paste or drag & drop images (max 30 MB per image) -
0
Hi @ismcagdas,
"ExpireDate" will only be overwritten if the parameter "c" is passed - so I don't think it's as safe as you're expecting.
You can see in the screenshot that the user can fully populate the arguments, and so long as they omit "c", those arguments will be not be overwritten.
On your second point about "use the same encryption algorithm on your flutter app, and pass
c". This is not possible. The server encrypts the "c" value using a key that should never be passed down to any clients. Doing so would risk leaking the key and allowing clients to create their own encrypted arguments (e.g JWT tokens), and we'd be in a far worse position :(Given that the goal is to allow a client to pass "reset code", "email" and "new password" to reset their password (this used to be possible prior to the change-set I mentioned above) - I think the best approach would be the following.
- Add a "ResetCodeExpiryTime" column to the AbpUsers table, along side "ResetCode"
- When the user requests a reset code, the expiry time is calculated and stored in this field (
SendPasswordResetCode) - When the user calls
ResetPasswordretrieve the expiry time from the database, instead of client provided arguments
The "c" encrypted arguments are a nice idea, but they become superfluous - not really adding any value.
Markdown is supportedCopy & paste or drag & drop images (max 30 MB per image) -
0
I would usually do a PR for my suggestions, but in this case, I believe the correct implementation requires a new sql migration - which is a higher risk of merge conflicts... but am happy to do it anyway. Shall I PR?
I'd need to better understand the goals of the encrypted
cparameter - why encrypt these values? What are the potential the attack vectors this encryption is supposed to protect against?Someone intercepts the email? The encryption only hides the user-id. Everything else is readable in the email body.
Someone is trawling shared-web-server log files, which can capture the full GET query string? Yes, the encryption hides everything here.
It's important to know the goals, because most of the encrypted values are already exposed in plain-text in the email, i.e, reset code, email address. So the encryption is really only protecting the user-id.
I suspect #2 is the intended goal. And the current implementation achieves that, but the way it's done exposes a code path that allows a malicious client to pass unencrypted values of their own choosing (such as any ExpiryDate) - and it breaks the non-web user flow. Assuming you agree with my above assessment, I think I understand it well enough to fix the implementation to mitigate the above vulnerability, and achieve both user flows:
- User clicks on encrypted hyperlink in email
- User manually types reset code into client app
I think, during revision of this feature, some additional vectors need to be addressed.
Rate limiting there is currently nothing in ANZ to guard against the password reset from being brute forced. The Authenticate method is essentially rate limited through the use of the lockout feature, but the reset is just as vulnerable, yet not protected at all. I think the ResetPassword method should piggyback off the same logic as Authenticate. 3 tries then blocked. How many times can a user get their reset code wrong? Low hanging fruit.
Notifying user of password change This relates not only to password reset, but also change of password once logged in. I believe I've raised this topic before, but can't find the thread - if it was in this forum, in the ANZ/github or ABP/github. When a user account password is changed, an email should be sent to the user to let them know. This is mainly to guard against someone changing the password once they have successfully logged in - because the password reset flow already notifies the user.. but it may as well cover both scenarios.
We could go even further, and store the users IP address as a claim, and block it should it ever change. Of course it will impact a user should they have dynamic IP (either through ISP, or VPN etc) - however this case would not be frequent, nor introduce disproportionate friction relative to the value it provides. Of course we'd have to invalidate the refresh token too.
The attack vector is
- malicious user obtains an authenticated session (i.e, steals JWT token? or username/password)
- malicious user accesses the portal API, and changes user account password. User is not aware until both their token and refresh expires? Which could be a long time that the malicious actor has access.
Another note: I also don't think the
ResetPasswordInputneeds all the args it has.- ReturnUrl - I can't find any references to this arg. Is it ever used?
- SingleSignId - same here - no references? Was all this just copy pasted from the AuthenticationModel?
- The reset code is 10 hex chars - heaps of entropy. Assuming the server assigns unique reset code (which can be achieved) - why do we need tenant id? Is this used by the tenant-switching middleware?
Reading all the above now, I think it would be valuable for me to write up a full proposal of changes and their correlating attack vectors before creating a PR. This would eventually be worthy of its own page in the docs. Shall I proceed?
Markdown is supportedCopy & paste or drag & drop images (max 30 MB per image) -
0
Hi @hra
Thank you for your feedback. I have created an issue for this matter. You can follow the progress from there. Have a nice day.
Markdown is supportedCopy & paste or drag & drop images (max 30 MB per image)
