1303678 - (CVE-2016-5297) Assertion failure: args.length() <= ARGS_LENGTH_MAX

Status: VERIFIED FIXED

(Core :: JavaScript Engine, defect)

Description

[André Bargull [:anba]]

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

Comment 1

[André Bargull [:anba]]

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

Comment 2

[Matt Wobensmith [:mwobensmith][:matt:]]

Hi Jan, can you comment on the severity of this?

Comment 3

[Jeff Walden [:Waldo]]

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.

Comment 4

[Jeff Walden [:Waldo]]

Attachment: Make {Invoke,Construct}Args::init check for too many arguments

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.

Comment 5

[Tooru Fujisawa [:arai]]

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?

Comment 6

[Jeff Walden [:Waldo]]

Attachment: Aurora/beta backport

Backport was easy, just had to s/ASCII// because the patch landed after bug 1289050 et al.

Comment 7

[Jeff Walden [:Waldo]]

Attachment: esr45 backport

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.

Comment 8

[Jeff Walden [:Waldo]]

Attachment: Test

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

Comment 9

[Jeff Walden [:Waldo]]

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.

Comment 10

[Jeff Walden [:Waldo]]

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

Comment 11

[Jeff Walden [:Waldo]]

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

Comment 12

[Tooru Fujisawa [:arai]]

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.

Comment 13

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

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.

Comment 14

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

Comment on attachment 8798297 [details] [diff] [review]
Aurora/beta backport

Sec-high, Aurora51+, Beta50+

Comment 15

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

Comment on attachment 8798298 [details] [diff] [review]
esr45 backport

Sec-high, ESR45+

Comment 16

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/releases/mozilla-aurora/rev/3a479252364d
https://hg.mozilla.org/releases/mozilla-beta/rev/6b3607656acc

Comment 17

[Ryan VanderMeulen [:RyanVM]]

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?

Comment 18

[Jeff Walden [:Waldo]]

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

Comment 19

[Carsten Book [:Tomcat]]

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

Comment 20

[Paul Silaghi, QA [:pauly]]

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?

Comment 21

[Till Schneidereit [:till]]

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

Comment 22

[Pulsebot]

Pushed by [e-mail]:
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

[Andreea Pavel [:apavel]]

https://hg.mozilla.org/mozilla-central/rev/88d9d20ee497