974212 - innerHTML getter: comments containing ">" should not become dangerous

Status: NEW

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

Description

[Jesse Ruderman]

Attachment: testcase

> var v = document.createElement("div"); 
> v.appendChild(document.createComment("1>"));
> v.innerHTML

Result: <!--1<-->

Expected: truncate the comment, omit/replace/escape the '<', or even throw.

Comment 1

[Boris Zbarsky [:bzbarsky]]

Spec issue, yes?

Comment 2

[Mario Heiderich]

AngularJS is affected by this as well:

http://code.google.com/p/mustache-security/wiki/AngularJS#mXSS_via_HTML_Import

Comment 3

[Jesse Ruderman]

This does appear to be a spec issue:

http://www.whatwg.org/specs/web-apps/current-work/multipage/the-end.html#html-fragment-serialization-algorithm

Comment 4

[Anne (:annevk)]

Presumably this is an issue for ProcessingInstruction too. However, is this something we want to fix? The "attack" works across browsers...

Comment 5

[Jesse Ruderman]

Yes, it should be fixed in all browsers. Why do you write "attack" in quotes?

Comment 6

[Anne (:annevk)]

Because various bits of the DOM not being serialization safe is known.

E.g. if instead of your comment you had

var comment = document.createElementNS("test", "img");
comment.setAttribute("onerror", "alert('doh')")
comment.setAttribute("src", "/")

you might think it was safe because it's a different namespace, but it wouldn't be.

Comment 7

[Jesse Ruderman]

Maybe we should fix that too. But I don't think it's especially relevant to comment serialization, which affects at least one popular library and causes *other parts* of the DOM to be completely wrong.

Comment 8

[Henri Sivonen (:hsivonen)]

I'd be OK with making document.createComment() throw if the argument contains "-->" if the DOM spec changes to say so.

(In reply to Mario Heiderich from comment #2)
> AngularJS is affected by this as well:
> 
> http://code.google.com/p/mustache-security/wiki/
> AngularJS#mXSS_via_HTML_Import

The advice there is incomplete. The page should say that foo.innerHTML += "bar" is a bug pattern. Don't use += with innerHTML. Use insertAdjacentHTML() instead.

Comment 9

[Anne (:annevk)]

Is that web-compatible? Would be easy enough to change the specification to require that.

Comment 10

[Henri Sivonen (:hsivonen)]

(In reply to Anne (:annevk) from comment #9)
> Is that web-compatible?

Dunno. We could add telemetry to check for frequency of occurrence, but we can't use telemetry to find out who the offenders (if any) are, because privacy.

Comment 11

[Firefox Bug Husbandry Bot]

https://bugzilla.mozilla.org/show_bug.cgi?id=1472046

Move all DOM bugs that haven’t been updated in more than 3 years and has no one currently assigned to P5.

If you have questions, please contact :mdaly.