1537924 - (CVE-2019-9810) ZDI-CAN-8368 IonMonkey MArraySlice has incorrect alias information

Status: VERIFIED FIXED

(Core :: JavaScript Engine, defect, P1)

Description

[Alex Gaynor [:Alex_Gaynor]]

Attachment: test.html

pwn2own 2019

Here is the JIT implementation of Array.slice:

  CompilerObject templateObj_;
  gc::InitialHeap initialHeap_;

  MArraySlice(MDefinition* obj, MDefinition* begin, MDefinition* end,
              JSObject* templateObj, gc::InitialHeap initialHeap)
      : MTernaryInstruction(classOpcode, obj, begin, end),
        templateObj_(templateObj),
        initialHeap_(initialHeap) {
    setResultType(MIRType::Object);
  }

 public:
  INSTRUCTION_HEADER(ArraySlice)
  TRIVIAL_NEW_WRAPPERS
  NAMED_OPERANDS((0, object), (1, begin), (2, end))

  JSObject* templateObj() const { return templateObj_; }

  gc::InitialHeap initialHeap() const { return initialHeap_; }

  AliasSet getAliasSet() const override {
    return AliasSet::Store(AliasSet::Element | AliasSet::ObjectFields);
  }
  bool possiblyCalls() const override { return true; }
  bool appendRoots(MRootList& roots) const override {
    return roots.append(templateObj_);
  }

This states that Array.sice does not have side effects, but actually they can be triggered via Symbol.species.

We execute JS that shortens the Array to cause OOB writes later on.
We spray TypedArrays and overwrite a length to gain arb r/w.
We then ROP to execute our shellcode.

Comment 1

[Alex Gaynor [:Alex_Gaynor]]

Attachment: server_simple.py

Comment 2

[Jan de Mooij [:jandem]]

Attachment: Shell test

Comment 3

[Jan de Mooij [:jandem]]

Attachment: More reduced shell test

Comment 4

[Ted Campbell [:tcampbell]]

We need to audit for similar conflicts of possiblyCalls() and getAliasSet().

Here is a similar example where we do the right thing:
https://searchfox.org/mozilla-central/rev/7c20ad925005fbad7b8e08813115f1ec7fa1c248/js/src/jit/MIR.h#8112-8114

Comment 5

[Jan de Mooij [:jandem]]

Attachment: More reduced shell test

Still repros with --no-threads on OS X.

Comment 6

[Jan de Mooij [:jandem]]

I'll post a minimal fix for this one. I think as follow-up we should try to make callVM assert MIR instructions are fully effectful and deal with any fall out.

Comment 7

[Jan de Mooij [:jandem]]

Attachment: Bug 1537924 part 1 - Simplify some alias sets in Ion. r?tcampbell!

Comment 8

[Gary Kwong [:gkw] [:nth10sd] (NOT official MoCo now)]

Attachment: stack

function f(o, n, v) {
    arr[n];
    o.slice();
    arr[n] = v;
}
x = []
class A extends Array {
    static get[Symbol.species]() {
        if (p) {
            arr.length = 0;
            for (let i = 0; i < 1024; i++) {
                x.push(new Uint8Array);
            }
        }
    }
}
let o = new A;
arr = Array(9).fill("");
p = 0;
for (let i = 0; i < 24; i++) {
    f(o, 2, i);
}
p = 1;
f(o, 7, 5);
arr = Array(26).fill("");
p = 0;
for (let i = 0; i < 24; i++) {
    f(o, 2, i);
}
p = 1;
f(o, 7, 5);

crashes js shell on m-c rev b2f1edb41241 compiled with --enable-debug --enable-more-deterministic with --fuzzing-safe --no-threads --ion-eager [@ js::NativeObject::sweepDictionaryListPointer].

Thread 1 "js-dbg-64-dm-li" received signal SIGSEGV, Segmentation fault.
js::NativeObject::sweepDictionaryListPointer (this=<optimized out>) at /home/ubuntu/trees/mozilla-central/js/src/vm/JSObject-inl.h:117
117 if (shape()->listp == shapePtr()) {
(gdb) bt
#0 js::NativeObject::sweepDictionaryListPointer (this=<optimized out>) at /home/ubuntu/trees/mozilla-central/js/src/vm/JSObject-inl.h:117
#1 JSObject::finalize (this=0x3cd9e5982e0, fop=0x7fffffffd368) at /home/ubuntu/trees/mozilla-central/js/src/vm/JSObject-inl.h:109
#2 0x0000555556e57ebf in js::gc::Arena::finalize<JSObject> (this=0x3cd9e598000, fop=0x7fffffffd368, thingKind=<optimized out>, thingSize=96)
at /home/ubuntu/trees/mozilla-central/js/src/gc/GC.cpp:591
#3 0x0000555556e28e5a in FinalizeTypedArenas<JSObject> (fop=0x7fffffffd368, src=0x7fffffffc2d0, dest=...,
thingKind=js::gc::AllocKind::OBJECT8_BACKGROUND, budget=..., keepArenas=<optimized out>) at /home/ubuntu/trees/mozilla-central/js/src/gc/GC.cpp:651
#4 0x0000555556df3423 in FinalizeArenas (fop=0x7fffffffd368, src=0x7fffffffc2d0, dest=..., thingKind=<optimized out>, budget=...,
keepArenas=js::gc::ArenaLists::KEEP_ARENAS) at /home/ubuntu/trees/mozilla-central/js/src/gc/GC.cpp:683
#5 0x0000555556df2b82 in js::gc::ArenaLists::backgroundFinalize (fop=0x3cd9e598310, listHead=0x3cd9e5c2000, empty=0x7fffffffd360)
at /home/ubuntu/trees/mozilla-central/js/src/gc/GC.cpp:3101
/snip

Comment 9

[Jan de Mooij [:jandem]]

This needs a follow-up patch with comments and test case at the very least, but I'll wait for a more reduced test from the fuzzing folks.

Comment 10

[Jan de Mooij [:jandem]]

I filed bug 1537942 to deal with this class of bugs in general.

Comment 11

[Ted Campbell [:tcampbell]]

Auditing cases that have a non-trivial alias set and also call C++ I get the following list. Note that the exploit is around C++ calls that wind up calling back into JS which is only a fraction of these.

MThrow
- The alias set is weird, but throw has special handling in Ion

MCreateThis
MCreateThisWithProto
MCreateArgumentsObject
MNewLexicalEnvironmentObject
MCopyLexicalEnvrionmentObject
- Controlled VM behaviour

MAtan2
MHypot
MPow
MPowHalf
MMathFunction
MSinCos
- Math functions that use CallWithABI and can't call scripted

MRandom
- Does not seem to even call C++

MCallGetIntrinsicValue
- Self-hosting only

MArraySlice
- EXPLOIT

MArrayJoin
- MITIGATED

MStringConvertCase
MStringSplit
MStringReplace
MRegExp
MRestCommon
MGetNameCache
MCallGetProperty
- Need more digging!

Comment 12

[Jan de Mooij [:jandem]]

Comment on attachment 9052651 [details]
Bug 1537924 part 1 - Simplify some alias sets in Ion. r?tcampbell!

Security Approval Request

• How easily could an exploit be constructed based on the patch?: Not too 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?: All
• If not all supported branches, which bug introduced the flaw?: Bug 1165052
• Do you have backports for the affected branches?: Yes
• If not, how different, hard to create, and risky will they be?: Should apply or be easy to apply.
• How likely is this patch to cause regressions; how much testing does it need?: Not expecting regressions but some fuzzing would be good anyway.

Comment 13

[Ted Campbell [:tcampbell]]

