laravel / fortify

Backend controllers and scaffolding for Laravel authentication.
https://laravel.com/docs/fortify
MIT License
1.62k stars 295 forks source link

Reset Password exposes valid accounts and shows reset form with invalid token #322

Closed CyberPunkCodes closed 3 years ago

CyberPunkCodes commented 3 years ago

Description:

The biggest issue I have with the way Fortify is out of the box, is that it exposes valid accounts, which helps would-be attackers at determining information they shouldn't know. Furthermore, both of these password reset routes that expose valid accounts, are not throttled!

When you click "Forgot Password" and enter in an email address, you shouldn't get an "That email doesn't exist" message. They should all be successful, and IF they entered in their email correctly, they actually get the email. While you might claim, "personal opinion, fix it yourself...", it seems the reset password process in Fortify isn't as easy to modify as say the "login" feature. The login page doesn't expose valid accounts, it just says the credentials are wrong, and it is throttled. Both, for the very same reason, that the password reset process, should be. Even the email verification is throttled!

Also, when you enter an invalid token for the password reset link that gets emailed to the user, the password reset form still shows with no errors. It makes it appear that the person is going to be able to change the password with an invalid token. It should probably check the database for that token prior to the page loading, and throw an error.

When you submit the form, obviously it fails and warns that the token was invalid. However, the form shouldn't be showing in the first place.

You can also enter any email address into the url, and it appears like you're going to be able to change it. Again, you can't when you actually submit the form.

Now, change the email address (either in the url or form) to anyone else. Accounts that don't exist, some that do... It will tell you, "We can't find a user with that email address.". Since this isn't throttled, a simple bash/php script could run tens of thousands of email addresses against this form and potentially get hits for valid accounts. Though, throttling doesn't stop brute forcing, as they use thousands of proxies to do their brute forcing 🤷‍♂️

The email address is useless on the reset password page anyways. You have a long, randomly generated token that expires. Just look the user up by the token. The email verification email is signed, shouldn't the reset password link be signed also?

You don't even have to submit the "forgot password" form or click the link in the email. Anyone that knows Laravel can figure it out and just navigate to it, putting any fake token in they want, and try to find every valid email they can.

https://example.test/reset-password/fake-token-here?email=admin%40example.test
https://example.test/reset-password/fake-token-here?email=joe%40gmail.com
https://example.test/reset-password/fake-token-here?email=betty%40yahoo.com
https://example.test/reset-password/fake-token-here?email=david%40protonmail.com

Solution

Forgot Password Route:

Reset Password Route:

y0rdie commented 3 years ago

How entitled do you sound, calm your passion and submit a PR 👍

driesvints commented 3 years ago

Hi @WadeShuler. I do understand all of the things you mentioned but I personally don't see them as problematic. You're always free to attempt PR's to improve these things in the way that you'd like to see. Thanks.

CyberPunkCodes commented 3 years ago

@driesvints - Whoever coded the login logic apparently thought that exposing whether an email address was a valid account was an issue, as well as thought it should be throttled. So what we have now, is a publicly free to access route that undoes what the login form sought to protect (exposing valid accounts). It's efforts were for nothing. What is the purpose of locking your front door if you leave the backdoor unlocked?

There is an issue with exposing whether an email is valid, and there are tons of security discussions online about it.

An attacker has a database of email/password combos compiled from previous data breaches. Say they have 500k, and a Laravel site has 100k users. They run the emails against the vulnerable route (ie: reset password) and get hits for 20 email accounts that are valid in the system. They then try previously known passwords for those emails, and manage to get into at least 1 account. - This is exactly why the login form hides whether an email is in the system.

taylorotwell commented 3 years ago

We do not consider exposing what accounts exist a security concern.

bcorcoran commented 3 years ago

I disagree; I think in some systems it's absolutely critical not to expose which accounts exist. That said, you can work around the password reset issue by editing your forgot-password.blade.php:

Here's the relevant portion of mine:

@if (session('status') || $errors->any())
  <div class="mb-4 font-medium text-sm text-green-600">
    {{ session('status') ?? __('passwords.sent') }}
  </div>
@endif

In my case, I replaced the sent key in lang/en/passwords.php with "Thanks! If the email address matches an account, a password reset link will be sent."

Remove the <x-validation-errors /> component as well.

HTH

gwidgery commented 2 years ago

Our organisation had a pen test conducted recently and this was one of the findings - that it was possible to enumerate the users of the application by using the reset password function. It was a low-risk finding, but a finding nonetheless.

It would be very useful to have a feature or configuration setting that would cause the password reset function to return a status of: 'If account exists we have emailed you'.

tomb1n0 commented 1 year ago

@driesvints @taylorotwell is there any appetite for someone to PR improvements on this area into Fortify? I have a couple of ideas but i wouldn't want to waste your (or my own) time on this.

Forgot Password

As far as this flow goes, the fix would probably be as simple as always returning a success state. I.e. "Thanks, you will receive an e-mail shortly with instructions on how to reset your password".

We can either put this behind a flag, or just make it a default (I guess this would be a breaking change though).

Self Registration

There's a subtle issue with self registration in that if you enter an e-mail address that already exists, you get a validation error stating the e-mail is already in use. This is basically the same issue as the forgot password flow but it's trickier to solve.

What i'd suggest we do is:

There will no doubt be some more thought required (e.g. someone might not want e-mail verification enabled, in which case the activation message is a bit weird).

driesvints commented 1 year ago

Right now we're not considering making any changes here sorry.

tomb1n0 commented 1 year ago

Thanks for the speedy reply @driesvints . A shame but good to know!

dfreer commented 1 year ago

Hopefully this will help anyone else running into this situation. My solution was to add the following to my App\Providers\FortifyServiceProvider class:

use Illuminate\Support\Facades\Password;
use Laravel\Fortify\Http\Responses\FailedPasswordResetLinkRequestResponse;
use Laravel\Fortify\Http\Responses\SuccessfulPasswordResetLinkRequestResponse;

public function register(): void
{
    $this->app->extend(FailedPasswordResetLinkRequestResponse::class, fn () => new SuccessfulPasswordResetLinkRequestResponse(Password::RESET_LINK_SENT));
}

This makes it so that regardless if the email is found or not it returns the same message.

patrickomeara commented 1 year ago

Adding to @dfreer's solution I would also recommend sending the password reset notification after the response, or dispatch a closure, so an automated script couldn't tell which emails are exposed based on response time.

    // User.php

    // This is overridden so the response isn't slowed down.
    public function sendPasswordResetNotification($token)
    {
        dispatch(fn () => $this->notify(new ResetPasswordNotification($token)))->afterResponse();
    }
rk4an commented 10 months ago

Hi, I also found this article: https://muhamadhhassan.me/preventing-user-enumeration-attack-in-laravel-apps

SimeonDominiq commented 2 months ago

Hopefully this will help anyone else running into this situation. My solution was to add the following to my App\Providers\FortifyServiceProvider class:

use Illuminate\Support\Facades\Password;
use Laravel\Fortify\Http\Responses\FailedPasswordResetLinkRequestResponse;
use Laravel\Fortify\Http\Responses\SuccessfulPasswordResetLinkRequestResponse;

public function register(): void
{
    $this->app->extend(FailedPasswordResetLinkRequestResponse::class, fn () => new SuccessfulPasswordResetLinkRequestResponse(Password::RESET_LINK_SENT));
}

This makes it so that regardless if the email is found or not it returns the same message.

2024! This still works magic. Thank you for being Cyber-aware.