Closed Bug 1009952 Opened 10 years ago Closed 10 years ago

Loads/stores from "static" typed array objects don't respond well to neutering of the underlying buffer

Categories

(Core :: JavaScript Engine: JIT, defect)

x86
All
defect
Not set
critical

Tracking

()

RESOLVED FIXED
mozilla32
Tracking Status
firefox29 --- wontfix
firefox30 + fixed
firefox31 + fixed
firefox32 + fixed
firefox-esr24 30+ fixed
b2g-v1.2 --- fixed
b2g-v1.3 --- fixed
b2g-v1.3T --- fixed
b2g-v1.4 --- fixed
b2g-v2.0 --- fixed
seamonkey2.26 --- wontfix

People

(Reporter: Waldo, Assigned: Waldo)

Details

(Keywords: sec-critical, Whiteboard: [adv-main30+][adv-esr24.6+][qa-])

Attachments

(4 files, 1 obsolete file)

La la la la la...

Found while auditing bug 991981.  The *second* time.  (That is, skimming the patch that was the result of the audit, a second time, prior to posting of one part of that patch.)  Better late than never!

This is an out-of-bounds read/write access -- may require some care to get memory overlaid just right, but this is sec-critical territory.  Spot-checking esr24 suggests this is a problem on all branches.

Interestingly this is x86-32 only -- I think because we do this optimization only when we can efficiently burn pointers into JIT code.  The actual rationale is not recorded in source right now.
Attachment #8422102 - Flags: review?(sphink)
Attached patch Patch (obsolete) — Splinter Review
Attachment #8422106 - Flags: review?(sphink)
Attachment #8422106 - Flags: review?(shu)
Comment on attachment 8422106 [details] [diff] [review]
Patch

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

Looks good to me.

::: js/src/jit/IonBuilder.cpp
@@ +7081,2 @@
>      obj->setImplicitlyUsedUnchecked();
> +

Style nit here and below: I'd group the two setImplicitlyUsedUnchecked calls together and have an empty line after the watchStateChangeForTypedArrayData call.
Attachment #8422106 - Flags: review?(shu) → review+
Comment on attachment 8422106 [details] [diff] [review]
Patch

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

I don't know enough about the jit to really say anything meaningful. But wow, this seems dangerous -- there isn't even a pinch point where your renaming would point this out.

Would it be worth adding one? For example, perhaps the MStoreTypedArrayElementStatic should not accept a plain TypedArrayObject* in its constructor. It could take a GuardedTypedArrayObject&, which you construct with a TypeObjectKey* and it calls watchStateChangeBlah in its constructor. Or at least have MStoreTypedArrayElementStatic take the TypeObjectKey* in its constructor and do the watchStateChange there?

And maybe something similar for the non-static ones? Probably easiest to forget about GuardedTypedArrayObject, and just pass in the TypeObjectKey* in addition to the MDefinition* elements. Oh, except you often don't have one; it looks like there's some magic bit set by 'setMovable' that means you won't need a guard(?). Maybe that path isn't worth bothering with. Oh -- it looks like that one would be best be done by modifying the MConstantElements constructor to ensure a guard.

I dunno, it's not really for this bug, but this stuff looks very sharp and pointy to me. It seems like if you're accepting a bare pointer to a typed array, you ought to make your caller prove to you that it's ok to use that pointer.

r=me for fixing the immediate problem here. MIR.h doesn't have anything else that takes a TypedArrayObject*. But I suppose it's not that pointer that's the problem, it's what you can access through that pointer (the length and data).

There are some similar-smelling RegExpObject*s flying around, and lots of JSObject*s.
Attachment #8422106 - Flags: review?(sphink) → review+
Comment on attachment 8422102 [details] [diff] [review]
Tests for MStoreTypedArrayElementStatic/MLoadTypedArrayElementStatic usage being unguarded by constraints

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

This crashes without the watchStateChange? Why? I thought you'd have to put something at that location, then check to be sure you're getting the right value back. (Or that your stores had an effect.) Or is there some assert that notices what's going on?

