Closed Bug 1322315 Opened 7 years ago Closed 7 years ago

Assertion failure: Integer input should be lower or equal than Upperbound., at js/src/jit/MacroAssembler.cpp:1597

Categories

(Core :: JavaScript Engine, defect, P1)

x86_64
Linux
defect

Tracking

()

VERIFIED FIXED
mozilla53
Tracking Status
firefox-esr45 51+ fixed
firefox50 --- wontfix
firefox51 + verified
firefox52 + verified
firefox53 + verified

People

(Reporter: decoder, Unassigned, NeedInfo)

Details

(4 keywords, Whiteboard: [jsbugmon:update][post-critsmash-triage][adv-main51+][adv-esr45.7+])

Attachments

(3 files)

The following testcase crashes on mozilla-central revision 8103c612b79c (build with --enable-posix-nspr-emulation --enable-valgrind --enable-gczeal --disable-tests --enable-debug --enable-optimize, run with --fuzzing-safe --thread-count=2 --baseline-eager --ion-check-range-analysis):

var proxy = Function.prototype.bind.call(new Proxy(Array, {}));
for (var i = 10; i < 349592; ++i) {
    var args = Array(i).fill(i);
    var array = new proxy(...args);
}


Backtrace:

 received signal SIGTRAP, Trace/breakpoint trap.
0x00007ffff7e16198 in ?? ()
#0  0x00007ffff7e16198 in ?? ()
[...]
#24 0x00007ffff7e3be0d in ?? ()
#25 0x0000000000803044 in js::jit::BoxInputsPolicy::adjustInputs (this=0x2026a10 <js::jit::BoxInputsPolicy::Data::thisTypePolicy()::singletonType>, def=0x7ffff3244008, alloc=..., this=0x2026a10 <js::jit::BoxInputsPolicy::Data::thisTypePolicy()::singletonType>) at js/src/jit/TypePolicy.h:85
#26 js::jit::ObjectPolicy<2u>::staticAdjustInputs (alloc=..., ins=0xfff8800000001001) at js/src/jit/TypePolicy.cpp:801
Backtrace stopped: previous frame identical to this frame (corrupt stack?)
rax	0x1001	4097
rbx	0x7fffffff41a8	140737488306600
rcx	0x1001	4097
rdx	0x0	0
rsi	0x7ffff7e14527	140737352123687
rdi	0x7ffff3325d08	140737273552136
rbp	0x7fffffff40f0	140737488306416
rsp	0x7ffffffec0a0	140737488273568
r8	0x0	0
r9	0x52	82
r10	0x7fffffff4118	140737488306456
r11	0x7ffff6b9f750	140737332770640
r12	0x8	8
r13	0x7ffff3244008	140737272627208
r14	0x1001	4097
r15	0x7ffff36ab8d0	140737277245648
rip	0x7ffff7e16198	140737352130968
=> 0x7ffff7e16198:	movabs $0xfff9000000000000,%r11
   0x7ffff7e161a2:	mov    %r11,(%rsp)


This looks like a range analysis bug to me, marking s-s until we have investigated if this has any security implications because previous bugs in this area were also s-s.
Whiteboard: [jsbugmon:update,bisect] → [jsbugmon:update]
JSBugMon: Bisection requested, result:
=== Treeherder Build Bisection Results by autoBisect ===

The "good" changeset has the timestamp "20160112055644" and the hash "0af7319a6a4169d94453f8fcfd78384459c343db".
The "bad" changeset has the timestamp "20160112064943" and the hash "592fc90e655a1ebd3968300b5ed6261d24ed4065".

Likely regression window: https://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=0af7319a6a4169d94453f8fcfd78384459c343db&tochange=592fc90e655a1ebd3968300b5ed6261d24ed4065
Bisection is not very useful, it's probably exposed by self-hosting bind.

NI nbp because RA.
Flags: needinfo?(nicolas.b.pierron)
sec-high unless nbp's analysis says it's not so bad.
Keywords: sec-high
The assertion is raised while checking the range of MArgumentsLength, which used to be constraint between [0; 4096] by the fact that we were not jumping into Ion if our call stack was larger than that, in order to avoid consuming too much C-stack space.

The fact that this assertion is raised can cause the chrome code to run out of stack space faster, and can also be used to create exploit which are mapping the whole memory by ignoring the length of an array before encoding accesses¹ and mutating the length of another object. (as seen in a previous exploit)

In the current test case, the function call comes from code produced by:
  js::jit::ICCall_ScriptedApplyArguments::Compiler::generateStubCode ²

In addition to fix this issue, we should double check that all the caller of "MacroAssembler::callJit" have either a fixed number of actual arguments which is below or equal to 4096, or that the we fallback after checking dynamically.


¹ When we have a bounds check, we verify if the bounds are reachable, otherwise, we remove the bound checks [2,3].  If we were to have some code which check that the index is always below "arguments.length", and that the length is always higher than "4096", then the bound check can potentially be removed.

function f(arr)
{
  if (arr.length <= 4096)
    return;
  for (var i = 0; i < arguments.length; i++) {
    // BoundsCheck(i, arr.length)  {i in [0; 4906], arr.length [4097; …]}
    arr[i] = 0x41414141;
  }
}

² All versions are likely to be affected by this bug (since 2013).

[2] http://searchfox.org/mozilla-central/source/js/src/jit/RangeAnalysis.cpp#3385-3402
[3] http://searchfox.org/mozilla-central/source/js/src/jit/Lowering.cpp#2996,3002-3003

Jan / Hannes, I am offline for the rest of the week, can you have a look?
Flags: needinfo?(jdemooij)
Flags: needinfo?(hv1989)
Keywords: sec-highsec-critical
Flags: needinfo?(hv1989)
Priority: -- → P1
Sorry I didn't have a chance to look at this before you came back. Can you take this?
Flags: needinfo?(jdemooij)
Flags: needinfo?(nicolas.b.pierron) → needinfo?(hv1989)
Attached patch PatchSplinter Review
Assignee: nobody → hv1989
Flags: needinfo?(hv1989)
Attachment #8825864 - Flags: review?(nicolas.b.pierron)
Comment on attachment 8825864 [details] [diff] [review]
Patch

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

::: js/src/jit/CodeGenerator.cpp
@@ +11730,5 @@
> +        masm.printf("input: %d\n", input);
> +        masm.move32(Imm32(r->lower()), input);
> +        masm.printf("lowerbound: %d\n", input);
> +        masm.move32(Imm32(current->mir()->info().script()->lineno()), input);
> +        masm.printf("lineno: %d\n", input);

nit: maybe another patch?  Also, maybe we should make a variadic printf for this case, and start using all registers.
Attachment #8825864 - Flags: review?(nicolas.b.pierron) → review+
Comment on attachment 8825864 [details] [diff] [review]
Patch

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

Stupid question, but the Arguments of the baseline frame should also be guarded, how did the baseline frame got more than 4096 arguments it-self?
I think this patch might be incomplete.

::: js/src/jit/CodeGenerator.cpp
@@ +11730,5 @@
> +        masm.printf("input: %d\n", input);
> +        masm.move32(Imm32(r->lower()), input);
> +        masm.printf("lowerbound: %d\n", input);
> +        masm.move32(Imm32(current->mir()->info().script()->lineno()), input);
> +        masm.printf("lineno: %d\n", input);

nit: maybe another patch?  Also, maybe we should make a variadic printf for this case, and start using all registers.
(In reply to Nicolas B. Pierron [:nbp] from comment #8)
> Comment on attachment 8825864 [details] [diff] [review]
> Patch
> 
> Review of attachment 8825864 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Stupid question, but the Arguments of the baseline frame should also be
> guarded, how did the baseline frame got more than 4096 arguments it-self?
> I think this patch might be incomplete.

I didn't find any guard on baseline. All our guards are only in Ion. I'll open a follow-up bug to discuss this.

> ::: js/src/jit/CodeGenerator.cpp
> @@ +11730,5 @@
> > +        masm.printf("input: %d\n", input);
> > +        masm.move32(Imm32(r->lower()), input);
> > +        masm.printf("lowerbound: %d\n", input);
> > +        masm.move32(Imm32(current->mir()->info().script()->lineno()), input);
> > +        masm.printf("lineno: %d\n", input);
> 
> nit: maybe another patch?  Also, maybe we should make a variadic printf for
> this case, and start using all registers.

This is just extra debug information. They are only compiled in for DEBUG builds. I even think it is nice to have it here to give this bug some more entropy. Since this is quite clearly only used for DEBUG I also don't see the need to optimize masm.printf for multiple registers.
[Security approval request comment]
How easily could an exploit be constructed based on the patch?
Removed most of it. The big clue to range-analysis is not in the patch anymore and I updated the comment to indicate that we just don't want to support big argument lengths. That is as much as we can hide from the patch. I assume actually creating an exploit should be quite hard.

Do comments in the patch, the check-in comment, or tests included in the patch paint a bulls-eye on the security problem?
I removed as much as possible.

Which older supported branches are affected by this flaw?
All

If not all supported branches, which bug introduced the flaw?
/

Do you have backports for the affected branches? If not, how different, hard to create, and risky will they be?
I confirmed the code hasn't changed. As a result all branches can take the same patch. Even esr45.

How likely is this patch to cause regressions; how much testing does it need?
Could cause some performance regressions for scripts using arguments and funapply with more than 16 arguments. I assume that isn't used often. And we have the same restriction for most code, except this edge case
Attachment #8826114 - Flags: sec-approval?
Attachment #8826114 - Flags: review+
Attached patch Follow-up patch.Splinter Review
If everything is patched we can land this.
Attachment #8826115 - Flags: review+
Comment on attachment 8826114 [details] [diff] [review]
Fix only (with less info)

I didn't request approval for release, since beta is going to release soonish. As a result I don't think we will spin off another release build?

[Approval Request Comment (esr45) ]
If this is not a sec:{high,crit} bug, please state case for ESR consideration:
Out of bounds write/read on arbitrary arrays. I.e. Random write/read

User impact if declined:
Possibly 0-day exploits

Fix Landed on Version:
(Hopefully 51, not yet landed)

Risk to taking this patch (and alternatives if risky):
Not that risky. Small / easy well understood patch.

String or UUID changes made by this patch:
/

See https://wiki.mozilla.org/Release_Management/ESR_Landing_Process for more info.

Approval Request Comment (beta/aurora)
[Feature/Bug causing the regression]:
Long time ago (could be as far as FF28)

[User impact if declined]:
Possibly 0-day exploits

[Is this code covered by automated tests?]:
Yes, we have jsapi-tests and I confirmed this is fixed with the testcase.

[Has the fix been verified in Nightly?]:
Not yet. I requested sec-approval. But in order to speed-up landing already requesting beta/aurora approval.

[Needs manual test from QE? If yes, steps to reproduce]:
/

[List of other uplifts needed for the feature/fix]:
/

[Is the change risky?]:
Not that risky. Small / easy well understood patch.

[Why is the change risky/not risky?]:
There could be some performance degradation for funapply with 16+ arguments. Though that should be very uncommon.
And we already have such limits for other similar code.

[String changes made/needed]:
/
Attachment #8826114 - Flags: approval-mozilla-esr45?
Attachment #8826114 - Flags: approval-mozilla-beta?
Attachment #8826114 - Flags: approval-mozilla-aurora?
Waiting for sec approval before we land/uplift this. It may need to wait till the RC build on Monday though.
Attachment #8826114 - Flags: sec-approval? → sec-approval+
sec-approval+. I'll let Liz do branch approvals.
Comment on attachment 8826114 [details] [diff] [review]
Fix only (with less info)

Thanks Al, great, let's get this into aurora, beta, and ESR branches.
Attachment #8826114 - Flags: approval-mozilla-esr45?
Attachment #8826114 - Flags: approval-mozilla-esr45+
Attachment #8826114 - Flags: approval-mozilla-beta?
Attachment #8826114 - Flags: approval-mozilla-beta+
Attachment #8826114 - Flags: approval-mozilla-aurora?
Attachment #8826114 - Flags: approval-mozilla-aurora+
https://hg.mozilla.org/mozilla-central/rev/81159dae5644
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla53
Track 51+ as sec-critical.
Whiteboard: [jsbugmon:update] → [jsbugmon:update][post-critsmash-triage]
Whiteboard: [jsbugmon:update][post-critsmash-triage] → [jsbugmon:update][post-critsmash-triage][adv-main51+][adv-esr45.7+]
Group: javascript-core-security → core-security-release
Status: RESOLVED → VERIFIED
JSBugMon: This bug has been automatically verified fixed.
JSBugMon: This bug has been automatically verified fixed on Fx52
JSBugMon: This bug has been automatically verified fixed on Fx51
Group: core-security-release
Comment on attachment 8826115 [details] [diff] [review]
Follow-up patch.

Hannes, are you interested in formatting/landing a patch you made a year ago?
Flags: needinfo?(hv1989)
Assignee: hv1989 → nobody
Keywords: bugmon
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: