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

Status: VERIFIED FIXED

(Core :: JavaScript Engine, defect, P1)

Description

[Christian Holler (:decoder)]

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() {
  eval(`
   try { 
    (function() {
      for (a of [{}, { __iterator__: function() {} }])
        for (b in a)
          return {p:1, q:2, r:3, s:4, t:5};
    })();
   } catch(ex) {}
  `);
});


Backtrace:

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.

Comment 1

[Fuzzing Team]

JSBugMon: Bisection requested, result:
autoBisect shows this is probably related to the following changeset:

The first bad revision is:
changeset:   https://hg.mozilla.org/mozilla-central/rev/24297940e626
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.

Comment 2

[Masatoshi Kimura [:emk]]

Sorry, I have no idea why removing __iterator__ support breaks things...

Comment 3

[Daniel Veditz [:dveditz]]

Luke: who should investigate this bug? You reviewed the potential regressor

Comment 4

[Luke Wagner [:luke]]

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.

Comment 5

[Jan de Mooij [:jandem]]

Attachment: Patch

Tricky OOM bug. We have something like this:

  for (a of array) {
      for (b in a) {
          return;
      }
  }

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...

Comment 6

[Jan de Mooij [:jandem]]

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.

Comment 7

[Alex Gaynor [:Alex_Gaynor]]

FWIW, on point (1) there's been some interesting research on exploitation in OOM situations: https://pacsec.jp/psj17/PSJ2017_Yuki_Chen_From_Out_of_Memory_to_Remote_Code_Execution_PacSec2017_final.pdf

Comment 8

[Tooru Fujisawa [:arai]]

Comment on attachment 8976914 [details] [diff] [review]
Patch

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

Thanks!

Comment 9

[Jan de Mooij [:jandem]]

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 10

[Jan de Mooij [:jandem]]

Comment on attachment 8976914 [details] [diff] [review]
Patch

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?
No.

> 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.

Comment 11

[Jan de Mooij [:jandem]]

Ah ESR 52 is likely unaffected (it's already marked as such) because bug 1147371 is in 53+.

Comment 12

[Al Billings [:abillings - ex-MoCo]]

Sec-approval+ for trunk. Please nominate a patch for Beta and ESR60 after it has been on trunk for a few days.

Comment 13

[Jan de Mooij [:jandem]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/7da8ac949eb4ca3b87013ba624ccd1c0e59f9ebf

I'll request beta/ESR approval later.

Comment 14

[Fuzzing Team]

JSBugMon: The testcase found in this bug no longer reproduces (tried revision 6b9076ac236c).

Comment 15

[Jan de Mooij [:jandem]]

For some reason the bug wasn't updated.

https://hg.mozilla.org/mozilla-central/rev/7da8ac949eb4ca3b87013ba624ccd1c0e59f9ebf

Comment 16

[Jan de Mooij [:jandem]]

Comment on attachment 8976914 [details] [diff] [review]
Patch

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.

Comment 17

[Fuzzing Team]

JSBugMon: This bug has been automatically verified fixed.

Comment 18

[Ryan VanderMeulen [:RyanVM]]

Comment on attachment 8976914 [details] [diff] [review]
Patch

Fixes a longstanding sec bug with Iterators. Approved for 61.0b10 and ESR 60.1.

Comment 19

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-beta/rev/7e502b527f07
https://hg.mozilla.org/releases/mozilla-esr60/rev/7f9fbb6a71d2

Comment 20

[Fuzzing Team]

JSBugMon: This bug has been automatically verified fixed on Fx61