Crash [@ js::gc::TenuredCell::zoneFromAnyThread] or Assertion failure: WeakMapBase::checkMarkingForZone(zone), at gc/GC.cpp:4852 with Debugger
Categories
(Core :: JavaScript: GC, defect, P1)
Tracking
()
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.
Reporter | ||
Comment 1•4 years ago
|
||
Reporter | ||
Comment 2•4 years ago
|
||
Comment hidden (obsolete) |
Comment hidden (obsolete) |
Reporter | ||
Updated•4 years ago
|
Reporter | ||
Comment 5•4 years ago
|
||
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.
Reporter | ||
Comment 6•4 years ago
|
||
(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?
Comment 7•4 years ago
|
||
Need-info Steve, based on comment 5.
Steve, feel free to raise the severity accordingly.
Comment 8•4 years ago
•
|
||
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.
Comment 9•4 years ago
|
||
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.)
Reporter | ||
Comment 10•4 years ago
|
||
(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.
Comment 11•4 years ago
|
||
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.
Comment 12•4 years ago
|
||
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.
Updated•4 years ago
|
Updated•4 years ago
|
Reporter | ||
Updated•7 months ago
|
Updated•5 months ago
|
Updated•5 months ago
|
Description
•