Closed Bug 592366 Opened 14 years ago Closed 14 years ago

Do not execute scripts whose owner doc is not the doc of the inserter parser or not the owner doc at the time of starting the script load

Categories

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

Product:
Core
Component:
DOM: Core & HTML
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED FIXED
Tracking Flags:
Tracking Status
blocking2.0 --- betaN+

People

(Reporter: hsivonen, Assigned: hsivonen)

References

(
URL
)

Details

(Keywords: html5)

Attachments

(1 file, 2 obsolete files)

Do not execute scripts whose owner doc is not the doc of the inserter parser or not the owner doc at the time of starting the script load
14.67 KB, patch
sicking
: review+
Details | Diff | Splinter Review 
By code inspection, it seems that the handling of defer scripts and script that block the parser is really broken if an ancestor of the node the parser is appending to has been moved to another document during the parse.

Consider <div><script> // move the div to another doc </script><script src=blocks.js></script><!-- we never get unblocked! -->

To make things work, if the script has a creator parser, the document associated with that parser should be used for obtaining the script loader instead of using the script's owner doc.
Another scenario:  Consider a XUL application which uses a DOMParser to generate a data document, and the data document has a <xul:script/> element.  Then the XUL app appends the parent of the script element to itself...

Do we want to allow running, loading or parsing scripts in that element?
(In reply to comment #1)
> Another scenario:  Consider a XUL application which uses a DOMParser to
> generate a data document, and the data document has a <xul:script/> element. 
> Then the XUL app appends the parent of the script element to itself...
> 
> Do we want to allow running, loading or parsing scripts in that element?

That's scary. Unless there's a really good use case for that kind of privilege escalation, I'd want to make DOMParser call PreventExecution() on each script.
URL: http://hsivonen.iki.fi/test/moz/move-...
Summary: Parser-inserted scipts should use the script loader of the parser's document--not the script's document → Parser-inserted scripts should use the script loader of the parser's document--not the script's document
sicking, are you OK with the general approach here?
 * Parser-inserted scripts use the script loader associated with the document of the parser.
 * Script-inserted scripts use the script loader associated with the owner doc of the script element
 * External scripts use the load group of the document of the script loader.
 * Security checks are performed against the owner doc at the time of the "run" algorithm.
 * Evaluation is done against the script global object of the owner doc at the time of evaluation.

If you are OK, I can cancel my pursuit to change the spec.
http://lists.whatwg.org/pipermail/whatwg-whatwg.org/2010-September/028356.html

Nominating as a blocker. Gecko's current behavior is very broken on http://hsivonen.iki.fi/test/moz/move-during-parse-parent2.html (the parser never gets unblocked) and IE9 has made an effort to be HTML5-compliant in the http://hsivonen.iki.fi/test/moz/move-during-parse-parent.html case and their implementation is less broken than ours in the http://hsivonen.iki.fi/test/moz/move-during-parse-parent2.html case. OTOH, this is a rather esoteric edge case that has been broken in Gecko forever (including Firefox 3.6.x).
blocking2.0: --- → ?
Keywords: html5
blocking2.0: ? → betaN+
Hmm.. so this patch scares me.

The fact that we now have an mDocument member, but for most intents and purposes it's not the one you want to use, seems like asking for bugs. If not now then in the future. And this is especially bad since often time when we need a document it is to do security checks.

There's also the fact that content policies will often only be called with the parsing document as context, whereas the script can execute in any context.

Additionally, I believe that some of our security checks use the loadgroup to figure out what security context (document/window/docshell/etc) to use.


I think we might want to always execute the script in the global of the document being parsed. I.e. the first three bullets in comment 4 sound good, but the last two should be to evaluate and do security checks using the document of the scriptloader.
Would be great to get bz's input on this too, see comment 4 and on.
How about just not executing the script if the documents involved don't match?  Would that break any sites?  If not, it seems like the safest behavior from a security viewpoint...
(In reply to comment #7)
> Hmm.. so this patch scares me.
> 
> The fact that we now have an mDocument member, but for most intents and
> purposes it's not the one you want to use, seems like asking for bugs. If not
> now then in the future. And this is especially bad since often time when we
> need a document it is to do security checks.
> 
> There's also the fact that content policies will often only be called with the
> parsing document as context, whereas the script can execute in any context.
> 
> Additionally, I believe that some of our security checks use the loadgroup to
> figure out what security context (document/window/docshell/etc) to use.

Which ones?

> I think we might want to always execute the script in the global of the
> document being parsed. I.e. the first three bullets in comment 4 sound good,
> but the last two should be to evaluate and do security checks using the
> document of the scriptloader.

When I had that reaction to the spec (http://lists.whatwg.org/pipermail/whatwg-whatwg.org/2010-September/028356.html), you said it made no security difference (http://lists.whatwg.org/pipermail/whatwg-whatwg.org/2010-September/028358.html). Even after I pointed out CSP (http://lists.whatwg.org/pipermail/whatwg-whatwg.org/2010-September/028384.html), you still *seemed* to be OK with the spec (http://lists.whatwg.org/pipermail/whatwg-whatwg.org/2010-September/028429.html).

I wrote this patch, because I seemed to be alone with my concern and others seemed to be OK with the spec.

What made you change your mind?

(In reply to comment #9)
> How about just not executing the script if the documents involved don't match? 

If which documents don't match? The owner doc and the parser's doc? Or the owner doc at the time of execution and the script loader's mDocument?
> Which ones?

Anything using nsILoadContext, or anything using getInterface for docshell-related stuff on notification callbacks.  This includes some cookie bits and various extensions at the very least (for nsILoadContext); searching for the docshell-related stuff is harder.

> The owner doc and the parser's doc? Or the owner doc at the time of execution
> and the script loader's mDocument?

"yes".  If there is any confusion at all about what document is involved, don't run the script.  This still leaves the loadgroup and parser unblocking question open, I suppose...
(In reply to comment #12)
> "yes".  If there is any confusion at all about what document is involved, don't
> run the script.  This still leaves the loadgroup and parser unblocking question
> open, I suppose...

Does this make us any safer when there's stuff like <img onload="dangerousCode();">?
Safer in terms of what?  Safer in terms of actually enforcing user-defined security policies in things like noscript?  Absolutely.
(In reply to comment #14)
> Safer in terms of what?  Safer in terms of actually enforcing user-defined
> security policies in things like noscript?  Absolutely.

Safer in terms of moved nodes not running scripts in the context of another script global object. (If the nodes have moved over in the first place, the must have been a same origin check anyway.)
> When I had that reaction to the spec
> (http://lists.whatwg.org/pipermail/whatwg-whatwg.org/2010-September/028356.html),
> you said it made no security difference
> (http://lists.whatwg.org/pipermail/whatwg-whatwg.org/2010-September/028358.html).
> Even after I pointed out CSP
> (http://lists.whatwg.org/pipermail/whatwg-whatwg.org/2010-September/028384.html),
> you still *seemed* to be OK with the spec
> (http://lists.whatwg.org/pipermail/whatwg-whatwg.org/2010-September/028429.html).
> 
> I wrote this patch, because I seemed to be alone with my concern and others
> seemed to be OK with the spec.
> 
> What made you change your mind?

Seeing the patch and how it made script loading much more fragile.

I'm sorry for not realizing this sooner :(
What should the spec say so that Gecko can match the spec?

I don't see any security problem with executing script-inserted scripts in the context of the script global object of the document whose active parser the inserted parser is as sicking suggested in comment 7.

bz, do you see a problem with doing that?

If in nsScriptLoader::ProcessRequest the owner doc of the script element doesn't match mDocument, what should happen? Should onerror fire?
The safest/simplest thing to do would be to say something like:

If the owner document of the <script> element is different from the document-that-is-being-parsed at the time when the script is parsed, or at the time when the script is ready to execute, then don't execute the script.


I'd say unless there are use cases for executing these scripts, then I'd prefer to keep things simple and not execute them at all.


Alternatively, the spec could say that scripts always execute in the context of the document-that-is-being-parsed. But again, I'd like to hear use cases or other reasons to execute them.
Comment on attachment 486331 [details] [diff] [review]
Make parser-inserted scripts use the script loader of the parser-associated document instead of the owner document of the script; make scripts evaluate using the global object of their owner document

Removing request here for now. If things change feel free to rerequest.
Attachment #486331 - Flags: review?(jonas)
If this patch is OK, let's get another bug on file for canceling pending scripts as soon as they are removed from the doc.
Attachment #486331 - Attachment is obsolete: true
Attachment #494687 - Flags: review?(jonas)
Comment on attachment 494687 [details] [diff] [review]
Do not execute scripts whose owner doc is not the doc of the inserter parser or not the owner doc at the time of starting the script load

Sweet!
Attachment #494687 - Flags: review?(jonas) → review+
Resummarizing as morphed during review.

http://hg.mozilla.org/mozilla-central/rev/22f53d50851a
Status: ASSIGNED → RESOLVED
Closed: 14 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Summary: Parser-inserted scripts should use the script loader of the parser's document--not the script's document → Do not execute scripts whose owner doc is not the doc of the inserter parser or not the owner doc at the time of starting the script load
Henri, what is the reason for the 500ms timeout you added here: <http://mxr.mozilla.org/mozilla-central/source/content/base/test/test_bug592366.html?force=1#40>?  Over in bug 649012, I'm killing these sorts of timesouts, and I want to know what is the event that you're actually expecting to happen during that 500ms to make sure that the test is testing the right thing?

If EvaluateScript is called when the script node is bound to the document (which I think is the case), the timeout shouldn't be necessary, and the code you're testing should be hit as part of each of the appendChild calls.  In that case, we can simply remove this timeout.
Blocks: FlakyTimeout
(In reply to comment #25)
> Henri, what is the reason for the 500ms timeout you added here:

The code above it adds an external script to the iframes. The timeout gives the external script a chance to execute. Note that even data: URLs need a couple of event loops cycles to be processed.
(In reply to comment #26)
> (In reply to comment #25)
> > Henri, what is the reason for the 500ms timeout you added here:
> 
> The code above it adds an external script to the iframes. The timeout gives the
> external script a chance to execute. Note that even data: URLs need a couple of
> event loops cycles to be processed.

What is the exact mechanism in which this event is processed?  Are there a fixed number of event loop round-about trips?  Is there an event/notification that we can use?  My concern here is whether there is any way for us to avoid using a timeout here, because just waiting 500ms doesn't seem reliable.
This sounds like the ever occurring "ensure that an asynchronously occurring event *doesn't* happen".

So there is no event that we can wait for since what we're doing here is to deliberately do nothing.

The upshot is that flakyness of timeout will here cause intermittent greens in a in sea of orange if we were to regress. Rather than a intermittent orange in a sea of green when the code works properly.


I don't have a good way to solve this. One thing could be to introduce something similar to SimpleTest.executeSoon(), but which takes an integer argument which indicates how many times to loop through the event loop.

But I'd be worried about people abusing that the same way they abuse setTimeout now. Maybe it would be slightly better since it's less flaky. And maybe we could name something like SimpleTest.waitForEventToNotHappen(number, callback) to reduce the risk of abuse.

Though it could be argued weather such a function would be better backed by a timer in that it would produce more permanent orange in case there is a regression.
(In reply to comment #28)
> This sounds like the ever occurring "ensure that an asynchronously occurring
> event *doesn't* happen".

Indeed.

> So there is no event that we can wait for since what we're doing here is to
> deliberately do nothing.

:(

> The upshot is that flakyness of timeout will here cause intermittent greens in
> a in sea of orange if we were to regress. Rather than a intermittent orange in
> a sea of green when the code works properly.

I'd prefer a solution where the test just plain fails all the time, if that's achievable.

> I don't have a good way to solve this. One thing could be to introduce
> something similar to SimpleTest.executeSoon(), but which takes an integer
> argument which indicates how many times to loop through the event loop.

I've already adopted this idea in a number of places, using something like this: <http://mxr.mozilla.org/mozilla-central/source/editor/libeditor/text/tests/test_bug527935.html?force=1#64>

> But I'd be worried about people abusing that the same way they abuse setTimeout
> now. Maybe it would be slightly better since it's less flaky. And maybe we
> could name something like SimpleTest.waitForEventToNotHappen(number, callback)
> to reduce the risk of abuse.

The abusing potential worries me too, but I think in general this is a useful idiom to support in SimpleTest.  And we can always mxr and file the abusers and publicly shame them.  ;-)  But I haven't made up my mind quite certainly yet.

> Though it could be argued weather such a function would be better backed by a
> timer in that it would produce more permanent orange in case there is a
> regression.

Hmm, I'm not sure what you mean.  Why would this be any worse than a timeout?  I mean, if we're waiting to see an event happens, if we hit the event loop 100 times or so, we can still be sure that the thing we're waiting for has not happened (because the test is blocked during this time, hopefully, unless somebody misuses this idiom).  And we have the upside of not racing against a magical time value in which things are expected to occur.

I find the notion of "hitting the event loop 100 times" much less flaky than "waiting for 1 second", for example, since in the latter case, we have no idea if the event loop has been hit enough times for the thing which we're watching for has had a chance to have happened.

What do you think?
> The timeout gives the external script a chance to execute.

Can't we watch for the "afterscriptexecute" event for the script instead?
Depends on: 726910
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: