Closed
Bug 1303678
(CVE-2016-5297)
Opened 8 years ago
Closed 8 years ago
Assertion failure: args.length() <= ARGS_LENGTH_MAX
Categories
(Core :: JavaScript Engine, defect)
Core
JavaScript Engine
Tracking
()
People
(Reporter: anba, Assigned: Waldo)
Details
(Keywords: sec-high, Whiteboard: [post-critsmash-triage][adv-main50+][adv-esr45.5+])
Attachments
(4 files)
19.83 KB,
patch
|
arai
:
review+
abillings
:
sec-approval+
|
Details | Diff | Splinter Review |
19.80 KB,
patch
|
Waldo
:
review+
ritu
:
approval-mozilla-aurora+
ritu
:
approval-mozilla-beta+
|
Details | Diff | Splinter Review |
29.48 KB,
patch
|
arai
:
review+
ritu
:
approval-mozilla-esr45+
|
Details | Diff | Splinter Review |
2.13 KB,
patch
|
arai
:
review+
|
Details | Diff | Splinter Review |
Test case: --- new (function(){print(arguments.length)}.bind(null, ...Array(300000).fill(0)))(...Array(200000+1).fill(0)) --- Asserts with: --- Assertion failure: args.length() <= ARGS_LENGTH_MAX, at /home/andre/git/mozilla-central/js/src/vm/Interpreter.cpp:427 --- Back trace: --- #0 0x0000000000cf9714 in js::InternalCallOrConstruct (cx=0x7ffff6965000, args=..., construct=js::CONSTRUCT) at /home/andre/git/mozilla-central/js/src/vm/Interpreter.cpp:427 #1 0x0000000000cfa142 in InternalConstruct (cx=0x7ffff6965000, args=...) at /home/andre/git/mozilla-central/js/src/vm/Interpreter.cpp:548 #2 0x0000000000cfa49f in js::Construct (cx=0x7ffff6965000, fval=..., args=..., newTarget=..., objp=...) at /home/andre/git/mozilla-central/js/src/vm/Interpreter.cpp:597 #3 0x0000000000db76cb in intrinsic_ConstructFunction (cx=0x7ffff6965000, argc=3, vp=0x7ffff16142a0) at /home/andre/git/mozilla-central/js/src/vm/SelfHosting.cpp:1923 #4 0x0000000000d35afb in js::CallJSNative (cx=0x7ffff6965000, native=0xdb7311 <intrinsic_ConstructFunction(JSContext*, unsigned int, JS::Value*)>, args=...) at /home/andre/git/mozilla-central/js/src/jscntxtinlines.h:235 #5 0x0000000000cf9a0e in js::InternalCallOrConstruct (cx=0x7ffff6965000, args=..., construct=js::NO_CONSTRUCT) at /home/andre/git/mozilla-central/js/src/vm/Interpreter.cpp:455 #6 0x0000000000cf9d4f in InternalCall (cx=0x7ffff6965000, args=...) at /home/andre/git/mozilla-central/js/src/vm/Interpreter.cpp:500 #7 0x0000000000cf9d8d in js::CallFromStack (cx=0x7ffff6965000, args=...) at /home/andre/git/mozilla-central/js/src/vm/Interpreter.cpp:506 #8 0x0000000000d075d1 in Interpret (cx=0x7ffff6965000, state=...) at /home/andre/git/mozilla-central/js/src/vm/Interpreter.cpp:2919 #9 0x0000000000cf9667 in js::RunScript (cx=0x7ffff6965000, state=...) at /home/andre/git/mozilla-central/js/src/vm/Interpreter.cpp:401 #10 0x0000000000cf9ace in js::InternalCallOrConstruct (cx=0x7ffff6965000, args=..., construct=js::NO_CONSTRUCT) at /home/andre/git/mozilla-central/js/src/vm/Interpreter.cpp:473 #11 0x0000000000cf9d4f in InternalCall (cx=0x7ffff6965000, args=...) at /home/andre/git/mozilla-central/js/src/vm/Interpreter.cpp:500 #12 0x0000000000cf9d8d in js::CallFromStack (cx=0x7ffff6965000, args=...) at /home/andre/git/mozilla-central/js/src/vm/Interpreter.cpp:506 #13 0x000000000117bd50 in js::jit::DoCallFallback (cx=0x7ffff6965000, frame=0x7fffffffa6a8, stub_=0x7ffff69b88c0, argc=3, vp=0x7fffffffa618, res=...) at /home/andre/git/mozilla-central/js/src/jit/BaselineIC.cpp:5998 #14 ... ---
Reporter | ||
Comment 1•8 years ago
|
||
intrinsic_ConstructFunction(JSContext*, unsigned, Value*) [vm/SelfHosting.cpp] doesn't check for ARGS_LENGTH_MAX. Probably started with https://hg.mozilla.org/mozilla-central/rev/f29f1d9a3cd3 (bug 1000780).
Updated•8 years ago
|
Group: core-security → javascript-core-security
Assignee | ||
Comment 3•8 years ago
|
||
As I recall, the argument-length checking isn't centralized right now, which is pretty depressing. There was some dubious reason for this, that we should probably just kill. Will look into this, and the consequences.
Flags: needinfo?(jdemooij) → needinfo?(jwalden+bmo)
Assignee | ||
Comment 4•8 years ago
|
||
There was a theory that callers could check these limits based on local knowledge, then everyone wouldn't have to pay the cost. In practice this seems to break down, and I doubt all the callers touched here actually do such checking. Some do -- the Debugger uses of ARGS_LENGTH_MAX seem like such an instance -- but others don't (I bet the wasm case can be trivially made to assert). And it's only one comparison, so really it's just not worth it. Move the checking into the chokepoint. As far as the consequences...I'm not immediately sure. For this to be hit, we have to be able to *allocate* memory for all the values to put into the overlong arguments list. That tends to weed out most integer-overflow possibilities, like in str_fromCodePoint and str_fromCharCode. And setting those aside, callers generally aren't relying on the 500k limit being respected -- although some assert it as good practice, or to avoid checking when forwarding args from one call to another. I'd prefer we just played it safe and backported this to all open branches. Writing the patch this way has the very mild benefit of giving the would-be attacker reading this patch a bunch of call sites to sift through, so it's non-obvious what the vector into the now-failing case is. But there are few enough a careful-enough attacker should be able to figure it out. So, there's only so much this fix can obfuscate.
Attachment #8796423 -
Flags: review?(arai.unmht)
Assignee | ||
Updated•8 years ago
|
Assignee: nobody → jwalden+bmo
Status: NEW → ASSIGNED
Assignee | ||
Updated•8 years ago
|
Flags: needinfo?(jwalden+bmo)
Comment 5•8 years ago
|
||
Comment on attachment 8796423 [details] [diff] [review] Make {Invoke,Construct}Args::init check for too many arguments Review of attachment 8796423 [details] [diff] [review]: ----------------------------------------------------------------- so the testcase patch will be landed separately after the fix is released on all branches, right?
Attachment #8796423 -
Flags: review?(arai.unmht) → review+
Assignee | ||
Comment 6•8 years ago
|
||
Backport was easy, just had to s/ASCII// because the patch landed after bug 1289050 et al.
Assignee | ||
Comment 7•8 years ago
|
||
Mostly simple porting work required, but there are enough additional callers of these functions that I'm going to play it safe and get another stamp.
Attachment #8798298 -
Flags: review?(arai.unmht)
Updated•8 years ago
|
Attachment #8798298 -
Flags: review?(arai.unmht) → review+
Updated•8 years ago
|
status-firefox49:
--- → wontfix
status-firefox50:
--- → affected
status-firefox52:
--- → affected
status-firefox-esr45:
--- → affected
Assignee | ||
Comment 8•8 years ago
|
||
Just to note for future reference, this test only is an effective test with self-hosted Function.prototype.bind. I don't remember how far back that goes, so if anyone tries this test on branches, its success doesn't mean this bug's failure is present. (It might or might not be possible to write effective branch tests, but it'd depend whether there were some other unguarded pathway to the assertion, which isn't a sure thing.)
Attachment #8798301 -
Flags: review?(arai.unmht)
Assignee | ||
Comment 9•8 years ago
|
||
Comment on attachment 8796423 [details] [diff] [review] Make {Invoke,Construct}Args::init check for too many arguments [Security approval request comment] > How easily could an exploit be constructed based on the patch? It's not clear if this is exploitable, exactly. I had difficulty tracking exactly where overlong args would flow, and I'm not sure whether there are any callers that would go wrong from the absence of the check added here. But it's a bounds-checking sort of violation -- so better to take a reasonably simple fix (that we can know handles all the cases because of C++ type-checking) than take a risk. *If* it's exploitable, the patch has a moderate number of places changed, in its course, that are *not* problematic, as camouflage. So there's that going for it. > Do comments in the patch, the check-in comment, or tests included > in the patch paint a bulls-eye on the security problem? No. (And as usual, tests will land sometime after the fix has gone into releases and percolated to users.) > Which older supported branches are affected by this flaw? > If not all supported branches, which bug introduced the flaw? Everything we still support. The exact trigger stumbled across here, tho, dates to self-hosting of Function.prototype.bind in bug 1000780. But other triggers lurking in the tree, or in branches, could go back further. > Do you have backports for the affected branches? If not, > how different, hard to create, and risky will they be? Backports attached. Aurora/beta backport trivial, esr45 backport only required a few more mechanical changes. > How likely is this patch to cause regressions; how much > testing does it need? Pretty unlikely, needs little. The meat of the patch is an integer comparison and error-report that any SpiderMonkey hacker could correctly write while sleeping.
Attachment #8796423 -
Flags: sec-approval?
Assignee | ||
Comment 10•8 years ago
|
||
Comment on attachment 8798297 [details] [diff] [review] Aurora/beta backport Approval Request Comment [Feature/regressing bug #]: bug 1000780, but the ultimate issue goes back further, and other code might manifest it even before that bug was fixed [User impact if declined]: uncertainty about what happens if a function call is permitted with more than ARGS_LENGTH_MAX arguments -- might show up as a bounds-checking issue, always worrisome [Describe test coverage new/current, TreeHerder]: test attached to bug [Risks and why]: very low -- C++'s type system ensures we fixed all the right places, and the meat of the patch is only a few super-simple lines [String/UUID change made/needed]: N/A
Attachment #8798297 -
Flags: review+
Attachment #8798297 -
Flags: approval-mozilla-beta?
Attachment #8798297 -
Flags: approval-mozilla-aurora?
Assignee | ||
Comment 11•8 years ago
|
||
Comment on attachment 8798298 [details] [diff] [review] esr45 backport [Approval Request Comment] If this is not a sec:{high,crit} bug, please state case for ESR consideration: It might be sec-high/critical, but the multiplicity of code affected makes it hard to know. Given that the fix is simple and all of a few lines modifying one function -- plus mechanical changes to all the places that call that function, that the compiler will yell at us til we've fixed them all -- it seems safer taking this than living without it. User impact if declined: Possible security issues, but we don't know for sure. Fix Landed on Version: trunk now, i.e. 52 once I get approval. Risk to taking this patch (and alternatives if risky): Little; the changes are mostly mechanical, and the non-mechanical bits any SpiderMonkey hacker could correctly write sleeping. String or UUID changes made by this patch: N/A
Attachment #8798298 -
Flags: approval-mozilla-esr45?
Comment 12•8 years ago
|
||
Comment on attachment 8798301 [details] [diff] [review] Test Review of attachment 8798301 [details] [diff] [review]: ----------------------------------------------------------------- So, this testcase will be landed only to m-c, right? This test fails on patched esr45 on OSX jsshell, as it throws "InternalError: allocation size overflow" instead of RangeError.
Attachment #8798301 -
Flags: review?(arai.unmht) → review+
Updated•8 years ago
|
tracking-firefox50:
--- → +
tracking-firefox51:
--- → +
tracking-firefox52:
--- → +
tracking-firefox-esr45:
--- → 50+
Keywords: sec-high
Comment 13•8 years ago
|
||
Comment on attachment 8796423 [details] [diff] [review] Make {Invoke,Construct}Args::init check for too many arguments sec-approval+ for trunk. I'll let release management approve branches here but we should take it.
Attachment #8796423 -
Flags: sec-approval? → sec-approval+
Comment on attachment 8798297 [details] [diff] [review] Aurora/beta backport Sec-high, Aurora51+, Beta50+
Attachment #8798297 -
Flags: approval-mozilla-beta?
Attachment #8798297 -
Flags: approval-mozilla-beta+
Attachment #8798297 -
Flags: approval-mozilla-aurora?
Attachment #8798297 -
Flags: approval-mozilla-aurora+
Comment on attachment 8798298 [details] [diff] [review] esr45 backport Sec-high, ESR45+
Attachment #8798298 -
Flags: approval-mozilla-esr45? → approval-mozilla-esr45+
https://hg.mozilla.org/releases/mozilla-aurora/rev/3a479252364d https://hg.mozilla.org/releases/mozilla-beta/rev/6b3607656acc
Comment 17•8 years ago
|
||
https://hg.mozilla.org/releases/mozilla-esr45/rev/46b07bdbf8b2 Jeff, this never landed on trunk and now the patch doesn't apply cleanly. Can you please rebase and land it when you get a chance?
Assignee | ||
Comment 18•8 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/b08857e08eb8
Flags: needinfo?(jwalden+bmo)
Comment 19•8 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/b08857e08eb8
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla52
Updated•8 years ago
|
Group: javascript-core-security → core-security-release
Updated•8 years ago
|
Flags: qe-verify+
Whiteboard: [post-critsmash-triage]
Updated•8 years ago
|
Whiteboard: [post-critsmash-triage] → [post-critsmash-triage][adv-main50+][adv-esr45.5+]
Comment 20•8 years ago
|
||
Reproduced the assertion on nightly 2016-09-19 debug build, Ubuntu 14.04 x64. Now, on 52.0a1 (2016-11-08), 51.0a2 (2016-11-08), 50.0b12 debug builds, after executing the testcase in comment 0 in the webconsole, I get: "RangeError: too many arguments provided for a function call". On ESR 45.5.0 debug build I get: "InternalError: allocation size overflow". Is this expected?
Flags: needinfo?(jwalden+bmo)
Comment 21•8 years ago
|
||
(In reply to Paul Silaghi, QA [:pauly] from comment #20) > Reproduced the assertion on nightly 2016-09-19 debug build, Ubuntu 14.04 x64. > Now, on 52.0a1 (2016-11-08), 51.0a2 (2016-11-08), 50.0b12 debug builds, > after executing the testcase in comment 0 in the webconsole, I get: > "RangeError: too many arguments provided for a function call". > On ESR 45.5.0 debug build I get: "InternalError: allocation size overflow". > Is this expected? Yes, that is expected. The issue here was precisely that we didn't properly handle too many arguments being passed to a function call under some circumstances. "Properly handling" in this case means throwing an error, because (by definition) we can't actually perform a successful function call with this amount of arguments.
Flags: needinfo?(jwalden+bmo)
Updated•8 years ago
|
Status: RESOLVED → VERIFIED
Flags: qe-verify+
Updated•8 years ago
|
Alias: CVE-2016-5297
Updated•8 years ago
|
Group: core-security-release
Comment 22•7 years ago
|
||
Pushed by jwalden@mit.edu: https://hg.mozilla.org/integration/mozilla-inbound/rev/88d9d20ee497 Add a test for constructing a bound function whose bound-target construct operation passes more than ARGS_LENGTH_MAX arguments. r=arai
Comment 23•7 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/88d9d20ee497
You need to log in
before you can comment on or make changes to this bug.
Description
•