Closed Bug 1406398 Opened 7 years ago Closed 7 years ago

Assertion failure: MOZ_ASSERT(isNative()) in in js::NativeObject::lookup

Categories

(Core :: JavaScript Engine, defect, P1)

Product:
Core
Component:
JavaScript Engine
Type:
defect

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla58
Tracking Flags:
Tracking Status
firefox-esr52 57+ fixed
firefox56 --- wontfix
firefox57 + fixed
firefox58 + fixed

People

(Reporter: anba, Assigned: anba)

References

Details

(Keywords: regression, sec-high, Whiteboard: [adv-main57+][adv-esr52.5+])

Attachments

(3 files)

bug1406398.patch
(deleted), patch
jandem
: review+
jandem
: feedback+
lizzard
: approval-mozilla-beta+
abillings
: sec-approval+
Details | Diff | Splinter Review
bug1406398-testcase.patch
(deleted), patch
jandem
: review+
Details | Diff | Splinter Review
bug1406398-esr52.patch
(deleted), patch
anba
: review+
lizzard
: approval-mozilla-esr52+
Details | Diff | Splinter Review
Test case: <html> <body> <iframe id="myiframe"></iframe> <script> var d = document.createElement("input"); Object.defineProperties(d, { p: { get() { myiframe.contentDocument.adoptNode(d); }, enumerable: true }, q: { enumerable: true } }); Object.values(d); </script> </body> </html> Stack trace: --- Thread 1 "firefox" received signal SIGSEGV, Segmentation fault. 0x00007fffe9b01118 in js::NativeObject::lookup (this=<optimised out>, cx=<optimised out>, id=...) at /home/andre/hg/mozilla-inbound/js/src/vm/NativeObject.cpp:260 260 MOZ_ASSERT(isNative()); (gdb) bt #0 0x00007fffe9b01118 in js::NativeObject::lookup(JSContext*, jsid) (this=<optimised out>, cx=<optimised out>, id=...) at /home/andre/hg/mozilla-inbound/js/src/vm/NativeObject.cpp:260 #1 0x00007fffe94ffb2f in EnumerableOwnProperties(JSContext*, JS::CallArgs const&, EnumerableOwnPropertiesKind) (cx=cx@entry=0x7fffdee5d000, args=..., kind=kind@entry=Values) at /home/andre/hg/mozilla-inbound/js/src/builtin/Object.cpp:1271 #2 0x00007fffe95003ce in obj_values(JSContext*, unsigned int, JS::Value*) (cx=0x7fffdee5d000, argc=<optimised out>, vp=<optimised out>) at /home/andre/hg/mozilla-inbound/js/src/builtin/Object.cpp:1331 #3 0x00007fffe9493981 in js::CallJSNative(JSContext*, bool (*)(JSContext*, unsigned int, JS::Value*), JS::CallArgs const&) (cx=0x7fffdee5d000, native=0x7fffe9500390 <obj_values(JSContext*, unsigned int, JS::Value*)>, args=...) at /home/andre/hg/mozilla-inbound/js/src/jscntxtinlines.h:293 #4 0x00007fffe948828f in js::InternalCallOrConstruct(JSContext*, JS::CallArgs const&, js::MaybeConstruct) (cx=cx@entry=0x7fffdee5d000, args=..., construct=construct@entry=js::NO_CONSTRUCT) at /home/andre/hg/mozilla-inbound/js/src/vm/Interpreter.cpp:495 #5 0x00007fffe948872d in InternalCall(JSContext*, js::AnyInvokeArgs const&) (cx=0x7fffdee5d000, args=...) at /home/andre/hg/mozilla-inbound/js/src/vm/Interpreter.cpp:540 #6 0x00007fffe947a85c in Interpret(JSContext*, js::RunState&) (args=..., cx=<optimised out>) at /home/andre/hg/mozilla-inbound/js/src/vm/Interpreter.cpp:546 #7 0x00007fffe947a85c in Interpret(JSContext*, js::RunState&) (cx=0x7fffdee5d000, state=...) at /home/andre/hg/mozilla-inbound/js/src/vm/Interpreter.cpp:3085 #8 0x00007fffe9487d0c in js::RunScript(JSContext*, js::RunState&) (cx=0x7fffdee5d000, state=...) at /home/andre/hg/mozilla-inbound/js/src/vm/Interpreter.cpp:435 .... --- Regressed by: bug 1232639 (Firefox 47)
Hat tip to :jandem for his comment in TryAssignNative which helped to discover this bug: http://searchfox.org/mozilla-central/rev/8efd128b48cdae3a6c6862795ce618aa1ca1c0b4/js/src/builtin/Object.cpp#761-763 :-)
Group: core-security → javascript-core-security
Priority: -- → P1
Attached patch bug1406398.patch (deleted) — — Splinter Review
I guess this change should be enough to fix the bug, but I still need to build the browser to verify it. Open questions: - I'm not sure about the security implications of this bug. I couldn't easily crash Firefox using simple test cases, but that probably doesn't mean anything. Maybe you can give me some pointers how to rate this issue? - I don't know where we place test cases for bugs like this one. Do they belong into js/xpconnect/crashtests? And I've picked a more or less nonsense commit message, so it's not too obvious that object transplanting is the actual issue here. I hope that's okay with you. :-)
Assignee: nobody → andrebargull
Status: NEW → ASSIGNED
Attachment #8917899 - Flags: feedback?(jdemooij)
needinfo jandem about the security implications here.
Blocks: 1232639
Flags: needinfo?(jdemooij)
Keywords: regression
Attachment #8917899 - Flags: feedback?(jdemooij) → feedback+
(In reply to André Bargull [:anba] from comment #4) > - I don't know where we place test cases for bugs like this one. Do they > belong into js/xpconnect/crashtests? That makes sense I think. Ideally we would add a shell function for this, see bug 1403679. Are you interested in trying that? We could make it fuzzing-unsafe for now... > And I've picked a more or less nonsense commit message, so it's not too > obvious that object transplanting is the actual issue here. I hope that's > okay with you. :-) Yes I would have done the same :)
(In reply to Daniel Veditz [:dveditz] from comment #5) > needinfo jandem about the security implications here. It's a sec-high probably - we get confused about the object layout and will treat a non-native object as a native object. I'd have to look at all memory accesses here to see if that's exploitable but best to assume it is.
Flags: needinfo?(jdemooij)
Keywords: sec-high
(In reply to Jan de Mooij [:jandem] from comment #7) > (In reply to André Bargull [:anba] from comment #4) > > - I don't know where we place test cases for bugs like this one. Do they > > belong into js/xpconnect/crashtests? > > That makes sense I think. Ideally we would add a shell function for this, > see bug 1403679. Are you interested in trying that? We could make it > fuzzing-unsafe for now... Yes, we should look into that. I think for starters we could probably even limit the supported set of objects for a shell testing function, maybe just PlainObject? (There also seems to be some kind of limitation for swap itself based on [1], but I don't know if that list is still up to date.) [1] http://searchfox.org/mozilla-central/rev/a984558fa2bbde6492d3fb918496fc0b0835b2ce/js/src/jsobj.cpp#1668-1674
tracking as sec-high
tracking-firefox57: ? → +
tracking-firefox58: ? → +
Comment on attachment 8917899 [details] [diff] [review] bug1406398.patch I've verified the patch works correctly using the new testing function from bug 1403679.
Attachment #8917899 - Flags: review?(jdemooij)
Attached patch bug1406398-testcase.patch (deleted) — — Splinter Review
Test case using the testing function from bug 1403679, to be landed after the bug is fixed and the fix is in release.
Attachment #8920537 - Flags: review?(jdemooij)
Attachment #8917899 - Flags: review?(jdemooij) → review+
Comment on attachment 8920537 [details] [diff] [review] bug1406398-testcase.patch Review of attachment 8920537 [details] [diff] [review]: ----------------------------------------------------------------- Thanks for adding the testing function!
Attachment #8920537 - Flags: review?(jdemooij) → review+
Comment on attachment 8917899 [details] [diff] [review] bug1406398.patch [Security approval request comment] How easily could an exploit be constructed based on the patch? - Probably not too easily, because an attacker would first know how to transplant objects (JS_TransplantObject), but it's probably also not too hard, given that the patch only changed a handful of lines. Do comments in the patch, the check-in comment, or tests included in the patch paint a bulls-eye on the security problem? - No. (No inline comments added, the check-in comment tries to draw the attention into a different direction, and the test case is in a separate patch.) Which older supported branches are affected by this flaw? - All supported branches (the bug was introduced in Firefox 47). If not all supported branches, which bug introduced the flaw? - See above. Do you have backports for the affected branches? If not, how different, hard to create, and risky will they be? - If Mercurial was less finicky about unrelated changes a few lines later, it would be possible to apply the patch as is to the older branches. But unfortunately that's not the case, so we need to prepare a separate patch for ESR52. How likely is this patch to cause regressions; how much testing does it need? - Unlikely, because it only moves an object check. No additional or manual testing is needed.
Attachment #8917899 - Flags: sec-approval?
Flags: needinfo?(rkothari)
Flags: needinfo?(lhenry)
Release management, this seems pretty safe to take on 57 (as well as 58 and ESR52) but I want to ask you before I give sec-approval on it given the release timeline.
Yes, that's fine, this can make it into tomorrow's beta 12 build if we approve it today.
Flags: needinfo?(lhenry) → needinfo?(abillings)
Comment on attachment 8917899 [details] [diff] [review] bug1406398.patch sec-approval+ for trunk. Please nominate other patches as requested as well so we can make tomorrow's beta.
Flags: needinfo?(abillings)
Attachment #8917899 - Flags: sec-approval? → sec-approval+
ni :anba for uplift requests
Flags: needinfo?(andrebargull)
Attached patch bug1406398-esr52.patch (deleted) — — Splinter Review
Rebased patch for ESR52. No change in behaviour, therefore carrying r+ from :jandem.
Attachment #8922277 - Flags: review+
Comment on attachment 8917899 [details] [diff] [review] bug1406398.patch Approval Request Comment [Feature/Bug causing the regression]: - bug 1232639 [User impact if declined]: - Crash, maybe exploitable (see comment #8). [Is this code covered by automated tests?]: - Yes. [Has the fix been verified in Nightly?]: - Yes. [Needs manual test from QE? If yes, steps to reproduce]: - No. [List of other uplifts needed for the feature/fix]: - None. [Is the change risky?]: - No. [Why is the change risky/not risky?]: - It only moves an object type check to ensure the type check occurs in every loop iteration instead of just once before entering the loop. [String changes made/needed]: - None.
Flags: needinfo?(andrebargull)
Attachment #8917899 - Flags: approval-mozilla-beta?
Comment on attachment 8922277 [details] [diff] [review] bug1406398-esr52.patch [Approval Request Comment] If this is not a sec:{high,crit} bug, please state case for ESR consideration: - It's a sec-high. User impact if declined: - Crash, maybe exploitable (see comment #8). Fix Landed on Version: - Comment #20 Risk to taking this patch (and alternatives if risky): - See comment #20 for risks; No alternatives known. String or UUID changes made by this patch: - None. See https://wiki.mozilla.org/Release_Management/ESR_Landing_Process for more info.
Attachment #8922277 - Flags: approval-mozilla-esr52?
Comment on attachment 8917899 [details] [diff] [review] bug1406398.patch Let's uplift for the beta 12 build.
Attachment #8917899 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
Comment on attachment 8922277 [details] [diff] [review] bug1406398-esr52.patch Fix for sec-high issue, let's take it for ESR.
Attachment #8922277 - Flags: approval-mozilla-esr52? → approval-mozilla-esr52+
https://hg.mozilla.org/mozilla-central/rev/c2cecb6d6f9b
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
status-firefox58: affected → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla58
Group: javascript-core-security → core-security-release
(In reply to André Bargull [:anba] from comment #21) > [Is this code covered by automated tests?]: > - Yes. > > [Has the fix been verified in Nightly?]: > - Yes. > > [Needs manual test from QE? If yes, steps to reproduce]: > - No. Setting qe-verify- based on André's assessment on manual testing needs and the fact that this fix has automated coverage.
Flags: qe-verify-
tracking-firefox-esr52: ? → 57+
Whiteboard: [adv-main57+][adv-esr52.5+]
Group: core-security-release
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: