nsIMessageSender should send the nsIPrincipal with the messages

RESOLVED FIXED in mozilla28

Status

()

defect
RESOLVED FIXED
6 years ago
5 months ago

People

(Reporter: baku, Assigned: baku)

Tracking

Trunk
mozilla28
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite ?

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments, 2 obsolete attachments)

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);
We can't trust principal coming from child, so where would we verify it is correct?
...and sicking told that appid+browser flag would be checked on the parent side. That is ok to me, I think.
Blocks: 923625
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)
Attachment #814319 - Flags: feedback?(jonas)
Attachment #813712 - Attachment description: principal.patch → Bug 916091 - patch 1 - nsIMessageSender should send the nsIPrincipal with the messages
Assignee: nobody → amarchesini
Duplicate of this bug: 919824
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+
Attachment #813712 - Flags: review?(bugs)
Attachment #814319 - Flags: review?(bugs)
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 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+
> >+        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?
(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?
(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.
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.
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.)
(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.
OS: Linux → All
Hardware: x86_64 → All
This doesn't do any permissionmanager checks so AssertPermission sounds wrong. AssertValidPrincipal maybe?
Oops, I was going to write AssertPrincipal, not Permission.
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)
Attachment #815895 - Flags: review?(bugs) → review+
Depends on: 904298
Depends on: 935494
https://hg.mozilla.org/mozilla-central/rev/fecda5f4a0df
https://hg.mozilla.org/mozilla-central/rev/6d3bd18a1227
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla28
Flags: in-testsuite?
bz pointed out that use of JS_InternString is just wrong here. That will keep the
string alive forever.
Depends on: 962393
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.