-
Notifications
You must be signed in to change notification settings - Fork 37
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 our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
XSS bypass by DOM Clobbering #7
Comments
Hey Soroush! Have you seen that this affects non-FF browsers? I've confirmed it on the latest FF but haven't seen it's working elsewhere (yet). |
I had used Firefox and quickly used mine and Pepe's payload to be honest. Doesn't seem to work on chrome either for me but as this solution is vulnerable fundamentally against DOM clobbering it is just matter of time for someone to find a proper bypass I guess. |
It is FF-only - like most clobbering problems. |
Well, my opinion is that the approach is flawed. You create a document fragment, innerHTML dirty node there, and attempt to sanitize inline (with the TreeWalker), hoping to remove the bad nodes. Any clobbering here will potentially leave the tree in a dirty state, ready to be XSSed, when later on Instead, you should create a separate guaranteed-clean tree and one-by-one clone & append only sanitized nodes there. |
Yet you have to have a clean document to begin with. Reminding of |
Yes. But that's straightforward, and with all approaches one needs at some point to get the unclobbered dirty tree root. |
There's a little complexity here re the directModifySource and isolatedTargetDOM flags, but unless I'm smoking crack, jSanity is not sanitizing inline as you suggest. There is a source doc created with document.implementation.createHTMLDocument and then elements / attributes / etc. are cherrypicked via a whitelist strategy and brought into the destination. By default the destination DOM is the current page. If you enable isolatedTargetDOM you effectively enable "paranoid mode," which involves maintaining a fully independent target DOM. Iirc there were perf implications to this and I never saw a bypass this would have mitigated, so the three-DOM approach is not currently enabled by default. We could revisit though if there's a need. |
Yup, you're right; I got confused by the logic (JS is hard ;) ). The correct approach is directModifySource = false and this line is crucial. Only ever insert elements that have been created from scratch. |
Ahh, but... isolatedTargetDOM is false by default and thus directModifySource is set to true. And now I do recall having a reservation about moving isolatedTargetDOM to false by default... We do cherrypick, but it doesn't in theory entirely eliminate the possibility that something will slip through. Re-creating elements completely from scratch would be more conservative. Anyway, if this worry can be substantiated with a PoC that works with isolatedTargetDOM set to false but not true, I'd be happy to enable isolatedTargetDOM by default. |
DOM Clobbering is addressed in Issue #5. But if there are any remaining issues here please let me know or just file a bug. |
Here is the vector:
some more vectors can be seen here: http://www.thespanner.co.uk/2012/06/05/jslr/
The above payload was by @cgvwzq – Pepe Vila to bypass JSLR (Gareth Heyes' old toy)
The text was updated successfully, but these errors were encountered: