Assertion failure: isInt32(), at dist/include/js/Value.h:605

VERIFIED FIXED in Firefox -esr52

Status

()

defect
--
critical
VERIFIED FIXED
2 years ago
2 years ago

People

(Reporter: decoder, Assigned: jonco)

Tracking

(Blocks 2 bugs, 4 keywords)

Trunk
mozilla56
x86_64
Linux
Points:
---
Dependency tree / graph
Bug Flags:
qe-verify -

Firefox Tracking Flags

(firefox-esr5255+ fixed, firefox54 wontfix, firefox55+ verified, firefox56+ verified)

Details

(Whiteboard: [jsbugmon:][adv-main55+][adv-esr52.3+], crash signature)

Attachments

(1 attachment, 2 obsolete attachments)

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

var lfLogBuffer = `
function f() {
    var i32 = new Int32Array(1);
    var f32 = new Float32Array(i32.buffer);
    for (var i = 0; i < 3; i++) {
      var { regExp, get,     }  = gczeal(9,10) 
        ? (yield) : (yield) = call(f32, "i32.store", []);
    }
}
f();
`;
loadFile(lfLogBuffer);
function loadFile(lfVarx) {
    try {
      oomTest(function() { eval(lfVarx); });
    } catch (lfVare) {}
}



Backtrace:

