Closed Bug 1459607 Opened 2 years ago Closed 2 years ago

Assertion failure: !v.isString() || v.toString()->isAtom() (Handle non-atomized strings outside IonBuilder.), at js/src/jit/IonBuilder.cpp:13624


(Core :: JavaScript Engine, defect, P1, critical)




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


(Reporter: decoder, Assigned: evilpie)


(Blocks 1 open bug)


(5 keywords, Whiteboard: [jsbugmon:update])


(2 files)

The following testcase crashes on mozilla-central revision fb435df9797a (build with --enable-posix-nspr-emulation --enable-valgrind --enable-gczeal --disable-tests --disable-profiling --enable-debug --enable-optimize, run with --fuzzing-safe --ion-eager --ion-offthread-compile=off):

  var p = Object.freeze([
    - undefined   
    + this


received signal SIGSEGV, Segmentation fault.
0x000000000073db9e in js::jit::IonBuilder::constant (this=0x7ffff5f911c0, v=...) at js/src/jit/IonBuilder.cpp:13623
#0  0x000000000073db9e in js::jit::IonBuilder::constant (this=0x7ffff5f911c0, v=...) at js/src/jit/IonBuilder.cpp:13623
#1  0x000000000073dbc1 in js::jit::IonBuilder::pushConstant (this=0x7ffff5f911c0, v=...) at js/src/jit/IonBuilder.cpp:3237
#2  0x0000000000742bbb in js::jit::IonBuilder::getElemTryCallSiteObject (this=this@entry=0x7ffff5f911c0, emitted=emitted@entry=0x7fffffff9f0e, obj=obj@entry=0x7ffff5f91b88, index=index@entry=0x7ffff5f91ca8) at js/src/jit/IonBuilder.cpp:8291
#3  0x0000000000770690 in js::jit::IonBuilder::jsop_getelem (this=0x7ffff5f911c0) at js/src/jit/IonBuilder.cpp:7801
#4  0x000000000078cec5 in js::jit::IonBuilder::inspectOpcode (this=this@entry=0x7ffff5f911c0, op=op@entry=JSOP_GETELEM) at js/src/jit/IonBuilder.cpp:2174
#5  0x000000000078eb18 in js::jit::IonBuilder::visitBlock (this=this@entry=0x7ffff5f911c0, cfgblock=cfgblock@entry=0x7ffff494b3a8, mblock=<optimized out>) at js/src/jit/IonBuilder.cpp:1563
#6  0x000000000078f4ed in js::jit::IonBuilder::traverseBytecode (this=this@entry=0x7ffff5f911c0) at js/src/jit/IonBuilder.cpp:1480
#7  0x00000000007902b2 in js::jit::IonBuilder::build (this=this@entry=0x7ffff5f911c0) at js/src/jit/IonBuilder.cpp:863
#8  0x000000000079d402 in js::jit::IonCompile (cx=<optimized out>, cx@entry=0x7ffff5f17000, script=<optimized out>, baselineFrame=baselineFrame@entry=0x0, osrPc=osrPc@entry=0x0, recompile=<optimized out>, optimizationLevel=<optimized out>) at js/src/jit/Ion.cpp:2101
#9  0x000000000079de97 in js::jit::Compile (cx=cx@entry=0x7ffff5f17000, script=script@entry=..., osrFrame=osrFrame@entry=0x0, osrPc=osrPc@entry=0x0, forceRecompile=forceRecompile@entry=false) at js/src/jit/Ion.cpp:2384
#10 0x000000000079dfbc in js::jit::CanEnterIon (cx=cx@entry=0x7ffff5f17000, state=...) at js/src/jit/Ion.cpp:2468
#11 0x00000000007c8959 in js::jit::MaybeEnterJit (cx=cx@entry=0x7ffff5f17000, state=...) at js/src/jit/Jit.cpp:140
#18 0x00000000004706c3 in Evaluate (cx=<optimized out>, argc=<optimized out>, vp=<optimized out>) at js/src/shell/js.cpp:2014
#19 0x00000000005b4f01 in js::CallJSNative (cx=0x7ffff5f17000, native=0x46fb50 <Evaluate(JSContext*, unsigned int, JS::Value*)>, args=...) at js/src/vm/JSContext-inl.h:280
#20 0x00000000005a94bf in js::InternalCallOrConstruct (cx=<optimized out>, cx@entry=0x7ffff5f17000, args=..., construct=construct@entry=js::NO_CONSTRUCT) at js/src/vm/Interpreter.cpp:467
#21 0x00000000005a989d in InternalCall (cx=0x7ffff5f17000, args=...) at js/src/vm/Interpreter.cpp:516
#22 0x00000000005a99ea in js::CallFromStack (cx=<optimized out>, args=...) at js/src/vm/Interpreter.cpp:522
#23 0x0000000000690a63 in js::jit::DoCallFallback (cx=<optimized out>, frame=0x7fffffffb508, stub_=<optimized out>, argc=<optimized out>, vp=0x7fffffffb4b8, res=...) at js/src/jit/BaselineIC.cpp:2382
#24 0x0000328edd29f71c in ?? ()
#46 0x0000000000000000 in ?? ()
rax	0x0	0
rbx	0x7ffff5f911c0	140737320128960
rcx	0x7ffff6c282ad	140737333330605
rdx	0x0	0
rsi	0x7ffff6ef7770	140737336276848
rdi	0x7ffff6ef6540	140737336272192
rbp	0x7fffffff9e80	140737488330368
rsp	0x7fffffff9e70	140737488330352
r8	0x7ffff6ef7770	140737336276848
r9	0x7ffff7fe4780	140737354024832
r10	0x58	88
r11	0x7ffff6b9e7a0	140737332766624
r12	0x7ffff4c990f0	140737300238576
r13	0x7ffff4c990c0	140737300238528
r14	0x7ffff5f91ca8	140737320131752
r15	0x7ffff5f911c0	140737320128960
rip	0x73db9e <js::jit::IonBuilder::constant(JS::Value const&)+222>
=> 0x73db9e <js::jit::IonBuilder::constant(JS::Value const&)+222>:	movl   $0x0,0x0
   0x73dba9 <js::jit::IonBuilder::constant(JS::Value const&)+233>:	ud2

Marking s-s because the test uses GC and the assertion also looks potentially s-s (type mismatch).
Whiteboard: [jsbugmon:update,bisect] → [jsbugmon:update]
JSBugMon: Bisection requested, result:
autoBisect shows this is probably related to the following changeset:

The first bad revision is:
user:        Tom Schuster
date:        Fri Apr 13 13:56:55 2018 +0200
summary:     Bug 1453932 - Optimize loads from CallSiteObjects for tagged template literals. r=jandem

This iteration took 282.521 seconds to run.
Flags: needinfo?(evilpies)
Assignee: nobody → evilpies
Flags: needinfo?(evilpies)
I didn't know this kind of nested eval would basically allow arbitrary constant objects to occur.... Anyway for CallSiteObjects the string should have been atomized by the parser.
Priority: -- → P1
Comment on attachment 8974156 [details] [diff] [review]
Only allow atomized strings from CallSiteObjects

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

::: js/src/jit/IonBuilder.cpp
@@ +8286,5 @@
>      }
> +    const Value& v = array->getDenseElement(uint32_t(idx));
> +    // Strings should have been atomized by the parser.
> +    if (v.isString() && !v.toString()->isAtom())

I think this should be:

  if (!v.isString() || !v.toString()->isAtom())
      return Ok();

We also don't want arbitrary (nursery) object pointers for instance.
Attachment #8974156 - Flags: review?(jdemooij) → review+
Keywords: sec-high
ARRR, I didn't mean to land this yet. Sorry!
Flags: needinfo?(dveditz)
Flags: needinfo?(abillings)
Comment on attachment 8974156 [details] [diff] [review]
Only allow atomized strings from CallSiteObjects

[Security approval request comment]
How easily could an exploit be constructed based on the patch?
Don't know

Do comments in the patch, the check-in comment, or tests included in the patch paint a bulls-eye on the security problem?
Kinda? I still think it's not really obvious how to construct such a constant froze object. 

Which older supported branches are affected by this flaw?
Only Beta

If not all supported branches, which bug introduced the flaw?
bug 1453932 introduced this flaw and code

Do you have backports for the affected branches? If not, how different, hard to create, and risky will they be?
Same patch

How likely is this patch to cause regressions; how much testing does it need?
Optional optimization, no real testing required
Attachment #8974156 - Flags: sec-approval?
Approval Request Comment
[Feature/Bug causing the regression]: bug 1453932
[User impact if declined]: crash/exploit
[Is this code covered by automated tests?]: test not landed
[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?]: two line code change that disables a bad optimization
[String changes made/needed]: none
Attachment #8974802 - Flags: approval-mozilla-beta?
Comment on attachment 8974156 [details] [diff] [review]
Only allow atomized strings from CallSiteObjects


At least it doesn't affect a release!
Flags: needinfo?(abillings)
Attachment #8974156 - Flags: sec-approval? → sec-approval+
Attachment #8974802 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
Flags: needinfo?(dveditz)
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla62
JSBugMon: This bug has been automatically verified fixed.
Group: javascript-core-security → core-security-release
Flags: in-testsuite?
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.