Skip to content
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

Disable DOM clobbering. #349

Open
mikewest opened this issue Nov 18, 2019 · 14 comments
Open

Disable DOM clobbering. #349

mikewest opened this issue Nov 18, 2019 · 14 comments

Comments

@mikewest
Copy link
Member

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 on Window:

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

@annevk
Copy link
Member

annevk commented Nov 18, 2019

See whatwg/html#2212 for prior discussion and the various things that need to be considered.

@koto
Copy link
Member

koto commented Nov 18, 2019

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

@bzbarsky
Copy link

I'm not quite sure I follow how the attack works. If the global variable involved is defined via var or let then that declaration will take precedence over the named properties object bits. So in the specific example in https://research.securitum.com/xss-in-amp4email-dom-clobbering/ what exactly is the setup with the AMP_MODE global that allows ids to override it? Unfortunately that post never shows or links to the full code involved...

DOM clobbering is also often a cause for concern in HTML sanitizers

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.

@koto
Copy link
Member

koto commented Nov 18, 2019

I'm not quite sure I follow how the attack works. If the global variable involved is defined via var or let then that declaration will take precedence over the named properties object bits. So in the specific example in https://research.securitum.com/xss-in-amp4email-dom-clobbering/ what exactly is the setup with the AMP_MODE global that allows ids to override it? Unfortunately that post never shows or links to the full code involved...

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.

DOM clobbering is also often a cause for concern in HTML sanitizers

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.

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/

@bzbarsky
Copy link

I'm guessing it's https://github.com/ampproject/amphtml/blob/0c5e03f962ebee782476a0da9f68600ba92c3cc1/src/mode.js#L50

OK, but is the point that "AMP_MODE" is sometimes not actually defined on Window?

It usually is that you're attempting to read some global variables off window, or properties/functions of DOM elements.

Those are very different situations, which may have different compat constraints, for what it's worth.

In the context of sanitizers, I guess the third option.

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).

@koto
Copy link
Member

koto commented Nov 18, 2019

I'm guessing it's https://github.com/ampproject/amphtml/blob/0c5e03f962ebee782476a0da9f68600ba92c3cc1/src/mode.js#L50

OK, but is the point that "AMP_MODE" is sometimes not actually defined on Window?

I don't know how AMP_MODE was set for AMP. I'm guessing win.AMP_MODE = xyz or var AMP_MODE just wasn't never called in that case.

It usually is that you're attempting to read some global variables off window, or properties/functions of DOM elements.

Those are very different situations, which may have different compat constraints, for what it's worth.

In the context of sanitizers, I guess the third option.

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).

To clarify for sanitizer case:

  1. Sanitizers gets an untrusted string with HTML snippet. It happens to contain nodes named to clobber DOM API functions (like 'Node.removeChild`)
  2. Sanitizers create a dirty DOM tree from that string
  3. Sanitizer attempts to call the function on a dirty element, expecting to get a function from the prototype chain. It's clobbered, node removal doesn't succeed.
  4. ...
  5. Profit

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.

@terjanq
Copy link

terjanq commented Nov 18, 2019

@bzbarsky

I am not sure about that specific example of AMP_MODE, but from my experience you can often see variables like:
window.CONFIG = window.CONFIG || {}

It usually happens when a global variable is shared between multiple scripts.

@bzbarsky
Copy link

Sanitizers gets an untrusted string with HTML snippet

OK, then we're definitely not dealing with the third option, thank you. So for sanitizers it sounds like [OverrideBuiltins] is the only issue, and that's only an issue for HTMLFormElement.

It's worth investigating how much people rely on that; the use counter mentioned in #349 (comment) does NOT count that.

@petamoriken
Copy link

@tomayac
Copy link

tomayac commented Apr 29, 2021

Likewise related: WICG/document-policy#32.

@securityMB
Copy link

Status update: recently a CL landed in Chromium that adds new UseCounters that will help us better understand DOM Clobbering patterns in the wild:

  • kDOMClobberedShadowedDocumentPropertyAccessed - which tracks named properties on document and is triggered only when the property shadows a pre-existing property (for example: <img name=getElementById>).
  • kDOMClobberedNotShadowedDocumentPropertyAccessed - which tracks named properties on document and is triggered only when the property does not shadow a pre-existing property.
  • kDOMClobberedShadowedFormPropertyAccessed - tracks shadowed properties on <form>
  • kDOMClobberedNotShadowedFormPropertyAccessed - tracks non-shadowed properties on <form>.

We will wait a couple of months to see the results and based on that we'll try to figure it out what exactly we can against various types of DOM Clobbering.

@mozfreddyb
Copy link

To clarify, this rules out any measurement for properties of window. Is this intentional?

@securityMB
Copy link

securityMB commented Feb 29, 2024

The UseCounter for window is already there. Also you can't shadow properties on window so there's no need to distinguish between these two cases as in these other examples.

That said, if you have an idea for another useful counter for window, let me know.

@annevk
Copy link
Member

annevk commented Feb 29, 2024

Disabling window[int] might be good too to prevent certain exfiltration attacks. That's also covered by COOP to some extent, but maybe the defense-in-depth is worth it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

10 participants