1011007 - MTypedArrayLength shouldn't assume the typed array length is immutable

Status: VERIFIED FIXED

(Core :: JavaScript Engine: JIT, defect)

Description

[Jan de Mooij [:jandem]]

Attachment: Testcase

Attached testcase fails in the shell:

$ ./js --ion-parallel-compile=off ~/dev/test.js
test.js:12:1 Error: Assertion failed: got 1000, expected 0

MTypedArrayLength::getAliasSet() assumes a typed array's length is immutable, so LICM hoists it out of the loop.

The bounds check can be hoisted too, but I can't get it to crash (it returns 0); probably because the ArrayBuffer data pointer always points to a chunk of memory large enough for the old length. MTypedArrayElements is safe, so changing the data pointer itself is fine as long as it's pointing to a chunk of memory that's big enough.

Comment 1

[Jan de Mooij [:jandem]]

Waldo, is hoisting the typed array length + bounds checks just a correctness issue now or is it still exploitable?

As I mentioned in comment 0, the data pointer is not hoisted, just the length + bounds checks.

Comment 2

[Jan de Mooij [:jandem]]

This is only a problem for typed arrays that don't have singleton type. Big typed arrays have singleton type AFAIK, and in that case we bake in the length + data pointer and add invalidation constraints; we don't do that for non-singleton arrays.

Comment 3

[Jeff Walden [:Waldo]]

> changing the data pointer itself is fine as long as it's pointing to a
> chunk of memory that's big enough

