1154923 - Compartment mismatch in js::ScriptSourceObject::initFromOptions

Status: RESOLVED FIXED

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

Description

[Andrew McCreight [:mccr8]]

I found these stacks on crash-stats:
https://crash-stats.mozilla.com/report/index/b543869b-c423-4b63-af9b-b52ad2150412
https://crash-stats.mozilla.com/report/index/e761453d-caab-41e9-a8f9-6cd162150413

This is similar, but the compartment mismatch is happening right after the call to ParseTask::finish():
https://crash-stats.mozilla.com/report/index/130afcfa-408b-4701-a063-882bc2150413

I'm not sure what the cause is.  GlobalHelperThreadState::finishParseTask() calls gc::MergeCompartments() before this.

Comment 1

[Andrew McCreight [:mccr8]]

I'm marking this sec-high because it is a mismatch, but because it is in the parser I'm not sure if it could really be exploited.

Comment 2

[Andrew McCreight [:mccr8]]

Can you look at this Jim? I'm still seeing this on trunk on crash-stats.

Comment 3

[Andrew McCreight [:mccr8]]

Here are more recent examples:
https://crash-stats.mozilla.com/report/index/ebccf39b-3a58-4b6b-92e9-1ca052150530
https://crash-stats.mozilla.com/report/index/1a5499da-c77c-4da5-a512-36a962150601

Comment 4

[Jim Blandy :jimb]

In all these crashes:

- We start an off-thread compilation of a <script> element.

- The SpiderMonkey helper thread completes the compilation and calls OffThreadScriptLoaderCallback, which queues a NotifyOffThreadScriptLoadCompletedRunnable to the main thread.

- The main thread event loop eventually calls NotifyOffThreadScriptLoadCompletedRunnable::Run. Now we've reached the stacks appearing in the crash reports.

- From here, nsScriptLoader::EvaluateScript chooses a global object to finish the compilation in: https://dxr.mozilla.org/mozilla-central/source/dom/base/nsScriptLoader.cpp#1123

- That's passed in to nsJSUtils::EvaluateString, which enters that global on the JSContext here: https://dxr.mozilla.org/mozilla-central/source/dom/base/nsJSUtils.cpp#239

- It then calls JS::FinishOffThreadScript, which notices that the JSContext is in a compartment different from the script the compilation produced.

It seems to me that Gecko is trying to enter the compilation's compartment on its JSContext, but SpiderMonkey is correct that this compartment is not the one the compilation was actually started in.

I wonder, is it possible that nsScriptLoader starts a compilation in one compartment, and then something happens that changes the JSObject global (inner window) associated with the nsScriptLoader, and then the compilation completes, but we don't recognize that we don't care about running the script any more?

Comment 5

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

The script loader is tied to the document, iirc, so if something changes the inner but not the document we could have a bad time.

Comment 6

[Olli Pettay [:smaug][[e-mail]]]

And document.open()/write() after document load creates a new inner window, yet keeps the old document.

Comment 7

[Jim Blandy :jimb]

I think it could be exploited, because it does introduce cross-compartment edges that should not be there, and those edges refer to script. So I think the sec-high mark is probably appropriate.

Comment 8

[Boris Zbarsky [:bzbarsky]]

Ok, so presumably the scenario here is an async script that finishes loading, we start the off-thread parse, and while it's parsing the page does a document.open().

What _should_ the DOM do in this case exactly?  We need to execute the script in the new global document.open() creates.  What are our options?  Clone the script into the new global?  Throw away the off-thread compile and restart the compile?  Something else?

Comment 9

[Olli Pettay [:smaug][[e-mail]]]

Assuming this is some async script, I'd assume we should not process the script if it is loaded after someone called document.open() after page load, since document.open() removes all the elements from document. (but I haven't checked what the spec says about this)
I don't see nsScriptLoader::ProcessRequest doing any IsIn*Doc() checks.

Comment 10

[Boris Zbarsky [:bzbarsky]]

> since document.open() removes all the elements from document.

Removing a <script> tag for a script that's started loading doesn't prevent it from running when the load finishes.  Simple testcase:

  <script>
  var s = document.createElement("script");
  s.src = "data:text/plain,alert('hello')";
  document.documentElement.appendChild(s);
  s.remove();
  </script>

> I don't see nsScriptLoader::ProcessRequest doing any IsIn*Doc() checks

Because it's not supposed to...

Comment 11

[Andrew McCreight [:mccr8]]

I guess I'll try to figure out something here.

Comment 12

[Andrew McCreight [:mccr8]]

I tried looking at this, but I have no idea what is going on here. Could you look at this, Boris? Thanks.

Comment 13

[Boris Zbarsky [:bzbarsky]]

Well, we know what's going wrong.  See comment 4.  The question is what behavior Spidermonkey will let us have here, from the options in comment 8...  Jason, Eric, who might be able to look into those options on the SpiderMonkey end here?

Comment 14

[Kannan Vijayan [:djvj]]

