Warn about potentially insecure filter configurations

i would at least go one step further, to disallow any input format that contains the php filter or without the html filter for anonymous users...

+1 for warning when registration is public. (at least, mabye even disallow)

Big -1 for absolutely disallowing users to do what they want with the filters. If this is on an intranet, there may be no need for such security. Of course, that's probably a small minority, but making that option impossible without hacking core is REALLY not the right way to do this.

I am, however, also against any warning to the user that is persistent. Anything that sits around and is red is incredibly annoying and hurts usability because of that. I would think something like a confirm form that further explains the reasons why this may be bad would be good, but we should be wary of adding warnings that aren't so much educational and just serve to annoy (just look at UAC in Vista).

Patch implements the functionality described by catch:
For sites which allow open registration, an error status message is displayed when anonymous users have access to an input filter which does not filter HTML or the PHP input filter.

Like Susurrus I disagreed with comment #2's suggestion to disallow such configurations completely.

I think that using the error level of status problem is appropriate however. A site which allowed open and registration and did not have proper filtration but which would receive this message in vain would be incredibly rare. An example would be a site which was secured by htaccess or some other means, or a site with its own custom filtration mechanism. I think it's worth bothering such a rare site with a potentially unnecessary error message for the benefit of the vast majority of sites.

patch looks visually solid, however I didn't test it yet.

I agree on error status, and on those edge cases, well... you shouldn't trust on a single line of defense ;)

maybe adding some tests?

Yes, need a test.

I wrote tests and improved the code a lot (writing the tests helped...)

I split the hook_requirements code into filter.install and php.install (as well as putting the tests separately.)

Filter functionality:
An error message is displayed on the status page when:
- The default format does not contain the HTML filter.
OR
- Anonymous users can access a format that does not contain the HTML filter.
OR
- Authenticated users can access a format that does not contain the HTML filter and user registration is open.

PHP Filter functionality:
An error message is displayed on the status page when:
- The default evaluates PHP.
OR
- Anonymous users can access a format that evaluates PHP.
OR
- Authenticated users can access a format that evaluates PHP and user registration is open.

The tests ensure error messages are displayed under these conditions.

can't you call the database outside the loop?

I looks like the database query could indeed be outside the loop.

Also I think we should warn about this on sites that have administrator approval for new users too - just because you restrict who can join your site doesn't mean they should be able to pwn it once their in.

I'll work on those two changes.

OK, new patch

Took the db query out of the loop and extended the error message to user registration set to open w/ approval. Updated tests.

Patch applies fine for me so not sure why the testbed fails. Here's the same patch without the initial ? files.

The testbed has been failing all weeks due to a memory issue on the server. I'll try to test this asap.

The last submitted patch failed testing.

This is stupid. What about input formats based on filters like Textile, Markdown or BBCode which escape all HTML? Forcing their input or output to be run through the HTML filter is undesirable. On the input side, you may have [code][/code] tags that are safe to use. On the output side, you may have advanced features (like a YouTube embed) that output tags whose general form is unsafe, but whose specific form isn't.

The proper solution is to let filters advertise themselves as "sanitizing", and have a standard testbed of XSS filters to verify this as much as possible. Or, to restructure the input format system so that every format must start with a special 'input filter' whose responsibility it is to output safe HTML (or advertise itself as unsafe), after which additional transformations can be made.

hmm... based upon modular support... i think it's the filter order that is important for your problem here...

what if i want to use [] and <>

in my opinion... HTML filter should be the only one that filters html

