Themers and Developers consider $title safe in node.tpl.php.

Since #1642070: Make use of entity labels in templates the variable $title was removed in favor of label as described here: http://drupal.org/node/1776718

The problem:

$title is still set, but is now insecure while it has been secure since 2007. As it will continue to work, developers will just continue using D7 templates, create code by old habits, etc. and such accidentally introduce XSS issues.

Reproduce:

Change $label to $title in node.tpl.php (or node.twig once basic node.twig is in with autoescape off) and use title of

<script>alert('xss');</script>

Suggested resolution (for this issue):

unset($variables[$title]);

or make secure via check_plain() after the node object has been flatted with code like this:

  // Flatten the node entity's member fields.
  $variables = array_merge((array) $node, $variables);

_This_ introduces the XSS and this is really difficult to see that a simple change from title to label could have such consequences.

Action

Lets fix it. (Shameless plug: With twig and autoescape enabled $title would automatically be secure ;-) )

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

chx’s picture

Title: XSS: node.tpl.php exposes insecure $title after change from $title to $label » Nuke // Flatten the node entity's member fields. from orbit
Category: bug » task
chx’s picture

Title: Nuke // Flatten the node entity's member fields. from orbit » Remove copying of node properties wholesale into theme variables
Berdir’s picture

This has already been done for the taxonomy term template, it only makes sense to do it here as well.

xjm’s picture

Berdir’s picture

Status: Active » Needs review
FileSize
657 bytes

Change is trivial, trying to see what breaks.

Status: Needs review » Needs work

The last submitted patch, remove-copying-node-to-variables-1810710-5.patch, failed testing.

Berdir’s picture

Status: Needs work » Needs review
FileSize
2.91 KB

Ok, let's see if I catched all of these wrong references.

Status: Needs review » Needs work

The last submitted patch, remove-copying-node-to-variables-1810710-7.patch, failed testing.

Berdir’s picture

Status: Needs work » Needs review
FileSize
4.27 KB

Had also to fix calls to rdf_mapping. This should pass.

chx’s picture

Status: Needs review » Reviewed & tested by the community

Good to go.

webchick’s picture

Status: Reviewed & tested by the community » Needs work

Hm. This looks fine to me, but afaics we just did exactly the opposite of this over at #1272870: No semantics for nested comments / bad for screen-readers, with the justification that themers want the option of having fine-grained control over their variables in templates. We should figure out a unified approach we can standardize across all templates.

node.tpl.php's docs need to be updated to reflect these variables being removed, so needs work for that.

Berdir’s picture

@webchick: These two issues don't conflict each other. The comment patch adds a number of explicit variables to the template, that is totally fine and not the problem here.

The problem here is the "ADD THEM ALL!!!"-approach of adding *everything* in $node to $variables, without making sure that it's safe for display.

Will check if the node.tpl.php templates need to be updated, it shouldn't be listing additional variables anyway.

Berdir’s picture

Status: Needs work » Needs review
FileSize
9.09 KB

Updated the node.tpl.php documentation to refer to the corresponding $node->something properties instead of variables. Not sure what exactly happens with this documentation in the twig version.

I also copied over the documentation to bartik's node.tpl.php, there's a changed block at the end which I left like that, I think it doesn't hurt to have them in sync again and again, Twig is going to rip that out anyway, right?

xjm’s picture

+++ b/core/modules/node/templates/node.tpl.phpundefined
@@ -42,10 +42,10 @@
- * - $type: Node type; for example, page, article, etc.
- * - $comment_count: Number of comments attached to the node.
- * - $uid: User ID of the node author.
- * - $created: Time the node was published formatted in Unix timestamp.
+ * - $node->type: Node type; for example, page, article, etc.
+ * - $node->comment_count: Number of comments attached to the node.
+ * - $node->uid: User ID of the node author.
+ * - $node->created: Time the node was published formatted in Unix timestamp.

So this is actually different from the approach I thought to take in this issue. I think that "safe" properties like these should be copied manually to the template, and just the blind extraction of everything from the object should go away.

Fabianx’s picture

In twig using $node->type is definitely easier as you just need to use {% if node.type %}, but for that you need to know that this is a node template.

I vote for copying variables manually wholesale for now and leave the preprocess cleanup to twig - when it happens.

I pinged jenlampton to comment here as well.

jenlampton’s picture

Status: Needs review » Needs work

I agree with @Fabianx

As part of the change to Twig, we'll be removing all the fine-grained variables from the preprocess layer anyway, leaving them there would just be waste. Theme devs can get at all the info they need, at any level, securely, via node.title, node.author, node.content.image.first.src, etc

One other thing it may not be worth mentioning here, is that I'm also proposing we rename all the variables in all templates to be consistent with one another - so we may be getting rid of anything called 'label' in the node template. We should call it 'title' by the time it gets to a template. see #1825216: Name variables consistently across all templates (preprocess)

I also agree with @xjm that we certainly should not be encouraging theme devs to print anything insecure directly from the node object, by giving them something to cut & paste from the docs that will get them in trouble.

Let's just leave the existing variables for now and we can cleanup as part of the theme system / preprocess overhaul we'll be doing with Twig :)

Berdir’s picture

Status: Needs work » Needs review
FileSize
4.5 KB

Sure, can do :)

I added all properties that are documented explicitly, I kept however the changes in the preprocess functions and did not add variables which are used there. Like langcode and rdf_mapping (there is quite a mess in rdf.module in regards to that, 50% of the call currently call it on the node and the others on variables).

No interdiff, that would probably be bigger than the patch anyway.

Status: Needs review » Needs work

The last submitted patch, remove-copying-node-to-variables-1810710-17.patch, failed testing.

Berdir’s picture

So...

comment_count obviously doesn't exist if comment.module isn't enabled, but is documented in node.tpl.php. What do we want to do with that? define in comment_preprocess_node() and keep the documentation?

And readmore is actually not used anywhere, with the exception of poll.module, which sets it to FALSE because nobody told poor poll.module that there is no such thing anymore :) So that feels like an obvious thing that we can remove?

Fabianx’s picture

comment_count: I think I would define it to NULL by default and copy from $node if it exists. That way all variables are available always, but can be NULL.

( Looping with isset() might even be enough. )

readmore: Get rid of it then I think :-).

Berdir’s picture

Ok, make sure that all the documented properties exist and removed readmore.

Berdir’s picture

Fabianx’s picture

Status: Needs review » Reviewed & tested by the community
+++ b/core/modules/rdf/rdf.moduleundefined
@@ -527,7 +527,7 @@ function rdf_preprocess_node(&$variables) {
       '#attributes' => array(
-        'content' => $variables['title'],
+        'content' => $variables['label'],
         'about' => $variables['node_url'],

That is another good catch of the title property!

Looks all good to me ...

=> RTBC

webchick’s picture

Status: Reviewed & tested by the community » Fixed

Committed and pushed to 8.x. Thanks!

Status: Fixed » Closed (fixed)

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

Anonymous’s picture

Issue summary: View changes

markup, reproduce