Problem/Motivation
Following #1616962: Replace $node->title with $node->label() and #1642070: Make use of entity labels in templates, Bartik uses the unsanitized $title
variable in node.tpl.php
, resulting in XSS vulnerabilities anywhere the teaser view mode is used.
Steps to reproduce
- Install D8 with the standard profile.
- Create an article with the title
<script>alert('XSS!');</strong>
and a taxonomy tagfoo
. - Visit the frontpage (main node listing). You will receive a JS alert.
- Visit the term listing for
foo
attaxonomy/term/1
. You will again receive a JS alert.
This is not reproducible for:
- Taxonomy term names (#1616972: Replace $term->name with $term->label()), which do not have an overridden template in a core theme.
- Comments, which are not yet converted (?).
- Core themes other than Bartik, which do not provide a
node.tpl.php
Proposed resolution
- Update the Bartik
node.tpl.php
to print a sanitized title. - Fix #1810710: Remove copying of node properties wholesale into theme variables to ensure that the unsanitized title will not be used accidentally.
- Add test coverage for all themes and entity templates to ensure that unsanitized titles are not printed.
See: template_preprocess_node()
Remaining tasks
Followups
- #1810710: Remove copying of node properties wholesale into theme variables
- Discuss whether
$title
should be provided as an alias for$label
in template_preprocess_node(). (See #10.) - Add more modular test coverage per entity type and view mode? (See #14.)
Original report by Fabianx
Since the label change, bartik prints still $title, which is not sanitized. It should print $label.
Reproduce: Put <script>alert('xss');</script>
in title of a node
Related to: #1810710: Remove copying of node properties wholesale into theme variables, which should be solved also.
This is not a duplicate as this is about bartik changing from $title to $label, while the above removes the XSS for all themes.
Changing from $title to $label needs to be done regardless of the outcome of the above issue.
Comment | File | Size | Author |
---|---|---|---|
#31 | bartik-1811684-31.patch | 4.66 KB | xjm |
#31 | interdiff-31.txt | 1.52 KB | xjm |
#29 | bartik-1811684-28.patch | 4.66 KB | xjm |
#29 | interdiff.txt | 1.52 KB | xjm |
#22 | bartik-1811684-21.patch | 4.47 KB | xjm |
Comments
Comment #1
larowlanTagging
Comment #2
xjmYowza. Looking at a test.
Comment #3
xjmI can't reproduce this locally. Here's what I did:
<em>Foo</em> <script>alert('XSS!');</script>
The title was properly escaped:
Also this appears to have coverage already in
PageTitleFilteringTest
.Can you provide more specific steps to reproduce?
Comment #4
Fabianx CreditAttribution: Fabianx commentedUpdate: The problem is only visible on the overview page (frontpage) / /node as the title is never printed for the /node/1 page.
Sorry for not being clearer.
Attached screenshot.
Comment #5
cweagans@Fabianx, does this happen anywhere a node is rendered with the Teaser view mode?
Comment #6
Fabianx CreditAttribution: Fabianx commented@cweagans: Yes, it should.
Comment #7
xjmOkay, reproduced. I'll update the summary with the STR.
Comment #8
xjmComment #9
xjmThis is 100% a release blocker.
Comment #9.0
xjmUpdated summary.
Comment #10
xjmNote that this is kind of WTF-y:
If #1810710: Remove copying of node properties wholesale into theme variables is fixed folks will be forced to update their themes to not use it (or have empty titles), but it's still a little odd to see
$title_prefix $title_attributes $title_suffix
but$label
. Might be worth discussing separately.Comment #11
Fabianx CreditAttribution: Fabianx commentedI would agree that $title should be kept for backwards compatibility as well, but as @xjm said that is different issue.
Comment #11.0
xjmUpdated issue summary.
Comment #12
xjmHere's a quick test demonstrating the bug, plus the fix. Note that this isn't actually the proper test; at a minimum
PageTitleFilteringTest.php
should be decoupled fromnode
by pulling the existing hunk for nodes along with the testing in each theme I've added here into a separate test. Ideally though we'd want to add coverage for other entity types and even other template variables and view modes.Comment #13
xjmWorking on a better test.
Comment #14
xjmHere we are. I wanted to be clever and do something like:
and then check
$output
, but I couldn't figure out how to gettheme()
to respond to the theme I was setting above. So, I've just used the standard profile.Comment #15
xjmSummary is incorrect here, and I should add a comment above the foreach. I'll reroll to add those two minor changes assuming the tests come back as expected.
Edit: and:
Would be better above the for loop.
And missing a docblock.
Comment #16
webchickLooping in the entity system and D8MI folks. This change comes from #1616952: Add a langcode parameter to EntityInterface::label(). Also pinged that issue with a note about this one.
Comment #17
Fabianx CreditAttribution: Fabianx commented@webchick: Uhm, I think it was this issue: #1642070: Make use of entity labels in templates and this change-notice: http://drupal.org/node/1776718
Comment #18
webchickOops. :) Right you are. Updated that issue instead.
Comment #19
xjmWith the cleanups from #15.
Comment #19.0
xjmIt is Fabianx not FabianX - that is someone else.
Comment #20
xjmErg, and I just realized I copied these incorrect docblocks as well.
Comment #22
xjmAnd a missing period. Sorry for the noise.
Comment #23
Alan Evans CreditAttribution: Alan Evans commentedReviewing ...
Comment #24
Alan Evans CreditAttribution: Alan Evans commentedThis is such a great test to have! Great to use on new contrib themes too.
Comment #25
Alan Evans CreditAttribution: Alan Evans commentedTested on a couple of contrib themes and revealed the bug there too :) Used the test to verify a patch against that theme. (I'm sure there will be plenty more though).
I'm wondering whether we need a new issue for expanding on this test a little. I think this is a great start, and easily enough to cover the bug here (wouldn't want to derail this issue as the test covers this case well and several others additionally), but it seems unlikely we've caught all the places where this could be an issue. Partially a question I guess of the circumstances under which various branches in template files get executed - it's possible that the majority of cases are actually covered here already in a relatively small number of page checks.
Just checking through Bartik in general, a couple of things jump out:
Comment #26
xjmRe: #25, the comment module isn't converted yet as far as I could see, unlike #1616962: Replace $node->title with $node->label() and #1616972: Replace $term->name with $term->label(). See template_preprocess_comment() -- it stores
$comment->subject
to$title
. If we do convert that we'll need to be careful of this bug, but at least this time we have tests to catch it. :)The title in
page.tpl.php
is covered byPageTitleFilteringTest
.And yeah, I'd love a followup issue to expand the test coverage, especially if someone can help with the bit I couldn't get working in #14. Then we could make it iterate over all known entity templates and view modes rather than hardcoding paths in the standard profile. :)
Comment #27
Alan Evans CreditAttribution: Alan Evans commented(Note - also verified that the test tests all available themes.)
Thanks for the clarification of comment and page titles.
Summary - really just the one minor nitpick on the variable type comment, otherwise can't fault this. Nice work! +1
Doesn't seem aligned with the actual default value.
Ahh, little Bobby Alert('JS') http://xkcd.com/327 ... Starts me thinking now whether we have explicit SQL injection tests ... another time.
Probably technically $this->node->type is preferable, but I doubt it's worth changing.
14 days to next Drupal core point release.
Comment #28
xjmWe surely do, but they're probably in the database layer tests. :)
Attached cleans up the other things in #27 and also improves the documentation a little. I'd like us to get in the habit of documenting why the given profiles and modules are used in a test.
Comment #29
xjmHelps if I actually upload the files.
Comment #31
xjmAnd if I don't include syntax errors.
Comment #32
Alan Evans CreditAttribution: Alan Evans commentedRechecked interdiff - +1 assuming testbot is happy.
Comment #33
Alan Evans CreditAttribution: Alan Evans commentedI swear I can't find any SQL injection tests still though (off topic here, just curious).
Comment #34
Fabianx CreditAttribution: Fabianx commentedFrom my point of view this is RTBC now, it fixes the issue, has extensive test coverage and some nice things for follow-up.
Comment #35
webchickVery nicely done, folks.
Committed and pushed to 8.x. Thanks!
Comment #36.0
(not verified) CreditAttribution: commentedUpdated issue summary.