Closed Bug 773886 Opened 12 years ago Closed 12 years ago

Prevent loading resources from app:// URIs from outside that app

Categories

(Core :: Networking, defect)

Product:
Core
Component:
Networking
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

()

Status:
VERIFIED FIXED
Milestone:
mozilla18
Project Flags:
blocking-kilimanjaro +
blocking-basecamp +

People

(Reporter: briansmith, Assigned: amac)

References

Details

(Whiteboard: [WebAPI:P2], [LOE:S])

Attachments

(2 files, 3 obsolete files)

Remove unneeded flag, and more space fixes
8.83 KB, patch
sicking
: review+
Details | Diff | Splinter Review
Interdiff file for the last patch
650 bytes, patch
Details | Diff | Splinter Review 
+++ 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.
Wouldn't it be enough to change
(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://).
(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.
What we need to do here is change nsScriptSecurityManager::CheckLoadURI such that it forbids "normal pages" from loading app:// urls.
(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.
(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?
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.
(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.
No longer blocks: b2g-updates
I'm back from my vacation. Since I had already been looking into this, want me to take this?
Sure, feel free to assign it to yourself.
Can't :) It's assigned to Jonas.
Assignee: jonas → amac
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?
Status: NEW → ASSIGNED
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.
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.
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.
I agree, a) seems like the better solution from a threat management standpoint.
Whiteboard: [WebAPI:P2]
Attached patch Patch for the affected files (obsolete) — — Splinter Review 
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)
Attachment #658989 - Flags: review?
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.
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).
Who should review this?
Whiteboard: [WebAPI:P2] → [WebAPI:P2], [LOE:S]
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.
Attachment #658989 - Flags: review?(jonas) → review-
Attached patch New version, with review changes (obsolete) — — Splinter Review 
Implemented the requested changes.

I also added a ContentUtils::IsExactPermDeny. I don't need it, but that way it stays homogeneous.
Attachment #658989 - Attachment is obsolete: true
Attachment #661631 - Flags: review?(jonas)
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 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 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.
Attachment #661631 - Flags: review?(jonas) → review+
(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.
Fixed another whitespace in TestSitePerm definition also.
Attachment #661631 - Attachment is obsolete: true
Attachment #661639 - Flags: review?(jonas)
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.
Attachment #661639 - Flags: review?(jonas) → review+
Comment on attachment 661645 [details] [diff] [review]
Remove unneeded flag, and more space fixes

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

Awesome!
Attachment #661645 - Flags: review+
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.
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.
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.
What does this need to land?  Some checkin-wanted flag?
Do I have to set that flag? If that's so, how? (I can't see anything similar on the options above)
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
https://hg.mozilla.org/mozilla-central/rev/5c69df73929c
Status: ASSIGNED → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla18
Keywords: verifyme
QA Contact: jsmith
Status: RESOLVED → VERIFIED
Keywords: verifyme
(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.
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: