Closed Bug 1406398 Opened 3 years ago Closed 3 years ago

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

Categories

(Core :: JavaScript Engine, defect, P1)

defect

Tracking

()

RESOLVED FIXED
mozilla58
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)

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.patchSplinter 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
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)
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)
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: 3 years ago
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-
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.