Allow form tags in Xss::filterAdmin()

Changes should be made against the development version and backported from there.

Given the above suggestions and patch, this is not a documentation bug, but a report that filter_xss_admin() is too aggressive and should allow more tags.

Upss...
Sorry I just read http://drupal.org/node/448554#comment-1533946 so forget about the style attribute :(
Thanks

Seems like a resonable idea to add legend and fieldset to filter_xss_admin. Here's a reroll (above patch wouldn't apply, partly because it wasn't well-formed, and partly because the function is in a different place now, namely common.inc rather than filter.module).

If we are going to change the function to match the documentation, we're also missing other form related elements as well as embedded objects such as iframe, object and embed. I can see the arguments for not including embedded objects as they could open security issues.

But adding the remaining form tags seems safe to me. They would include:

<form>
<input>
<textarea>
<label>
<select>
<optgroup>
<option>
<button>

I'll try and roll a patch later this evening to include those.

Attached is a patch that adds the following tags to filter_xss_admin():

<button>
<fieldset>
<form>
<input>
<label>
<legend>
<optgroup>
<option>
<select>
<textarea>

#7 is good. It would be nice to add HTML5 tags at some point, but that should probably be done in a separate patch.

Would love to have sign off from the security team on this.

With this patch -- what elements are we actually blocking?

@sun, can you clarify your question? It's a whitelist, so it's blocking everything not listed. Probably most importantly, it blocks <script> and <link>, but it also blocks <foo>, <bar>, and so on. It's impossible to list them all.

bump. This is an API change and it apparently needs a security review. If it doesn't make it into D7, it will likely be moved to D8.

#732992: Allow filter_xss_admin() to accept HTML5 tags

I don't agree this is a duplicate. The goal here is to allow some more basic HTML tags, but the other issue is about HTML5. It only looks like a possible duplicate because that issue wound up talking about allowing the list to be variable... but even if that's the direction it goes in, it still doesn't solve this issue in the general case.

For this patch, I'm wondering about the security of allowing <form> tags. Seems like a low-level site administrator with filter_xss_admin() access could use that to build a form that posts to an external website (e.g. to harvest credit card numbers or that kind of thing)?

One problem is that it's not totally clear what filter_xss_admin() is and isn't supposed to protect against; the documentation is not as specific as it could be. But I sort of interpret it to mean that it allows a rogue site administrator to completely deface the content of a website, but isn't supposed to let them do anything that can directly harm the site's users, in which case <form> does seem pretty scary at a first glance.

Given #732992: Allow filter_xss_admin() to accept HTML5 tags and other issues, wouldn't it make sense to turn the whitelist into a blacklist?

I.e., filter out what we know to be harmful and want to get rid off, but leave everything else?

Until we can reliably update the list as quickly as new tag support is being added to browsers, a blacklist seems like a big security risk. Consider if we had a blacklist currently Drupal would be allowing unsafe HTML5 tags through. And the actual whitelist functionality happens at a lower level in filter_xss, so this would be allowing XSS attacks beyond just admin users.

I agree that a blacklist is not a good long term solution.

If we want to be really confident that the tags we're adding are not introducing XSS I think using the tests from http://htmlpurifier.org/ or building new tests using the examples on http://html5sec.org/ would be a great solution.

#7: 605664-updated.patch queued for re-testing.

I agree with David Rothstein here and think this is won't fix:

For this patch, I'm wondering about the security of allowing
tags. Seems like a low-level site administrator with filter_xss_admin() access could use that to build a form that posts to an external website (e.g. to harvest credit card numbers or that kind of thing)?

You could argue that form hijacking isn't an XSS issue, and you'd be right, but still doesn't seem like a good idea.

There are other issues open for other tags, like #2776667: Review/update $adminTags variable for new html elements to be allowed so we should just deal with forms here.

+1 to won't fix - this issue has been open 12 years, there has been little activity on this issue suggesting it hasn't been a problem for most users, there doesn't seem to be a good argument for allowing form elements except for "the docs are wrong", and there is a good security-related reason for not allowing it.

Agree with catch, sun, longwave and David

Thanks everyone for your input