Program terminated with signal SIGSEGV, Segmentation fault.
#0  0x000000000067ffc0 in JS::Value::toInt32 (this=<optimized out>) at dist/include/js/Value.h:605
#1  js::TypedArrayObject::length (this=this@entry=0x7ff2e4299100) at js/src/vm/TypedArrayObject.h:164
#2  0x0000000000c43f11 in js::TypedArrayObject::assertZeroLengthArrayData (this=0x7ff2e4299100) at js/src/vm/TypedArrayObject.cpp:147
#3  0x0000000000c43f7f in js::TypedArrayObject::elements (this=0x7ff2e4299100) at js/src/vm/TypedArrayObject.h:173
#4  js::TypedArrayObject::hasInlineElements (this=0x7ff2e4299100) at js/src/vm/TypedArrayObject.cpp:272
#5  0x0000000000c486b0 in js::TypedArrayObject::objectMoved (obj=<optimized out>, old=0x7ff2e4299100) at js/src/vm/TypedArrayObject.cpp:194
#6  0x00000000009c81c3 in RelocateCell (thingSize=96, thingKind=js::gc::AllocKind::OBJECT8_BACKGROUND, src=0x7ff2e4299100, zone=0x7ff2e40e4000) at js/src/jsgc.cpp:1993
#7  RelocateArena (sliceBudget=..., arena=0x7ff2e4299000) at js/src/jsgc.cpp:2023
#8  js::gc::ArenaList::relocateArenas (this=this@entry=0x7ff2e40e4238, toRelocate=0x0, toRelocate@entry=0x7ff2e42cb000, relocated=0x7ff2e42cb000, sliceBudget=..., stats=...) at js/src/jsgc.cpp:2063
#9  0x00000000009c93ae in js::gc::ArenaLists::relocateArenas (this=this@entry=0x7ff2e40e40b0, zone=zone@entry=0x7ff2e40e4000, relocatedListOut=@0x7ffe1a1569d8: 0x7ff2e4239000, reason=reason@entry=JS::gcreason::DEBUG_GC, sliceBudget=..., stats=...) at js/src/jsgc.cpp:2110
#10 0x00000000009c954b in js::gc::GCRuntime::relocateArenas (this=this@entry=0x7ff2e595e5f8, zone=zone@entry=0x7ff2e40e4000, reason=reason@entry=JS::gcreason::DEBUG_GC, relocatedListOut=@0x7ffe1a1569d8: 0x7ff2e4239000, sliceBudget=...) at js/src/jsgc.cpp:2147
#11 0x00000000009c98e7 in js::gc::GCRuntime::compactPhase (this=this@entry=0x7ff2e595e5f8, reason=reason@entry=JS::gcreason::DEBUG_GC, sliceBudget=..., lock=...) at js/src/jsgc.cpp:5727
#12 0x00000000009d7939 in js::gc::GCRuntime::incrementalCollectSlice (this=this@entry=0x7ff2e595e5f8, budget=..., reason=reason@entry=JS::gcreason::DEBUG_GC, lock=...) at js/src/jsgc.cpp:6228
#13 0x00000000009d8b94 in js::gc::GCRuntime::gcCycle (this=this@entry=0x7ff2e595e5f8, nonincrementalByAPI=nonincrementalByAPI@entry=false, budget=..., reason=reason@entry=JS::gcreason::DEBUG_GC) at js/src/jsgc.cpp:6465
#14 0x00000000009d9488 in js::gc::GCRuntime::collect (this=this@entry=0x7ff2e595e5f8, nonincrementalByAPI=nonincrementalByAPI@entry=false, budget=..., reason=reason@entry=JS::gcreason::DEBUG_GC) at js/src/jsgc.cpp:6614
#15 0x00000000009dac1c in js::gc::GCRuntime::runDebugGC (this=this@entry=0x7ff2e595e5f8) at js/src/jsgc.cpp:7151
#16 0x0000000000d28ac5 in js::gc::GCRuntime::gcIfNeededAtAllocation (this=0x7ff2e595e5f8, cx=cx@entry=0x7ff2e5924000) at js/src/gc/Allocator.cpp:230
#17 0x0000000000d36918 in js::gc::GCRuntime::checkAllocatorState<(js::AllowGC)1> (this=<optimized out>, cx=0x7ff2e5924000, kind=js::gc::AllocKind::OBJECT_LIMIT) at js/src/gc/Allocator.cpp:191
#18 0x0000000000d3a092 in js::Allocate<JSScript, (js::AllowGC)1> (cx=cx@entry=0x7ff2e5924000) at js/src/gc/Allocator.cpp:142
#19 0x0000000000a2c2d4 in JSScript::Create (cx=0x7ff2e5924000, options=..., sourceObject=..., sourceObject@entry=..., bufStart=bufStart@entry=0, bufEnd=246, toStringStart=0, toStringEnd=246) at js/src/jsscript.cpp:2627
#20 0x0000000000a8163e in BytecodeCompiler::createScript (this=0x7ffe1a157340, toStringStart=<optimized out>, toStringEnd=<optimized out>) at js/src/frontend/BytecodeCompiler.cpp:272
#21 0x0000000000aadc7d in BytecodeCompiler::createScript (this=0x7ffe1a157340) at js/src/frontend/BytecodeCompiler.cpp:266
#22 BytecodeCompiler::compileScript (this=this@entry=0x7ffe1a157340, environment=..., environment@entry=..., sc=sc@entry=0x7ffe1a1572e0) at js/src/frontend/BytecodeCompiler.cpp:336
#23 0x0000000000aae46a in BytecodeCompiler::compileEvalScript (enclosingScope=..., environment=..., this=0x7ffe1a157340) at js/src/frontend/BytecodeCompiler.cpp:402
#24 js::frontend::CompileEvalScript (cx=cx@entry=0x7ff2e5924000, alloc=..., environment=environment@entry=..., enclosingScope=..., options=..., srcBuf=..., sourceObjectOut=0x0) at js/src/frontend/BytecodeCompiler.cpp:601
#25 0x000000000056bf90 in EvalKernel (cx=cx@entry=0x7ff2e5924000, v=..., v@entry=..., evalType=evalType@entry=DIRECT_EVAL, caller=..., env=env@entry=..., pc=<optimized out>, vp=...) at js/src/builtin/Eval.cpp:318
#26 0x000000000056c75e in js::DirectEval (cx=cx@entry=0x7ff2e5924000, v=..., vp=vp@entry=...) at js/src/builtin/Eval.cpp:438
#27 0x000000000060c7f0 in js::jit::DoCallFallback (cx=0x7ff2e5924000, frame=0x7ffe1a158b38, stub_=<optimized out>, argc=<optimized out>, vp=0x7ffe1a158ae8, res=...) at js/src/jit/BaselineIC.cpp:2437
#28 0x00000e1b84031d77 in ?? ()
rax	0x0	0
rbx	0x7ff2e4299100	140681186742528
rcx	0x7ff2e5c5aa2d	140681213749805
rdx	0x0	0
rsi	0x7ff2e5f29770	140681216694128
rdi	0x7ff2e5f28540	140681216689472
rbp	0x7ffe1a1565d0	140729336030672
rsp	0x7ffe1a1565d0	140729336030672
r8	0x7ff2e5f29770	140681216694128
r9	0x7ff2e701a740	140681234458432
r10	0x58	88
r11	0x7ff2e5bd1750	140681213187920
r12	0x7ff2e42ef280	140681187095168
r13	0x7ff2e4299100	140681186742528
r14	0x60	96
r15	0x7ff2e42ef280	140681187095168
rip	0x67ffc0 <js::TypedArrayObject::length() const+112>
=> 0x67ffc0 <js::TypedArrayObject::length() const+112>:	movl   $0x0,0x0
   0x67ffcb <js::TypedArrayObject::length() const+123>:	ud2    