(In reply to Ted Campbell [:tcampbell] from comment #11)

MStringConvertCase
MStringSplit
MStringReplace
MRegExp
MRestCommon
MGetNameCache
MCallGetProperty
- Need more digging!

Jan helped go through the list and these look fine. We change MCallGetProperty to better match MGetPropertyCache, but it is not strictly a bug.

That initial list was based on looking at 'maybeCalls', but there aren't guarantees that it covers all CallVM so we are sanity checking that.

Comment 14

[Gary Kwong [:gkw] [:nth10sd] (NOT official MoCo now)]

Interestingly, my testcase in comment 8 points to this regressing changeset:

autobisectjs shows this is probably related to the following changeset:

The first bad revision is:
changeset: https://hg.mozilla.org/mozilla-central/rev/e68932c65a3f
user: Paul Bone
date: Wed Mar 06 12:57:04 2019 +0000
summary: Bug 1532857 - Use SubChunkLimit when re-enabling the nursery r=jonco

Setting needinfo? from Paul here as well to ensure we don't have anything GC-related falling through the cracks.

Comment 15

[Jan de Mooij [:jandem]]

(In reply to Gary Kwong [:gkw] [:nth10sd] from comment #14)

Interestingly, my testcase in comment 8 points to this regressing changeset:

False positive I'm pretty sure. Likely changes what's affected by the memory corruption.

Comment 16

[Gary Kwong [:gkw] [:nth10sd] (NOT official MoCo now)]

Comment on attachment 9052651 [details]
Bug 1537924 part 1 - Simplify some alias sets in Ion. r?tcampbell!

I've done a short cursory fuzzing round to ensure that this patch didn't seem to immediately blow things up.

Comment 17

[Andrew McCreight [:mccr8]]

Updated summary from tcampbell.

Comment 18

[Paul Bone [:pbone]]

(In reply to Jan de Mooij [:jandem] from comment #15)

(In reply to Gary Kwong [:gkw] [:nth10sd] from comment #14)

Interestingly, my testcase in comment 8 points to this regressing changeset:

False positive I'm pretty sure. Likely changes what's affected by the memory corruption.

I think so too.

Comment 19

[Daniel Veditz [:dveditz]]

I assume this patch is sufficient to fix the bug and there aren't any blindingly obvious other places with the same problem given the discussion and the sec-approval request. But still, "part 1" -- should we be waiting for "part 2"?

Comment 20

[Ted Campbell [:tcampbell]]

There is no part 2 (we flattened the patches). Blindingly obvious has been checked for. I'll run a few less obvious checks over the next hour, but I don't expect it to turn up anything.

Good to go!

Comment 21

[Daniel Veditz [:dveditz]]

Comment on attachment 9052651 [details]
Bug 1537924 part 1 - Simplify some alias sets in Ion. r?tcampbell!

sec-approval=dveditz

branch landing approval a=dveditz after consulting RyanVM on irc

Comment 22

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/229759a67f4f
https://hg.mozilla.org/releases/mozilla-beta/rev/28911550fa18
https://hg.mozilla.org/releases/mozilla-release/rev/e8e770918af7

Comment 23

[Ted Campbell [:tcampbell]]

Attachment: Bug 1537924 - [ESR60] Simplify some alias sets in Ion

Comment 24

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-esr60/rev/cba90e815e4e

Comment 25

[Ted Campbell [:tcampbell]]

Looking at places that CallVM but do not possiblyCalls (and thus weren't in the first pass check), none are of concern. They either have no scripted side-effects or don't set a problematic alias-set.

The list was:

• IonBinaryArithICUpdate
• IonCompareICUpdate
• IonForcedRecompile
• IonGetIteratorICUpdate
• IonGetPropertyICUpdate
• IonHasOwnICUpdate
• IonInICUpdate
• IonSetPropertyICUpdate
• IonUnaryArithICUpdate
• ArrayConstructorOneArg
• BitOr
• ConcatStrings
• ConvertElementsToDoubles
• CreateThisWithTemplate
• Debug_CheckSelfHosted
• DeleteElementStrict
• DeletePropertyNonStrict
• DoToNumeric
• GetPrototypeOf
• GlobalNameConflictsCheckFromIon
• HomeObjectSuperBase
• InitPropGetterSetterOperation
• InlineTypedObjectCreateCopy
• Int32ToString
• IsPossiblyWrappedTypedArray
• IsPrototypeOf
• MulValues
• NamedLambdaObjectCreateTemplateObject
• NewArrayIteratorObject
• NewArrayOperation
• NewArrayWithGroup
• NewCallObject
• NewDenseCopyOnWriteArray
• NewObjectOperationWithTemplate
• NewStringIteratorObject
• NewStringObject
• NewTypedArrayWithTemplateAndLength
• NumberToString
• ObjectClassToString
• SetDenseElement
• StartDynamicModuleImport
• StringFromCharCode
• StringFromCodePoint
• StringToNumber
• StringsEqual
• StringsNotEqual
• SubstringKernel
• ToObjectSlow
• ToStringSlow

(A bunch of these should probably be marked as possiblyCalls, that is an Ion heuristic issue, not a security issue)

Comment 26

[Christian Holler (:decoder)]

Comment on attachment 9052651 [details]
Bug 1537924 part 1 - Simplify some alias sets in Ion. r?tcampbell!

No issues found in fuzzing.

Comment 27

[Ted Campbell [:tcampbell]]

Attachment: Exploit without Stage2

This is test.html without the need for stage2. It should crash unpatched browsers.

Comment 28

[Ciprian Georgiu, Desktop QA]

I was able to reproduce this crash using the "Exploit without Stage2" testcase from comment 27, on an affected Beta 65.0b12.

The crash is no longer reproducible on the fixed builds: 68.0a1, 67.0b4, 66.0.1 and 60.6.1esr. Tested with Windows 7 x64

Comment 29

[Alex Gaynor [:Alex_Gaynor]]

This was Richard Zhu and Amat Cama's entry from pwn2own 2019.

Comment 30

[Emma Humphries ☕️🎸🧞‍♀️✨ (she/they) [:emceeaich] (Pacific Time) use needinfo]

Adding :tmaity to CC list as part of RCA Phase II review.

Comment 31

[Emma Humphries ☕️🎸🧞‍♀️✨ (she/they) [:emceeaich] (Pacific Time) use needinfo]

Requesting a root cause entered for blockers in recent releases in security groups. See :tmaity for details.