See: https://www.drupal.org/SA-CORE-2015-003
http://cgit.drupalcode.org/drupal/commit/?h=7.x&id=731dfacab8bf39918c135...

A cross-site scripting vulnerability was found in the autocomplete functionality of forms. The requested URL is not sufficiently sanitized.

This vulnerability is mitigated by the fact that the malicious user must be allowed to upload files.

Credit for the D6/D7 version of this patch (the security release):

effulgentsia, Pere Orga, benjy, tim.plunkett, larowlan, pwolanin, David_Rothstein
CommentFileSizeAuthor
#14 2554221-autocomplete-14.patch541 bytestim.plunkett
PASSED: [[SimpleTest]]: [PHP 5.5 MySQL] 106,599 pass(es). View
#12 interdiff.txt437 bytesamateescu
#12 2554221-12.patch503 bytesamateescu
PASSED: [[SimpleTest]]: [PHP 5.5 MySQL] 106,608 pass(es). View
#9 interdiff.txt1014 bytesamateescu
#9 2554221-9.patch511 bytesamateescu
PASSED: [[SimpleTest]]: [PHP 5.5 MySQL] 106,588 pass(es). View
#3 autocomplete-2554221.1.patch1.49 KBlarowlan
FAILED: [[SimpleTest]]: [PHP 5.5 MySQL] 104,171 pass(es), 2 fail(s), and 0 exception(s). View
Members fund testing for the Drupal project. Drupal Association Learn more

Comments

webchick created an issue. See original summary.

larowlan’s picture

Assigned: Unassigned » larowlan

prodding

larowlan’s picture

Status: Active » Needs review
FileSize
1.49 KB
FAILED: [[SimpleTest]]: [PHP 5.5 MySQL] 104,171 pass(es), 2 fail(s), and 0 exception(s). View

a start

Status: Needs review » Needs work

The last submitted patch, 3: autocomplete-2554221.1.patch, failed testing.

larowlan’s picture

Assigned: larowlan » Unassigned

Done for now

larowlan’s picture

Looks like we're using a query string here anyway. Instead of path parts. Should we just add a test, if we can?

amateescu’s picture

+++ b/core/lib/Drupal/Core/Render/Element/FormElement.php
@@ -130,7 +130,9 @@ public static function processAutocomplete(&$element, FormStateInterface $form_s
-        $element['#attributes']['data-autocomplete-path'] = $url->getGeneratedUrl();
+        // Prefix the uri with the script name to bypass relative urls - forcing
+        // non-clean urls. e.g. index.php/some/path.
+        $element['#attributes']['data-autocomplete-path'] = \Drupal::request()->getScriptName() . '/' . $url->getGeneratedUrl();

Since we are already using query strings for autocomplete in 8.x, I don't think it's possible for browsers to interpret some/path?q=../some_file.txt as an actual path to a file. So this part of the patch should not be needed.

Should we just add a test, if we can?

We can't execute JS in a test until #2469713: Step 2: Create a JavaScriptTestBase using PhantomJs Driver/Binary happens.

catch’s picture

We also don't support ?q= any more, so there's no way to set the current path via it.

amateescu’s picture

Status: Needs work » Needs review
FileSize
511 bytes
PASSED: [[SimpleTest]]: [PHP 5.5 MySQL] 106,588 pass(es). View
1014 bytes

Let's remove that change then.

tim.plunkett’s picture

Status: Needs review » Reviewed & tested by the community

Yep, that seems right. Nothing else to do here.

pwolanin’s picture

Status: Reviewed & tested by the community » Needs work

Yes, we think 8 was mostly not vulnerable to this because it was in a query string.

I think the \s part of the pattern may not be relevant since it looks like we are calling trim() on it already https://api.jquery.com/jQuery.trim/

We may want to just skip removal of ../ but maybe a useful defense in depth.

amateescu’s picture

Status: Needs work » Needs review
FileSize
503 bytes
PASSED: [[SimpleTest]]: [PHP 5.5 MySQL] 106,608 pass(es). View
437 bytes

Yep, it doesn't really hurt to have it.

pwolanin’s picture

Status: Needs review » Needs work

No, the D7 pattern should match spaces at the beginning OR ../ anywhere or spaces at the end.

So if we just want to strip ../ the pattern should just be something like /\.{2,}\//g

tim.plunkett’s picture

Status: Needs work » Needs review
FileSize
541 bytes
PASSED: [[SimpleTest]]: [PHP 5.5 MySQL] 106,599 pass(es). View

Yep, D7 was

/
^\s+
|
\.{2,}\/
|
\s+$
/g

with those 3 cases

So we do want exactly what @pwolanin said, which is removing the ^ and $ from the last patch.

amateescu’s picture

Agreed, the rule from #12 looked a bit strange to me as well, thanks for fixing it :)

pwolanin’s picture

Status: Needs review » Reviewed & tested by the community

Thanks. I double-checked the pattern in the console and I think it's right now.

effulgentsia’s picture

We may want to just skip removal of ../ but maybe a useful defense in depth.

How is this a useful defense in depth? Are we worried about D8 contrib modules calling extractLastTerm() and appending that to a path portion of a URL? How likely is that given that the D8 routing system isn't friendly to embedded "/" characters within a path slug, which is what prompted us to refactor core autocompletes to use a query string in the first place?

catch’s picture

Status: Reviewed & tested by the community » Needs review
effulgentsia’s picture

Priority: Critical » Major
Issue tags: -security +Security improvements

Downgrading from critical, because I think for D8, this is a pure hardening question rather than fixing a vulnerability.

Version: 8.0.x-dev » 8.1.x-dev

Drupal 8.0.6 was released on April 6 and is the final bugfix release for the Drupal 8.0.x series. Drupal 8.0.x will not receive any further development aside from security fixes. Drupal 8.1.0-rc1 is now available and sites should prepare to update to 8.1.0.

Bug reports should be targeted against the 8.1.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.2.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

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

Drupal 8.1.9 was released on September 7 and is the final bugfix release for the Drupal 8.1.x series. Drupal 8.1.x will not receive any further development aside from security fixes. Drupal 8.2.0-rc1 is now available and sites should prepare to upgrade to 8.2.0.

Bug reports should be targeted against the 8.2.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.3.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

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

Drupal 8.2.6 was released on February 1, 2017 and is the final full bugfix release for the Drupal 8.2.x series. Drupal 8.2.x will not receive any further development aside from critical and security fixes. Sites should prepare to update to 8.3.0 on April 5, 2017. (Drupal 8.3.0-alpha1 is available for testing.)

Bug reports should be targeted against the 8.3.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.4.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

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

Drupal 8.3.6 was released on August 2, 2017 and is the final full bugfix release for the Drupal 8.3.x series. Drupal 8.3.x will not receive any further development aside from critical and security fixes. Sites should prepare to update to 8.4.0 on October 4, 2017. (Drupal 8.4.0-alpha1 is available for testing.)

Bug reports should be targeted against the 8.4.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.5.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.