Marking s-s because the test uses gczeal and TypedArrays plus the assertion shows a type problem, so this is very likely a security issue. The testcase is intermittent but should reproduce in less than 10 runs. If you could figure out how to make this testcase deterministic that would be good for future testing.
Whiteboard: [jsbugmon:update,bisect] → [jsbugmon:bisect]
JSBugMon: Cannot process bug: Unable to automatically reproduce, please track manually.
Whiteboard: [jsbugmon:bisect] → [jsbugmon:]
Comment 2 is probably invalid due to the intermittent testcase. Since this involves TypedArrays, setting needinfo? from Steve as a start.
Flags: needinfo?(sphink)
It looks like compacting GC is involved, so maybe Jon could take a look.
Flags: needinfo?(jcoppeard)
Keywords: sec-high
Flags: needinfo?(jcoppeard)
Posted patch bug1369994-typed-array-init (obsolete) — Splinter Review
The condition in the assertion doesn't always hold because we can fail initialisation of template objects part way through.  The patch shuffles around the initialisation code so that doesn't happen.
Assignee: nobody → jcoppeard
Flags: needinfo?(sphink)
Attachment #8878468 - Flags: review?(jdemooij)
Comment on attachment 8878468 [details] [diff] [review]
bug1369994-typed-array-init

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

Do you think this is s-s? If yes we shouldn't add the test.
Attachment #8878468 - Flags: review?(jdemooij) → review+
I can't immediately see how this could create a vulnerability, but leaving s-s since typed arrays are sufficiently complicated that there could be one.  I'll remove the test.
Group: javascript-core-security
Group: javascript-core-security
[Security approval request comment]
How easily could an exploit be constructed based on the patch? Very difficult, requiring arranging an OOM to happen during JIT compilation.

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? Everything back to FF 50.

If not all supported branches, which bug introduced the flaw? Bug 1248163.

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

How likely is this patch to cause regressions; how much testing does it need? Unlikely.
Attachment #8878468 - Attachment is obsolete: true
Attachment #8878503 - Flags: sec-approval?
Attachment #8878503 - Flags: review+
Blocks: 1248163
sec-approval+ for checkin on June 26 or after. We'll want Beta and ESR52 patches nominated and approved to go in after that.
Whiteboard: [jsbugmon:] → [jsbugmon:][checkin on 6/26]
Attachment #8878503 - Flags: sec-approval? → sec-approval+
https://hg.mozilla.org/integration/mozilla-inbound/rev/e7414bbec32d3e075551485eb83580021aea70d8

This grafts cleanly to Beta and ESR52, so please nominate it when you get a chance. Sorry for adding the wrong reviewer to the commit message too, will fix on uplift :(
Flags: needinfo?(jcoppeard)
Whiteboard: [jsbugmon:][checkin on 6/26] → [jsbugmon:]
Updated patch.
Attachment #8878503 - Attachment is obsolete: true
Attachment #8881465 - Flags: review+
https://hg.mozilla.org/mozilla-central/rev/d516c35eabf1
Status: NEW → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla56
Status: RESOLVED → VERIFIED
JSBugMon: This bug has been automatically verified fixed.
Group: javascript-core-security → core-security-release
Comment on attachment 8881465 [details] [diff] [review]
bug1369994-typed-array-init v3

Approval Request Comment
[Feature/Bug causing the regression]: Bug 1248163.
[User impact if declined]: Possible crash / security vulnerability.
[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's a simple change to initialise a template object before a fallible operation to avoid exposing it to GC before it's been initialised. 
[String changes made/needed]: None.
Attachment #8881465 - Flags: approval-mozilla-beta?
Comment on attachment 8881465 [details] [diff] [review]
bug1369994-typed-array-init v3

[Approval Request Comment]
If this is not a sec:{high,crit} bug, please state case for ESR consideration:
User impact if declined: sec-high bug.
Fix Landed on Version: 56.
Risk to taking this patch (and alternatives if risky): Low.
String or UUID changes made by this patch: None.
Attachment #8881465 - Flags: approval-mozilla-esr52?
Comment on attachment 8881465 [details] [diff] [review]
bug1369994-typed-array-init v3

sec-high fix for beta55
Attachment #8881465 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
Attachment #8881465 - Flags: approval-mozilla-esr52? → approval-mozilla-esr52+
Whiteboard: [jsbugmon:] → [jsbugmon:][adv-main55+][adv-esr52.3+]
(In reply to Jon Coppeard (:jonco) from comment #16)
> [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 Jon Coppeard's assessment on manual testing needs and the fact that this fix has automated coverage.
Flags: qe-verify-
JSBugMon: This bug has been automatically verified fixed on Fx55
Group: core-security-release
You need to log in before you can comment on or make changes to this bug.