1651001 - Assertion failure: !mEntered, at mozilla/Vector.h:470 or Assertion failure: WeakMapBase::checkMarkingForZone(zone), at gc/GC.cpp:4844 or Crash [@ MaybeCheckWeakMapMarking] with recomputeWrappers

Status: VERIFIED FIXED

(Core :: JavaScript Engine, defect, P1)

Description

[Christian Holler (:decoder)]

The following testcase crashes on mozilla-central revision 20200703-b48777a21aab (debug build, run with --fuzzing-safe --no-threads):

try { 
  v2 = (x = []); 
  g1 = newGlobal({ sameZoneAs: x }); 
  enableShellAllocationMetadataBuilder(); 
  g1.Uint8ClampedArray.prototype = b1;
} catch(e) {}
startgc(9);
recomputeWrappers();

Backtrace:

received signal SIGSEGV, Segmentation fault.
#0  0x000055555627f97f in js::GCMarker::severWeakDelegate(JSObject*, JSObject*) ()
#1  0x0000555555b0a6e5 in JS::Compartment::removeWrapper(js::ObjectWrapperMap::Ptr) ()
#2  0x0000555555a967dc in js::RemapWrapper(JSContext*, JSObject*, JSObject*) ()
#3  0x0000555555a97ce8 in js::RecomputeWrappers(JSContext*, js::CompartmentFilter const&, js::CompartmentFilter const&) ()
#4  0x00005555557ecc31 in RecomputeWrappers(JSContext*, unsigned int, JS::Value*) ()
#5  0x0000555555942582 in CallJSNative(JSContext*, bool (*)(JSContext*, unsigned int, JS::Value*), js::CallReason, JS::CallArgs const&) ()
[...]
#16 0x00005555557c1739 in Shell(JSContext*, js::cli::OptionParser*, char**) ()
#17 0x00005555557b9c85 in main ()
rax	0x555556f8f15d	93825019736413
rbx	0x7ffff5a3b148	140737314533704
rcx	0x555558383840	93825040660544
rdx	0x0	0
rsi	0x7ffff7105770	140737338431344
rdi	0x7ffff7104540	140737338426688
rbp	0x7fffffffb5a0	140737488336288
rsp	0x7fffffffb570	140737488336240
r8	0x7ffff7105770	140737338431344
r9	0x7ffff7f9bd40	140737353727296
r10	0x58	88
r11	0x7ffff6dac7a0	140737334921120
r12	0x7ffff5a3b170	140737314533744
r13	0x7ffff602a3b8	140737320756152
r14	0x7ffff5a3b180	140737314533760
r15	0x32b8ce388200	55769315181056
rip	0x55555627f97f <js::GCMarker::severWeakDelegate(JSObject*, JSObject*)+895>
=> 0x55555627f97f <_ZN2js8GCMarker17severWeakDelegateEP8JSObjectS2_+895>:	movl   $0x1d6,0x0
   0x55555627f98a <_ZN2js8GCMarker17severWeakDelegateEP8JSObjectS2_+906>:	callq  0x55555584855e <abort>

Not sure if this is the same bug as bug 1647747 since the assertion is very generic and this one uses recomputeWrappers. Marking s-s until we have investigated if this is a shell-only problem or might affect the browser as well.

Comment 1

[Christian Holler (:decoder)]

Attachment: Testcase

Comment 2

[Jon Coppeard (:jonco)]

Looks like it's related to the incremental weak marking barriers.

Comment 3

[Jason Kratzer [:jkratzer]]

Bugmon Analysis:
Verified bug as reproducible on mozilla-central 20200707034119-dd1766e040a2.
The bug appears to have been introduced in the following build range:
> Start: 063d499e409ffb2862a2fc49f20835dab96d94f7 (20200425151933)
> End: 436fde13601e1558959b083556f44b7367c0b1a5 (20200425153542)
> Pushlog: https://hg.mozilla.org/integration/autoland/pushloghtml?fromchange=063d499e409ffb2862a2fc49f20835dab96d94f7&tochange=436fde13601e1558959b083556f44b7367c0b1a5

Comment 4

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

Yes, this is a case where the map, key, delegate, and value are all in the same zone. (But the key and delegate are cross-compartment from each other, as they have to be.) And the delegate was marked by ExposeGCThingToActiveJS during recomputeWrappers, when it calls RemapWrapper to re-map the key to the same delegate as it already had. Fun! This will be an issue with me assuming the nuking wrappers is a one-way process; I hadn't realized things could be un-nuked.

Comment 5

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

Attachment: Bug 1651001 - post-barrier delegate restoration

Comment 6

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

Minimal testcase:

g1 = newGlobal({ sameZoneAs: this });
enableShellAllocationMetadataBuilder();
g1.Object;
startgc(1);
recomputeWrappers();

Comment 7

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

Comment on attachment 9163887 [details]
Bug 1651001 - post-barrier delegate restoration

Security Approval Request

• How easily could an exploit be constructed based on the patch?: Difficult.

