Closed
Bug 1070083
Opened 10 years ago
Closed 10 years ago
Compartment mismatch in MMI code
Categories
(Firefox OS Graveyard :: RIL, defect)
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)
12.24 KB,
text/plain
|
Details | |
10.91 KB,
patch
|
bholley
:
review+
|
Details | Diff | Splinter Review |
7.46 KB,
patch
|
bholley
:
review+
|
Details | Diff | Splinter Review |
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.
Reporter | ||
Comment 1•10 years ago
|
||
Comment 2•10 years ago
|
||
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•10 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.
Reporter | ||
Comment 4•10 years ago
|
||
The lack of debug testing on b2g means we're never going to find these issues either.
Comment 5•10 years ago
|
||
And we need to change JSAPI usage so that compartment handling is explicit. No plain JS::Values as params. Always JSContext too.
Comment 6•10 years ago
|
||
> 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?
Reporter | ||
Comment 7•10 years ago
|
||
We could, but it's not going to mean anything useful after bholley nukes JSContexts shortly is it?
Comment 8•10 years ago
|
||
That's a good point...
Reporter | ||
Comment 9•10 years ago
|
||
[Blocking Requested - why for this release]: this appears to be a regression from bug 843452.
Blocks: 843452
blocking-b2g: --- → 2.1?
Comment 10•10 years ago
|
||
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?
Comment 12•10 years ago
|
||
bug 1070222 is about generic improvements for compartment handling.
Reporter | ||
Comment 13•10 years ago
|
||
(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
Comment 14•10 years ago
|
||
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•10 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•10 years ago
|
||
Assignee | ||
Comment 17•10 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)
Comment 18•10 years ago
|
||
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....
Comment 19•10 years ago
|
||
Comment on attachment 8493639 [details] [diff] [review] Patch, v1 Yeah, AutoJSAPI is the thing to use these days. http://mxr.mozilla.org/mozilla-central/source/dom/base/ScriptSettings.h?rev=cafe6d84215b&mark=371-371#368
Attachment #8493639 -
Flags: review?(bugs) → review-
Assignee | ||
Comment 20•10 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. :)
Comment 21•10 years ago
|
||
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
Updated•10 years ago
|
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•10 years ago
|
||
Address comment #18. - Don't use AutoSafeJSContext.
Attachment #8493639 -
Attachment is obsolete: true
Assignee | ||
Comment 23•10 years ago
|
||
Assignee | ||
Updated•10 years ago
|
Attachment #8494436 -
Flags: review?(bzbarsky)
Attachment #8494436 -
Flags: review?(bugs)
Assignee | ||
Comment 24•10 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 25•10 years ago
|
||
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.
Comment 26•10 years ago
|
||
(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 27•10 years ago
|
||
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 28•10 years ago
|
||
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)
Comment 29•10 years ago
|
||
Filed bug 1072315 as a proposal to make junkscope usage a bit more understandable.
Assignee | ||
Comment 30•10 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•10 years ago
|
||
Rebase and address comment #25.
Attachment #8494436 -
Attachment is obsolete: true
Assignee | ||
Comment 32•10 years ago
|
||
Rebase and address comment #25.
Attachment #8494440 -
Attachment is obsolete: true
Assignee | ||
Comment 33•10 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•10 years ago
|
Attachment #8495029 -
Flags: review?(bobbyholley)
Updated•10 years ago
|
Attachment #8495024 -
Flags: review?(bobbyholley) → review+
Comment 34•10 years ago
|
||
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+
Assignee | ||
Comment 35•10 years ago
|
||
Try result: https://tbpl.mozilla.org/?tree=Try&rev=39be4790fd0f
Assignee | ||
Comment 36•10 years ago
|
||
https://hg.mozilla.org/integration/b2g-inbound/rev/90fc951b321c https://hg.mozilla.org/integration/b2g-inbound/rev/10346199c21b
Comment 37•10 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/90fc951b321c https://hg.mozilla.org/mozilla-central/rev/10346199c21b
Status: NEW → RESOLVED
Closed: 10 years ago
status-firefox33:
--- → unaffected
status-firefox34:
--- → unaffected
status-firefox35:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → 2.1 S5 (26sep)
Updated•10 years ago
|
status-firefox-esr31:
--- → unaffected
Comment 38•9 years ago
|
||
Clearing the blocking nom for 2.2? as this is already fixed/verified on that branch per the status flag
blocking-b2g: 2.2? → ---
Updated•9 years ago
|
Group: core-security
You need to log in
before you can comment on or make changes to this bug.
Description
•