How hard would it be to test the |if (tarrType->unknownProperties())| part?
Flags: needinfo?(jwalden+bmo)
(In reply to Steve Fink [:sfink] from comment #3)
> Comment on attachment 8422106 [details] [diff] [review]
> Patch
> 
> Review of attachment 8422106 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> I don't know enough about the jit to really say anything meaningful. But
> wow, this seems dangerous -- there isn't even a pinch point where your
> renaming would point this out.
> 
> Would it be worth adding one? For example, perhaps the
> MStoreTypedArrayElementStatic should not accept a plain TypedArrayObject* in
> its constructor. It could take a GuardedTypedArrayObject&, which you
> construct with a TypeObjectKey* and it calls watchStateChangeBlah in its
> constructor. Or at least have MStoreTypedArrayElementStatic take the
> TypeObjectKey* in its constructor and do the watchStateChange there?
> 
> And maybe something similar for the non-static ones? Probably easiest to
> forget about GuardedTypedArrayObject, and just pass in the TypeObjectKey* in
> addition to the MDefinition* elements. Oh, except you often don't have one;
> it looks like there's some magic bit set by 'setMovable' that means you
> won't need a guard(?). Maybe that path isn't worth bothering with. Oh -- it
> looks like that one would be best be done by modifying the MConstantElements
> constructor to ensure a guard.
> 
> I dunno, it's not really for this bug, but this stuff looks very sharp and
> pointy to me. It seems like if you're accepting a bare pointer to a typed
> array, you ought to make your caller prove to you that it's ok to use that
> pointer.
> 
> r=me for fixing the immediate problem here. MIR.h doesn't have anything else
> that takes a TypedArrayObject*. But I suppose it's not that pointer that's
> the problem, it's what you can access through that pointer (the length and
> data).
> 
> There are some similar-smelling RegExpObject*s flying around, and lots of
> JSObject*s.

I'm not fully understanding the concern here, partially due to language I don't understand (pinch point?). Is the concern here one of how do we guarantee that the TypedArrayObject will remain constant? That's taken care of by type barriers elsewhere, and is no more scary than other pointers we bake in to the jitcode.

