Problem/Motivation

In \Drupal\Core\Access\CsrfTokenGenerator::validate() it's possible that $token is something other than a string. In PHP 8 this causes a warning.

The only times $token is something other than a string is in our test coverage - \Drupal\Tests\Core\Access\CsrfTokenGeneratorTest. On PHP < 8 this method will return FALSE in these case because that's the behaviour of hash_equals(). In PHP 8 hash_equals only accepts strings therefore we need to check the type of $token and return early. See https://3v4l.org/KFNqm

Not that once we change \Drupal\Core\Access\CsrfTokenGenerator::validate() to use scalar typehints this "problem" goes away.

Proposed resolution

Ensure that $token is a string.

Remaining tasks

User interface changes

API changes

Data model changes

Release notes snippet

CommentFileSizeAuthor
#2 3156880-2.patch748 bytesalexpott
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

alexpott created an issue. See original summary.

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
CreditAttribution: alexpott at Acro Commerce, Thunder commented
Status: Active » Needs review
Issue tags: +PHP 8.0
Parent issue: » #3109885: [meta] Ensure compatibility of Drupal 9 with PHP 8.0 (as it evolves)
FileSize
3156880-2.patch748 bytes
alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
CreditAttribution: alexpott at Acro Commerce, Thunder commented
Issue summary: View changes
alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
CreditAttribution: alexpott at Acro Commerce, Thunder commented
Issue summary: View changes

So looking at https://3v4l.org/KFNqm I really wonder how useful this change is. If you're passing something that's not string on a real site you are already triggering warnings. In fact in order to maintain BC we need to trigger a warning from... 

