Bug 1192401 (CVE-2015-4507)

Crash due to Assertion failure: getSlotRef(EVAL).isUndefined(), at js/src/vm/GlobalObject.h:147

RESOLVED FIXED in Firefox 41

Status

()

defect
RESOLVED FIXED
4 years ago
2 years ago

People

(Reporter: spandan.veggalam, Assigned: fitzgen)

Tracking

({sec-moderate})

42 Branch
mozilla43
Points:
---
Bug Flags:
sec-bounty +
in-testsuite +

Firefox Tracking Flags

(firefox40 wontfix, firefox41 fixed, firefox42 fixed, firefox43 fixed, firefox-esr38 wontfix, b2g-v2.0 wontfix, b2g-v2.0M wontfix, b2g-v2.1 wontfix, b2g-v2.1S wontfix, b2g-v2.2 wontfix, b2g-v2.2r wontfix, b2g-master fixed)

Details

(Whiteboard: [post-critsmash-triage][adv-main41+])

Attachments

(4 attachments)

Reporter

Description

4 years ago
User Agent: Mozilla/5.0 (X11; Ubuntu; Linux x86_64; rv:39.0) Gecko/20100101 Firefox/39.0
Build ID: 20150629114049

Steps to reproduce:

mozilla-central revision 892594bdad30 (build with: --enable-optimize --enable-posix-nspr-emulation --enable-valgrind --enable-gczeal --enable-debug)

const dbg = new Debugger();
const g = evalcx("lazy");
dbg.addDebuggee(g);
dbg.memory.trackingAllocationSites = true;
g.eval("this.alloc = {}");


Actual results:

Assertion failure: getSlotRef(EVAL).isUndefined(), at js/src/vm/GlobalObject.h:147



Expected results:

undefined or some other relevant message
Thanks for the report! CC'ing the debugger gurus.

Nick, I'll NI you because of the trackingAllocationSites bits, please forward if that's unrelated.
Flags: needinfo?(nfitzgerald)
Posted file full backtrace
Flags: needinfo?(nfitzgerald)
bt 10

