864558 - It's a horrible idea to do |new JS::Value[n]| and not root it/its contents while filling it in, and while using it after

Status: RESOLVED FIXED

(Firefox OS Graveyard :: Gaia::SMS, defect)

Description

[Jeff Walden [:Waldo]]

Attachment: Patch

In SmsFilter::GetNumbers:

  uint32_t length = mData.numbers().Length();

  if (length == 0) {
    *aNumbers = JSVAL_NULL;
    return NS_OK;
  }

  JS::Value* numbers = new JS::Value[length];

  for (uint32_t i=0; i<length; ++i) {
    numbers[i].setString(JS_NewUCStringCopyN(aCx, mData.numbers()[i].get(),
                                             mData.numbers()[i].Length()));
  }

  aNumbers->setObjectOrNull(JS_NewArrayObject(aCx, length, numbers));
  NS_ENSURE_TRUE(aNumbers->isObject(), NS_ERROR_FAILURE);

  return NS_OK;

If any of the JS_NewUCStringCopyN trigger a GC, anything before |numbers[i]| is likely to be garbage that'll trigger a crash on GC after the JS_NewArrayObject object gets collected at some later time.

Similarly, in SmsManager::Send:

  JS::Value* requests = new JS::Value[size];

  for (uint32_t i=0; i<size; ++i) {
    JS::Value number;
    if (!JS_GetElement(cx, &numbers, i, &number)) {
      return NS_ERROR_INVALID_ARG;
    }

    nsresult rv = Send(cx, global, number.toString(), aMessage, &requests[i]);
    NS_ENSURE_SUCCESS(rv, rv);
  }

  aReturn->setObjectOrNull(JS_NewArrayObject(cx, size, requests));
  NS_ENSURE_TRUE(aReturn->isObject(), NS_ERROR_FAILURE);

|aNumbers| is user-controlled.  JS_GetElement can call arbitrary JS code.  Send may as well, to create the SmsRequest objects that get returned.  All returned values will get shoved into |requests|, but none of those locations are roots, so the returned objects will probably end up being collected.  Then when the JS_NewArrayObject object gets GC'd, we'll probably crash.

The solution in both places is to use a value array type that the JS engine knows about, so that everything in it is properly rooted.

The SmsManager::Send code also looks pretty broken if someone calls the API passing a non-array object, or an array that doesn't contain all strings, so I "fixed" those issues too, in passing, so that at least crashes don't happen.  There seems to be no spec for how any of this should work, so not-crashing is the best I can do in a quick fix.

Technically as the SMS code is behind the privileged firewall these issues aren't vulnerabilities, in theory, but I'd rather play it safe just in case.

Comment 1

[Mounir Lamouri (:mounir)]

Comment on attachment 740546 [details] [diff] [review]
Patch

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

Sounds good to me but I would like someone that knows better than me about the JS API to have a look at that too.

Comment 2

[Blake Kaplan (:mrbkap) (inactive)]

Comment on attachment 740546 [details] [diff] [review]
Patch

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

::: dom/mobilemessage/src/SmsFilter.cpp
@@ +137,3 @@
>    }
>  
> +  for (uint32_t i = 0; i < length; ++i) {

This should use nsTArrayToJSArray <http://mxr.mozilla.org/mozilla-central/source/js/xpconnect/public/nsTArrayHelpers.h#51>.

Comment 3

[Jeff Walden [:Waldo]]

(In reply to Blake Kaplan (:mrbkap) from comment #2)
> This should use nsTArrayToJSArray

bz says no, for at least as long as that uses JS_SetElement (which can trigger setters on Array.prototype[0] and Object.prototype[0], and so on for other indexes).

Also that method seems to freeze the returned array, which seems pretty unlikely for any DOM API to do now.  (Maybe DOM APIs will start having frozen objects, or arrays with non-writable length, but it's not happening right now, and it should be much much clearer in the API if that's supposed to happen, I think.)

Comment 4

[Jeff Walden [:Waldo]]

Comment on attachment 740546 [details] [diff] [review]
Patch

[Security approval request comment]
How easily could an exploit be constructed based on the patch?
Easy enough to trigger the crash, but exploiting it would require feeding in data just so to a privileged b2g app right now.

Do comments in the patch, the check-in comment, or tests included in the patch paint a bulls-eye on the security problem?
It's obvious enough.

Which older supported branches are affected by this flaw?  If not all supported branches, which bug introduced the flaw?
Goes back to when this SMS code was introduced, so all b2g releases/repos/etc.

Do you have backports for the affected branches? If not, how different, hard to create, and risky will they be?
Should be trivial to create.

How likely is this patch to cause regressions; how much testing does it need?
Unlikely, doesn't need much testing.

Comment 5

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

Comment on attachment 740546 [details] [diff] [review]
Patch

Sec-approval+.

If we want this on branches, I expect that release management will want a security rating before they approve nominated patches.

Comment 6

[Jeff Walden [:Waldo]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/96509dd0406d

Comment 7

[Jeff Walden [:Waldo]]

Comment on attachment 740546 [details] [diff] [review]
Patch

[Approval Request Comment]
Bug caused by (feature/regressing bug #): bug 674725
User impact if declined: possible arbitrary code execution if the right inputs are fed to SMS code
Testing completed: in on m-c, but I'm not sure anyone's actually running this code on m-c -- it has to go to branches and such to get any use, I believe
Risk to taking this patch: low, just rewriting a little bit of code to use correct JSAPI idioms
String or UUID changes made by this patch: N/A

Comment 8

[Jeff Walden [:Waldo]]

Actually, correction, it's in inbound now -- should make m-c on the next merge.

Comment 9

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/96509dd0406d

Comment 10

[Lukas Blakk [:lsblakk] use ?needinfo]

Comment on attachment 740546 [details] [diff] [review]
Patch

This should get nominated for branches as well, if low risk to land, since it's a sec-critical bug we'll do the uplift to mozilla-b2g18.  Is this also affected on ESR17? If yes, please nominate for that branch as well.

Comment 11

[Ryan VanderMeulen [:RyanVM]]

Please provide a branch-specific patch.

Comment 12

[bhavana bajaj [:bajaj]]

Adding needsinfo on :Waldo to help get the branch specific patches asap.We will be going to build with our second last beta on Tuesday and since Fx21 is affected and this is a sec-crit issue with a low risk patch ,requesting for uplift.

Comment 13

[Jeff Walden [:Waldo]]

Attachment: b2g18, aurora patch

This applies against b2g18.

Comment 14

[Jeff Walden [:Waldo]]

Attachment: esr17, b2g18-1.0.0, b2g18-1.0.1, beta patch

Comment 15

[bhavana bajaj [:bajaj]]

(In reply to Jeff Walden [:Waldo] (remove +bmo to email) from comment #13)
> Created attachment 743340 [details] [diff] [review]
> b2g18 patch
> 
> This applies against b2g18.

Since status-firefox21/22 are affected as well and since this a sec-crit regression, don't we want to uplift this on beta/aurora ? Are you suggesting these are not affected then ?

Comment 16

[Jeff Walden [:Waldo]]

(In reply to bhavana bajaj [:bajaj] from comment #15)
> Since status-firefox21/22 are affected as well and since this a sec-crit
> regression, don't we want to uplift this on beta/aurora ? Are you suggesting
> these are not affected then ?

Not suggesting anything at all.  :-)  I created the two backports I uploaded.  Then I got bogged down waiting on cloning all the other trees just mentioned in those patch titles, and I went and did other things while waiting on that.  Returning from those I saw the clones had finished, and I was able to determine exactly which patch applied where.  (This was such a mess because the affected files got moved around semi-recently and were in varying states across the train branches and b2g branches, and so I had next to no idea what would ever apply anywhere.  :-\ )  I think these patches should be good to go everywhere now.

Comment 17

[bhavana bajaj [:bajaj]]

(In reply to Jeff Walden [:Waldo] (remove +bmo to email) from comment #16)
> (In reply to bhavana bajaj [:bajaj] from comment #15)
> > Since status-firefox21/22 are affected as well and since this a sec-crit
> > regression, don't we want to uplift this on beta/aurora ? Are you suggesting
> > these are not affected then ?
> 
> Not suggesting anything at all.  :-)  I created the two backports I
> uploaded.  Then I got bogged down waiting on cloning all the other trees
> just mentioned in those patch titles, and I went and did other things while
> waiting on that.  Returning from those I saw the clones had finished, and I
> was able to determine exactly which patch applied where.  (This was such a
> mess because the affected files got moved around semi-recently and were in
> varying states across the train branches and b2g branches, and so I had next
> to no idea what would ever apply anywhere.  :-\ )  I think these patches
> should be good to go everywhere now.

ah, I see... missed reading the title's :P . Please request nomination on these asap so they land by tomorrow . Thanks !

Comment 18

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-b2g18/rev/d8bbbcabcae1

Comment 19

[Jeff Walden [:Waldo]]

Comment on attachment 743340 [details] [diff] [review]
b2g18, aurora patch

The previous patch has approval-mozilla-b2g18+, so I'll carry that forward to here wrt that branch.  :-)

[Approval Request Comment]
Bug caused by (feature/regressing bug #): bug 674725
User impact if declined: possible arbitrary code execution if the right inputs are fed to SMS code
Testing completed: in on m-c, but I'm not sure anyone's actually running this code on m-c -- it has to go to branches and such to get any use, I believe
Risk to taking this patch: low, just rewriting a little bit of code to use correct JSAPI idioms
String or UUID changes made by this patch: N/A

Comment 20

[Jeff Walden [:Waldo]]

Comment on attachment 743342 [details] [diff] [review]
esr17, b2g18-1.0.0, b2g18-1.0.1, beta patch

Not sure if I need approval-mozilla-b2g18+ for landings in v100/v101 repos or something else, or if the previous one applies, so requesting that for this just to be safe.

[Approval Request Comment]
Bug caused by (feature/regressing bug #): bug 674725
User impact if declined: possible arbitrary code execution if the right inputs are fed to SMS code
Testing completed: in on m-c, but I'm not sure anyone's actually running this code on m-c -- it has to go to branches and such to get any use, I believe
Risk to taking this patch: low, just rewriting a little bit of code to use correct JSAPI idioms
String or UUID changes made by this patch: N/A

[Approval Request Comment]
Bug caused by (feature/regressing bug #): bug 674725
User impact if declined: possible arbitrary code execution if the right inputs are fed to SMS code
Testing completed: in on m-c, but I'm not sure anyone's actually running this code on m-c -- it has to go to branches and such to get any use, I believe
Risk to taking this patch: low, just rewriting a little bit of code to use correct JSAPI idioms
String or UUID changes made by this patch: N/A

[Approval Request Comment]
If this is not a sec:{high,crit} bug, please state case for ESR consideration: sec-critical
User impact if declined: possible arbitrary code execution if the right inputs are fed to SMS code
Fix Landed on Version: trunk == 23, I believe
Risk to taking this patch (and alternatives if risky): low, just rewriting a little bit of code to use correct JSAPI idioms
String or UUID changes made by this patch: N/A

Comment 21

[bhavana bajaj [:bajaj]]

Comment on attachment 743342 [details] [diff] [review]
esr17, b2g18-1.0.0, b2g18-1.0.1, beta patch

Approving for beta as its sec-crit and has a low risk patch.I am assuming there is no impact to desktop here, but please add qawanted if there is any testing needed here

Request to land asap so this can get into our next beta going to land shortly.

Comment 22

[Jeff Walden [:Waldo]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/578fbbc87af1
https://hg.mozilla.org/releases/mozilla-beta/rev/6dfc0800664b

Pretty sure I got the affected/fixed flag changes correct, someone please do 'em right if I screwed them up.  :-)

Comment 23

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-esr17/rev/97d8d5ed9338