Problem/Motivation

There have been a few security issues created regarding this topic, however they have been pushed to be "made public" indicating that the "documenation should be enough":

string $message: (optional) The translated message to be displayed to the user. For consistency with other messages, it should begin with a capital letter and end with a period.

That implies that the message has been routed via t() and if t() is used correctly, there should not be raw html in there.

Documentation aside, what is "intended" does not mitigate that it is still technically possible to inject anything one desires via drupal_set_message().

A very real world example of this abuse is by the popular devel module which exploits this loophole in core to display it's debugging output which does inject HTML (<script> and <style> tags).

Any module or custom code can pass whatever they want in drupal_set_message() and then it's simply printed (as is) in theme_status_messages().

Most themes normally do not override this theme function, so they rely on core's implementation.

Simple example of exploit:

drupal_set_message('<script>alert("oh noes!")</script>');

Unless a developer knows about this "implied" assumption, they could easily do something like the following and thus open up an XSS security vulnerability:

// Some "message" node.
$node = node_load(1);

// A user provided message.
// Note: this could be a simple "text" field which doesn't have "filtering"
// because it was designed to be a "simple" message.
$message_field = field_get_items('node', $node, 'field_message');

// The message type (i.e. warning, error, status).
$type_field = field_get_items('node', $node, 'field_type');

// Extract the message:
// <script>window.alert("Let's see what damage we can do here.");</script>
$message = $message_field[0]['value'];

// Extract the type.
$type = $type_field[0]['value'];

// Display the message.
drupal_set_message($message, $type);

Proposed resolution

Add an additional $output argument to the function signature of drupal_set_message() like is done in drupal_set_title().

By default it should pass through the most permissive filter: filter_xss_admin() with the option to "passthrough".

---

Note: this doesn't affect 8.x because variables printed in Twig templates that aren't an instance of MarkupInterface are auto-escaped.

Remaining tasks

  • Create patch
  • Create tests

User interface changes

None

API changes

Adds a new $output argument to the drupal_set_message signature.

Data model changes

None

Comments

jacobbednarz’s picture

Status: Active » Needs review
FileSize
500 bytes
FAILED: [[SimpleTest]]: [MySQL] 41,131 pass(es), 9 fail(s), and 0 exception(s). View

Attached proposed patched and would appreciate some feedback on it.

Status: Needs review » Needs work

The last submitted patch, 1: sanitise-drupal_set_message-output-2408955-1.patch, failed testing.

jacobbednarz’s picture

No idea why this is failing on the batch tests :s Can someone shed some light on this one?

karthikkumarbodu’s picture

Assigned: Unassigned » karthikkumarbodu
Issue tags: +#SprintWeekend2015
YesCT’s picture

Category: Feature request » Bug report
Issue tags: -#SprintWeekend2015 +SprintWeekend2015, +security

Thank you for opening this issue.

If you think XSS is a possibility, this is probably a bug, and a security related one, not a feature request.

We should all try and use the same sprint tag. According to https://groups.drupal.org/node/447258 it should be SprintWeekend2015 with no #.

Status: Needs work » Needs review

Status: Needs review » Needs work

The last submitted patch, 1: sanitise-drupal_set_message-output-2408955-1.patch, failed testing.

Status: Needs work » Needs review

Status: Needs review » Needs work

The last submitted patch, 1: sanitise-drupal_set_message-output-2408955-1.patch, failed testing.

markcarver’s picture

Title: drupal_set_message should filter output » drupal_set_message should filter output by default
Assigned: karthikkumarbodu » Unassigned
Issue summary: View changes

I had created a security issue about this in response to the SA released regarding the Bootstrap base theme a while back: https://www.drupal.org/node/2824413

Updated the issue summary to reflect what was said in that issue and a more plausible example of why this needs to be plugged.

markcarver’s picture

Issue summary: View changes
markcarver’s picture

Issue summary: View changes
markcarver’s picture

Issue summary: View changes
markcarver’s picture

Issue summary: View changes
greggles’s picture

changing to the more standard tag.

jacobbednarz’s picture

Assigned: Unassigned » jacobbednarz
Issue tags: -SprintWeekend2015
jacobbednarz’s picture

markcarver & greggles, thanks for the updates - I'll look to redo a patch taking this approach instead. Hopefully someone gets some attention on it after that.

jacobbednarz’s picture

Here is the updated patch following the suggestion to make this more like the drupal_set_title API.

markcarver’s picture

Status: Needs review » Needs work
+++ b/includes/bootstrap.inc
@@ -2036,7 +2041,9 @@ function watchdog($type, $message, $variables = array(), $severity = WATCHDOG_NO
+function drupal_set_message($message = NULL, $type = 'status', $repeat = TRUE, $output = FILTER_XSS) {

There isn't actually a FILTER_XSS constant defined (yet).

That being said, I think we should go ahead and define the following:

/**
 * Flag used to indicate that text is not sanitized, so run filter_xss().
 *
 * @see drupal_set_message()
 */
define('FILTER_XSS', 0);

/**
 * Flag used to indicate that text is not sanitized, so run filter_xss_admin().
 *
 * @see drupal_set_message()
 */
define('FILTER_XSS_ADMIN', 1);

Then use a switch statement to check for all of these constants (including PASS_THROUGH), defaulting to FILTER_XSS_ADMIN.

jacobbednarz’s picture

Thanks for the feedback markcarver. I debated using a switch but ended up sticking with a single if because I didn't want to build for conditions we don't have yet (i.e. we only expect two conditions, why would you need a multi value switch?).

I'll go ahead and update those constants as that was my mistake - I made the assumption we already had them.

jacobbednarz’s picture

markcarver, re-reading the updated description and your previous comments, I'm getting a mixed message around what you think should be supported. Are we intending to support filter_admin_xss() (being the default) and passing through the value? Or do we also want to add support for filter_xss() too? I probably contributed to the confusion here by performing a check using FILTER_XSS :P

markcarver’s picture

Are we intending to support filter_admin_xss() (being the default)

Yes.

Or do we also want to add support for filter_xss() too?

Yes.

That's why there should be a swtich because it should support four types of outputs: CHECK_PLAIN, FILTER_XSS_ADMIN (default), FILTER_XSS and PASS_THROUGH.

edit: The reason this needs more output options (unlike drupal_set_title()) is because this has the ability to, ultimately, be rendered as HTML.

jacobbednarz’s picture

I've updated the patch to reflect your thoughts markcarver. Hopefully this is what you are after :)

markcarver’s picture

Status: Needs review » Needs work

Was going to write a few notes, but probably better to just provide the code I was envisioning:

  if ($output !== PASS_THROUGH) {
    switch ($output) {
      case CHECK_PLAIN:
        $message = check_plain($message);
        break;

      case FILTER_XSS:
        $message = filter_xss($message);
        break;

      case FILTER_XSS_ADMIN:
      default:
        $message = filter_xss_admin($message);
        break;
    }
  }
jacobbednarz’s picture

Sure, that's definitely doable!

I see you've updated the comments to include CHECK_PLAIN now as an option. In the future, I think it would be helpful to instead add another comment to show the changes as they happen and notify watchers. Otherwise it's going to cause quite a confusion for those who follow the email chains for review/feedback.

jacobbednarz’s picture

Updated the code with your ideas and an added guard to ensure we do not try to filter if $message is NULL or empty. I didn't see this before but it will ensure that if we get NULL or an empty string it bypasses sanitisation since it won't be needed.

markcarver’s picture

+++ b/includes/bootstrap.inc
@@ -2036,7 +2057,24 @@ function watchdog($type, $message, $variables = array(), $severity = WATCHDOG_NO
+  if ($output !== PASS_THROUGH && !empty($message)) {
...
   if ($message || $message === '0' || $message === 0) {

Instead of adding the !empty condition, how about we just add this entire if/switch statement below in the existing if statement where it already checks if it's empty.

I see you've updated the comments to include CHECK_PLAIN now as an option. In the future, I think it would be helpful to instead add another comment to show the changes as they happen and notify watchers. Otherwise it's going to cause quite a confusion for those who follow the email chains for review/feedback.

Sorry. It's a hard habit to break, especially if I've just posted something and forgot to add my train of thought.

FWIW, it's best to only use the emails as what they're intended for: notifications (at least until d.o emails changes of edits). I used to make that mistake on issues too, especially if there's a back and forth going on in an active issue. I tend to just keep tabs on watching changes in the browser since comments can be edited.

markcarver’s picture

+++ b/includes/bootstrap.inc
@@ -2036,7 +2057,24 @@ function watchdog($type, $message, $variables = array(), $severity = WATCHDOG_NO
+        $message = filter_admin_xss($message);

Also, this should be filter_xss_admin().

jacobbednarz’s picture

Instead of adding the !empty condition, how about we just add this entire if/switch statement below in the existing if statement where it already checks if it's empty.

I'm on the fence about this one because that existing check is already quite complex. I'll change it for now but will look to refactor that section later on.

Also, this should be filter_xss_admin().

Doh!

markcarver’s picture

+++ b/includes/bootstrap.inc
@@ -2036,8 +2059,25 @@ function watchdog($type, $message, $variables = array(), $severity = WATCHDOG_NO
+          $message = FILTER_XSS_ADMIN($message);

The function is not all caps, that's just constants.

jacobbednarz’s picture

markcarver’s picture

Cool! Thanks @jacobbednarz!!!

Now we'll just have to wait for the tests to kick in. Seems like 7.x-dev is broken ATM? Not sure.

Also, I'm adding a related issue that will allow the Drupal Bootstrap base theme to remove its own filtering once this gets in and is released.

markcarver’s picture

jacobbednarz’s picture

7.x-dev does look busted - once that becomes green, I'll look to add some test coverage for this but right now it's extremely hit and miss :/

David_Rothstein’s picture

I would lean against making this change.

Breaking Devel is pretty bad to begin with, but it could break other things too (any custom code out there that uses a tag which filter_xss_admin() doesn't allow).

In the vast majority of cases, drupal_set_message() is used on strings that have been run through t() and sanitized there. It is possible to imagine situations where it isn't (like in the issue summary) but that's a lot less common than examples like drupal_set_title($node->title)...

I would definitely be in favor of improving the documentation to emphasize that this is HTML output and it's always the caller's responsibility to filter, though.

By the way, the tests are passing now. (I triggered the 7.x branch tests to rerun - it must have been a temporary error with the testbots that was causing them to fail.)

markcarver’s picture

The only thing that filter_xss_admin() doesn't allow is <script> and <style> tags. Devel is likely to be the only, heavy, "casualty" regarding this change, and it's only for development purposes. Even if it weren't, it'd likely be a very, very small list of affected code.

I was thinking about this earlier and I think we we could easily mitigate this by also, optionally, checking a certain variable is set and treat that like PASS_THROUGH:

$conf['drupal_set_message_pass_through'] = TRUE;

That would allow anyone who's running into issues to quickly turn it off until they can find the appropriate code and either fix it or add the PASS_THROUGH output flag (like I have already done in Devel, see #2831622: Add PASS_THROUGH argument to drupal_set_message() in dpm()).

This approach is very similar to the what was done for SA-CORE-2013-002 (image derivatives):

$conf['image_allow_insecure_derivatives'] = TRUE;
markcarver’s picture

Issue tags: +Needs change record

It's also worth mentioning that if code needs to be conditional, all one has to do is do a check against core's VERSION, as I have already done in #2831619: Check core version and determine if filter_xss_admin() should be used in bootstrap_status_messages().

Which reminds me, all of this should be added to a change record.

jacobbednarz’s picture

In the vast majority of cases, drupal_set_message() is used on strings that have been run through t() and sanitized there.

This is the opposite of my past encounters and I really wish we had smarter defaults to prevent this from becoming an issue in modules or themes. If you know what you are doing, by all means, let's bypass it entirely but I think these defaults (with documentation) will be the best outcome.

I was thinking about this earlier and I think we we could easily mitigate this by also, optionally, checking a certain variable is set and treat that like PASS_THROUGH:

$conf['drupal_set_message_pass_through'] = TRUE;

This sounds like a great option! If we think this helpful, I'd gladly update the patch to reflect this.

Which reminds me, all of this should be added to a change record.

Thanks for the reminder! I'll add this to the list once I add some test coverage for these changes.

greggles’s picture

This is a disruptive change to a stable release, so I agree with David Rothstein that we should be really careful before making a disruptive change.

I also think our API should do what it can to protect developers from making careless mistakes.

In my personal anecdotal experience I do not remember much XSS that would be blocked by this change. For anyone advocating for this change, can you point to some instances in contrib (ideally in security advisories, but if the module wasn't stable then in a public issue) that would have been caught/protected by this change?

markcarver’s picture

https://www.drupal.org/node/2445955
http://cgit.drupalcode.org/regcode/commit/?id=344cdc21d35b7b1d0fd2059810...

---

#2382229-9: SA-CONTRIB-2014-061, CVE-2014-2715 - Fix:

drupal_set_message('You confirmed file deletion for room ' .$fr->room . '.');

---

I don't have time to google for all possible contrib/drupal_set_message/SA/XSS issues and the above aren't exactly "smoking guns", but it does show that not all developers are aware that they should be using t() to "sanitize" their text before sending it to drupal_set_message(). This problem is a lot more common that people are letting on.

In fact, the user provided content via fields (in the IS example) is something that I personally have done in custom code on a client site; before realizing that it needed to be filtered (even if the field was considered just a "simple" text field). Will this change impact that code? No, it doesn't abuse drupal_set_message() by injecting <script> or <style> tags. I just just passes the value of the field (user provided content) to drupal_set_message().

The fact of the matter is that drupal_set_message() technically allows anything to be passed and then it is printed directly to the browser via theme_status_messages().

This issue is just about hardening a function where the majority of people are, in fact, "following the rules". This simply ensures no XSS by default with filter_xss_admin() (the most permissive filter) so practically no existing code would be adversely affected. I don't see how that can be considered "disruptive".

The only publicly/popular known module that abuses the "implication/assumption" that $message should only be a "translated message" is devel and it's only for "development purposes"; it wouldn't be a "production stopper".

Regardless, even if there were a smattering of custom code that also abuses drupal_set_message() with <script> or <style> tags would have the ability to bypass this filtering by adding the PASS_THROUGH constant to drupal_set_message().

Also, adding a check for $conf['drupal_set_message_pass_through'] = TRUE; in the above patches would also allow a site to instantly disable this hardening if they really deem this change as "disruptive" to their custom code.

David_Rothstein’s picture

filter_xss_admin() blocks more than just <script> and <style>. For example, it blocks <form>, <object> and several other tags too. While putting a form or a video inside a message presumably isn't that common, it's certainly possible.

***

The security examples above are interesting. One is just misuse or non-use of t(), but the regcode example has this:

-    drupal_set_message(variable_get('regcode_voucher_message', 'Voucher code used successfully.'));
+    drupal_set_message(check_plain(variable_get('regcode_voucher_message', 'Voucher code used successfully.')));

Examples like that, plus maybe cases like drupal_set_message($e->getMessage(), 'error') (where the error message comes from an API that could have untrusted user input) are definitely less intuitive.

***

Still, I want to make sure to think about the big picture here. Drupal 7 generally requires developers to sanitize their own output (with only a few exceptions) and there are tons of other places where it's more likely for a developer to make a mistake than drupal_set_message() (the regcode commit certainly has good examples of that). Doing a one-off for drupal_set_message() and making a specific opt-out for that... is that really the right pattern? That just adds one more thing to the list of exceptions, in addition to the risk of breaking code.

Thinking out loud, what would interest me more at this point in Drupal 7's release cycle would be a tool developers could use to spot cases where they forgot to sanitize something. For example a global mode that when enabled on a local dev site, records every string that passes safely through t(), check_plain(), check_markup(), filter_xss(), etc. Then if drupal_set_message() (and also certain key theme functions) are passed a string that is not on that list, it could display a warning message with a modified backtrace showing where the unsanitized string came from. There would be some false positives of course, but it would catch cases like the above. If the goal is to help developers find security mistakes, I think it's worth thinking about as an alternative to this issue, with no risk of breaking anything on existing sites.

Alan D.’s picture

Drupal 7 generally requires developers to sanitize their own output (with only a few exceptions) and there are tons of other places where it's more likely for a developer to make a mistake than drupal_set_message()

I totally agree. It is over 6 years too late imho.

Plus once you blindly trust this data using filter_xss_admin(), img tags linking off to other domains can be inserted which in itself is a security issue, albeit of a lesser magnitude.

If this patch was added before September 16, 2010, tag it as a beta blocker and get it in... I'd be the first to shout you a beer for removing the need to apply the 6 or 7 releases onto each and every site that we look after.

markcarver’s picture

For example, it blocks <form>, <object> and several other tags too. While putting a form or a video inside a message presumably isn't that common, it's certainly possible.

Which is what this whole issue is about. The "documentation" only states the following assumption:

string $message: (optional) The translated message to be displayed to the user. For consistency with other messages, it should begin with a capital letter and end with a period.

As has been said to me many times (in other issues), the intention of this function is that is passed a (relatively) simple line of verbiage, nothing more. However, the reality is that this function is what is "blindly trusting". A developer can pass whatever it wants and it, ultimately, makes its way to the browser as is.

Furthermore, devs shouldn't be injecting <form>, <object> or any other types of markup into messages to begin with. We have other APIs/modules for those kinds of elements. It's drupal_set_message(), not drupal_set_form().

Drupal 7 generally requires developers to sanitize their own output (with only a few exceptions)

Exactly. So, because core doesn't sanitize this, the Bootstrap base theme "had to" (to get past the security issue about XSS vulnerabilities) because it overrides theme_status_messages() and prints out potentially insecure code.

---

I understand the hesitation, but this is a blatant security vulnerability. We don't shy away from the responsibility of maintaining secure code... no matter the "inconvenience".

---

I totally agree. It is over 6 years too late imho.

Security related issues are never "too late". 6.x is (officially) EOL, 7.x is not.

greggles’s picture

Security related issues are never "too late". 6.x is (officially) EOL, 7.x is not.

This is certainly true that we have historically changed APIs if there is a strong security reason to do so. Hardenings like this have the potential of dramatically reducing the frequency of security issues, but there's only one known issue that this would have prevented. If the security pros don't override the concern for a stable API within a major release then we shouldn't make the change.

@David_Rothstein in your memory, do you feel like failing to sanitize is a common mistake?

Alan D.’s picture

FYI, using filter_xss_admin() still has issues trusting the img.src attribute

i.e 3 common issues

* Tracking users, src="http://example.com/track-users.png"
* Using as a ddos vector or spam bot, src="http://example.com/guestbook.php?action=post&message=spam-url.example.com"
* or even just something irritating like src=""https://accounts.google.com/logout" or src="/user/logout".

Bootstrap base theme "had to"

Out of interest, from the Drupal security team or an external audit?

markcarver’s picture

  $output = [];
  $tests = [
    '<img src="http://example.com/track-users.png" />',
    '<img src="http://example.com/guestbook.php?action=post&message=spam-url.example.com" />',
    '<img src="https://accounts.google.com/logout" />',
    '<img src="/user/logout" />',
  ];
  foreach ($tests as $test) {
    $output[] = filter_xss_admin($test);
  }
  print_r($output);

Doesn't mangle those in any way. If, however, you're referring to them being used in <script>, then of course they'd be stripped, the entire tag gets stripped... kind of the whole point of filter_xss_admin().

I think people are forgetting what drupal_set_message() is actually intended for. Status messages are not meant to be anything more than text, with the exception of basic HTML for lists and links. If you're putting in "forms", "objects", "trackers", or anything else in messages... you're "doing it wrong" in the first place.

---

It also, still, does not negate the fact that there is a rather easy "solution" to revert this change (as was, again, mentioned above) by introducing a check for:

$conf['drupal_set_message_pass_through'] = TRUE;

This introduces a very easy way to "turn off this change" if a site is truly abusing this API and it somehow actually royally f's up their site. I suspect, however, that this number would actually be quite negligible.

Quick change to settings.php, boom... done! Not that damn difficult. I mean, seriously......

---

Out of interest, from the Drupal security team or an external audit?

All three. The original security issue was only about menu links. It was kicked back (by the security team) with "the entire code base should be audited for other potential XSS leaks". Someone was assigned (not myself), waited (for over a year) and I finally took it upon myself to do said audit (after I was able to finally get some time). I went through, template by template and traced everything back to its source to determine where (if any) sanitization occurred.

From my experience with security related issues, it has never been a pleasant experience anticipating what constitute a "security risk" and what doesn't. I decided to play it safe and uploaded a patch which included many different XSS plugs, the least of which was for bootstrap_status_message(). I then submitted said patch for a security review. No one objected or even mentioned that core doesn't care about messages being vulnerable to XSS. I took this as a sign that no one actually knew that there was a core vulnerability (because they would have mentioned that this wasn't needed, right?).

But maybe it's just that it doesn't matter what core does (I'm guessing to preserve BC?) and that just contrib must be more secure?

---

This is a security hole. It is technically possible, despite how improbable it may actually be.

I was under the impression that our community took these kinds of security issues, regardless of how "small" they might appear to be, seriously.

Is that a false assessment?

If this were truly a "security" related issue, it wouldn't be so quick to dismiss just the idea of addressing this issue; let alone the POC to prove the vulnerability, patches to fix it and even proposed solutions to mitigate the change.

Let's call the this what it really is: keeping our skeletons deep in the closet.

Quite frankly, I'm sick of core and contrib being held to two different standards. I'm also quite upset that I'm being attacked for doing what the security team (and core committers) should be doing. How did I become the champion of trying to patch a blatant security issue??. This is so backwards.

Alan D.’s picture

I'm not complaining, just interested! Love Bootstrap and easy to overwrite anything like this. You're doing a great job :)

markcarver’s picture

Maybe so. But it sure feels like I'm the only one that is treating this like a serious issue.... because it is.

edit: added a break to separate the last segment of my previous post (because I just realized that it looked like it was all in response to you, sorry).

jacobbednarz’s picture

@markcarver You are definitely not alone in wanting to express how serious this issue is. I raised this issue back in 2015 after I was also shocked to find that this wasn't considered a vulnerability by the Drupal Core team however there were (big name) sites at the time that were not using sanitised drupal_set_message.

I've stood back a little on this because:

  • A) I've moved on from Drupal and this isn't a daily pain point for me any more
  • B) 12 months with no solution isn't convincing me this will get resolved any time soon

I'm 100% in your corner for this one and want to get this shipped so please let me know if there is anything else I can do here to help.

markcarver’s picture

Maybe add the check for the proposed variable above, it never got added to the patch. I'd do it, but I don't have my local for 7.x setup right at this moment.

markcarver’s picture

Status: Needs review » Needs work

So I guess the status should be CNW.

Alan D.’s picture

Status: Needs work » Needs review

An unforeseen side-effect of this is that new contrib releases may start to rely on this feature, and older Drupal versions will suddenly become vulnerable. So a security hardening improvement actually creates a security hole dependent on the version of core that you are running.

serious this issue is

It's not, it is simply a hardening procedure to fix errors done upstream by developers breaking the golden rule of secure web programming (irrespective of the base framework); blindly trusting user data.

Maybe add the check for the proposed variable above, it never got added to the patch.

Yuks, please no. If this gets in, nothing in contrib can not be trusted every again, so having the flag would be like suddenly making the entire system vulnerable. People will use it to fix an issue and just leave it on.

image_allow_insecure_derivatives bypasses the risk to the site running out of disk space in a targeted attack, (i.e. restore via a backup is all that is required), this flag would expose everything to a possible XSS attack, thus full control of the system without a sign that anything is wrong.

Set back to Needs review for the patch as is. It's really a decision for the core maintainers if they want this in or not :)