Compartment mismatch in MMI code

RESOLVED FIXED in Firefox 35, Firefox OS v2.2

Status

RESOLVED FIXED
4 years ago
4 years ago

People

(Reporter: khuey, Assigned: edgar)

Tracking

({assertion, regression, sec-moderate})

unspecified
2.1 S5 (26sep)
ARM
Gonk (Firefox OS)
assertion, regression, sec-moderate
Dependency tree / graph

Firefox Tracking Flags

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

Details

Attachments

(3 attachments, 3 obsolete attachments)

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.

Comment 3

4 years ago
(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.

Comment 5

4 years ago
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.)
(Assignee)

Comment 15

4 years ago
(In reply to Olli Pettay [:smaug] from comment #11)
> Edgar, could you take a look at this?

Sure.
Assignee: nobody → echen
Flags: needinfo?(echen)
(Assignee)

Comment 16

4 years ago
Created attachment 8493639 [details] [diff] [review]
Patch, v1
(Assignee)

Comment 17

4 years ago
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....
(Assignee)

Comment 20

4 years ago
(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
status-b2g-v1.4: --- → unaffected
status-b2g-v2.0: --- → unaffected
status-b2g-v2.1: --- → unaffected
status-b2g-v2.2: --- → affected
Keywords: regression
(Assignee)

Comment 22

4 years ago
Created attachment 8494436 [details] [diff] [review]
Part 1: Use the right compartment, v2

Address comment #18.
- Don't use AutoSafeJSContext.
Attachment #8493639 - Attachment is obsolete: true
(Assignee)

Comment 23

4 years ago
Created attachment 8494440 [details] [diff] [review]
Part 2: Remove AutoSafeJSContext, v1
(Assignee)

Updated

4 years ago
Attachment #8494436 - Flags: review?(bzbarsky)
Attachment #8494436 - Flags: review?(bugs)
(Assignee)

Comment 24

4 years ago
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.
(Assignee)

Updated

4 years ago
Blocks: 1072710
(Assignee)

Comment 30

4 years ago
(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.
(Assignee)

Comment 31

4 years ago
Created attachment 8495024 [details] [diff] [review]
Part 1: Use the right compartment, v3

Rebase and address comment #25.
Attachment #8494436 - Attachment is obsolete: true
(Assignee)

Comment 32

4 years ago
Created attachment 8495029 [details] [diff] [review]
Part 2: Remove AutoSafeJSContext, v2

Rebase and address comment #25.
Attachment #8494440 - Attachment is obsolete: true
(Assignee)

Comment 33

4 years ago
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)
(Assignee)

Updated

4 years ago
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
Last Resolved: 4 years ago
status-b2g-v2.2: affected → fixed
status-firefox33: --- → unaffected
status-firefox34: --- → unaffected
status-firefox35: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → 2.1 S5 (26sep)
status-firefox-esr31: --- → unaffected
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.