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
#12 interdiff.txt437 bytesamateescu
#12 2554221-12.patch503 bytesamateescu
#9 interdiff.txt1014 bytesamateescu
#9 2554221-9.patch511 bytesamateescu
#3 autocomplete-2554221.1.patch1.49 KBlarowlan

Issue fork drupal-2554221

Command icon Show commands

Start within a Git clone of the project using the version control instructions.

Or, if you do not have SSH keys set up on git.drupalcode.org:

About issue forks

Comments

webchick created an issue. See original summary.

larowlan’s picture

larowlan
Location 🇦🇺🏝.au GMT+10
commented
Assigned: Unassigned » larowlan

prodding

larowlan’s picture

larowlan
Location 🇦🇺🏝.au GMT+10
commented
Status: Active » Needs review
StatusFileSize
new autocomplete-2554221.1.patch1.49 KB

a start

Status: Needs review » Needs work

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

larowlan’s picture

larowlan
Location 🇦🇺🏝.au GMT+10
commented
Assigned: larowlan » Unassigned

Done for now

larowlan’s picture

larowlan
Location 🇦🇺🏝.au GMT+10
commented

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

amateescu commented 
+++ 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

catch
he/him
Primary language English
commented

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

amateescu’s picture

amateescu commented
Status: Needs work » Needs review
StatusFileSize
new 2554221-9.patch511 bytes
new interdiff.txt1014 bytes

Let's remove that change then.

tim.plunkett’s picture

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

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

pwolanin’s picture

pwolanin commented
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

amateescu commented
Status: Needs work » Needs review
StatusFileSize
new 2554221-12.patch503 bytes
new interdiff.txt437 bytes

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

pwolanin’s picture

pwolanin commented
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

tim.plunkett
he/him
Primary language English
Location Philadelphia
commented
Status: Needs work » Needs review
StatusFileSize
new 2554221-autocomplete-14.patch541 bytes

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

amateescu commented

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

pwolanin’s picture

pwolanin commented
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

effulgentsia commented

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

catch
he/him
Primary language English
commented
Status: Reviewed & tested by the community » Needs review
effulgentsia’s picture

effulgentsia commented
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.

kwoxer’s picture

kwoxer commented

Seem to work for me. Patch is working. Hopefully soon a js test.

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

Drupal 8.4.4 was released on January 3, 2018 and is the final full bugfix release for the Drupal 8.4.x series. Drupal 8.4.x will not receive any further development aside from critical and security fixes. Sites should prepare to update to 8.5.0 on March 7, 2018. (Drupal 8.5.0-alpha1 is available for testing.)

Bug reports should be targeted against the 8.5.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.6.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.5.x-dev » 8.6.x-dev

Drupal 8.5.6 was released on August 1, 2018 and is the final bugfix release for the Drupal 8.5.x series. Drupal 8.5.x will not receive any further development aside from security fixes. Sites should prepare to update to 8.6.0 on September 5, 2018. (Drupal 8.6.0-rc1 is available for testing.)

Bug reports should be targeted against the 8.6.x-dev branch from now on, and new development or disruptive changes should 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.

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

Drupal 8.6.x will not receive any further development aside from security fixes. Bug reports should be targeted against the 8.8.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.9.x-dev branch. For more information see the Drupal 8 and 9 minor version schedule and the Allowed changes during the Drupal 8 and 9 release cycles.

Version: 8.8.x-dev » 8.9.x-dev

Drupal 8.8.7 was released on June 3, 2020 and is the final full bugfix release for the Drupal 8.8.x series. Drupal 8.8.x will not receive any further development aside from security fixes. Sites should prepare to update to Drupal 8.9.0 or Drupal 9.0.0 for ongoing support.

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

Version: 8.9.x-dev » 9.2.x-dev

Drupal 8 is end-of-life as of November 17, 2021. There will not be further changes made to Drupal 8. Bugfixes are now made to the 9.3.x and higher branches only. For more information see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

Version: 9.2.x-dev » 9.3.x-dev

Version: 9.3.x-dev » 9.4.x-dev

Drupal 9.3.15 was released on June 1st, 2022 and is the final full bugfix release for the Drupal 9.3.x series. Drupal 9.3.x will not receive any further development aside from security fixes. Drupal 9 bug reports should be targeted for the 9.4.x-dev branch from now on, and new development or disruptive changes should be targeted for the 9.5.x-dev branch. For more information see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

Version: 9.4.x-dev » 9.5.x-dev

Drupal 9.4.9 was released on December 7, 2022 and is the final full bugfix release for the Drupal 9.4.x series. Drupal 9.4.x will not receive any further development aside from security fixes. Drupal 9 bug reports should be targeted for the 9.5.x-dev branch from now on, and new development or disruptive changes should be targeted for the 10.1.x-dev branch. For more information see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

smustgrave’s picture

smustgrave commented
Status: Needs review » Needs work
Issue tags: +Needs Review Queue Initiative, +Needs tests

This issue is being reviewed by the kind folks in Slack, #needs-review-queue-initiative. We are working to keep the size of Needs Review queue [2700+ issues] to around 400 (1 month or less), following Review a patch or merge request as a guide.

This still seems relevant
Tagging for tests to show the issue though.

Not sure after7 years if this is still a major.

Version: 9.5.x-dev » 11.x-dev

Drupal core is moving towards using a “main” branch. As an interim step, a new 11.x branch has been opened, as Drupal.org infrastructure cannot currently fully support a branch named main. New developments and disruptive changes should now be targeted for the 11.x branch. For more information, see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

prudloff made their first commit to this issue’s fork.

prudloff opened merge request !12730

Version: 11.x-dev » main

Drupal core is now using the main branch as the primary development branch. New developments and disruptive changes should now be targeted to the main branch.

Read more in the announcement.

prudloff’s picture

prudloff commented
Status: Needs work » Needs review
Issue tags: -Needs tests

If I understand correctly, this is just a hardening on D8+ so I'm not sure we can write a test that shows an actual problem being fixed.
That said, I added a test that fails without the patch.

smustgrave’s picture

smustgrave commented
Status: Needs review » Needs work
Issue tags: +Needs issue summary update

Can we update the summary please.