Closed Bug 1070083 Opened 10 years ago Closed 10 years ago

Compartment mismatch in MMI code

Categories

(Firefox OS Graveyard :: RIL, defect)

ARM
Gonk (Firefox OS)
defect
Not set
normal

Tracking

(firefox33 unaffected, firefox34 unaffected, firefox35 fixed, firefox-esr31 unaffected, b2g-v1.4 unaffected, b2g-v2.0 unaffected, b2g-v2.1 unaffected, b2g-v2.2 fixed)

RESOLVED FIXED
2.1 S5 (26sep)
Tracking Status
firefox33 --- unaffected
firefox34 --- unaffected
firefox35 --- fixed
firefox-esr31 --- unaffected
b2g-v1.4 --- unaffected
b2g-v2.0 --- unaffected
b2g-v2.1 --- unaffected
b2g-v2.2 --- fixed

People

(Reporter: khuey, Assigned: edgar)

References

Details

(Keywords: assertion, regression, sec-moderate)

Attachments

(3 files, 3 obsolete files)

STR:

1. Flash the device with a debug build.
2. Boot the device and launch the Settings App
3. Go into "Device Information" and then click on "More Information"

That triggers a compartment mismatch.
Yeah, this code is just busted:

587 MobileConnectionRequestParent::NotifySendCancelMmiSuccess(JS::Handle<JS::Value> aResult)
588 {
589   AutoSafeJSContext cx;
590   RootedDictionary<MozMMIResult> result(cx);
591
592   if (!result.Init(cx, aResult)) {


What made people think that aResult was in the default compartment of the safe JS context, exactly?

This method should be taking a JSContext from the caller and using that.

Also, while we're here, it sure would be nice to get rid of all that manual jsapi for the additionalInformation bit: just use a sequence of unions, then check that they're all the same type.
(In reply to Boris Zbarsky [:bz] from comment #2)
> MobileConnectionRequestParent::NotifySendCancelMmiSuccess(JS::Handle<JS::
> Value> aResult)
> 588 {
> 589   AutoSafeJSContext cx;
> 590   RootedDictionary<MozMMIResult> result(cx);
> 591
> 592   if (!result.Init(cx, aResult)) {
> 
> 
> What made people think that aResult was in the default compartment of the
> safe JS context, exactly?
Compartments are black magic, and hard for reviewers to catch compartment issues.
We really need some static checking there.
The lack of debug testing on b2g means we're never going to find these issues either.
And we need to change JSAPI usage so that compartment handling is explicit.
No plain JS::Values as params. Always JSContext too.
> and hard for reviewers to catch compartment issues

Here's a simple rule of thumb in today's world.  If you're using AutoSafeJSContext, you're probably doing it wrong.....

> No plain JS::Values as params.

I was just thinking the same: can we make having a jsval param imply an implict_jscontext in xpidl?
We could, but it's not going to mean anything useful after bholley nukes JSContexts shortly is it?
That's a good point...
[Blocking Requested - why for this release]:
this appears to be a regression from bug 843452.
Blocks: 843452
blocking-b2g: --- → 2.1?
Bug 843452 wasn't in v2.1 branch so this wouldn't block 2.1, but blocks 2.2 maybe.
blocking-b2g: 2.1? → 2.2?
Edgar, could you take a look at this?
Flags: needinfo?(echen)
bug 1070222 is about generic improvements for compartment handling.
(In reply to Hsin-Yi Tsai (OOO Sep. 22) [:hsinyi] from comment #10)
> Bug 843452 wasn't in v2.1 branch so this wouldn't block 2.1, but blocks 2.2
> maybe.

That 2.1S[4-6] means 2.2 is not confusing at all :P
Or Edgar, if you don't have time, feel free to assign this to me.
(I really should have caught this when reviewing.)
(In reply to Olli Pettay [:smaug] from comment #11)
> Edgar, could you take a look at this?

Sure.
Assignee: nobody → echen
Flags: needinfo?(echen)
Attached patch Patch, v1 (obsolete) — Splinter Review
Comment on attachment 8493639 [details] [diff] [review]
Patch, v1

I found some other codes also have the compartment issue, I fix them in this patch as well. Could you help to review this patch for me? Thank you.
Attachment #8493639 - Flags: review?(bugs)
Wait.  Why the changes from AutoJSAPI to AutoSafeJSContext?

AutoSafeJSContext is deprecated and shouldn't be used in new code.  Not least because it does not enter a useful compartment!

And please do file a followup of getting rid of the array JSAPI bits in there....
(In reply to Boris Zbarsky [:bz] from comment #18)
> 
> And please do file a followup of getting rid of the array JSAPI bits in
> there....

We are working on getting rid of "jsval" in mobileConnection interface for some reason (please see bug 1047196), those array JSAPI bits will be removed there as well. :)
This sounds like a bad crash, but I assume this isn't exposed to web content, which will make exploiting it difficult, so I'm marking it moderate.
Keywords: sec-moderate
Address comment #18.
- Don't use AutoSafeJSContext.
Attachment #8493639 - Attachment is obsolete: true
Attachment #8494436 - Flags: review?(bzbarsky)
Attachment #8494436 - Flags: review?(bugs)
Comment on attachment 8494440 [details] [diff] [review]
Part 2: Remove AutoSafeJSContext, v1

In most of cases we don't need to enter some compartment, so use AutoJSContext.
Attachment #8494440 - Flags: review?(bzbarsky)
Attachment #8494440 - Flags: review?(bugs)
Comment on attachment 8494440 [details] [diff] [review]
Part 2: Remove AutoSafeJSContext, v1

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

::: dom/mobileconnection/ipc/MobileConnectionParent.cpp
@@ +433,5 @@
> +  // Junk Scope.
> +  // Note that we are going to get rid of the "jsval" used in MobileConnection's
> +  // interface in bug 1047196, then we don't need these things.
> +  AutoJSContext cx;
> +  JSAutoCompartment ac(cx, xpc::PrivilegedJunkScope());

This should be:

AutoJSAPI jsapi;
if (NS_WARN_IF(!jsapi.Init(xpc::PrivilegedJunkScope())) {
  return false;
}
JSContext* cx = jsapi.cx();


Note that using xpc::PrivilegedJunkScope requires explicit case-by-case approval from the XPConnect module owner (me), along with a comment indicating that I have approved the usage. Given the ongoing work in bug 1047196, I'm ok with this as a temporary workaround. But please do the following:

* Annotate these cases with a comment indicating that bholley has approved it.
* File a followup bug dependent on bug 1047196 to make sure that all junk scope usage has been removed from dom/mobileconnection.
(In reply to Bobby Holley (:bholley) from comment #25)

> Note that using xpc::PrivilegedJunkScope requires explicit case-by-case
> approval from the XPConnect module owner (me)

This obviously hints that our setup is rather broken. I guess we should have a talk during the work week how to make
JSAPI usage sane.
Comment on attachment 8494436 [details] [diff] [review]
Part 1: Use the right compartment, v2

I think bholley should then review these.
Attachment #8494436 - Flags: review?(bzbarsky)
Attachment #8494436 - Flags: review?(bugs)
Comment on attachment 8494440 [details] [diff] [review]
Part 2: Remove AutoSafeJSContext, v1

... since I can't keep up with the latest xpconnect setup.
Attachment #8494440 - Flags: review?(bzbarsky)
Attachment #8494440 - Flags: review?(bugs)
Filed bug 1072315 as a proposal to make junkscope usage a bit more understandable.
Blocks: 1072710
(In reply to Bobby Holley (:bholley) from comment #25)
>
> * File a followup bug dependent on bug 1047196 to make sure that all junk
> scope usage has been removed from dom/mobileconnection.

Filed bug 1072710, thank you.
Rebase and address comment #25.
Attachment #8494436 - Attachment is obsolete: true
Rebase and address comment #25.
Attachment #8494440 - Attachment is obsolete: true
Comment on attachment 8495024 [details] [diff] [review]
Part 1: Use the right compartment, v3

Hi bholley, may I have your review? Thank you.
Attachment #8495024 - Flags: review?(bobbyholley)
Attachment #8495029 - Flags: review?(bobbyholley)
Attachment #8495024 - Flags: review?(bobbyholley) → review+
Comment on attachment 8495029 [details] [diff] [review]
Part 2: Remove AutoSafeJSContext, v2

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

Looks great. Thanks for doing that. r=bholley
Attachment #8495029 - Flags: review?(bobbyholley) → review+
https://hg.mozilla.org/mozilla-central/rev/90fc951b321c
https://hg.mozilla.org/mozilla-central/rev/10346199c21b
Status: NEW → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Target Milestone: --- → 2.1 S5 (26sep)
Clearing the blocking nom for 2.2? as this is already fixed/verified on that branch per the status flag
blocking-b2g: 2.2? → ---
Group: core-security
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: