[XSS, Security] Bypass found using DOMPurify tests

[cure53]

Hi all,

it appears that jSanity can be bypassed using multiple techniques. We have not yet sorted out which ones - but the bypassed reproduce cleanly when using the DOMPurify test cases.

Steps to reproduce:

• Visit jSanity demo: http://jsanity.azurewebsites.net/jsanity-demo-pretty.htm
• Copy HTML from DOMPurify test-cases: https://cure53.de/purify
• Paste into jSanity demo-textarea, then click "Sanitize!"
• Watch multiple alerts pop up

Strangely, we don't see any errors on the console, so we are not sure yet what causes the problem. Please let us know if this description is sufficient for you or if we should have a closer look together.

Additional Info:

• The issue appears to only reproduce on Firefox
• The issue might be caused by this line: https://github.com/Microsoft/JSanity/blob/master/jsanity-0.2.js#L870
• The assumed core problem is described here: https://github.com/cure53/DOMPurify/releases/tag/0.6.7

[koto]

Eh, you beat me to it ;)

These are the issues so far:

DOM Clobbering, for now it only breaks the sanitizer code.

<img id=createTreeWalker sanitizer-throws-in=firefox />
<form sanitizer-throws-in=ie,ff><input name=remove /><input name=removeNode /></form>

var in Object will let through prototype properties:

<constructor desc="element name bypass, harmless" constructor="same here">
<b style="constructor: url(//do-stuff.harmless-now)">a</b>

HTML comments should not be let through, otherwise mXSS via e.g. document.createComment('--><script>alert(1)</script>')

[randomdross]

The DOM clobbering example is clobbering the createTreeWalker method which jSanity then accesses here:

        // Do an inorder traversal, then build up a document fragment and when it's finished attach it into the doc
        // Nodefilter currently disabled for perf (yes, it makes a difference!)
        tw = srcDoc.createTreeWalker(srcDoc.body, NodeFilter.SHOW_ALL, /* nodeFilter */ null, false);

So in FF, what's the fully vetted safe way to always get the real DOM method? I know we've been down this road before. =)

[cure53]

@randomdross There is no reliable way. Essentially, two challenges need to be tackled here:

• Getting a fresh document you can trust: https://github.com/cure53/DOMPurify/blob/master/src/purify.js#L317
• Check if it was clobbered: https://github.com/cure53/DOMPurify/blob/master/src/purify.js#L366

Our code seems to be safe against clobbering from what I can see. But it took a long time to get there :)

[koto]

Shouldn't Object.getOwnPropertyDescriptor(Expected.prototype, 'property-name').get.apply(potentiallyClobbered) do the trick?

[cure53]

It should - but is it implemented correctly in all browsers you would want to target? And what to fall back to if not? The possible solutions are plenty - yet the intersection with compatible browsers is small.

Unlike server-side XSS filters, the client-side pendants have to deal with that as good as possible. One of the few yet existing disadvantages of this approach.

[koto]

It should - but is it implemented correctly in all browsers you would want to target?

It appears so, yes. YMMV though.

And what to fall back to if not?

That one is simple for sanitizers at least. Return nothing.

The possible solutions are plenty - yet the intersection with compatible browsers is small.

Luckily we should only support ones that have a TreeWalker + createHTMLDocument / template support, and use IE10+ because, well, we all know. It looks as if the snippet above can be trusted (and I prefer that to https://github.com/cure53/DOMPurify/blob/master/src/purify.js#L366). If we get paranoid enough, we can't even trust instanceof checks.

[randomdross]

You know I think that beyond addressing clobbering in jSanity, this is a great opportunity to effect positive change in browsers re security. Well, trying to get a change out of legacy IE may be hopeless, but I wonder if FF would be willing to take a code change, given that:

1. Their DOM is demonstrably more clobber-able than other browsers in this case.
2. In this case it creates a bypass in a sanitizer.
3. If nothing else, this violates the "principle of least surprise" that suggests if you try to access a method on a freshly created & populated HTML document, you'll get the legit DOM method.

If they don't want to change the default, maybe at least support some kind of "noclobber" mode.

I hope the FF behavior isn't somehow codified in a W3C spec.

I'll file a FF bug shortly. Lemme know if you're aware of any precedent where they've already dealt w/this.

[randomdross]

The FF createTreeWalker DOM clobbering example is now tracked as a FF bug here:
https://bugzilla.mozilla.org/show_bug.cgi?id=1222906

[bzbarsky]

Shouldn't Object.getOwnPropertyDescriptor(Expected.prototype, 'property-name').get.apply(potentiallyClobbered) do the trick?

No, for something like createTreeWalker, since that's a value property, not an accessor property. Something like:

Expected.prototype.createTreeWalker

would work, though.

<img id=createTreeWalker sanitizer-throws-in=firefox />

This would presumably throw in Chrome too: <img id=createTreeWalker name=foo />. See https://html.spec.whatwg.org/multipage/dom.html#dom-document-nameditem-filter

[randomdross]

I will move to the suggested:

tw = srcDoc.prototype.createTreeWalker(srcDoc.body, NodeFilter.SHOW_ALL, /* nodeFilter */ null, false);

...unless anyone sees a bypass. It sounds like this will only work for methods though.

[bzbarsky]

srcDoc.prototype won't work at all, since that returns undefined normally. You want Document.prototype.createTreeWalker. Note that srcDoc.body might well also not be the actual <body> if there is an <img name="body"> in there...

[randomdross]

Ack, yes. Thank you. =)

[randomdross]

This is now covered by the three code changes above & additional tracking in #12 for the potential remaining issues Koto mentioned.