Updated: Comment #N

Problem/Motivation

#2141041: CsrfTokenGenerator::validate() should do an identical compare added string casting to Crypt::hmacBase64(), this just masks a problem in calling code by brushing it under the carpet. base64_encode() can also accept any scalar values - these don't have to be strings. This function could easily be being passed something other than what you expect (and what it expects). This casting could also lead to some weird bugs in peoples code that they will find very difficult to debug.

Proposed resolution

Throw an exception if strings are not passed to this function.

Remaining tasks

patch, review, updated tests.

User interface changes

None

API changes

None, unless you are passing some other datatypes to Crypt::hmacBase64()!

CommentFileSizeAuthor
#14 2145881.14.patch7.29 KBalexpott
#14 8-14-interdiff.txt1.51 KBalexpott
#8 interdiff-2145881-8.txt3.1 KBdamiankloip
#8 2145881-8.patch6.67 KBdamiankloip
#5 interdiff-2145881-5.txt695 bytesdamiankloip
#5 2145881-5.patch5.61 KBdamiankloip
#2 2145881-2.patch4.93 KBdamiankloip

Comments

damiankloip’s picture

damiankloip commented
Issue summary: View changes
damiankloip’s picture

damiankloip commented
Status: Active » Needs review
StatusFileSize
new 2145881-2.patch4.93 KB

So how about something like this?

I also changed the tests in the csrfTokenTest too, to assert the exceptions.

damien tournoud’s picture

damien tournoud commented
Status: Needs review » Reviewed & tested by the community

I suggested this during the making of the security release, but that was deemed too intrusive for Drupal 6 and 7.

On visual inspection, the patch looks good to me.

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 2: 2145881-2.patch, failed testing.

damiankloip’s picture

damiankloip commented
Status: Needs work » Needs review
StatusFileSize
new 2145881-5.patch5.61 KB
new interdiff-2145881-5.txt695 bytes

I guess drupal_get_js() should make sure it's not passing NULL to drupal_get_token().

dawehner’s picture

dawehner
Primary language German
commented
Status: Needs review » Reviewed & tested by the community

Awesome test coverage!!

tim.plunkett’s picture

tim.plunkett
he/him
Primary language English
Location Philadelphia
commented 
+++ b/core/tests/Drupal/Tests/Core/Access/CsrfTokenGeneratorTest.php
@@ -121,9 +116,7 @@ public function providerTestValidateParameterTypes() {
-      array(array()),
-      array(TRUE, 'foo'),
-      array(0, 'foo'),

So we're losing coverage for these three?

damiankloip’s picture

damiankloip commented
StatusFileSize
new 2145881-8.patch6.67 KB
new interdiff-2145881-8.txt3.1 KB

We don't lose here really, but how about this, then we get the best of both worlds? keeping the old tests (with strings as the token value) as moving the invalid parameters (exceptions thrown) to another test.

damiankloip’s picture

damiankloip commented
Status: Reviewed & tested by the community » Needs review
tim.plunkett’s picture

tim.plunkett
he/him
Primary language English
Location Philadelphia
commented

Thanks!

tim.plunkett’s picture

tim.plunkett
he/him
Primary language English
Location Philadelphia
commented
Status: Needs review » Reviewed & tested by the community

...

damien tournoud’s picture

damien tournoud commented 
       if (!defined('MAINTENANCE_MODE')) {
-        $setting['ajaxPageState']['theme_token'] = drupal_get_token($theme_key);
+        $setting['ajaxPageState']['theme_token'] = drupal_get_token((string) $theme_key);
       }

^ This looks like sweeping a bug under the carpet?

damiankloip’s picture

damiankloip commented

Potentially, can we open a follow up? As it's not really the fault of this patch. What's the bug? Theme key not being set in tests?

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
commented
Status: Reviewed & tested by the community » Needs review
StatusFileSize
new 8-14-interdiff.txt1.51 KB
new 2145881.14.patch7.29 KB

#12so lets not do that!

damiankloip’s picture

damiankloip commented
Status: Needs review » Reviewed & tested by the community

Nice work Alex. Rtbc for the code on top of the original rtbc!

xjm’s picture

xjm
she/her
Primary language English
commented

14: 2145881.14.patch queued for re-testing.

damiankloip’s picture

damiankloip commented

14: 2145881.14.patch queued for re-testing.

catch’s picture

catch
he/him
Primary language English
commented
Status: Reviewed & tested by the community » Fixed

Committed/pushed to 8.x, thanks!

Status: Fixed » Closed (fixed)

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