Skip to content

Commit

Permalink
force host on password reset notification
Browse files Browse the repository at this point in the history
  • Loading branch information
@taylorotwell
taylorotwell committed May 7, 2017
1 parent 0f3003d commit cef1055
Showing 1 changed file with 1 addition and 1 deletion.
2 changes: 1 addition & 1 deletion src/Illuminate/Auth/Notifications/ResetPassword.php
View file
Original file line number Diff line number Diff line change
Expand Up @@ -46,7 +46,7 @@ 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', $this->token))
->action('Reset Password', url(config('app.url').route('password.reset', $this->token, false)))
->line('If you did not request a password reset, no further action is required.');
}
}

7 comments on commit cef1055

@AdamEsterle
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@taylorotwell

-I pointed A.com's password reset form to B.com's endpoint using B.com's _token.
-B.com received the request and sent an email, but the email still pointed to B.com (5.4.21, before the above change)

Is there another way they are spoofing this?

@nCrazed
Copy link
Contributor

@nCrazed nCrazed commented on cef1055 May 9, 2017 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@AdamEsterle in your example you're still hitting B.com's endpoint with B.com's domain. The exploit is in hitting B.com's IP with something other than B.com.

Laravel (prior to this patch) will happily use any hostname and if the web server is not configured to check the host and simply accepts all incoming requests then it's possible to trick the application into sending a spoofed reset link.

From what I gather the exploit is unlikely to be viable on domain-based multi-tenant applications because they inspect and validate domains by design. Ironically this fix breaks the password reset functionality on such applications.

@boynet
Copy link

@boynet boynet commented on cef1055 May 9, 2017 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@nCrazed I know this bug for a lot of time didnt consider it exploit on the laravel side more bad server configuration...

this change can actually break a lot of laravel code out there as I don't know how many devs are actually changed the config('app.url') as to my knowledge until this point it only used for some artisan command which not everyone used

@bbashy
Copy link
Contributor

@bbashy bbashy commented on cef1055 May 9, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

And even worse, in Laravel 5.1 it said this command was only used for Artisan commands;

/*
|--------------------------------------------------------------------------
| Application URL
|--------------------------------------------------------------------------
|
| This URL is used by the console to properly generate URLs when using
| the Artisan command line tool. You should set this to the root of
| your application so that it is used when running Artisan tasks.
|
*/

'url' => env('APP_URL', 'http://localhost'),

@nCrazed
Copy link
Contributor

@nCrazed nCrazed commented on cef1055 May 9, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@bbashy not just 5.1, it's the same in the master.

@bbashy
Copy link
Contributor

@bbashy bbashy commented on cef1055 May 9, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@nCrazed Yeah, sorry, I just meant it first appeared in 5.1 (as far as I know) and people now think of it as Artisan based only.

@martinbean
Copy link
Contributor

@martinbean martinbean commented on cef1055 Nov 14, 2017 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

From what I gather the exploit is unlikely to be viable on domain-based multi-tenant applications because they inspect and validate domains by design. Ironically this fix breaks the password reset functionality on such applications.

@nCrazed Yup, I’m having this problem right now 😩

Just been trawling the Laravel source code for a “nice” place to override the URL used in password reset emails, which has led me here. I have a multi-tenant CMS, but reset URLs are being generated with the application’s domain and not the domain of the site it was created.

Please sign in to comment.