Fix issue 91

[mmndaniel]

An attempt to fix 91, using approach c which I mentioned here...

[weizman]

Thanks for the PR @mmndaniel ! Highly appreciated 🙏

I'm not sure what the intent here is, but I'm pretty sure something is missing.

According to MDN, the API expects a node rather than a string. Also, the variable you use inside the try-catch block does not exist.

Without the try-catch you'd see this solution doesn't work currently, do you mind explaining again the logic you had in your mind behind this attempt please?

[mmndaniel]

Ah shit, sorry, was coding in a rush! Anyway, the idea is to use XMLSerializer to normalize the HTML and prevent namespace confusion (that leads to the mXSS bug). See section 5.2 here.

[weizman]

yo @mmndaniel check out #108

[mmndaniel]

from the paper linked above:

attacks can be upgraded
to allow recursive mutation – making double-, triple- and
further multiply-encoded escapes and entities useful in the
attack scenario, immediately when multiple innerHTMLaccess to the same element takes place.

So I'm afraid single innerHTML assignment isn't enough (I will try to implement this type of attack soon). It MIGHT be possible to re-assign until two consecutive assignments result the same childNodes.length, but this approach a. have performance drawbacks and b. possibly vulnerable, if an attacker could craft a payload that mutates only after 3 consecutive assignments (not sure if that's possible, but the with complexity of (X/Math/HT)ML and SVG ser/de, who knows).

[weizman]

Here's another idea. Unlike DOMPurify and other projects, in Snow we allow ourselves to disallow certain states if (a) they are highly uncommon in legitimate usage and (b) are dangerous to Snow.

So how about dropping innerHTML set attempts that specifically make use of these special problematic tags? For example, if the html contains a mglyph and/or mtext tags, just block the whole innerHTML set attempt entirely?

Just thinking out loud here, would that be effective?

[mmndaniel]

I don't think it's the way forward because it's possible to implement mutation attack without any "special" tags, just svg, style, etc are enough. See this PR, I suspect that's the right approach to address it.

[weizman]

ending up with #123