Bug 983344 (CVE-2014-1514)

ZDI-CAN-2225 vmtypedarrayobject.cpp dst ptr is checked the length is not checked

VERIFIED FIXED in Firefox 28

Status

()

defect
VERIFIED FIXED
5 years ago
2 years ago

People

(Reporter: curtisk, Assigned: sfink)

Tracking

({regression, sec-critical})

unspecified
mozilla30
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(blocking-b2g:1.3+, firefox27 wontfix, firefox28+ verified, firefox29+ verified, firefox30+ verified, firefox-esr2428+ verified, b2g18 fixed, b2g-v1.1hd fixed, b2g-v1.2 fixed, b2g-v1.3 fixed, b2g-v1.3T fixed, b2g-v1.4 fixed, fennec28+)

Details

(Whiteboard: [pwn2own 2014][adv-main28+][adv-esr24.4+])

Attachments

(6 attachments, 5 obsolete attachments)

44 bytes, text/html
Details
538 bytes, text/plain
Details
6.50 KB, patch
Details | Diff | Splinter Review
2.88 KB, patch
Details | Diff | Splinter Review
3.47 KB, patch
sfink
: review+
dveditz
: sec-approval+
Details | Diff | Splinter Review
1.65 KB, patch
sfink
: review+
Details | Diff | Splinter Review
No description provided.
Summary: ZDI-CAN-2225 → ZDI-CAN-2225 vmtypedarrayobject.cpp dst ptr is checked the length is not checked
Alias: CVE-2014-1514
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.
Assignee

Comment 10

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

Comment 11

5 years ago
Posted file fromArray crasher
The attached test case crashes for me in a debug build, no asserts.
Attachment #8390872 - Attachment mime type: application/javascript → text/plain
QA Contact: mwobensmith
> 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.
Assignee

Comment 13

5 years ago
I wonder if we should fix some of these with something like this.
Attachment #8390894 - Flags: feedback?(jwalden+bmo)
Assignee

Updated

5 years ago
Assignee: nobody → sphink
Status: NEW → ASSIGNED
(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?
Flags: needinfo?(ptheriault)
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.  :-(
Attachment #8390894 - Flags: feedback?(jwalden+bmo)
+12,-12 seems better than +20 overall or so.
Attachment #8391001 - Flags: review?(sphink)
Attachment #8391001 - Flags: review?(sphink)
Posted patch Test (obsolete) — Splinter Review
Attachment #8391001 - Attachment is obsolete: true
Attachment #8391011 - Flags: review?(sphink)
Posted patch Patch to go with the test (obsolete) — Splinter Review
Attachment #8391012 - Flags: review?(sphink)
Yes, this is needed in 1.3.
Flags: needinfo?(ptheriault)
blocking-b2g: --- → 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.  :-)
blocking-b2g: 1.3? → 1.3+
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?
Attachment #8391012 - Attachment is obsolete: true
Attachment #8391012 - Flags: review?(sphink)
Attachment #8391066 - Flags: review?(sphink)
Posted patch b2g26 patchSplinter Review
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?
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.
tracking-fennec: --- → ?
Assignee

Comment 28

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

Comment 29

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

Comment 30

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

Comment 31

5 years ago
I made the minor updates. Ready for landing. Author=Waldo, r=sfink.
Attachment #8391011 - Attachment is obsolete: true
Attachment #8391066 - Attachment is obsolete: true
Attachment #8391471 - Flags: review+
Assignee

Comment 32

5 years ago
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.
Attachment #8391471 - Attachment is obsolete: true
Attachment #8391477 - Flags: review+
Assignee

Comment 33

5 years ago
Test, for landing, author=Waldo, r=sfink
Attachment #8391478 - Flags: review+
Assignee

Comment 34

5 years ago
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.
Attachment #8391477 - Flags: sec-approval?
Attachment #8391477 - Flags: approval-mozilla-esr24?
Attachment #8391477 - Flags: approval-mozilla-beta?
Attachment #8391477 - Flags: approval-mozilla-aurora?
Blocks: 720949
Keywords: regression
Comment on attachment 8391477 [details] [diff] [review]
do not cache length or data pointer

sec-approval=dveditz
Attachment #8391477 - Flags: sec-approval? → sec-approval+
Comment on attachment 8391477 [details] [diff] [review]
do not cache length or data pointer

[Triage Comment]
approve all the things
Attachment #8391477 - Flags: approval-mozilla-release+
Attachment #8391477 - Flags: approval-mozilla-esr24?
Attachment #8391477 - Flags: approval-mozilla-esr24+
Attachment #8391477 - Flags: approval-mozilla-beta?
Attachment #8391477 - Flags: approval-mozilla-beta+
Attachment #8391477 - Flags: approval-mozilla-aurora?
Attachment #8391477 - Flags: approval-mozilla-aurora+
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.
Flags: needinfo?(sphink)
Assignee

Comment 39

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

Comment 46

5 years ago
Verified as fixed on Firefox 29 and 30.
Status: RESOLVED → VERIFIED
Keywords: verifyme
Whiteboard: [pwn2own 2014] → [pwn2own 2014][adv-main28+][adv-esr24.4+]
tracking-fennec: ? → 28+
Flags: in-testsuite+
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.
Group: core-security
Duplicate of this bug: 1346963
Duplicate of this bug: 1346972
You need to log in before you can comment on or make changes to this bug.