Core should consistently filter_xss_admin() on $site_slogan and check_plain $site_name

Here's the HEAD and 6.x-dev patches.

Looks good, footer and mission are gone in D7 but this looks like a backport to D6 as well.

Setting to previous status - testbot was broken.

Committed to CVS HEAD. Moving to D6.

The D6 patch was included as well and the changes are identical. Ready for commit?

Committed to Drupal 6, thanks!

Applied the patch and my D7 HEAD did still function. I added some &%$§ to the head and it works. From the code I'd say its okay.

Cool, let's get some tests for this please.

Okay the test seems quiet easy to me. I just enter some markup for the title like "###< / title > ###". Without the patch the html is broken. With this patch I can check for the escaped string inside the real < title > and everything is fine. Done this by hand and it testable.

But I have a question before I go coding this as I am new to writing tests for core:

Where should it go to?

Is there already a testcase it can be added or should this go in a new file to setup where?

And one more thing just appeared to me:

I thought slogan is now a block? If I activate "slogan" in Garland as this is patched by this patch as well it is added to the site like it was in D6? I mean this is a problem about writing the test. Should I add the slogan block or just activate garlands settings?

Thanks to stBorchert I figured that I should put this into system.test. So I made a little test to check if site name and slogan are escaped correctly. And I reworked the patch to latest HEAD.

wrong path...

Reroll against latest HEAD.

I believe we want filter_xss_admin() instead, since these values are entered by the administrator.

Also, could you please add a line of PHPDoc to:

+  function testTitleXSS() {

filter_xss_admin() for slogan and check_plain() for the site_name? If Garland did this before for site_name and it did workout why don't keep this behavior?
Otherwise I fear we would need more context where and why site_name is used or split it up in different variables. Both not really desirable to me.

Sure will do probably tomorrow :)

I currently can't test the slogan with garland.
#602372: $site_slogan no longer used in page.tpl.php

I don't know if changing the default theme to stark during the test is an appreciated thing to do. Should tests always use garland?

D6 patch attached.

Okay, here's a patch that uses check_plain() for $site_name.

Incidentally, I ran into this problem when I ran into the ampersand issue as in #9.

New Patch against latest HEAD.

As discussed site_name is check_plain() and slogan filter_xss_admin(). The test for the slogan is escaped right now. And I did add a short comment.

Why is this tagged with Minelli as it just adds CSS and no tpl files or do I miss something?

Tested the patch in #29 on D6. Confirming it fixes the double-escaping of characters such as '&' in site titles in Garland.

Removing Minnelli tag since Garland and Minnelli have been merged into one theme now, and bumping to normal (since a lot of sites want to use ampersands in their site name).

Patch in #30 works for me. Anybody else care to RTBC?

Patch in #30 works on D7 from CVS (with offsets).

RTBC.

Tagging with DrupalWTF, because, really, not managing to get a plain old ampersand in a site name is an embarrassment.

Setting this back to "needs work" as it just came in from #602372: $site_slogan no longer used in page.tpl.php that garland now uses filter_xss_admin in template.php.

I believe we could ignore that the variables are filtered twice but lets get this clean once and for all.

This Garland Patch did add more use of filter_xss_admin() and did access the variables site_name and site_slogan directly. But finally we can check slogan within Garland again.

I did change this to $vars['site_slogan']; and $vars['site_name']; so we use the globally filtered versions.

Now the patch runs fine again and tests if slogan and title are XSS proof.

Did use wrong path for patch resubmitting.

subscribing

Committed to CVS HEAD. Thanks.

Let's backport this to 6 -- the patch from #29 worked on 6 when I tested it in December.

The patch omitted some occurrences. This fixes a bunch of them, including when site_name is used in mails (e.g. the lost password mail).

Looks good, could be rtbc, though I see that aggregator.pages.inc has a check_plain that's no longer necessary? Separate issue?