(please give an example of html filtering that can't be done with the html filter)

I totally agree with Steven. Additionally, HTML is just one language - and one of our goals is to support different output languages. See also http://groups.drupal.org/node/15996 and http://groups.drupal.org/node/15979

What about input formats based on filters like Textile, Markdown or BBCode which escape all HTML?

they would run before the html-filter, giving escaped html (not <a> but <a>) to the html filter which will apprear in the source code, displaying <a> on the screen.

@alexanderpas you totally missed the point. This example shows that it is already unnecessary to require the HTML filter in that case.

However, if you factor in advanced features (like a safe YouTube embed), this means that you are not just asking people to enable the HTML filter unnecessarily, but also to add unsafe tags like <object> and <embed> to the allowed tag list in order to make existing safe features work. And even more, in this situation, the HTML filter has to be placed later in the filter chain to not fuck things up, when in normal situations, the HTML filter should go first.

So you're requiring an unconventional and very specific configuration in situations that are already safe today. This is adding confusion to a part of Drupal that already confuses most people and in fact promotes unsafe behaviour.

Not to mention that this example demonstrates that the proposed patch fails to catch unsafe situations in the first place, like adding object and embed tags to the HTML filter.

you're totally right about that...

now i'm wondering.... as a usecase... how to allow youtube <object> tags, and disallow other <object> tags, allow bbcode including [code], disallow html except <code>

I can only think of a three-step process....
CLAIM - SANITIZE - OUTPUT

CLAIM:
htmlfilter - claims <code> tags
youtube - claims <object> tags that are for youtube
bbcode - claims [code] tags

SANITIZE:
youtube - ensures youtube objects follow specs.
htmlfilter - removes unwanted HTML (doesn't see youtube HTML and [code], has already identified <code> blocks.)

OUTPUT:
bbcode - modifies BBcode into HTML

in this model, htmlfilter won't see the youtube code, or the bbcode [code], it only sees a placeholder or something....

also, i think this fixes issues with filter-order.

The following should be a good addition to this patch. Right now one can add script, iframe or object to the list of allowed tags and sleep happily -- and the filter's help string: "Allows you to restrict the HTML tags the user can use. It will also remove harmful content such as JavaScript events, JavaScript URLs and CSS styles from those tags that are not removed." doesn't mention that some tags should never be added. At least there should be a warning or a confirmation.

The HTML filter could actually refuse to run if given some tags -- I believe it doesn't make sense to filter if someone allows script, style, iframe, frame, meta, link, object, embed, applet, layer etc.

Maybe there could be also a confirmation/warning for anything with src or similar attribute (like img, bgsound, input), which is usually not exploitable by itself, but makes CSRF easier (and CSRF vulnerabilities are not so rare even in core: DRUPAL-SA-2006-025, DRUPAL-SA-2006-025, DRUPAL-SA-2007-017, DRUPAL-SA-2008-005, DRUPAL-SA-2008-044, DRUPAL-SA-2008-047). It's too late here, does that make sense? :)

Well, too bad that Textile and Markdown do not generate safe HTML.

The Markdown filter doesn't even escape HTML. Used on it's own, <script>whatever()</script> would do fine. Textile would need

For Textile after the HTML filter use:

For Markdown after the HTML filter (src="javascript:" works fine in Opera 9.64) use:

Barry Jaspan proposed a taint flag (or a set of taint flags) system.

[...] We declare two new properties of input filters: "Allows XSS attacks" and "Prevents XSS attacks." Every input format is born with the "Allows" taint bit set (since by default, no filtering is performed). Sequentially, each input filter with "Allows" set turns on the "Allows" taint bit for the format, and each input filter with "Prevents" set turns off the "Allows" taint bit for the format. Any format that ends with the "Allows" taint bit set gets a warning, or is disabled unless the loudly-worded checkbox from above it checked, or whatever.

Obviously, we'd need more states to declare the PHP filter and other formats whose unsafeness doesn't hinge on HTML as Unsafe. It also relies on filter authors to properly mark their filters, but they are better suited to do that than end users.

I'm marking this as a release blocker. There's not one week where a blog on Planet Drupal (!) doesn't have its filters misconfigured. If those people cannot figure it out, who can?

alexanderpas:

i would at least go one step further, to disallow any input format that contains the php filter or without the html filter for anonymous users...

There is currently at least one reason for making "Full HTML" the default; and that is blogapi module, which always uses the default format. Although perhaps that should be addressed elsewhere.

(please give an example of html filtering that can't be done with the html filter)

http://drupal.org/project/wysiwyg_filter does what the HTML filter cannot; it filters HTML attributes and style properties.