making HTML safe in PHP

[sssphp]

I have a forum program (phpbb); in their last upgrade, they included security measures for HTML included in posts. One of these includes stripping out any value not enclosed in quotes. Example:

<a href="/page.htm">LINK</a>

works fine, but:

<a href=/page.htm>LINK</a>

becomes:

<a>LINK</a>

This not only causes problems in many well-intentioned posts, but also for people quoting or editing posts that were written before the upgrade, without using quotes. A better solution (in my mind), if unquoted values are indeed a security risk(???) would be to ADD QUOTES to any unquoted values. Thus:

<a href=/page.htm>LINK</a>

should become:

<a href="/page.htm">LINK</a>

Unfortunately, I don't have the php expertise to do that. I tried requesting help at phpbb.com, but got no response. Hoping someone can assist here.

This is the new code causing the problem:

      // If HTML is enabled, we'll try to make it safe 
      // This approach is quite agressive and anything that does not look like a valid tag 
      // is going to get converted to HTML entities 
      $message = stripslashes($message); 
      $html_match = '#<[^\w<]*(\w+)((?:"[^"]*"|\'[^\']*\'|[^<>\'"])+)?>#'; 
      $matches = array(); 

  $message_split = preg_split($html_match, $message); 
  preg_match_all($html_match, $message, $matches); 

  $message = ''; 

  foreach ($message_split as $part) 
  { 
     $tag = array(array_shift($matches[0]), array_shift($matches[1]), array_shift($matches[2])); 
     $message .= htmlspecialchars($part) . clean_html($tag); 
  } 

  $message = addslashes($message);

Any way to modify this to acheive the desired result? Or is it NOT something I want to do for security reasons? Maybe someone can offer a better solution.

[MarkR]

The only really good solution I'm aware of is:

• Parse the post as a HTML document using DOMDocument::loadHTML (if necessary you may need to prepend some stuff to set the encoding correctly)
• Scan the resulting DOM, removing elements and attributes not on a "whitelist"
• use saveHTML to re-output the DOM, but skip the html / head / body elements which will have been generated - ideally spit out only the body fragment's children.

Now, what HTML elements and attributes are safe? A lot less than you think.

Consider:

<img src="javascript:evil code here" >
<a onload="evil code here">
<div style="behavior:evil code here"> <!-- may work in IE -->
<div style="-moz-binding:evil code here"> <!-- may work in moz -->

And many other possibilities. There are ways of injecting Javascript code into CSS, which makes it particularly tricky. There are also ways of injecting JS code into URLs for instance href attribtues.

I made some code which does this - but I don't claim it's totally safe. I will post it shortly. It's not pretty.

Mark

[MarkR]

Here is the example I promised.

It's not pretty and will require significant refactoring before being used in production. Moreover, it will need to be fixed so that it works with the encoding that your forums are using otherwise the HTML parser will treat everything at latin1 (see the docs).

Note that I've also got in this example, a particularly evil test case which contains all of the types of exploit that I'm currently aware of.

Mark

[Weedpacket]

MarkR wrote:

- Scan the resulting DOM, removing elements and attributes not on a "whitelist"

Just an observation; but would using strip_tags before loading into DOM be more convenient? You'll still need to vet the tags that remain of course (since strip_tags doesn't bother to look at attributes of whitelisted elements), but that would presumably be easier than using the DOM to do it.

[MarkR]

Weedpacket wrote:

Just an observation; but would using strip_tags before loading into DOM be more convenient?

Maybe, but I don't trust strip_tags. It doesn't have a complete HTML parser therefore may spot things which aren't tags and strip them anyway. There are a lot of peculiar cases:

<a title="<script> example of a valid allowable title</script>" href="somewhere">Valid link</a>

This is speculation and I've no idea whether strip_tags will handle this or not. I know that a lot of naive parsers written with preg_match by people, will not.

Moreover, DOMDocument::loadHTML is a fully-fledged "tagsoup" parser that will try fairly well to take badly formed HTML and make it into a DOM anyway. Then you'd end up with better HTML out the end.

You'll still need to vet the tags that remain of course (since strip_tags doesn't bother to look at attributes of whitelisted elements), but that would presumably be easier than using the DOM to do it.

You're certainly right about it being easier, but if you need the DOM anyway, what's the point?

I am sure that there are some attacks which involve badly-formed HTML which will be ignored by strip_tags, but still interpreted by the browser. Other (evil) possibilities involve adding whitespace or nulls at inopportune places. I actually have a request validation checker which blocks embedded nulls and some other control characters in the request anyway (I cannot think of ANY valid use for them).

Mark

[sssphp]

Wow -- THANK YOU to all for the input. Several questions (keep in mind that my php knowledge is very limited):

1. Mark, is your code tested and ready to go (bug-free), and if so, do you think I can add it to my forum in place of the above code in my first post, or would it need to be adapted to work with the rest of the code there.

2. The test part at the end of the code -- should I remove that before using; if so, exactly which lines do I delete?

3. Do I have your permission to submit your code to phpbb with the suggestion that they consider it as an alternative way of cleaning up the HTML?

Thanks again for your help.

[sssphp]

After looking at your code more closely, I see that it would need to be integrated into my forum. From what I can tell looking at phpbb's code, the user's post would come to your 'cleanser' code as the variable:

$message

It also looks like it picks it up after the 'cleanse' as the same variable, so if you can guide me on where to insert/substitute $message for variables in your code, I can give it a test run.

[MarkR]

sssphp wrote:

1. Mark, is your code tested and ready to go (bug-free), and if so, do you think I can add it to my forum in place of the above code in my first post, or would it need to be adapted to work with the rest of the code there.

No, absolutely not. I make no guarantees that what it does is at all safe and that it won't strip out desirable things or leave undesirable things in.

Also see the caveats above.

It will
- Destroy the formatting of the HTML, whitespace-wise, probably
- Not work correctly with most encodings, you should fix is so that it does by working out how it works and reading the PHP manula to see how to make DOMDocument::loadHTML load other encodings

2. The test part at the end of the code -- should I remove that before using; if so, exactly which lines do I delete?

If you can't understand the test code, DO NOT USE THIS, your skill level is obviously too little.

3. Do I have your permission to submit your code to phpbb with the suggestion that they consider it as an alternative way of cleaning up the HTML?

Be my guest, but as you don't seem to know how it works, it seems highly unlikely that they'd accept it.

Mark

[sssphp]

okay, thanks.