776649 - Add a C++ helper to get an application permission set from IPDL actors

Status: RESOLVED FIXED

(Core :: IPC, defect)

Description

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

(Normally I would want to say "security principal", but that may be overloaded too much in gecko.)

The master gecko process has access to many sensitive system interfaces, like modem, bluetooth, wifi, etc.  It can delegate some control of these interfaces to privileged content processes.  However, it must *only* delegate that control to content processes which have the requisite permissions.  Otherwise, bugs in gecko can allow compromised processes to pwn the entire phone.

This bug is the first step in that work: given a PFoo that's part of the PContent protocol tree, we want to be able to look up the set of capabilities given to that PFoo object.

I don't know how Mounir's permissions code works, but it should be relatively easy to hook this up.  Justin, the code we added to attach a manifest URL probably needs to change to attach the shiny new thing that Mounir added.

Comment 1

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

Note, we need to expose something similar to privileged JS, probably through the message manager, because some of our cross-process API impls are in JS.

Comment 2

[Justin Lebar (not reading bugmail)]

I think you may be behind the times.

We now pass in the app ID to the PBrowser constructor, so you can just stick that into the TabBrowser constructor.  IOW, this is so simple, you probably want to do this as part of whatever patch will use this data.

Comment 3

[Justin Lebar (not reading bugmail)]

> into the TabBrowser constructor.

er, into the TabParent constructor.

Comment 4

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

Excellent.  How do we get from app ID to capability set?

Comment 5

[Justin Lebar (not reading bugmail)]