The test included here is pretty well-commented and points in the direction of where the beginning issue is, but it requires a shell-only testing function (or a privileged Cu API). But it's a hard thing to trigger and control. I know it involves changing document.domain, and you'd have to do it during an incremental GC, which together make it really hard because you'd have to get access to objects to look up in a WeakMap both before and after the change. (It may even make it impossible? This change might require changing the domain twice and accessing the objects in between when things are inaccessible. But I'm not at all sure of that.)

But if there is a way to trigger it, you end up with a UAF.

• Do comments in the patch, the check-in comment, or tests included in the patch paint a bulls-eye on the security problem?: Unknown
• Which older supported branches are affected by this flaw?: 78
• If not all supported branches, which bug introduced the flaw?: Bug 1633176
• Do you have backports for the affected branches?: No
• If not, how different, hard to create, and risky will they be?: I think this patch should apply cleanly.
• How likely is this patch to cause regressions; how much testing does it need?: It only changes things when you're following the already problematic path, so it shouldn't disturb regular operation.

Comment 8

[Daniel Veditz [:dveditz]]

If it's that tricky to trigger reliably in a normal web context probably better to not crash land into the 79 release with no betas left

Comment 9

[Daniel Veditz [:dveditz]]

Comment on attachment 9163887 [details]
Bug 1651001 - post-barrier delegate restoration

sec-approval+

Comment 10

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

https://hg.mozilla.org/integration/autoland/rev/4c1fac6f83f5fdbab5f62ed6d68c4904795e6218
https://hg.mozilla.org/mozilla-central/rev/4c1fac6f83f5

Comment 11

[Jason Kratzer [:jkratzer]]

Bugmon Analysis:
Bug marked as FIXED but still reproduces on mozilla-central 20200720214226-2f2cb7c9bcce.

Comment 12

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

