Closed Bug 1408412 Opened 4 years ago Closed 4 years ago

Max number of actual arguments is not checked everywhere

Categories

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

defect

Tracking

()

RESOLVED FIXED
mozilla58
Tracking Status
firefox-esr52 57+ fixed
firefox56 --- wontfix
firefox57 + fixed
firefox58 + fixed

People

(Reporter: jandem, Assigned: jandem)

Details

(Keywords: sec-critical, Whiteboard: [adv-main57+][adv-esr52.5+])

Attachments

(1 file)

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();
(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).
tracking as sec-crit.
Priority: -- → P1
Assignee: nobody → jdemooij
Status: NEW → ASSIGNED
Attached patch PatchSplinter Review
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.
Attachment #8921405 - Flags: review?(nicolas.b.pierron)
Attachment #8921405 - Flags: review?(nicolas.b.pierron) → review+
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.
Attachment #8921405 - Flags: sec-approval?
sec-approval+ for trunk. This seems pretty safe. Can you please nominate patches for ESR52 and Beta immediately as well?
Flags: needinfo?(jdemooij)
Attachment #8921405 - Flags: sec-approval? → sec-approval+
Flags: needinfo?(rkothari)
Flags: needinfo?(lhenry)
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.
Flags: needinfo?(jdemooij)
Attachment #8921405 - Flags: approval-mozilla-esr52?
Attachment #8921405 - Flags: approval-mozilla-beta?
Flags: needinfo?(rkothari)
Flags: needinfo?(lhenry)
Comment on attachment 8921405 [details] [diff] [review]
Patch

Fix for sec critical issue, let's land it for beta 12 and for ESR.
Attachment #8921405 - Flags: approval-mozilla-esr52?
Attachment #8921405 - Flags: approval-mozilla-esr52+
Attachment #8921405 - Flags: approval-mozilla-beta?
Attachment #8921405 - Flags: approval-mozilla-beta+
https://hg.mozilla.org/mozilla-central/rev/aaf54dce048a
Status: ASSIGNED → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla58
Group: javascript-core-security → core-security-release
(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.
Flags: qe-verify-
NI myself to land a test for this later.
Flags: needinfo?(jdemooij)
Whiteboard: [adv-main57+][adv-esr52.5+]
(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
Flags: needinfo?(jdemooij)
Group: core-security-release
You need to log in before you can comment on or make changes to this bug.