Timing issues when using shield

[ericcornelissen]

When using this project to protect against DOM Clobbering, there is a timing issue when dynamically inserting new HTML that allows attackers to temporarily clobber and thus potentially bypass the protections it offers. This stems from the fact that code immediately following the dynamic insertion executes before the MutationObserver callback has a change to run.

The basic affected code looks like the below and here's a live demo: https://shield-timing.glitch.me/ (using @weizman/shield@0.0.1).

// Dynamically insert some clobbering element, contrived example:
const untrustedElement = document.createElement("img");
untrustedElement.setAttribute("id", "foobar");
document.body.appendChild(untrustedElement);

// immediately read from clobberable objects, e.g.
console.log(window.foobar);

See also https://github.com/SoheilKhodayari/DOMClobbering/issues/10

[ericcornelissen]

Additionally, because of the past names map on <form>s (unlike document or window), if and only if a dynamically clobbered property is accessed on a HTMLFormElement during this time window it will remain clobbered going forward. See https://shield-timing-past-names-map.glitch.me/ for a demo.

EDIT: I noticed this project doesn't actually prevent clobbering of <form>s, so I updated the demo to use a custom simplified implementation of this project's approach that supports forms.

[weizman]

Hi @ericcornelissen, glad to see this interests you!

The "timing issue" is well known and was left out of the threat model's scope intentionally.

That's because I'm not convinced there's a likely attack scenario where the malicious introduction of the clobbering and the mistakenly access to the clobbered property both take place in the same sync operation - from what I gathered, there will always be an async gap between the two, in which shield will always have the chance to address the clobbering attack (I am no DOMC expert and am willing to take that statement back given some concrete contradicting proof).

As for forms, can you share a JS snippet that once executed under https://weizmangal.com/shield/ proves it to be vulnerable?

[ericcornelissen]

The "timing issue" is well known and was left out of the threat model's scope intentionally.

Well known in what sense? I would personally not expect the average developer looking for a defense against DOM clobbering to realize this (I certainly didn't). I think it's completely fair to consider it out of scope, but since it doesn't seem obvious to me, I think it would be beneficial to make this explicit in some kind of "limitations" section.

That's because I'm not convinced there's a likely attack scenario where the malicious introduction of the clobbering and the mistakenly access to the clobbered property both take place in the same sync operation - from what I gathered, there will always be an async gap between the two, in which shield will always have the chance to address the clobbering attack (I am no DOMC expert and am willing to take that statement back given some concrete contradicting proof).

If there is an async gap, you're right. However, I don't see why that would always be the case. Is there anything "wrong" with the code sample I provided (besides it arguably being contrived)? Why wouldn't a developer access any property on document or a <form> (all of which are clobberable) or a missing property on window after inserting new HTML?

I don't have any real-world bug ready for you, and if that's the only way I could convince you I won't try beyond this.

As for forms, can you share a JS snippet that once executed under https://weizmangal.com/shield/ proves it to be vulnerable?

I'm not entirely sure how you want me to share this since the page doesn't test forms, but if you paste the following in to the browser console you'll see that it doesn't print "FORM" (or errors) but rather the input node.

const $form = document.createElement("form");
const $input = document.createElement("input");
$input.name = "tagName";
$form.appendChild($input);
console.log($form.tagName);