Closed Bug 1030997 Opened 6 years ago Closed 6 years ago

nsFrameMessageManager does not reconstruct principal object on receiveMessage

Categories

(Core :: DOM: Content Processes, defect)

x86_64
Linux
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla34

People

(Reporter: qdot, Assigned: baku)

References

Details

Attachments

(1 file)

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.
Attached patch principal.patchSplinter Review
Smaug, can we expose nsIPrincipal here in this way? Tests pass but maybe you don't like it.
Attachment #8467757 - Flags: review?(bugs)
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.
Attachment #8467757 - Flags: review?(bugs)
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.
Flags: needinfo?(kyle)
Comment on attachment 8467757 [details] [diff] [review]
principal.patch

Hmm, or maybe we have strong enough runtime assertions to make sure this is fine.
Attachment #8467757 - Flags: review?(bugs)
(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 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.
Attachment #8467757 - Flags: review?(bugs) → review+
Comment on attachment 8467757 [details] [diff] [review]
principal.patch

Or, hmm, the assertions aren't very strict.
I wonder why.
Attachment #8467757 - Flags: review+ → review?(bugs)
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.
Attachment #8467757 - Flags: review?(bugs) → review+
Flags: needinfo?(kyle)
https://hg.mozilla.org/mozilla-central/rev/ae218ffbbc9c
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla34
You need to log in before you can comment on or make changes to this bug.