916091 - nsIMessageSender should send the nsIPrincipal with the messages

Status: RESOLVED FIXED

Description

[Andrea Marchesini [:baku]]

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

[Olli Pettay [:smaug]]

We can't trust principal coming from child, so where would we verify it is correct?

Comment 2

[Olli Pettay [:smaug]]

...and sicking told that appid+browser flag would be checked on the parent side. That is ok to me, I think.

Comment 3

[Andrea Marchesini [:baku]]

Attachment: Bug 916091 - patch 1 - nsIMessageSender should send the nsIPrincipal with the messages

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 }

Comment 4

[Andrea Marchesini [:baku]]

Attachment: patch 2 - checking the principal

Comment 6

[Andrea Marchesini [:baku]]

Jonas, any news about this feedback? I think these 2 patches are ready. Maybe I can ask smaug to review them?

Comment 7

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

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.

Comment 8

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

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.

Comment 9

[Olli Pettay [:smaug]]

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

Comment 10

[Olli Pettay [:smaug]]

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

Comment 11

[Andrea Marchesini [:baku]]

> >+        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?

Comment 12

[Andrea Marchesini [:baku]]

(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

[Olli Pettay [:smaug]]

(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

[Olli Pettay [:smaug]]

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

[Olli Pettay [:smaug]]

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

Comment 16

[Andrea Marchesini [:baku]]

Attachment: patch 1 - nsIMessageSender should send the nsIPrincipal with the messages

Comment 17

[Andrea Marchesini [:baku]]

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

Comment 18

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

This doesn't do any permissionmanager checks so AssertPermission sounds wrong. AssertValidPrincipal maybe?

Comment 19

[Olli Pettay [:smaug]]

Oops, I was going to write AssertPrincipal, not Permission.

Comment 20

[Andrea Marchesini [:baku]]

Attachment: patch 2 - checking the principal

Of course this is built on top of another patch for the other bug.

Comment 21

[Andrea Marchesini [:baku]]

https://tbpl.mozilla.org/?tree=Try&rev=773d913e93db

Comment 22

[Andrea Marchesini [:baku]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/fecda5f4a0df
https://hg.mozilla.org/integration/mozilla-inbound/rev/6d3bd18a1227

Comment 23

[Wes Kocher (:KWierso)]

https://hg.mozilla.org/mozilla-central/rev/fecda5f4a0df
https://hg.mozilla.org/mozilla-central/rev/6d3bd18a1227

Comment 24

[Olli Pettay [:smaug]]

bz pointed out that use of JS_InternString is just wrong here. That will keep the
string alive forever.