7.x-3.8 breaks dpm() Krumo and string output

Fix Suggestions

1. The script and style tags will never be added. Adding these tags defeats the whole point of preventing an XSS attack. This is why even using filter_xss_admin(), the most permissive filter, wouldn't work either.
2. No
3. See above.

Simply put: anything (which is everything) that uses drupal_set_message() is stored "as is". There is no filtering of any kind. This means that a module, in theory, could simply pass user provided input without first filtering it. Some may argue that it's the responsibility of the module to filter this, but ultimately these messages are printed/displayed by the theme.

$value = $field['value']; // <script>window.alert('Let's see what damage we can do here.');</script>
drupal_set_message($value);

Messages are meant to be simple text/markup. Nothing more.

In fact, I would argue that dpm() is actually abusing and exploiting this loophole in core. They shouldn't be hi-jacking/displaying this output in messages. They should be using a dedicated block that outputs this information. However, given that devel is so "set in stone" with this method, I doubt this will or should change.

Regardless, it's not really their issue as it's something that, technically, is permissible by core and [now] restricted by this base theme (for good reason).

The reason I'm closing this issue though is because of the following reasons:

1. This is about display debug information. Displaying debug does not supersede security related issues.
2. There are many ways to "debug" something, like using Xdebug in an IDE (which is far preferable to spitting out random markup on a site anyway).
3. If you really have to "debug" something by printing it out, use the following (also supplied by devel):
   

   // Immediately output krumo markup and die.
   kpr($variable);
   die();
   

