Closed Bug 905370 Opened 6 years ago Closed 6 years ago

Data race on js::SelfHostedClass::head (in JSRuntime::finishSelfHosting())

Categories

(Core :: JavaScript Engine, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla26
Tracking Status
firefox23 --- unaffected
firefox24 + fixed
firefox25 + fixed
firefox26 + fixed
firefox-esr17 --- unaffected
b2g18 --- unaffected

People

(Reporter: bent.mozilla, Assigned: till)

References

Details

(Keywords: csectype-race, regression, sec-critical)

Attachments

(3 files, 1 obsolete file)

JSRuntime::finishSelfHosting() runs on multiple threads (workers) concurrently and modifies a shared static list:

  mozjs.dll!finishSelfHosting - selfhosting.cpp:774
  mozjs.dll!DestroyContext - jscntxt.cpp:283
  mozjs.dll!JS_DestroyContext - jsapi.cpp:873
  xul.dll!~WorkerJSRuntime - runtimeservice.cpp:853
  xul.dll!Run - runtimeservice.cpp:944

Marking s-s until someone can prove that this is not exploitable :)
Till, could you look at this?  It involves self hosting.  Thanks.
Flags: needinfo?(till)
Looking
Assignee: general → till
Status: NEW → ASSIGNED
Flags: needinfo?(till)
I wonder why this doesn't blow up constantly. Instead, it made its way into beta. :(
Attachment #794034 - Flags: review?(bhackett1024)
Things bleeding between runtimes sounds bad, so I'm going to mark this sec-critical.
Attachment #794034 - Flags: review?(bhackett1024) → review+
These three patches should all apply cleanly.

I think a try run is advisable before landing, but I don't know the rules for that. Do I just push the m-i patch to try, accepting that that leaks information, or do we land the patches in the (warranted, IMO) hope that they'll stick?
Attachment #794604 - Flags: review+
Yeah, a try run is fine.  I change the commit message so it doesn't mention the bug, but not everybody even does that...
Thanks for the info, that makes sense.

Rebased and pushed to try here:
https://tbpl.mozilla.org/?tree=Try&rev=c858e0bc2437
Attachment #794034 - Attachment is obsolete: true
Attachment #794609 - Flags: review+
Try run is green, but it occurred to me that running some unit tests might be advisable. New run:
https://tbpl.mozilla.org/?tree=Try&rev=2cdc1ab75f88
Comment on attachment 794602 [details] [diff] [review]
Move SelfHostedClass list to JSRuntime. For beta.

[Approval Request Comment]
Bug caused by (feature/regressing bug #): bug 844882
User impact if declined: potentially sec-critical data race and overwriting of data belonging to other threads
Testing completed (on m-c, etc.): not landed, but green try runs
Risk to taking this patch (and alternatives if risky): low
String or IDL/UUID changes made by this patch: none
Attachment #794602 - Flags: approval-mozilla-beta?
Comment on attachment 794604 [details] [diff] [review]
Move SelfHostedClass list to JSRuntime. For aurora.

[Approval Request Comment]
Bug caused by (feature/regressing bug #): bug 844882
User impact if declined: potentially sec-critical data race and overwriting of data belonging to other threads
Testing completed (on m-c, etc.): not landed, but green try runs
Risk to taking this patch (and alternatives if risky): low
String or IDL/UUID changes made by this patch: none
Attachment #794604 - Flags: approval-mozilla-aurora?
Comment on attachment 794602 [details] [diff] [review]
Move SelfHostedClass list to JSRuntime. For beta.

approval after it actually lands on trunk and things are green.
Attachment #794602 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
Comment on attachment 794604 [details] [diff] [review]
Move SelfHostedClass list to JSRuntime. For aurora.

approval after it actually lands on trunk and things are green.
Attachment #794604 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
But, wait a sec, you didn't fill out sec-approval? for this. 

Sorry for the spam but we need the sec-approval? template to be filled out first.
Attachment #794602 - Flags: approval-mozilla-beta+ → approval-mozilla-beta?
Attachment #794604 - Flags: approval-mozilla-aurora+ → approval-mozilla-aurora?
Comment on attachment 794609 [details] [diff] [review]
Move SelfHostedClass list to JSRuntime.

[Security approval request comment]
How easily could an exploit be constructed based on the patch? Not sure, but since it causes worker threads to be able to free objects from other threads, including the main thread, it can easily be used to create use-after-free scenarios. So probably somewhat 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? beta, aurora

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

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

How likely is this patch to cause regressions; how much testing does it need? not likely
Attachment #794609 - Flags: sec-approval?
Comment on attachment 794609 [details] [diff] [review]
Move SelfHostedClass list to JSRuntime.

sec-approval for trunk and I'll approve it for beta and aurora but please only land after it has cleanly landed on trunk.
Attachment #794609 - Flags: sec-approval? → sec-approval+
Attachment #794604 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Attachment #794602 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
Attachment #794602 - Flags: review+
Attachment #794602 - Flags: approval-mozilla-beta?
Attachment #794602 - Flags: approval-mozilla-beta+
Attachment #794604 - Flags: review+
Attachment #794609 - Flags: review+
Seems like I accidentally overrode the beta a+ for the beta patch.
Attachment #794602 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
Oh, hum. So this required a clobber for windows to build cleanly on inbound. I didn't think of touching the respective CLOBBER files before pushing to beta and aurora, so I just triggered clobbers and restarted the windows builds via the web interface.
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Group: core-security
You need to log in before you can comment on or make changes to this bug.