Problem/Motivation

Drupal is secure by default. Developers should opt out of security not opt in. The new https://drupal.org/node/2067859 page title default to pass through violates this maxim.

And I must add, I waited long enough after raising this in private for something to happen but it doesn't: in general Drupal 8 totally dropped the ball on this. It's astonishing and insanely worrisome that intentionally (it made it into the change notice) such a change was a) conceived b) reviewed and c) committed. One of the fundamental strengths of Drupal is the security. I have found several fundamental weaknesses this year without running a bit of code just by reading docs or code. What are you guys doing, hm?

Also, the change notice

  public function getTitle() {
    return  'Foo: ' . \Drupal::config()->get('system.site')->get('name');
  }

this is missing translation and is an XSS hole. Oh and a fatal error cos it should be config('system.site') but what's a fatal between friends. What are you guys doing? (no it's not that the config() syntax changed since the change notice was written, I checked.)

I have looked for a _title_callback in core , found the theme settings page and added the recommended return \Drupal::config('system.site')->get('name'); code to ThemeHandler::getName for a quick test. The results are attached. It's not a pretty picture.

Also this just gets better and better every freakin' minute wherever I look, cos this particular callback is defined as theme_handler:getName which uses a service ID aaaaaaand that's not in the change notice.

Proposed resolution

Note the page title is fixed by twig autoescape.

Remaining tasks

Do something with breadcrumbs.

User interface changes

None.

API changes

Files: 
CommentFileSizeAuthor
#39 breadcrumb-xss-2264041-39.patch2.67 KBdawehner
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 76,558 pass(es). View
#35 2264041-35.patch1.8 KBcilefen
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 44,588 pass(es), 12,813 fail(s), and 6,825 exception(s). View
#26 2264041_26.patch1.81 KBchx
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 71,571 pass(es). View
#24 2264041-23.patch2.16 KBpwolanin
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 71,577 pass(es). View
#22 2264041-22.patch1.05 KBpwolanin
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 71,627 pass(es). View
#17 Selection_170.png32.97 KBchx
#11 2264041.patch1.02 KBdawehner
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 71,168 pass(es). View

Comments

dawehner’s picture

Would it be okay to move to filter_xss_admin() instead?

chx’s picture

Why not filter_xss($title, array()) then? Or even strip_tags although someone would need to look whether strip_tags is reliable. I can't readily see what html tags are even valid in title.

chx’s picture

Title: Revert to default check plain on page title » Strip tags from the page title
Issue summary: View changes
chx’s picture

Issue summary: View changes
benjy’s picture

I had a quick look at this and adding the below in hook_preprocess_page() goes straight to the page.

$variables['title'] = '<script>alert("xss");</script>';

Since this is the recommended way now drupal_set_title() has gone, I think a strip_tags by default would be preferred somewhere?

Unfortunately I'm not sure where it would go, drupal_prepare_page() is too early for preprocess functions and DefaultHtmlFragmentRenderer::render() doesn't feel right either.

chx’s picture

Fixing #5 is beyond what I proposed; even D7 fails short there; although I would be madly in love with anyone who fixed that :D

Consider template_preprocess_html doing strip_tags($page->getTitle()) and template_preprocess_html NOT doing any of that: $variables['title'] = $variables['page']['#title']; this would be a good place to add a strip tags or a filter_xss with a very few tags allowed or hell even filter_xss_admin if that's what I can get but doing nothing is unacceptable.

Stopping template processes blowing XSS holes, I think, we are talking of a security hardening and not a fundamentally broken subsystem as we have it now.

benjy’s picture

I could be wrong but my thinking was that the change notice indicates that in D7 it was like:

<?php
// This would be escaped
drupal_set_title($my_title);
?>

So when people are converting their modules to D8, they'd come along and change it to

<?php
// This won't be escaped.
$vars['title'] = $my_title;
?>

And they'd inadvertently introduce a possible security issue.

chx’s picture

Where does the change notice suggest adding anything like that to preprocess? I can not find $var , process , a mention of theme or anything like that. If it does suggest so then it must nuked from orbit and fast before someone thinks that's a good idea.

benjy’s picture

I've seen contrib modules doing stuff like that when overriding the title, for example: http://drupalcode.org/project/h1_title_override.git/blob/HEAD:/h1_title_...

The change notice doesn't specifically mention preprocess but I wouldn't have thought that would stop some people attempting it this way when they're trying to override one off node/page titles.

I'd like to see title to be run through a strip tags, seems like a sensible default to me.

chx’s picture

We could do in template_process_page and amend the change notice telling people not to mess with the title in process_page. That hardening is backportable, even.

dawehner’s picture

Status: Active » Needs review
FileSize
1.02 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 71,168 pass(es). View

So maybe this?

chx’s picture

I do not think this is the right place ; however my solution is not working 'cos process is removed (even if the doxygen for it is not). Great. Someone who knows what's going in D8 needs to find a place after preprocess but before templating.

dawehner’s picture

I do not think this is the right place ;

Well, we can also do that directly in the template.

chx’s picture

The process removal change notice points to this https://drupal.org/node/2052023 change notice about RenderWrapper.

pwolanin’s picture

trying to trace through the flow in D7 (omg) I this it is always check_plain() by default, though there is the option to bypass.

I'm not clear why the patch in #11 would resolve the problem of an unsafe #title?

chx’s picture

Issue summary: View changes
chx’s picture

