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.
Problem/Motivation
Now that we have had an SA for Views 7.x-3.x, we opted to address the D8 vulnerability publicly.
http://drupalcode.org/project/views.git/commit/ddf8181bd13f69ffbeeee14ae... ( Google cache )
http://drupal.org/node/1948358
Proposed resolution
Remaining tasks
Task | Novice task? | Contributor instructions | Complete? |
---|---|---|---|
Add automated tests | Instructions | ||
Add steps to reproduce the issue, or confirm if it can't be reproduced | Novice | Instructions |
User interface changes
N/A
API changes
N/A
Comment | File | Size | Author |
---|---|---|---|
#35 | patch-xss-views-1948418-34.patch | 9.26 KB | webflo |
#35 | patch-xss-views-1948418-34.interdiff.txt | 2.45 KB | webflo |
#33 | patch-xss-views-1948418-33.patch | 10.34 KB | webflo |
#31 | patch-xss-views-1948418-31.patch | 8.37 KB | webflo |
#30 | views.view_.sa_contrib_2013_035.yml | 6.66 KB | webflo |
Comments
Comment #1
dstolHere's a potential patch from s.d.o. I am not the author of it. All the credit is due to grisendo
Comment #3
dawehnerRerolled that it can be committed.
Comment #4
galooph CreditAttribution: galooph commentedRerolled so that it will apply - file locations have changed since the last patch.
Not too sure how this should be done now:
Do we use the striptags function in twig. I've done
<h3 class="views-ui-view-title views-table-filter-text-source">{{ title | striptags }}</h3>
for now, but not sure if that's correct!Comment #6
galooph CreditAttribution: galooph commented#4: drupal-1948418-4.patch queued for re-testing.
Comment #8
dawehnerInstead of check_plain() we should better use String::checkPlain().
Comment #10
galooph CreditAttribution: galooph commentedThanks for the review, dawehner.
I've changed the patch to use String::checkPlain() and I've used it in template_preprocess_views_ui_view_info() to sanitise the title variable there, instead of in the twig template.
Comment #11
galooph CreditAttribution: galooph commentedComment #12
xjmForward-porting an SA sounds critical.
Comment #13
xjmComment #14
dawehnerI wonder whether we actually still needs due the fact that auto-espacing landed in core.
Comment #15
cilefen CreditAttribution: cilefen commentedComment #16
martin107 CreditAttribution: martin107 commentedFrom #14 I suspect @dawehner might be correct, but don't want the "Needs roll" tags to slow up the review process, on an already slow moving critical.
Comment #18
martin107 CreditAttribution: martin107 commentedFixed up 2 things.
Comment #21
amitgoyal CreditAttribution: amitgoyal commentedReroll of #18.
Comment #23
cilefen CreditAttribution: cilefen commentedI am not sure how this was supposed to work.
Comment #24
catchTagging with D8 upgrade path, see issue summary of #2341575: [meta] Provide a beta to beta/rc upgrade path for why.
Comment #25
xjmComment #26
gaurav.goyal CreditAttribution: gaurav.goyal commentedPatch needs a reroll, changing the status to needs work and adding the tag Needs reroll
Comment #27
gaurav.goyal CreditAttribution: gaurav.goyal commentedPatch rerolled,
Thanks a ton @cilefen for mentoring me on this reroll.
Removing needs reroll tag.
Comment #28
xjmSo we need to:
In #3, we've discussed elsewhere that we should actually be removing early
checkPlain()
calls, so my recommendation would be to add only the test in that case.Comment #29
chx CreditAttribution: chx commentedComment #30
webflo CreditAttribution: webflo commentedBuild a view in standard profile that exposes the vulnerabilities. But D8 is atm not vulnerable because some of this is nuked by twig autoescape and also
HandlerBase::adminLabel
contains a call toString::checkPlain
.Comment #31
webflo CreditAttribution: webflo commentedThe test proves that Views UI is not vulnerable to SA-CONTRIB-2013-035. But it shows another bug, we double escape the admin labels in views ui.
Comment #33
webflo CreditAttribution: webflo commentedComment #35
webflo CreditAttribution: webflo commentedThe exported view was not valid because the Views Wizard added fields without plugin_id to the view.
Comment #36
jibranComment #37
dawehnerMore marquee!
The follow up is filled, so let's get this one in.
Have you considered to check for
String::checkPlain('actual-readable-string')
instead?Comment #38
alexpottCommitted 6efeaf7 and pushed to 8.0.x. Thanks!
Comment #41
jcnventura@webflo, you missed something #2473907: Tests not being run by testbot due to missing summary line