Last Comment Bug 773886 - Prevent loading resources from app:// URIs from outside that app
: Prevent loading resources from app:// URIs from outside that app
Status: VERIFIED FIXED
[WebAPI:P2], [LOE:S]
:
Product: Core
Classification: Components
Component: Networking (show other bugs)
: unspecified
: All All
: -- normal (vote)
: mozilla18
Assigned To: Antonio Manuel Amaya Calvo (:amac)
: Jason Smith [:jsmith]
Mentors:
Depends on:
Blocks: 769350 773891
  Show dependency treegraph
 
Reported: 2012-07-13 17:39 PDT by Brian Smith (:briansmith, :bsmith, use NEEDINFO?)
Modified: 2012-10-01 15:21 PDT (History)
23 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
+
+


Attachments
Patch for the affected files (4.65 KB, patch)
2012-09-06 13:54 PDT, Antonio Manuel Amaya Calvo (:amac)
jonas: review-
Details | Diff | Review
New version, with review changes (9.68 KB, patch)
2012-09-16 13:55 PDT, Antonio Manuel Amaya Calvo (:amac)
jonas: review+
Details | Diff | Review
Fixed whitespace and removed unwanted change in Payments.jsm (8.84 KB, patch)
2012-09-16 15:19 PDT, Antonio Manuel Amaya Calvo (:amac)
jonas: review+
Details | Diff | Review
Remove unneeded flag, and more space fixes (8.83 KB, patch)
2012-09-16 15:35 PDT, Antonio Manuel Amaya Calvo (:amac)
jonas: review+
Details | Diff | Review
Interdiff file for the last patch (650 bytes, patch)
2012-09-16 15:36 PDT, Antonio Manuel Amaya Calvo (:amac)
no flags Details | Diff | Review

Description Brian Smith (:briansmith, :bsmith, use NEEDINFO?) 2012-07-13 17:39:08 PDT
+++ This bug was initially created as a clone of Bug #769350 +++

Apps should not be able to load resources from other apps, and web content should not be able to load content from apps.

fabrice noted that "the homescreen needs to access icons from inside the package while being another app itself," so it seems like we need to have a (certified-app-only?) permission that allows an app to load another app's resources.