function theme_aggregator_page_opml($variables) {
  $feeds = $variables['feeds'];

  drupal_add_http_header('Content-Type', 'text/xml; charset=utf-8');

  $output  = "<?xml version=\"1.0\" encoding=\"utf-8\"?>\n";
  $output .= "<opml version=\"1.1\">\n";
  $output .= "<head>\n";
  $output .= '<title>' . check_plain(variable_get('site_name', 'Drupal')) . "</title>\n";
  $output .= '<dateModified>' . gmdate(DATE_RFC2822, REQUEST_TIME) . "</dateModified>\n";
  $output .= "</head>\n";
  $output .= "<body>\n";
  foreach ($feeds as $feed) {
    $output .= '<outline text="' . check_plain($feed->title) . '" xmlUrl="' . check_url($feed->url) . "\" />\n";
  }
  $output .= "</body>\n";
  $output .= "</opml>\n";

  print $output;
}

@markabur: Which check_plain() in that function is not necessary?

I was looking at the one wrapping the site_name:

check_plain(variable_get('site_name', 'Drupal'))

Isn't it already plain by this point?

The site_name variable contains the raw unescaped string. So we should always pass it through check_plain() when outputting it in HTML or XML.

Ok, I see. I was thinking that template_preprocess_page was affecting site_name there.

Cross-linking #655742: HTML HEAD title cannot be safely output differently - looks like I didn't caught maintenance page preprocessing over there.

subscribe.

@sun is this needs work then?

