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:
"/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 bees 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 mon 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?
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./#%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 bees 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 mon 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?
Share Improve this question edited Nov 8, 2012 at 14:18 sidonaldson asked Nov 8, 2012 at 10:57 sidonaldsonsidonaldson 25.3k10 gold badges60 silver badges64 bronze badges 4- Yes, you will need to fix this all over your files. What do you mean by "escaping data breaks if-statements"? And why were jQuery selectors stored in URL hashes at all? – Bergi Commented Nov 8, 2012 at 11:32
- Basically their pagination uses GET params and the URL fragment even though each load is a post back. Then they consume this information directly into jQuery without parsing it and there is a lot of logic where if(param[0] == "str") etc etc. To escape this I would have to escape both values. – sidonaldson Commented Nov 8, 2012 at 12:42
- Yes, you should use a proper router and state <-> selector mapper – Bergi Commented Nov 8, 2012 at 13:52
- I agree. If I had the opportunity to rewrite the code I would use a proper router and no user imput could leak through. However the pressing issue here is speed and simplicity as this is not my code and I don't want to spend days going through it (I don't even know the site well enough to do that). It's far from ideal :-/ – sidonaldson Commented Nov 8, 2012 at 14:21
2 Answers
Reset to default 7If 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 plex injection mechanisms.
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.