(In reply to Chris Jones [:cjones] [:warhammer] from comment #4)
> Excellent.  How do we get from app ID to capability set?

I think you use the app ID + whether it's in a mozbrowser to generate the "extended origin", and then you ask the security manager.  But I think mounir wrote all this code, and I haven't looked at it, so he's a much better person to give details.

Comment 6

[Mounir Lamouri (:mounir)]

Using the permission manager: you can pass a principal and ask if it is allowed to do 'foo'. The permission manager has new methods that take a principal. For the moment, they are simply calling the URI methods. We simply have to change the storage and store the appId + browser info (or extendedOrigin) to make this happens. This hasn't been done yet but shouldn't be hard though.

Comment 8

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

Attachment: Approach for IPDL/C++, sans distracting goop

Because there's always goop!

In addition to feedback on general approach, which I think is fairly noncontroversial, I specifically don't quite know what to do where the "FIXME/XXX" comment is.

Do I need to manually call nsIPrincipal::GetPreferences() when I create the nsIPrincipal in TabParent, and store the annotation table in TabParent?  Or is there a better approach?

Comment 9

[Boris Zbarsky [:bzbarsky]]

Hrm.  Do we use the annotation table for anything?  I thought it was only used for enablePrivilege....

Comment 10

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

From my reading of nsBasePrincipal::IsCapabilityEnabled, it looks like the capability check is done against the annotation table, like so

    *result = (ht->Get(&key) == (void *) AnnotationEnabled);

But it's entirely possible that I'm looking at the wrong code.

Comment 11

[Boris Zbarsky [:bzbarsky]]

Well, yes, but that just means that it always tests false if enablePrivilege wasn't used.

And nsSystemPrincipal has its own impl, of course.

Comment 12

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

Mounir pointed me at the right place.  I was asking the wrong security manager.

Comment 13

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

messagemanager work will have to go to a followup.

Comment 14

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

Attachment: Add a HasCapabilities() API for checking whether apps loaded in PBrowserParent/PContentParent have the specific capability

This patch seems to work, but when I test it with HasCapability("sms"), I get UNKNOWN_ACTION so I guess that hasn't been hooked up.

Comment 15

[Justin Lebar (not reading bugmail)]

Comment on attachment 645207 [details] [diff] [review]
Add a HasCapabilities() API for checking whether apps loaded in PBrowserParent/PContentParent have the specific capability

>diff --git a/content/base/src/nsFrameLoader.cpp b/content/base/src/nsFrameLoader.cpp
>--- a/content/base/src/nsFrameLoader.cpp
>+++ b/content/base/src/nsFrameLoader.cpp
>@@ -1998,17 +1998,18 @@ nsFrameLoader::TryRemoteBrowser()
> 
>   // If our owner has no app manifest URL, then this is equivalent to
>   // ContentParent::GetNewOrUsed().
>   nsAutoString appManifest;
>   GetOwnerAppManifestURL(appManifest);
>   ContentParent* parent = ContentParent::GetForApp(appManifest);
> 
>   NS_ASSERTION(parent->IsAlive(), "Process parent should be alive; something is very wrong!");
>-  mRemoteBrowser = parent->CreateTab(chromeFlags, isBrowserElement, appId);
>+  mRemoteBrowser = parent->CreateTab(chromeFlags, isBrowserElement,
>+                                     appId, appManifest);

There is a bijection between app manifest URL and app ID, so you don't need
to add anything here.

I'm not actually sure how to go from appID to app manifest URL, but surely you
can (and if you can't, we should add that).  Mounir should know.

>diff --git a/dom/ipc/Capabilities.cpp b/dom/ipc/Capabilities.cpp
>new file mode 100644
>--- /dev/null
>+++ b/dom/ipc/Capabilities.cpp
>@@ -0,0 +1,55 @@
>+/* -*- Mode: C++; tab-width: 8; indent-tabs-mode: nil; c-basic-offset: 2 -*-
>+ * vim: sw=2 ts=8 et :
>+ */
>+/* This Source Code Form is subject to the terms of the Mozilla Public
>+ * License, v. 2.0. If a copy of the MPL was not distributed with this file,
>+ * You can obtain one at http://mozilla.org/MPL/2.0/. */
>+
>+#include "ContentParent.h"
>+#include "Capabilities.h"
>+#include "nsContentUtils.h"
>+#include "nsIPermissionManager.h"
>+#include "TabParent.h"
>+
>+using namespace mozilla::dom;
>+
>+namespace mozilla {
>+
>+bool
>+HasCapability(PBrowserParent* aActor, const char* aCapability)

This is such a general name, I'd very much prefer we didn't stick it in the
global mozilla namespace.  You can either create an IPCCapabilities class with
static methods, stick these functions in a namespace, or make their names less
general.

>+{
>+  TabParent* tab = static_cast<TabParent*>(aActor);
>+  nsIPrincipal* principal = tab->GetPrincipal();
>+  if (!principal) {
>+    return false;
>+  }

It's a stretch to say that the principal corresponding to the app's manifest
URL is the TabParent's "principal".  Indeed, any page loaded into the TabParent
will have a different principal (possibly but not necessarily with the same
origin as the manifest URL).

I'd prefer if we pushed the "GetPrincipal" logic down into this function and
had TabParent expose only its app ID.

>+  nsCOMPtr<nsIPermissionManager> permissionManager =
>+    do_GetService(NS_PERMISSIONMANAGER_CONTRACTID);
>+  if (!permissionManager) {
>+    return false;
>+  }
>+
>+  PRUint32 perm = nsIPermissionManager::DENY_ACTION;
>+  // FIXME/XXX where are we supposed to get the annotations table?
>+  // How do we initialize it with persistent capabilities from
>+  // manifest?
>+  return (NS_SUCCEEDED(permissionManager->TestExactPermissionFromPrincipal(
>+                         principal, aCapability, &perm)) &&
>+          perm == nsIPermissionManager::ALLOW_ACTION);

I don't know, sorry.

>diff --git a/dom/ipc/ContentParent.cpp b/dom/ipc/ContentParent.cpp
>--- a/dom/ipc/ContentParent.cpp
>+++ b/dom/ipc/ContentParent.cpp
>@@ -833,16 +838,21 @@ ContentParent::AllocPCompositor(ipc::Tra
> PBrowserParent*
> ContentParent::AllocPBrowser(const PRUint32& aChromeFlags,
>                              const bool& aIsBrowserElement,
>                              const PRUint32& aAppId)
> {
>+  // We only use this Alloc() method when the content processes asks
>+  // us to open a window.  In that case, we ignore the app-related
>+  // parameters of the constructor message, and have the new PBrowser
>+  // inherit the attributes of its opener in
>+  // TabParent::BrowserFrameOpenWindow().
>   TabParent* parent = new TabParent();
>   if (parent){
>     NS_ADDREF(parent);
>   }
>   return parent;
> }

Instead of silently ignoring the args, I'd prefer that we release-time asserted
that they're correct.  This "crash if we receive unexpected args" approach
matches what you suggested in another bug last night.

>diff --git a/dom/ipc/TabParent.cpp b/dom/ipc/TabParent.cpp
>--- a/dom/ipc/TabParent.cpp
>+++ b/dom/ipc/TabParent.cpp
>@@ -28,71 +28,117 @@
> #include "nsIDOMEvent.h"
> #include "nsIDOMEventTarget.h"
> #include "nsIDOMHTMLFrameElement.h"
> #include "nsIDOMWindow.h"
> #include "nsIDialogCreator.h"
> #include "nsIPromptFactory.h"
> #include "nsIURI.h"
> #include "nsIMozBrowserFrame.h"
>+#include "nsIScriptSecurityManager.h"
> #include "nsIViewManager.h"
> #include "nsIWidget.h"
> #include "nsIWindowWatcher.h"
> #include "nsNetUtil.h"
> #include "nsPIDOMWindow.h"
> #include "nsPrintfCString.h"
> #include "nsSerializationHelper.h"
> #include "nsServiceManagerUtils.h"
> #include "nsThreadUtils.h"
> #include "TabChild.h"
> #include "TabParent.h"
> 
> using namespace mozilla::dom;
> using namespace mozilla::ipc;
> using namespace mozilla::layers;
> using namespace mozilla::layout;
>+using namespace mozilla::services;
> using namespace mozilla::widget;
> using namespace mozilla::dom::indexedDB;
> 
> // The flags passed by the webProgress notifications are 16 bits shifted
> // from the ones registered by webProgressListeners.
> #define NOTIFY_FLAG_SHIFT 16
> 
> namespace mozilla {
> namespace dom {
> 
> TabParent *TabParent::mIMETabParent = nsnull;
> 
> NS_IMPL_ISUPPORTS3(TabParent, nsITabParent, nsIAuthPromptProvider, nsISecureBrowserUI)
> 
>+TabParent::TabParent(bool aIsBrowserElement, PRUint32 aAppId,
>+                     const nsAString& aManifestURL)
>+  : mFrameElement(NULL)
>+  , mIMESelectionAnchor(0)
>+  , mIMESelectionFocus(0)
>+  , mIMEComposing(false)
>+  , mIMECompositionEnding(false)
>+  , mIMECompositionStart(0)
>+  , mIMESeqno(0)
>+  , mManifestURL(aManifestURL)
>+  , mAppId(aAppId)
>+  , mDPI(0)
>+  , mActive(false)
>+  , mIsBrowserElement(aIsBrowserElement)
>+  , mShown(false)
>+{
>+}
>+
> TabParent::TabParent()
>   : mFrameElement(NULL)
>   , mIMESelectionAnchor(0)
>   , mIMESelectionFocus(0)
>   , mIMEComposing(false)
>   , mIMECompositionEnding(false)
>   , mIMECompositionStart(0)
>   , mIMESeqno(0)
>+  , mAppId(nsIScriptSecurityManager::UNKNOWN_APP_ID)
>   , mDPI(0)
>   , mActive(false)
>+  , mIsBrowserElement(false)
>   , mShown(false)
> {
> }
> 
> TabParent::~TabParent()
> {
> }
> 
> void
> TabParent::SetOwnerElement(nsIDOMElement* aElement)
> {
>   mFrameElement = aElement;
>   TryCacheDPI();
> }
> 
>+nsIPrincipal*
>+TabParent::GetPrincipal()
>+{
>+  if (mAppId != nsIScriptSecurityManager::UNKNOWN_APP_ID && !mPrincipal) {
>+    nsIScriptSecurityManager* secMan = nsContentUtils::GetSecurityManager();
>+    if (!secMan) {
>+      // We'll try again next time someone asks for our principal.
>+      return nsnull;
>+    }
>+    nsCOMPtr<nsIIOService> ioService = GetIOService();
>+    nsresult rv;
>+    nsCOMPtr<nsIURI> target;
>+    rv = NS_NewURI(getter_AddRefs(target), mManifestURL,
>+                   nsnull, nsnull, ioService);
>+    if (NS_FAILED(rv)) {
>+      return nsnull;
>+    }
>+    // If this fails, there's not really anything we can do ...
>+    secMan->GetAppCodebasePrincipal(target, mAppId, mIsBrowserElement,
>+                                    getter_AddRefs(mPrincipal));
>+  }
>+  return mPrincipal;
>+}

Per earlier, I'd prefer that this didn't live as a public method on TabParent,
because it's only meaningful for the purposes of HasCapability().

>@@ -1030,16 +1076,21 @@ TabParent::MaybeForwardEventToRenderFram
> 
> bool
> TabParent::RecvBrowserFrameOpenWindow(PBrowserParent* aOpener,
>                                       const nsString& aURL,
>                                       const nsString& aName,
>                                       const nsString& aFeatures,
>                                       bool* aOutWindowOpened)
> {
>+  TabParent* opener = static_cast<TabParent*>(aOpener);
>+  mManifestURL = opener->mManifestURL;
>+  mAppId = opener->mAppId;
>+  mIsBrowserElement = opener->mIsBrowserElement;

If we add the release-time assertion above, then we don't need this change,
right?  Certainly it's better to set this data during the constructor, if we
can.

Also, I don't think mIsBrowserElement should be set to
opener->mIsBrowserElement; an app should be able to create a new window that is
or isn't a browser element, but a browser element should only be able to create
another browser element.

Sorry I don't have global knowledge for this review; I need to learn this stuff
too.

Comment 16

[Justin Lebar (not reading bugmail)]

Comment on attachment 645207 [details] [diff] [review]
Add a HasCapabilities() API for checking whether apps loaded in PBrowserParent/PContentParent have the specific capability

er, that was meant to be an r-; I'd like to have another look.

Comment 17

[Mounir Lamouri (:mounir)]

Comment on attachment 645207 [details] [diff] [review]
Add a HasCapabilities() API for checking whether apps loaded in PBrowserParent/PContentParent have the specific capability

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

I'm not a big fan of using "Capabilities" for "process permissions". If you don't want to use "Permissions", could you at least put a comment in the header explaining what that is.

::: dom/ipc/Capabilities.cpp
@@ +32,5 @@
> +
> +  PRUint32 perm = nsIPermissionManager::DENY_ACTION;
> +  // FIXME/XXX where are we supposed to get the annotations table?
> +  // How do we initialize it with persistent capabilities from
> +  // manifest?

What is that comment about? The things the principal gets from the prefs?

::: dom/ipc/ContentParent.h
@@ +73,5 @@
>       * <iframe mozbrowser>.
>       * |aAppId| indicates which app the tab belongs to.
>       */
> +    TabParent* CreateTab(PRUint32 aChromeFlags, bool aIsBrowserElement,
> +                         PRUint32 aAppId, const nsAString& aManifestURL);

As Justin pointed, you can add a method to the AppsService that will return you what you want: the app object or the app origin. No need to pass the manifest URL here.

::: dom/ipc/TabParent.cpp
@@ +121,5 @@
> +    }
> +    nsCOMPtr<nsIIOService> ioService = GetIOService();
> +    nsresult rv;
> +    nsCOMPtr<nsIURI> target;
> +    rv = NS_NewURI(getter_AddRefs(target), mManifestURL,

Please use the app's origin, not the manifest.

@@ +128,5 @@
> +      return nsnull;
> +    }
> +    // If this fails, there's not really anything we can do ...
> +    secMan->GetAppCodebasePrincipal(target, mAppId, mIsBrowserElement,
> +                                    getter_AddRefs(mPrincipal));

Could you at least print a warning if this is failing?

Comment 18

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

> >diff --git a/dom/ipc/Capabilities.cpp b/dom/ipc/Capabilities.cpp
> >+bool
> >+HasCapability(PBrowserParent* aActor, const char* aCapability)
> 
> This is such a general name, I'd very much prefer we didn't stick it in the
> global mozilla namespace.  You can either create an IPCCapabilities class
> with
> static methods, stick these functions in a namespace, or make their names
> less
> general.
> 

Alright.  How does AppProcessHasCapabaility() suit?

I used the short name because we're matching on argument type.  But I
don't care that much.

> >+{
> >+  TabParent* tab = static_cast<TabParent*>(aActor);
> >+  nsIPrincipal* principal = tab->GetPrincipal();
> >+  if (!principal) {
> >+    return false;
> >+  }
> 
> It's a stretch to say that the principal corresponding to the app's manifest
> URL is the TabParent's "principal".  Indeed, any page loaded into the
> TabParent
> will have a different principal (possibly but not necessarily with the same
> origin as the manifest URL).
> 

I agree we need a better name for this.  However, we're returning an
nsIPrincipal we construct from app information, so I don't have any
ideas for better names here that wouldn't increase confusion.

> I'd prefer if we pushed the "GetPrincipal" logic down into this function and
> had TabParent expose only its app ID.
> 

I don't know how I would cache the constructed principal with that
approach.  I assume that constructing the nsIPrincipal is relatively
expensive.  Maybe that's an invalid assumption.

> >+  // FIXME/XXX where are we supposed to get the annotations table?
> >+  // How do we initialize it with persistent capabilities from
> >+  // manifest?
> 
> I don't know, sorry.

This was a stale comment, my fault.

> 
> >diff --git a/dom/ipc/ContentParent.cpp b/dom/ipc/ContentParent.cpp

> >+  // We only use this Alloc() method when the content processes asks
> >+  // us to open a window.  In that case, we ignore the app-related
> >+  // parameters of the constructor message, and have the new PBrowser
> >+  // inherit the attributes of its opener in
> >+  // TabParent::BrowserFrameOpenWindow().
> >   TabParent* parent = new TabParent();
>
> Instead of silently ignoring the args, I'd prefer that we release-time
> asserted
> that they're correct.

Does the content process even send back the same app information for
popup windows?  Can we rely on that?

> >diff --git a/dom/ipc/TabParent.cpp b/dom/ipc/TabParent.cpp

> Also, I don't think mIsBrowserElement should be set to
> opener->mIsBrowserElement; an app should be able to create a new window that
> is
> or isn't a browser element, but a browser element should only be able to
> create
> another browser element.
> 

That makes sense.  However, I don't understand how mIsBrowserElement
affects the nsIPrincipal we construct.  Maybe mounir can comment.

Comment 19

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

(In reply to Mounir Lamouri (:mounir) from comment #17)
>
> I'm not a big fan of using "Capabilities" for "process permissions". If you
> don't want to use "Permissions", could you at least put a comment in the
> header explaining what that is.

Windows calls the analogue of this thing the "security descriptor".
POSIX calls something similar "process capabilities".  There's not
really a standard name here; "process permissions" certainly isn't a
standard name.

> 
> ::: dom/ipc/Capabilities.cpp
> @@ +32,5 @@
> > +
> > +  PRUint32 perm = nsIPermissionManager::DENY_ACTION;
> > +  // FIXME/XXX where are we supposed to get the annotations table?
> > +  // How do we initialize it with persistent capabilities from
> > +  // manifest?
> 
> What is that comment about? The things the principal gets from the prefs?
> 

Ignore that --- vestige of me looking at the wrong security manager.

> ::: dom/ipc/ContentParent.h
> @@ +73,5 @@
> >       * <iframe mozbrowser>.
> >       * |aAppId| indicates which app the tab belongs to.
> >       */
> > +    TabParent* CreateTab(PRUint32 aChromeFlags, bool aIsBrowserElement,
> > +                         PRUint32 aAppId, const nsAString& aManifestURL);
> 
> As Justin pointed, you can add a method to the AppsService that will return
> you what you want: the app object or the app origin. No need to pass the
> manifest URL here.
> 

I'll see if I can cargo-cult my way through here ... if you guys can
provide some pointers, this might go a bit more quickly.

> @@ +128,5 @@
> > +      return nsnull;
> > +    }
> > +    // If this fails, there's not really anything we can do ...
> > +    secMan->GetAppCodebasePrincipal(target, mAppId, mIsBrowserElement,
> > +                                    getter_AddRefs(mPrincipal));
> 
> Could you at least print a warning if this is failing?

Sure.

Comment 20

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

Attachment: part 1: Add getAppById() to do just that

Passes tests! ;)

Comment 21

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

Attachment: part 2: Add an API for checking whether apps loaded in PBrowserParent/PContentParent have the specific capability, v2

I can't tell if this is working on device because the OOP sms app seems to have grown a new crash before we hit the capability check :|.  Will investigate more tomorrow.

Comment 22

[Justin Lebar (not reading bugmail)]

> I assume that constructing the nsIPrincipal is relatively
> expensive.  Maybe that's an invalid assumption.

Premature optimization is the root of all evil.  :)

I seriously doubt that we'll be making so many capability checks that, even if constructing a principal were very expensive, we'd notice.

Comment 23

[Justin Lebar (not reading bugmail)]

> That makes sense.  However, I don't understand how mIsBrowserElement
> affects the nsIPrincipal we construct.  Maybe mounir can comment.

The set of cookies (and indexeddb, localstorage, etc) that a page can see is determined by the tuple (page origin, app id, is-browser-frame).

Comment 24

[Mounir Lamouri (:mounir)]

Comment on attachment 645684 [details] [diff] [review]
part 1: Add getAppById() to do just that

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

There is a confusion between localId and id here. It's sad but we need both.

::: dom/apps/src/AppsService.js
@@ +28,5 @@
> +  },
> +
> +  getAppById: function getAppById(aId) {
> +    debug("GetAppById( " + aId + " )");
> +    return DOMApplicationRegistry.getAppById(aId);

This is unlikely going to do what you want. You should use localId.

::: dom/apps/src/Webapps.jsm
@@ +55,5 @@
>      this.messages = ["Webapps:Install", "Webapps:Uninstall",
> +                     "Webapps:GetSelf",
> +                     "Webapps:GetInstalled", "Webapps:GetNotInstalled",
> +                     "Webapps:Launch", "Webapps:GetAll",
> +                     "Webapps:InstallPackage", "Webapps:GetBasePath"];

Are you changing anything here?

::: dom/apps/tests/test_apps_service.xul
@@ +39,5 @@
> +
> +  SimpleTest.is(appsService.getAppById(Ci.nsIScriptSecurityManager.NO_APP_ID),
> +                null,
> +                "getAppById() should return null for NO_APP_ID");
> +

Could you call the method for app 0 and see it's non-null. We have pre-installed app in the test profile.
You could also test nsIScriptSecurityManager.UNKNOWN_APP_ID and check that it's null.

::: dom/interfaces/apps/nsIAppsService.idl
@@ +18,5 @@
>  [scriptable, uuid(1210a0f3-add3-4381-b892-9c102e3afc42)]
>  interface nsIAppsService : nsISupports
>  {
>    mozIDOMApplication getAppByManifestURL(in DOMString manifestURL);
> +  mozIDOMApplication getAppById(in long id);

Rename this "GetAppByLocalId()". Unfortunately, "id" is an uuid used for app synchronization (services).

Comment 25

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

(In reply to Justin Lebar [:jlebar] (jury duty 7/25) from comment #22)
> > I assume that constructing the nsIPrincipal is relatively
> > expensive.  Maybe that's an invalid assumption.
> 
> Premature optimization is the root of all evil.  :)
> 

That's not quite the whole picture.  The way to write fast code is to not write slow code.  Deliberately writing slow code just to not "prematurely optimize" is wrong, for example you would r- if I created the principal in a for(1000000) loop.  (I hope!)

Note, I draw a distinction here between designing code not to run slowly, and micro-optimizing bits/bytes/isns.

> I seriously doubt that we'll be making so many capability checks that, even
> if constructing a principal were very expensive, we'd notice.

This code needs to be fast enough to run with each received IPC message.  If I know ahead of time that constructing the principal is too expensive for that, then why bother landing code I'll need to change right afterwards in a followup.

Comment 26

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

(In reply to Mounir Lamouri (:mounir) (on VACATION until August 5th) from comment #24)
> Comment on attachment 645684 [details] [diff] [review]
> part 1: Add getAppById() to do just that
> 
> Review of attachment 645684 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> There is a confusion between localId and id here. It's sad but we need both.
> 

I have no idea what these are, obviously.

> ::: dom/apps/src/Webapps.jsm
> @@ +55,5 @@
> >      this.messages = ["Webapps:Install", "Webapps:Uninstall",
> > +                     "Webapps:GetSelf",
> > +                     "Webapps:GetInstalled", "Webapps:GetNotInstalled",
> > +                     "Webapps:Launch", "Webapps:GetAll",
> > +                     "Webapps:InstallPackage", "Webapps:GetBasePath"];
> 
> Are you changing anything here?
> 

Fixing indentation.

Comment 27

[Justin Lebar (not reading bugmail)]

I feel like this is a mischaracterization of the "premature optimization is the root of all evil" argument.

There exist ways to write code other than "optimize based on guesswork and assumptions" and "write obviously wasteful code"; the fallacy in comment 25 is a straw man.

> If I know ahead of time that constructing the principal is too expensive [...], then why 
> bother landing code I'll need to change right afterwards in a followup.

Indeed, but you said you don't know that: "I assume that constructing the nsIPrincipal is relatively expensive.  Maybe that's an invalid assumption."

The hyperbole in the aphorism is there to serve as a counterpoint, in one's mind, when faced with exactly this kind of temptation.

Comment 28

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

We shouldn't waste time on continuing to argue about the ways we agree with other, but I didn't know whether constructing nsIPrincipal was relatively expensive (~DB access), you said "not to my knowledge", and that's good enough for me.  But then you additionally brought out the "premature optimization" admonition which is a different issue.

I'm happy to continue the hermeneutical discussion, but not here! ;)

Comment 29

[Mounir Lamouri (:mounir)]

Comment on attachment 645685 [details] [diff] [review]
part 2: Add an API for checking whether apps loaded in PBrowserParent/PContentParent have the specific capability, v2

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

I don't understand why you want to keep "Capabilities" in the name and not "Permissions". What about "AppProcessPermissions"?

Instead of this pattern:
if (foo) {
  return false;
}
Could you use:
NS_ENSURE_{TRUE,SUCCESS}(foo, false);
so we get a warning. Most of the case, those tests are sanity checks, we should even put a NS_ERROR() there.

The code looks generally good (can't really judge the IPC parts though) but something bothers me: an iframe from another origin in an app will not be able to use any permission that the app isn't actually using? will it get automatically permissions that the app is using? That's not really clear to me.

::: dom/ipc/AppProcessCapabilities.cpp
@@ +47,5 @@
> +
> +  nsCOMPtr<nsIIOService> ioService = GetIOService();
> +  if (!ioService) {
> +    return false;
> +  }

No need to do that, NS_NewURI will get the ioService if you don't pass it.

::: dom/ipc/PContent.ipdl
@@ +98,5 @@
>  };
> +
> +struct AppData {
> +  bool isBrowserElement;
> +  uint32_t appId;

nit: in most code we put appId first.

::: dom/ipc/TabParent.cpp
@@ +32,5 @@
>  #include "nsIDialogCreator.h"
>  #include "nsIPromptFactory.h"
>  #include "nsIURI.h"
>  #include "nsIMozBrowserFrame.h"
> +#include "nsIScriptSecurityManager.h"

Not sure why you need that.

@@ +49,5 @@
>  using namespace mozilla::dom;
>  using namespace mozilla::ipc;
>  using namespace mozilla::layers;
>  using namespace mozilla::layout;
> +using namespace mozilla::services;

or that

::: dom/ipc/TabParent.h
@@ +47,5 @@
>                  , public nsIAuthPromptProvider
>                  , public nsISecureBrowserUI
>  {
>  public:
> +    TabParent(bool aIsBrowserElement, PRUint32 aAppId);

nit: in most code, we put appId first.

Comment 30

[Mounir Lamouri (:mounir)]

BTW, FWIW, creating principals shouldn't be expensive. It's basically, a dtor, two methods being called and one hash table lookup. Very likely minimal footprint compared to the entire IPC communication.

Comment 31

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 645685 [details] [diff] [review]
part 2: Add an API for checking whether apps loaded in PBrowserParent/PContentParent have the specific capability, v2

The API looks fine, but I think that instead of the hoops you had to jump through we should have a

  nsIPrincipal getPrincipal(in boolean inBrowserElement);

or whatnot on mozIDOMApplication.  The hoops can be in there.  ;)

Alternately, we could have a way to test permissions off the app directly...

Comment 32

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

(In reply to Mounir Lamouri (:mounir) (on VACATION until August 5th) from comment #29)
> Comment on attachment 645685 [details] [diff] [review]
> part 2: Add an API for checking whether apps loaded in
> PBrowserParent/PContentParent have the specific capability, v2
> 
> Review of attachment 645685 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> I don't understand why you want to keep "Capabilities" in the name and not
> "Permissions". What about "AppProcessPermissions"?
> 

That's just inventing a new name out of thin air.  Similarly, I don't quite understand why you feel strongly about "process permissions".  At least "process capabilities" is reminiscent of the POSIX analogue.

But I don't feel strongly about it.

> Instead of this pattern:
> if (foo) {
>   return false;
> }
> Could you use:
> NS_ENSURE_{TRUE,SUCCESS}(foo, false);
> so we get a warning. Most of the case, those tests are sanity checks, we
> should even put a NS_ERROR() there.
> 

I'll gladly do

 if (!foo) {
   NS_WARNING("No foo");
   return false;
 }

Comment 33

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

(In reply to Boris Zbarsky (:bz) from comment #31)
> Comment on attachment 645685 [details] [diff] [review]
> part 2: Add an API for checking whether apps loaded in
> PBrowserParent/PContentParent have the specific capability, v2
> 
> The API looks fine, but I think that instead of the hoops you had to jump
> through we should have a
> 
>   nsIPrincipal getPrincipal(in boolean inBrowserElement);
> 
> or whatnot on mozIDOMApplication.  The hoops can be in there.  ;)
> 
> Alternately, we could have a way to test permissions off the app directly...

Hmmmmm interesting suggestion!  I like the idea of |mozIDOMApplication* TabParent::GetApp()|, and just testing against that.

Justin/Mounir, sound good to you too?  Then we can just hand the mozIDOMApplication* to ContentParent::CreateTab().

Comment 34

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

jlebar, what I would want to do is change the code here

  // If our owner has no app manifest URL, then this is equivalent to
  // ContentParent::GetNewOrUsed().
  nsAutoString appManifest;
  GetOwnerAppManifestURL(appManifest);
  ContentParent* parent = ContentParent::GetForApp(appManifest);

  NS_ASSERTION(parent->IsAlive(), "Process parent should be alive; something is very wrong!");
  mRemoteBrowser = parent->CreateTab(chromeFlags, isBrowserElement, appId);

to instead look up the mozIDOMApplication and pass that around.  I suspect this code is vestigial, pre-apps, but do you know of any reason not to convert this to mozIDOMApplication?

Comment 35

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

This goes back to the discussion we had in the ContentParent::GetForApp() bug, in which you wanted to wait on something mounir was hacking on.  I'm asking if that thing was this.

Comment 36

[Justin Lebar (not reading bugmail)]

> I like the idea of |mozIDOMApplication* TabParent::GetApp()|

Me too.

> I suspect this code is vestigial, pre-apps, but do you know of any reason not to convert this to 
> mozIDOMApplication?

You'd still have to pass around isBrowserElement.  So we're passing around the app instead of the appID, is the only change.  Passing the app ID here is certainly more convenient if you have to send it across process boundaries.

Is the apps service even available from a child process?  I thought it wasn't (see bug 777135), but I can't tell from the code.  That would throw a wrench in this whole larger plan.

Comment 37

[Justin Lebar (not reading bugmail)]

In particular, I don't think you have to pass the app to ContentParent::CreateTab() in order to do what bz suggested.  But I don't care whether you pass the app or the ID to ContentParent::CreateTab.  Six of one.

Comment 38

[Justin Lebar (not reading bugmail)]

(In reply to Chris Jones [:cjones] [:warhammer] from comment #35)
> This goes back to the discussion we had in the ContentParent::GetForApp()
> bug, in which you wanted to wait on something mounir was hacking on.  I'm
> asking if that thing was this.

You're referring to bug 762802 comment 11?  It's applicable here, but I'm not sure exactly how it applies, so I think we can safely ignore it.  :)

Comment 39

[Mounir Lamouri (:mounir)]

(In reply to Justin Lebar [:jlebar] (PTO July 26) from comment #36)
> Is the apps service even available from a child process?  I thought it
> wasn't (see bug 777135), but I can't tell from the code.  That would throw a
> wrench in this whole larger plan.

It is not.

Comment 40

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

Attachment: part 1: Refactor content-process/browser creation to use mozIDOMApplication for passing app info

Comment 41

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

Attachment: part 2: Add mozIDOMApplication.hasPermission

I really don't like duplicating this code, but I don't really understand what I'm doing here or how things are supposed to work.

Comment 42

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

Attachment: part 3: Add an API for checking whether apps loaded in PBrowserParent/PContentParent have the specific capability

Hopefully this one feels a little better ...

Comment 43

[Justin Lebar (not reading bugmail)]

> +      let principal = secMan.getAppCodebasePrincipal(uri, appId,
> +                                                     /*mozbrowser*/true);

As we discussed on IRC last night, I don't think this is correct.

The browser app has a different set of permissions than a browser tab.  If you set mozbrowser here, aren't you always getting the permissions for the browser tab?

I guess this is a reason why putting "HasPermission()" on the app doesn't make sense -- you need more than just the app to know whether you have permission to do X: You need the app and the boolean indicating whether the given frame is inside mozbrowser.

Comment 44

[Justin Lebar (not reading bugmail)]

> -      // If the frame is actually an app, we should not mark it as a browser.
> -      if (appId != nsIScriptSecurityManager::NO_APP_ID) {
> -        isBrowserElement = false;
> -      }

If you remove this, then we'll mark every app as a browser.  That means that there will be no difference between the browser app and a browser tab.

As I said earlier, I don't understand the advantage of passing around an app instead of an app id, which is what we do everywhere else.  (For example, nsIDocShell has a GetAppID method, not a GetApp method.)  What are you trying to accomplish with that change?  Do you just feel it's cleaner?  Or is it somehow necessary to do your app::HasPermission() code (which, we established, can't actually work, because we need to know the is-browser bit in order to determine whether a process has permission to do X)?

Comment 45

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

Siiiigh.

(In reply to Justin Lebar [:jlebar] (PTO July 26) from comment #43)
> > +      let principal = secMan.getAppCodebasePrincipal(uri, appId,
> > +                                                     /*mozbrowser*/true);
> 
> As we discussed on IRC last night, I don't think this is correct.
> 

Our discussion did not resolve my confusion, so I'm not surprised.

> The browser app has a different set of permissions than a browser tab.  If
> you set mozbrowser here, aren't you always getting the permissions for the
> browser tab?
> 

We need to document this.  I thought you had agreed that mozbrowser xor mozapp.  At some point in the past, we had to write <iframe mozbrowser mozapp="manifest"> but *only* because "mozbrowser" also happened to mean "enable new shiny things".  But otherwise it was meaningless for mozapp.

If we were standardizing, in an ideal world, there would be <app> and <browser>.  The constructions <app browser> or <browser app> make no sense.  Right?

When you say, "we need to also know mozbrowser for mozapp frames", it sounds like you see meaning in either <app browser> or <browser app> (or both?).  Where's our misunderstanding?

> I guess this is a reason why putting "HasPermission()" on the app doesn't
> make sense -- you need more than just the app to know whether you have
> permission to do X: You need the app and the boolean indicating whether the
> given frame is inside mozbrowser.

I really hope this doesn't need to be the case.

Maybe this will get through my thick head eventually.

(In reply to Justin Lebar [:jlebar] (PTO July 26) from comment #44)
> As I said earlier, I don't understand the advantage of passing around an app
> instead of an app id, which is what we do everywhere else.

It's a strong type, actually represents the app object with getters for object attributes (don't have to indirect through a service), and knows how to answer questions like HasPermissions() etc.?

> (For example,
> nsIDocShell has a GetAppID method, not a GetApp method.)

And nsGlobalWindow has GetApp(), but no GetAppId().  Which one is right.

If there's a master plan somewhere, please lead me to it! :)  I don't like trying to reverse engineer these things ;).

Comment 46

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

To digest a bit more the things I don't understand

 - what are the semantics of <app browser> or <browser app>?  Or is the question incorrectly posed?  If so, why?

 - when should clients prefer one or the other of App* strong type vs. integer local app ID?

Comment 47

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

Oy, how was Jonas not CC'd ...

Comment 48

[Justin Lebar (not reading bugmail)]

>  I thought you had agreed that mozbrowser xor mozapp.

And I thought we'd agreed not, so that was a wholly unproductive a productive conversation.  :)

I think you're still confused about what these parameters mean in this context.

The app-id and is-browser boolean are two of the three values used in the (page origin, app-id, is-browser) tuple used in the new "extended origin".  This extended origin is used for two purposes:

 1) To determine whether a page has permission to do X (that is, the extended origin is used as a security principal), and

 2) To separate out the storage of different pages.  Two pages share the same cookies, localstorage, etc. iff they have the same extended origin.

If you keep in mind how this extended origin is used, you may have less difficulty understanding how the values are set.

In particular, if is-app implied not is-browser and is-browser implied not is-app, then consider how that would affect (2) above: It would cause all browser tabs across all apps to share the same storage, because all browser tabs would use the extended origin tuple (page origin, no-app, true).

So therefore we cannot have the constraint is-app xor is-browser.

What we want is for all the <iframe mozbrowser>s (that is, "browser tabs") inside an app to share the same storage, and for any <iframe mozbrowser>s in a different app to have different storage.

Therefore we say, for our purposes here, that an <iframe mozbrowser> inside an <iframe mozapp> gets its parent's app id.

It may help to think about the extended origin tuple (page-origin, app-id, is-browser) as a first-order approximation to the token constructed as follows:

  Let f be the frame whose extended origin we wish to calculate.  The extended origin will be a tuple with two elements (o, l).  Let o be f's origin.  l is a list computed as follows:

   i)   Let l be the empty list.
   ii)  Let g = f.
   iii) Do until g is null or has no <iframe mozbrowser> or <iframe mozapp> in its parent chain:
        1) If g is not an <iframe mozbrowser> or <iframe mozapp>, walk up g's parent chain until we encounter <iframe mozbrowser> or <iframe mozapp> and call that frame element g.
        2) If g is an <iframe mozbrowser>, append (true, NULL) to l.  If g is an <iframe mozapp>, append (false, g's app-id) to l.
        3) Let g be be g's parent frame.

The tuple constructed as above uniquely identifies a mozbrowser or mozapp nested arbitrarily inside other browsers/apps.  But since we don't allow arbitrary nesting, we can get away with our three-tuple.

Thinking about it this way also shows why I was uncomfortable with TabParent having a GetPrincipal() method.  The principal returned would always have the correct value for |l|, but the value for |o| would be the app's origin, not the origin of the requesting page.  It would be the correct thing to use only for backstop security checks in the parent; it would be wrong to use for non-backstop security checks (where the child hasn't already performed its own check), and it would be wrong to use as a storage token.

>> I guess this is a reason why putting "HasPermission()" on the app doesn't
>> make sense -- you need more than just the app to know whether you have
>> permission to do X: You need the app and the boolean indicating whether the
>> given frame is inside mozbrowser.
>
> I really hope this doesn't need to be the case.

The difficulty you're encountering here is that we need to know which app an <iframe mozbrowser> is contained inside for the purposes of (2), deciding how to partition the frame's storage.

But you're right, we don't need to know which app the mozbrowser is inside for the purposes of (1), the permissions check, because all browser tabs are equally unprivileged. (*)  For the purposes of permissions only, you could declare that <iframe mozbrowser> inside <iframe mozapp> has no associated app.

(*) This isn't entirely right if we allow saved permissions -- If I load facebook.com in a browser tab and allow it to access my geolocation with no prompt, that permission grant should not apply to facebook.com in a browser tab in a different app.

> nsIDocShell has a GetAppID method, not a GetApp method.)
>
> And nsGlobalWindow has GetApp(), but no GetAppId().  Which one is right.

The nsGlobalWindow methods are going away as soon as I finish my patch to remove them.

>> As I said earlier, I don't understand the advantage of passing around an app
>> instead of an app id, which is what we do everywhere else.
>
> It's a strong type, actually represents the app object with getters for object attributes (don't have to 
> indirect through a service), and knows how to answer questions like HasPermissions() etc.?

The question is why should you pass an app to the TabParent constructor; obviously the TabParent could convert the app ID to an app and use that to access HasPermissions() and whatnot.  But I buy strong typing as an argument for passing the app there.

Comment 49

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

Thanks for the detailed reply ... will reread more carefully to see if you answered my question, but in the case of (2) above, isn't the app ID sufficient to identify the data storage area?  Why the additional bit?

Comment 50

[Justin Lebar (not reading bugmail)]

(In reply to Chris Jones [:cjones] [:warhammer] from comment #49)
> Thanks for the detailed reply ... will reread more carefully to see if you
> answered my question, but in the case of (2) above, isn't the app ID
> sufficient to identify the data storage area?  Why the additional bit?

If we didn't have the is-browser bit, then if the browser app loads facebook inside a vanilla <iframe>, that would share storage with facebook inside <iframe mozbrowser>.  The idea is that <iframe mozbrowser/mozapp> should be a storage barrier (as expressed in the algorithm from comment 48).

Comment 51

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

Is the key here that
 * each <app> gets its own storage
 * within each <app>, all <browser>s get a storage area.  It's separate from the enclosing <app>.

So if I install
 - camera app
 - firefox app

and I run the camera, run the browser and load some "tabs", there will be the following data storage areas
 0, window-manager/system-app data
 1. camera data
 2. firefox data
    3. firefox <browser> tab data (not really hierarchical, just writing this way for clarity)

If so, then having a separate storage area for <app> and then its <browser>s-within-<app> doesn't really make sense to me ... why do that?

Comment 52

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

Sorry, mid-aired.

Your comment 50 doesn't answer my questions though :).

Comment 53

[Justin Lebar (not reading bugmail)]

>  * each <app> gets its own storage
>  * within each <app>, all <browser>s get a storage area.  It's separate from the enclosing <app>.

This is correct.

> So if I install
>  - camera app
>  - firefox app
>
> and I run the camera, run the browser and load some "tabs", there will be the following data 
> storage areas
>
>  0, window-manager/system-app data
>  1. camera data
>  2. firefox data
>  3. firefox <browser> tab data
>
> If so, then having a separate storage area for <app> and then its <browser>s-within-<app> doesn't 
> really make sense to me ... why do that?

Can I turn the question around and ask "why not"?  Why would you want these to share storage?

The browser app is a highly-privileged space, whereas browser tabs are not.  Isn't it a good thing that they're segregated?

At a practical level, we want to be able to "clear browser private data", as a browser app function, which effectively means, nuke the <browser>-within-<app> storage.

Comment 54

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

(In reply to Justin Lebar [:jlebar] (PTO July 26) from comment #53)
> > If so, then having a separate storage area for <app> and then its <browser>s-within-<app> doesn't 
> > really make sense to me ... why do that?
> 
> Can I turn the question around and ask "why not"?  Why would you want these
> to share storage?
> 

To remove an extra bit that confused the hell out of me ;).

> At a practical level, we want to be able to "clear browser private data", as
> a browser app function, which effectively means, nuke the
> <browser>-within-<app> storage.

That is a winning argument!

OK, thanks for being patient in explaining this. :)

Comment 55

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

Attachment: part 1: Refactor content-process/browser creation to use mozIDOMApplication for passing app info, v2

Comment 56

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 646046 [details] [diff] [review]
part 3: Add an API for checking whether apps loaded in PBrowserParent/PContentParent have the specific capability

This looks much nicer, but don't we still need to do the whole in/out of browser tab thing?

Comment 57

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

You are correct sir.  However, I see that I currently have

  hasPermission():
      //...
      let principal = secMan.getAppCodebasePrincipal(uri, appId,
                                                     /*mozbrowser*/true);

which was successfully granting SMS permissions to the SMS app.  That's fine, what we expect .... I think.

But in the case of <browser>s within the browser app, the principal here will be for the browser app's appId, and mozbrowser will always be true.  So that will give remote content the same capabilities as the browser app itself, which runs in-process.  That's an avoidable privilege escalation.

I don't really see a way around that as long as
 - we're using the same "app codebase principal" for two very different purposes
 - can't trust any URIs handed to us by the content process
 - ... using the app's origin for the URI here?

Maybe the last one is a bug, maybe we should use a "null URI" for <browser>.  I'll try tomorrow and see if it does what I want.

On the other hand, in this case the browser app only has system XHR and mozbrowser privileges, which don't expose more data or more capabilities than what <browser> content processes will already need to have.  So maybe I'm overthinking.

Comment 58

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

Also, if the null URI approach "works", it should deny all permission requests, I think.  So maybe we should just not bother calling hasPermission() at all for <browser> content.

Comment 59

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

Comment on attachment 646299 [details] [diff] [review]
part 1: Refactor content-process/browser creation to use mozIDOMApplication for passing app info, v2

I think there are some bugs here.  Need to think about it with a fresher mind.

Comment 60

[Justin Lebar (not reading bugmail)]

> let principal = secMan.getAppCodebasePrincipal(uri, appId,
>                                                /*mozbrowser*/true);
>
> which was successfully granting SMS permissions to the SMS app.  That's fine, what we expect .... I 
> think.

That is certainly not what I expect!  Why should a browser tab inside the SMS app have access to SMS?  (Or, put another why, why should a browser tab inside the browser app have permission to create browser tabs?)

Comment 61

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

Per IRC discussion, will define away this problem for this code, by not calling hasPermission() with the app's origin but isBrowser = true.

Comment 62

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

Attachment: part 1: Refactor content-process/browser creation to use mozIDOMApplication for passing app info, v3

Comment 63

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

Attachment: part 2: Add mozIDOMApplication.hasPermission, v2

Comment 64

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

Attachment: part 3: Add an API for checking whether apps loaded in PBrowserParent/PContentParent have the specific capability, v2

Carrying over sr=bz.  Addresses isBrowser comment.

Comment 65

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

Attachment: part 3: Add an API for checking whether apps loaded in PBrowserParent/PContentParent have the specific capability, v2

Let's try the right patch this time ...

Comment 66

[[:fabrice] Fabrice Desré]

Comment on attachment 646701 [details] [diff] [review]
part 2: Add mozIDOMApplication.hasPermission, v2

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

I would also like Jonas to r+ the IDL changes

::: dom/apps/src/Webapps.js
@@ +301,5 @@
> +  hasPermission: function(permission) {
> +      let appsService = Cc['@mozilla.org/AppsService;1']
> +                        .getService(Ci.nsIAppsService);
> +      let appId = appsService.getAppLocalIdByManifestURL(this.manifestURL);
> +      var uri = Services.io.newURI(this.origin, null, null); 

Are we testing perms against the origin, and not the manifestURL? How will that work with several apps per origin?

::: dom/apps/src/Webapps.jsm
@@ +287,5 @@
> +                                                       /*mozbrowser*/false);
> +        let perm = Services.perms.testExactPermissionFromPrincipal(principal,
> +                                                                   permission);
> +        return (perm === Ci.nsIPermissionManager.ALLOW_ACTION);
> +      }

We don't need that here. This is just a helper to filter out what we serialize to disk.

::: dom/interfaces/apps/nsIDOMApplicationRegistry.idl
@@ +40,5 @@
>    /* startPoint will be used when several launch_path exists for an app */
>    nsIDOMDOMRequest launch([optional] in DOMString startPoint);
>    nsIDOMDOMRequest uninstall();
> +  /* Return true if this app has |permission|. */
> +  boolean hasPermission(in string permission);

Nit: DOMString

Comment 67

[Justin Lebar (not reading bugmail)]

Comment on attachment 646700 [details] [diff] [review]
part 1: Refactor content-process/browser creation to use mozIDOMApplication for passing app info, v3

>diff --git a/dom/ipc/ContentChild.cpp b/dom/ipc/ContentChild.cpp
>--- a/dom/ipc/ContentChild.cpp
>+++ b/dom/ipc/ContentChild.cpp
>@@ -393,20 +393,22 @@ PCompositorChild*
> ContentChild::AllocPCompositor(ipc::Transport* aTransport,
>                                base::ProcessId aOtherProcess)
> {
>     return CompositorChild::Create(aTransport, aOtherProcess);
> }
> 
> PBrowserChild*
> ContentChild::AllocPBrowser(const PRUint32& aChromeFlags,
>-                            const bool& aIsBrowserElement,
>-                            const PRUint32& aAppId)
>+                            const AppId& aApp,
>+                            const bool& aIsBrowserElement)
> {
>-    nsRefPtr<TabChild> iframe = new TabChild(aChromeFlags, aIsBrowserElement, aAppId);
>+    PRUint32 appId = aApp.get_TrustedAppId().id();

I'm not convinced that this Trusted/Untrusted ID business adds anything.

In general, the child should trust data it receives from the parent, and the parent should not trust data it receives from the child.  That should be true everywhere.

Therefore the types are misleading: If ContentParent::AllocPBrowser receives a TrustedAppId, it must not trust it!  At that point, it's not really accurate to say that the datatype is "trusted".

Does IPDL let us declare different Alloc signatures for child and parent?  If so, could we have the alloc-in-child take an ID, and the alloc-in-parent take a TabParent?

r- until we can figure this out.

>diff --git a/dom/ipc/ContentParent.cpp b/dom/ipc/ContentParent.cpp
>--- a/dom/ipc/ContentParent.cpp
>+++ b/dom/ipc/ContentParent.cpp
>-ContentParent*
>-ContentParent::GetForApp(const nsAString& aAppManifestURL)
>+/*static*/ TabParent*
>+ContentParent::CreateBrowser(mozIDOMApplication* aApp, bool aIsBrowserElement,
>+                             ContentParent** aProcess)
> {
>-    if (aAppManifestURL.IsEmpty()) {
>-        return GetNewOrUsed();
>+    if (!aApp) {
>+        if (ContentParent* cp = GetNewOrUsed()) {
>+            *aProcess = cp;
>+            return static_cast<TabParent*>(
>+                cp->SendPBrowserConstructor(
>+                    /*chromeFlags*/0,
>+                    TrustedAppId(nsIScriptSecurityManager::NO_APP_ID),
>+                    aIsBrowserElement));
>+        }
>+        return nsnull;

Be sure to change to nullptr here and elsewhere.

One can get the ContentParent off a TabParent by calling Manager(), right?  If so, can we nix the out param?

>@@ -830,32 +862,52 @@ PCompositorParent*
> PBrowserParent*
> ContentParent::AllocPBrowser(const PRUint32& aChromeFlags,
>-                             const bool& aIsBrowserElement,
>-                             const PRUint32& aAppId)
>+                             const AppId& aApp,
>+                             const bool& aIsBrowserElement)
> {
>-  TabParent* parent = new TabParent();
>-  if (parent){
>+    // We only use this Alloc() method when the content processes asks
>+    // us to open a window.  In that case, we're expecting to see the
>+    // opening PBrowser as its app descriptor, and we can trust the data
>+    // associated with that PBrowser since it's fully owned by this
>+    // process.
>+    if (AppId::TUntrustedAppId != aApp.type()) {
>+        NS_ERROR("Content process attempting to forge app ID");
>+        return nsnull;
>+    }
>+    TabParent* opener = static_cast<TabParent*>(
>+        aApp.get_UntrustedAppId().idParent());
>+
>+    // Popup windows of isBrowser frames are isBrowser if the parent
>+    // isBrowser.  Allocating a !isBrowser frame with same app ID
>+    // would allow the content to access data it's not supposed to.
>+    if (opener && opener->IsBrowserElement() && !aIsBrowserElement) {
>+        NS_ERROR("Content process attempting to escalate data access privileges");
>+        return nsnull;
>+    }

I didn't realize this until just now, but allowing !isBrowser to create
isBrowser popups (as this code correctly does) is important for in-process
<iframe mozbrowser>.  In the case when you have an OOP app with an in-process
browser, and the browser opens a popup, the popup will come across as an
isBrowser popup from a !isBrowser process, and that's not a security error.

>@@ -377,17 +377,18 @@ TabChild::BrowserFrameProvideWindow(nsID
>                                     const nsACString& aFeatures,
>                                     bool* aWindowIsNew,
>                                     nsIDOMWindow** aReturn)
> {
>   *aReturn = nsnull;
> 
>   nsRefPtr<TabChild> newChild =
>     static_cast<TabChild*>(Manager()->SendPBrowserConstructor(
>-      /* aChromeFlags = */ 0, mIsBrowserElement, mAppId));
>+                               /* aChromeFlags = */ 0,
>+                               UntrustedAppId(nsnull, this), mIsBrowserElement));

To make sure I understand what IPDL is doing here: UntrustedAppId has two
fields, idParent and idChild, and only one of them is non-null at any time?
And when you pass an UntrustedAppId through IPC, the code changes the idParent
to idChild (or vice versa) as necessary?  But for whatever reason, the codegen
has only one constructor, which takes two arguments.  But we expect only one of
them to be non-null?

Comment 68

[Jason Duell]

I'm trying to figure out how to use this patch to get secure AppIds/inBrowserElement values to necko channelParents  (right now we're just trusting whatever values the child sends us via IPDL, and these can be forged if you've got control over the channel callbacks, i.e. just implement your own nsILoadContext with appID=whatever).

IPDL necko channels are always created on the child which creates the ParentChannel.  Right now only PHttpChannel passes in a TabChild into the constructor--I assume I'll need to add these for other protocols that use the cache (PFTP, PWyciwyg).

How do I get AppId/InBrowserElement from a TabParent?  The AppProcessHasPermission() function are only for char* named permissions.

Comment 69

[Justin Lebar (not reading bugmail)]

> How do I get AppId/InBrowserElement from a TabParent?  The AppProcessHasPermission() function are 
> only for char* named permissions.

With part 1 in this bug, call TabParent::GetApp() and TabParent::IsBrowserElement().  Those are both trusted fields (and perhaps we should indicate as such in the header).

I'd have to see the code to understand whether you should be doing that or doing some sort of permission check, though...

Comment 70

[Justin Lebar (not reading bugmail)]

In case this clarifies things:

<sicking> jlebar: i thought that we had cases when <iframe mozbrowser> was going to run in-process?
<jlebar> sicking, we do.
<sicking> jlebar: maybe i'm misunderstanding comment 69
<sicking> jlebar: if <iframe mozbrowser> can run in-process, then you can't always know if a process is running content which is in-browser or not?
<jlebar> sicking, Correct.
<sicking> jlebar: i.e. a single process can contain content which is both in-browser and no in-browser
<jlebar> sicking, correct.
<jlebar> sicking, There's a greater-than-or-equal relation involved here:
<jlebar> sicking, What the parent process sees as the child process's permissions must be greater than or equal to the permissions that any content running in the child process can have.
<jlebar> sicking, So in particular, if we're running <iframe mozbrowser> in a child process that also contains app code, then to maintain that invariant, we would give "no-mozbrowser" to the child process.
<sicking> ok, i think i understand

Comment 71

[Jason Duell]

> I'd have to see the code to understand whether you should be doing that or 
> doing some sort of permission check

This is for cookies--I'm using (appId, isBrowserElement) as a tuple to index separate cookie namespaces.  I'm under the impression I don't need any permission check beyond having an AppId value that can't be tampered with by the child.  If I understand comment 70 correctly, I can get a tamper-proof AppId, but it may be shared by both the app and its mozbrowser element, in which case TabParent::IsBrowserElement is going to return false always.  If that's true I assume it's OK to instead get isIsBrowserElement from the nsILoadContext that the child sends, and trust it, since we can't really enforce any security guarantees from the parent process about what content is in/out of a browser element in the face of an attack that compromises the child.  

So I should get "bool inBrowser = TabParent::IsBrowserElement || nsILoadContent.isInBrowserElement", so when we do have a separate process for mozbrowser, we don't let it spoof as non-mozbrowser.  Correct?

Comment 72

[Justin Lebar (not reading bugmail)]

> This is for cookies--I'm using (appId, isBrowserElement) as a tuple to index separate cookie 
> namespaces.

Okay, that sounds reasonable to me!

> I'm under the impression I don't need any permission check beyond having an AppId value that can't 
> be tampered with by the child.

Well, you should make sure that an app which only contains browser-frames is not requesting a non-browser frame cookie store.

Browser tabs can run in- or out-of-process.  If the browser tabs run in the same process as the app, then when the app process requests cookies, we have to grant the request for either the non-browser cookie store or the browser cookie store.

OTOH if we know that the process only contains browser tabs and does not contain app code, then we should not grant that process access to the app's data store.  (That would mean allowing a compromised browser tab to read the full browsing history stored in the browser app.)

So what you want is RELEASE_ASSERT(TabParent::IsBrowserElement implies nsILoadContent.isInBrowserElement).  That is, !TabParent::IsBrowserElement || nsILoadContent.isInBrowserElement.

Comment 73

[Jason Duell]

bool inBrowser = TabParent::IsBrowserElement || nsILoadContent.isInBrowserElement

gives me a "safe" inBrowser element, in the sense that we don't get non-browser permissions if the Tabparent doesn't want them.

I'm happy to also add the malfeasance detection/response you suggest, but to do so I'll need a way to tell when a mozbrowser is running in a dedicated process--is there a way to know that?   Also, is RELEASE_ASSERT in the parent (i.e. parent drops dead upon detecting misbehaving child) really the response here, vs killing the child?

Comment 74

[Justin Lebar (not reading bugmail)]

> bool inBrowser = TabParent::IsBrowserElement || nsILoadContent.isInBrowserElement
> 
> gives me a "safe" inBrowser element, in the sense that we don't get non-browser permissions if the 
> Tabparent doesn't want them.

Maybe && would do that, but surely not ||.  Unless I misunderstand?

> I'm happy to also add the malfeasance detection/response you suggest, but to do so I'll need a way 
> to tell when a mozbrowser is running in a dedicated process--is there a way to know that?

That's TabParent::IsBrowserElement.

> Also, is RELEASE_ASSERT in the parent (i.e. parent drops dead upon detecting misbehaving child) 
> really the response here, vs killing the child?

No, killing the child is the right thing.  :)

Given how much confusion there is here, I definitely need to write this up somewhere.  But I don't want to write a long essay until we've baked this API a little longer...

Comment 75

[Jason Duell]

> Maybe && would do that, but surely not ||

The || works for both the the separate process case (if TabParent says it's a mozbrowser, it's a mozbrowser), and the shared-process case (otherwise we go with whatever the child report).  But you're right that to also do the kill-bad-child logic this will look different.

> That's TabParent::IsBrowserElement.

Ah, right.  Thanks.

I didn't get an answer to this question:

> Right now only PHttpChannel passes in a TabChild into the constructor--I 
> assume I'll need to add these for other protocols that use the cache (PFTP, PWyciwyg)
> 

Looks like we'll need this so we have a mechanism for getting a parent-verified AppId, which we need assuming we'll keep separate namespaces for the HTTP cache, which IIRC we will, as content will vary with cookies and we don't trust servers to put Vary: Cookies when it does].

How secure is this mechanism?  I.e. if an attacker opens a PHttpChannel and can pass in anything they want for the TabChild parameter, does IPDL enforce that only legal TabParents can be requested?  Should be just a matter of keeping a list of TabParents associated with each IPDL socket/connection...

Also, and hopefully finally:  GetApp() returns a mozIDOMApplication, and I don't see any way in that interface to get the integer AppId.  Are we planning to add that?  It's not in the patches here.

Comment 76

[Jason Duell]

To avoid confusion, may I suggest that TabParent::IsBrowserElement be renamed something like "InSeparateBrowserElementProcess"?

Comment 77

[Justin Lebar (not reading bugmail)]

> The || works for both the the separate process case (if TabParent says it's a mozbrowser, it's a 
> mozbrowser), and the shared-process case (otherwise we go with whatever the child report).  But 
> you're right that to also do the kill-bad-child logic this will look different.

Okay, I /think/ we're on the same page now.  :)

> Looks like we'll need this so we have a mechanism for getting a parent-verified AppId

That's TabParent::AppID().  I think you'd have to add that to those other protocols, unless you can get the TabParent/ContentParent from the protocol's manager chain?

> How secure is this mechanism?  I.e. if an attacker opens a PHttpChannel and can pass in anything 
> they want for the TabChild parameter, does IPDL enforce that only legal TabParents can be 
> requested?

That's a good question for cjones, but at least: We certainly intend for this to be a secure mechanism, regardless of what it is at the moment.

> Also, and hopefully finally:  GetApp() returns a mozIDOMApplication, and I don't see any way in 
> that interface to get the integer AppId.  Are we planning to add that?

AIUI the mozIDOMApplication is the thing that's exposed to the DOM.  The numeric ID is an internal implementation detail, so we wouldn't put that on the DOM interface.

I think you can go from app manifest URI to app ID using the application service or somesuch; I can look it up in the morning if you can't find it.  But depending on exactly how you're doing this, perhaps you should use nsIPrincipal::ExtendedOrigin, which is effectively an (origin, app-id, is-browser) tuple.

Comment 78

[Jason Duell]

Cookies don't use ExtendedOrigin (the namespace is per {app+inBrowser}, not per-origin).  There are also a number of other places (keeping track of TCP traffic per app, for instance) where we need the AppId, or at least some kind of ID (we could use a string, but it's got to fit in an Sqlite database, so hashing on mozIDOMApplication* isn't it.   So I'm guessing the int AppID is the way to go.  It doesn't need to be in mozIDOMApplication necessarily--I can shove whatever funkiness we use to get it into NS_GetAppInfo() in nsNetUtil.h.  Let me know.

(Do we intend to make appId suitable for array operations?  I.e. if it's 1,2,3, etc we could create arrays of TcpSocketInputBytes[max_ID_seen_so_far] and index them with the appId.  This might be efficient for simple things like arrays of int counts, even if we're not reusing AppIds when apps are uninstalled.  I'm guessing the answer is no and that we should use a hashtable<int,...> instead for all such lookups.)

Thanks for the extended Q & A (and my apologies for highjacking this bug--I keep suspecting that something I'll say will change the patches here, but perhaps not :)

Comment 79

[Jason Duell]

Assuming we can safely cast from nsITabParent to the underlying TabParent class we could just put 'int GetAppId()' as a public but non-IDL member of TabParent?

Comment 80

[Justin Lebar (not reading bugmail)]

> Cookies don't use ExtendedOrigin (the namespace is per {app+inBrowser}, not per-origin).

The namespace of a cookie /is/ per-origin, right?  mozilla.org can't read my google.com cookie.  I understand that may not match how you implement it, although I think Mounir & co. probably intended that you'd use the extended origin as a drop-in replacement for the existing origin.

> There are also a number of other places (keeping track of TCP traffic per app, for instance) where 
> we need the AppId, or at least some kind of ID (we could use a string, but it's got to fit in an 
> Sqlite database, so hashing on mozIDOMApplication* isn't it)

I think you should use the application manifest URL (or its SHA-1 hash) for this.  I don't know if the numeric IDs are intended to be persistent, but I doubt it.

> Assuming we can safely cast from nsITabParent to the underlying TabParent class we could just put 
> 'int GetAppId()' as a public but non-IDL member of TabParent?

You can safely static_cast from nsITabParent to TabParent (in fact, nsITabParent does not define any methods; it's only there to facilitate that static cast), but I think you should be using the manifest URL, not the ID.

Comment 81

[Justin Lebar (not reading bugmail)]

Comment on attachment 646701 [details] [diff] [review]
part 2: Add mozIDOMApplication.hasPermission, v2

> +[scriptable, uuid(8de25e36-b4cb-4e89-9310-a199dce4e5f4)]
>  interface mozIDOMApplication  : nsISupports
>  [...]
> +  /* Return true if this app has |permission|. */
> +  boolean hasPermission(in string permission);

Chris: This is an object which is exposed to the DOM, right?  I don't think we want to add hasPermission to the DOM object.

Comment 82

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

(In reply to Fabrice Desré [:fabrice] from comment #66)
> Comment on attachment 646701 [details] [diff] [review]
> part 2: Add mozIDOMApplication.hasPermission, v2
> 
> ::: dom/apps/src/Webapps.js
> @@ +301,5 @@
> > +  hasPermission: function(permission) {
> > +      let appsService = Cc['@mozilla.org/AppsService;1']
> > +                        .getService(Ci.nsIAppsService);
> > +      let appId = appsService.getAppLocalIdByManifestURL(this.manifestURL);
> > +      var uri = Services.io.newURI(this.origin, null, null); 
> 
> Are we testing perms against the origin, and not the manifestURL? How will
> that work with several apps per origin?
> 

Mounir thinks testing against the manifest URL is "ugly" or something ....

We're in danger of non-convergence here.

> ::: dom/apps/src/Webapps.jsm
> @@ +287,5 @@
> > +                                                       /*mozbrowser*/false);
> > +        let perm = Services.perms.testExactPermissionFromPrincipal(principal,
> > +                                                                   permission);
> > +        return (perm === Ci.nsIPermissionManager.ALLOW_ACTION);
> > +      }
> 
> We don't need that here. This is just a helper to filter out what we
> serialize to disk.
> 

No, this is what the apps service was returning to my C++ code.  Without this definition present, I was unable to call hasPermission(), XPConnect threw NO_SUCH_METHOD.  I don't know if that's expected behavior but that's how it worked.

> ::: dom/interfaces/apps/nsIDOMApplicationRegistry.idl
> @@ +40,5 @@
> >    /* startPoint will be used when several launch_path exists for an app */
> >    nsIDOMDOMRequest launch([optional] in DOMString startPoint);
> >    nsIDOMDOMRequest uninstall();
> > +  /* Return true if this app has |permission|. */
> > +  boolean hasPermission(in string permission);
> 
> Nit: DOMString

The permission manager uses |string| exclusively ....

Where do I go from here?

Comment 83

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

(In reply to Justin Lebar [:jlebar] from comment #67)
> Comment on attachment 646700 [details] [diff] [review]
> part 1: Refactor content-process/browser creation to use mozIDOMApplication
> for passing app info, v3
> 
> >diff --git a/dom/ipc/ContentChild.cpp b/dom/ipc/ContentChild.cpp
> >--- a/dom/ipc/ContentChild.cpp
> >+++ b/dom/ipc/ContentChild.cpp
> >@@ -393,20 +393,22 @@ PCompositorChild*
> > ContentChild::AllocPCompositor(ipc::Transport* aTransport,
> >                                base::ProcessId aOtherProcess)
> > {
> >     return CompositorChild::Create(aTransport, aOtherProcess);
> > }
> > 
> > PBrowserChild*
> > ContentChild::AllocPBrowser(const PRUint32& aChromeFlags,
> >-                            const bool& aIsBrowserElement,
> >-                            const PRUint32& aAppId)
> >+                            const AppId& aApp,
> >+                            const bool& aIsBrowserElement)
> > {
> >-    nsRefPtr<TabChild> iframe = new TabChild(aChromeFlags, aIsBrowserElement, aAppId);
> >+    PRUint32 appId = aApp.get_TrustedAppId().id();
> 
> I'm not convinced that this Trusted/Untrusted ID business adds anything.
> 

It's just syntax.

> Therefore the types are misleading: If ContentParent::AllocPBrowser receives
> a TrustedAppId, it must not trust it!  At that point, it's not really
> accurate to say that the datatype is "trusted".
> 

If we attempt to send data that needs trust from untrusted->trusted, it's rejected.  If we attempt to do the opposite it's nonsensical.  I'm not sure why this rubs you the wrong way but let's drop the extra wrapper datatypes that are intended to clarify these flows, since apparently they're not doing so.

> Does IPDL let us declare different Alloc signatures for child and parent? 
> If so, could we have the alloc-in-child take an ID, and the alloc-in-parent
> take a TabParent?
> 

No.

> >diff --git a/dom/ipc/ContentParent.cpp b/dom/ipc/ContentParent.cpp
> >--- a/dom/ipc/ContentParent.cpp

> One can get the ContentParent off a TabParent by calling Manager(), right? 
> If so, can we nix the out param?
> 

Sure.

> I didn't realize this until just now, but allowing !isBrowser to create
> isBrowser popups (as this code correctly does) is important for in-process
> <iframe mozbrowser>.  In the case when you have an OOP app with an in-process
> browser, and the browser opens a popup, the popup will come across as an
> isBrowser popup from a !isBrowser process, and that's not a security error.
> 

We don't support that in v1, I thought.

> To make sure I understand what IPDL is doing here: UntrustedAppId has two
> fields, idParent and idChild, and only one of them is non-null at any time?
> And when you pass an UntrustedAppId through IPC, the code changes the
> idParent
> to idChild (or vice versa) as necessary?  But for whatever reason, the
> codegen
> has only one constructor, which takes two arguments.  But we expect only one
> of
> them to be non-null?

Yes.

Comment 84

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

(In reply to Justin Lebar [:jlebar] from comment #81)
> Comment on attachment 646701 [details] [diff] [review]
> part 2: Add mozIDOMApplication.hasPermission, v2
> 
> > +[scriptable, uuid(8de25e36-b4cb-4e89-9310-a199dce4e5f4)]
> >  interface mozIDOMApplication  : nsISupports
> >  [...]
> > +  /* Return true if this app has |permission|. */
> > +  boolean hasPermission(in string permission);
> 
> Chris: This is an object which is exposed to the DOM, right?  I don't think
> we want to add hasPermission to the DOM object.

hasPermission isn't exposed.

Comment 85

[Justin Lebar (not reading bugmail)]

>> I didn't realize this until just now, but allowing !isBrowser to create
>> isBrowser popups (as this code correctly does) is important for in-process
>> <iframe mozbrowser>.  In the case when you have an OOP app with an in-process
>> browser, and the browser opens a popup, the popup will come across as an
>> isBrowser popup from a !isBrowser process, and that's not a security error.
>
> We don't support that in v1, I thought.

Actually, forget this whole bit.  We'll never support this unless we supported cross-process DOM calls.

> I'm not sure why this rubs you the wrong way

What I find really confusing is that we do

  foo(AppID id):
    if id is a trusted id, reject it, because we don't accept trusted ids in this circumstance.

It's one thing to use types to assert control flow trust, but here we're doing the opposite: the types assert the untrusted actor's claim of whether it would like us to trust its data, and then we have to decide if we trust /that/.

Comment 86

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

(In reply to Justin Lebar [:jlebar] from comment #85)
> > I'm not sure why this rubs you the wrong way
> 
> What I find really confusing is that we do
> 
>   foo(AppID id):
>     if id is a trusted id, reject it, because we don't accept trusted ids in
> this circumstance.
> 

Perhaps the naming is bad?  TrustedId was meant to be, IdRequiresTrust.  (Not that that's a much better name ...)

  foo(AppID id):
     if id is IdRequiresTrust, reject it, because we don't trust the sender

At any rate, I'll do the next iteration without those wrappers.

Comment 87

[Justin Lebar (not reading bugmail)]

>> Chris: This is an object which is exposed to the DOM, right?  I don't think
>> we want to add hasPermission to the DOM object.
>
> hasPermission isn't exposed.

I dug through the apps code, and it looks like you're right.  The object we return to the DOM is very similar to mozIDOMApplication, but not actually a mozIDOMApplication object.  See webapps.jsm::_cloneAppObject.

I'd argue that we should not call it mozI/DOM/Application, but at least we could put a comment in the IDL...

Comment 88

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

I meant more specifically that, whether or not the objects are handed to the content DOM, 'hasPermission' is not one of the __exposedProps__.

Comment 89

[[:fabrice] Fabrice Desré]

(In reply to Chris Jones [:cjones] [:warhammer] from comment #88)
> I meant more specifically that, whether or not the objects are handed to the
> content DOM, 'hasPermission' is not one of the __exposedProps__.

That doesn't prevent it (yet) to be seen by content code unfortunately. So your current patch effectively exposes hasPermission() to web content.

And _cloneAppObject() was initially designed to be used as a serializing sanitizer, not to create internal objects. We'll need to split this up in two functions.

If you want it to be seen only by chrome code, I think you need another interface extending mozIDOMApplication

Comment 90

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

Can you describe exactly what you want, and please answer questions in comment 82?  I don't have a clear idea of where to go to get r+ at this point.

Comment 91

[[:fabrice] Fabrice Desré]

(In reply to Chris Jones [:cjones] [:warhammer] from comment #82)

> > Are we testing perms against the origin, and not the manifestURL? How will
> > that work with several apps per origin?
> > 
> 
> Mounir thinks testing against the manifest URL is "ugly" or something ....
> 
> We're in danger of non-convergence here.

My main concern is that useing the origin means that we can't set different permissions when we'll have several apps per origin. 

> > ::: dom/apps/src/Webapps.jsm
> > @@ +287,5 @@
> > > +                                                       /*mozbrowser*/false);
> > > +        let perm = Services.perms.testExactPermissionFromPrincipal(principal,
> > > +                                                                   permission);
> > > +        return (perm === Ci.nsIPermissionManager.ALLOW_ACTION);
> > > +      }
> > 
> > We don't need that here. This is just a helper to filter out what we
> > serialize to disk.
> > 
> 
> No, this is what the apps service was returning to my C++ code.  Without
> this definition present, I was unable to call hasPermission(), XPConnect
> threw NO_SUCH_METHOD.  I don't know if that's expected behavior but that's
> how it worked.
>
>
> > ::: dom/interfaces/apps/nsIDOMApplicationRegistry.idl
> > @@ +40,5 @@
> > >    /* startPoint will be used when several launch_path exists for an app */
> > >    nsIDOMDOMRequest launch([optional] in DOMString startPoint);
> > >    nsIDOMDOMRequest uninstall();
> > > +  /* Return true if this app has |permission|. */
> > > +  boolean hasPermission(in string permission);
> > 
> > Nit: DOMString
> 
> The permission manager uses |string| exclusively ....
> 
> Where do I go from here?

Here's what I would try:

Define a new interface 
mozIInternalApplication : mozIDOMApplication {
  boolean hasPermission(in string permission)
}

Change getAppByManifestURL() in Webapps.jsm to something like:

  if (app.manifestURL == aManifestURL) {
       let res = this._cloneAppObject(app);
       res.hasPermission = function(aPerm) {
         ... // your code
      }
        return res;

and remove what you added to Webapps.js

Could this work for you with this setup?
      }

Comment 92

[Jason Duell]

re: comment 80: > The namespace of a cookie /is/ per-origin, right?

Nope.  Desktop Firefox has one cookie DB (cookies have their own funky rules about origins: if you made the DB per-origin we can't share cookies between host1.foo.com and host2.foo.com), so the logical B2G mapping is one per app (plus another one if an app has a browser element, so browser cookies don't mingle with the app's).

If a 20-byte SHA-1 string per cookie entry in SQLite is the smallest/fastest thing to use, so be it.  If we can get an integer type things would be faster and the cookie DB will take up less room (possibly including RAM: the cookie service winds up storing all cookies in RAM, and I'll need to either keep the SHA-1 field in each entry (perhaps we can get all same-values to point to the same nsString internal buffer), or keep some sort of int->SHA-1 string mapping.  These seem like needless hoops to jump through if there's any way we can provide a consistent int AppId.

Comment 93

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

(In reply to Fabrice Desré [:fabrice] from comment #91)
> (In reply to Chris Jones [:cjones] [:warhammer] from comment #82)
> 
> Could this work for you with this setup?
>

It does!  Thanks :).

Comment 94

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

Attachment: part 1: Add mozIApplication in order to expose a hasPermission() through it

Comment 95

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

Comment on attachment 648618 [details] [diff] [review]
part 1: Add mozIApplication in order to expose a hasPermission() through it

Dangit, wrong patch.

Comment 96

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

Attachment: part 1: Add mozIApplication in order to expose a hasPermission() through it, v3

Comment 97

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

Attachment: part 2: Refactor content-process/browser creation to use mozIApplication for passing app info, v4

Comment 98

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

Attachment: part 3: Add an API for checking whether apps loaded in PBrowserParent/PContentParent have the specific capability

carrying over r=jlebar sr=bz

Comment 99

[Justin Lebar (not reading bugmail)]

Comment on attachment 648627 [details] [diff] [review]
part 2: Refactor content-process/browser creation to use mozIApplication for passing app info, v4

>diff --git a/dom/ipc/ContentChild.cpp b/dom/ipc/ContentChild.cpp
>--- a/dom/ipc/ContentChild.cpp
>+++ b/dom/ipc/ContentChild.cpp
>@@ -397,20 +397,21 @@ PCompositorChild*
> PBrowserChild*
> ContentChild::AllocPBrowser(const PRUint32& aChromeFlags,
>-                            const bool& aIsBrowserElement,
>-                            const PRUint32& aAppId)
>+                            const bool& aIsBrowserElement, const AppId& aApp)
> {
>-    nsRefPtr<TabChild> iframe = new TabChild(aChromeFlags, aIsBrowserElement, aAppId);
>+    PRUint32 appId = aApp.get_uint32_t();

What happens if aApp is not a uint32_t?  We crash the process?  Mostly, I want
to make sure we don't send 0, which would probably not be good.

>-ContentParent*
>-ContentParent::GetForApp(const nsAString& aAppManifestURL)
>+/*static*/ TabParent*
>+ContentParent::CreateBrowser(mozIApplication* aApp, bool aIsBrowserElement)
> {
>-    if (aAppManifestURL.IsEmpty()) {
>-        return GetNewOrUsed();
>+    if (!aApp) {
>+        if (ContentParent* cp = GetNewOrUsed()) {

Nit: if ((...)) so the compiler doesn't complain about the assignment-in-if.

>@@ -149,17 +154,17 @@ rpc protocol PContent
> 
> both:
>     // Depending on exactly how the new browser is being created, it might be
>     // created from either the child or parent process!
>     //
>     // The child creates the PBrowser as part of
>     // TabChild::BrowserFrameProvideWindow, and the parent creates the PBrowser
>     // as part of ContentParent::CreateTab.
>-    async PBrowser(PRUint32 chromeFlags, bool isBrowserElement, PRUint32 appId);
>+    async PBrowser(PRUint32 chromeFlags, bool isBrowserElement, AppId appId);

Please update the comment to indicate that the AppId received by the parent
must be a PBrowser, while the AppId received by the child must be a uint32_t.

Comment 100

[[:fabrice] Fabrice Desré]

Comment on attachment 648626 [details] [diff] [review]
part 1: Add mozIApplication in order to expose a hasPermission() through it, v3

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

::: dom/apps/src/Webapps.jsm
@@ +692,5 @@
>        if (app.manifestURL == aManifestURL) {
> +        let res = this._cloneAppObject(app);
> +        res.hasPermission = function(permission) {
> +          let localId = DOMApplicationRegistry.getAppLocalIdByManifestURL(
> +            this.manifestURL);

Nit: you could use |this.localId| by adding localId in |_cloneAppObject|

::: dom/interfaces/apps/nsIDOMApplicationRegistry.idl
@@ +51,5 @@
> +{
> +  /* Return true if this app has |permission|. */
> +  boolean hasPermission(in string permission);
> +};
> +

Nit: can you move this interface to its own file since it's not web-content facing?

Comment 101

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

Jonas, the review here knocks out 3 blockers right off, and unblocks work on 2 more.

Comment 102

[Justin Lebar (not reading bugmail)]

Comment on attachment 648626 [details] [diff] [review]
part 1: Add mozIApplication in order to expose a hasPermission() through it, v3

Can we kick this SR over to Mounir?

Comment 103

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

Fabrice, are you OK with that?

Comment 104

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

Comment on attachment 648626 [details] [diff] [review]
part 1: Add mozIApplication in order to expose a hasPermission() through it, v3

Internal API, so fabrice doesn't think sr is needed anymore.

Comment 105

[Mounir Lamouri (:mounir)]

(In reply to Fabrice Desré [:fabrice] from comment #91)
> (In reply to Chris Jones [:cjones] [:warhammer] from comment #82)
> 
> > > Are we testing perms against the origin, and not the manifestURL? How will
> > > that work with several apps per origin?
> > > 
> > 
> > Mounir thinks testing against the manifest URL is "ugly" or something ....
> > 
> > We're in danger of non-convergence here.
> 
> My main concern is that useing the origin means that we can't set different
> permissions when we'll have several apps per origin. 

We use the origin to pass an URL. We also use the appId so this is not going to make multi-app per origin impossible. Given that the passed URL is going to be compared to origin at some point, using the app's origin is by far the best solution because we know the app's pages are inside the app origin while the app manifest could be in a different origin (I don't know if we check that on install but the fact that I have to check prove that we shouldn't do that).

(In reply to Chris Jones [:cjones] [:warhammer] from comment #104)
> Comment on attachment 648626 [details] [diff] [review]
> part 1: Add mozIApplication in order to expose a hasPermission() through it,
> v3
> 
> Internal API, so fabrice doesn't think sr is needed anymore.

There are no rules that says internal API shouldn't be super-reviewed...

Comment 106

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

https://hg.mozilla.org/integration/mozilla-inbound/rev/2be79bff0426
https://hg.mozilla.org/integration/mozilla-inbound/rev/c132707b8ca0
https://hg.mozilla.org/integration/mozilla-inbound/rev/8fc9f7605bb6

Comment 107

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

Backed out

https://hg.mozilla.org/integration/mozilla-inbound/rev/1be3b987748a

Comment 108

[Ryan VanderMeulen [:RyanVM]]

In addition to the Mac build failures, this also had mochitest-2 failures.
https://tbpl.mozilla.org/php/getParsedLog.php?id=14245582&tree=Mozilla-Inbound

Comment 109

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

https://hg.mozilla.org/integration/mozilla-inbound/rev/97a699cd8ae9
https://hg.mozilla.org/integration/mozilla-inbound/rev/0ecd2d7ed3ae
https://hg.mozilla.org/integration/mozilla-inbound/rev/d2465f38b85b

Comment 110

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/97a699cd8ae9
https://hg.mozilla.org/mozilla-central/rev/0ecd2d7ed3ae
https://hg.mozilla.org/mozilla-central/rev/d2465f38b85b