SA-CORE-2009-005 and SA-CORE-2009-006 - Drupal core - Cross site scripting

subscribe, thanks.

Just FYI, I probably will not have time to commit this tonight, but should have some time tomorrow.

I committed #2 with a concatenation fix (and clarification that we mean Form API, not Field API).

#1 looks like it still needs discussion.

So I actually like #1 a lot, because it means Drupal will ensure that we never get this bug again.

But it is a regression for themers in that they can no longer have control over the page title without overriding a theme function.

I've tagged this as "needs themer review" and will try and get a few people to chime in here.

I think the module String Replace or Localization (Translation System) should be able to provide a quick and painless way to change page titles on a per-node basis. This is a hack that is always helpful when theming.

This may solve some of yours (Webchicks) and other peoples problems associated with #1. If you want to programmatic control page title, then it looks like you can pre-process the param's or as you stated make

The StringReplace is actually an amazing light weight localization application built for this purpose. It would be AWSOME if it started to accept tokens and or php, but it really is a great program!

I for one would not mind it a bit of the $variables['title'] were controlled by an over-ridable theme function if it helps the security of Drupal itself. The $page_title variable is still accessible from within the template.php file or a custom module via _preprocess_page() right? No foul.

And another thing. This gets yet another piece of Drupal's output into a theme function, and I'm all for that! :)

I'd say to commit this to D7, after you loose the themes/pushbutton/page.tpl.php hunk. [EDIT: and all those other themes that no longer exist.]

I changed my mind after I re-read the entire issue just now. This is only a problem for users of a custom-configured, eight year old browser that has two more recent & stable releases? Are we really gonna change absolutely every theme out there and affect every themer's way of life just so people can't exploit a browser that even MS says is too buggy to support anymore?

Senpai: corporates are usually sticking with older versions for much longer time

Still I think he has a point saying that it's kind of weird to support something that's officially unsupported...
I'm not saying let's not care about all those people who still use old IEs (and probably don't know the evil they are doing), but it's definitely worth considering.

Xerox has not allowed its users to upgrade to IE7 because some of their systems are using old scripting. At the same time, I don't support IE6 at all anymore, and I run a site full of people who dont know how to reload a page (yet surprisingly they all use Firefox)... go figure!

meba: that's true but then we could argue they'll need to disable autodetect encoding or take responsibility themselves for exposing their users to XSS. Anyone actually administering a Drupal site in IE6 is barmy.

That's not to say I'm against the change, I didn't follow the sec issue, but we should at least consider a @TODO to review it closer to release based on IE6 usage.

I just reread the security announcement and it states that even IE7 has this "bug"...

Let's get this committed so it doesn't get forgotten, then discuss if there's a better alternative in a new issue.

After reading the security announcement, II would suspect that this method of attack is larger then what the scope of this patch will touch.

There are numerous vulnerabilities, and I have found that a simple solution is not the most flexible and best solution to protect against such attacks. For instance, I may insert an image that is 1px x 1px and is transparent, that is located on MY site. When a user accesses this image I may record all of the details of that user, and even record what page that user visited.

There are ALLOT of problems associated with flash based applications using the embed functions. These are recorded, but also very hard to prevent. The best way to do so is to control where the flash is coming from. Even then the vulnerabilities in flash are rather robust. But it doesn't stop there, there is XSLT and other types of valid XML (Which may pass most HTML Filters) will add to the list of potential sources of attacks.

I have come to put my faith into the only solution that I know and trust. One that is a GNU application, but that I have a suspicion is not well liked by members of the Drupal Community. So I suggest this with great hesitation for the wide responses that will result due to the nature of the application.

HTMLPurifier will protect against all of these forms of attack. I use it without hesitation and give my uses very wide scope into what they can input as HTML. This solution ultimately free's the users to insert any html they wish, and will result in a well formed HTML/XHTML output that protects against these vulnerabilities.

I have talked to many people about this solution on 5.x and 6.x above that of the standard HTML filter that comes with Drupal, and the most common response is that it provides the flexibility and security at the price of the performance during submitting an application. With HTMLFilter you can filter out where external images are allowed and where they are not. There is also a number of plug ins that enable you to insert FLASH from the trusted sources you (the developer) decide are trusted!

I have found that no other filter provides the safety of HTML Purifier, and the wonderful Well Formed output it provides is truly an amazing bonus. I personally think that embedding a outside security based filter is a benefit in that these issues maybe resolved in an independent community focused on protecting the possible malicious input users enter into a resulting document that is safe to the consumers of this data. Now I am not Positive that HTML Purifier protects against this specific threat, but I am impressed by the output, control, and how functional this filter is.

It comes at a cost of processing power. However I think that if you realize that computational power keeps growing that it will become less of burden as years go by. I also have not seen any side effects on the low scale of current web based solutions. Also do we want to limit our users or empower them with a comprehensive solution.

I provide no clear answers, only suggestions as to how HTML is processed in the future. For me, I will continue to use HTML Purifier for as long as it remains the best solution for my users.

This is one specific vulnerability which has a working patch. If you don't want users to use img tags, you can disable it in your input format.

1. There's no way that patch will apply with chameleon, pushbutton, etc. hunks; testing bot must be shut off. Please don't mark patches RTBC unless you've tested them.
2. Since we're in code thaw, I'd really rather explore what the best solution is, rather than just porting the fix from 6.x which had to be somewhat obtuse because we didn't want to leave non-technical users with themes coded by others stranded and vulnerable.

For example, do we need to remove this variable at all in D7? Can we change all of the core to conform to the "$head first, then title" standard (or a separate charset variable for the meta tag?), document it in the upgrade pages, and trust that coder module will warn people as they're making the other changes required? I'm not sure, but I'd love to discuss it.

Hey guys, I think all of you know, but noone still said it. Did you saw that currently we're outputing Content-Type meta tag two times in a row (after this fix)? Am only I thinking that drupal_final_markup() function in the patch is totally useless? It is needed only if you force-cleaned your $head variable in preprocess. So why we should care about that?

drupal_final_markup() is a crude, but efficient, protection against incorrect order of the content-type header and the header in themes.

It has been designed as a stop-gap solution for D5 and D6. It has the (harmless) flaw of outputting the content-type header twice. In D7, we should rather ensure that the theming layer ensure that out-of-the-box.

I'll fight to death against the inclusion of drupal_final_markup() into D7. I like #1 solution very much.

I think us quite enough to add the strings AddDefaultCharset UTF-8 in .htaccess file in order to resolve this problem.
The IE reads character table only one time.

@pvasili: not true. Drupal already outputs a HTTP Content-Type header. It doesn't prevent IE from trying to second guess the encoding.

@Damien Tournoud:
Encoding, specified on the server will be the best solution
When specifying the value HTTP Content-Type of the server , this attack will fail (absolutely).

My 2 cents regarding drupal_final_markup().. The print module - by design - doesn't call the theme engine after building the node specifically to prevent the theme from ruining the simpler presentation required..

I'm OK with setting the node title in drupal_get_html_head() as suggested in #1, but I strongly disagree with the suggestion in #20 to have it handled by the theming layer.

@pvasili: please read my comment in #22, we do that already. And IE still tries to guess the encoding of the content.

@jcnventura: I don't get your point. #1 suggest to move all headers to drupal_get_html_head(), effectively making "sure that the theming layer ensure out-of-the-box [that the headers are in the correct order]", as I suggested in #20.

@Damien Tournoud: sorry, I didn't consider drupal_get_html_head() part of the theming layer.. I thought you meant moving it to a theme_head() function or something like that... My bad.

SA-CORE-2009-006 also has a related item in book module, which should be factored in here.

Drupal 7 doesn't have drupal_final_markup(), the first chunk can't apply.

Could someone please take a look at this so we can ship alpha 1 with no known security holes? :)

AFAIK this one either can be marked fixed, or will need to change quite a bit. We introduced html.tpl.php and page.tpl.php split.

The UTF-7 vulnerability was addressed by the change in #552478: Restrict "self-closing" tags to only void elements in drupal_pre_render_html_tag. The charset is explicitly set before any other output to head, bypassing IE6/IE7's codepage sniffing logic.

/**
 * Returns elements that are always displayed in the HEAD tag of the HTML page.
 */
function _drupal_default_html_head() {
  // Add default elements. Make sure the Content-Type comes first because the
  // IE browser may be vulnerable to XSS via encoding attacks from any content
  // that comes before this META tag, such as a TITLE tag.
  $elements['system_meta_content_type'] = array(
    '#type' => 'html_tag',
    '#tag' => 'meta',
    '#attributes' => array(
      'http-equiv' => 'Content-Type',
      'content' => 'text/html; charset=utf-8',
    ),
    // Security: This always has to be output first.
    '#weight' => -1000,
  );

I did still manage to trip IE7 up though using the following steps:
1) Load a page containing the title as outlined in the OP. My encoding is set to "Auto-Select" by default.
2) Switch "Auto-Select" off.
3) Switch "Auto-Select" on again.

Step 3 reloads the page and, amongst a flurry of javascripts errors, executes the script, even though the correct charset was specified through both an HTTP header as well as the meta http-equiv... Does seem that it would require an extremely willing and cooperative victim to pull this off.

Seems like this has been fixed elsewhere. I don't believe there is anything we can do to solve #31, which is a different issue, looks like a dumb bug in IE7 but is not really exploitable.