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)
Tracking
()
People
(Reporter: Waldo, Assigned: Waldo)
Details
(Keywords: sec-critical, Whiteboard: [adv-main30+][adv-esr24.6+][qa-])
Attachments
(4 files, 1 obsolete file)
1.02 KB,
patch
|
sfink
:
review+
|
Details | Diff | Splinter Review |
1.98 KB,
patch
|
Waldo
:
review+
lsblakk
:
approval-mozilla-aurora+
lsblakk
:
approval-mozilla-beta+
abillings
:
sec-approval+
|
Details | Diff | Splinter Review |
3.67 KB,
patch
|
Waldo
:
review+
lmandel
:
approval-mozilla-esr24+
|
Details | Diff | Splinter Review |
1.20 KB,
patch
|
Details | Diff | Splinter Review |
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)
Assignee | ||
Comment 1•10 years ago
|
||
Attachment #8422106 -
Flags: review?(sphink)
Attachment #8422106 -
Flags: review?(shu)
Comment 2•10 years ago
|
||
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 3•10 years ago
|
||
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 4•10 years ago
|
||
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?
Updated•10 years ago
|
Flags: needinfo?(jwalden+bmo)
Comment 5•10 years ago
|
||
(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.
Assignee | ||
Comment 6•10 years ago
|
||
(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)
Assignee | ||
Comment 7•10 years ago
|
||
(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).
Assignee | ||
Comment 8•10 years ago
|
||
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+
Updated•10 years ago
|
status-firefox29:
--- → wontfix
status-firefox30:
--- → affected
status-firefox31:
--- → affected
status-firefox32:
--- → affected
status-firefox-esr24:
--- → affected
tracking-firefox30:
--- → +
tracking-firefox31:
--- → +
tracking-firefox32:
--- → +
tracking-firefox-esr24:
--- → +
Comment 9•10 years ago
|
||
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+
Updated•10 years ago
|
Group: core-security
Updated•10 years ago
|
Attachment #8422102 -
Flags: review?(sphink) → review+
Assignee | ||
Comment 10•10 years ago
|
||
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.
Comment 11•10 years ago
|
||
Patches for branch nomination then, please, as we are nearing our final weeks of FF30 beta.
Flags: needinfo?(jwalden+bmo)
Assignee | ||
Comment 12•10 years ago
|
||
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.
Assignee | ||
Comment 13•10 years ago
|
||
Backed out, then relanded with CLOBBER: https://hg.mozilla.org/integration/mozilla-inbound/rev/57b0932e2f06
Flags: needinfo?(jwalden+bmo)
Assignee | ||
Comment 14•10 years ago
|
||
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?
Assignee | ||
Comment 15•10 years ago
|
||
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?
Comment 16•10 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/57b0932e2f06
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla32
Updated•10 years ago
|
Comment 17•10 years ago
|
||
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+
Assignee | ||
Comment 18•10 years ago
|
||
https://hg.mozilla.org/releases/mozilla-aurora/rev/c5a58d90da90 https://hg.mozilla.org/releases/mozilla-beta/rev/4fd82c282519 b2g/esr24 backports shortly.
Assignee | ||
Updated•10 years ago
|
Comment 19•10 years ago
|
||
https://hg.mozilla.org/releases/mozilla-b2g30_v1_4/rev/4fd82c282519
status-b2g-v1.2:
--- → affected
status-b2g-v1.3:
--- → affected
status-b2g-v1.3T:
--- → affected
status-b2g-v1.4:
--- → fixed
status-b2g-v2.0:
--- → fixed
Flags: in-testsuite?
Assignee | ||
Comment 20•10 years ago
|
||
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?
Assignee | ||
Comment 21•10 years ago
|
||
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?
Assignee | ||
Comment 22•10 years ago
|
||
https://hg.mozilla.org/releases/mozilla-b2g28_v1_3/rev/57cd741e4d0b https://hg.mozilla.org/releases/mozilla-b2g26_v1_2/rev/4c359ff76654
Updated•10 years ago
|
Attachment #8423384 -
Flags: approval-mozilla-b2g28?
Updated•10 years ago
|
Attachment #8431270 -
Flags: approval-mozilla-b2g26?
Updated•10 years ago
|
Assignee | ||
Comment 23•10 years ago
|
||
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?
Assignee | ||
Comment 24•10 years ago
|
||
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 25•10 years ago
|
||
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+
Assignee | ||
Comment 26•10 years ago
|
||
https://hg.mozilla.org/releases/mozilla-esr24/rev/8c406adf76a0
Updated•10 years ago
|
Group: javascript-core-security
Updated•10 years ago
|
Whiteboard: [adv-main30+][adv-esr24.6+]
Comment 27•10 years ago
|
||
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.
Assignee | ||
Comment 28•10 years ago
|
||
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).
Comment 29•10 years ago
|
||
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-]
Comment 30•10 years ago
|
||
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
Comment 31•10 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/58f5bf899265
Flags: in-testsuite? → in-testsuite+
Updated•9 years ago
|
Group: core-security
You need to log in
before you can comment on or make changes to this bug.
Description
•