Problem/Motivation

PHP has the hash_equals() function which should be used when comparing strings in security validation contexts. Currently CsrfTokenGenerator uses ===, which is bad.

The only problem is that hash_equals() does not exist in PHP 5.5, but we can use Crypt::hashEquals().

Proposed resolution

Use Crypt::hashEquals().

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

klausi created an issue. See original summary.

klausi’s picture

Issue summary: View changes

Ah, we have Crypt::hashEquals().

Thinking more about this: CSRF attacks are performed blind, so an attacker can never look at the response and do timing analysis. I still think it is a good idea to do this because our csrf_token service might be used in other ways we don't know where an attacker has access to the response.

klausi’s picture

Status: Active » Needs review
FileSize
505 bytes

Patch.

Status: Needs review » Needs work

The last submitted patch, 3: csrf-timing-2822499-3.patch, failed testing.

klausi’s picture

Status: Needs work » Needs review
FileSize
1.11 KB
633 bytes

Ugh, we should never pass NULL to a string validate function.

greggles’s picture

Status: Needs review » Reviewed & tested by the community

Makes sense to me. Thanks, Klausi!

  • catch committed 329c020 on 8.3.x
    Issue #2822499 by klausi: CsrfTokenGenerator should use timing attack...
catch’s picture

Version: 8.3.x-dev » 8.2.x-dev

Committed/pushed to 8.3.x, thanks!

This looks fine to me for 8.2.x as well - any reason not to cherry-pick?

greggles’s picture

IMO it seems good to backport/cherry pick, yes.

klausi’s picture

RTBC+1, the queued test run for 8.2.x cam back green.

alexpott’s picture

Status: Reviewed & tested by the community » Fixed

I've cherry-picked to 8.2.x as a bug and a security improvement with no BC implications I think this is fine.

  • alexpott committed d8bd4db on 8.2.x authored by catch
    Issue #2822499 by klausi: CsrfTokenGenerator should use timing attack...

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.