1120655 - Make the analysis detect compartment iterator invalidation

Status: RESOLVED FIXED

(Core :: JavaScript: GC, defect)

Description

[Steve Fink [:sfink] [:s:]]

Bug 1119579 found a case where we are iterating over the list of compartments while potentially modifying it. The hazard analysis can be modified to detect this case, since the main ways the list could be modified are (1) a garbage collection collecting one or more of the compartments and (2) running JS script code that creates new compartments. Running script can always GC, so the same mechanism can be used for both: label CompartmentsIter a GCPointer so the analysis will complain if you can GC while it is alive.

Comment 1

[Steve Fink [:sfink] [:s:]]

Attachment: Label various compartment iters as GCPointers

The analysis change is straightforward, though it exposed a legitimate hazard.

Comment 2

[Terrence Cole [:terrence]]

Comment on attachment 8547789 [details] [diff] [review]
Label various compartment iters as GCPointers

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

Neat!

Comment 3

[Steve Fink [:sfink] [:s:]]

Attachment: Fix out of bounds error in JS_iterateCompartments

JS_IterateCompartments takes a general callback, and thus is inherently unsafe.

There is a single caller in the tree, which goes over the principals and grabs out their script source strings. Unfortunately, it appears that this can not only GC, but it *might* be possible for it to run script got since it goes through nsIURI.

One way to fix this is to not worry about the exact behavior of the iterator, and just make sure it never accesses anything out of bounds. If you call script or GC during iteration, you may or may not get a callback for all live compartments. But you should never see any dead ones, nor should you access anything off the end of the list.

I'm really not sure whether this is better or worse than suppressing GC during iteration. My concern with suppressing GC is that I think the iteration may happen for about:memory, which is commonly loaded when memory is low.

Comment 4

[Terrence Cole [:terrence]]

Comment on attachment 8547795 [details] [diff] [review]
Fix out of bounds error in JS_iterateCompartments

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

Not showing all compartments in about:memory would also be pretty bad. This seems less bad at least.

::: js/src/gc/Zone.h
@@ +386,5 @@
>  
>      bool done() const {
>          MOZ_ASSERT(it);
> +        return it < zone->compartments.begin() ||
> +               it >= zone->compartments.begin();

I'm guessing you meant it >= *end* and not it >= begin here.

Comment 5

[Steve Fink [:sfink] [:s:]]

The next hazard reported is:

Function '_ZN2js11WeakMapBase16traceAllMappingsEPNS_13WeakMapTracerE|void js::WeakMapBase::traceAllMappings(js::WeakMapTracer*)' has unrooted 'c' of type 'class js::CompartmentsIterT<js::ZonesIter>' live across GC call js::WeakMapBase.traceMappings at js/src/jsweakmap.cpp:135

The problem here is js::TraceWeakMaps. It is called by https://dxr.mozilla.org/mozilla-central/source/xpcom/base/CycleCollectedJSRuntime.cpp#245 and another place in the same file, and it ends up calling the js::WeakMapTracer.callback function pointer.

One lesson: callback-based APIs make things tough for the static analysis.

So I believe this is a false positive. But determining that is not straightforward. This is called by nsCycleCollector::FixGrayBits, which in fact *does* GC, but only after calling mJSRuntime->FixWeakMappingGrayBits(). So it's not like I can just say "the cycle collector won't GC while it's figuring out the gray bits."

I guess the trick would be to prove that the WeakMapTracer.callback won't GC. The easiest way to do that would be to make the API use a virtual function.

Comment 6

[Steve Fink [:sfink] [:s:]]

(In reply to Terrence Cole [:terrence] from comment #4)
> > +        return it < zone->compartments.begin() ||
> > +               it >= zone->compartments.begin();
> 
> I'm guessing you meant it >= *end* and not it >= begin here.

Nah, the indentation lined up better with them both using begin(), so I think I'll keep it that way.

:-) Thanks for the catch

Comment 7

[Andrew McCreight [:mccr8]]

If you can't get to fixing the analysis soon, sfink, maybe you could spin the bounds check off into a separate bug?

Comment 8

[Steve Fink [:sfink] [:s:]]

Sorry, this dropped off my radar. Seems like I'll need 3 bugs -- one for the reviewed fix here that can land anytime (but needs sec-approval), one for the traceMappings thing that I don't have a fix for yet, and one for the analysis fix that can't land until the other two are in.

Comment 9

[Steve Fink [:sfink] [:s:]]

Attachment: Fix out of bounds error in JS_iterateCompartments,

Comment 10

[Steve Fink [:sfink] [:s:]]

Comment on attachment 8569986 [details] [diff] [review]
Fix out of bounds error in JS_iterateCompartments,

Doh! bzexport insisted on reusing this bug even though I changed the number in the patch. Note to self: change the mq patch name too.

I'll just obsolete this one, since I'm moving it to bug 1137326 anyway.

Comment 11

[Andrew McCreight [:mccr8]]

Is this ready to land?

Comment 12

[Steve Fink [:sfink] [:s:]]

(In reply to Andrew McCreight [:mccr8] from comment #11)
> Is this ready to land?

Yes, as of yesterday with https://hg.mozilla.org/mozilla-central/rev/7d8224f00aec (from bug 1137336). That was the last of the fixes to land.

I marked lots of things as affected, due to bug 1137326, but that doesn't mean we need to land this everywhere. The only thing we really need to avoid is having this patch land on a branch that one of the actual fixes (in dependent bugs) has not landed on. Because that would make the hazard analysis fail on treeherder and point directly to a problem. Having this patch landed on a given branch just means that we could detect future landings that add hazards of a sort it can detect, which is pretty unlikely in the first place for any branch other than mozilla-central.

Comment 13

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

Backed out in https://hg.mozilla.org/integration/mozilla-inbound/rev/ea3e5ea5c6f2 for introducing some hazards to the build:

https://treeherder.mozilla.org/logviewer.html#?job_id=7217268&repo=mozilla-inbound

Comment 14

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

Is this going to make the current 39 (trunk) cycle (and then maybe branches)? I know it bounced two weeks ago on checkin but I'm not sure where it is in the queue to be addressed. Since it is a sec-high rated issue, I want to follow up on it.

Comment 15

[Steve Fink [:sfink] [:s:]]

(In reply to Al Billings [:abillings] from comment #14)
> Is this going to make the current 39 (trunk) cycle (and then maybe
> branches)? I know it bounced two weeks ago on checkin but I'm not sure where
> it is in the queue to be addressed. Since it is a sec-high rated issue, I
> want to follow up on it.

Thanks for checking. When I expanded this check to include zone invalidation, I uncovered a few more problems, and decided that it would be better to make this pattern safe rather than guard against it with the analysis (since I couldn't come up with a clean way of fixing some of the revealed hazards other than by suppressing GC entirely, which could lead to OOM crashes.)

So I will not be landing the reviewed patch after all. Instead, I'd like to do something different here. I will be uploading that patch shortly.

Comment 16

[Steve Fink [:sfink] [:s:]]

Attachment: Rename lastGC -> destroyingRuntime

First, a purely cosmetic patch because it confused me when tracing through this stuff. There's a 'lastGC' boolean floating around. I wasn't sure if it meant "previous GC", "last slice in an incremental GC", or what. It turns out that it means it's the final GC when destroying the whole JSRuntime. So I renamed it to 'destroyingRuntime'.

Comment 17

[Steve Fink [:sfink] [:s:]]

Attachment: Suppress zone/compartment collection while iterating

The real patch. This tracks whether we are currently iterating over zones or compartments, and prevents them from being swept if so. This cleans up a whole pile of hazards (I think there were 11, though most were false positives.)

Comment 18

[Terrence Cole [:terrence]]

Comment on attachment 8579436 [details] [diff] [review]
Rename lastGC -> destroyingRuntime

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

That reads much better.

Comment 19

[Terrence Cole [:terrence]]

Comment on attachment 8579438 [details] [diff] [review]
Suppress zone/compartment collection while iterating

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

::: js/src/gc/GCRuntime.h
@@ +1006,5 @@
>      int64_t lastGCTime;
>  
>      JSGCMode mode;
>  
> +    mozilla::Atomic<size_t, mozilla::ReleaseAcquire> activeIters;

Could we call it |activeZoneIters| so that it is clearer in sweepZones what sort of iterations cause us to bail?

Comment 20

[Steve Fink [:sfink] [:s:]]

Attachment: Rollup patch for approvals and backports

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

The patch is about fixing a general class of problems, so it doesn't point to any specific exploit itself. But it does give a good direction to look for exploits. On the other hand, iterator invalidation is already pretty well known as a ripe area to find boundary overruns in, and this particular set of iterators is harder than most to exploit.

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

Not really.

Which older supported branches are affected by this flaw?

The specific problems are varied, but they date way back. esr31 definitely has *something* related to this in it.

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

Sadly, I expect there will be some minor syntactical differences that will make this a little different for the different branches. They'll be doing exactly the same thing, though, and should be easy to create.

How likely is this patch to cause regressions; how much testing does it need?

At least in theory, the only thing that this patch can introduce is OOM during iteration, but the amount of memory we're talking about here is tiny. And the relevant iterations are rare. So I'd say fairly low risk.

Comment 21

[Steve Fink [:sfink] [:s:]]

Comment on attachment 8580354 [details] [diff] [review]
Rollup patch for approvals and backports

Carrying over r=terrence

Comment 22

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

Comment on attachment 8580354 [details] [diff] [review]
Rollup patch for approvals and backports

sec-approval+.

How hard would this be to get on at least Aurora (yes, I saw the comment in the approval request)?

Comment 23

[Steve Fink [:sfink] [:s:]]

(In reply to Al Billings [:abillings] from comment #22)
> Comment on attachment 8580354 [details] [diff] [review]
> Rollup patch for approvals and backports
> 
> sec-approval+.
> 
> How hard would this be to get on at least Aurora (yes, I saw the comment in
> the approval request)?

I'll need to wait for the tree to open so I can get it into inbound first, but I'll upload a version for aurora now (as expected, it wasn't an automatic backport, but it was easy).

Comment 24

[Steve Fink [:sfink] [:s:]]

Attachment: Backport to aurora

Comment 25

[Steve Fink [:sfink] [:s:]]

Attachment: Backport to beta

Comment 26

[Steve Fink [:sfink] [:s:]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/81de36604bf2
https://hg.mozilla.org/integration/mozilla-inbound/rev/74aab037e628

Comment 27

[Steve Fink [:sfink] [:s:]]

Comment on attachment 8580354 [details] [diff] [review]
Rollup patch for approvals and backports

[Approval Request Comment]
If this is not a sec:{high,crit} bug, please state case for ESR consideration:
User impact if declined: potentially exploitable iterator invalidation bugs. Though in reviewing a list of the zone-specific hazards in https://ftp-ssl.mozilla.org/pub/mozilla.org/firefox/try-builds/[e-mail]-fa0c0df17ee3/try-linux64-br-haz/hazards.txt.gz , they all look hard to trigger. The compartment iterator ones were worse, but I've already backported spot fixes to the worst of those.

Fix Landed on Version: 39

Risk to taking this patch (and alternatives if risky): very slight chance of increased OOMs

String or UUID changes made by this patch: none

See https://wiki.mozilla.org/Release_Management/ESR_Landing_Process for more info.

Approval Request Comment
[Feature/regressing bug #]: This has been a problem for a very long time. Taking a sample problem, the code in the JS Debugger function addAllGlobalsAsDebuggees can be made to iterate out of bounds at least as far back as Fx28, since that code was changed in bug 933882. But the previous code had pretty much the same issue.

[Describe test coverage new/current, TreeHerder]: try push, and it is now on inbound (though it's too early to see if it will bounce.)

[Risks and why]: very slight chance to increase OOMs, though these iterations are rare outside of a GC (and inside of a GC, they can't cause problems.) A logic bug would be more likely, so that the GC does something wrong if it happens during an iteration, but the approach taken seems safe.

[String/UUID change made/needed]: none

Note that the aurora version of this patch is already approved for aurora. I'm not going to land until I see this happy on inbound, though. The backporting sheriff fairy is free to land the appropriate versions.

Comment 28

[Steve Fink [:sfink] [:s:]]

Well, it required a trivial bustage fix for introducing a single-arg constructor, anyway:
https://hg.mozilla.org/integration/mozilla-inbound/rev/25c0b4f618fb

Comment 29

[Steve Fink [:sfink] [:s:]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/16babf5d700c
https://hg.mozilla.org/releases/mozilla-aurora/rev/525829e43c33

Comment 30

[Lawrence Mandel [:lmandel] (use needinfo)]

abillings and I agree that as we're at the end of the 37 cycle and don't need to take this fix in 37 we're going to defer to 38.

Comment 31

[Lawrence Mandel [:lmandel] (use needinfo)]

Comment on attachment 8580354 [details] [diff] [review]
Rollup patch for approvals and backports

ESR approval will be granted once we ship ESR 31.6.0.

Comment 32

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/81de36604bf2
https://hg.mozilla.org/mozilla-central/rev/74aab037e628
https://hg.mozilla.org/mozilla-central/rev/25c0b4f618fb

Comment 33

[Liz Henry (:lizzard) (relman/hg->git project)]

cc-ing bkerensa. He couldn't see this bug and needs to since he's doing release management for ESR.

Comment 34

[Benjamin Kerensa [:bkerensa]]

Lawrence wanted me to check per comment 33 I was unable to see these bugs and will need my privleges adjusted but do we want to respin on these?

Comment 35

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

This is tracking for 38 (see tracking flag). See comment 30 and comment 31 as well.

Comment 36

[Ryan VanderMeulen [:RyanVM]]

FWIW, the beta patch doesn't apply to b2g37 at all. This'll need more rebasing for the remaining backports.

Comment 37

[Steve Fink [:sfink] [:s:]]

Attachment: Backport to b2g37

Here's a rebased backport to b2g37. Let me know what other backports I should do.

Comment 38

[Steve Fink [:sfink] [:s:]]

Attachment: Backport to esr31

This backport was not so clean. Hopefully it will work for similar branches.

Comment 39

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-esr31/rev/bf3bfefc5189

Comment 40

[Ryan VanderMeulen [:RyanVM]]

Backed out from esr31 for bustage.
https://treeherder.mozilla.org/logviewer.html#?job_id=67023&repo=mozilla-esr31

Comment 41

[Ryan VanderMeulen [:RyanVM]]

Also on Windows:
https://treeherder.mozilla.org/logviewer.html#?job_id=67024&repo=mozilla-esr31

Comment 42

[Steve Fink [:sfink] [:s:]]

Attachment: Backport to esr31

Hopefully fixed the Windows build breakage:

https://treeherder.mozilla.org/#/jobs?repo=try&revision=93a3615ffa2d

Comment 43

[Steve Fink [:sfink] [:s:]]

Attachment: Backport to esr31

Moar fixes. (Pull in RyanVM's fix.)

Comment 44

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-esr31/rev/65ba2099c3b2

Comment 45

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-b2g37_v2_2/rev/ffdef091deab
https://hg.mozilla.org/releases/mozilla-b2g34_v2_1/rev/f23f1c45c456
https://hg.mozilla.org/releases/mozilla-b2g32_v2_0/rev/f61d3d2ec9d0
https://hg.mozilla.org/releases/mozilla-b2g30_v1_4/rev/0673402a88ac

Comment 46

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-b2g34_v2_1s/rev/f23f1c45c456
https://hg.mozilla.org/releases/mozilla-b2g32_v2_0m/rev/f61d3d2ec9d0

Comment 47

[[SV Manager] Florin Mezei, QA (:FlorinMezei)]

Is there a need for manually testing this fix? If yes, could you please provide some detailed steps on how to do it?

Comment 48

[Steve Fink [:sfink] [:s:]]

(In reply to Florin Mezei, QA (:FlorinMezei) from comment #47)
> Is there a need for manually testing this fix? If yes, could you please
> provide some detailed steps on how to do it?

No.