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

  1. Install D8 with the standard profile.
  2. Create an article with the title <script>alert('XSS!');</strong> and a taxonomy tag foo.
  3. Visit the frontpage (main node listing). You will receive a JS alert.
  4. Visit the term listing for foo at taxonomy/term/1. You will again receive a JS alert.

This is not reproducible for:

Proposed resolution

See: template_preprocess_node()

Remaining tasks

Followups

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.

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

larowlan’s picture

Issue tags: +Needs tests

Tagging

xjm’s picture

Assigned: Unassigned » xjm

Yowza. Looking at a test.

xjm’s picture

Assigned: xjm » Unassigned
Status: Active » Postponed (maintainer needs more info)
FileSize
61.17 KB

I can't reproduce this locally. Here's what I did:

  1. Installed core, fresh DB, settings.php, and HEAD as of an hour ago, with the minimal profile.
  2. Enabled Bartik and set it as the default.
  3. Created a content type.
  4. Added a node with title: <em>Foo</em> <script>alert('XSS!');</script>

The title was properly escaped:

no_xss.jpg

Also this appears to have coverage already in PageTitleFilteringTest.

Can you provide more specific steps to reproduce?

Fabianx’s picture

Status: Postponed (maintainer needs more info) » Active
FileSize
12.77 KB
31.87 KB

Update: 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.

 xss alert
 xss with no title

cweagans’s picture

@Fabianx, does this happen anywhere a node is rendered with the Teaser view mode?

Fabianx’s picture

@cweagans: Yes, it should.

xjm’s picture

Okay, reproduced. I'll update the summary with the STR.

xjm’s picture

Assigned: Unassigned » xjm
xjm’s picture

Priority: Major » Critical

This is 100% a release blocker.

xjm’s picture

Issue summary: View changes

Updated summary.

xjm’s picture

Note that this is kind of WTF-y:

  <?php print render($title_prefix); ?>
  <?php if (!$page): ?>
    <h2<?php print $title_attributes; ?>>
      <a href="<?php print $node_url; ?>"><?php print $label; ?></a>
    </h2>
  <?php endif; ?>
  <?php print render($title_suffix); ?>

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.

Fabianx’s picture

Status: Needs review » Active

I would agree that $title should be kept for backwards compatibility as well, but as @xjm said that is different issue.

xjm’s picture

Issue summary: View changes

Updated issue summary.

xjm’s picture

Status: Active » Needs review
FileSize
4.5 KB
3.53 KB

Here'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 from node 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.

xjm’s picture

Status: Active » Needs work

Working on a better test.

xjm’s picture

Status: Needs work » Needs review
FileSize
4.43 KB
3.46 KB

Here we are. I wanted to be clever and do something like:

