20
\$\begingroup\$

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

\$\endgroup\$
  • \$\begingroup\$ strip_tags may be a little too much. You get questions on Stack Overflow like "I tried (newline) it didn't work", where it makes no sense because the HTML example is hidden. \$\endgroup\$ – bjb568 Jun 2 '14 at 22:53
  • \$\begingroup\$ I think strip_tags is appropriate. My users won't be posting code snippets, so I don't need to worry about that. \$\endgroup\$ – Meredith Jun 3 '14 at 2:28
  • \$\begingroup\$ You can introduce yourself by saying "Hi, my name is <name>" \$\endgroup\$ – bjb568 Jun 3 '14 at 2:30
  • \$\begingroup\$ Hmm...good point. Do you have any suggestions? \$\endgroup\$ – Meredith Jun 3 '14 at 2:33
  • 4
8
+100
\$\begingroup\$

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.

\$\endgroup\$
  • \$\begingroup\$ I fixed the style attribute issue (the most recent version is at the bottom of my question). I think I'll run image src's against a whitelist (imgur, puush, etc) so tracking images can't be used. Could you expand on PHP injection? \$\endgroup\$ – Meredith Jun 10 '14 at 19:48
  • \$\begingroup\$ I don't think PHP injection is very common, but if you ever pass user input into eval(), system(), passthru(), deserialize(), or similar functions then you're at risk. \$\endgroup\$ – Sophit Jun 10 '14 at 20:56
  • \$\begingroup\$ Alright, I don't do any of those so I should be safe \$\endgroup\$ – Meredith Jun 10 '14 at 21:15
4
\$\begingroup\$

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

\$\endgroup\$
  • \$\begingroup\$ This is more of a comment than an answer and seems to miss the entire point of the question. \$\endgroup\$ – Meredith Jun 6 '14 at 17:31
  • 1
    \$\begingroup\$ @Meredith, how so? I meant to say HTML5 elements, as it has new HTML elements, you should also check to make sure that the XHTML elements are stripped as well, that may be more difficult though as you can pretty much create what you want. \$\endgroup\$ – Malachi Jun 6 '14 at 18:03
  • \$\begingroup\$ The question asks how to enforce that a user's input is a safe subset of HTML. Saying little more than "HTML5 introduced new elements" isn't an on topic answer. \$\endgroup\$ – Meredith Jun 6 '14 at 18:12
  • \$\begingroup\$ you are already whitelisting a safe subset, using an old PHP funciton, I am saying that I would test it to make sure that it works first, then you can go from there. \$\endgroup\$ – Malachi Jun 6 '14 at 18:15
  • 1
    \$\begingroup\$ Also regarding <a href="http://www.not-a-real-site-but-a-virus-injection-point.com>Click here</a>, I don't think that's a huge problem. It'd still be an issue unless I outright disallowed links and really the only way to solve it is to have moderators remove fishy links. \$\endgroup\$ – Meredith Jun 9 '14 at 18:30

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.