Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[5.5] [Security] Close remember_me Timing Attack Vector #21320

Merged
merged 4 commits into from Sep 21, 2017
Merged

[5.5] [Security] Close remember_me Timing Attack Vector #21320

merged 4 commits into from Sep 21, 2017

Conversation

@mcordingley
Copy link
Contributor

@mcordingley mcordingley commented Sep 21, 2017

The current remember_me token verification process leaves the application open to a timing attack.

Since the default is for the token to be stored as a cookie and for cookies to be encrypted, an attacker would have to know the application secret to exploit this. However, should a custom guard be used or cookies not be encrypted, it becomes possible to tease this value out.

The proposed change switches to comparing the token using a constant-time comparison. This makes it impossible to learn the value of the token by timing responses, independently of guard or encryption settings.

@taylorotwell taylorotwell merged commit 22471ae into laravel:5.5 Sep 21, 2017
2 checks passed
2 checks passed
continuous-integration/styleci/pr The StyleCI analysis has passed
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@mcordingley mcordingley deleted the mcordingley:mcordingley-patch-1 branch Sep 21, 2017
@GrahamCampbell GrahamCampbell changed the title [Security] Close remember_me Timing Attack Vector [5.5] [Security] Close remember_me Timing Attack Vector Sep 21, 2017
@@ -70,10 +70,9 @@ public function retrieveByToken($identifier, $token)
{
$user = $this->conn->table($this->table)

This comment has been minimized.

@GrahamCampbell

GrahamCampbell Sep 21, 2017
Member

This is surely the same as $user = $this->conn->table($this->table)->find($identifier); now?

This comment has been minimized.

@richardkeep

richardkeep Sep 21, 2017
Contributor

Yes. Definitely

@Naxon
Copy link
Contributor

@Naxon Naxon commented Sep 22, 2017

After upgrading I get this error:
hash_equals(): Expected known_string to be a string, null given (View: /home/vagrant/Code/the-gate/resources/views/welcome.blade.php)

In this line:
return $model && hash_equals($model->getRememberToken(), $token) ? $model : null;

@Kyslik Kyslik mentioned this pull request Sep 22, 2017
@crynobone
Copy link
Member

@crynobone crynobone commented Sep 22, 2017

A recent PR change the way remember token updated on logout, now during logout it will reset to null which would cause the issue

@themsaid
Copy link
Member

@themsaid themsaid commented Sep 22, 2017

@crynobone can you please link me to this change? it seems like the remember_me token is renewed on logout, but never set to null!

@at-phucnguyen
Copy link

@at-phucnguyen at-phucnguyen commented Oct 14, 2017

@mcordingley @taylorotwell
How about previous versions like 5.2? was it fixed?

@mcordingley
Copy link
Contributor Author

@mcordingley mcordingley commented Oct 16, 2017

@at-phucnguyen I don't think 5.2 is getting any more updates, even security ones. If you're worried about security, upgrade as soon as you're able. You're missing out on far more than just this one issue.

@bytestream
Copy link

@bytestream bytestream commented Oct 26, 2017

A similar fix should be applied to 5.1 given it's LTS for security fixes exists until June 2018?

@Quix0r
Copy link

@Quix0r Quix0r commented Aug 29, 2018

Has the (possible, not reproduced by me) introduced bug reported by @Naxon being addressed?

@Quix0r
Copy link

@Quix0r Quix0r commented Aug 29, 2018

Is #21323 the fix?

@cerw
Copy link

@cerw cerw commented Sep 4, 2018

How about 5.4?

@devcircus
Copy link
Contributor

@devcircus devcircus commented Sep 4, 2018

Security updates stopped in January for 5.4.
https://laravel.com/docs/5.6/releases#support-policy

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet