934000 - Port bug 864558 to MobileMessageManager.cpp

Status: RESOLVED FIXED

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

Description

[Jeff Walden [:Waldo]]

I'm not sure exactly what happened here, but when I fixed bug 864558, I did so wrt SmsManager.cpp.  Apparently that file got forked -- but not deleted -- into MobileMessageManager.cpp, in bug 844431, which needs the same set of fixes performed to it.

I'm really not sure why the searching I did at the time of that bug didn't uncover these uses -- the forking appears to have happened before that bug started, and before any activity happened on it.  Bleh.  Probably worth doing another set of searches to make sure nothing *else* escaped the dragnet, or something.  :-(

Comment 1

[Jeff Walden [:Waldo]]

Attachment: Trunk patch

Straightforward enough.  (Previous patches did s/array/object/ on the comment near the first bit -- turns out, at least on trunk now, there's a throw if obj && !array, so the comment's correct now.  Might have been before, too, actually, but it's definitely not worth checking.)

Comment 2

[Jeff Walden [:Waldo]]

On doublecheck, this kills off all the remaining instances of "new JS::Value", "new jsval", "new Value" referring to JS::Value, etc. in Gecko.  So I'm pretty sure it's a full fix.

Comment 3

[Jeff Walden [:Waldo]]

Comment on attachment 827731 [details] [diff] [review]
Trunk 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?
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?
Yes, will post them seriatim after this.

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

Comment 4

[Jeff Walden [:Waldo]]

Attachment: esr24

[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
Testing completed: none except what it'll get after landing on trunk, but little to none required, this is straightforward
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 5

[Jeff Walden [:Waldo]]

Attachment: aurora

All the branch-targeted patches here have the same approval-request characteristics, so I won't bother copy-pasting one comment N times.  :-)

Comment 6

[Jeff Walden [:Waldo]]

Attachment: beta

Comment 7

[Jeff Walden [:Waldo]]

Attachment: b2g18

Comment 8

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

If this is B2G only, is there a reason to take this in ESR24?

Comment 9

[Jeff Walden [:Waldo]]

Not sure.  I *think* this code gets built even on non-b2g, or at least one of the files in the SmsManager.cpp file fixed in the previous bug did.  But I think the code might be unreachable there.

Comment 10

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

Comment on attachment 827731 [details] [diff] [review]
Trunk patch

sec-approval+ for trunk. I'm not sure if we need this on branches or not as a b2g only issue.

Comment 11

[Lukas Blakk [:lsblakk] use ?needinfo]

Matt - is it possible for you to look into comment 9 and determine where this does get reached (and thus, need to land)?

Comment 12

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

I'm afraid that I can't answer that question.

Comment 13

[Andrew McCreight [:mccr8]]

There's no possible way SMS is enabled on esr24.  We're only even running tests on desktop, and I really doubt we support SMS from your desktop machine. ;)  I mean, on the other hand it isn't like it would hurt to land this code there, as we're not even going to build it.  Very low risk!

Comment 14

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

No, we only land necessary things on ESR after it forks.

Comment 15

[Jason Smith [:jsmith]]

(In reply to [e-mail] [:lsblakk] from comment #11)
> Matt - is it possible for you to look into comment 9 and determine where
> this does get reached (and thus, need to land)?

The patch here really only affects B2G - unrelated to desktop & fxandroid. It involves the mobile message API, which is used by the SMS app for MMS.

Comment 16

[Jason Smith [:jsmith]]

(In reply to Jason Smith [:jsmith] from comment #15)
> (In reply to [e-mail] [:lsblakk] from comment #11)
> > Matt - is it possible for you to look into comment 9 and determine where
> > this does get reached (and thus, need to land)?
> 
> The patch here really only affects B2G - unrelated to desktop & fxandroid.
> It involves the mobile message API, which is used by the SMS app for MMS.

Oh and that means the patch needs to land on trunk, b2g26, and b2g18.

Comment 17

[Lukas Blakk [:lsblakk] use ?needinfo]

minusing for beta, this can get direct uplift to b2g26 branch since it's a koi+ and should be found.

Comment 18

[Alex Keybl [:akeybl]]

checkin-needed for m-c, m-b2g26, and m-b2g18

Comment 19

[Jeff Walden [:Waldo]]

I'm told over IRC that the trunk patch will need s/JS_ValueToString/JS::ToString/ to work, just to note.  Trying to round up all the patches here for pushing tonight, uploaded to bug, but may not have time -- we'll see, if I do I'll obsolete everything here and leave just the obvious targeted patches up.

Comment 20

[Jeff Walden [:Waldo]]

Attachment: Trunk patch

Comment 21

[Jeff Walden [:Waldo]]

Attachment: b2g18-targeted patch

Comment 22

[Jeff Walden [:Waldo]]

Attachment: b2g26-targeted patch

Comment 23

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/b2g-inbound/rev/806ccbfd6fdf

Comment 24

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/806ccbfd6fdf

Comment 25

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-b2g26_v1_2/rev/5bafaa326de5
https://hg.mozilla.org/releases/mozilla-b2g18/rev/a9fa9a04390d

Comment 26

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-b2g18_v1_1_0_hd/rev/a9fa9a04390d