Title: Strip tags from the page title » Page title code and the freakin' change notice contains an XSS hole
Issue summary: View changes
FileSize
32.97 KB
chx’s picture

Issue summary: View changes
chx’s picture

Status: Needs review » Active

There isn't a lot to review in that patch, TBH.

chx’s picture

Title: Page title code and the freakin' change notice contains an XSS hole » [sechole] Page title code and the freakin' change notice contains an XSS hole
pwolanin’s picture

So, in this issue Tim argued that the return of the title resolver (and hence the title callback) is expected be safe and we added documentation that effect to the title resolver: #2267763: Page titles with %placeholders are broken when used as breadcrumbs

However, re-reading the arguments here I am now thinking that's not viable given the example here. Can we put the xss filter into the title resolver?

pwolanin’s picture

Status: Active » Needs review
FileSize
1.05 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 71,627 pass(es). View

Actually - I take that back. the comment added to that interface is right - it's the title resolver implementation that's missing the step of sanitizing the title that comes from the title callback.

chx’s picture

Good start but won't cover XSS in preprocess. You need to a RenderWrapper call postpone the ripping to the very last minute. (process_page in d7 was doing drupal_get_title and drupal_set_title was check_plain'ing)

pwolanin’s picture

FileSize
2.16 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 71,577 pass(es). View

So, this should catch more, but doesn't really help with preprocess. Maybe we need to save the original title and filter it again if it changed?

chx’s picture

If you need to do this twice then it's living proof that the whole system is fundamentally unsecure. That should not never, ever be necessary (as it is not in D7). I can repeat #23 if you want: template_preprocess_page must do something (and perhaps template_preprocess_html should too) that causes a stripping to run after every preprocess. My understanding of D8 is very limited so I can only repeat what I read in the change notice: the mechanism for this is RenderWrapper.

chx’s picture

FileSize
1.81 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 71,571 pass(es). View

We can do , for example , this. Alternatively we can do all sorts of twig wizardry: new functions, building a new mountain of twig nodes w/e.

pwolanin’s picture

@chx - ok, though we are then twice filtering anything that comes out of the title resolver.

pwolanin’s picture

It feels like a good part of the problem here is that in 7 we had the parameter for drupal_set_title() to we could know when to pass-through the value but it was sanitized by default.

If PHP had support for "tainted" strings this might be an easier problem to solve. Ot possible with TWIG auto-escape if that's enabled.

In the mean time, perhaps the title resolver or other case that wasn't a string passed though could wrap it in a simple object instance (possibly with just __toString()) and we can then treat all non-object titles as tainted and needing to be sanitized?

pwolanin’s picture

I don't see that Drupal 7 actually protects against an unsafe value being set during the page preprocess step. I see this as the process step:


function template_process_page(&$variables) {
  if (!isset($variables['breadcrumb'])) {
    // Build the breadcrumb last, so as to increase the chance of being able to
    // re-use the cache of an already rendered menu containing the active link
    // for the current page.
    // @see menu_tree_page_data()
    $variables['breadcrumb'] = theme('breadcrumb', array('breadcrumb' => drupal_get_breadcrumb()));
  }
  if (!isset($variables['title'])) {
    $variables['title'] = drupal_get_title();
  }
...
}

So we are only getting a safe value if nothing was set by a theme or module before.

however, dawehner points out that most code would use drupal_set_title() rather than setting the variable, so it was easy to do the right thing.

chx’s picture

#1825952: Turn on twig autoescape by default makes short work of what my patch attempts clumsily to do. The breadcrumb is a different story since HTML TRUE.

pwolanin’s picture

chx - yes, though we'd need to figure out how to not double-escape titles such as those already run through t() with placeholders

I think my patch in #24 at least addresses the current breadcumb hole and others from custom titles. Do you want me to pull that into a different issue? add a test?

chx’s picture

Add a test please and repurpose this for the breadcrumb fix as that is not doable by autoescape. As for double escape; the whole issue and patch is about avoiding double escaping.

chx’s picture

Issue summary: View changes
dawehner’s picture

cilefen’s picture

FileSize
1.8 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 44,588 pass(es), 12,813 fail(s), and 6,825 exception(s). View

Reroll.

Status: Needs review » Needs work

The last submitted patch, 35: 2264041-35.patch, failed testing.

dawehner’s picture

@chx
Is still issue actually still needed? Just curious.

chx’s picture

As in #32: is there a test for XSS'd breadcrumbs? #2314599: Use title/url instead of l() for building breadcrumb doesn't seem to contain a test...

dawehner’s picture

Status: Needs work » Needs review
FileSize
2.67 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 76,558 pass(es). View

Here is one. Not sure whether it is a bug that in case you hard-code the HTML inside the routing system, you can put everything in there. This is basically the same problem as t('

'), which we consider as safe in a lot of places.
Fabianx’s picture

Status: Needs review » Reviewed & tested by the community

RTBC, as the route is not user supplied data, this should be fine.

Thanks for the test.

alexpott’s picture

Title: [sechole] Page title code and the freakin' change notice contains an XSS hole » Add a test to ensure title callbacks are not vulnerable to XSS
Status: Reviewed & tested by the community » Fixed

Committed 4300214 and pushed to 8.0.x. Thanks!

  • alexpott committed 4300214 on 8.0.x
    Issue #2264041 by pwolanin, dawehner, cilefen, chx: Fixed Add a test to...

Status: Fixed » Closed (fixed)

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