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

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

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

[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

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

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

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

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

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

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!

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

(In reply to lsblakk@mozilla.com [: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.

(In reply to Jason Smith [:jsmith] from comment #15) > (In reply to lsblakk@mozilla.com [: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.

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

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

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.

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