Jonas said that, in order to do this, we may need a new implementation of app:// channels, so that asyncOpen can access the load group context to allow/deny access to the app resource based on the requesting context.
Comment 1 Antonio Manuel Amaya Calvo (:amac) 2012-07-18 16:51:53 PDT
Wouldn't it be enough to change
Comment 2 Antonio Manuel Amaya Calvo (:amac) 2012-07-18 17:01:45 PDT
(In reply to Antonio Manuel Amaya Calvo from comment #1)
> Wouldn't it be enough to change

Ugh sorry about that. I was going to ask if wouldn't it be enough to change URI_LOADABLE_BY_ANYONE by URI_LOADABLE_BY_SUBSUMERS and to place the homescreen on a domain that has the rest of the apps as subsumers? 

But... that would work without changing anything else for standard apps. For third party apps though some magic would be required to make the homescreen domain a parent of every possible domain. So probably not a good idea then. 

By the way, does URI_NORELATIVE actually do anything? Neither with MXR nor with a generous use of grep can I find it used anywhere. It's returned on a lot of places, but can't find anywhere that actually checks its value to do something. 

And... if the only exception is homescreen, and it only needs to access the icons, wouldn't it be better to make the icons a separate part of the package, accessible by anyone that can actually enumerate the installed apps (such as the homescreen). That way no extra permission would be needed and the homescreen could only load the icon (and probably not through app:// but through file:// so no exceptions are required on the border controls for app://).
Comment 3 Brian Smith (:briansmith, :bsmith, use NEEDINFO?) 2012-07-18 17:13:19 PDT
(In reply to Antonio Manuel Amaya Calvo from comment #2)
> By the way, does URI_NORELATIVE actually do anything? Neither with MXR nor
> with a generous use of grep can I find it used anywhere. It's returned on a
> lot of places, but can't find anywhere that actually checks its value to do
> something. 

No. See bug 773898 and bug 652114.

> And... if the only exception is homescreen, and it only needs to access the
> icons, wouldn't it be better to make the icons a separate part of the
> package, accessible by anyone that can actually enumerate the installed apps
> (such as the homescreen). That way no extra permission would be needed and
> the homescreen could only load the icon (and probably not through app:// but
> through file:// so no exceptions are required on the border controls for
> app://).

Alternatively, we could have the app installer extract the icon and give it to the home screen app and have the home screen app store the icon however it wants.
Comment 4 Jonas Sicking (:sicking) 2012-07-18 22:01:09 PDT
What we need to do here is change nsScriptSecurityManager::CheckLoadURI such that it forbids "normal pages" from loading app:// urls.
Comment 5 [:fabrice] Fabrice Desré 2012-07-20 04:28:13 PDT
(In reply to Brian Smith (:bsmith) from comment #3)

> Alternatively, we could have the app installer extract the icon and give it
> to the home screen app and have the home screen app store the icon however
> it wants.

There can be multiple icons specified in the manifest (different sizes, and possibly overridden by locale) so we can't do that.
Comment 6 Antonio Manuel Amaya Calvo (:amac) 2012-07-30 05:36:21 PDT
(In reply to Jonas Sicking (:sicking) from comment #4)
> What we need to do here is change nsScriptSecurityManager::CheckLoadURI such
> that it forbids "normal pages" from loading app:// urls.

How do you define what is a 'normal page' though?
Comment 7 Antonio Manuel Amaya Calvo (:amac) 2012-07-30 15:02:25 PDT
I've been playing a little with this (and putting generous debug traces on CheckLoadUriWithPrincipal :) ) and discovered some things:

1. On the current implementation of the app protocol handler the flags aren't being applied. The attribute should be protocolFlags (AppProtocolHandler.js line 31) and it's protocolFlags2.

2. Changing the permissions to URI_DANGEROUS_TO_LOAD (and protocolflags2 to protocolflags) solves part of the problem. At least app:// can't be loaded from other protocols anymore. 

But you can still load app://whatever from any other app://otherwhatever. So the homescreen still works which is good and you can still open any app from inside the browser app, which isn't that good.
Comment 8 [:fabrice] Fabrice Desré 2012-07-31 00:50:18 PDT
(In reply to Antonio Manuel Amaya Calvo from comment #7)
> I've been playing a little with this (and putting generous debug traces on
> CheckLoadUriWithPrincipal :) ) and discovered some things:
> 
> 1. On the current implementation of the app protocol handler the flags
> aren't being applied. The attribute should be protocolFlags
> (AppProtocolHandler.js line 31) and it's protocolFlags2.

ouch - looks like I missed a qref before landing... we should fix that asap.

> 2. Changing the permissions to URI_DANGEROUS_TO_LOAD (and protocolflags2 to
> protocolflags) solves part of the problem. At least app:// can't be loaded
> from other protocols anymore. 

true

> But you can still load app://whatever from any other app://otherwhatever. So
> the homescreen still works which is good and you can still open any app from
> inside the browser app, which isn't that good.

To do that we probably need what Jonas described.
Comment 9 Antonio Manuel Amaya Calvo (:amac) 2012-08-29 03:04:15 PDT
I'm back from my vacation. Since I had already been looking into this, want me to take this?
Comment 10 [:fabrice] Fabrice Desré 2012-08-29 04:03:26 PDT
Sure, feel free to assign it to yourself.
Comment 11 Antonio Manuel Amaya Calvo (:amac) 2012-08-29 04:16:31 PDT
Can't :) It's assigned to Jonas.
Comment 12 Antonio Manuel Amaya Calvo (:amac) 2012-09-03 08:49:49 PDT
I've been thinking about what a 'normal page' is in this context. Oh, and the problem isn't restricted to the homescreen App. The System app also needs to be able to load app:// resources (since launching applications is basically creating a new iframe and loading the launch URL there). As I see it, we have two options:

a) Define a new permission (or maybe reuse/expand the webapps permission?) so only applications with that permission can load app:// resources for a different domain. Then we can just give the new permission to the system and homescreen apps and we're done.

b) Define a new protocol flag, something like: URL_LOADABLE_BY_CERTIFIED that: 
  * Forbids loading from a different protocol and
  * Allow loading pages for a different domain only if the calling page is certified.

Option b would require also a small change on the Gaia browser app, since it should enforce a restriction to forbid an app:// uri to be loaded directly (be it by WebActivity, or by directly entering the URL on the URL bar). Note that the restriction is just for the initial URL load since anything that the URL loads is covered by the protocol handler itself. 

Personally I like more option b since it seems more in line on how the rest of the flags work (this is after all just another restriction that could be enforced on a protocol handler). Option a seems more flexible but really it would be something more specific to this protocol only.

So what do you think?
Comment 13 Jonas Sicking (:sicking) 2012-09-03 09:07:18 PDT
I think we should do the a) proposal above. I think we can even reuse/extend the webapps permission to also allow loading resources from other app:// domains.
Comment 14 Jonas Sicking (:sicking) 2012-09-03 09:14:42 PDT
The reason I don't want to do b) is that I don't think that all certified apps should be allowed to load resources from other apps. There simply isn't a reason for them to do so, and so it's nice defence-in-depth if we only allow certain apps to be able to figure out which other apps you have installed.

In general I don't think being a installed/privileged/certified app should give you any additional privileges. It's much more explicit and easier to control if you have to enumerate all the additional permissions that you need in your app manifest and then controlled through nsIPermissionManager. That way we have better control over who can do what and we have the ability to modify that on a more fine-grained level.
Comment 15 Antonio Manuel Amaya Calvo (:amac) 2012-09-03 16:11:49 PDT
Ok, I still think than the b) option is more in line with the way the rest of the protocols work, and could be reused for any other protocol that had a similar restriction but... I see your point also, and it's true than using an explicit permission for this would mitigate a possible compromise of a certified app. And on top of that the a) option doesn't require modifying the browser app.

