Updated: Comment #26

Problem/Motivation

$name is not sanitized and can cause possible security problems if not handled properly in template.

Steps to reproduce

  • Add a new taxonomy term with code in its name:
    <script>alert("script");</script>
  • Print $name in taxonomy-term.tpl.php . Alert window will appear.

Proposed resolution

  • In D7 $name is not sanitized inside (taxonomy-term.tpl.php) and needs to be sanitized to prevent possible security problems.
  • Allow basic tags such i,span,b,em etc inside term name to prevent breaking existing sites.

Remaining tasks

  • Sanitize $name variable using filter_xss() or other function . We can still allow users to use basic tags (Someone may be using i,span tags inside term name)

Original report by borisbaldinger

In template_preprocess_taxonomy_term() the $variables array and the $term object are merged:

<?php
$variables
= array_merge((array) $term, $variables);
?>

This is not really required because the whole term is available within the variables:

<?php
$variables
['term']
?>
Files: 
CommentFileSizeAuthor
#18 term-name-sanitized-1751054-18.patch1.29 KBDavid_Rothstein
PASSED: [[SimpleTest]]: [MySQL] 40,737 pass(es).
[ View ]
#14 drupal-taxonomy_term_tpl-1751054-12.patch652 bytesvomiand
PASSED: [[SimpleTest]]: [MySQL] 39,819 pass(es).
[ View ]
#11 term-name-documentation-1751054-11.patch652 bytesBerdir
PASSED: [[SimpleTest]]: [MySQL] 39,387 pass(es).
[ View ]
#8 drupal-taxonomy-term-1751054-7.patch1.24 KBborisbaldinger
PASSED: [[SimpleTest]]: [MySQL] 40,384 pass(es).
[ View ]
#1 drupal-taxonomy-term-1751054.patch1.24 KBborisbaldinger
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch drupal-taxonomy-term-1751054.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]

Comments

StatusFileSize
new1.24 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch drupal-taxonomy-term-1751054.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]

I removed the merge of the template variables array and the term. I think we don't need to backport this to D7 because many people are using the flattened variables.
Also i removed the $name from the comments in the template file because this variable isn't available anymore because it came from the merger of the term with the variables array.

Status:Active» Needs review

+1 from me, found this during working on #1642070: Make use of entity labels in templates because it is like an defacto standard, that variables which are non arrays are "secure", and if an themer uses $term->something he is more carefull :)

Status:Needs review» Reviewed & tested by the community

Second that, agree that this should be removed. There's no need to have $name and $term_name/label.

Issue tags:+needs backport to D7

adding tags, because:
* - $name: the (sanitized) name of the term.
is actually wrong! it is not sanitized.

Issue tags:-needs backport to D7

#1: drupal-taxonomy-term-1751054.patch queued for re-testing.

Status:Reviewed & tested by the community» Needs work
Issue tags:+needs backport to D7

The last submitted patch, drupal-taxonomy-term-1751054.patch, failed testing.

Status:Needs work» Needs review
StatusFileSize
new1.24 KB
PASSED: [[SimpleTest]]: [MySQL] 40,384 pass(es).
[ View ]

Rewrote the patch on the actual codebase of drupal 8.x

Status:Needs review» Reviewed & tested by the community

reroll looks good. RTBC

Version:8.x-dev» 7.x-dev
Status:Reviewed & tested by the community» Patch (to be ported)

Committed/pushed to 8.x.

I'm not sure if this can be backported, but I'll leave that to webchick/David as to balancing the bug fix vs. API change. Whether it is or not we'll need a change notification I think.

Status:Patch (to be ported)» Needs review
StatusFileSize
new652 bytes
PASSED: [[SimpleTest]]: [MySQL] 39,387 pass(es).
[ View ]

I think we just want to fix the documentation here, everything else would be a API change.

Here's a try, not sure if this is correct in regards to coding/documentation standards.

Issue tags:+Novice

+++ b/modules/taxonomy/taxonomy-term.tpl.phpundefined
@@ -5,7 +5,8 @@
+ * - $name: (deprecated) the unsanitized name of the term, use $term_name
+ *   instead.

This looks reasonable to me. Two small fixes: "The" should be capitalized, and it should be two sentences rather than joined with a comma. (This is an example of what prescriptive English teachers call a comma splice.)

Assigned:borisbaldinger» Unassigned
Status:Needs review» Needs work

Status:Needs work» Needs review
StatusFileSize
new652 bytes
PASSED: [[SimpleTest]]: [MySQL] 39,819 pass(es).
[ View ]

Hi

I fixed issue from comment #12.

Please review.

I hope this helps.

Status:Needs review» Reviewed & tested by the community

Looks good.

Thanks @vomiand!

Status:Reviewed & tested by the community» Fixed
Issue tags:+7.22 release notes

Wow, that's a pretty serious documentation bug (worthy of a release notes mention)...

Committed to 7.x - thanks! http://drupalcode.org/project/drupal.git/commit/ba9b60d

Title:Taxonomy template variables are getting merged with the term itselfTaxonomy template variables are getting merged with the term itself (security aspects of $name variable were documented wrong)
Priority:Normal» Major
Status:Fixed» Needs review
Issue tags:+Security improvements
StatusFileSize
new1.29 KB
PASSED: [[SimpleTest]]: [MySQL] 40,737 pass(es).
[ View ]

You know, the fact that that documentation mistake has been there for so long makes me think we should really just sanitize it. Too much chance people out there have security holes in their themes and are being led by the documentation to believe they don't.

Issue tags:-Security improvements

Yeah, that seems worthwhile to me. Two questions, though:

  1. Do we risk breaking sites that intentionally put markup in term names, or something? (Maybe it's worth it given the security implications.)
  2. Do we want a test for D7 confirming that it's sanitized? (D8 has a bunch of these already).

Issue tags:+Security improvements

Hm, you are right, $term_name is run through check_plain() so you can't have any markup in there at all.

I don't know... Deliberately putting HTML in a taxonomy term name sounds like an unusual thing to try to do, but it's possible someone is doing it. We could run $name through filter_xss() or something instead, although that seems wasteful for a variable that very few people are likely to even be using.

Tests sound like a good idea also :)

Hm, given that term names are already check_plain()'d everywhere (see e.g. taxonomy_field_formatter_view()), it seems highly unlikely that someone has been using tags in them :)

+1 from me.

Issue tags:+Needs tests

Alright, let's just do this then.

core/modules/system/lib/Drupal/system/Tests/Theme/EntityFilteringThemeTest.php in D8 might be a starting point for ideas on how to test it.

Could someone provide a bit more clear info of what exactly should be tested?

http://drupal.org/node/1427826 contains instructions for updating the issue summary with the summary template.

The summary may need to be updated with information from comments.

Issue summary:View changes

Updated issue summary.

Issue summary:View changes

Attended on DrupalCon Prague 13 mentored sprint and picked up the last comment and attempted to update and fix the issue summary.

As the wrong comment has been there for so long might be worth to sanitize the variable and left the comment there because this makes it true for D7. In D7 $name in unsanitized. Inside (taxonomy-term.tpl.php).

We can sanitize $name variable using filter_xss() or other function to prevent possible security problems. We can still allow users to use basic tags (Someone may be using i,span tags inside term name).

Issue summary:View changes

Updated issue summary.

Issue summary:View changes

Updated issue summary.

Issue summary:View changes

Adding steps to reproduce & Problem/Motivation

Issue summary:View changes

Updated issue summary.