Closed Bug 904298 Opened 7 years ago Closed 7 years ago

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

Categories

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

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla28
blocking-b2g koi+
Tracking Status
firefox26 --- wontfix
firefox27 --- wontfix
firefox28 --- fixed
b2g-v1.2 --- fixed

People

(Reporter: reuben, Assigned: reuben)

References

Details

(Whiteboard: [systemsfe][qa-])

Attachments

(2 files, 10 obsolete files)

7.10 KB, patch
sicking
: review+
Details | Diff | Splinter Review
2.52 KB, patch
Details | Diff | Splinter Review
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.
Attached patch WIP (obsolete) — Splinter Review
bent, can you take a quick look at this? Am I going in a completely wrong direction?
Attachment #789284 - Flags: feedback?(bent.mozilla)
Attachment #789284 - Attachment is obsolete: true
Attachment #789284 - Flags: feedback?(bent.mozilla)
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.
Attachment #789882 - Flags: review?(jonas)
Attachment #789882 - Attachment description: Implement mozilla::CheckPermission, v1 → 1 - Implement mozilla::CheckPermission, v1
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.
Attachment #789890 - Flags: review?(jonas)
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.
Attachment #789907 - Flags: review?(jonas)
Blocks: 901214
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
Attachment #789882 - Flags: review?(jonas) → review+
As discussed on IRC, with review comments from previous version addressed.
Attachment #789882 - Attachment is obsolete: true
Attachment #792547 - Flags: review?(jonas)
Attachment #792547 - Attachment description: Implement mozilla::CheckPermission, v2 → 1 - Implement mozilla::CheckPermission, 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;
   }
Attachment #789890 - Attachment is obsolete: true
Attachment #789890 - Flags: review?(jonas)
Attachment #792548 - Flags: review?(jonas)
I forgot to qref a couple #includes.
Attachment #792547 - Attachment is obsolete: true
Attachment #792547 - Flags: review?(jonas)
Attachment #792855 - Flags: review?(jonas)
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.
Attachment #789907 - Attachment is obsolete: true
Attachment #789907 - Flags: review?(jonas)
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.
Attachment #792855 - Flags: review?(jonas) → review+
Jonas, can we land these patches the way they are to fix the notifications crash (bug 901214) and then keep working here?
Flags: needinfo?(jonas)
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 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
blocks a blocker - koi+
blocking-b2g: --- → koi+
Whiteboard: [systemfe]
Whiteboard: [systemfe] → [systemsfe]
(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…
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.
No longer blocks: 901214
blocking-b2g: koi+ → koi?
To whoever makes that decision, note that "no longer requires this implementation" means Notifications are now never leaving the child, bypassing the permission system.
(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?
(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 :)
Flags: needinfo?(jonas)
blocking-b2g: koi? → -
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;
}
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?
Attachment #815892 - Flags: review?(reuben.bmo)
Flags: needinfo?(reuben.bmo)
(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.
Flags: needinfo?(reuben.bmo)
Comment on attachment 815892 [details] [diff] [review]
patch 2 - renaming IsPrincipalValid to AssertAppPrincipal

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

LGTM
Attachment #815892 - Flags: review?(reuben.bmo) → review+
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.
Attachment #792548 - Flags: review?(jonas)
> 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.
There we go, sorry for the delays Andrea!
Attachment #792855 - Attachment is obsolete: true
Attachment #815892 - Attachment is obsolete: true
Attachment #817615 - Flags: review?(jonas)
Attachment #792548 - Attachment is obsolete: true
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 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.
Attachment #817615 - Flags: review?(jonas) → review-
(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.
Flags: needinfo?(jonas)
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.
Flags: needinfo?(jonas)
Blocks a blocker.
Blocks: 920302
blocking-b2g: - → koi?
blocking-b2g: koi? → koi+
- 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).
Attachment #817615 - Attachment is obsolete: true
Attachment #824818 - Flags: review?(jonas)
Blocks: 916091
Blocks: 923625
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();
Attachment #824818 - Flags: review?(jonas) → review-
Attachment #824818 - Attachment is obsolete: true
Attachment #826723 - Flags: review?(jonas)
Attached patch interdiff v5 v6Splinter Review
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.
Attachment #826723 - Flags: review?(jonas) → review+
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
Summary: Streamline permission checking/prompting for notifications/geolocation. → Implement a way to validate and properly handle permission requests coming from browser tabs
https://hg.mozilla.org/mozilla-central/rev/6a23d5d65b4d
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla28
Reuben - Does the landing of this patch automatically fix bug 920302?
Flags: needinfo?(reuben.bmo)
(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.
Flags: needinfo?(reuben.bmo)
Blocks: 935361
Blocks: 935494
Whiteboard: [systemsfe] → [systemsfe][qa-]
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.
The patch for bug 920302 had the wrong commit message and points to this bug.
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.