- Notifications
You must be signed in to change notification settings - Fork535
Security: Mitigate DOM XSS vulnerability#21
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Uh oh!
There was an error while loading.Please reload this page.
Conversation
Your version of the script is prone to DOM XSS attacks:`parseHTML('<img src=x onerror=alert(1)>')` will execute the script immediately. This altered version prevents the execution of the script.`$.parseHTML('<img src=x onerror=alert(1)>')` is still vulnerable to this attack, so generally it's not recommended to parse arbitrary HTML on the client side.This is true, but, as you said, our solution doesn't differ from jQuery's. There is a safe way to parse HTML, but IIRC, it's not supported by Chrome. |
Look at the PR attached, there is the safer version. And it works perfectly on Chrome. Please don't let people use the vulnerable version, it's already on StackOverflow all over the place. It is really harmful. I'll also open a issue on jQuery itself, maybe this issue can be fixed there as well. |
Sorry, I completely missed that it was a PR. This won't work in ie8, so please create an ie8.js with the original solution, and move yours to ie9.js. |
It might be possible to get this running on IE8 as well, I'll look into it and update the PR later. |
Ok, I added the improved IE8 compatible version as well. |
Wow, that solution is unexpected, I'm tempted to just leave the insecure version for ie8, as I don't think I can in good conscious tell people they should be creating an ActivX control to avoid using jQuery. In any case, ie8 solution needs to support ie8+, so add a fallback to the ie9+ method, and we're good to go. |
… but you can in good conscious tell them to use code which is easily vulnerable to XSS? I'd definitely prefer the ActiveX version then. This is AFAIK the only safe way in IE<=8 to do this. |
If trusted input is being parsed, there's nothing wrong with it. Without knowing how it's being used, I can't say that you must, or even should, use an ActiveX control to replicate jQuery's behavior. |
Ok, I'll update the PR offering both solutions for IE8 with a warning that one is vulnerable to XSS and should only be used with caution. Are you fine with this? |
Ok, now I listed both versions. |
I merged it without the ActiveX version. If the jQuery version was secure in this way, it would be a different story, but the ultimate point of this website is to showcase how to replicate jQuery's functionality, not the many improvements which could be made to it. Thank you for your contribution, I hope improving security for sites targeting IE9 and above is good enough. |
Then please re-add at least the XSS warning to both jQuery and IE8. I'll file this issue for jQuery as well. |
Your version of the script is prone to DOM XSS attacks:
parseHTML('<img src=x onerror=alert(1)>')will execute the script immediately. This altered version prevents the execution of the script.$.parseHTML('<img src=x onerror=alert(1)>')is still vulnerable to this attack, so generally it's not recommended to parse arbitrary HTML on the client side.