Closed Bug 864558 Opened 8 years ago Closed 8 years ago

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

Categories

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

defect
Not set
critical

Tracking

(firefox20 wontfix, firefox21+ fixed, firefox22+ fixed, firefox23+ fixed, firefox-esr1721+ fixed, b2g1821+ fixed, b2g18-v1.0.0 wontfix, b2g18-v1.0.1 affected)

RESOLVED FIXED
Tracking Status
firefox20 --- wontfix
firefox21 + fixed
firefox22 + fixed
firefox23 + fixed
firefox-esr17 21+ fixed
b2g18 21+ fixed
b2g18-v1.0.0 --- wontfix
b2g18-v1.0.1 --- affected

People

(Reporter: Waldo, Assigned: Waldo)

References

Details

(Keywords: csectype-uaf, sec-critical, Whiteboard: [adv-main21+][adv-esr1706+])

Attachments

(3 files)

Attached patch PatchSplinter Review
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.
Attachment #740546 - Flags: review?
Attachment #740546 - Flags: review? → review?(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.
Attachment #740546 - Flags: review?(mrbkap)
Attachment #740546 - Flags: review?(mounir)
Attachment #740546 - Flags: review+
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>.
Attachment #740546 - Flags: review?(mrbkap) → review+
(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 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.
Attachment #740546 - Flags: sec-approval?
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.
Attachment #740546 - Flags: sec-approval? → sec-approval+
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
Attachment #740546 - Flags: approval-mozilla-b2g18?
Actually, correction, it's in inbound now -- should make m-c on the next merge.
https://hg.mozilla.org/mozilla-central/rev/96509dd0406d
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
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.
Attachment #740546 - Flags: approval-mozilla-b2g18? → approval-mozilla-b2g18+
Please provide a branch-specific patch.
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.
Flags: needinfo?(jwalden+bmo)
This applies against b2g18.
Flags: needinfo?(jwalden+bmo)
(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 ?
Attachment #743342 - Attachment description: And esr17 → esr17, b2g18-1.0.0, b2g18-1.0.1 patch
Attachment #743340 - Attachment description: b2g18 patch → b2g18, aurora patch
Attachment #743342 - Attachment description: esr17, b2g18-1.0.0, b2g18-1.0.1 patch → esr17, b2g18-1.0.0, b2g18-1.0.1, beta patch
(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.
(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 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
Attachment #743340 - Flags: approval-mozilla-aurora?
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
Attachment #743342 - Flags: approval-mozilla-esr17?
Attachment #743342 - Flags: approval-mozilla-beta?
Attachment #743342 - Flags: approval-mozilla-b2g18?
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.
Attachment #743342 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
Attachment #743340 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
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.  :-)
Attachment #743342 - Flags: approval-mozilla-esr17?
Attachment #743342 - Flags: approval-mozilla-esr17+
Attachment #743342 - Flags: approval-mozilla-b2g18?
Attachment #743342 - Flags: approval-mozilla-b2g18+
Whiteboard: [adv-main21+][adv-esr1706+]
Depends on: 934000
Group: core-security
You need to log in before you can comment on or make changes to this bug.