Closed Bug 1652391 Opened 4 years ago Closed 4 years ago

Crash [@ js::gc::TenuredCell::zoneFromAnyThread] or Assertion failure: WeakMapBase::checkMarkingForZone(zone), at gc/GC.cpp:4852 with Debugger

Categories

(Core :: JavaScript: GC, defect, P1)

All
Linux
defect

Tracking

()

RESOLVED DUPLICATE of bug 1647747
Tracking Status
firefox79 --- fixed
firefox80 --- fixed

People

(Reporter: gkw, Unassigned)

References

(Blocks 1 open bug)

Details

(Keywords: reporter-external, sec-low, testcase)

Crash Data

Attachments

(2 files)

try {
  // Mixed in from: js/src/jit-test/tests/debug/gc-05.js
  let x = newGlobal();
  let y = newGlobal();
  x.x = y.eval("x = {}");
  Debugger(x).onDebuggerStatement = function (m) {
    z = m.eval("x").return;
  }
  x.eval("debugger");
  y.eval("x.prop = []");
  z.getOwnPropertyDescriptor("prop");
  y.runOffThreadScript();
} catch (e) {};
gcslice(9999);

Compiled using GCC 9.3.0 and Clang 9 with:

AR=ar sh ./configure --enable-debug --with-ccache --enable-gczeal --enable-debug-symbols --disable-tests

Run with:

--fuzzing-safe --no-threads --no-baseline --no-ion --more-compartments

Tested on m-c rev 22f5f7e91444.

Not sure how the sec folks will rate this as this seems to involve Debugger, but again deferring to more knowledgeable people.

Flags: sec-bounty?
Summary: Crash [@ js::gc::TenuredCell::zoneFromAnyThread] or Assertion failure: WeakMapBase::checkMarkingForZone(zone), at gc/GC.cpp:4852 → Crash [@ js::gc::TenuredCell::zoneFromAnyThread] or Assertion failure: WeakMapBase::checkMarkingForZone(zone), at gc/GC.cpp:4852 with Debugger

D'oh, seems to be fixed by the patch in bug 1647747, but here's another testcase for you, :sfink, and please land the patch.

Probably not sec-anything either, as per bug 1647747 comment 10.

(In reply to Gary Kwong [:gkw] [:nth10sd] (NOT official MoCo now) from comment #5)

Probably not sec-anything either, as per bug 1647747 comment 10.

... unless I'm wrong due to the ASan stack here?

Group: core-security → javascript-core-security
See Also: → 1647747

Need-info Steve, based on comment 5.
Steve, feel free to raise the severity accordingly.

Severity: -- → S4
Component: JavaScript Engine → JavaScript: GC
Flags: needinfo?(sphink)
Priority: -- → P1

This was fixed as part of bug 1647747, though I think it may reveal that one of the problems actually showed up earlier than I thought.

The timeline of events:

36f0316d3c4e (Fx78) - incremental weakmap landing. No bug yet.
04311d1cf723 (Fx79) - stop collecting the nursery every sweep slice. I believe this may have introduced the bug here.
22f5f7e91444 (Fx79) - the version tested in this bug
8190007924dd (Fx80) - bug 1647747 fix.

The symptom shown in comment 2 makes this part of the bug more serious than what I was seeing in bug 1647747 comment 10. Here, we're reading some random value out of the nursery as if it were the tenured heap, and then using that to make a decision about what to mark. It would be very difficult to turn this into a UAF, but it's theoretically possible. It also appears to require the use of Debugger.

Flags: needinfo?(sphink)

The result of the previous comment is that I think the fix in bug 1647747 should be backported to 79 after all. (The specific part that is needed is in the attachment labeled "Followups to DebuggerWeakMap changes", but it can't be landed separately and is already rolled into the other phabricator revision.)

(In reply to Steve Fink [:sfink] [:s:] from comment #9)

The result of the previous comment is that I think the fix in bug 1647747 should be backported to 79 after all.

Am I right to say that if not for this bug, there would have been no intention of backporting this to Fx79?

Bug 1647747 comment 10 seems to suggest this, if I'm reading it correctly, reproduced below for context:

(In reply to Steve Fink [:sfink] [:s:] from bug 1647747 comment #10)

(In reply to Ryan VanderMeulen [:RyanVM] from bug 1647747 comment #9)

Based on the severity of the bug, I assume this fix can ride the trains and won't require uplift. Feel free to scream if that's incorrect though.

Yes, that seems ok. It looks scarily close to a UAF (as marking bugs generally are), but actually this would just result in a key being dropped from a weakmap so that it wouldn't be there if you looked it up again. Correctness bug, but not a security problem. In this case, it's a DebuggerWeakMap, and I'm not sure exactly what effect that would have. Perhaps just the loss of expando-like properties on the DebuggerObject?

I'm trying to build a value proposition for some sort of sec-bounty value for this particular bug. If I didn't file this, Fx79 would not have had the patch uplifted judging by Steve's comment above. Thus, filing this bug did prove the need for a patch uplift, and I feel there is some value shown from that.

Is this putting users at risk from web content if it requires the Debugger object? Even if you can get your malicious page debugged you can't directly influence the arguments. Guessing sec-low at best.

Keywords: sec-low

It does require Debugger, since they are the only weakmaps whose keys are not same-zone with the map.

Looking at the case where you rely on the user to debug the page, you don't need to directly control the arguments. You only need them to be nursery-allocated, and therefore young. It wouldn't be too hard to trigger and have incorrect behavior of some sort. But to do anything at all with it, even just to get a crash, you would need to control the contents of the heap pretty precisely. It would be very difficult. I agree with sec-low. Especially since it's not enough to just debug the page; you'd probably need a breakpoint to be hit or something like that.

Status: NEW → RESOLVED
Closed: 4 years ago
Resolution: --- → DUPLICATE

Bounty-minus because it's low severity.

Flags: sec-bounty? → sec-bounty-
Group: javascript-core-security → core-security-release
Group: core-security-release
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: