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)
Tracking
()
People
(Reporter: decoder, Unassigned, NeedInfo)
Details
(4 keywords, Whiteboard: [jsbugmon:update][post-critsmash-triage][adv-main51+][adv-esr45.7+])
Attachments
(3 files)
2.97 KB,
patch
|
nbp
:
review+
|
Details | Diff | Splinter Review |
1.20 KB,
patch
|
h4writer
:
review+
lizzard
:
approval-mozilla-aurora+
lizzard
:
approval-mozilla-beta+
lizzard
:
approval-mozilla-esr45+
abillings
:
sec-approval+
|
Details | Diff | Splinter Review |
2.80 KB,
patch
|
h4writer
:
review+
|
Details | Diff | Splinter Review |
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.
Updated•7 years ago
|
Whiteboard: [jsbugmon:update,bisect] → [jsbugmon:update]
Comment 1•7 years ago
|
||
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
Comment 2•7 years ago
|
||
Bisection is not very useful, it's probably exposed by self-hosting bind. NI nbp because RA.
Flags: needinfo?(nicolas.b.pierron)
Comment 4•7 years ago
|
||
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?
Updated•7 years ago
|
Flags: needinfo?(hv1989)
Priority: -- → P1
Comment 5•7 years ago
|
||
Sorry I didn't have a chance to look at this before you came back. Can you take this?
Flags: needinfo?(jdemooij)
Updated•7 years ago
|
Flags: needinfo?(nicolas.b.pierron) → needinfo?(hv1989)
Comment 6•7 years ago
|
||
Assignee: nobody → hv1989
Flags: needinfo?(hv1989)
Attachment #8825864 -
Flags: review?(nicolas.b.pierron)
Updated•7 years ago
|
status-firefox50:
--- → affected
status-firefox51:
--- → affected
status-firefox52:
--- → affected
status-firefox-esr45:
--- → affected
Updated•7 years ago
|
tracking-firefox53:
--- → +
Comment 7•7 years ago
|
||
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 8•7 years ago
|
||
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.
Comment 9•7 years ago
|
||
(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.
Comment 10•7 years ago
|
||
[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+
Comment 11•7 years ago
|
||
If everything is patched we can land this.
Attachment #8826115 -
Flags: review+
Updated•7 years ago
|
Keywords: leave-open
Comment 12•7 years ago
|
||
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?
Comment 13•7 years ago
|
||
Waiting for sec approval before we land/uplift this. It may need to wait till the RC build on Monday though.
Updated•7 years ago
|
Attachment #8826114 -
Flags: sec-approval? → sec-approval+
Comment 14•7 years ago
|
||
sec-approval+. I'll let Liz do branch approvals.
Comment 15•7 years ago
|
||
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+
Comment 16•7 years ago
|
||
Pushed to inbound as discussed with RyanVM: https://hg.mozilla.org/integration/mozilla-inbound/rev/81159dae56440e1f412656b7f927d4c503d05384
Comment 17•7 years ago
|
||
uplift |
https://hg.mozilla.org/releases/mozilla-aurora/rev/21a5859e934a https://hg.mozilla.org/releases/mozilla-beta/rev/f4fd63a8302c https://hg.mozilla.org/releases/mozilla-esr45/rev/0617dd4b444d
https://hg.mozilla.org/mozilla-central/rev/81159dae5644
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla53
Updated•7 years ago
|
Updated•7 years ago
|
Whiteboard: [jsbugmon:update] → [jsbugmon:update][post-critsmash-triage]
Updated•7 years ago
|
Whiteboard: [jsbugmon:update][post-critsmash-triage] → [jsbugmon:update][post-critsmash-triage][adv-main51+][adv-esr45.7+]
Updated•7 years ago
|
Group: javascript-core-security → core-security-release
Updated•7 years ago
|
Comment 20•7 years ago
|
||
JSBugMon: This bug has been automatically verified fixed. JSBugMon: This bug has been automatically verified fixed on Fx52
Updated•7 years ago
|
Comment 21•7 years ago
|
||
JSBugMon: This bug has been automatically verified fixed on Fx51
Updated•7 years ago
|
Group: core-security-release
Updated•6 years ago
|
Keywords: leave-open
Comment 22•6 years ago
|
||
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)
You need to log in
before you can comment on or make changes to this bug.
Description
•