Allowing users to input a safe subset of HTML

Question

I am currently working on a project that requires users to input text that may or may not be formatted (if it helps, just think of it as a Stack Exchange clone, even though that's not quite accurate). Instead of opting for some sort of BB Code or other markup language to allow users to format their text, I figure it'd probably less stressful for the user to just use a subset of HTML. My users are all generally pretty experienced computer users, so I don't expect that to be much of an issue.

My plan is to have a whitelist of tags and attributes. Anything that doesn't match the whitelist will be removed. I figure that as long as I don't accidentally allow a dangerous attribute or element and I implement the code well, there won't be any problems.

I think I generally implemented the code well, but my knowledge of security issues is shaky at best, hence why I'm posting the code here.

public function set($value){
    /*
        First, I want to make sure that we're dealing with a string, so I cast it to a string. Should I throw an error if it's not a string instead?

        Next, I transform \n to <br>, so that whitespace appears correctly. I think it might be better to skip this step and use a CSS rule to allow whitespace.
    */
    $value    =    (string) $value;
    $value    =    nl2br(trim($value));

    /*
        Here is my whitelist. Notice the multiple br elements.
    */
    $value    =    strip_tags($value, "<a><img><table><tr><td><th><h2><h3><h4><blockquote><ul><ol><li><br><br/><br />");

    /*
        This caused some issues in my tests, where it'd unnecessarily escape certain things, so I commented it out. Are shell command injections something I need to worry about?
    */
    //$value    =    escapeshellcmd($value);

    $dom = new DOMDocument;
    if($value === ""){
        return false;
    }
    $dom->loadHTML($value);

    $nodes = $dom->getElementsByTagName('*');
    foreach($nodes as $node){
        if($node->hasAttributes()){
            foreach($node->attributes as $attr){
                $name    =    $attr->name;
                $value    =    $attr->value;
                switch($name){
                    /*
                        These elements are generally safe. Worst case scenario, someone just creates some weird looking HTML, which I'm not too worried about.
                    */
                    case "id":        break;
                    case "style":    break;
                    case "src":        break;
                    case "alt":        break;
                    case "class":    break;
                    /*
                        This is my biggest concern. You can embed inline javascript with <a href="javascript:foo();"></a>, so I want to prevent this. However, links are important so I don't want to completely strip this attribute.
                    */
                    case "href": 
                        if(strpos($value, "javascript") !== false){
                            $node->removeAttribute($name);
                        }
                        break;
                    default:
                        $node->removeAttribute($name);
                        break;
                }
            }
        }
    }

    $value    =    $dom->saveXML($dom->documentElement->firstChild);

    /*
        PHP is weird and adds a bunch of extra elements around the HTML, and it's easiest to just strip the tags again. It's a little hacky, but it works every time (I think).
    */
    $value    =    strip_tags($value, "<a><img><table><tr><td><th><h2><h3><h4><blockquote><ul><ol><li><br><br/><br />");

    /*
        I don't exactly remember why this is here. I think it ensures that all br elements are in the same format, but it probably doesn't do a very good job at it.
    */
    $value    =    str_replace("br/", "br", $value);

    /*
        This ensures that our input is the right length. I chose to do this after we removed all the junk just as a little present for the user.
    */
    if(strlen($value) > $this->length){
        $this->text    =    substr($value, 0, $this->length);
    }else{
        $this->text    =    $value;
    }
    return $this;
}

I'm not too worried about SQL injection so my biggest concern is XSS and I guess any forms of injection that I'm not familiar with.

Here's a link to a more testable version of the code.

Changes that I'll be making:

• Use a case insensitive search when checking URLs for "javascript:"
• Remove the style attribute from the whitelist (now I need a way for users to bold/italicize their text, I'll probably allow <b> and <i>, I guess)
• Check image src's against a whitelist
• Remove the nl2br stuff and use white-space: pre-wrap in my CSS instead

Here's what I currently have

Answer 1

The way I see it, you must protect yourself from the following types of attacks:

1. XSS (Javascript injection to run on the browser)
2. PHP Injection
3. Information Disclosure

Allowing <img src="whatever"/> violates #3, so malicious users can embed a transparent pixel and see everyone who looks at a given page. I believe the style attribute could accomplish the same attack.

You may be vulnerable to PHP injection if you're ever careless about how you embed this user-generated content into your web pages. This can be very serious, because PHP runs on the server. It would expose any SQL databases and perhaps the whole system and network.

It appears that your code is vulnerable to XSS because Javascript can be embedded in the style attribute. You'll have to check what can be embedded in every attribute for HTML4 and HTML5. XSS attacks are serious because the script has access to your website with the current users session. In general XSS attacks can steal cookies, make new posts to propagate the XSS attack to other users, and attempt to change user account settings.

In my opinion, allowing users to embed HTML is dangerous, and you'd better do your homework. You'd be better off using some alternate markup like StackExchange uses.

Answer 2

PHP strip_tags Documentation

you should check out the documentation and what people are saying about the function as well so that you know exactly what is happening with the code you are using.

---

I would also heavily test this against HTML5 elements, or add those elements if you wish to whitelist them as well, from the documentation it seems you are protected from the PHP tags already. you should find the newest list of HTML/XHTML tags and test the usage with all of them.

---

Something that is already mentioned in the Comments is the href's and the src's you said that you would whitelist these, but the problem isn't always whether the information they inject is dangerous to you.

Case in point, someone posts a normal link like <a href="http://www.not-a-real-site-but-a-virus-injection-point.com>Click here</a> and it passes your whitelist so it is visible on your message board or chat room, but when someone clicks the link it runs a script on that webpage in the client browser.

This attack could leave their account (on your site) vulnerable and you would never know it, and neither would they.

There is an insane amount of XSS vulnerabilities out there, and the list grows every day, which means that the only thing you can do is to keep reading about security issues on the internet and Testing your Application accordingly.

---

Links

XSS Wiki Page

OWASP Cross-site Scripting (XSS)

OWASP is a good source for security information from what I have read

Here is a Cheat Sheet from OWASP for XSS vulnerability protection

XSS (Cross Site Scripting) Prevention Cheat Sheet