Support for Drupal 7 is ending on 5 January 2025—it’s time to migrate to Drupal 10! Learn about the many benefits of Drupal 10 and find migration tools in our resource center.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
| Comment | File | Size | Author |
|---|---|---|---|
| #14 | 2554221-autocomplete-14.patch | 541 bytes | tim.plunkett |
| #12 | interdiff.txt | 437 bytes | amateescu |
| #12 | 2554221-12.patch | 503 bytes | amateescu |
| #9 | interdiff.txt | 1014 bytes | amateescu |
| #9 | 2554221-9.patch | 511 bytes | amateescu |
Comments
Comment #2
larowlanprodding
Comment #3
larowlana start
Comment #5
larowlanDone for now
Comment #6
larowlanLooks like we're using a query string here anyway. Instead of path parts. Should we just add a test, if we can?
Comment #7
amateescu CreditAttribution: amateescu for Pfizer, Inc. commentedSince 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.txtas an actual path to a file. So this part of the patch should not be needed.We can't execute JS in a test until #2469713: Step 2: Create a JavaScriptTestBase using PhantomJs Driver/Binary happens.
Comment #8
catchWe also don't support ?q= any more, so there's no way to set the current path via it.
Comment #9
amateescu CreditAttribution: amateescu for Pfizer, Inc. commentedLet's remove that change then.
Comment #10
tim.plunkettYep, that seems right. Nothing else to do here.
Comment #11
pwolanin CreditAttribution: pwolanin as a volunteer and at Acquia commentedYes, 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.
Comment #12
amateescu CreditAttribution: amateescu for Pfizer, Inc. commentedYep, it doesn't really hurt to have it.
Comment #13
pwolanin CreditAttribution: pwolanin as a volunteer and at Acquia commentedNo, 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,}\//gComment #14
tim.plunkettYep, D7 was
with those 3 cases
So we do want exactly what @pwolanin said, which is removing the ^ and $ from the last patch.
Comment #15
amateescu CreditAttribution: amateescu for Pfizer, Inc. commentedAgreed, the rule from #12 looked a bit strange to me as well, thanks for fixing it :)
Comment #16
pwolanin CreditAttribution: pwolanin as a volunteer and at Acquia commentedThanks. I double-checked the pattern in the console and I think it's right now.
Comment #17
effulgentsia CreditAttribution: effulgentsia at Acquia commentedHow 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?
Comment #18
catchComment #19
effulgentsia CreditAttribution: effulgentsia at Acquia commentedDowngrading from critical, because I think for D8, this is a pure hardening question rather than fixing a vulnerability.
Comment #24
kwoxer CreditAttribution: kwoxer commentedSeem to work for me. Patch is working. Hopefully soon a js test.
Comment #33
smustgrave CreditAttribution: smustgrave at Mobomo commentedThis 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.