+++ b/core/lib/Drupal/Core/Access/CsrfTokenGenerator.php
@@ -86,8 +86,14 @@ public function validate($token, $value = '') {
+    // PHP 8.0 strictly typehints for hash_equals. Maintain BC until we can
+    // enforce scalar typehints on this method.
+    if (!is_string($token)) {
+      return FALSE;
+    }

... when we return false. The only reason the test passes is because we do some jiggery-pokery with the error handler:

  public function testValidateParameterTypes($token, $value) {
    $this->setupDefaultExpectations();

    // The following check might throw PHP fatals and notices, so we disable
    // error assertions.
    set_error_handler(function () {
      return TRUE;
    });
    $this->assertFalse($this->generator->validate($token, $value));
    restore_error_handler();
  }
andypost’s picture

andypost
he/him
Primary language Russian
CreditAttribution: andypost as a volunteer and at Skilld commented

It may use deprecation to pass not string and strict type in d10, also check could be used to skip compute

Gábor Hojtsy’s picture

Gábor Hojtsy
he/him
Primary language Hungarian
Location Hungary
CreditAttribution: Gábor Hojtsy at Acquia commented

@alexpott: based on your link what you mean is PHP 5 and 7 already have a Warning if what is passed is not a string, so that would surface somehow for people currently. Copying here for posterity:

Output for 7.3.0 - 7.3.21, 7.4.0 - 7.4.10

Warning: hash_equals(): Expected user_string to be a string, bool given in /in/KFNqm on line 3
bool(false)

Output for 5.6.0 - 5.6.40, 7.0.0 - 7.0.33, 7.1.0 - 7.1.33, 7.2.0 - 7.2.33

Warning: hash_equals(): Expected user_string to be a string, boolean given in /in/KFNqm on line 3
bool(false)

Do we consider it part of the (Drupal) API that a warning is thrown? We could only throw a userspace warning anyway, so we could not reproduce the same system warning per say I think. So not sure it is worth reproducing that.

I would either remove the test from CsrfTokenGeneratorTest as the situation is already producing warnings on the site. Or commit this patch as-is if for some reason we think non-strings should be supported. Let me dig into the history of CsrfTokenGeneratorTest.

Gábor Hojtsy’s picture

Gábor Hojtsy
he/him
Primary language Hungarian
Location Hungary
CreditAttribution: Gábor Hojtsy at Acquia commented

The test coverage was added in #2141041: CsrfTokenGenerator::validate() should do an identical compare then refactored in #2145881: Remove string casting from Crypt::hmacBase64(). Throw exceptions instead to throw exceptions instead of type casting for non-scalars. It does not type-cast scalars anymore to strings though, as the later issue claims they could be non-strings.

I did not find proof that non-strings would be accepted by either https://www.php.net/manual/en/function.base64-encode.php or https://www.php.net/manual/en/function.hash-hmac.php though so I don't think that was even true then.

Gábor Hojtsy’s picture

Gábor Hojtsy
he/him
Primary language Hungarian
Location Hungary
CreditAttribution: Gábor Hojtsy at Acquia commented
Status: Needs review » Reviewed & tested by the community

Based on my above research, all in all I think the safest is to keep the code not throwing Fatal errors on PHP 8 for things we thought we support but PHP clearly did not support. Which is what the patch does.

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
CreditAttribution: alexpott at Acro Commerce, Thunder commented 
+++ b/core/lib/Drupal/Core/Access/CsrfTokenGenerator.php
@@ -86,8 +86,14 @@ public function validate($token, $value = '') {
+    $value = $this->computeToken($seed, $value);
+    // PHP 8.0 strictly typehints for hash_equals. Maintain BC until we can
+    // enforce scalar typehints on this method.
+    if (!is_string($token)) {
+      return FALSE;
+    }
 
-    return hash_equals($this->computeToken($seed, $value), $token);
+    return hash_equals($value, $token);

@andypost asked

Just no reason to compute before check is string

The reason we do this after the call to $this->computeToken() is to maintain BC. If we do

    // PHP 8.0 strictly typehints for hash_equals. Maintain BC until we can
    // enforce scalar typehints on this method.
    if (!is_string($token)) {
      return FALSE;
    }

first then \Drupal\Tests\Core\Access\CsrfTokenGeneratorTest::testInvalidParameterTypes() fails in 3 out of the four cases.

hussainweb’s picture

hussainweb
Primary language English
CreditAttribution: hussainweb at Axelerant commented

I'm wondering if it's better to throw an exception in \Drupal\Core\Access\CsrfTokenGenerator::validate to match the behaviour, rather than maintain this order in code (which might not be intuitive later). I'm guessing the exceptions are being thrown already by \Drupal\Component\Utility\Crypt::hmacBase64 anyway.

OR does this sound like a job for a follow-up?

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
CreditAttribution: alexpott at Acro Commerce, Thunder commented

@hussainweb I don't think we should throw an exception in this issue because that is a behaviour change. We should be fixing this in a way that can go all the way back to 8.9.x with the minimum of impact.

hussainweb’s picture

hussainweb
Primary language English
CreditAttribution: hussainweb at Axelerant commented

@alexpott, yeah, scratch that. I am mixing up $token and $value in the code. We can change the test to make sure that the code is logical and easy to read but that would imply change of behaviour and it's difficult to imagine that. It's best to not touch that in this issue.

andypost’s picture

andypost
he/him
Primary language Russian
CreditAttribution: andypost as a volunteer and at Skilld commented

I'm also ++ to improve code instead of just workaround, so why not just change tests and file separate issue for 8.9.x to keep the same unit tests.

I find strange that #9 said about calling of protected method means API/behavior change

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
CreditAttribution: alexpott at Acro Commerce, Thunder commented

@andypost it's not about a protected method. It's about the inputs to a public method i.e. CsrfTokenGenerator::validate() resulting different behaviour. Given this is a a very security sensitive method I'd argue that maintain current behaviour of when exceptions are thrown or not is important.

  • catch committed f2c21c1 on 9.1.x 
    Issue #3156880 by alexpott, Gábor Hojtsy, andypost, hussainweb: \Drupal\...
catch’s picture

catch
he/him
Primary language English
CreditAttribution: catch at Third and Grove commented
Status: Reviewed & tested by the community » Fixed

Agreed with no behaviour changes here. Let's open a follow up to discuss refactoring things a bit.

Committed f2c21c1 and pushed to 9.1.x. Thanks!

Don't see a reason to backport this one, but please re-open if you disagree.

andypost’s picture

andypost
he/him
Primary language Russian
CreditAttribution: andypost as a volunteer and at Skilld commented

Filed follow-up #3173720: Refactor CsrfTokenGenerator::validate()

Status: Fixed » Closed (fixed)

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