> #0  js::GlobalObject::setOriginalEval (this=0x10657c060, evalobj=0x106588380) at GlobalObject.h:148
> #1  0x000000010011e954 in FinishObjectClassInit (cx=0x103221000, ctor={<js::HandleBase<JSObject *>> = {<No data fields>}, ptr = 0x7fff5fbfab58}, proto={<js::HandleBase<JSObject *>> = {<No data fields>}, ptr = 0x7fff5fbfab98}) at Object.cpp:1097
> #2  0x0000000100301695 in js::GlobalObject::resolveConstructor (cx=0x103221000, global={<js::HandleBase<js::GlobalObject *>> = {<No data fields>}, ptr = 0x7fff5fbfad10}, key=JSProto_Object) at GlobalObject.cpp:206
> #3  0x0000000100300df8 in js::GlobalObject::ensureConstructor (cx=0x103221000, global={<js::HandleBase<js::GlobalObject *>> = {<No data fields>}, ptr = 0x7fff5fbfad10}, key=JSProto_Object) at GlobalObject.cpp:99
> #4  0x0000000100a1ba1d in JS_ResolveStandardClass (cx=0x103221000, obj={<js::HandleBase<JSObject *>> = {<No data fields>}, ptr = 0x7fff5fbfb158}, id={<js::HandleBase<jsid>> = {<No data fields>}, ptr = 0x7fff5fbfb738}, resolved=0x7fff5fbfae9b) at jsapi.cpp:1314
> #5  0x000000010001b56e in sandbox_resolve (cx=0x103221000, obj={<js::HandleBase<JSObject *>> = {<No data fields>}, ptr = 0x7fff5fbfb158}, id={<js::HandleBase<jsid>> = {<No data fields>}, ptr = 0x7fff5fbfb738}, resolvedp=0x7fff5fbfae9b) at js.cpp:2381
> #6  0x000000010033e8ac in js::CallResolveOp (cx=0x103221000, obj={<js::HandleBase<js::NativeObject *>> = {<No data fields>}, ptr = 0x7fff5fbfb158}, id={<js::HandleBase<jsid>> = {<No data fields>}, ptr = 0x7fff5fbfb738}, propp={<js::MutableHandleBase<js::Shape *>> = {<No data fields>}, ptr = 0x7fff5fbfb130}, recursedp=0x7fff5fbfaf8f) at NativeObject-inl.h:388
> #7  0x00000001003368ef in js::LookupOwnPropertyInline<(js::AllowGC)1> (cx=0x103221000, obj={<js::HandleBase<js::NativeObject *>> = {<No data fields>}, ptr = 0x7fff5fbfb158}, id={<js::HandleBase<jsid>> = {<No data fields>}, ptr = 0x7fff5fbfb738}, propp={<js::MutableHandleBase<js::Shape *>> = {<No data fields>}, ptr = 0x7fff5fbfb130}, donep=0x7fff5fbfb117) at NativeObject-inl.h:477
> #8  0x000000010033a669 in NativeGetPropertyInline (cx=0x103221000, obj={<js::HandleBase<js::NativeObject *>> = {<No data fields>}, ptr = 0x7fff5fbfb308}, receiver={<js::HandleBase<JSObject *>> = {<No data fields>}, ptr = 0x7fff5fbfb418}, id={<js::HandleBase<jsid>> = {<No data fields>}, ptr = 0x7fff5fbfb738}, nameLookup=NotNameLookup, vp={<js::MutableHandleBase<JS::Value>> = {<js::MutableValueOperations<JS::MutableHandle<JS::Value> >> = {<js::ValueOperations<JS::MutableHandle<JS::Value> >> = {<No data fields>}, <No data fields>}, <No data fields>}, ptr = 0x1067100b0}) at NativeObject.cpp:1910
> #9  0x000000010033a57a in js::NativeGetProperty (cx=0x103221000, obj={<js::HandleBase<js::NativeObject *>> = {<No data fields>}, ptr = 0x7fff5fbfb308}, receiver={<js::HandleBase<JSObject *>> = {<No data fields>}, ptr = 0x7fff5fbfb418}, id={<js::HandleBase<jsid>> = {<No data fields>}, ptr = 0x7fff5fbfb738}, vp={<js::MutableHandleBase<JS::Value>> = {<js::MutableValueOperations<JS::MutableHandle<JS::Value> >> = {<js::ValueOperations<JS::MutableHandle<JS::Value> >> = {<No data fields>}, <No data fields>}, <No data fields>}, ptr = 0x1067100b0}) at NativeObject.cpp:1954
> (More stack frames follow...)
I'm not really sure what is going on here as I don't have a good grasp of how lazy standard classes work. Somewhat obvious, given the test case and backtrace, is that there are issues with tracking allocations and then instantiating lazy standard classes.

Digging in some more...
Status: UNCONFIRMED → NEW
Ever confirmed: true
Assignee: nobody → nfitzgerald
Status: NEW → ASSIGNED
Duplicate of this bug: 1188342
Marking as sec-moderate because it requires the debugger running some very specific steps.
Keywords: sec-moderate

Comment 9

4 years ago
Comment on attachment 8646503 [details] [diff] [review]
Do not capture SavedFrame stacks before Object.prototype has been initialized

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