The watchStateChange is needed because it's an assumption at a finer granularity than just TI types for TypedArrayObjects. For a normal JS object, we also barrier on the types of its fields. But since TypedArrayObjects are non-native, we don't track their field types, otherwise we could just treat the .data and .length fields as any other property, and can think of neutering as setting them to null.
(In reply to Steve Fink [:sfink] from comment #4)
> How hard would it be to test the |if (tarrType->unknownProperties())| part?

I don't know a huge amount about that part of it -- that just derives from what's currently (or currently after patches I have in flight) done elsewhere for typed array data-movement/truncation defenses.

I just tried to see if I could get it hit right now.  I didn't have any success.  There aren't many ways to get that condition in place in isolation of this code.  Using the object as a prototype is the biggest one, but that seems to prevent compilation at some even-earlier phase than the method I changed, so I didn't even enter IonBuilder::jsop_setelem at all.  There might be a way, but I suspect it's not worth my time to figure it out, not when the correctness of checking and bailing on that condition is relatively clear in its own right.
Flags: needinfo?(jwalden+bmo)
(In reply to Steve Fink [:sfink] from comment #4)
> This crashes without the watchStateChange? Why? I thought you'd have to put
> something at that location, then check to be sure you're getting the right
> value back. (Or that your stores had an effect.) Or is there some assert
> that notices what's going on?

What happens is the typed array's *old* data pointer gets used for the load or the store -- not the new one for the 512K backing store added in bug 982974.  The neuter operation discards the old data, so when the load/store happens in these testcases, it loads/stores from freed memory, likely causing a crash (but of course no guarantee because of allocator internals and all).

Interestingly, the store testcase (I didn't bother varying the load testcase, but it's likely the same in this respect) doesn't crash with a 64K buffer size, but does with 512K.  Probably some allocator intricacy (maybe more-eager freeing of memory that's sufficiently large?).

The ultimate problem here is the JITs *by design* skip checks they can optimize away, so there's no way to assert/test the condition being relied on here, without perturbing the JITted code in likely-bad ways (for performance for certain, and for the similarity of JIT code in opt builds to JIT code in debug builds likely).
I expect this will/should land approximately the same time as bug 991981, bug 995679, and bug 999651, on trunk and branches as close to simultaneously as possible, as close to release as possible, given it's all roughly the same system being changed.  (Actually I think this patch might have a dependency on the patch in bug 999651, but my mind is sufficiently split and task-burdened that I'm not going to verify that unless it's specifically requested.)

[Security approval request comment]
> How easily could an exploit be constructed based on the patch?
You can write arbitrary data to arbitrary locations in the memory used in the past to back typed arrays.  I imagine it's pretty easy to bootstrap that into an exploit.

> Do comments in the patch, the check-in comment, or tests included in the
> patch paint a bulls-eye on the security problem?
A cursory glance at what the code's doing probably makes fairly obvious the intent of the code.  And if you understand that, it's obvious how you might screw up, and that this patch would fix that.  We almost certainly want to land this as late as possible.

> Which older supported branches are affected by this flaw?
Back to at least ESR24, perhaps beyond that, but I didn't think we cared enough about earlier versions to be worth any investigation whatsoever.

> If not all supported branches, which bug introduced the flaw?
Looks like bug 864214.  (I'd rather not mark the dependency until after this is fixed, to not draw attention to this code.)

> Do you have backports for the affected branches? If not, how different,
> hard to create, and risky will they be?
Shouldn't be too bad to create them.

> How likely is this patch to cause regressions; how much testing
> does it need?
This seems relatively low-risk.  The changes are small and non-intricate, and the patch fits on a single (smallish-font-size) terminal screen.  Plus there are the two testcases for the code in question to verify with (although because of platform vagaries, it's possible for either test to "pass" even when the underlying problem exists -- I just know both tests crashed with my compiler, on my system).
Attachment #8422106 - Attachment is obsolete: true
Attachment #8423384 - Flags: sec-approval?
Attachment #8423384 - Flags: review+
Comment on attachment 8423384 [details] [diff] [review]
Final trunk patch

sec-approval+ for trunk. Let's get this in and then get Aurora, Beta, and ESR24 patches made and nominated.
Attachment #8423384 - Flags: sec-approval? → sec-approval+
Group: core-security
Attachment #8422102 - Flags: review?(sphink) → review+
Ugh.  The constraint mechanism stuff dates back to the 28ish, maybe 27ish time frame.  If you wanted to backport it to 26 or earlier, you'd need to import patches that look fairly large and complicated at a glance.

I think it's likely that for branches where that doesn't exist, I'll just disable this optimization, and any others that depend on constraints invalidating code with burned-in constants in them.  The cost to load a pointer or integer or two is not the end of the world, and it'll vastly simplify portions of the backporting story.
Patches for branch nomination then, please, as we are nearing our final weeks of FF30 beta.
Flags: needinfo?(jwalden+bmo)
Folded into mega-rollup patch with a bunch of other issues:

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

Backports are happening, aurora/beta by EOD today, hopefully.
Backed out, then relanded with CLOBBER:

https://hg.mozilla.org/integration/mozilla-inbound/rev/57b0932e2f06
Flags: needinfo?(jwalden+bmo)
Comment on attachment 8423384 [details] [diff] [review]
Final trunk patch

[Approval Request Comment]
Bug caused by (feature/regressing bug #): introduction of neutering, it appears
User impact if declined: sec-critical writes to out-of-bounds memory
Testing completed (on m-c, etc.): landed on m-c, test verified m-c patch, test here used to verify aurora patch
Risk to taking this patch (and alternatives if risky): pretty little, this is a nice, simple patch, actually
String or IDL/UUID changes made by this patch: N/A
Attachment #8423384 - Flags: approval-mozilla-aurora?
Comment on attachment 8423384 [details] [diff] [review]
Final trunk patch

Also looks to backport to beta basically straightforwardly.

(But again, leave the landing to me.)
Attachment #8423384 - Flags: approval-mozilla-beta?
https://hg.mozilla.org/mozilla-central/rev/57b0932e2f06
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla32
Comment on attachment 8423384 [details] [diff] [review]
Final trunk patch

Approving for uplift to Aurora/Beta - if this can land as-is to ESR24 please nom otherwise put up the branch patch for ESR before the end of week to make sure this gets into next Monday's build.
Attachment #8423384 - Flags: approval-mozilla-beta?
Attachment #8423384 - Flags: approval-mozilla-beta+
Attachment #8423384 - Flags: approval-mozilla-aurora?
Attachment #8423384 - Flags: approval-mozilla-aurora+
Comment on attachment 8423384 [details] [diff] [review]
Final trunk patch

With some fuzzing at the edges for renames and such, this applies to b2g28.

Comment 14 approval request comments apply here (and also, this landed in aurora/beta earlier today, for extra testing-ness).

Again don't want help landing this.
Attachment #8423384 - Flags: approval-mozilla-b2g28?
As noted in bug 999651 comment 27, the invalidation mechanism for typed array data changing was added after 26 and in place by 28, and it's not backport-feasible.  Pull out the big hammer: just turn off the optimization on 26.

Comment 14 approval request comments apply (and also a fix was in aurora/beta earlier today).

No landing help wanted here.
Attachment #8431270 - Flags: review+
Attachment #8431270 - Flags: approval-mozilla-b2g26?
Attachment #8423384 - Flags: approval-mozilla-b2g28?
Attachment #8431270 - Flags: approval-mozilla-b2g26?
Comment on attachment 8431270 [details] [diff] [review]
b2g26: don't optimize loads/stores from "static" typed array objects

Comment 14 approval request comments apply, plus this has landed elsewhere since.

The test I used to verify my backport isn't quite the trunk patch, but it's very nearly so.  I'll post the branch-appropriate test for anyone who happens to want to play along at home at some point.
Attachment #8431270 - Flags: approval-mozilla-esr24?
ESR24 won't inline typed loads/stores unless the index expression is a right-shift by constant log2(byte width of elements).  A bit bizarre flat-out constants don't work, but oh well.  I bet I can blame benchmarks.  :-P
Comment on attachment 8431270 [details] [diff] [review]
b2g26: don't optimize loads/stores from "static" typed array objects

ESR24 approval granted.
Attachment #8431270 - Flags: approval-mozilla-esr24? → approval-mozilla-esr24+
Group: javascript-core-security
Whiteboard: [adv-main30+][adv-esr24.6+]
The tests here don't appear to do anything that looks different pre- or post-fix. I could be missing something. Suggestions on verification would be welcome, thank you.
You have to test on x86 -- doesn't appear on x86-64 or ARM.  Shell requires --ion-eager to reproduce, and in newer tree also --ion-parallel-compile=off.  ESR24 only will verify with the different patch attached here, noted as being for that.  Ping me on IRC if you need to know anything else, I'm on right now (although not entirely sure how much longer, I really should leave for the day at some point :-) but want to see a few things are wrapped up before I do).
Jeff gave me some assistance on IRC. These tests should crash an affected build. However, I'm not able to reproduce on Win32 js shell with --ion-eager/--ion-parallel-compile=off using several builds of Fx30/Fx31/Fx32 from early May 2014. 

Jeff says he's tested this himself somewhat thoroughly, and while I would highly prefer to verify myself, time necessitates that I mark it [qa-] for Fx24.6.0esr and Fx30 release.
Whiteboard: [adv-main30+][adv-esr24.6+] → [adv-main30+][adv-esr24.6+][qa-]
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.
Group: core-security
You need to log in before you can comment on or make changes to this bug.