983344 - (CVE-2014-1514) ZDI-CAN-2225 vmtypedarrayobject.cpp dst ptr is checked the length is not checked

Status: VERIFIED FIXED

(Core :: JavaScript Engine, defect)

Comment 4

[Curtis Koenig [:curtisk-use curtis.koenig+bzATgmail.com]]]

Attachment: index.html

Comment 5

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

Reporter for credit is whom?

Comment 6

[Olli Pettay [:smaug][[e-mail]]]

Is this effectively the same as Bug 982974?

Comment 7

[Jan de Mooij [:jandem]]

(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.

Comment 8

[Daniel Veditz [:dveditz]]

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

George Hotz

Comment 9

[Lukas Blakk [:lsblakk] use ?needinfo]

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.

Comment 10

[Steve Fink [:sfink] [:s:]]

(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. :(

Comment 11

[Steve Fink [:sfink] [:s:]]

Attachment: fromArray crasher

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

Comment 12

[Gary Kwong [:gkw] [:nth10sd] (NOT official MoCo now)]

> 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.

Comment 13

[Steve Fink [:sfink] [:s:]]

Attachment: Track things that can mutate ArrayBuffers

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

Comment 14

[Shu-yu Guo [:shu]]

(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.

Comment 15

[Jason Smith [:jsmith]]

Paul - Is this needed on FxOS 1.3?

Comment 16

[Jeff Walden [:Waldo]]

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.  :-(

Comment 17

[Jeff Walden [:Waldo]]

Attachment: Patch (without a test, having trouble getting testcases here to work locally, oddly)

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

Comment 18

[Jeff Walden [:Waldo]]

Attachment: Test

Comment 19

[Jeff Walden [:Waldo]]

Attachment: Patch to go with the test

Comment 20

[Jeff Walden [:Waldo]]

Yes, this is needed in 1.3.

Comment 21

[Jeff Walden [:Waldo]]

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.  :-)

Comment 22

[Jeff Walden [:Waldo]]

Attachment: 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).

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?

Comment 23

[Jeff Walden [:Waldo]]

Attachment: b2g26 patch

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.

Comment 24

[Andrew McCreight [:mccr8]]

> 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.

Comment 25

[Ryan VanderMeulen [:RyanVM]]

(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?

Comment 26

[Ioana (away)]

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)

Comment 27

[Ryan VanderMeulen [:RyanVM]]

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 28

[Steve Fink [:sfink] [:s:]]

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.

Comment 29

[(no longer active)]

(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 30

[Steve Fink [:sfink] [:s:]]

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.)

Comment 31

[Steve Fink [:sfink] [:s:]]

Attachment: Combined patch - do not cache length or data pointer

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

Comment 32

[Steve Fink [:sfink] [:s:]]

Attachment: do not cache length or data pointer

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.

Comment 33

[Steve Fink [:sfink] [:s:]]

Attachment: fromArray test

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

Comment 34

[Steve Fink [:sfink] [:s:]]

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 35

[Daniel Veditz [:dveditz]]

Comment on attachment 8391477 [details] [diff] [review]
do not cache length or data pointer

sec-approval=dveditz

Comment 36

[Daniel Veditz [:dveditz]]

Comment on attachment 8391477 [details] [diff] [review]
do not cache length or data pointer

[Triage Comment]
approve all the things

Comment 37

[Daniel Veditz [:dveditz]]

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 38

[Lukas Blakk [:lsblakk] use ?needinfo]

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.

Comment 39

[Steve Fink [:sfink] [:s:]]

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.

Comment 40

[Ryan VanderMeulen [:RyanVM]]

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

Comment 41

[Ryan VanderMeulen [:RyanVM]]

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

Comment 42

[Jeff Walden [:Waldo]]

Also pushed to inbound just now, fwiw:

https://hg.mozilla.org/integration/mozilla-inbound/rev/fa8369f3ad57

Comment 43

[Ryan VanderMeulen [:RyanVM]]

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

Comment 44

[Jan de Mooij [:jandem]]

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);

Comment 45

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

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

Comment 46

[Ioana (away)]

Verified as fixed on Firefox 29 and 30.

Comment 47

[Jeff Walden [:Waldo]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/43bdcb0d3c73

Comment 48

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/43bdcb0d3c73

Comment 49

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

Why are we adding checkins from six months ago now?

Comment 50

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

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