That's the case right now because of bug 982974's backstop.  Neutered buffers are (if their data switches, which it doesn't always) given new data of equivalent length to the old, precisely to attempt to deal with out-of-bounds accesses present in the current code, without a full fix for bug 982974.  It doesn't work always, as bug 991981 shows, but it *does* work here, where the elements are up-to-date because loaded directly from the object.

This backstopping is memory-hogging and relatively stupid.  We'll kill it after bug 991981 is fixed (perhaps a cycle or two later for baking/safety, not that the timing matters here).  In the meantime, I...think, in light of the backstop, this is not an exploitable issue.  But I wouldn't say I'm nearly confident enough of my understanding of all this, or certainty in it, to rely on that and not fix it this cycle.

You should be aware of bug 999651 and bug 995679, if you're not already.  Of course those only apply to the cases where we can flat-out inline a constant, but it's more background info on this typed-array stuff being buggy in one case or another.

I imagine this is solvable with another AliasSet flag and all that, as the orthodox approach.  I'll write the patch, because I want to.  :-)  Expect it later tonight.

Comment 4

[Jeff Walden [:Waldo]]

Attachment: Test

By the way, because you passed "same-data" to neuter in your original testcase, you should *expect* to read zeroes out of the array rather than crash.  Had you passed "change-data", you would then have been loading from the backstopped memory, which would turn into a null dereference if we removed backstopping (but won't because we'll fix bug 991981 first).

Comment 5

[Jeff Walden [:Waldo]]

Attachment: Give MTypedArrayLength an AliasSet with a new TypedArrayLength flag

And the simple patch.

I take it we don't try super-hard to remove new alias flags from existing alias sets, where possible, when adding them (in the interests of micro-perf gains)?

Comment 6

[Jan de Mooij [:jandem]]

(In reply to Jeff Walden [:Waldo] (remove +bmo to email) from comment #4)
> By the way, because you passed "same-data" to neuter in your original
> testcase, you should *expect* to read zeroes out of the array rather than
> crash.  Had you passed "change-data", you would then have been loading from
> the backstopped memory, which would turn into a null dereference if we
> removed backstopping (but won't because we'll fix bug 991981 first).

I tried both "same-data" and "change-data"; same-data is probably the last I tried :)

Thanks for taking this!

Comment 7

[Jan de Mooij [:jandem]]

Comment on attachment 8423604 [details] [diff] [review]
Give MTypedArrayLength an AliasSet with a new TypedArrayLength flag

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

Thanks!

(In reply to Jeff Walden [:Waldo] (remove +bmo to email) from comment #5)
> I take it we don't try super-hard to remove new alias flags from existing
> alias sets, where possible, when adding them (in the interests of micro-perf
> gains)?

Most operations that have the default flags (all flags) are things like calls/ICs that can do arbitrary things; it's hard to optimize that without special casing every Native and unlikely to be a measurable win (the most interesting operations are inlined already). We could add more categories (like you're doing here), I think eventually we'll want that for things like dense Length/InitializedLength etc.

Comment 8

[Jeff Walden [:Waldo]]

Comment on attachment 8423604 [details] [diff] [review]
Give MTypedArrayLength an AliasSet with a new TypedArrayLength flag

[Security approval request comment]
> How easily could an exploit be constructed based on the patch?
I don't think it can -- the backstop added in bug 982974 defends against this being a problem.  But if we missed a branch, or didn't ship the corresponding security update to someone (say, an older b2g user), it's sec-critical for them, difficulty corresponding to the difficulty of exploiting any out-of-bounds write, more or less.

> Do comments in the patch, the check-in comment, or tests
> included in the patch paint a bulls-eye on the security problem?
Not squarely.  Someone familiar with the aliasing concept could probably work backwards from this patch to the issue implicated here.

> Which older supported branches are affected by this flaw?
> If not all supported branches, which bug introduced the flaw?
All of them.  And as noted earlier, anyone that didn't get bug 982974's patch can consider this sec-critical.

> Do you have backports for the affected branches? If not,
> how different, hard to create, and risky will they be?
Haven't tried, should be easy.

> How likely is this patch to cause regressions; how much
> testing does it need?
Should be safe with fairly little testing.  We have the testcase in hand here to test with, too, tho I wouldn't land it immediately, just in case.

Comment 9

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

Comment on attachment 8423604 [details] [diff] [review]
Give MTypedArrayLength an AliasSet with a new TypedArrayLength flag

sec-approval+ for trunk. Let's not land any test yet though.

Comment 10

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

Checkin plz?

Comment 11

[Jeff Walden [:Waldo]]

Landed as part of a mega-rollup change for slight obfuscatory purposes:

https://hg.mozilla.org/integration/mozilla-inbound/rev/2c631967ab9e

Backports happening, hopefully aurora/beta by EOD today.

Comment 12

[Jeff Walden [:Waldo]]

Backed out, then relanded with CLOBBER:

https://hg.mozilla.org/integration/mozilla-inbound/rev/57b0932e2f06

Comment 13

[Jeff Walden [:Waldo]]

Attachment: Aurora backport

Still no patch-landing help wanted.

[Approval Request Comment]
Bug caused by (feature/regressing bug #): introduction of neutering
User impact if declined: accesses to the "ballast" allocated for neutered buffers that was added by bug 982974; *appears* to not be a security issue, strictly, but extremely dodgy
Testing completed (on m-c, etc.): landed in m-c, test here verified against m-c, used to verify against aurora as well
Risk to taking this patch (and alternatives if risky): pretty little, AliasSet stuff is conservative about what can alias what (the problem here being we explicitly said no aliasing happened, which was a lie)
String or IDL/UUID changes made by this patch: N/A

Comment 14

[Jeff Walden [:Waldo]]

Comment on attachment 8429709 [details] [diff] [review]
Aurora backport

Again backports to beta easily.

(And again, please leave landing to me.)

Comment 15

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/57b0932e2f06

Comment 16

[Lawrence Mandel [:lmandel] (use needinfo)]

Given the severity and ease of backporting, can you please create a branch patch and nom for inclusion in ESR24 as well?

Comment 17

[Lukas Blakk [:lsblakk] use ?needinfo]

Comment on attachment 8429709 [details] [diff] [review]
Aurora backport

Not sure why this sec-critical issue wasn't being tracked, adding flags to keep this on our radar and for ESR too.

Comment 18

[Jeff Walden [:Waldo]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/c5a58d90da90
https://hg.mozilla.org/releases/mozilla-beta/rev/4fd82c282519

b2g/esr24 backports shortly.

Comment 19

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-b2g30_v1_4/rev/4fd82c282519

Comment 20

[Jeff Walden [:Waldo]]

And *now* they tell me b2g28/b2g26 are open for landings in sec-high/sec-critical bugs without explicit approval.  :-)  Landings to happen super-shortly.

Comment 21

[Jeff Walden [:Waldo]]

https://hg.mozilla.org/releases/mozilla-b2g28_v1_3/rev/57cd741e4d0b
https://hg.mozilla.org/releases/mozilla-b2g26_v1_2/rev/4c359ff76654

Comment 22

[Jeff Walden [:Waldo]]

Comment on attachment 8429709 [details] [diff] [review]
Aurora backport

Patch doesn't apply to ESR24, but the concept is exactly the same and all, so let's just flag this again.

Comment 13 approval comments all apply here (plus the patch is now on all branches newer than 24).

Comment 23

[Lawrence Mandel [:lmandel] (use needinfo)]

Comment on attachment 8429709 [details] [diff] [review]
Aurora backport

ESR24 approval granted.

Comment 24

[Jeff Walden [:Waldo]]

https://hg.mozilla.org/releases/mozilla-esr24/rev/8c406adf76a0

Comment 25

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

Confirmed problem on 2014-04-04, Fx30.
Verified fixed on 2014-06-03. Fx24.6.0esr, Fx30, Fx31 and Fx32.

Comment 26

[Justin Wood (:Callek)]

trying to apply this to SeaMonkey 2.26.1 (Gecko 29) resulted in patch conflicts, and due to the nature of this patchset it seems like I won't be able to take it.

Comment 27

[Carsten Book [:Tomcat]]

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