Framework: Wrong URL reset link when using subdomains

[rodrigoroma]

• Laravel Version: 5.5.3
• PHP Version: 7.1.7 (homestead)
• Database Driver & Version: MySQL

Description:

Out of the box, Laravel is sending reset password e-mail with the wrong password reset link when using subdomains.

It was suposed to be http://subdomain.app.com/password/reset/<token> but it is sending http://app.com/password/reset/<token>. I'm using Auth::routes().

It's related to commit cef10551820530632a86fa6f1306fee95c5cac43 but I can't figure out how to address this issue without creating security issues (as stated in the commit).

[sisve]

how to address this issue without creating security issues

You just have to validate that the current host is yours, so that the generated link points to your server.

The security issue you're mentioning is due to people running nginx configs that allows all hostnames and let Laravel handle it all, and then a Laravel application that presumes that all incoming requests have valid hosts. The exploit was people sending a request to your server with a header "Host: sisves-evil-server.com", in which case Laravel would generate a reset link that pointed to my evil server, and if someone clicked the link I had the reset token and thus ability to reset the account password to one of my choosing.

Just make sure you validate the hosts of the incoming requests, and respond accordingly (404 Not Found or 418 I'm a teapot) if it's not supported by your application.

[rodrigoroma]

Hi @sisve, thanks for your reply!

In my specific case, I have my own server with a Laravel application with only 3 specific subdomains (it's not a multi-tenancy and neither a shared domain, it's only 3 specific sub-domains that I created).

I changed the link that is sent to the e-mail to http://subdomain.app.com/password/reset/<token> and now it's working properly (out of the box, it was http://app.com/password/reset/<token> wich results in a 404 error because my authentication routes are in the subdomain).

I'm not 100% sure but I guess that I haven't introduced any kind of vulnerability by doing this.

IMHO, the framework should out of the box send a valid reset link to the e-mail (instead of a 404 error link).

[sisve]

I cannot properly answer your question without you telling how you changed the link. Did you change APP_URL? Did you override the toMail method and hardcoded another url? Both solutions would work properly. The only issue is if you let the incoming Host header decide what link you generate.

[rodrigoroma]

Hi @sisve !

I created a new notification and overrided the sendPasswordResetNotification method on my user model to use this new notification that I've created.

In the notification, I'm generating the link with route('password.reset', $this->token).

[sisve]

That means that Laravel will use the current host to generate the link, and that you are probably vulnerable again. You are not vulnerable if your nginx configuration only listens/accepts a few accepted hostnames.

The only way I believe I can be more clear is to use upper case characters.

VALIDATE THE HOSTNAME OF THE INCOMING PASSWORD RESET REQUEST. DO NOT GENERATE PASSWORD RESET NOTIFICATIONS WHEN IT IS AN UNKNOWN HOSTNAME.

Is there anything else that needs clarification?

[rodrigoroma]

So in my case I'm not vulnerable because my routes file is something like:

Route::domain('subdomain.' . env('APP_DOMAIN'))->group(function () {
    Auth::routes();
});

In this case, I'm validating the full domain before calling route('password.reset', $this->token). But anyway, it's a good point to restrict nginx to only accept requests from my domain to improve security.

The main point here is: shouldn't Laravel reset password feature work out of the box even on subdomains?

[bbashy]

@rodrigoroma It should work by default if you set APP_URL to be the URL. If it's dynamic *.domain.tld it shouldn't be vulnerable since you own domain.tld?

There's a very slim scenario for the vulnerability anyway... no one should accept all domains for their site.

[rodrigoroma]

@bbashy thanks for you reply! But I think that's not the point.

To be more clear, suppose you have 2 subdomains:

• www.example.com -> the main website
• restricted.example.com -> the part of the website with all the authentication routes

In this scenario, I think the right value for APP_URL is http://www.example.com because this is the main URL, right? But it doesn't hold the reset password routes.

As in cef10551820530632a86fa6f1306fee95c5cac43 the reset password link sent thought e-mail will be http://www.example.com/password/reset/<token> wich results in a 404 error page because this route is in another subdomain.

One could change the APP_URL to restricted.example.com but suppose you have a lot of subdomains each one with different authentication routes (using different guards).

[bbashy]

@rodrigoroma If the auth routes (plus password reset form) are on restricted.example.com, then send that through to the config so it's correct.

Personally I think it should be reverted or be using another config var for password reset base route.

[rodrigoroma]

I think setting the APP_URL config to match the auth routes is not the best solution because Laravel allows you to have multiple authentications, including multiple authentication routes each route in a different URL.

In my case, I have 2 authentication routes, each one in a different subdomain. But fortunatelly only one of these two authentication systems have password reset capability.

Maybe a good solution would be to have protected $domain variable in the ResetPasswordController. By default, this variable could be config('app.url') but people would be able to override.

What do you guys think?

[bbashy]

@rodrigoroma You can't set properties on classes to functions.

[rodrigoroma]

@bbashy I couldn't understand. Why shouldn't I be able to set a protected $domain variable in my ResetPasswordController?

There's already a protected $redirectTo variable in it and my suggestion is to add a similar one.

[bbashy]

@rodrigoroma You can add a property on the class but you won't be able to set it to config() helper as you stated the default should be that.

[rodrigoroma]

So we could leave the default value as it already is but we could give the option to override this with a protected $domain that could be set to config(). What do you think?

[bbashy]

@rodrigoroma I don't think you understand what I mean. PHP doesn't allow functions to be set on properties.

[rodrigoroma]

Ok, now I got it!

But anyway, it could be done using a method instead of a variable.

[slakbal]

Hi everyone, not sure if this is a related issue: https://github.com/laravel/framework/issues/21616

[bbashy]

@slakbal I doubt it.

[rodrigoroma]

@tillkruss was it resolved?

[tillkruss]

Not sure, it's been inactive for months. Is this still an issue?

[developerdesks]

For this
->action('Reset Password', url(config('app.url').route('password.reset', $this->token, false)))
Add this one
->action('Reset Password', url(config('app.url'.':'.'app.port').route('password.reset', $this->token, false)))
It will work.

[rodrigoroma]

Yes, it will work! But it's not an out of the box solution, so it's necessary to create a new notification and override the sendPasswordResetNotification method on the user model to use this new custom notification.

As far as I know, there was no updates to tackle this issue.

[slalji]

I resolved this issue by changing the vendor\laravel\framework\src\Illuminate\Auth\Notifications\ResetPassword.php that comes out of the box.

replace line
->action('Reset Password', url(config('app.url').route('password.reset', $this->token, false)))
to
->action('Reset Password', url(route('password.reset', $this->token, false)))

config('app.url') is your url root but you want relative path instead

or you can extend the notification class

namespace Larashop\Notifications;

use Illuminate\Notifications\Notification;
use Illuminate\Notifications\Messages\MailMessage;

class ResetPassword extends Notification
{

public function __construct($token)
{
    $this->token = $token;
}


public function via($notifiable)
{
    return ['mail'];
}

public function toMail($notifiable)
{
    return (new MailMessage)
        ->line('You are receiving this email because we received a password reset request for your account.')
        ->action('Reset Password', route('password.reset.token',['token' => $this->token]))
        ->line('If you did not request a password reset, no further action is required.');
}

}

[sisve]

@slalji And you also opened up your application to a huge security problem; a (presumedly evil) middle-man can generate password requests that point to his server, and anyone clicking on those mails will give the token to that server, and can use that token to change the password for the user and highjack the account.

[rodrigoroma]

Exactly. And this security vulnerability was resolved in commit cef10551820530632a86fa6f1306fee95c5cac43

[raoufOu]

you have only to change APP_URL=http://localhost whith http://your_projet.test/

[rodrigoroma]

@raoufOu it only works when not using subdomains.

[danmatthews]

I've just cracked open a new issue for this as i believe it's a true bug, along with how i worked around it.
https://github.com/laravel/framework/issues/27045