Closed
Bug 916091
Opened 9 years ago
Closed 9 years ago
nsIMessageSender should send the nsIPrincipal with the messages
Categories
(Core :: DOM: Core & HTML, defect)
Core
DOM: Core & HTML
Tracking
()
RESOLVED
FIXED
mozilla28
People
(Reporter: baku, Assigned: baku)
References
Details
Attachments
(2 files, 2 obsolete files)
74.26 KB,
patch
|
Details | Diff | Splinter Review | |
6.18 KB,
patch
|
smaug
:
review+
|
Details | Diff | Splinter Review |
void sendAsyncMessage([optional] in AString messageName, [optional] in jsval obj, [optional] in jsval objects, [optional] in nsIPrincipal principal); jsval sendSyncMessage([optional] in AString messageName, [optional] in jsval obj, [optional] in jsval objects, [optional] in nsIPrincipal principal);
Comment 1•9 years ago
|
||
We can't trust principal coming from child, so where would we verify it is correct?
Comment 2•9 years ago
|
||
...and sicking told that appid+browser flag would be checked on the parent side. That is ok to me, I think.
Assignee | ||
Comment 3•9 years ago
|
||
Jonas, I need a feedback in order to know if this is 'safe' enough. This patch propagates informations about the principal from sendAsyncMessage/sendSyncMessage to the event received by the parent process. The event contains: "principal" { "appId" : <something>, "origin" : <the nsIPrincipal.origin>, "isInBrowserElement" : <nsIPrincipal.isInBrowserElement> } If the principal is not shared, this object will be: "principal" : { "appId" : -1, "origin" : "", "isInBrowserElement" : undefined }
Attachment #813712 -
Flags: feedback?(jonas)
Assignee | ||
Comment 4•9 years ago
|
||
Attachment #814319 -
Flags: feedback?(jonas)
Assignee | ||
Updated•9 years ago
|
Attachment #813712 -
Attachment description: principal.patch → Bug 916091 - patch 1 - nsIMessageSender should send the nsIPrincipal with the messages
Assignee | ||
Updated•9 years ago
|
Assignee: nobody → amarchesini
Assignee | ||
Comment 6•9 years ago
|
||
Jonas, any news about this feedback? I think these 2 patches are ready. Maybe I can ask smaug to review them?
Flags: needinfo?(jonas)
Comment on attachment 813712 [details] [diff] [review] Bug 916091 - patch 1 - nsIMessageSender should send the nsIPrincipal with the messages Review of attachment 813712 [details] [diff] [review]: ----------------------------------------------------------------- The general approach here seems exactly right. Though Olli should review it.
Attachment #813712 -
Flags: feedback?(jonas) → feedback+
Comment on attachment 814319 [details] [diff] [review] patch 2 - checking the principal Review of attachment 814319 [details] [diff] [review]: ----------------------------------------------------------------- This seems approximately right, though the actual IsPrincipalValid obviously needs a better implementation. However I think IsPrincipalValid should take care of killing the child process whenever it returns false. Otherwise we'll have to repeat the KillHard call everywhere, and we'd be bound to forget somewhere.
Attachment #814319 -
Flags: feedback?(jonas) → feedback+
Assignee | ||
Updated•9 years ago
|
Attachment #813712 -
Flags: review?(bugs)
Assignee | ||
Updated•9 years ago
|
Attachment #814319 -
Flags: review?(bugs)
Comment 9•9 years ago
|
||
Comment on attachment 814319 [details] [diff] [review] patch 2 - checking the principal I don't know what I should review here since IsPrincipalValid is obviously just some placeholder without any actual implementation. Other parts look fine, but this should not land with dummy IsPrincipalValid
Attachment #814319 -
Flags: review?(bugs) → review-
Comment 10•9 years ago
|
||
Comment on attachment 813712 [details] [diff] [review] Bug 916091 - patch 1 - nsIMessageSender should send the nsIPrincipal with the messages > /** > * Message managers provide a way for chrome-privileged JS code to > * communicate with each other, even across process boundaries. > * > * Message managers are separated into "parent side" and "child side". > * These don't always correspond to process boundaries, but can. For > * each child-side message manager, there is always exactly one >@@ -247,17 +248,18 @@ interface nsIMessageSender : nsIMessageL update the uuid of nsIMessageSender >@@ -295,31 +297,33 @@ interface nsISyncMessageSender : nsIMess ditto >+ JS::Rooted<JSString*> originValue(ctx, JS_GetEmptyString(JS_GetRuntime(ctx))); I think we should default to null origin, not empty string >+ JS_DefineProperty(ctx, param, "principal", principalValue, nullptr, nullptr, JSPROP_ENUMERATE); Please document this new property and its content in nsIMessageManager.idl See nsIMessageListener there
Attachment #813712 -
Flags: review?(bugs) → review+
Flags: needinfo?(jonas)
Assignee | ||
Comment 11•9 years ago
|
||
> >+ JS::Rooted<JSString*> originValue(ctx, JS_GetEmptyString(JS_GetRuntime(ctx)));
> I think we should default to null origin, not empty string
I can avoid the setting of the origin if the principal is not set. Would it be the same?
Assignee | ||
Comment 12•9 years ago
|
||
(In reply to Olli Pettay [:smaug] from comment #9) > Comment on attachment 814319 [details] [diff] [review] > patch 2 - checking the principal > > I don't know what I should review here since IsPrincipalValid is obviously > just > some placeholder without any actual implementation. ah.. IsPrincipalValid returns true only if MOZ_CHILD_PERMISSIONS is not defined. Otherwise the correct implementation is done by bug 904298. Can you take another glance of this patch?
Comment 13•9 years ago
|
||
(In reply to Andrea Marchesini (:baku) from comment #11) > > >+ JS::Rooted<JSString*> originValue(ctx, JS_GetEmptyString(JS_GetRuntime(ctx))); > > I think we should default to null origin, not empty string > > I can avoid the setting of the origin if the principal is not set. Would it > be the same? well, null and undefined are not the same... Perhaps message.principal should be null if we don't have origin and other data.
Comment 14•9 years ago
|
||
yeah, I think I'd prefer that. message object always has .principal property, but if that property is non-null, it is a valid one with .origin and other values set to something reasonable.
Comment 15•9 years ago
|
||
But ok, so I agree with sicking that IsPrincipalValid should kill, not the caller of IsPrincipalValid. And because IsPrincipalValid become more than just a check, I'd rename it to something like AssertPermission() (to be similar to AssertPermission() and AssertProcess() etc.)
Assignee | ||
Comment 16•9 years ago
|
||
Attachment #813712 -
Attachment is obsolete: true
Assignee | ||
Comment 17•9 years ago
|
||
(In reply to Olli Pettay [:smaug] from comment #15) > But ok, so I agree with sicking that IsPrincipalValid should kill, not the > caller of IsPrincipalValid. > And because IsPrincipalValid become more than just a check, I'd rename it to > something like > AssertPermission() (to be similar to AssertPermission() and AssertProcess() > etc.) Renaming and killing should be a task for bug 904298.
Assignee | ||
Updated•9 years ago
|
OS: Linux → All
Hardware: x86_64 → All
This doesn't do any permissionmanager checks so AssertPermission sounds wrong. AssertValidPrincipal maybe?
Comment 19•9 years ago
|
||
Oops, I was going to write AssertPrincipal, not Permission.
Assignee | ||
Comment 20•9 years ago
|
||
Of course this is built on top of another patch for the other bug.
Attachment #814319 -
Attachment is obsolete: true
Attachment #815895 -
Flags: review?(bugs)
Updated•9 years ago
|
Attachment #815895 -
Flags: review?(bugs) → review+
Assignee | ||
Comment 21•9 years ago
|
||
https://tbpl.mozilla.org/?tree=Try&rev=773d913e93db
Assignee | ||
Comment 22•9 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/fecda5f4a0df https://hg.mozilla.org/integration/mozilla-inbound/rev/6d3bd18a1227
https://hg.mozilla.org/mozilla-central/rev/fecda5f4a0df https://hg.mozilla.org/mozilla-central/rev/6d3bd18a1227
Status: NEW → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla28
Comment 24•9 years ago
|
||
bz pointed out that use of JS_InternString is just wrong here. That will keep the string alive forever.
Updated•3 years ago
|
Component: DOM → DOM: Core & HTML
You need to log in
before you can comment on or make changes to this bug.
Description
•