10

I recently posted this question on Code Review and it was recommended that I ask you guys about it.

Basically, this will be used to allow users to generate formatted content. It gets put inside of HTML tags, so I don't have to worry about an attacker breaking out of an attribute. If that's not clear, here's an example:

<div>
Generated content
</div>

Edit: I'm not inserting the content into an attribute, so escaping quotes isn't a concern. I know I still have to check for bad attributes inside the user-generated content, which is what a large portion of the script is for.

I've established that I'm vulnerable to an attacker posting a malicious link or posting a tracking image, but that's something I'm willing to accept. I don't think you can prevent that without white listing URLs, which would also dramatically reduce the freedom users have. If there is a feasible way to fix this vulnerability, I'd love to hear about it.

I've read the OWASP XSS cheat sheet, and I think I have all of those bases covered.

What do I have to worry about outside of that cheat sheet? Did I miss anything on it? Is my code future-proof? Am I in way over my head? Should I just switch to BBCode or Markup?

Current code

Improve this question
8
  • Why do ou say “I don't have to worry about an attacker breaking out of an attribute”? A div wrapper around user-submitted code does not affect the interpretation of attributes in it, and e.g. onclick=... would allow simple injection of JavaScript.
    – Jukka K. Korpela
    Jun 16, 2014 at 6:49
  • @JukkaK.Korpela I guess I wasn't clear about that. I'll clarify it right now.
    – Meredith
    Jun 16, 2014 at 6:52
  • 1
    @FranciscoPresencia </body> gets stripped and the post would appear empty
    – Meredith
    Jun 17, 2014 at 0:38
  • 2
    Why not just have users write in Markdown? It's everywhere nowadays, even this place.
    – Zoey Mertes
    Jun 17, 2014 at 1:02
  • 1
    What holes I can think of: CSS let trolls ruin the layout. ID attribute can conflict with existing ids on the page. Invalid HTML breaking the page and bypassing the filter. Invalid HTML with Unicode magic bypassing the filter. User-generated HTML violating validation and accessibility, which some developers care about them.
    – Ming-Tang
    Jun 17, 2014 at 3:58

3 Answers 3

Reset to default
13

You are in a correct path as you go with whitelisting, but implementing it bullet proof is tricky.

I.e. your links can be fooled to execute JS by just writing:

<a href="JAVASCRIPT:xxx">xss</a>

Also, especially older browsers may execute JS in img src etc.

I'd recommend you to go with HTMLPurifier, which, besides XSS, also helps you to deal with broken HTML (tag nesting etc.).

Improve this answer
2
  • Thanks for the advice, but your example doesn't actually work. See: codepad.org/PBpovpRF
    – Meredith
    Jun 16, 2014 at 18:09
  • Yep stripos (instead of strpos) catches the specific problem there were in the original code. But I think it is a good example how easy it is to miss something which can lead to XSS. That is why a specific library (combined with CSP as pointed out by SilverlightFox) is the way to go.
    – timoh
    Jun 17, 2014 at 7:17
10

Summary: Use an HTML sanitizer, but only in combination with a Content Security Policy as there are constantly new ways to bypass filters.

After your own processing to allow the subset of white-listed HTML tags through, you should pass your content through a HTML sanitizer.

As well as using some sort of HTML sanitiser, it is recommended to implement a Content Security Policy too on pages with user HTML content. This is a browser implemented mechanism that will stop in-line JavaScript from being executed if the user does manage to insert malicious script. You can set the CSP to allow external .js content only (either on your domain or others that you white-list - e.g. Google Hosted Libraries).

These steps will ensure that if a user does enter javascript:alert('foo'); or whatever as a link, it will not be executed. If you are allowing img tags and a tags though, these could possibly point to any web address. They could track, but session information will not be able to be sent with them (e.g. document.cookie) because of the CSP.

There are always holes found in HTML sanitizers eventually as browsers and the language of the web develop, such as this one in old versions of HTML Purifier (<= v4.1.0). This is why I recommend both approaches in combination with each other to ensure gaps in one method won't leave you vulnerable.

Improve this answer
2
  • I actually wasn't aware that you could block internal javascript, so thanks for letting me know about it. So you think I should use my solution and HTML purifier together?
    – Meredith
    Jun 16, 2014 at 18:11
  • @Meredith: Yes, that was my suggestion. If you cannot do everything in HTML Purifier then you could chain your processing: Your subset code -> HTML Purifier -> CSP to make output safe. If any users have browsers don't support CSP then you have already cleaned the output, if there are any vulnerabilities found in HTML Purifier then you have already disabled JavaScript.
    – SilverlightFox
    Jun 16, 2014 at 18:41
-1

It is not recommended you try to implement this yourself. If you do decide to go with user-provided HTML you should take a look at OWASP ESAPI for php here.

Improve this answer

You must log in to answer this question.

Not the answer you're looking for? Browse other questions tagged .