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

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Ben Turner (not reading bugmail, use the needinfo flag!)]

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 :)

Comment 1

[Andrew McCreight [:mccr8]]

Till, could you look at this?  It involves self hosting.  Thanks.

Comment 2

[Till Schneidereit [:till]]

Looking

Comment 3

[Till Schneidereit [:till]]

Attachment: Move SelfHostedClass list to JSRuntime.

I wonder why this doesn't blow up constantly. Instead, it made its way into beta. :(

Comment 4

[Andrew McCreight [:mccr8]]

Things bleeding between runtimes sounds bad, so I'm going to mark this sec-critical.

Comment 5

[Till Schneidereit [:till]]

Attachment: Move SelfHostedClass list to JSRuntime. For beta.

Comment 6

[Till Schneidereit [:till]]

Attachment: Move SelfHostedClass list to JSRuntime. For aurora.

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?

Comment 7

[Andrew McCreight [:mccr8]]

Yeah, a try run is fine.  I change the commit message so it doesn't mention the bug, but not everybody even does that...

Comment 8

[Till Schneidereit [:till]]

Attachment: Move SelfHostedClass list to JSRuntime.

Thanks for the info, that makes sense.

Rebased and pushed to try here:
https://tbpl.mozilla.org/?tree=Try&rev=c858e0bc2437

Comment 9

[Till Schneidereit [:till]]

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 10

[Till Schneidereit [:till]]

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

Comment 11

[Till Schneidereit [:till]]

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

Comment 12

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

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.

Comment 13

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

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.

Comment 14

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

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.

Comment 15

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

See https://wiki.mozilla.org/Security/Bug_Approval_Process

Comment 16

[Till Schneidereit [:till]]

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

Comment 17

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

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.

Comment 18

[Shu-yu Guo [:shu]]

Seems like I accidentally overrode the beta a+ for the beta patch.

Comment 19

[Till Schneidereit [:till]]

Landed on m-i: https://hg.mozilla.org/integration/mozilla-inbound/rev/be4a241c1cf3

Comment 20

[Till Schneidereit [:till]]

https://hg.mozilla.org/releases/mozilla-beta/rev/d856284278cc
https://hg.mozilla.org/releases/mozilla-aurora/rev/83ae78484491

Comment 21

[Till Schneidereit [:till]]

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.

Comment 22

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/be4a241c1cf3