974208 - (mXSS) [meta] innerHTML=innerHTML should not introduce XSS holes into web sites

Status: RESOLVED INACTIVE

(Core :: DOM: Core & HTML, defect)

Description

[Jesse Ruderman]

The innerHTML Apocalypse: https://www.hackinparis.com/talk-mario-heiderich
https://cure53.de/fp170.pdf

The presentation seems to place blame on web sites. We can't make innerHTML=innerHTML have no effect, but we should be able to prevent the worst outcomes (XSS) by making reasonable tweaks to the innerHTML getter.

Comment 1

[Mario Heiderich]

Do you have any document showing which properties mutate in Gecko (and maybe other browsers) and an eval about which ones should and which ones shouldn't?

We analyzed the problem for a different vendor and soon realized, that systematic approach is mandatory (and possible). I am not comfortable though of sharing details in a public ticket for contract reasons.

Comment 2

[Henri Sivonen (:hsivonen)]

So what "reasonable tweaks" are being proposed here? We are implementing the relevant specs correctly, AFAICT, and those specs are constrained by backward compatibility.

The slides claim that "Firefox screwed up with SVG". How? The behavior is by design and doesn't appear to result in the alert() running.

If your sanitizer is not HTML5-aware, it should drop <svg>, <style> and the content of <style>. If your sanitizer is not HTML5-aware, it should drop <style> and the content of style. If your sanitizer doesn't drop <style> and its content, it had better have a CSS parser, sanitizer and serializer, too!

If your sanitizer isn't based on an HTML parser, followed by whitelist-based filtering, followed by an HTML serializer, the browser can't help you. There will be another exploit around the corner anyway. Whether your sanitizer is SVG-aware or not doesn't change this.

Comment 3

[Mario Heiderich]

This is how: https://bugzilla.mozilla.org/show_bug.cgi?id=650001

The problem is old and was fixed promptly back then. Firefox indeed "screwed up" by decoding &lt; inside <svg><style>. If that was by design then the design was bad - and Gecko did the right thing to eliminate the behavior.

Comment 4

[Henri Sivonen (:hsivonen)]

(In reply to Mario Heiderich from comment #3)
> This is how: https://bugzilla.mozilla.org/show_bug.cgi?id=650001
> 
> The problem is old and was fixed promptly back then. Firefox indeed "screwed
> up" by decoding &lt; inside <svg><style>.

No. As I said in bug 650001 comment 31, the bug was in the serializer. That is, the bug was in how the innerHTML *getter* *en*coded the less-than sign, not in how the innerHTML *setter* *de*coded &lt;.

(In reply to Mario Heiderich from comment #1)
> Do you have any document showing which properties mutate in Gecko

What do you mean by properties that mutate?

foo.innerHTML = foo.innerHTML is most likely a bug. This means that you are writing a bug when you do foo.innerHTML += "<div>bar</div>". Apart from the += syntax being convenient, it is Mozilla's fault that Web authors have learned to do this, because Mozilla resisted implementing insertAdjacentHTML() due to (in retrospect) misguided anti-Microsoft pro-W3C politics. However, that error has been fixed since Firefox 8. insertAdjacentHTML() is faster, too: https://hacks.mozilla.org/2011/11/insertadjacenthtml-enables-faster-html-snippet-injection/

As filed, this Bugzilla item does not appear to be particularly actionable. What changes, concretely, are being proposed, considering the constraint that the changes must not break the Web? (Any such changes would need to be pursued as spec bugs.)

Comment 5

[Henri Sivonen (:hsivonen)]

Setting needinfos for the question "What changes, concretely, are being proposed, considering the constraint that the changes must not break the Web?"

Comment 6

[Jesse Ruderman]

Generally, I'm only asking for the innerHTML getter to avoid adding scripts where there were no scripts before. Most of these changes involve making the getter represent the original DOM better, but we'll have to consider whether they "break the Web" on a case-by-case basis.

Concretely, the only case I know about is bug 974212.

Comment 7

[Christian Holler (:decoder)]

Unassigning because it is unlikely Jesse will work on this. Freddy, is this bug still relevant (since you filed a blocking bug?

Comment 8

[Frederik Braun [:freddy]]

This one does not seem actionable as per comment 5, but bug 1205631 still does