Reporter for credit is whom?

Is this effectively the same as Bug 982974?

(In reply to Olli Pettay [:smaug] from comment #6) > Is this effectively the same as Bug 982974? Yes it's a different typed array method but also accessing invalid memory after neutering.

(In reply to Al Billings [:abillings] from comment #5) > Reporter for credit is whom? George Hotz

Also tracking as per bug 982974 so we can mark disabled (if turning off neutering for 28 is an option) and continue with looking for proper fix in later version.

(In reply to Jan de Mooij [:jandem] from comment #7) > (In reply to Olli Pettay [:smaug] from comment #6) > > Is this effectively the same as Bug 982974? > > Yes it's a different typed array method but also accessing invalid memory > after neutering. Yes, and the bug 982974 patch won't fix this. The problem here is in copyFromArray. It receives a 'len' parameter saying how many elements to copy, which the caller has ensured won't escape the bounds of the destination array. But the source array's elements' getters are called, which can neuter the destination array and make 'len' invalid. Worse, the neutered array's data pointer will be reset to point to inline storage, giving (write) access to object memory. 'dest' will still be stale, but we have an explicit check to update it in case a GC happens. So the exploit forces a GC, dest now points to inline memory, and user-provided data is written to an object shortly after the destination array. :(

The attached test case crashes for me in a debug build, no asserts.

> The attached test case crashes for me in a debug build, no asserts. This testcase only asserts mozilla-central from end-Feb onwards: autoBisect shows this is probably related to the following changeset: The first bad revision is: changeset: http://hg.mozilla.org/mozilla-central/rev/03355461606c user: Nicholas D. Matsakis date: Fri Feb 21 12:32:24 2014 -0500 summary: Bug 975456 -- Preserve invariant that views on a neutered buffer have a NULL data pointer r=shu It doesn't assert on mozilla-release, but maybe that's intended.

I wonder if we should fix some of these with something like this.

(In reply to Gary Kwong [:gkw] [:nth10sd] from comment #12) > > This testcase only asserts mozilla-central from end-Feb onwards: > > autoBisect shows this is probably related to the following changeset: > > The first bad revision is: > changeset: http://hg.mozilla.org/mozilla-central/rev/03355461606c > user: Nicholas D. Matsakis > date: Fri Feb 21 12:32:24 2014 -0500 > summary: Bug 975456 -- Preserve invariant that views on a neutered > buffer have a NULL data pointer r=shu > > It doesn't assert on mozilla-release, but maybe that's intended. That r= should be r=sfink.

Paul - Is this needed on FxOS 1.3?

Comment on attachment 8390894 [details] [diff] [review] Track things that can mutate ArrayBuffers Review of attachment 8390894 [details] [diff] [review]: ----------------------------------------------------------------- This seems kind of bigger and more engineered than I'd really like -- particularly in the short run for something branch-targeted. Let's just do spot-fixing rather than come up with some crazier thing that avoids one or two dereferences and some bit-twiddling. I actually wrote up that patch just now, before reloading this bug to see you'd already started this. :-(

+12,-12 seems better than +20 overall or so.

Yes, this is needed in 1.3.

Comment on attachment 8391012 [details] [diff] [review] Patch to go with the test Review of attachment 8391012 [details] [diff] [review]: ----------------------------------------------------------------- ::: js/src/vm/TypedArrayObject.cpp @@ +938,5 @@ > NativeType n; > if (!nativeFromValue(cx, src[i], &n)) > return false; > dest[i] = n; > + } while (i < len); Obviously I mean (++i < len) here. :-)

This patch, and the test, both backport trivially to aurora and beta. The test doesn't work on b2g26 because there's no neuter function there. Using |serialize(ab, [ab]);| cargo-culted from something here doesn't work, either, because, um. There's no neutering JSAPI function in that tree. JS_StealArrayBufferContents is only called from a webaudio thing, and to be honest I'm very disinclined to go to that effort to test only for b2g26 (particularly when I don't know webaudio at all, and the patch is simple and readable and obviously enough correct). The patch doesn't backport to b2g26, either. I'll post a b2g26-ready version in a sec. Do I need versions for anything other than trunk, aurora, beta, and b2g26, or will that cover it?

hg qimport didn't import it fully-cleanly, but the manual porting process was straightforward. If the other patch gets review, that should cover this.

> Do I need versions for anything other than trunk, aurora, beta, and b2g26, or will that cover it? That should be okay. It will need to go on various b2g trees, but they are similar enough to those other branches that it may work okay. RyanVM knows where things have to land, and will let you know if a patch doesn't apply cleanly.

(In reply to Jeff Walden [:Waldo] (remove +bmo to email) from comment #22) > Do I need versions for anything other than trunk, aurora, beta, and b2g26, > or will that cover it? esr24? Or will the b2g26 patch work there too?

Used the testcase on a Firefox 28 beta and got this crash: https://crash-stats.mozilla.com/report/index/3ea63b83-5038-4d85-9630-1db3d2140314. Also tested on the 03/14 Aurora, where I get different crashes: https://crash-stats.mozilla.com/report/index/a80a5aea-d385-4b9d-ac68-da2992140314 https://crash-stats.mozilla.com/report/index/607f4707-d5ce-4db3-a8e1-5a94f2140314 https://crash-stats.mozilla.com/report/index/1c08938f-65f8-4b35-b8ad-1fddc2140314 Do these all correspond to this same bug? (they look pretty different)

Looks like jstypedarray.cpp on esr24 and b2g18 has the same basic code as b2g26, so it could probably be made to apply without too much trouble.

Comment on attachment 8391066 [details] [diff] [review] Trunk/aurora/beta: Fix the ++ typo, quell an opt-build unused-variable warning Review of attachment 8391066 [details] [diff] [review]: ----------------------------------------------------------------- Yeah, this is the right way to do it. The only reason I didn't immediately do the obvious thing is that I was vaguely thinking that the other way might apply more generally, but I don't think it will. "More generally" as in both "applies to more places in the code", which it doesn't seem like it would, and "can be adapted to anything we add in the future that mutates cached state", which is too theoretical and would require checking all of the users anyway to make sure we're properly updating for all the mutable state. ::: js/src/vm/TypedArrayObject.cpp @@ +953,5 @@ > NativeType n; > if (!nativeFromValue(cx, v, &n)) > return false; > > + len = thisTypedArray->length(); I still prefer len = Min(len, thisTypedArray->length()). Right now, it makes no difference. But we occasionally talk about making ArrayBuffers resizeable. It probably won't happen anytime soon because it's such a mess, as these bugs show. But if an ArrayBuffer/typed array pair *were* expandable, this would make us copy more than was requested.

(In reply to Jeff Walden [:Waldo] (remove +bmo to email) from comment #22) > Created attachment 8391066 [details] [diff] [review] > Trunk/aurora/beta: Fix the ++ typo, quell an opt-build unused-variable > warning > > This patch, and the test, both backport trivially to aurora and beta. > > The test doesn't work on b2g26 because there's no neuter function there. > Using |serialize(ab, [ab]);| cargo-culted from something here doesn't work, > either, because, um. There's no neutering JSAPI function in that tree. > JS_StealArrayBufferContents is only called from a webaudio thing, and to be > honest I'm very disinclined to go to that effort to test only for b2g26 > (particularly when I don't know webaudio at all, and the patch is simple and > readable and obviously enough correct). If you need help on how Web Audio uses this, I'd be happy to help. Ping me on irc?

Comment on attachment 8391011 [details] [diff] [review] Test Review of attachment 8391011 [details] [diff] [review]: ----------------------------------------------------------------- ::: js/src/tests/js1_8_5/extensions/typedarray-set-neutering.js @@ +37,5 @@ > +}); > + > +a.set(src); > + > +// Not really needed This comment should probably be expanded. // The unpatched code should crash during the set() call above, // but just in case we'll access this array (which will have been overwritten.)

I made the minor updates. Ready for landing. Author=Waldo, r=sfink.

Oops, shouldn't combine patch + test, since dveditz said on IRC that the test would make it too easy for people to make an exploit.

Test, for landing, author=Waldo, r=sfink

Comment on attachment 8391477 [details] [diff] [review] do not cache length or data pointer [Approval Request Comment] Bug caused by (feature/regressing bug #): bug 720949 User impact if declined: exploitable memory overwrite from content when someone's being evil Testing completed (on m-c, etc.): local testing only Risk to taking this patch (and alternatives if risky): minimal String or IDL/UUID changes made by this patch: none [Security approval request comment] How easily could an exploit be constructed based on the patch? Somewhat easily, for someone familiar with this sort of problem Do comments in the patch, the check-in comment, or tests included in the patch paint a bulls-eye on the security problem? no Which older supported branches are affected by this flaw? going back to 18 If not all supported branches, which bug introduced the flaw? bug 720949 Do you have backports for the affected branches? If not, how different, hard to create, and risky will they be? they should be the same or nearly the same How likely is this patch to cause regressions; how much testing does it need? unlikely. Other than the funky cases this patch fixes, the logic doesn't change.

Comment on attachment 8391477 [details] [diff] [review] do not cache length or data pointer sec-approval=dveditz

Comment on attachment 8391477 [details] [diff] [review] do not cache length or data pointer [Triage Comment] approve all the things

Comment on attachment 8391068 [details] [diff] [review] b2g26 patch Review of attachment 8391068 [details] [diff] [review]: ----------------------------------------------------------------- ::: js/src/vm/TypedArrayObject.cpp @@ +2292,5 @@ > SkipRoot skipSrc(cx, &src); > > + JSRuntime *runtime = cx->runtime(); > +#ifdef DEBUG > + uint64_t gcNumber = runtime->gcNumber; Should runtime also be in the #ifdef DEBUG? I don't see anyone using it, but maybe there are side-effects you rely on?

Comment on attachment 8391477 [details] [diff] [review] do not cache length or data pointer You'll need to do the uplifts (at least to mozilla-release) on your own as Ryan is out now for the day but we'll need this landed to m-r for going to build on our FF28 RC.

https://hg.mozilla.org/mozilla-central/rev/07dc951bcff5 Whoops, didn't look at dveditz's comment. Yes, that should totally be in #idef DEBUG. I suppose that'll burn the warnings-as-errors builds.

https://hg.mozilla.org/releases/mozilla-aurora/rev/e7f941c91aaa https://hg.mozilla.org/releases/mozilla-beta/rev/b5722e8c9a77

https://hg.mozilla.org/releases/mozilla-b2g26_v1_2/rev/35a1e0eb3275 https://hg.mozilla.org/releases/mozilla-esr24/rev/dac1c0de36da https://hg.mozilla.org/releases/mozilla-b2g18/rev/74d4570c3aaf

Also pushed to inbound just now, fwiw: https://hg.mozilla.org/integration/mozilla-inbound/rev/fa8369f3ad57

Backed out and re-landed on b2g18 with a bustage fix. https://hg.mozilla.org/releases/mozilla-b2g18/rev/38689fcdae92 Finished the remaining merges: https://hg.mozilla.org/releases/mozilla-release/rev/b5722e8c9a77 https://hg.mozilla.org/releases/mozilla-b2g28_v1_3/rev/b5722e8c9a77 https://hg.mozilla.org/releases/mozilla-b2g18_v1_1_0_hd/rev/38689fcdae92

Note that the exploit makes use of our executable JIT memory to run its shell code. We have bug 977805 on file to mark JIT code as non-writable, hopefully we can do that without hurting perf too much. I'll look into that next week. At this point the attacker can already read/write arbitrary memory though, so that bug is unlikely to stop a determined attacker, but it could make other attacks harder or less predictable. // create function var lol = new Function(["a","b"], "return a+b;"); for (var i = 0; i < 0x10000; i++) lol(i, i); // write in shellcode var jsscript_addr = read32(getAddress(lol)+0x14); var code = read32(jsscript_addr+0x38); for (var i = 0; i < sc.length; i++) { write32(code+i*4, sc[i]); } // 1+1 = CALC var a = lol(1,1);

Confirmed bug on Fx27.0.1, Windows 8. Verified fix on Fx28, Fx24.4.0esr.

Verified as fixed on Firefox 29 and 30.

Why are we adding checkins from six months ago now?

Decoder has pointed out to me that the timestamp here in the hg links is not when these went in. My bad.