So unless anyone has another opinion, we'll implement the a) option then.
Comment 16 Lucas Adamski [:ladamski] 2012-09-04 13:37:42 PDT
I agree, a) seems like the better solution from a threat management standpoint.
Comment 17 Antonio Manuel Amaya Calvo (:amac) 2012-09-06 13:54:46 PDT
Created attachment 658989 [details] [diff] [review]
Patch for the affected files

I finally opted for something between options a and b (see comment#12). I added a new flag to avoid making an specific check for the app:// protocol, and used the webapps-manage permission (webapps doesn't exist)
Comment 18 Antonio Manuel Amaya Calvo (:amac) 2012-09-06 14:00:13 PDT
Added a patch that fixes the problem for me. What it does is:

* Define a new protocol flag (to avoid making the change too protocol-specific)
* Modify AppProtocolHandler to apply the new flag (also, to apply any flags, see  comment 7
* Modify CheckLoadUriWithPrincipal so when the protocol has the flag set it only allows opening URIs from another domain if the opening domain/app has the webapps-manage permission set.

Note that currently there is a couple of bugs in Gaia (or at least I think they're  bugs) where the system and homescreen apps ask for the incorrect permission in their manifest (webapps instead of webapps-manage) and where the build process adds the webapps-manage to EVERY application in the build, which kinda defeats the purpose of this patch. I'm filing an issue (and will do a pull request with the changes) on Gaia now.
Comment 19 Antonio Manuel Amaya Calvo (:amac) 2012-09-06 14:19:01 PDT
Opened the issue https://github.com/mozilla-b2g/gaia/issues/4427 on Gaia for the permissions errors (and made a pull request that fixes it also).
Comment 20 Antonio Manuel Amaya Calvo (:amac) 2012-09-08 05:28:53 PDT
Who should review this?
Comment 21 Jonas Sicking (:sicking) 2012-09-16 12:26:30 PDT
Comment on attachment 658989 [details] [diff] [review]
Patch for the affected files

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

Nit: There's somewhitespace at the end of a few of the lines here. Feel free to fix that too.

Patch looks great, just need to fix the permissionmanager check. Sorry our permission manager fits our needs very poorly these days.

::: caps/src/nsScriptSecurityManager.cpp
@@ +1419,5 @@
> +            // In this case, we allow opening only if the source and target URIS
> +            // are on the same domain, or the opening URI has the webapps 
> +            // permision granted
> +            if (!SecurityCompareURIs(sourceBaseURI,targetBaseURI) &&
> +                !nsContentUtils::IsSitePermAllow(aPrincipal,WEBAPPS_PERM_NAME)){

Actually, this will end up calling TestPermissionFromPrincipal, but I think we want TestExactPermissionFromPrincipal. Yeah, the permissionmanager API is really bad :(

The easiest fix is probably to add a nsContentUtils::IsExactSitePermAllow function.

::: netwerk/base/public/nsIProtocolHandler.idl
@@ +245,5 @@
> +    /**
> +     * URIs for this protocol require the webapps permission on the principal
> +     * when opening URIs for a different domain. See bug#773886
> +     */
> +    const unsigned long URI_NEEDS_WEBAPPS_PERM= (1<<16);

Whitespace before '='.

Also, maybe rename this to URI_CROSS_ORIGIN_NEEDS_WEBAPPS_PERM since we still allow same-origin loads without the perm.
Comment 22 Antonio Manuel Amaya Calvo (:amac) 2012-09-16 13:55:21 PDT
Created attachment 661631 [details] [diff] [review]
New version, with review changes

Implemented the requested changes.

I also added a ContentUtils::IsExactPermDeny. I don't need it, but that way it stays homogeneous.
Comment 23 Antonio Manuel Amaya Calvo (:amac) 2012-09-16 13:56:16 PDT
Quick question, I think I got rid of all the extra spaces at the end of lines, but... is there any way to check for that quickly?
Comment 24 [:fabrice] Fabrice Desré 2012-09-16 14:05:27 PDT
Comment on attachment 661631 [details] [diff] [review]
New version, with review changes

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

You can check trailing whitespace in your favorite editor usually.

::: dom/payment/Payment.jsm
@@ +25,5 @@
>                                     "@mozilla.org/preferences-service;1",
>                                     "nsIPrefService");
>  
>  function debug (s) {
> +  dump("-*- PaymentManager: " + s + "\n");

Nit: undo this change
Comment 25 Jonas Sicking (:sicking) 2012-09-16 14:45:32 PDT
Comment on attachment 661631 [details] [diff] [review]
New version, with review changes

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

r=me if you fix my and Fabrice's nits.

Thanks!!

::: content/base/src/nsContentUtils.cpp
@@ +2899,5 @@
>  
>  bool
>  nsContentUtils::IsSitePermAllow(nsIPrincipal* aPrincipal, const char* aType)
>  {
> +  return TestSitePerm(aPrincipal, aType, nsIPermissionManager::ALLOW_ACTION,false);

Add a space after the last ','. Same thing at the other callsites.
Comment 26 Antonio Manuel Amaya Calvo (:amac) 2012-09-16 15:07:51 PDT
(In reply to Fabrice Desré [:fabrice] from comment #24)
> Comment on attachment 661631 [details] [diff] [review]
> New version, with review changes
> 
> Review of attachment 661631 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> You can check trailing whitespace in your favorite editor usually.
> 
> ::: dom/payment/Payment.jsm
> @@ +25,5 @@
> >                                     "@mozilla.org/preferences-service;1",
> >                                     "nsIPrefService");
> >  
> >  function debug (s) {
> > +  dump("-*- PaymentManager: " + s + "\n");
> 
> Nit: undo this change

Ugh, hardly a nit. Forgot to leave that as it was after checking something for QA saturday night :(. Changing now.
Comment 27 Antonio Manuel Amaya Calvo (:amac) 2012-09-16 15:19:16 PDT
Created attachment 661639 [details] [diff] [review]
Fixed whitespace and removed unwanted change in Payments.jsm

Fixed another whitespace in TestSitePerm definition also.
Comment 28 Jonas Sicking (:sicking) 2012-09-16 15:26:07 PDT
Comment on attachment 661639 [details] [diff] [review]
Fixed whitespace and removed unwanted change in Payments.jsm

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

If you are just fixing review comments, and the reviewer marked the previous patch with r+, you don't need to actually ask for review again.

And if you are fixing more things, please detail what else was changed so that the reviewer doesn't have to look through the whole patch again. Ideally by attaching an interdiff.

::: netwerk/protocol/app/AppProtocolHandler.js
@@ +30,1 @@
>                    Ci.nsIProtocolHandler.URI_NOAUTH |

Whitespace here is off now. If you're changing this to use protocolFlags, could you also remove the NORELATIVE part since that's not something that we actually want.
Comment 29 Antonio Manuel Amaya Calvo (:amac) 2012-09-16 15:35:54 PDT
Created attachment 661645 [details] [diff] [review]
Remove unneeded flag, and more space fixes
Comment 30 Antonio Manuel Amaya Calvo (:amac) 2012-09-16 15:36:34 PDT
Created attachment 661646 [details] [diff] [review]
Interdiff file for the last patch
Comment 31 Jonas Sicking (:sicking) 2012-09-16 15:38:05 PDT
Comment on attachment 661645 [details] [diff] [review]
Remove unneeded flag, and more space fixes

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

Awesome!
Comment 32 [:fabrice] Fabrice Desré 2012-09-16 16:05:19 PDT
Does this means that a store can't access the icons from apps that were installed from this very same store? One does not need the webapps-manage permission to call mozApps.getInstalled(), and at this point we trust the caller to get information from the app.
Comment 33 Jonas Sicking (:sicking) 2012-09-16 16:13:49 PDT
The store can call getInstalled still, but it can't load data from apps tgat it has installed. Do they want/need to do that?

It would be tricky to let stores access data from apps they have installed. Especially once we allow hosted apps to add/remove resources from their appcache.
Comment 34 Jonas Sicking (:sicking) 2012-09-16 16:18:34 PDT
On an unrelated note, we should probably make the app protocol imlementation honor this bug too. I.e. to be fully sure that apps can't load cross app data, we should make the app protocol get the appid that created the channel and make sure that it is either loading same-app data, or that it has webapps-manage permission.

As a separate patch/bug of course.
Comment 35 Andrew Overholt [:overholt] 2012-09-19 13:18:29 PDT
What does this need to land?  Some checkin-wanted flag?
Comment 36 Antonio Manuel Amaya Calvo (:amac) 2012-09-24 01:09:39 PDT
Do I have to set that flag? If that's so, how? (I can't see anything similar on the options above)
Comment 37 Andrew Overholt [:overholt] 2012-09-25 07:19:55 PDT
I think this is the relevant document: https://developer.mozilla.org/en-US/docs/Developer_Guide/How_to_Submit_a_Patch#Committing_the_patch and
Comment 38 Ryan VanderMeulen [:RyanVM] 2012-09-25 09:59:06 PDT
I don't see a Try run here, so I've pushed it to Try to be safe. I'll land it if it's green.

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

Also Antonio, please make sure that your future patches include the information given below. It makes life easier for those checking in on your behalf.
https://developer.mozilla.org/en-US/docs/Creating_a_patch_that_can_be_checked_in
Comment 40 Mounir Lamouri (:mounir) 2012-09-26 04:08:04 PDT
https://hg.mozilla.org/mozilla-central/rev/5c69df73929c
Comment 41 Antonio Manuel Amaya Calvo (:amac) 2012-09-28 03:59:14 PDT
(In reply to Andrew Overholt [:overholt] from comment #37)
> I think this is the relevant document:
> https://developer.mozilla.org/en-US/docs/Developer_Guide/
> How_to_Submit_a_Patch#Committing_the_patch and

(In reply to Ryan VanderMeulen from comment #38)
> I don't see a Try run here, so I've pushed it to Try to be safe. I'll land
> it if it's green.
> 
> https://tbpl.mozilla.org/?tree=Try&rev=de4b3b31df98
> 
> Also Antonio, please make sure that your future patches include the
> information given below. It makes life easier for those checking in on your
> behalf.
> https://developer.mozilla.org/en-US/docs/
> Creating_a_patch_that_can_be_checked_in

Thanks both for the info, I'll be sure to do that next time around.

Note You need to log in before you can comment on or make changes to this bug.