904298 - Implement a way to validate and properly handle permission requests coming from browser tabs

Status: RESOLVED FIXED

(Core :: DOM: Core & HTML, defect)

Description

[Reuben Morais [:reuben]]

Right now APIs that need to prompt for permissions implement the check/prompt logic by themselves. We want to have a unified mechanism for doing this, and eventually move all permission logic to use it.

Comment 1

[Reuben Morais [:reuben]]

Attachment: WIP

bent, can you take a quick look at this? Am I going in a completely wrong direction?

Comment 2

[Reuben Morais [:reuben]]

Attachment: 1 - Implement mozilla::CheckPermission, v1

So CheckPermission takes a principal and a permission name, validates the principal and returns the permission value. Consumers can check the return value and prompt if necessary.

Comment 3

[Reuben Morais [:reuben]]

Attachment: 2 - Use CheckPermission for Geolocation and Notifications

This converts geolocation and desktop-notification checks (which are exposed to Web content) to using CheckPermission. We can then move the prompting code away from the child.

Comment 4

[Reuben Morais [:reuben]]

Attachment: 3 - Move PContentPermissionRequest from PBrowser to PContent

This will allow us to move the prompting logic to the parent. I think this was in PBrowser because it forwards the frame element to nsIContentPermissionPrompt, but our implementation already handles a null frame by forwarding the request to the browser window, so it looks like the change is fine.

Comment 5

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

Comment on attachment 789882 [details] [diff] [review]
1 - Implement mozilla::CheckPermission, v1

Review of attachment 789882 [details] [diff] [review]:
-----------------------------------------------------------------

::: dom/ipc/AppProcessChecker.cpp
@@ +137,5 @@
> +
> +  nsString manifestURL;
> +  appService->GetManifestURLByLocalId(aPrincipal->GetAppId(), manifestURL);
> +  NS_ENSURE_TRUE(manifestURL != EmptyString(),
> +                 nsIPermissionManager::UNKNOWN_ACTION);

I don't think you need to do this. If there is no manifestURL for the appId then the permission manager really shouldn't contain any entries for that appId.