foreach ($this->themes as $theme) {
  variable_set('theme_default', $theme);

  $output = theme('node', array(
    'node' => $this->node,
      'elements' => array(
        '#view_mode' => 'teaser',
        '#node' => $this->node
      ),
    ));

and then check $output, but I couldn't figure out how to get theme() to respond to the theme I was setting above. So, I've just used the standard profile.

xjm’s picture

+++ b/core/modules/system/lib/Drupal/system/Tests/Theme/EntityFilteringThemeTest.phpundefined
@@ -0,0 +1,142 @@
+   * Tests the handling of HTML by drupal_set_title() and drupal_get_title()
+   */
+  function testThemedEntity() {
+
+    foreach ($this->themes as $theme) {
+      variable_set('theme_default', $theme);

Summary 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:

+++ b/core/modules/system/lib/Drupal/system/Tests/Theme/EntityFilteringThemeTest.phpundefined
@@ -0,0 +1,142 @@
+      // Check paths where various view modes of the entities are rendered.
+      $paths = array(
+        'user',
+        'node',
+        'node/' . $this->node->nid,
+        'taxonomy/term/' . $this->term->tid,
+      );

Would be better above the for loop.

+++ b/core/modules/system/lib/Drupal/system/Tests/Theme/EntityFilteringThemeTest.phpundefined
@@ -0,0 +1,142 @@
+class EntityFilteringThemeTest extends WebTestBase {

And missing a docblock.

webchick’s picture

Looping 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.

Fabianx’s picture

@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

webchick’s picture

Oops. :) Right you are. Updated that issue instead.

xjm’s picture

Assigned: xjm » Unassigned
FileSize
2.53 KB
4.54 KB
3.58 KB

With the cleanups from #15.

xjm’s picture

Issue summary: View changes

It is Fabianx not FabianX - that is someone else.

xjm’s picture

FileSize
841 bytes
4.47 KB
+++ b/core/modules/system/lib/Drupal/system/Tests/Theme/EntityFilteringThemeTest.phpundefined
@@ -0,0 +1,145 @@
+  /**
+   * Implement getInfo().
+   */
...
+  /**
+   * Implement setUp().
+   */

Erg, and I just realized I copied these incorrect docblocks as well.

Status: Needs review » Needs work

The last submitted patch, interdiff-20.patch, failed testing.

xjm’s picture

Status: Needs work » Needs review
FileSize
711 bytes
4.47 KB

And a missing period. Sorry for the noise.

Alan Evans’s picture

  • Reproduced the issue pre-patch
  • Patched and retested
  • Verified patch corrects the issue

Reviewing ...

Alan Evans’s picture

  • forced a fail in bartik and verified caught by the test
  • forced fails in the core template and verified caught by test

This is such a great test to have! Great to use on new contrib themes too.

Alan Evans’s picture

Tested 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:

  • There is a $title in comment.tpl.php - does this one need this treatement also? It is a core entity, but I'm not 100% sure whether it needs the same treatment. It looks like it most likely does not though.
  • There's also one in page.tpl.php ... but this one seems unlikely to be trouble, as there's no associated entity
xjm’s picture

Re: #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 by PageTitleFilteringTest.

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. :)

Alan Evans’s picture

Status: Needs review » Needs work

(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

+++ b/core/modules/system/lib/Drupal/system/Tests/Theme/EntityFilteringThemeTest.phpundefined
@@ -0,0 +1,139 @@
+   * @var array

Doesn't seem aligned with the actual default value.

+++ b/core/modules/system/lib/Drupal/system/Tests/Theme/EntityFilteringThemeTest.phpundefined
@@ -0,0 +1,139 @@
+    $this->user->name = $this->xss_label;

Ahh, little Bobby Alert('JS') http://xkcd.com/327 ... Starts me thinking now whether we have explicit SQL injection tests ... another time.

+++ b/core/modules/system/lib/Drupal/system/Tests/Theme/EntityFilteringThemeTest.phpundefined
@@ -0,0 +1,139 @@
+      'node_type' => 'article',

Probably technically $this->node->type is preferable, but I doubt it's worth changing.

14 days to next Drupal core point release.

xjm’s picture

Starts me thinking now whether we have explicit SQL injection tests.

We 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.

xjm’s picture

Status: Needs work » Needs review
Issue tags: -Needs tests
FileSize
1.52 KB
4.66 KB

Helps if I actually upload the files.

Status: Needs review » Needs work

The last submitted patch, bartik-1811684-28.patch, failed testing.

xjm’s picture

Status: Needs work » Needs review
FileSize
1.52 KB
4.66 KB

And if I don't include syntax errors.

Alan Evans’s picture

Rechecked interdiff - +1 assuming testbot is happy.

Alan Evans’s picture

I swear I can't find any SQL injection tests still though (off topic here, just curious).

Fabianx’s picture

Status: Needs review » Reviewed & tested by the community

From my point of view this is RTBC now, it fixes the issue, has extensive test coverage and some nice things for follow-up.

webchick’s picture

Status: Reviewed & tested by the community » Fixed

Very nicely done, folks.

Committed and pushed to 8.x. Thanks!

Automatically closed -- issue fixed for 2 weeks with no activity.

Anonymous’s picture

Issue summary: View changes

Updated issue summary.