Problem/Motivation

We've added protection to core for https://www.drupal.org/sa-core-2018-002 and https://www.drupal.org/sa-core-2018-004 but we omitted test coverage to not give hackers any hints of how to exploit the vulnerability. However both have now been publicly exploited therefore we should add the tests so that we don't break this important functionality.

Proposed resolution

Add tests

Remaining tasks

User interface changes

None

API changes

None

Data model changes

None

CommentFileSizeAuthor
#9 2969053-9.patch10.27 KBalexpott
#9 7-9-interdiff.txt1.65 KBalexpott
#7 2969053-7.patch10.27 KBalexpott
#7 4-7-interdiff.txt1.18 KBalexpott
#4 2969053-4.patch10.27 KBalexpott
#4 3-4-interdiff.txt1.82 KBalexpott
#3 2969053-3.patch10.27 KBalexpott
#2 2969053-2.patch6.59 KBalexpott
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
FileSize
2969053-2.patch6.59 KB

Here's a start from the work on tests in the security issue. I think the tests can be improved to be easier to maintain.

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
CreditAttribution: alexpott at Acro Commerce, Thunder commented
FileSize
2969053-3.patch10.27 KB

Here's a rework so that you don't have to set a requests query, cookie, and request parameters all the same.

Bit of a complete re-work so no interdiff.

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
CreditAttribution: alexpott at Acro Commerce, Thunder commented
FileSize
3-4-interdiff.txt1.82 KB
2969053-4.patch10.27 KB

Slight tweak so that all the Request object is tested together and all of the globals are tested together.

alexpott’s picture

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

I've used PHPUnit and Xdebug to confirm that the current test exercises 100% of the code. So at least we know every line is exercised. It would be good to confirm that all the logic is tested. Note that external destinations are not a concern because they are removed by \Drupal\Core\EventSubscriber\RedirectResponseSubscriber::sanitizeDestination(). I think though at some point we should file a follow to merge that functionality into the RequestSanitizer and also fix it. The conversion to Symfony's request introduced 2 bugs in it. Opened an issue for this #2969207: RedirectResponseSubscriber::sanitizeDestination() was upgraded from Drupal 7 incorrectly

benjy’s picture

benjy CreditAttribution: benjy at Unearthed commented

Tests look good to me, tiny phpcs issue.

  1. +++ b/core/tests/Drupal/Tests/Core/Security/RequestSanitizerTest.php
@@ -0,0 +1,228 @@
+    $this->errors[] = compact("errno", "errstr");

    +++ b/core/tests/Drupal/Tests/Core/Security/RequestSanitizerTest.php
@@ -0,0 +1,228 @@
+      if ($error["errstr"] === $errstr && $error["errno"] === $errno) {

    Normally array keys and function args are single quotes.

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
CreditAttribution: alexpott at Acro Commerce, Thunder commented
FileSize
4-7-interdiff.txt1.18 KB
2969053-7.patch10.27 KB

Thanks for the review @benjy. Patch attached addresses that and also a couple of other code style things.

samuel.mortenson’s picture

samuel.mortenson
he/him
Primary language English
CreditAttribution: samuel.mortenson at Acquia commented

Nits:

  1. +++ b/core/tests/Drupal/Tests/Core/Security/RequestSanitizerTest.php
@@ -0,0 +1,227 @@
+  /**
+   * @param \Symfony\Component\HttpFoundation\Request $request
+   *   The request to sanitize.

    Missing comment in docblock. 

  2. +++ b/core/tests/Drupal/Tests/Core/Security/RequestSanitizerTest.php
@@ -0,0 +1,227 @@
+    // Set up globals.
+    $_GET = $request->query->all();
+    $_POST = $request->request->all();
+    $_COOKIE = $request->cookies->all();
+    $_REQUEST = array_merge($request->cookies->all(), $request->query->all(), $request->request->all());
+    $request->server->set('QUERY_STRING', http_build_query($request->query->all()));
+    $_SERVER['QUERY_STRING'] = $request->server->get('QUERY_STRING');

    I think $request->overrideGlobals() will do this as well.

Other than that this looks like good coverage.

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
CreditAttribution: alexpott at Acro Commerce, Thunder commented
FileSize
7-9-interdiff.txt1.65 KB
2969053-9.patch10.27 KB

Thanks for the review @samuel.mortenson

1. Fixed
2. $request->overrideGlobals() does more - it also sorts the query string. Personally I think we should do this on every request to normalise stuff for caches but that's not what we do now. One thing investigating this idea did bring up is that the test assumes the php ini var request_order is CGP but actually by default in php.ini its GP for security reasons so I've changed the test to reflect that.

samuel.mortenson’s picture

samuel.mortenson
he/him
Primary language English
CreditAttribution: samuel.mortenson at Acquia commented
Status: Needs review » Reviewed & tested by the community

Interdiff and a re-review looks good to me.

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 9: 2969053-9.patch, failed testing. View results

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
CreditAttribution: alexpott at Acro Commerce, Thunder commented
Status: Needs work » Reviewed & tested by the community

HEAD failing.

Version: 8.6.x-dev » 8.7.x-dev

Drupal 8.6.0-alpha1 will be released the week of July 16, 2018, which means new developments and disruptive changes should now be targeted against the 8.7.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

alexpott credited David_Rothstein.

alexpott credited Heine.

alexpott credited Jasu_M.

alexpott credited Pere Orga.

alexpott credited cashwilliams.

alexpott credited drumm.

alexpott credited larowlan.

alexpott credited mlhess.

alexpott credited quicksketch.

alexpott credited tim.plunkett.

alexpott’s picture

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

Adding contributors to the original issues because they all reviewed the test coverage too.

alexpott credited dawehner.

alexpott credited dsnopek.

alexpott credited pwolanin.

alexpott’s picture

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

Some more people.

tim.plunkett credited xjm.

tim.plunkett’s picture

tim.plunkett
he/him
Primary language English
Location Philadelphia
CreditAttribution: tim.plunkett at Acquia commented

  • catch committed 7c0ce17 on 8.7.x 
    Issue #2969053 by alexpott, tim.plunkett, Pere Orga, cashwilliams,...

  • catch committed 36137d6 on 8.6.x 
    Issue #2969053 by alexpott, tim.plunkett, Pere Orga, cashwilliams,...
catch’s picture

catch
he/him
Primary language English
CreditAttribution: catch at Third and Grove commented
Version: 8.7.x-dev » 8.6.x-dev
Status: Reviewed & tested by the community » Fixed

Committed/pushed to 8.7.x and cherry-picked to 8.6.x. Thanks!

Status: Fixed » Closed (fixed)

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