REVIEW: Fix for bypass in Firefox thanks to a newly discovered mXSS

[cure53]

Hi @neilj and @fhemberger!

Today I ran into a mXSS issue on latest Gecko that causes a bypass under certain conditions. The problem is, that Firefox shows different behavior for innerHTML interaction than any other browser when doing that in an SVG context. I talked to @freddyb of Mozilla and we developed a fix.

The mXSS bug in Firefox causes a bypass when the sanitized HTML is later not being applied with innerHTML but with document.write() or alike. From a security standpoint, I find this to be close to critical. The cause for this issue is a parser behavior change in Gecko when dealing with HTML elements inside inline SVG documents. I will publish the attack vector after the fix has been reviewed (contact me offline for a PoC).

This fix might however be breaking so I'd love to hear your opinion. The tests are green and things look okay - but better to have that one triple-checked:

Changeset
https://github.com/cure53/DOMPurify/compare/DOMParser?expand=1

Test Results
https://travis-ci.org/cure53/DOMPurify/builds/80807894

Opinions are welcome!

Cheers,
.mario

[fhemberger]

If it's a breaking change, that means this should be a 1.0.0 release (semver-major). And the reason for it should be documented. What browsers could break by switching from document.implementation.createHTMLDocument to window.DOMParser?

[cure53]

It might be a breaking change but I hope it's not, hence my review request :) So far it looks like it's not - but I cannot judge that on my own.

[fhemberger]

If all the browser tests are green, it seems to work … I'm sorry, but at some point you lost me with all the DOM voodoo going on. I must admit, I'm not that much into that particular topic to be of much help here. (Especially after exclusively working on the server side the last 14+ months.) ;)

[cure53]

Aye, okay :) Are the code conventions all met? If so, I'd merge and prepare a release.

[mozfreddyb]

s/@freddyb/@mozfreddyb ;-)

[fhemberger]

LGTM

[cure53]

Thx. I am preparing the 0.6.7 release now.

[koto]

So, what was the bypass?

[cure53]

It was documented along with the release:

https://github.com/cure53/DOMPurify/releases/tag/0.6.7

<script>
// This is SAFE (but shouldn't be!)
document.body.innerHTML='<svg><p><style><img src="</style><img src=x onerror=alert(1)//">'
</script>


<script>
// This is UNSAFE
document.write('<svg><p><style><img src="</style><img src=x onerror=alert(1)//">')
</script>

[mozfreddyb]

I think it was mentioned in the release notes, here's the issue discussed in Bugzilla:
https://bugzilla.mozilla.org/show_bug.cgi?id=1205631

[cure53]

@mozfreddyb "discussed" :)

The issue is still existing, FF is still unreliable and delivers broken innerHTML.
DOMPurify patches around it successfully - but we had to change a lot for that.

[mozfreddyb]

Seems like a spec problem to me. shrugs
document.write was invented when HTML parsing wasn't properly defined.
You may have also gotten better results when supplying a doctype prior to your .write().

[cure53]

Nope, it's a Firefox problem :)

FF doesn't properly handle innerHTML in the SVG context. Even MSIE does it right :P