1030997 - nsFrameMessageManager does not reconstruct principal object on receiveMessage

Status: RESOLVED FIXED

(Core :: DOM: Content Processes, defect)

Description

[Kyle Machulis [:qdot] [:kmachulis] (INACTIVE)]

When sending a principal as an argument in sendAsyncMessage via a MessageManager in content, the corresponding ReceiveMessage call in parent does not reconstruct the principal object. Instead, it just creates a javascript object with the appid/uri/etc. 

Not sure if we just need to add a QueryInterface set to this or if it will be more involved.

Comment 1

[Andrea Marchesini [:baku]]

Attachment: principal.patch

Smaug, can we expose nsIPrincipal here in this way? Tests pass but maybe you don't like it.

Comment 2

[Olli Pettay [:smaug][[e-mail]]]

Well, wasn't the idea that since we can't trust that principal to be right anyway (since it is coming from the child process), we'd just use a fake principal-like thing.

Comment 3

[Olli Pettay [:smaug][[e-mail]]]

Kyle, why would we need a real nsIPrincipal object? And how would it be used. It should be effectively
a null principal in chrome process, I think.

Comment 4

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 8467757 [details] [diff] [review]
principal.patch

Hmm, or maybe we have strong enough runtime assertions to make sure this is fine.

Comment 5

[Andrea Marchesini [:baku]]

(In reply to Olli Pettay [:smaug] from comment #2)
> Well, wasn't the idea that since we can't trust that principal to be right
> anyway (since it is coming from the child process), we'd just use a fake
> principal-like thing.

Sharing a full nsIPrincipal object should not be a security issue for what I can see.
But maybe I'm wrong...

Comment 6

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 8467757 [details] [diff] [review]
principal.patch

Add a check for the return value of nsContentUtils::WrapNative.

And looks like this could be fine. We don't serialize system principal ever, so
at least we can't get that kind of powerful principals from child, and
we have the assertions which kill the child process when needed.

Comment 7

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 8467757 [details] [diff] [review]
principal.patch

Or, hmm, the assertions aren't very strict.
I wonder why.

Comment 8

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 8467757 [details] [diff] [review]
principal.patch

But I guess we should can't have origin checks, and need to rely on appid.

Comment 9

[Andrea Marchesini [:baku]]

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

Comment 10

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/ae218ffbbc9c

Comment 11

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/ae218ffbbc9c