Warn about potentially insecure filter configurations

catch - June 27, 2008 - 17:55
Project:Drupal
Version:7.x-dev
Component:filter.module
Category:task
Priority:critical
Assigned:Unassigned
Status:needs work
Issue tags:Release blocker, wysiwyg
Description

If an input format is configured with the php filter or withut the html filter, and is enabled for anonymous/authenticated users, we should have hook_requirements spit out a warning. Cwgordon noted it should only do this if registration is open, which might avoid it being overly annoying for sites where it's not likely to be a security risk

#1

catch - June 27, 2008 - 17:55
Priority:normal» critical

#2

alexanderpas - July 2, 2008 - 18:10

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)

#3

Susurrus - July 3, 2008 - 00:47

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).

#4

Jody Lynn - October 17, 2008 - 19:55
Status:active» needs review

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.

AttachmentSize
filter-security-error.patch 2.22 KB
Testbed results
filter-security-error.patchfailedFailed: Failed to apply patch. Detailed results

#5

alexanderpas - October 18, 2008 - 14:20

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?

#6

earnie - October 19, 2008 - 15:41
Status:needs review» needs work

Yes, need a test.

#7

Jody Lynn - October 19, 2008 - 23:25
Status:needs work» needs review

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.

AttachmentSize
filter-warnings.patch 14.99 KB
Testbed results
filter-warnings.patchfailedFailed: Failed to apply patch. Detailed results

#8

alexanderpas - October 21, 2008 - 03:27

can't you call the database outside the loop?

#9

catch - October 23, 2008 - 10:40
Status:needs review» needs work

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.

#10

Jody Lynn - October 23, 2008 - 15:53
Assigned to:Anonymous» Jody Lynn

I'll work on those two changes.

#11

Jody Lynn - October 23, 2008 - 17:22
Status:needs work» needs review

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.

AttachmentSize
filter-warnings-2.patch 16.11 KB
Testbed results
filter-warnings-2.patchfailedFailed: Failed to install HEAD. Detailed results

#12

Jody Lynn - November 7, 2008 - 01:32

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

AttachmentSize
filter-warnings-2.patch 15.99 KB
Testbed results
filter-warnings-2.patchfailedFailed: Failed to install HEAD. Detailed results

#13

catch - November 7, 2008 - 01:41

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

#14

Anonymous (not verified) - November 13, 2008 - 13:20
Status:needs review» needs work

The last submitted patch failed testing.

#15

Steven - November 14, 2008 - 23:33

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.

#16

alexanderpas - November 15, 2008 - 01:02

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)

#17

sun - November 15, 2008 - 02:33

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

#18

alexanderpas - November 17, 2008 - 20:31

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 &lt;a&gt;) to the html filter which will apprear in the source code, displaying <a> on the screen.

#19

Steven - November 17, 2008 - 20:59

@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.

#20

alexanderpas - November 18, 2008 - 03:11

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.

#21

wrwrwr - January 10, 2009 - 04:24

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? :)

#22

Heine - March 20, 2009 - 11:36

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

==<script>whatever()</script>
==

For Textile after the HTML filter use:

!http://example.com(x"onerror="&#106;&#97;&#118;&#97;&#115;&#99;&#114;&#105;&#112;&#116;&#58;&#97;&#108;&#101;&#114;&#116;&#40;&#39;&#120;&#115;&#115;&#39;&#41;")!

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

![](&#106;&#97;&#118;&#97;&#115;&#99;&#114;&#105;&#112;&#116;&#58;&#97;&#108;&#101;&#114;&#116;&#40;&#39;&#120;&#115;&#115;&#39;&#41;)

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.

Example filter orders:

Default start: sets taint flag
HTML Filter  : unsets taint flag

-> Input format is safe


Default start: sets taint flag
HTML Filter  : unsets taint flag
Textile      : sets taint flag

-> Input format is unsafe


Default start: sets taint flag
Textile      : sets taint flag
HTML Filter  : unsets taint flag

-> Input format is safe


Default start: sets taint flag
Ideal Textile: unsets taint flag

-> Input format is safe


Default start : sets taint flag
HTML Filter   : unsets taint flag
YouTube       : -

-> Input format is safe

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.

#23

Heine - March 25, 2009 - 18:38

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?

#24

grendzy - April 16, 2009 - 06:19

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.

#25

sun - April 21, 2009 - 17:06
Assigned to:Jody Lynn» Anonymous
Issue tags:+wysiwyg
 
 

Drupal is a registered trademark of Dries Buytaert.