I suppose this code ensures that SavedFrame.prototype is instantiated?
Attachment #8646503 - Flags: review?(shu) → review+
(In reply to Shu-yu Guo [:shu] from comment #9)
> I suppose this code ensures that SavedFrame.prototype is instantiated?

Yes: https://dxr.mozilla.org/mozilla-central/rev/d4f3a8a75577e4af2914a4e899ca2e724f9715c4/js/src/vm/SavedStacks.cpp#1189
https://hg.mozilla.org/integration/mozilla-inbound/rev/6f05420e460d

How far back does this go?
Flags: needinfo?(nfitzgerald)
Flags: in-testsuite+
Keywords: checkin-needed
Looking at the source in each of aurora/beta/release/esr38, I think all branches are affected.
Flags: needinfo?(nfitzgerald)
Comment on attachment 8646503 [details] [diff] [review]
Do not capture SavedFrame stacks before Object.prototype has been initialized

This existing patch should still apply on aurora.

Approval Request Comment
[Feature/regressing bug #]: js::SavedStacks / Debugger API allocation site tracking
[User impact if declined]: possible crashes when using the Debugger API
[Describe test coverage new/current, TreeHerder]: Fairly extensive test coverage, plus new testcase that crashes without the rest of the patch
[Risks and why]: Very little. Only pref'd off developer facing features use this part of the Debugger API at the moment.
[String/UUID change made/needed]: None
Attachment #8646503 - Flags: approval-mozilla-aurora?
Posted patch beta.patchSplinter Review
Patch for beta. See comment 14.
Attachment #8648121 - Flags: approval-mozilla-beta?
Do we want to uplift this to release/esr? How do we decide what should or should not be uplifted that far?
Beta is probably far enough. We typically don't take sec-moderates on esr38 without a really good reason to do so. And for release it'd basically have to be chemspill-worthy to take. Thanks for the quick reply and rebase!
https://hg.mozilla.org/mozilla-central/rev/6f05420e460d
Status: ASSIGNED → RESOLVED
Last Resolved: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla43
Reporter

Comment 19

4 years ago
Thanks for your consideration and fixation.
Comment on attachment 8648121 [details] [diff] [review]
beta.patch

Fix a crash, taking it.
Attachment #8648121 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
Attachment #8646503 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Kind of baffled why that is reported as failing.

Unless I am completely misunderstanding, I don't think it is timing out as it says "timed-out: false". It seems to complain about "InternalError: too much recursion" and the exit 3 is what I remember being used for uncaught exceptions. But failing because of too much recursion is *also* strange because the top of the test in question has "|jit-test| allow-oom; allow-overrecursed" and I would think "allow-overrecursed" would allow this test to pass in its current state.

Is there a practical difference between "|jit-test| allow-overrecursed" and "|jit-test| error: InternalError: too much recursion"? I see some tests using one and some using the other.

On top of all that, I'm also baffled as to how this change would affect that test case.

Anyways, I'm not sure what the immediate action should be. Should we back this out from Beta?
Flags: needinfo?(nfitzgerald)
Maybe shu can weigh in on that?
(In reply to Nick Fitzgerald [:fitzgen][:nf] from comment #23)
> But failing because of too much recursion is *also* strange
> because the top of the test in question has "|jit-test| allow-oom;
> allow-overrecursed" and I would think "allow-overrecursed" would allow this
> test to pass in its current state.

I added that allow-overrecursed last week so it's not on beta:

https://hg.mozilla.org/integration/mozilla-inbound/rev/45abee091485

Maybe I should have filed a bug for that, but it was a followup to unbreak the tree and I didn't expect this to start happening on other branches out of the blue...

> Is there a practical difference between "|jit-test| allow-overrecursed" and
> "|jit-test| error: InternalError: too much recursion"? I see some tests
> using one and some using the other.

I think the former means overrecursion is allowed but the test will pass if we don't overrecurse. In the latter case, we require an overrecursion error.

Updated

4 years ago
Group: core-security → core-security-release
Whiteboard: [post-critsmash-triage]
Whiteboard: [post-critsmash-triage] → [post-critsmash-triage][adv-main41+]
Alias: CVE-2015-4507
Flags: sec-bounty?
Flags: sec-bounty? → sec-bounty+
Group: core-security-release
You need to log in before you can comment on or make changes to this bug.