818167 - Handle IsBrowserElement permissions correctly

Status: RESOLVED DUPLICATE of bug 829030

(Core :: DOM: Device Interfaces, defect)

Description

[Gregor Wagner [:gwagner]]

Bug 817758 was marked as fixed so I create a new one.

Currently we can't assert on the parent side that a permission was granted to a web page in the browser.

https://hg.mozilla.org/mozilla-central/file/0035f77045bc/dom/ipc/AppProcessPermissions.cpp#l36

Comment 1

[Justin Lebar (not reading bugmail)]

I said this on IRC to gwagner, but to say it here: I am not convinced this is important.

In our architecture, we have to trust the browser process when it says "I'm acting on behalf of URL X."  Therefore this backstop parent-side check for "does the browser process have permission P?" boils down to "has the user granted /any website/ permission P?".  This is not a particularly strong security check.

Comment 2

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

I agree with comment 1.

Ideally I'd like to have an API where we transfer nsIPrincipals to the parent process with any call that we make. Then have an API on the parent side which answers the question "can this process act on the behalf of this principal". If the answer is "no" we kill the child process, if the answer is "yes" we use the principal to do security checks just like we would in the child process.

But this won't meaningfully increase the security from what we have now.

Comment 3

[Doug Turner (:dougt)]

for v.1, should we just grant the browser process geolocation permission?

Comment 4

[Justin Lebar (not reading bugmail)]

(In reply to Doug Turner (:dougt) from comment #3)
> for v.1, should we just grant the browser process geolocation permission?

AIUI the parent-process backstop checks are named "*AppProcessPermission*" to indicate that they are only for app processes and not for browser processes.  (The comments in the header do not help clarify this, unfortunately.  I can say that because I reviewed and probably nitted on those comments.)

I would say the solution is not to call AssertAppProcessPermission for browser processes, or to rename and modify the methods so that they always return true for browser processes.

Comment 5

[Justin Lebar (not reading bugmail)]

> or to rename and modify the methods so that they always return true for browser processes.

I guess this might be more complicated, because there are some permissions you never want to grant to browser processes.  That logic needs to live somewhere; it either lives in the callers, or we push it down into AssertAppProcessHasPermission.  The former seems sane to me.

Maybe we should add an assertion to AssertAppProcessHasPermission so that we don't use it for browser processes.

Comment 6

[Doug Turner (:dougt)]

sicking, gregor and I do not believe that this blocks 813758.  Because of this, it no longer blocks.