(In reply to Boris Zbarsky [:bz] from comment #8)
> Throw away the off-thread compile and restart the compile?

This seems like the best option to me.  Is a |document.open()| while an async script is off-thread-parsing a relatively rare occurrence?  If so, we can probably discard the compile and re-schedule it.

So the logic would be to check the new global, and if it differs, throw away the compile compartment and re-schedule a parse?

Comment 15

[Boris Zbarsky [:bzbarsky]]

> Is a |document.open()| while an async script is off-thread-parsing a relatively rare
> occurrence?

Yes.

> So the logic would be to check the new global, and if it differs, throw away the compile
> compartment and re-schedule a parse?

That's the idea.  Should that happen in SpiderMonkey or the scriptloader?  I think the scriptloader hands over ownership of the buffer to SpiderMonkey, so if we can do all this inside SpiderMonkey that would be ideal...

Comment 16

[Kannan Vijayan [:djvj]]

(In reply to Boris Zbarsky [:bz] from comment #15)
> That's the idea.  Should that happen in SpiderMonkey or the scriptloader?  I
> think the scriptloader hands over ownership of the buffer to SpiderMonkey,
> so if we can do all this inside SpiderMonkey that would be ideal...

This would be difficult.  The interaction for off-main-thread parsing goes back and forth between the scriptloader and spidermonkey.

Scriptloader starts the off-main-thread parse, which causes spidermonkey to schedule the parse on a different thread, and when that completes, spidermonkey dispatches an event back to scriptloader, which calls FinishOffThreadScript back on the main thread.

The particular invocation of FinishOffThreadScript happens inside nsJSUtils::EvaluateString.  When the scriptloader calls EvaluateString, it expects the script to run synchronously.  It may be OK for the script to be re-scheduled at that point, but that change in behaviour would need to be carefully verified and might cause weird breakages in assumptions (I don't know the code well enough to say that it wouldn't).

It seems best if ScriptLoader handles this itself.  When it gets notified that a parse is complete (via NotifyOffThreadScriptLoadCompletedRunnable::Run() - which runs on the main thread), it needs to immediately call into spidermonkey to finish the off-thread parse.  If a mismatched global is detected at that point, it can re-schedule without ever going into the script execution codepath.

Comment 17

[Andrew McCreight [:mccr8]]

Maybe the second time's the charm.

Comment 18

[Andrew Overholt [:overholt]]

What's the status here, Andrew?

Comment 19

[Andrew McCreight [:mccr8]]

My most recent patch leaks:
  https://treeherder.mozilla.org/#/jobs?repo=try&revision=c813c7fd7e75

Calling GetScriptGlobalObject() inside nsScriptLoader::ProcessOffThreadRequest() is doing that somehow.

Maybe I should just throw a release compartment check into ParseTask::finish() and call it a day.

Comment 20

[Andrew McCreight [:mccr8]]

Attachment: Compartment checking for off-thread script loading. WIP.

This is what I have so far. The try run is green, but if I make the code in nsScriptLoader::ProcessOffThreadRequest() always fail then pages aren't loading correctly, while I think the desired behavior is that things load properly, just more slowly. So I may not be inserting the check in the right place.

Comment 21

[Andrew McCreight [:mccr8]]

Maybe I should just make these two compartment checks fatal and be done with it. This wasn't happening very much in the wild, so it shouldn't negatively affect stability.

Comment 22

[Andrew McCreight [:mccr8]]

Attachment: Add a version of assertSameCompartment that works in all versions.

Not the most elegant solution, but this has been sitting around for too long.

try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=3d074ec9baaa

Comment 23

[Terrence Cole [:terrence]]

Comment on attachment 8755525 [details] [diff] [review]
Add a version of assertSameCompartment that works in all versions.

Review of attachment 8755525 [details] [diff] [review]:
-----------------------------------------------------------------

This seems fine as a short-term hack. Let's just make sure that the real bug gets fixed as well.

Comment 24

[Andrew McCreight [:mccr8]]

Comment on attachment 8755525 [details] [diff] [review]
Add a version of assertSameCompartment that works in all versions.

[Security approval request comment]
How easily could an exploit be constructed based on the patch? Not very easily.

Do comments in the patch, the check-in comment, or tests included in the patch paint a bulls-eye on the security problem? no

Which older supported branches are affected by this flaw? all

If not all supported branches, which bug introduced the flaw?

Do you have backports for the affected branches? If not, how different, hard to create, and risky will they be? should be easy

How likely is this patch to cause regressions; how much testing does it need? This patch detects a bad situation and just crashes. I can't be sure how much this actually happens. It didn't happen much in the wild on Nightly and Aurora, as of a year ago, but who knows now. So that's a risk.

Comment 25

[Al Billings [:abillings - ex-MoCo]]

This is too late for 47. We're making release builds in less than a week. This can check in on June 21, 2016, two weeks into the next cycle.

We'll want branch patches at that time as well, including ESR45.

Comment 26

[Al Billings [:abillings - ex-MoCo]]

(This sec-approval+ only applies to checkin on 6/21)

Comment 27

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/88213a36d165

Comment 28

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/88213a36d165

Comment 29

[Andrew McCreight [:mccr8]]

Comment on attachment 8755525 [details] [diff] [review]
Add a version of assertSameCompartment that works in all versions.

Approval Request Comment
[Feature/regressing bug #]: old
[User impact if declined]: sec-high
[Describe test coverage new/current, TreeHerder]: this code runs frequently
[Risks and why]: This turns a very rare Nightly/Aurora crash into one that will happen on all branches. The main risk is that this gets triggered frequently on beta or release. But, if it is, then probably bad things are happening anyways, so maybe it isn't too bad to just crash safely.
[String/UUID change made/needed]: none

Comment 30

[Andrew McCreight [:mccr8]]

This has been on Nightly for a few days, and it doesn't seem to have caused any issues.

Comment 31

[Sylvestre Ledru [:Sylvestre]]

Comment on attachment 8755525 [details] [diff] [review]
Add a version of assertSameCompartment that works in all versions.

Fix a sec-high, taking it

Comment 32

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/releases/mozilla-aurora/rev/f96fe0adc4e6
https://hg.mozilla.org/releases/mozilla-beta/rev/b101ef9dfd00
https://hg.mozilla.org/releases/mozilla-esr45/rev/2786beb35a38