(In reply to Jason Kratzer [:jkratzer] from comment #11)

Bugmon Analysis:
Bug marked as FIXED but still reproduces on mozilla-central
20200720214226-2f2cb7c9bcce.

How are you reproducing? I have built with that revision, and I cannot reproduce. I also downloaded the JS shell from taskcluster push [1] job [2] and cannot reproduce with that, using either the original test in comment 0 or my test in comment 6. I am using the flags --fuzzing-safe --no-threads.

[1] https://treeherder.mozilla.org/#/jobs?repo=mozilla-central&selectedTaskRun=OAUPVIYXQ92m1WyoYfpxyQ.0&revision=73ee67c9a90d179ca5b623efd0673294f682fd0d

[2] https://treeherder.mozilla.org/#/jobs?repo=mozilla-central&selectedTaskRun=OAUPVIYXQ92m1WyoYfpxyQ.0

Comment 13

[Jason Kratzer [:jkratzer]]

Steve, I needed to use the testcase in attachment 9161803 [details] in order to reproduce.

I'm using the latest debug build from taskcluster via fuzzfetch (https://github.com/MozillaSecurity/fuzzfetch):
python -m fuzzfetch --debug --target js

Executing the testcase via:

> m-c-20200721160732-debug/dist/bin/js --fuzzing-safe --no-threads testcase.js
Assertion failure: !mEntered, at /builds/worker/workspace/obj-build/dist/include/mozilla/Vector.h:470
Segmentation fault (core dumped)

Comment 14

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

Ah! Thank you. I was using the one from comment 0 only. The test case in the attachment reproduces nicely, all over the place. It may be that there are two different bugs involved here.

Comment 15

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

It's a separate bug, though triggered by pretty much the same circumstances. It's an iterator invalidation problem -- when a key with a delegate is nuked (thus losing its delegate), the delegate is looked up in the gcWeakKeys map and the appropriate key removed from the list (there may be multiple keys in different compartments with the same delegate). But also, the key itself is added to the map, which could cause the map to be resized and rehashed. The lists are stored inline in the table, so this invalidates the list for the delegate that is being processed. Ironically, the symptom here is an assertion meant to catch reentry, which often happens with iterator invalidation, but nothing is being reentered -- it's just that the reentry tracking value (on a different data structure than the one being iterated) is corrupted when the rehash happens.

I'll try to fix this without doing O(n^2) work in the common case. (But that might make the code too complex.) A tomorrow problem.

Comment 16

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

Oh, right. There was a critical piece missing from that description, which was why I didn't think this was a problem originally: I had assumed that the key and delegate would always be in different zones, and gcWeakKeys is per-Zone, so removal from the delegate Zone should not have collided with insertion in the key Zone. But I are dumb; keys and delegates will always be in different compartments, not necessarily different zones.

I could move gcWeakKeys back to the compartment to fix this, but that does regress memory a little. I'll just accumulate these in a Vector and crash on OOM.

Comment 17

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

Attachment: Bug 1651001 - avoid reentry with same-zone weakmap delegates

Comment 18

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

Comment on attachment 9165489 [details]
Bug 1651001 - avoid reentry with same-zone weakmap delegates

Security Approval Request

• How easily could an exploit be constructed based on the patch?: About the same as the other, though possibly a little easier, since this one only requires nuking a wrapper, not remapping it. (I have only reproduced it thus far with the original test case, which remaps.) Then again, simpler scenarios would be even less exploitable.
• 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?: 78
• If not all supported branches, which bug introduced the flaw?: Bug 1633176
• Do you have backports for the affected branches?: No
• If not, how different, hard to create, and risky will they be?: It should backport cleanly.
• How likely is this patch to cause regressions; how much testing does it need?: It's pretty safe. It replaces a nested iteration with a non-nested one, so it's going purely in a safer direction. This does introduce a new source of intentional crashing on OOM, but it would be very unlikely to hit that case in practice. (At a minimum, you would need 10 different same-zone compartments that all wrap the same object, and every compartment would need to use the wrapper in a weakmap. And you'd also need to OOM.)

Comment 19

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

Should I request landing for 78 and 79 even if I personally don't think it's necessary to backport? I think there's very little reason to backport this one if the other isn't backported. It is not the same bug, but the risk and effects are pretty much the same. But perhaps I'm not the one who should be making that decision.

Comment 20

[Daniel Veditz [:dveditz]]

Comment on attachment 9165489 [details]
Bug 1651001 - avoid reentry with same-zone weakmap delegates

sec-approval+

Comment 21

[Daniel Veditz [:dveditz]]

It was too late to land in 79 anyway, but I think we should get these patches onto ESR for the Fx80 release date (78.2)

Comment 22

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

https://hg.mozilla.org/integration/autoland/rev/99ea4da3c229d541d4fc3491253b4e5fe448340e
https://hg.mozilla.org/mozilla-central/rev/99ea4da3c229

Comment 23

[BugBot [:suhaib / :marco/ :calixte]]

As part of a security bug pattern analysis, we are requesting your help with a high level analysis of this bug. It is our hope to develop static analysis (or potentially runtime/dynamic analysis) in the future to identify classes of bugs.

Please visit this google form to reply.

Comment 24

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

Comment on attachment 9163887 [details]
Bug 1651001 - post-barrier delegate restoration

ESR Uplift Approval Request

• If this is not a sec:{high,crit} bug, please state case for ESR consideration: (sec-high)
• User impact if declined: Possibly a very small stability issue, though that hasn't been observed in practice. More importantly, a UAF that I think is difficult to exploit.
• Fix Landed on Version: 81
• Risk to taking this patch: Low
• Why is the change risky/not risky? (and alternatives if risky): It only triggers in the unusual circumstance where it is needed, and it patches up some stale bookkeeping.
• String or UUID changes made by this patch: none

Comment 25

[BugBot [:suhaib / :marco/ :calixte]]

The patch landed in nightly and beta is affected.
:sfink, is this bug important enough to require an uplift?
If not please set status_beta to wontfix.

For more information, please visit auto_nag documentation.

Comment 26

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

(In reply to Release mgmt bot [:sylvestre / :calixte / :marco for bugbug] from comment #25)

The patch landed in nightly and beta is affected.
:sfink, is this bug important enough to require an uplift?
If not please set status_beta to wontfix.

Honestly, I don't really know where beta is at this point. As you can see from the above, I though that this was landing in 80, not 81.

I'll just request landing approval. Unless we're at late beta or something, I think it should go in.

Comment 27

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

Comment on attachment 9165489 [details]
Bug 1651001 - avoid reentry with same-zone weakmap delegates

Beta/Release Uplift Approval Request

• User impact if declined: UAF, so rare crashes and some small possibility of exploit.
• Is this code covered by automated tests?: Yes
• Has the fix been verified in Nightly?: Yes
• Needs manual test from QE?: No
• If yes, steps to reproduce:
• List of other uplifts needed: None
• Risk to taking this patch: Low
• Why is the change risky/not risky? (and alternatives if risky): It only triggers in the unusual circumstance where it is needed, and it patches up some stale bookkeeping.
• String changes made/needed: none

Comment 28

[Julien Cristau [:jcristau]]

Comment on attachment 9163887 [details]
Bug 1651001 - post-barrier delegate restoration

this one landed on central while it was still 80

Comment 29

[Julien Cristau [:jcristau]]

Comment on attachment 9165489 [details]
Bug 1651001 - avoid reentry with same-zone weakmap delegates

approved for 80.0b3

Comment 30

[Julien Cristau [:jcristau]]

https://hg.mozilla.org/releases/mozilla-beta/rev/2e5115ca1880

Comment 31

[Jason Kratzer [:jkratzer]]

Bugmon Analysis:
Verified bug as fixed on rev mozilla-central 20200803094100-84b257d07031.

Comment 32

[Ryan VanderMeulen [:RyanVM][PTO June 24-28]]

Comment on attachment 9163887 [details]
Bug 1651001 - post-barrier delegate restoration

Approved for 78.2esr.

Comment 33

[Ryan VanderMeulen [:RyanVM][PTO June 24-28]]

https://hg.mozilla.org/releases/mozilla-esr78/rev/2b9182c2d8aa
https://hg.mozilla.org/releases/mozilla-esr78/rev/9fd18fc98364