Join GitHub today
GitHub is home to over 40 million developers working together to host and review code, manage projects, and build software together.
Sign upDisable DOM clobbering. #349
Comments
This comment has been minimized.
This comment has been minimized.
|
See whatwg/html#2212 for prior discussion and the various things that need to be considered. |
This comment has been minimized.
This comment has been minimized.
|
DOM clobbering is also often a cause for concern in HTML sanitizers, a common source of bypasses is when a sanitizer gets confused when traversing the dirty DOM tree. Alternatively, the nodes that are in the end sanitized, may - instead of executing JS, confuse the application via DOM clobbering. What these examples show is that perhaps - if blocking clobbering altogether turns out infeasible due to backwards compatibility - we could mark individual nodes themselves as not clobbering. Then a sanitizer could simply disable the clobbering for the whole dirty tree, and traverse it in a normal way, and - whatever the output is, the resulting nodes would not be able to clobber the rest of the application as well. This would be backwards compatible, and we could make the native sanitizer work like that from the start. // @mozfreddyb |
This comment has been minimized.
This comment has been minimized.
|
I'm not quite sure I follow how the attack works. If the global variable involved is defined via
Just to check, in this context does the term "clobbering" refer to named getters in general, [OverrideBuiltins] named getters, or any properties on objects that might shadow the prototype ("expandos")? Those are quite different problems that might require quite different solutions depending on what the actual issues are. |
This comment has been minimized.
This comment has been minimized.
I'm guessing it's https://github.com/ampproject/amphtml/blob/0c5e03f962ebee782476a0da9f68600ba92c3cc1/src/mode.js#L50 It usually is that you're attempting to read some global variables off window, or properties/functions of DOM elements.
In the context of sanitizers, I guess the third option. The attacks have to be tailored to an exact sanitizer logic, but it roughly looks like this: https://fastmail.blog/2015/12/20/sanitising-html-the-dom-clobbering-issue/ |
This comment has been minimized.
This comment has been minimized.
OK, but is the point that "AMP_MODE" is sometimes not actually defined on Window?
Those are very different situations, which may have different compat constraints, for what it's worth.
If that is the case (that is, you are trying to sanitize a DOM fragment that untrusted script was allowed to touch), then that is quite far afield from the original issue filed in this bug (which is about the window's named properties object). The only possible solutions there are either separate worlds (in the Chrome sense) or membranes that only expose the default behavior (in the Firefox sense). |
This comment has been minimized.
This comment has been minimized.
I don't know how AMP_MODE was set for AMP. I'm guessing
To clarify for sanitizer case:
I agree it's a different case than overriding properties on window. Both however are called DOM clobbering, as the source of clobbering comes from DOM and its behavior of treating id/name attributes specially. |
This comment has been minimized.
This comment has been minimized.
|
I am not sure about that specific example of AMP_MODE, but from my experience you can often see variables like: It usually happens when a global variable is shared between multiple scripts. |
This comment has been minimized.
This comment has been minimized.
OK, then we're definitely not dealing with the third option, thank you. So for sanitizers it sounds like It's worth investigating how much people rely on that; the use counter mentioned in #349 (comment) does NOT count that. |
This comment has been minimized.
This comment has been minimized.
|
related: #251 |
https://research.securitum.com/xss-in-amp4email-dom-clobbering/ is a good example of the kinds of attacks enabled by the somewhat unexpected mapping of elements into the global namespace via the
namedItem()getter onWindow:We can't turn this off by default, as ~8% of pages depend on it in one way or another in Chrome's dataset, but it would be lovely if we could disable this footgun via (something like?) FP.
@clelland