Nope, all fine (although I didn't do an in-depth review). Sorry for the confusion.

+++ modules/user/user.module	15 Apr 2010 21:35:25 -0000
@@ -2305,19 +2305,19 @@ function user_build_content($account, $v
-  $message['subject'] .= _user_mail_text($key . '_subject', $language, $variables);
+  $message['subject'] .= str_replace("\n", " ", drupal_html_to_text(_user_mail_text($key . '_subject', $language, $variables)));

Why this change? Looks odd.

This patch is pretty hard to review which suggest something is wrong -- we need to figure out how to simplify this in D8 or so.

Yes, that change is wrong. Mails are HTML by default in D7 and may be converted to plain-text prior to sending by the mail engine only.

Subjects are always just plain-text AFAICT, but the output from _user_mail_text() is HTML-like.

This patch uses a slightly different approach. It adds a new argument to _user_mail_text() that tells whether to output in HTML or plain-text. system_tokens() is adjusted to convert site_slogan to plain-text when $options['sanitize'] is FALSE. I assume the intention behind the 'sanitize' option for token_replace() is to determine, whether the tokens should be formatted as HTML or as plain-text. It also removes a few occurrences of site_mission that were a left-over from #428800: Convert mission statements to a region with blocks.

+++ modules/system/system.tokens.inc	26 Apr 2010 20:19:17 -0000
@@ -155,24 +155,19 @@ function system_tokens($type, $tokens, a
           $replacements[$original] = $sanitize ? check_plain($site_name) : $site_name;
...
-          $replacements[$original] = $sanitize ? check_plain($slogan) : $slogan;
...
-          $replacements[$original] = $sanitize ? filter_xss($mission) : $mission;
+          $replacements[$original] = $sanitize ? filter_xss_admin($slogan) : decode_entities(strip_tags($slogan));

When not sanitizing, the value should be returned as-is. @see http://api.drupal.org/api/function/token_replace/7

+++ modules/system/system.tokens.inc	26 Apr 2010 20:19:17 -0000
@@ -155,24 +155,19 @@ function system_tokens($type, $tokens, a
-        case 'mission':

Why do we remove the [site:mission] token?

+++ modules/user/user.module	26 Apr 2010 20:19:17 -0000
@@ -2308,28 +2308,28 @@ function user_build_content($account, $v
 function user_mail($key, &$message, $params) {
   $language = $message['language'];
   $variables = array('user' => $params['account']);
-  $message['subject'] .= _user_mail_text($key . '_subject', $language, $variables);
-  $message['body'][] = _user_mail_text($key . '_body', $language, $variables);
+  $message['subject'] .= _user_mail_text($key . '_subject', $language, $variables, TRUE, FALSE);
+  $message['body'][] = _user_mail_text($key . '_body', $language, $variables, TRUE, TRUE);
 }

This looks wonky, but the more I think about it, the more I think it's correct.

The mail body should be HTML, which means that we need to sanitize it. When sending plain-text mails, the mail engine will invoke drupal_html_to_text(), which in turn will convert all HTML to plain-text.

The mail subject only seems to be processed via http://api.drupal.org/api/function/mime_header_encode/7 in DefaultMailSystem::mail(), which also makes me wonder whether we're not missing some fundamental sanitation for security here. Why don't we sanitize the subject here?

At the very least, we need to explain this with inline comments for each message element here.

96 critical left. Go review some!

When not sanitizing, the value should be returned as-is. @see http://api.drupal.org/api/function/token_replace/7

Isn't that a weird/useless/dangerous behaviour? If I replace tokens without sanitation, and the tokens are inserted as-is, I wouldn't know which format the output string is. Some tokens are HTML (e.g. site_slogan) and some are plain-text (e.g. site_name), and they are all inserted into the same string that would become some undefined mix of HTML and plain-text. I cannot just pass the output string through check_plain() or filter_xss() without changing the meaning of the string.

I think there may be two meaningful behaviours of 'sanitize' == FALSE: Either 1) the output is supposed to be plain-text, i.e. with no entites or tags, or 2) the output is supposed to be unsanitized HTML, i.e. site_name should still be check_plain()'ed. We can always derive the plain-text version from the sanitized or unsanitized HTML versions using decode_entities(strip_tags()). We cannot derive the unsanitized version from the sanitized, so perhaps we should stick with option 2?

Why do we remove the [site:mission] token?

site_mission was removed in #428800: Convert mission statements to a region with blocks. This is just a left-over.

Why don't we sanitize the subject here?

Mail subjects are plain-text. Mail clients do not interpret subjects as HTML, so they shouldn't be escaped used check_plain().

Good thinking. 2) makes more sense to me. Exceeds my horizon, even though I heavily reviewed + approved the token-in-core patch. We likely need to pull eaton into this issue. And, we likely need to review all other token replacements.

As this might have security implications, bumping to critical.

This patch implements option 2).

Re #56:

As this might have security implications, bumping to critical.

Adding the tag too, in that case.

I saw a filter.module hunk in another issue dealing with this, which isn't included here - I think we should do all these at once.

This isn't an exploitable security issue, because both of those require 'administer site configuration', which already lets you take over the site, see http://drupal.org/Security-announcements-process-policy. So not a release blocker, but should still be fixed for consistency.

+1 for the D6 backport. this is embarrasing that a simple ampersand will not show correctly....

Another +1 for the D6 backport. Ampersands are found everywhere in English, at least. It's frustrating not to be able to use them.

Reroll + updated documentation in system.api.php and token.inc.

I don't really agree with the changes to token behavior at this late in the game.

This is a patch without the token changes.

I'll file a separate issue for the token stuff. I hope that we can fix both, though.

In that case, we need to move all available discussion around #55 into a separate issue, which should be critical. What has been described above sounds like a violation of the safe string theory for the web.

I filed #974782: Resulting string format of token_replace(..., array('sanitize' => FALSE)) is undefined about the token issue.

Reroll.

+++ themes/garland/template.php	24 Nov 2010 23:29:27 -0000
@@ -110,19 +110,19 @@ function garland_preprocess_page(&$vars)
   // Set a variable for the site name title and logo alt attributes text.
   $slogan_text = $vars['site_slogan'];
   $site_name_text = $vars['site_name'];
-  $vars['site_name_and_slogan'] = $site_name_text . ' ' . $slogan_text;
+  $vars['site_name_and_slogan'] = strip_tags($site_name_text . ' ' . $slogan_text);

I don't understand this change.

Powered by Dreditor.

$site_name_and_slogan is only used inside attributes of an tag where HTML tags are not allowed:

<img src="<?php print $logo ?>" alt="<?php print $site_name_and_slogan ?>"
     title="<?php print $site_name_and_slogan ?>" id="logo" />

+++ themes/garland/template.php	24 Nov 2010 23:29:27 -0000
@@ -110,19 +110,19 @@ function garland_preprocess_page(&$vars)
   $slogan_text = $vars['site_slogan'];

Let's do the strip_tags() as well as check_plain() here instead;i.e.:

$slogan_text = check_plain(strip_tags($vars['site_slogan']));

$site_name_text is already check_plain()'ed, so it doesn't contain any tags.

Powered by Dreditor.

Done.

I also added strip_tags() arround filter_xss_admin() at to the two occurences of the following line (because we cannot have tags in the <title> element):

      $head_title['slogan'] = filter_xss_admin(variable_get('site_slogan', ''));

+++ themes/garland/template.php	28 Nov 2010 01:24:52 -0000
@@ -108,19 +108,19 @@ function garland_preprocess_page(&$vars)
   // Set a variable for the site name title and logo alt attributes text.
-  $slogan_text = $vars['site_slogan'];
+  $slogan_text = strip_tags($vars['site_slogan']);

We need a wrapping check_plain() here, because it is going to be output in an HTML attribute.

Powered by Dreditor.

$vars['site_slogan'] has been passed through filter_xss_admin() in template_preprocess_page(), so it contains sanitized HTML with tags and valid entities. Calling strip_tags() removes the tags but preserves the entities. Wrapping it in check_plain() would double-encode the entities, and we don't want that. Right?

Reroll.

Is this actually still a problem on D7?

I just tried setting my site name to 'D7 & co' and it worked fine -- ampersands being the prime example case for this bug.

(still needs fixing on D6 though)

subscribing

Yes, it is still a problem in D7 and D8. The problem can be seen if you set the site name to a & b. In some places it is displayed as a & b and in some places as a & b.

Here is a reroll for D8.

Subscribing.

Reroll after #22336: Move all core Drupal files under a /core folder to improve usability and upgrades.

#85: site-name-slogan-follow-up-8.patch queued for re-testing.

Raising severity because this bug makes us vulnerable to XSS (requires "administer site configuration" permission), see #1506408: Bartik maintenance page site_name/site_slogan vulernable to xss.

@c960657 note that "administer site configuration" is on the list of permissions that are considered to be able to take over a site http://drupal.org/security-advisory-policy

Let's settle on major based on http://drupal.org/node/45111 and the fact that this requires an administrator to even input the XSS.

#85: site-name-slogan-follow-up-8.patch queued for re-testing.

Almost none of this applies now that #1496542: Convert site information to config system is in.

Should be a novice reroll.

Hi, trying my hands on this although I can't find where system.test is now.

@dsdeiz: The system test you are looking for is now found here: core/modules/system/lib/Drupal/system/Tests/System/TokenReplaceTest.php

Ah cool. Thx!

This really needs a proper issue summary and STR... Meanwhile, I suggest referring to comments #12 and #80 for a starting place re: manual tests.

iflista

#1: 461938-1.patch queued for re-testing.

#1: 461938-1-D6.patch queued for re-testing.

iflista

iflista

Updated issue summary.

Updated issue summary.

Issue summary updated.

I manually compared + reviewed the original #77 with the latest patch and the changes are still the same and valid.

The essential changes:

1) Any values output for the TITLE in HEAD must not contain HTML markup. (d'oh)

2) The site_name template variable is changed to consistently contain a escaped/sanitized string ready for output in HTML. The previous code used filter_xss_admin() in some but not all locations. But the site_name variable is first and foremost used to output it literally (or alternatively, in HTML attributes). With this patch, any theme that's doing unholy things with site_name needs to retrieve the original on its own.

3) The site_slogan variable was not escaped/sanitized at all for the maintenance page, but requires filter_xss_admin() at minimum. This means that any unholy SCRIPT and other stuff within the site slogan is stripped off.

4) The site:slogan token is adjusted accordingly. That one was more and actually too secure previously, as it used check_plain() instead of filter_xss_admin().

All of this makes sense.

The latest patch also applies cleanly, so we're ready to move forward.

+++ b/core/includes/theme.inc
@@ -2564,7 +2564,7 @@ function template_preprocess_page(&$variables) {
-  $variables['site_name']         = (theme_get_setting('toggle_name') ? filter_xss_admin($site_config->get('name')) : '');
+  $variables['site_name']         = (theme_get_setting('toggle_name') ? check_plain($site_config->get('name')) : '');

@@ -2778,8 +2778,8 @@ function template_preprocess_maintenance_page(&$variables) {
-  $variables['site_name']         = (theme_get_setting('toggle_name') ? $site_name : '');
...
+  $variables['site_name']         = (theme_get_setting('toggle_name') ? check_plain($site_name) : '');

FWIW, this doesn't look backportable to me.

Sorry, I'm blatantly mistaken; the patch doesn't apply at all anymore. Massive havoc.

Not putting back at RTBC, but #94 applies cleanly for me. Slight offset with patch -p1, no complaints from git apply.

Jesus. Please ignore me! Applying the identical patch twice obviously leads to 100% rejects. ;) Back to RTBC. :)

Committed to 8.x. Thanks!

I made the backport to d7, here it is the patch

The change to D7's TokenReplaceTest is missing.

Furthermore:

+++ b/includes/theme.inc
@@ -2482,7 +2482,7 @@ function template_preprocess_html(&$variables) {
-      $head_title['slogan'] = filter_xss_admin(variable_get('site_slogan', ''));
+      $head_title['slogan'] = strip_tags(variable_get('site_slogan', ''));

This needs both filter_xss_admin() and strip_tags() -- please check the D8 patch.

+++ b/includes/theme.inc
@@ -2539,7 +2539,7 @@ function template_preprocess_page(&$variables) {
-  $variables['site_name']         = (theme_get_setting('toggle_name') ? filter_xss_admin(variable_get('site_name', 'Drupal')) : '');
+  $variables['site_name']         = (theme_get_setting('toggle_name') ? check_plain(variable_get('site_name', 'Drupal')) : '');

@@ -2747,8 +2747,8 @@ function template_preprocess_maintenance_page(&$variables) {
-  $variables['site_name']         = (theme_get_setting('toggle_name') ? variable_get('site_name', 'Drupal') : '');
...
+  $variables['site_name']         = (theme_get_setting('toggle_name') ? check_plain(variable_get('site_name', 'Drupal')) : '');

As mentioned before, I'm really not sure whether we can backport this particular change.

+++ b/includes/theme.inc
@@ -2747,8 +2747,8 @@ function template_preprocess_maintenance_page(&$variables) {
-  $variables['site_slogan']       = (theme_get_setting('toggle_slogan') ? variable_get('site_slogan', '') : '');
...
+  $variables['site_slogan']       = (theme_get_setting('toggle_slogan') ? strip_tags(filter_xss_admin(variable_get('site_slogan', ''))) : '');

This needs filter_xss_admin() only.

I added the missing part and changed the other sutff

[edit] sorry, I posted on the wrong tab, my mistake

iflista

Updating remaining tasks

I can confirm that sun's "Missing parts" is applied in afeijo patch.
I have applied it locally, no problem. Also i have ran simple test, and put the patch thought Drupal code sniffer, no issues.

@sun's comment in #106 ("I'm really not sure whether we can backport this particular change") doesn't seem to have been discussed or addressed yet.... And the code he refers to looks suspicious to me also. Someone might be calling check_plain() in a template file, so adding a new one here would cause things to be double-escaped.

Also, in general, it seems like this patch is going to enforce a "no HTML in site names" rule that wasn't enforced (or at least was only partially enforced) before? If I have a theme that prints <h1 class="site-name">$site_name</h1> at the top of the page and I expect $site_name to have some HTML in it, that seems reasonable and I don't see why we want to change that, especially in Drupal 7. In short, my issue here (and I think it applies to Drupal 8 too) is that it's not clear to me from reading this issue why $site_slogan is consistently getting filter_xss_admin() called on it as part of this patch (and strip_tags(filter_xss_admin()) when it's being output in places like <head>) but $site_name is getting something different. Why is the site name fundamentally different from the site slogan?

I think there's two separate issues here:

1. I can use HTML in my site's name - I'm not sure if this has been decided or not as to whether this is legitimate
2. I can use ampersands (and possibly other non-alphanumeric chars) in my site name - this is a bug which I originally reported and it got merged into this mega-thread

Can we come up with a way of fixing #2 first and getting that into Drupal, and then decide if #1 is something that should be done, and if so, come up with a way of achieving that separately?

I joined this thread at #63 in June 2010, wanting to be able to put an ampersand in my site's title. It's a bit disheartening to see that a simple thing like that has taken more than 2 years and is still going!

Moving back to 8.x per #112 for more discussion.

#107: Inconsistent-use-of-filter_xss_admin-461938-107.patch queued for re-testing.

Removing Novice tag until discussion and a decision is made.

I don't know why this is in 8.x queue. What is the outstanding question that affects 8.x? Moving to 7.x since I think that's resolved.

For 7.x I think the question/concern is that we might break contrib themes and a question of whether or not that's OK. In my opinion that's OK. It's why we have release notes :)

Because this patch introduced behaviors in Drupal 8 that appear to be a regression from Drupal 7...

You can reproduce this with Bartik (no contrib or custom theme needed) and the following site name (pathological example chosen to get all cases in here at once):

Text & <em>HTML</em> & <script type="text/javascript">alert('xss');</script>

In Drupal 7, if you look at your site header (i.e. the primary place where your site name is displayed), it all works correctly. The word "HTML" is displayed italicized, the "&" displays as expected, and the script tags are filtered out to neutralize the XSS attack.

In Drupal 8, meanwhile, everything is escaped, and thus you can't have the word "HTML" italicized in your site name anymore.

We do need to standardize on something, but I think it's still very unclear why Drupal 8 is currently standardizing on check_plain() rather than filter_xss_admin() (and in particular why it's making the site name behave differently from the site slogan)... If that's really intentional, I guess this issue can go back to Drupal 7 (and maybe needs a Drupal 8 change notice?) but I'm still not clear on why filter_xss_admin() isn't being used for both of them.

For Drupal 7, it might not be possible to standardize on anything, but at a minimum we could do a patch that made sure nothing is ever unescaped (i.e., switched to filter_xss_admin() any places that currently are unescaped) and left everything else alone?

Thanks for stating the questions/problems. That's also part of why why I changed the title of this issue to head toward a solution.

You call it a regression, I'm saying it's "Works as designed" since some of D6 did check_plain on the site_name and some of it didn't. It makes sense to me for the sitename to not contain html which is why it would get check_plained consistently.

If folks feel it should allow some html that's fine with me as well (if so, please update the title). I'd rather make a decision and move forward than let this fester. I don't see any harm in filter_xss_admin on site name as well and am totally willing to be persuaded.

Adjusting spacing

Going to see if we can get some eyes on this over the weekend.

This issue has been stuck for a long time. Should we (if possible) split it into smaller pieces? It seems like there's a bit of a stalemate now.

@Cottser, yes if you know some splits that can be made please do. > 100 comments and last patch in 2012, everything has changed a number of times over. Those functions in the patch don't exist anymore. Xss::adminFilter() and String::checkPlain are the replacements. And autoescape is now on, so all kinds of diff.

Maybe a good split is to use consistency for these values by its location. For example, <title> in <head> shouldn't have markup. But in other locations it is acceptable and sometimes (many times?) it is desired.

I just tested manually on 8.0.x with the string in #120 and the title and slogan are totally escaped in Bartik.

Sounds like maybe we should just move this issue to D7 since Twig autoescape fixed it for D8?

Makes sense to me, thanks for the suggestion xjm.

Thanks @greggles. Cleaning up the backport tags.