javascript - Preventing DOM XSS

Question

We recently on-boarded someone else's code which has since been tested, and failed, for DOM XSS attacks. Basically the url fragments are being passed directly into jQuery selectors and enabling JavaScript to be injected, like so:

"http://website.com/#%3Cimg%20src=x%20onerror=alert%28/XSSed/%29%3E)"
$(".selector [thing="+window.location.hash.substr(1)+"]");

The problem is that this is occurring throughout their scripts and would need a lot of regression testing to fix e.g. if we escape the data if statements won't return true any more as the data won't match.

The JavaScript file in question is concatenated at build time from many smaller files so this becomes even more difficult to fix.

Is there a way to prevent these DOM XSS attacks with some global code without having to go through and debug each instance.

---

I proposed that we add a little regular expression at the top of the script to detect common chars used in XSS attacks and to simply kill the script if it returns true.

 var xss = window.location.href.match(/(javascript|src|onerror|%|<|>)/g);

if(xss != null) return;

This appears to work but I'm not 100% happy with the solution. Does anyone have a better solution or any useful insight they can offer?

Answer 1

If you stick to the regular expression solution, which is far from ideal but may be the best choice given your constraints:

Rather than defining a regular expression matching malicious hashes (/(javascript|src|onerror|%|<|>)/g), I would define a regular expression matching sound hashes (e.g. /^[\w_-]*$/).

It will avoid false-positive errors (e.g. src_records), make it clear what is authorized and what isn't, and block more complex injection mechanisms.

Answer 2

Your issue is caused by that jQuery's input string may be treated as HTML, not only as selector.

Use native document.querySelector() instead of jQuery.

If support for IE7- is important for you, you can try Sizzle selector engine which likely, unlike jQuery and similar to native querySelector(), does not interpret input string as something different from a selector.