MTypedArrayLength shouldn't assume the typed array length is immutable

VERIFIED FIXED in Firefox 30

Status

()

VERIFIED FIXED
5 years ago
4 years ago

People

(Reporter: jandem, Assigned: Waldo)

Tracking

({sec-critical})

unspecified
mozilla32
sec-critical
Points:
---
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(firefox29 wontfix, firefox30+ verified, firefox31+ verified, firefox32 verified, firefox-esr2430+ verified, b2g-v1.2 fixed, b2g-v1.3 fixed, b2g-v1.3T fixed, b2g-v1.4 fixed, b2g-v2.0 fixed, seamonkey2.26 wontfix)

Details

(Whiteboard: [adv-main30+][adv-esr24.6+])

Attachments

(4 attachments)

(Reporter)

Description

5 years ago
Posted file 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.
(Reporter)

Comment 1

5 years ago
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.
Flags: needinfo?(jwalden+bmo)
(Reporter)

Comment 2

5 years ago
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.
(Assignee)

Comment 3

5 years ago
> 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.
Assignee: jdemooij → jwalden+bmo
Flags: needinfo?(jwalden+bmo)
(Assignee)

Comment 4

5 years ago
Posted patch TestSplinter Review
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).
Attachment #8423603 - Flags: review?(jdemooij)
(Assignee)

Comment 5

5 years ago
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)?
Attachment #8423604 - Flags: review?(jdemooij)
(Reporter)

Comment 6

5 years ago
(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!
(Reporter)

Updated

5 years ago
Attachment #8423603 - Flags: review?(jdemooij) → review+
(Reporter)

Comment 7

5 years ago
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.
Attachment #8423604 - Flags: review?(jdemooij) → review+
(Assignee)

Comment 8

5 years ago
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.
Attachment #8423604 - Flags: sec-approval?
status-b2g-v1.2: --- → affected
status-b2g-v1.3: --- → affected
status-b2g-v1.3T: --- → affected
status-b2g-v1.4: --- → affected
status-b2g-v2.0: --- → affected
status-firefox29: --- → wontfix
status-firefox30: --- → affected
status-firefox31: --- → affected
status-firefox32: --- → affected
status-firefox-esr24: --- → affected
Keywords: sec-critical
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.
Attachment #8423604 - Flags: sec-approval? → sec-approval+
Group: javascript-core-security
Checkin plz?
Flags: needinfo?(jwalden+bmo)
(Assignee)

Comment 11

5 years ago
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.
Flags: needinfo?(jwalden+bmo)
(Assignee)

Comment 12

5 years ago
Backed out, then relanded with CLOBBER:

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

Comment 13

5 years ago
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
Attachment #8429709 - Flags: review+
Attachment #8429709 - Flags: approval-mozilla-aurora?
(Assignee)

Comment 14

5 years ago
Comment on attachment 8429709 [details] [diff] [review]
Aurora backport

Again backports to beta easily.

(And again, please leave landing to me.)
Attachment #8429709 - Flags: approval-mozilla-beta?
https://hg.mozilla.org/mozilla-central/rev/57b0932e2f06
Status: ASSIGNED → RESOLVED
Last Resolved: 5 years ago
status-b2g-v2.0: affected → fixed
status-firefox32: affected → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla32
Given the severity and ease of backporting, can you please create a branch patch and nom for inclusion in ESR24 as well?
Flags: needinfo?(jwalden+bmo)
tracking-firefox30: --- → +
tracking-firefox31: --- → +
tracking-firefox-esr24: --- → 30+
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.
Attachment #8429709 - Flags: approval-mozilla-beta?
Attachment #8429709 - Flags: approval-mozilla-beta+
Attachment #8429709 - Flags: approval-mozilla-aurora?
Attachment #8429709 - Flags: approval-mozilla-aurora+
(Assignee)

Updated

5 years ago
status-firefox30: affected → fixed
status-firefox31: affected → fixed
https://hg.mozilla.org/releases/mozilla-b2g30_v1_4/rev/4fd82c282519
status-b2g-v1.4: affected → fixed
Flags: in-testsuite?
(Assignee)

Comment 20

5 years ago
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.
status-b2g-v1.3T: affected → fixed
(Assignee)

Comment 22

5 years ago
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).
Attachment #8429709 - Flags: approval-mozilla-esr24?
Comment on attachment 8429709 [details] [diff] [review]
Aurora backport

ESR24 approval granted.
Attachment #8429709 - Flags: approval-mozilla-esr24? → approval-mozilla-esr24+
(Assignee)

Comment 24

5 years ago
https://hg.mozilla.org/releases/mozilla-esr24/rev/8c406adf76a0
status-firefox-esr24: affected → fixed
Group: javascript-core-security
Whiteboard: [adv-main30+][adv-esr24.6+]
Confirmed problem on 2014-04-04, Fx30.
Verified fixed on 2014-06-03. Fx24.6.0esr, Fx30, Fx31 and Fx32.
Status: RESOLVED → VERIFIED
status-firefox30: fixed → verified
status-firefox31: fixed → verified
status-firefox32: fixed → verified
status-firefox-esr24: fixed → verified
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.
status-seamonkey2.26: --- → wontfix
Group: core-security
You need to log in before you can comment on or make changes to this bug.