4. You could also try other possible modules/solutions, although they may also use the same method of "printing to messages" like devel does as, unfortunately, this idea has permeated through-out this community as the appropriate way to "debug" something (which it's not):
   • https://www.drupal.org/project/krumong
   • https://www.drupal.org/project/ref_debug
   • https://www.drupal.org/project/ld

It's also worth mentioning that you can override this theme function your sub-theme to remove the filtering yourself. It must be noted, however, that by doing so, you would intentionally open a vulnerability simply for the sake of "debugging".

One extra option to #3 is rather than entirely removing the filtering in your sub-theme, make it a conditional based on the existence of devel (or some other variable that determines if currently in a development or production environment).

For example, your sub-theme template $output code could be changed from this:

$output .= '  <li>' . _bootstrap_filter_xss($message) . "</li>\n";

to this:

$output .= '  <li>' . (!module_exists('devel') ? _bootstrap_filter_xss($message) : $message) . "</li>\n";

This also breaks the "Menu HTML" module for adding HTML tags on the menu. I added more tags in the $allowed_tags and fix the problem.

May I suggest to set the $allowed_tags into a textarea in the bootstrap theme and sub-theme settings, so admin can customize it without hacking the code. Thank you.

No. There will be no "UI" added to mitigate this change.

If you need to change the, now, "out-of-the-box" output you will have to override it in a sub-theme (as @ron_s suggests above).

It should be noted again that by doing this though, you are purposefully opening up a potential security vulnerability.

This is why it must remain a "more difficult (not impossible) thing to override" and not some "easy UI setting".

Mark,

I think you are right on all counts in how you see this issue, but I'd like to know why you believe Devel is "set in stone" with their method. Have changes to the dpm() output method been debated and shut down in the past?

Also, is there a reason this only came out as a security issue for the Bootstrap theme? I only realized this was related to the theme when I switched to the built-in Garland and found that dpm() works as expected. Is the expectation (or perhaps just ideal) that all themes will make similar security updates?

I'd like to know why you believe Devel is "set in stone" with their method.

Because it has existed this way for a very, very long time. The function is an acronym for "Devel Print Message" a change in their method (as was done for dsm() -> dpm()) would likely not happen again. I'm not saying that it can't be done, but rather it's highly unlikely given that D8 is now out and most development is focused on that.

Is there a reason this only came out as a security issue for the Bootstrap theme?

The security issue, originally, only pertained to menu links. However reviewing the theme there were many vulnerabilities found due to the nature of where the "source" of the variables came from. This was just one of them that was discovered in the process.

As to your last two questions, that is currently being discussed. I will post more information when it becomes available to do so.

Expanding on #4 (I'd make a patch, but it looks like it wouldn't get committed, which I totally understand):

You can override bootstrap_status_messages() in your subtheme's template.php file, and make edits similar to the following:

    if (!empty($status_heading[$type])) {
      if (!module_exists('devel')) {
        $output .= '<h4 class="element-invisible">' . _bootstrap_filter_xss($status_heading[$type]) . "</h4>\n";
      }
      else {
        $output .= '<h4 class="element-invisible">' . $status_heading[$type] . "</h4>\n";
      }
    }

    if (count($messages) > 1) {
      $output .= " <ul>\n";
      foreach ($messages as $message) {
        if (!module_exists('devel')) {
          $output .= '  <li>' . _bootstrap_filter_xss($message) . "</li>\n";
        }
        else {
          $output .= ' <li>' . $message . "</li>\n";
        }
      }
      $output .= " </ul>\n";
    }

@markcarver -- clearly you feel strongly about this, so I'm sure this won't change your mind, but you're wrong here. drupal_set_message requires that you pass it sanitized text. If it were intended to be self-sanitizing, drupal would call filter_xss_admin in drupal_set_message itself. It shouldn't be the theme's job to enforce that. Core doesn't consider this a vulnerability, so I don't think bootstrap should either.

kpr is a clunky substitute since it doesn't preserve messages across redirects. Also very interested in what you think is the "right" way to debug while allowing you to easily drill down in the crazy drupal arrays. Webprofiler is great in d8 but there is no substitute in D7.

clearly you feel strongly about this, so I'm sure this won't change your mind, but you're wrong here

This isn't about me. I do not "feel strongly". As I stated above, this was a revelation that was made in light of the other XSS related security issues. No, I am not "wrong". The possibility of an XSS attack via printing out messages is a very real one.

drupal_set_message requires that you pass it sanitized text.

No. drupal_set_message() does not "require" anything. The docs merely suggest to pass "translated" text (e.g. using t()), nothing more. There is absolutely nothing that prevents a developer from passing whatever it is that they want.

If it were intended to be self-sanitizing, drupal would call filter_xss_admin in drupal_set_message itself.

Yes, it should. But it doesn't. See related issue for actual examples.

It shouldn't be the theme's job to enforce that.

It is until core fixes it. Also, most themes don't override theme_status_messages() like this one does.

Core doesn't consider this a vulnerability, so I don't think bootstrap should either.

Core didn't know about the vulnerability, aside from a few security issues (yes there have been). All of which they have said could be "made public" with the resolution of "hardening it like drupal_set_title() which filters by default with the option to override".

kpr is a clunky substitute since it doesn't preserve messages across redirects. Also very interested in what you think is the "right" way to debug while allowing you to easily drill down in the crazy drupal arrays. Webprofiler is great in d8 but there is no substitute in D7.

Xdebug

Anything that prints Drupal's "render arrays of doom" to the browser is the wrong approach. This isn't "my opinion", it's just a technical fact. These arrays are arbitrary in nature and can contain massive amounts of data and even recursion of the same data. This puts a massive amount of strain on PHP/server eating up resources (like memory).

If, hypothetically (and in D8), themes were only Twig then yes, it would probably be a little easier and safer to print this information to the screen. However, themes in Drupal are not just a solitary templating language and in D7, they're still technically PHP.

Regardless, what it boils down to is this: the data in Drupal is created using PHP and developers should be using a real PHP debugger, not some browser based facsimile.

The issue you referenced is 4 years old, and it would seem not enough of a vulnerability to be handled using the security issue process by core team (I'm guessing? Don't know exactly how this works, but there's the public, unresolved issue, soo...)

drupal_set_message implicitly requires that you sanitize the content you pass to it, just like tons of other ways to output to browser. I agree it should be documented if it isn't changed based on that issue.

The default implementation of theme_status_messages doesn't sanitize output, so the fact that you're overriding it doesn't seem relevant as a distinction.

drupal_set_title was previously plaintext only, and the ability to allow some markup was added as an opt-in enhancement. Changing the default behavior of drupal_set_message would break at least some existing stuff.

The only way you trigger this vulnerability is by writing other bad server side code. An xss attack with messages can't occur unless bad server side php code prints user-supplied input without sanitizing, which is a pretty universal thing. Get input from user? Sanitize. Pretty straightforward.

xdebug makes me sad cause it chokes with > 2 levels of nesting on your typical drupal arrays. I do use it, but it's painful compared to krumo or kint for quick tasks that don't require stepping.

Regardless of your feelings about the meaning of wrongness, what you're stating is opinion not fact. No more arguments from me tho, thanks for reading.

The issue you referenced is 4 years old

I don't know what issue you're referring to, but the one I linked in my last comment was created January 16, 2015 - 19:30: #2408955: drupal_set_message should filter output by default.

and it would seem not enough of a vulnerability to be handled using the security issue process by core team

Interest/activity doesn't equate severity or validity (which is also an answer to the above).

drupal_set_message implicitly requires that you sanitize the content you pass to it

No it doesn't. It assumes. Which is why a lot of security vulnerabilities exist in the first place.

The default implementation of theme_status_messages doesn't sanitize output, so the fact that you're overriding it doesn't seem relevant as a distinction.

Most templates don't. It's just another "assumption" of D7 (which is why D8's auto-escape was such a difficult task BTW). Regardless, the reason it doesn't isn't because it isn't vulnerable, it's because it assumes that it's just sanitized text (with no HTML), which isn't actually always true.

drupal_set_title was previously plaintext only, and the ability to allow some markup was added as an opt-in enhancement. Changing the default behavior of drupal_set_message would break at least some existing stuff.

No, most stuff wouldn't break. It would use/should use filter_xss_admin by default, which kind of the whole point of preventing XSS vulnerabilities. Sure, things like devel would technically "break", but that's an intentional break. They would easily be able to fix it by creating a release that simply adds an explicit PASS_THROUGH constant for the new proposed $output parameter (like in drupal_set_title). And since it's an addition, it would work on all Drupal versions. Thus, no BC break.

The only way you trigger this vulnerability is by writing other bad server side code. An xss attack with messages can't occur unless bad server side php code prints user-supplied input without sanitizing, which is a pretty universal thing. Get input from user? Sanitize. Pretty straightforward.

Nope, not true. I had already updated that related issue with a real POC example on how user generated content could very easily make it's way to drupal_set_message using fields.

This is also yet another assumption that the developer is seasoned enough to know about the "implied assumption" that user generated content should be sanitized. If a developer is new to Drupal, they could pass anything they want to this function and not realize the security implications that go along with it.

xdebug makes me sad cause it chokes with > 2 levels of nesting on your typical drupal arrays. I do use it, but it's painful compared to krumo or kint for quick tasks that don't require stepping.

Then you either don't have it configured correctly or using an IDE that doesn't integrate with it properly. I don't have any issues using with PHPStorm.

Regardless of your feelings about the meaning of wrongness, what you're stating is opinion not fact. No more arguments from me tho, thanks for reading.

It would seem that you are the one having "feelings" about this, not me. What I'm stating is indeed fact. The examples on the related issue proves that an XSS attack can indeed happen. That isn't an "opinion".

Refusing to do a "quoted retort post", but a few notes:

You're right 2 years not 4, not sure which one i was looking at.

Your POC requires more incorrect server side code. There are a ton of more destructive things we could do if i can execute server side code.

No still all opinions. Sorry.

I don't have feelings about it except that it's breaking a convenient tool I used to be able to use without a hassle because of your dogmatic approach, which makes me sad, so i guess that's a feeling, but it's not really strong.

Which is not to say I don't appreciate all the hard work you've put into this theme, which I find generally awesome. So thanks for that.

Your POC requires more incorrect server side code.

This doesn't even make sense. I think you're confusing what we in the community refer to as "coding standards" or "best practice" as a way to "allow this to be acceptable". It's not. It does not change that fact that a person can literally bypass "intent" (intentional or not) in the case of drupal_set_message().

No still all opinions. Sorry.

Does the following execute in a vanilla install of D7:

drupal_set_message('<script>alert("I execute JavaScript!")</script>');

Yes.

That isn't "my" opinion. This is a fact.

Markup that outputs <script> and <style> tags (and various other JS related things, like "on" event HTML attributes) is what is filtered out by filter_xss() and filter_xss_admin() and why those functions exist.

That isn't "my" opinion. This is a fact.

The security team considers any code that outputs to the browser and doesn't filter, at a minimum the <script> tags, a "XSS Vulnerability".

That isn't "my" opinion. This is a fact.

To get the security issue (which are always private discussions between the security team and the maintainers and one that I was apart of for quite a while) "fixed" and "released", I had to go through the entire code base of this project and fix "all the templates" the output (by this project) was sanitized (regardless if core wasn't by default).

That isn't "my" opinion. This is a fact.

Are several of my co-workers actually on the Security Team. Yes.

That isn't "my" opinion. This is a fact.

Have I been through enough security issues (across several projects) to have an inkling of what is actually considered a "security vulnerability" is? Yes.

That isn't "my" opinion. This is a fact.

I don't have feelings about it except that it's breaking a convenient tool I used to be able to use without a hassle because of your dogmatic approach, which makes me sad, so i guess that's a feeling, but it's not really strong.

Which is not to say I don't appreciate all the hard work you've put into this theme, which I find generally awesome. So thanks for that.

I do get that. I've seen you around the issue queue for quite a while now.

However, you have been very vocal on this issue in the past few hours to the point of insinuating insults. Only to, then literally, make it a point to brand me as "dogmatic" about an issue that has very clear steps to reproduce.

It's quite clear that you indeed have very strong feelings on this topic. Enough to ignore some pretty basic facts. I know your feelings are not directed at me personally though and I don't take it that way.

I do need you to understand, though, that this isn't "my opinion". I take security issues very seriously.

Just because it breaks a "popular developer tool" does not justify risking leaving a security vulnerability open.

---

On a slight lighter note, I think it's worth mentioning that when that related issue gets committed, I will gladly add a version check of core to see if it no longer has to be sanitized. Once that happens, and when devel adds the PASS_THROUGH argument to its drupal_set_message() invocation, then devel should work again just fine in both vanilla core and bootstrap.

I went ahead and created the follow-up issue I just mentioned above, but it's currently postponed until the fix in core is made.

Relevant devel issue.

wow very nice technical debate and internal information shared in this thread..
good job @markcarver and @fearlsgroove

#9 worked well for me

Thanks @markcarver for making bootstrap secure :)
Tanks @ron_s to give an easy solution for developing :)

Yes, I understand that this is a hack but it is a helpful hack for my purposes since devel should only be enabled in development and not on a production site. This code allows me to update our site to 3.10 without breaking dsm() for those on our dev team that still use it.

I'm including it here because I think that it could be useful for other folks.

As a work-around, I added the following to _bootstrap_filter_xss in bootstrap/includes/common.inc:

    if (module_exists('devel')) {
      $devel_tags = array(
        'style',
        'script',
        'pre'
      );

      $allowed_tags = array_merge($allowed_tags, $devel_tags);
    }

@SpartyDan, it should be noted that this approach completely nullifies the whole point of _bootstrap_filter_xss() (or any kind of XSS filtering for that matter). XSS occurs from maliciously executed JavaScript found in <script>, <style> and even <svg> tags.

Instead, you should follow the approach of overriding the theme function in your sub-theme as outlined above (#4 and #9) if you really need this functionality added back in, even if only temporarily.

I built a workaround by having Krumo messages rendered in a block instead of through drupal_set_message(). Obviously this is intentionally bypassing XSS filters and shouldn't be left enabled on production sites, but for development this was a quick fix. My Code here: https://copy.mx/view/8a2a8f75

Another workaround: module https://www.drupal.org/project/devel_debug_log "... provides the ddl($message) function, which the developer can use to save a debug message. If an object or array is supplied as $message, it will be displayed using the Krumo debugging tool. Messages can be viewed at Reports > Debug Messages.".

Complete function from #9 with else case for count($messages) > 1
To be copied in subtheme template.php file

function SUBTHEME_status_messages($variables) {
	$display = $variables ['display'];
	$output = '';
	
	$status_heading = array (
			'status' => t ( 'Status message' ),
			'error' => t ( 'Error message' ),
			'warning' => t ( 'Warning message' ),
			'info' => t ( 'Informative message' ) 
	);
	
	// Map Drupal message types to their corresponding Bootstrap classes.
	// @see http://twitter.github.com/bootstrap/components.html#alerts
	$status_class = array (
			'status' => 'success',
			'error' => 'danger',
			'warning' => 'warning',
			// Not supported, but in theory a module could send any type of message.
			// @see drupal_set_message()
			// @see theme_status_messages()
			'info' => 'info' 
	);
	
	// Retrieve messages.
	$message_list = drupal_get_messages ( $display );
	
	// Allow the disabled_messages module to filter the messages, if enabled.
	if (module_exists ( 'disable_messages' ) && variable_get ( 'disable_messages_enable', '1' )) {
		$message_list = disable_messages_apply_filters ( $message_list );
	}
	
	foreach ( $message_list as $type => $messages ) {
		$class = (isset ( $status_class [$type] )) ? ' alert-' . $status_class [$type] : '';
		$output .= "<div class=\"alert alert-block$class messages $type\">\n";
		$output .= "  <a class=\"close\" data-dismiss=\"alert\" href=\"#\">&times;</a>\n";
		
		if (! empty ( $status_heading [$type] )) {
			if (! module_exists ( 'devel' )) {
				$output .= '<h4 class="element-invisible">' . _bootstrap_filter_xss ( $status_heading [$type] ) . "</h4>\n";
			} else {
				$output .= '<h4 class="element-invisible">' . $status_heading [$type] . "</h4>\n";
			}
		}
		
		if (count ( $messages ) > 1) {
			$output .= " <ul>\n";
			foreach ( $messages as $message ) {
				if (! module_exists ( 'devel' )) {
					$output .= '  <li>' . _bootstrap_filter_xss ( $message ) . "</li>\n";
				} else {
					$output .= ' <li>' . $message . "</li>\n";
				}
			}
			$output .= " </ul>\n";
		} else {
			if (! module_exists ( 'devel' )) {
				$output .= _bootstrap_filter_xss ( $messages [0] );
			} else {
				$output .= $messages [0];
			}
		}
		$output .= "</div>\n";
	}
	return $output;
}

I'd like to remind everyone that devel/dpm is only for "developer" benefit, and not even a good one at that.

Closing comments on this issue since there has been enough discussion and other "solutions" available.