1408412 - Max number of actual arguments is not checked everywhere

Status: RESOLVED FIXED

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

Description

[Jan de Mooij [:jandem]]

Ion has a limit of maxStackArgs (4096), and that's what range analysis uses as upper bound for MArgumentsLength.

We don't check this limit consistently when making calls, for instance calls from Baseline ICs. The testcase below asserts with --ion-check-range-analysis --no-threads:

$ dist/bin/js --ion-check-range-analysis --no-threads test.js
Assertion failure: Integer input should be lower or equal than Upperbound., at MacroAssembler.cpp:1710
Trace/BPT trap: 5

Range analysis bugs like this one are sec-crit because we could incorrectly eliminate bounds checks.

The easiest fix (to backport) is a one-liner changing MArgumentsLength range analysis info to use ARGC_LIMIT instead of maxStackArgs. After that, we should deoptimize JIT -> JIT and Wasm -> JIT calls with an insane number of arguments.

---

function g() {
    x = arguments.length;
}
function f() {
    with(this) {};
    for (var i = 0; i < 1000; i++) {
        g();
    }
    var s = "for (var j = 0; j < 5000; j++) g(";
    for (var i = 0; i < 5000; i++)
        s += i + ",";
    s += "1);";
    eval(s);
}
f();

Comment 1

[Jan de Mooij [:jandem]]

(In reply to Jan de Mooij [:jandem] from comment #0)
> The easiest fix (to backport) is a one-liner changing MArgumentsLength range
> analysis info to use ARGC_LIMIT instead of maxStackArgs.

Or even safer, ARGS_LENGTH_MAX, as that also covers fun_apply et al (that case should be fine, but can't hurt to be more conservative with this).

Comment 2

[Julien Cristau [:jcristau]]

tracking as sec-crit.

Comment 3

[Jan de Mooij [:jandem]]

Attachment: Patch

This is just the low-risk fix we can backport.

In follow-ups we can actually enforce JitOptions.maxStackArgs for Wasm and JIT callers and revert this, but that's more complicated.

Comment 4

[Jan de Mooij [:jandem]]

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

[Security approval request comment]
> How easily could an exploit be constructed based on the patch?
Might be possible for a determined attacker.

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

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

> How likely is this patch to cause regressions; how much testing does it need?
Unlikely.

Comment 5

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

sec-approval+ for trunk. This seems pretty safe. Can you please nominate patches for ESR52 and Beta immediately as well?

Comment 6

[Jan de Mooij [:jandem]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/aaf54dce048a85806833529a5d53ecc46156ef47

Comment 7

[Jan de Mooij [:jandem]]

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

Approval Request Comment
[Feature/Bug causing the regression]: Old bug.
[User impact if declined]: Security issues.
[Is this code covered by automated tests?]: Yes.
[Has the fix been verified in Nightly?]: Not yet.
[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?]: Very low risk.
[Why is the change risky/not risky?]: Just changes a constant to a different one.
[String changes made/needed]: None.

Comment 8

[Liz Henry (:lizzard Please n-i to RyanVM, jcristau, or pascal)]

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

Fix for sec critical issue, let's land it for beta 12 and for ESR.

Comment 9

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/aaf54dce048a

Comment 10

[Ryan VanderMeulen [:RyanVM]]

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

Comment 11

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-esr52/rev/8426754b7130

Comment 12

[Andrei Vaida [:avaida]]

(In reply to Jan de Mooij [:jandem] from comment #7)
> [Is this code covered by automated tests?]: Yes.
> [Has the fix been verified in Nightly?]: Not yet.
> [Needs manual test from QE? If yes, steps to reproduce]: No.

Setting qe-verify- based on Jan's assessment on manual testing needs and the fact that this fix has automated coverage.

Comment 13

[Jan de Mooij [:jandem]]

NI myself to land a test for this later.

Comment 14

[Jan de Mooij [:jandem]]

(In reply to Jan de Mooij [:jandem] from comment #13)
> NI myself to land a test for this later.

Landed the test:

https://hg.mozilla.org/integration/mozilla-inbound/rev/996616295128d61343f50270fe79b3ee49b9579b

Comment 15

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

https://hg.mozilla.org/mozilla-central/rev/996616295128