Bug 12234 – Using createContextualFragment to insert a <script> does not cause the script to execute

• Status: RESOLVED
• Resolution: FIXED
• Component: DOM

[David Smith]

The testcase url creates an document fragment from an html string containing a script element, but the script is not actually run. Firefox runs the script as expected.

[Alexey Proskuryakov]

A similar case:

document.body.innerHTML+="<script>alert('SUCCESS')</script>"

May or may not need or may already have a separate bug for this.

[Eric Seidel (no email)]

This might get fixed by accident as part of bug 39259 when we get to fragment parsing.

[Henri Sivonen]

Was this bug report motivated by breakage on a real-world site?

Compare with https://bugzilla.mozilla.org/show_bug.cgi?id=599588

[David Smith]

Yes, it was. I worked around it for release though (and then the product was killed shortly afterwards).

[Henri Sivonen]

(In reply to comment #4)
> Yes, it was. I worked around it for release though (and then the product was killed shortly afterwards).

What was the use case you had? Note that if WebKit made createContextualFragment create fragments with executable scripts, inserting the fragments to the document wouldn't guarantee the execution order between external scripts or between external and internal scripts.

[Henri Sivonen]

FWIW, we explicitly decided to make these executable in Firefox 4.

[Eric Seidel (no email)]

Is this a regression from the HTML5 parser re-write?

[Ryosuke Niwa]

I talked with Eric about this and it seems like HTML5 spec intentionally prohibits the script elements coming from the fragments to run.

See http://www.whatwg.org/specs/web-apps/current-work/#parsing-main-inhead where it says:

If the parser was originally created for the HTML fragment parsing algorithm, then mark the script element as "already started". (fragment case)

[Henri Sivonen]

(In reply to comment #8)
> I talked with Eric about this and it seems like HTML5 spec intentionally prohibits the script elements coming from the fragments to run.

Yes, but http://www.w3.org/Bugs/Public/show_bug.cgi?id=11191

[Ryosuke Niwa]

I'm concerned about security implication of enabling scripts.  I can't convince myself that there aren't any websites that rely on the fact WebKit does not execute scripts coming from createContextualFragment.  

While not running scripts expected to run will break the websites, running scripts not expected to run will create a XSS security vulnerability.

I do understand that a fragment created by createContextualFragment is no different than the fragment created by other means and Firefox folks want WebKit to be compatible with Firefox, however, I would avoid the risk of creating a XSS vulnerability at all cost.

[Adam Barth]

rniwa, thanks for being sensitive to creating XSS vulnerabilities.  However, in this case, we're not opening up a new vulnerability.  The attacker can already use other syntactic constructs to execute script, similar to how the attacker can run script via innerHTML even though innerHTML doesn't execute <script> tags.

[Ryosuke Niwa]

(In reply to comment #11)
> rniwa, thanks for being sensitive to creating XSS vulnerabilities.  However, in this case, we're not opening up a new vulnerability.  The attacker can already use other syntactic constructs to execute script, similar to how the attacker can run script via innerHTML even though innerHTML doesn't execute <script> tags.

Ok.  Then we probably should fix this bug to be compatible with Firefox.  Special-casing fragment parsing first seemed strange weird but a number of developers pointed out that the fragment created by createContextualFragment is no different from a fragment created by manually assembling nodes.  And because the script element manually inserted into a fragment runs when the fragment is inserted into a document, we should also run the script parsed by createContextualFragment when the fragment is inserted into a document.

[Ryosuke Niwa]

Per whatwg discussion, we should fix this bug: http://lists.whatwg.org/pipermail/whatwg-whatwg.org/2011-February/030351.html

Also see the bug 86376.

[Ryosuke Niwa]

Note there appears to be a bug in the spec: http://lists.whatwg.org/pipermail/whatwg-whatwg.org/2012-May/036088.html

I'm going to post a patch based on my assumption that we should be clearing both parser inserted and already started flags in createContextualFragment.

[Ryosuke Niwa]

Created attachment 142890 [details]
Fixes the bug

[Ryosuke Niwa]

I can separate the enum rename if reviewers wanted but I didn't feel a need for this simple refactoring other than to boost the number of patches I'm going to commit :)

[Adam Barth]

Comment on attachment 142890 [details]
Fixes the bug

View in context: https://bugs.webkit.org/attachment.cgi?id=142890&action=review

ok

> Source/WebCore/html/parser/HTMLConstructionSite.cpp:340
> +    // For createContextualFragment, the specifications say to mark it parser-inserted and already-started and later unmark them.
> +    // However, we short circuit that logic to avoid the subtree traversal to find script elements since scripts can never see
> +    // those flags or effects thereof

Thanks for adding this comment.  Can we add a link to the spec?  We like to do that in the parser because we track the spec so closely.  (nit: please add a . at the end of the sentence.)

[Ryosuke Niwa]

(In reply to comment #17)
> > Source/WebCore/html/parser/HTMLConstructionSite.cpp:340
> > +    // For createContextualFragment, the specifications say to mark it parser-inserted and already-started and later unmark them.
> > +    // However, we short circuit that logic to avoid the subtree traversal to find script elements since scripts can never see
> > +    // those flags or effects thereof
> 
> Thanks for adding this comment.  Can we add a link to the spec?  We like to do that in the parser because we track the spec so closely.  (nit: please add a . at the end of the sentence.)

Would http://html5.org/specs/dom-parsing.html#dom-range-createcontextualfragment work?

[Adam Barth]

Yeah, perfect.

[Ryosuke Niwa]

Committed r117731: <http://trac.webkit.org/changeset/117731>

[Ryosuke Niwa]

*** Bug 64860 has been marked as a duplicate of this bug. ***