@@ +144,5 @@
> +    do_GetService(NS_PERMISSIONMANAGER_CONTRACTID);
> +  NS_ENSURE_TRUE(pm, nsIPermissionManager::UNKNOWN_ACTION);
> +
> +  uint32_t permission = nsIPermissionManager::UNKNOWN_ACTION;
> +  nsresult rv = pm->TestPermissionFromPrincipal(aPrincipal, aPermission,

Use TestExactPermissionFromPrincipal here

Comment 6

[Reuben Morais [:reuben]]

Attachment: 1 - Implement mozilla::CheckPermission, v2

As discussed on IRC, with review comments from previous version addressed.

Comment 7

[Reuben Morais [:reuben]]

Attachment: 2 - Use CheckPermission for Geolocation and Notifications, v2

Update consumers to use the API in CheckPermission v2. Interdiff:

diff -u b/dom/ipc/ContentParent.cpp b/dom/ipc/ContentParent.cpp
--- b/dom/ipc/ContentParent.cpp
+++ b/dom/ipc/ContentParent.cpp
@@ -2426,7 +2426,8 @@
                                          const nsString& aBidi, const nsString& aLang,
                                          const IPC::Principal& aPrincipal)
 {
-    uint32_t permission = mozilla::CheckPermission(aPrincipal, "desktop-notification");
+    uint32_t permission = mozilla::CheckPermission(this, aPrincipal,
+                                                   "desktop-notification");
     if (permission != nsIPermissionManager::ALLOW_ACTION) {
         return true;
     }
@@ -2443,7 +2444,8 @@
 ContentParent::RecvCloseAlert(const nsString& aName,
                               const IPC::Principal& aPrincipal)
 {
-    uint32_t permission = mozilla::CheckPermission(aPrincipal, "desktop-notification");
+    uint32_t permission = mozilla::CheckPermission(this, aPrincipal,
+                                                   "desktop-notification");
     if (permission != nsIPermissionManager::ALLOW_ACTION) {
         return true;
     }
@@ -2525,7 +2527,8 @@
 ContentParent::RecvAddGeolocationListener(const IPC::Principal& aPrincipal,
                                           const bool& aHighAccuracy)
 {
-  uint32_t permission = mozilla::CheckPermission(aPrincipal, "geolocation");
+  uint32_t permission = mozilla::CheckPermission(this, aPrincipal,
+                                                 "geolocation");
   if (permission != nsIPermissionManager::ALLOW_ACTION) {
     return true;
   }

Comment 8

[Reuben Morais [:reuben]]

Attachment: 1 - Implement mozilla::CheckPermission, v3

I forgot to qref a couple #includes.

Comment 9

[Reuben Morais [:reuben]]

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

Comment 10

[Reuben Morais [:reuben]]

Comment on attachment 789907 [details] [diff] [review]
3 - Move PContentPermissionRequest from PBrowser to PContent

So this is trickier than I suspected. shell.js monitors visibility of the frame element to remove some notifications, for example. Maybe we can find the PBrowser matching the passed Principal and then use that for prompting. I'm still not sure what's the best way forward here, so I'm investigating the possible options.

Comment 11

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

Comment on attachment 792855 [details] [diff] [review]
1 - Implement mozilla::CheckPermission, v3

Review of attachment 792855 [details] [diff] [review]:
-----------------------------------------------------------------

r=me with that fixed

::: dom/ipc/AppProcessChecker.cpp
@@ +144,5 @@
> +
> +  // If the child only runs inBrowserElement content and the principal claims
> +  // it's not browser element, it's lying.
> +  bool inBrowserElementMismatch = tab->IsBrowserElement() &&
> +                                  !aPrincipal->GetIsInBrowserElement();

You should do this inside the loop. So that you cover the case of when a single process has both PBrowsers that run the app itself as well as browserElements.

Comment 12

[Reuben Morais [:reuben]]

Jonas, can we land these patches the way they are to fix the notifications crash (bug 901214) and then keep working here?

Comment 13

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

Comment on attachment 792855 [details] [diff] [review]
1 - Implement mozilla::CheckPermission, v3

Review of attachment 792855 [details] [diff] [review]:
-----------------------------------------------------------------

::: dom/ipc/AppProcessChecker.cpp
@@ +161,5 @@
> +{
> +  if (!IsPrincipalValid(aActor, aPrincipal)) {
> +    // Lying children receive the king's justice.
> +    NS_WARNING("Principal is invalid, killing app process");
> +    static_cast<ContentParent*>(aActor)->KillHard();

Actually, move this KillHard call into IsPrincipalValid

Comment 14

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

Comment on attachment 792548 [details] [diff] [review]
2 - Use CheckPermission for Geolocation and Notifications, v2

Review of attachment 792548 [details] [diff] [review]:
-----------------------------------------------------------------

I don't see anywhere that these new checks check that "desktop-notification" is an app permission before it permits it to be used by any origin within the app?

::: dom/ipc/ContentParent.cpp
@@ +2428,5 @@
> +{
> +    uint32_t permission = mozilla::CheckPermission(this, aPrincipal,
> +                                                   "desktop-notification");
> +    if (permission != nsIPermissionManager::ALLOW_ACTION) {
> +        return true;

Shouldn't this kill the child process as well?

@@ +2446,5 @@
> +{
> +    uint32_t permission = mozilla::CheckPermission(this, aPrincipal,
> +                                                   "desktop-notification");
> +    if (permission != nsIPermissionManager::ALLOW_ACTION) {
> +        return true;

Same here

Comment 15

[Jason Smith [:jsmith]]

blocks a blocker - koi+

Comment 16

[Reuben Morais [:reuben]]

(In reply to Jonas Sicking (:sicking) (vacation until 9/9. In norway until 9/16) from comment #14)
> Comment on attachment 792548 [details] [diff] [review]
> 2 - Use CheckPermission for Geolocation and Notifications, v2
> 
> Review of attachment 792548 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> I don't see anywhere that these new checks check that "desktop-notification"
> is an app permission before it permits it to be used by any origin within
> the app?

We should only check if "desktop-notification" is an app permission when the principal is NOT inBrowserElement, right? Browser tabs won't have the permission, but we should still prompt/grant permission to that origin when requested. In that case, can put that check in CheckPermission.

> ::: dom/ipc/ContentParent.cpp
> @@ +2428,5 @@
> > +{
> > +    uint32_t permission = mozilla::CheckPermission(this, aPrincipal,
> > +                                                   "desktop-notification");
> > +    if (permission != nsIPermissionManager::ALLOW_ACTION) {
> > +        return true;
> 
> Shouldn't this kill the child process as well?

We don't want to kill browser tabs that use the Notifications API without having permission…

Comment 17

[Jason Smith [:jsmith]]

Apparently the block this blocks no longer requires this implementation - it's works for me on 9/12/2013 build. As such, renominating to minus as this no longer blocks a blocker.

Comment 18

[Reuben Morais [:reuben]]

To whoever makes that decision, note that "no longer requires this implementation" means Notifications are now never leaving the child, bypassing the permission system.

Comment 19

[Jason Smith [:jsmith]]

(In reply to Reuben Morais [:reuben] from comment #18)
> To whoever makes that decision, note that "no longer requires this
> implementation" means Notifications are now never leaving the child,
> bypassing the permission system.

Ah, so that actually sounds bad then. What's the implications specifically?

Comment 20

[Reuben Morais [:reuben]]

(In reply to Jason Smith [:jsmith] from comment #19)
> (In reply to Reuben Morais [:reuben] from comment #18)
> > To whoever makes that decision, note that "no longer requires this
> > implementation" means Notifications are now never leaving the child,
> > bypassing the permission system.
> 
> Ah, so that actually sounds bad then. What's the implications specifically?

Well, it's not a big deal: if you manage to compromise a child process, you can DOS the phone with Notifications :)

Comment 21

[Andrea Marchesini [:baku]]

Comment on attachment 792855 [details] [diff] [review]
1 - Implement mozilla::CheckPermission, v3

Review of attachment 792855 [details] [diff] [review]:
-----------------------------------------------------------------

::: dom/ipc/AppProcessChecker.cpp
@@ +126,5 @@
>  }
>  
> +bool
> +IsPrincipalValid(PContentParent* aActor,
> +                 nsIPrincipal* aPrincipal)

Implement IsPrincipalValid also when MOZ_CHILD_PERMISSIONS is not defined. Like this:

bool
IsPrincipalValid(mozilla::dom::PContentParent* aParent,
                 nsIPrincipal* aPrincipal)
{
  return true;
}

Comment 22

[Andrea Marchesini [:baku]]

Attachment: patch 2 - renaming IsPrincipalValid to AssertAppPrincipal

Reuben, can you use this piece of code? This is needed by bug 916091 - read the comment 15.

Can you tell me when these patches should land?

Comment 23

[Reuben Morais [:reuben]]

(In reply to Andrea Marchesini (:baku) from comment #22)
> Created attachment 815892 [details] [diff] [review]
> patch 2 - renaming IsPrincipalValid to AssertAppPrincipal
> 
> Reuben, can you use this piece of code? This is needed by bug 916091 - read
> the comment 15.

Yep.

> Can you tell me when these patches should land?

I'm blocked by not having a working device to test this code :( As soon as I can flash a dev image on my Hamachi I will land it.

Comment 24

[Reuben Morais [:reuben]]

Comment on attachment 815892 [details] [diff] [review]
patch 2 - renaming IsPrincipalValid to AssertAppPrincipal

Review of attachment 815892 [details] [diff] [review]:
-----------------------------------------------------------------

LGTM

Comment 25

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

Comment on attachment 792548 [details] [diff] [review]
2 - Use CheckPermission for Geolocation and Notifications, v2

Review of attachment 792548 [details] [diff] [review]:
-----------------------------------------------------------------

I think there's agreement that this isn't quite ready yet. Please re-request review if that's not the case.

Comment 26

[Andrea Marchesini [:baku]]

> I'm blocked by not having a working device to test this code :( As soon as I
> can flash a dev image on my Hamachi I will land it.

You can use the emulator. It's easier to debug code with it.

Comment 27

[Reuben Morais [:reuben]]

Attachment: 1 - Implement mozilla::{AssertAppPrincipal,CheckPermission}, v4

There we go, sorry for the delays Andrea!

Comment 28

[Reuben Morais [:reuben]]

Comment on attachment 817615 [details] [diff] [review]
1 - Implement mozilla::{AssertAppPrincipal,CheckPermission}, v4

Review of attachment 817615 [details] [diff] [review]:
-----------------------------------------------------------------

::: dom/ipc/AppProcessChecker.cpp
@@ +171,5 @@
> +
> +  // If the permission is not in a browser element, make sure that aPermission
> +  // is an app permission before checking the origin.
> +  if (!aPrincipal->GetIsInBrowserElement() &&
> +      !AssertAppProcess(aActor, ASSERT_APP_HAS_PERMISSION, aPermission)) {

Hmm. Using AssertAppProcess here is probably not a good idea since it'll kill the app if the permission is not granted. Should I get the app principal using secMan and do this check manually?

Comment 29

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

Comment on attachment 817615 [details] [diff] [review]
1 - Implement mozilla::{AssertAppPrincipal,CheckPermission}, v4

Review of attachment 817615 [details] [diff] [review]:
-----------------------------------------------------------------

::: dom/ipc/AppProcessChecker.cpp
@@ +172,5 @@
> +  // If the permission is not in a browser element, make sure that aPermission
> +  // is an app permission before checking the origin.
> +  if (!aPrincipal->GetIsInBrowserElement() &&
> +      !AssertAppProcess(aActor, ASSERT_APP_HAS_PERMISSION, aPermission)) {
> +    return nsIPermissionManager::DENY_ACTION;

I don't understand this part. Can you describe why it's needed?

@@ +182,5 @@
> +
> +  uint32_t permission = nsIPermissionManager::UNKNOWN_ACTION;
> +  nsresult rv = pm->TestExactPermissionFromPrincipal(aPrincipal, aPermission,
> +                                                     &permission);
> +  NS_ENSURE_SUCCESS(rv, nsIPermissionManager::UNKNOWN_ACTION);

What I think is missing is that we for all content want to check not just that the passed in principal has permission, but also that the "app principal" has the permission. I.e. that the permission was enumerated in the manifest.

This requirement doesn't apply to all permissions, but I think it'll apply to all permissions that come through this function. At least for now.

Comment 30

[Reuben Morais [:reuben]]

(In reply to Jonas Sicking (:sicking) Please ni? if you want feedback from comment #29)
> ::: dom/ipc/AppProcessChecker.cpp
> @@ +172,5 @@
> > +  // If the permission is not in a browser element, make sure that aPermission
> > +  // is an app permission before checking the origin.
> > +  if (!aPrincipal->GetIsInBrowserElement() &&
> > +      !AssertAppProcess(aActor, ASSERT_APP_HAS_PERMISSION, aPermission)) {
> > +    return nsIPermissionManager::DENY_ACTION;
> 
> I don't understand this part. Can you describe why it's needed?

It checks if the app principal has the permission, no? https://mxr.mozilla.org/mozilla-central/source/dom/apps/src/AppsUtils.jsm#42
Though I'll probably have to check this manually to avoid killing the process unnecessarily.

> @@ +182,5 @@
> > +
> > +  uint32_t permission = nsIPermissionManager::UNKNOWN_ACTION;
> > +  nsresult rv = pm->TestExactPermissionFromPrincipal(aPrincipal, aPermission,
> > +                                                     &permission);
> > +  NS_ENSURE_SUCCESS(rv, nsIPermissionManager::UNKNOWN_ACTION);
> 
> What I think is missing is that we for all content want to check not just
> that the passed in principal has permission, but also that the "app
> principal" has the permission. I.e. that the permission was enumerated in
> the manifest.
> 
> This requirement doesn't apply to all permissions, but I think it'll apply
> to all permissions that come through this function. At least for now.

See above.

Comment 31

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

Yeah, you shouldn't call AssertAppProcess here as it'll kill the child process if the permission isn't already granted. That's not expected behavior from a function named "CheckPermission" I'd say. It also makes it inconsistent since you're not killing the child if the security check further down fails.

Also, you should *always* do this check, even if aPrincipal->GetIsInBrowserElement() is true.

Also, I think it's ok for the application not to have ALLOW_PERMISSION if it has PROMPT_PERMISSION. However we should always return the minimum permission level between the "app principal" and the passed in principal. So if the app principal says DENY we should deny, if the app says PROMPT we should prompt (unless the passed in principal says DENY in which case we should deny). Etc.

Comment 32

[Reuben Morais [:reuben]]

Blocks a blocker.

Comment 33

[Reuben Morais [:reuben]]

Attachment: 1 - Implement mozilla::{AssertAppPrincipal,CheckPermission}, v5

- Checking the app principal first. (Is there a better way to get it from the passed in principal?)
- Returning min(appPermission, permission) if neither is DENY/UNKNOWN (so that it'll prompt if one of them is PROMPT and the other is ALLOW)

I've been running into several problems with my B2G environment so this is largely untested (it works for notifications in the browser).

Comment 34

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

Comment on attachment 824818 [details] [diff] [review]
1 - Implement mozilla::{AssertAppPrincipal,CheckPermission}, v5

Review of attachment 824818 [details] [diff] [review]:
-----------------------------------------------------------------

Close, but still needs a couple of fixes.

::: dom/ipc/AppProcessChecker.cpp
@@ +139,5 @@
> +bool
> +AssertAppPrincipal(PContentParent* aActor,
> +                   nsIPrincipal* aPrincipal)
> +{
> +  NS_ENSURE_TRUE(aPrincipal, false);

You should probably call KillHard here too. That way we always kill before returning false.

@@ +168,5 @@
> +nsIPrincipal*
> +GetAppPrincipal(nsIPrincipal* aPrincipal)
> +{
> +  nsIURI* uri;
> +  nsresult rv = aPrincipal->GetURI(&uri);

No, this is not the right URI. You need to get the appid from aPrincipal. Then call the app manager and get the manifest URL for that appid.

The safest thing is probably to make this function take an appid rather than a principal since that's the only part of aPrincipal that you should use.

@@ +177,5 @@
> +  nsCOMPtr<nsIScriptSecurityManager> secMan =
> +    do_GetService(NS_SCRIPTSECURITYMANAGER_CONTRACTID);
> +
> +  nsIPrincipal* appPrincipal;
> +  rv = secMan->GetAppCodebasePrincipal(uri, appId, true, &appPrincipal);

The third argument should be 'false' here.

@@ +215,5 @@
> +  }
> +
> +  // If neither of the permissions are DENY_ACTION, we return the smaller
> +  // level (if app says PROMPT but principal says DENY, we deny).
> +  return std::min(appPerm, permission);

I'd rather not rely on the numeric values of the *_ACTION enums. So just do something like:

if (appPerm == PROMPT_ACTION || permission == PROMPT_ACTION)
  return PROMPT_ACTION;

if (appPerm == ALLOW_ACTION || permission == ALLOW_ACTION)
  return ALLOW_ACTION;

NS_RUNTIMEABORT();

Comment 35

[Reuben Morais [:reuben]]

Attachment: 1 - Implement mozilla::{AssertAppPrincipal,CheckPermission}, v6

Comment 36

[Reuben Morais [:reuben]]

Attachment: interdiff v5 v6

Comment 37

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

Comment on attachment 826723 [details] [diff] [review]
1 - Implement mozilla::{AssertAppPrincipal,CheckPermission}, v6

Review of attachment 826723 [details] [diff] [review]:
-----------------------------------------------------------------

r=me with the below fixed. Yay!

::: dom/ipc/AppProcessChecker.cpp
@@ +170,5 @@
> +  return false;
> +}
> +
> +nsIPrincipal*
> +GetAppPrincipal(uint32_t aAppId)

Change the return type here to already_AddRefed<nsIPrincipal>

@@ +201,5 @@
> +    do_GetService(NS_PERMISSIONMANAGER_CONTRACTID);
> +  NS_ENSURE_TRUE(pm, nsIPermissionManager::DENY_ACTION);
> +
> +  // Make sure that `aPermission' is an app permission before checking the origin.
> +  nsIPrincipal* appPrincipal = GetAppPrincipal(aPrincipal->GetAppId());

Change this to

nsCOMPtr<nsIPrincpal> = GetAppPrincipal(...);

Right now you are leaking the principal. The rule of thumb is, never return a addrefed raw pointer. Either use out arguments, or preferably, return an already_AddRefed. That way the type system will help you not leak.

Comment 38

[Reuben Morais [:reuben]]

Thanks for the reviews :)

I was going to use this bug to switch some users of AssertAppProcess to CheckPermission but by now new bugs have been filed, so I'll do that work in those bugs.

https://hg.mozilla.org/integration/b2g-inbound/rev/260b222e5f63

Comment 39

[Reuben Morais [:reuben]]

Let's try that again.

https://hg.mozilla.org/integration/b2g-inbound/rev/6a23d5d65b4d

Comment 40

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/6a23d5d65b4d

Comment 41

[Jason Smith [:jsmith]]

Reuben - Does the landing of this patch automatically fix bug 920302?

Comment 42

[Reuben Morais [:reuben]]

(In reply to Jason Smith [:jsmith] from comment #41)
> Reuben - Does the landing of this patch automatically fix bug 920302?

No, but it makes fixing that bug really easy. I'm working on a patch right now.

Comment 43

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-b2g26_v1_2/rev/88e841e3d6d6

Comment 44

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/8ce724c02b7c

Comment 45

[Philip Chee]

http://mxr.mozilla.org/comm-central/source/mozilla/toolkit/components/alerts/nsIAlertsService.idl#35
Please update the comment in the IDL to reflect the addition of an extra parameter.

Comment 46

[Gregor Wagner [:gwagner]]

The patch for bug 920302 had the wrong commit message and points to this bug.