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

Use timing safe string comparison in CSRF filter #3126

Closed
wants to merge 1 commit into from

Conversation

@barryvdh
Copy link
Contributor

barryvdh commented Nov 11, 2014

Use a timing safe comparison, as provided by the Symfony Security Component.

As proposed by by @lasselehtinen and @ircmaxell in ba0cf2a

(I'm not a security expert, so they more knowledge about this)

@crynobone

This comment has been minimized.

Copy link
Contributor

crynobone commented Nov 11, 2014

I somehow would prefer if we convert this to a class.

Route::filter('csrf', 'Illuminate\Foundation\Filters\VerifyCsrfToken');

This way if we need to improve the functionality, developer just need to run composer update.

@barryvdh

This comment has been minimized.

Copy link
Contributor Author

barryvdh commented Nov 11, 2014

Yes I guess that is why Taylor move it to the core in L5 (see the related PR above), I think it's probably too late to change that in 4.2?

@crynobone

This comment has been minimized.

Copy link
Contributor

crynobone commented Nov 11, 2014

I think it's probably too late to change that in 4.2?

IMHO it easier to tell developer to replace the closure with above versus you need to add the import, change line x with y, but that just me.

@barryvdh

This comment has been minimized.

Copy link
Contributor Author

barryvdh commented Nov 11, 2014

No I agree, that would be a better option.

@GrahamCampbell
GrahamCampbell reviewed Nov 11, 2014
View changes
app/filters.php Outdated
@@ -1,4 +1,5 @@
<?php
use Symfony\Component\Security\Core\Util\StringUtils;

This comment has been minimized.

Copy link
@GrahamCampbell

GrahamCampbell Nov 11, 2014

Member
<?php

use Symfony\Component\Security\Core\Util\StringUtils;

New line before code please.

@GrahamCampbell
GrahamCampbell reviewed Nov 11, 2014
View changes
app/filters.php Outdated
@@ -83,7 +84,7 @@

Route::filter('csrf', function()
{
if (Session::token() !== Input::get('_token'))
if (!StringUtils::equals(Session::token(), Input::get('_token')))

This comment has been minimized.

Copy link
@GrahamCampbell

GrahamCampbell Nov 11, 2014

Member

Spaces around the ! please.

This comment has been minimized.

Copy link
@barryvdh

barryvdh Nov 12, 2014

Author Contributor

Fixed both issues

@barryvdh

This comment has been minimized.

Copy link
Contributor Author

barryvdh commented Nov 11, 2014

@GrahamCampbell before I fix your CS issues, what do you think about moving the filter to the framework as suggested?

@GrahamCampbell

This comment has been minimized.

Copy link
Member

GrahamCampbell commented Nov 11, 2014

It's already in the framework as of 5.0. I don't see the urgent need to put it in 4.2 as well.

@crynobone

This comment has been minimized.

Copy link
Contributor

crynobone commented Nov 11, 2014

  1. It be easier if we want to backport the security fixes to 4.1 and even 4.0
  2. If later we want to include headers + cookie usage of csrf token for js as what in 5.0, we just update the framework code.
Use a timing safe comparison, as provided by the Symfony Security Component.
@barryvdh barryvdh force-pushed the barryvdh:patch-3 branch to 1b6744e Nov 12, 2014
@barryvdh barryvdh deleted the barryvdh:patch-3 branch Jun 1, 2015
Markr10 added a commit to Wybren-Jongstra/Stender that referenced this pull request Jun 10, 2015
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants
You can’t perform that action at this time.