Closed Bug 1461324 Opened 3 years ago Closed 3 years ago

Assertion failure: isObject(), at dist/include/js/Value.h:749 with OOM and __iterator__


(Core :: JavaScript Engine, defect, P1)




Tracking Status
firefox-esr52 --- unaffected
firefox-esr60 61+ fixed
firefox60 --- wontfix
firefox61 + verified
firefox62 + verified


(Reporter: decoder, Assigned: jandem)


(Blocks 1 open bug)


(5 keywords, Whiteboard: [jsbugmon:update,ignore][adv-main61+][adv-esr60.1+])


(1 file)

The following testcase crashes on mozilla-central revision a7461494a7a0 (build with --enable-posix-nspr-emulation --enable-valgrind --enable-gczeal --disable-tests --disable-profiling --enable-debug --without-intl-api --enable-optimize --target=i686-pc-linux-gnu, run with --fuzzing-safe --cpu-count=2 --baseline-eager):

oomTest(function() {
   try { 
    (function() {
      for (a of [{}, { __iterator__: function() {} }])
        for (b in a)
          return {p:1, q:2, r:3, s:4, t:5};
   } catch(ex) {}


received signal SIGSEGV, Segmentation fault.
#0  0x080b4d8e in JS::Value::toObject (this=0xffff9fa8) at dist/include/js/Value.h:749
#1  0x08401bf1 in js::jit::ProcessTryNotesBaseline (pc=0xffff9f5c, rfe=0xffffa264, ei=..., frame=..., cx=<optimized out>) at js/src/jit/JitFrames.cpp:432
#2  js::jit::HandleExceptionBaseline (pc=<optimized out>, rfe=0xffffa264, frame=..., cx=<optimized out>) at js/src/jit/JitFrames.cpp:524
#3  js::jit::HandleException (rfe=<optimized out>) at js/src/jit/JitFrames.cpp:710
#4  0x230e9703 in ?? ()
#5  0x230dbb85 in ?? ()
#6  0x083f7c2d in EnterJit (cx=<optimized out>, cx@entry=0xf6e17800, state=..., code=0x23c81120 "\351\033") at js/src/jit/Jit.cpp:101
#7  0x083f887f in js::jit::MaybeEnterJit (cx=0xf6e17800, state=...) at js/src/jit/Jit.cpp:163
#8  0x081e456f in js::RunScript (cx=<optimized out>, state=...) at js/src/vm/Interpreter.cpp:402
#9  0x081e4bd5 in js::InternalCallOrConstruct (cx=<optimized out>, args=..., construct=js::NO_CONSTRUCT) at js/src/vm/Interpreter.cpp:489
#10 0x081e4ec0 in InternalCall (cx=cx@entry=0xf6e17800, args=...) at js/src/vm/Interpreter.cpp:516
#11 0x081e503f in js::CallFromStack (cx=0xf6e17800, args=...) at js/src/vm/Interpreter.cpp:522
#12 0x082c3d24 in js::jit::DoCallFallback (cx=<optimized out>, frame=0xffffab08, stub_=0xf54f1010, argc=0, vp=0xffffaad0, res=...) at js/src/jit/BaselineIC.cpp:2382
#13 0x230e869a in ?? ()
#14 0xf54f1010 in ?? ()
#15 0x230dbb85 in ?? ()
#16 0x083f7c2d in EnterJit (cx=<optimized out>, cx@entry=0xf6e17800, state=..., code=0x23c80890 "\351\033") at js/src/jit/Jit.cpp:101
#17 0x083f887f in js::jit::MaybeEnterJit (cx=0xf6e17800, state=...) at js/src/jit/Jit.cpp:163
#18 0x081e456f in js::RunScript (cx=<optimized out>, state=...) at js/src/vm/Interpreter.cpp:402
#19 0x081e6ee9 in js::ExecuteKernel (cx=<optimized out>, script=..., envChainArg=..., newTargetValue=..., evalInFrame=..., result=<optimized out>) at js/src/vm/Interpreter.cpp:700
#20 0x0821d81a in EvalKernel (cx=<optimized out>, v=..., v@entry=..., evalType=evalType@entry=DIRECT_EVAL, caller=..., env=..., pc=0xf65b4275 "{\001", vp=...) at js/src/builtin/Eval.cpp:323
#21 0x0821dec0 in js::DirectEval (cx=<optimized out>, v=..., vp=...) at js/src/builtin/Eval.cpp:433
#22 0x082c436d in js::jit::DoCallFallback (cx=<optimized out>, frame=0xffffb778, stub_=0xf65c8050, argc=1, vp=0xffffb738, res=...) at js/src/jit/BaselineIC.cpp:2366
#23 0x230e869a in ?? ()
#24 0xf65c8050 in ?? ()
#25 0x230dbb85 in ?? ()
#26 0x083f7c2d in EnterJit (cx=<optimized out>, cx@entry=0xf6e17800, state=..., code=0x230fe760 "\351\033") at js/src/jit/Jit.cpp:101
#32 0x086816f2 in JS_CallFunction (cx=<optimized out>, obj=..., fun=..., args=..., rval=...) at js/src/jsapi.cpp:2948
#33 0x084fdaa1 in OOMTest (cx=<optimized out>, argc=<optimized out>, vp=<optimized out>) at js/src/builtin/TestingFunctions.cpp:1774
#38 0x082c3d24 in js::jit::DoCallFallback (cx=<optimized out>, frame=0xffffc2e8, stub_=0xf65b5030, argc=1, vp=0xffffc2a8, res=...) at js/src/jit/BaselineIC.cpp:2382
#39 0x230e869a in ?? ()
#40 0xf65b5030 in ?? ()
#41 0x230dbb85 in ?? ()
#42 0x083f7c2d in EnterJit (cx=<optimized out>, cx@entry=0xf6e17800, state=..., code=0x230ea200 "\351\033") at js/src/jit/Jit.cpp:101
#53 main (argc=<optimized out>, argv=<optimized out>, envp=<optimized out>) at js/src/shell/js.cpp:9294
eax	0x0	0
ebx	0x8ea4000	149569536
ecx	0xf7d9f864	-136710044
edx	0x0	0
esi	0x8ea4000	149569536
edi	0x8401ba2	138419106
ebp	0xffff9ed8	4294942424
esp	0xffff9ed0	4294942416
eip	0x80b4d8e <JS::Value::toObject() const+62>
=> 0x80b4d8e <JS::Value::toObject() const+62>:	movl   $0x0,0x0
   0x80b4d98 <JS::Value::toObject() const+72>:	ud2

Marking this s-s because it only reproduces on 32-bit, it requires JIT and we had vulnerabilities with the same assertion before.
JSBugMon: Bisection requested, result:
autoBisect shows this is probably related to the following changeset:

The first bad revision is:
user:        Masatoshi Kimura
date:        Thu Aug 24 22:17:40 2017 +0900
summary:     Bug 1098412 - Remove __iterator__ implementation. r=luke

This iteration took 277.085 seconds to run.
Whiteboard: [jsbugmon:update,bisect] → [jsbugmon:update]
Sorry, I have no idea why removing __iterator__ support breaks things...
Flags: needinfo?(VYV03354)
Luke: who should investigate this bug? You reviewed the potential regressor
Blocks: 1098412
Flags: needinfo?(luke)
Keywords: sec-high
In theory those patches were pure code removal, so it's probably just the change in meaning of __iterator__ that prevents the bisection from going back further.  Jan says he might be the more familiar with this code.
Flags: needinfo?(luke) → needinfo?(jdemooij)
Flags: needinfo?(jdemooij)
Attached patch PatchSplinter Review
Tricky OOM bug. We have something like this:

  for (a of array) {
      for (b in a) {

What happens when we |return| is:

* We have bytecode to close the for-in iterator.
* We have bytecode to close the for-of iterator. As part of this, we have a JSOP_STRICTNE. In Baseline we push some extra Values on the stack for this.
* We throw OOM under DoCompareFallback in Baseline.
* We enter the exception handler where we then think we still have to close the for-in (because of the extra values on the stack).

I think the simplest fix is to use the existing inForOfIterClose flag and make JSTRY_FOR_IN and JSTRY_DESTRUCTURING_ITERCLOSE no-ops if that's set...
Assignee: nobody → jdemooij
Attachment #8976914 - Flags: review?(arai.unmht)
This will be hard to exploit:

(1) You have to trigger an OOM in a location where it's pretty difficult to do that without oomTest.
(2) The interesting code is guarded by obj->is<PropertyIteratorObject>() so you'd need to know the PropertyIteratorObject Class* pointer and then store it somewhere to set this up.
FWIW, on point (1) there's been some interesting research on exploitation in OOM situations:
Comment on attachment 8976914 [details] [diff] [review]

Review of attachment 8976914 [details] [diff] [review]:

Attachment #8976914 - Flags: review?(arai.unmht) → review+
Priority: -- → P1
It might make sense to leave this as sec-high, even though in practice it probably isn't. Should be easy to backport to 61 and ESR 60; I'd suggest wontfix for ESR 52 at this point.
Comment on attachment 8976914 [details] [diff] [review]

Requesting sec-approval just to be sure, but again this is probably more sec-moderate than sec-high.

[Security approval request comment]
> How easily could an exploit be constructed based on the patch?
Very difficult.

> Do comments in the patch, the check-in comment, or tests included in the patch paint a bulls-eye on the security problem?

> Which older supported branches are affected by this flaw?
Hard to say but probably best to assume all.

> Do you have backports for the affected branches? If not, how different, hard to create, and risky will they be?
Should be easy.

> How likely is this patch to cause regressions; how much testing does it need?
Unlikely but it would be good to have it on Nightly for at least a few days before backporting.
Attachment #8976914 - Flags: sec-approval?
Ah ESR 52 is likely unaffected (it's already marked as such) because bug 1147371 is in 53+.
Sec-approval+ for trunk. Please nominate a patch for Beta and ESR60 after it has been on trunk for a few days.
Attachment #8976914 - Flags: sec-approval? → sec-approval+
Whiteboard: [jsbugmon:update] → [jsbugmon:update,ignore]
JSBugMon: The testcase found in this bug no longer reproduces (tried revision 6b9076ac236c).
For some reason the bug wasn't updated.
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla62
Comment on attachment 8976914 [details] [diff] [review]

Approval Request Comment
[Feature/Bug causing the regression]: Bug 1147371 or a later bug.
[User impact if declined]: Potential security issues.
[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?]: Pretty low risk.
[Why is the change risky/not risky?]: Adds some checks that are similar to checks we already have for try-catch/finally when closing for-of iterators.
[String changes made/needed]: None.
Attachment #8976914 - Flags: approval-mozilla-esr60?
Attachment #8976914 - Flags: approval-mozilla-beta?
Group: javascript-core-security → core-security-release
JSBugMon: This bug has been automatically verified fixed.
Comment on attachment 8976914 [details] [diff] [review]

Fixes a longstanding sec bug with Iterators. Approved for 61.0b10 and ESR 60.1.
Attachment #8976914 - Flags: approval-mozilla-esr60?
Attachment #8976914 - Flags: approval-mozilla-esr60+
Attachment #8976914 - Flags: approval-mozilla-beta?
Attachment #8976914 - Flags: approval-mozilla-beta+
Whiteboard: [jsbugmon:update,ignore] → [jsbugmon:update,ignore][adv-main61+][adv-esr60.1+]
JSBugMon: This bug has been automatically verified fixed on Fx61
Group: core-security-release
You need to log in before you can comment on or make changes to this bug.