820206 - Validate "Webapps:*" message parameters in the parent process

Status: RESOLVED FIXED

(Core Graveyard :: DOM: Apps, defect, P2)

Description

[Paul Theriault [:pauljt] (no longer reading bugmail)]

The Webapps:Install passes an app object which describes the app to be installed. This app object is derived from the manifest contents and location by the child. There are many validation checks in the child (webapps.js) that are not replicated in the parent. Potential issues:
- Passing a manifestURL which doesnt match the manifest object. 
- Request an app type of privileged or certified (should be web only)
- Supply an arbitrary appid, which may overwrite an existing app, or potentially allow arbitray file overwrites (since id is used for a directory name)

The are probably other issues - I haven't tested spoofing these message from a child process yet. For now these issues are raised from the results of code review only.

Comment 1

[Paul Theriault [:pauljt] (no longer reading bugmail)]

(Combining all Webapps:* issues in one bug, since the fixes are releated)The following issues were found during auditing:

Webapps:Install
The Webapps:Install passes an app object which describes the app to be installed. This app object is derived from the manifest contents and location by the child. There are many validation checks in the child (webapps.js) that are not replicated in the parent. Potential issues:
- Passing a manifestURL which doesnt match the manifest object. 
- Request an app type of privileged or certified (should be web only)
- Supply an arbitrary appid, which may overwrite an existing app, or potentially allow arbitray file overwrites (since id is used for a directory name)

Webapps:Uninstall
Webapps.js only allows an page to uninstall apps itself (app.origin is set to current origin). Being able to craft this message allows child to uninstall any application. Parent could check that origin being unstailled is origin of sender of message - but I guess this would break the home screen. So perhaps only allow uninstall for other origins if the sender has the webapps-manager permission.

Webapps:GetSelf
Check in parent that app id corresponds to the page sending this message. I think a compromised process could use the Webapps:GetList message (see below) to retrieve all the valid app ids, and then get the app objects using this message. 

Webapps:CheckInstalled
Similar to GetSelf - before sending this message, the are checks in child to ensure it may load manifest (http://mxr.mozilla.org/mozilla-central/source/dom/apps/src/Webapps.js#173) but this check can be bypassed if child process is compromised. 

Webapps:GetInstalled
Origin is set to current origin in the child. Compromised child could request app object for any origin.

Webapps:Launch
Not check in parent that origin of message matches origin. A compromised child which knows app ids and origins, can launch arbitrary apps.

Webapps:InstallPackage
Similar issues to Webapps:Install - parameter validation is in child not parent.

Webapps:GetBasePath
This is used by the app:// protocol handler to get the actual file loaction of the package on disk. A compromised child could use this message to do the same. I don’t know enough about packaged app loading to say whether or not this is preventable (is loading done by child or parent?)

Webapps:GetList
This is used by AppService to return list of apps to the child (importing webapps.jsm doesn’t work OOP afaik.) So anything that uses the AppService will need this, but there is not permission associated with this, so all child processes will have access to this. 

Webapps:RegisterForMessages
Webapps.js uses this to register for specific messages, but there is no check in the parent as to what messages are being listened for. I am not sure if this is an issue - would there be any benefit to the child to be able add itself as a listener for arbitrary messages (perhaps to intercept sensitive messages?)

Comment 2

[Jason Smith [:jsmith]]

Someone from triage should comment on whether anything in the above comments blocks. We may need to break this bug into separate parts of what blocks vs. not though.

Comment 3

[[:fabrice] Fabrice Desré]

Some of these methods are not tied to the page doing the request, but to an object (usually an app object). For instance, launch() is a method on the app object, so I'm not sure how to fix the issue for these methods.

What is actually safe to pass from the child to the parent?

Comment 4

[Paul Theriault [:pauljt] (no longer reading bugmail)]

This effort may need more definition about restrictions/assumptions of this API. I have assumed that an App should only be to get an |app| object for itself (same origin), unless it has the webapps-manage permission. And hence we would be able to have a check in the parent code which handles Webapps:Launch to check that the child  which sent the message (message.target I think?) either has the webapps-manage permission, or is same-origin with the app manifest of the app being launched.

This may be naive - I am basing this assumption on what I have been able to glean from code review. Not sure if this would stomp on any valid use cases. The Homescreen App does have a webapps-manage permission. However the Marketplace doesn't it seems, and I had heard you could launch apps from there. Does anyone know more about this? (how does it even get the list of apps if it doesn't have the permission though....?)

Comment 5

[[:fabrice] Fabrice Desré]

An page can get access to app object in different ways:

- the app itself can get its own app object using getSelf().
- a store can get all apps that were installed from the store.
- an app with the webapps-manage privileges can get all apps.

Once you have access to an app object, nothing prevents you to call any method on this object. This includes launch(), uninstall(), checkForUpdate(), download(). You can also set listeners for events on the object.

I'm trying to get the appId from the child sending messages to the parent, but no luck so far.

Comment 6

[Andrew Overholt [:overholt]]

Andrea, please take this off Fabrice's overflowing plate :)

Comment 7

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

Sorry about the churn. I just realized that this is pretty similar to bug 821607, so maybe something that Gene could take a look at?

Comment 8

[Gene Lian (I already quit Mozilla)]

(In reply to Jonas Sicking (:sicking) from comment #7)
> Sorry about the churn. I just realized that this is pretty similar to bug
> 821607, so maybe something that Gene could take a look at?

Glad to take this! :)

Comment 9

[Gene Lian (I already quit Mozilla)]

(In reply to Jonas Sicking (:sicking) from comment #7)
> Sorry about the churn. I just realized that this is pretty similar to bug
> 821607, so maybe something that Gene could take a look at?

After looking more into this issue, eventually I think I need to release the assignee because I'm actually on a vacation from 12/24 to 1/3 and it's a bit hard for me to solve that before C3 (1/1)... especially when I was aware of being assigned to this bug just few days before leaving for the vacation.

At bug 821671 (still under Jonas' review), I've already provided a utility function to check if the message carries the right manifest URL belonging to that child, which might help to solve partial issues here.

I'm glad to grab it again if no one is able to support to take this.

Comment 10

[Gene Lian (I already quit Mozilla)]

My vacation is over. Let me retake this. ;)

Comment 11

[Paul Theriault [:pauljt] (no longer reading bugmail)]

Gene, I noticed another potential attack vector when reviewing updates. Basically the child tell the parent to update, and tells it the manifestURL for the app it is updating:

http://mxr.mozilla.org/mozilla-central/source/dom/apps/src/Webapps.js#399
399     cpmm.sendAsyncMessage("Webapps:CheckForUpdate",
400                           { manifestURL: this.manifestURL,
401                             oid: this._id,
402                             requestID: this.getRequestId(request) });

Actually, now that I look deeper, _oid_ isn't used in the parent, so all that an app could do would be to cause another app to check for an update. So this probably isn't an issue. But maybe we should remove the _oid_ if it isnt removed, to avoid this in the future.

Comment 12

[Paul Theriault [:pauljt] (no longer reading bugmail)]

correction in last sentence: But maybe we should remove the _oid_ if it isnt USED, to avoid this in the future.

Comment 13

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

Here are the messages that I think we can reasonably check in the parent right now:

> Webapps:Install
> Webapps:InstallPackage

We should check more things in the parent than we are it sounds like. But ultimately we'll have to allow calls to this one from any child process.

> Webapps:Uninstall

This one we should lock down more tightly. We should only allow this call from the app itself, and from apps which have "manage-apps" privilege.

> Webapps:GetSelf

The parent process should be able to check that this one is only called from the app itself. However I also don't think it's a blocker to add parent checks for this one since you can derive the same information from other messages.

> Webapps:CheckInstalled

This one must be permitted from any child process. I don't know that there are any checks that makes sense to do in the parent.

> Webapps:GetInstalled

Currently we just track which origin is used to install an app. If we instead tracked origin+appid+browserflag for who installed an app then we could in the parent enforce that this function is only called to get the list of apps installed form that appid.

> Webapps:Launch
> Not check in parent that origin of message matches origin. A compromised
> child which knows app ids and origins, can launch arbitrary apps.

This one must be permitted from any child process. I don't know that there are any checks that makes sense to do in the parent.

> Webapps:GetBasePath

This one is not a bigger information leak than Webapps:CheckInstalled I would think?

> Webapps:GetList

I think this one could be restricted to apps which have "manage-webapps" permission?

> Webapps:RegisterForMessages

Isn't this one just system message handling which I thought we did have decent checks in the parent for?


Ultimately I think the only real blocker here is adding checks to Webapps:Uninstall. But maybe Webapps:Install/Package needs more sanity checks in the parent too?

Comment 14

[[:fabrice] Fabrice Desré]

(In reply to Jonas Sicking (:sicking) from comment #13)

> > Webapps:Uninstall
> 
> This one we should lock down more tightly. We should only allow this call
> from the app itself, and from apps which have "manage-apps" privilege.

When you say "from the app itself", you mean that only "twitter" (or the homescreen that has manage-webapps privilege) could uninstall "twitter"? This is slightly different from the current behavior where we let anyone that got a app object call methods on this object. So for instance, a store could provide a "uninstall" button in its own UI, which seems a valid use case.


> > Webapps:GetInstalled
> 
> Currently we just track which origin is used to install an app. If we
> instead tracked origin+appid+browserflag for who installed an app then we
> could in the parent enforce that this function is only called to get the
> list of apps installed form that appid.

I agree, moving to appid + browserflag is the way to go here. We'll also need that when adding support for multiple apps per origin.


> > Webapps:GetBasePath
> 
> This one is not a bigger information leak than Webapps:CheckInstalled I
> would think?

This lets the attacker know the location on disk where the application manifest and package are. Not sure if that's exploitable now that we have remoted the app:// protocol and use proper permissions.

> > Webapps:GetList
> 
> I think this one could be restricted to apps which have "manage-webapps"
> permission?

It's only used internally by other chrome code, so if we can check that the caller has a system principal let's do that.

> > Webapps:RegisterForMessages
> 
> Isn't this one just system message handling which I thought we did have
> decent checks in the parent for?

No, this one is for internal management of message broadcasting. I'm not sure yet what to do here, and if it's exploitable meaningfully.

> 
> Ultimately I think the only real blocker here is adding checks to
> Webapps:Uninstall. But maybe Webapps:Install/Package needs more sanity
> checks in the parent too?

I mostly agree, but note that Webapps:Uninstall also triggers a confirmation dialog, so there's no way to wipe all apps without user consent.

Comment 15

[Gene Lian (I already quit Mozilla)]

Hi Jonas and Fabrice,

I had lots of questions about why/how to do the checks in the parent. Could you please return some feedbacks or hints to me? Any thought is highly appreciated!

(In reply to Jonas Sicking (:sicking) from comment #13)
> Here are the messages that I think we can reasonably check in the parent
> right now:
> 
> > Webapps:Install
> > Webapps:InstallPackage
> 
> We should check more things in the parent than we are it sounds like. But
> ultimately we'll have to allow calls to this one from any child process.

If we'll allow any child process to do these, why do we need to add extra checks in parent for now? What do you suggest for the checks in parent if we really need to do so?

> > Webapps:Uninstall
> 
> This one we should lock down more tightly. We should only allow this call
> from the app itself, and from apps which have "manage-apps" privilege.

We might be able to pass the child's app ID and check it in the parent to ensure this is only called from the app itself. But why do we need to consider other apps since the current design is only to uninstall the app itself?

> > Webapps:GetSelf
> 
> The parent process should be able to check that this one is only called from
> the app itself. However I also don't think it's a blocker to add parent
> checks for this one since you can derive the same information from other
> messages.

Do you mean the hacked child could get other apps' objects from other messages (like Webapps:CheckInstalled?) so it doesn't help a lot to do extra checks for Webapps:GetSelf?

> > Webapps:CheckInstalled
> 
> This one must be permitted from any child process. I don't know that there
> are any checks that makes sense to do in the parent.

But how to prevent the hacked child sending a fake manifestURL that Paul mentioned in comment #1?

> > Webapps:GetInstalled
> 
> Currently we just track which origin is used to install an app. If we
> instead tracked origin+appid+browserflag for who installed an app then we
> could in the parent enforce that this function is only called to get the
> list of apps installed form that appid.

I think I can pass the app ID and check it in the parent.

> > Webapps:Launch
> > Not check in parent that origin of message matches origin. A compromised
> > child which knows app ids and origins, can launch arbitrary apps.
> 
> This one must be permitted from any child process. I don't know that there
> are any checks that makes sense to do in the parent.

So we don't need to worry that the hacked child process could launch arbitrary apps? Why? Because the activities mechanism already allow us to do so?

> > Webapps:GetBasePath
> 
> This one is not a bigger information leak than Webapps:CheckInstalled I
> would think?

It seems this message has been removed from Webapps.jsm. No need to consider it.

> > Webapps:GetList
> 
> I think this one could be restricted to apps which have "manage-webapps"
> permission?
>
> It's only used internally by other chrome code, so if we can check that
> the caller has a system principal let's do that.

What's the feature of a system principal? Could you please give me some hints about how to detect it?

> > Webapps:RegisterForMessages
> 
> Isn't this one just system message handling which I thought we did have
> decent checks in the parent for?
>
> No, this one is for internal management of message broadcasting. I'm
> not sure yet what to do here, and if it's exploitable meaningfully.

I don't quite understand how or why to do the check exactly since it's just passing some normal message names during initialization and uninitialization and should have no way to imitate other apps' identities. Right?


Btw, it there any reason why we don't need to examine the following messages?

  Webapps:GetAppInfo
  Webapps:Download
  Webapps:cancelDownload
  Webapps:CheckForUpdate

where the following ones already has the "webapps-manage" permission check so we don't need to consider them:

  Webapps:GetAll
  Webapps:GetNotInstalled
  Webapps:ApplyDownload


Sorry for bothering you guys with so many questions at a time!

Comment 16

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

(In reply to Fabrice Desré [:fabrice] (not reading bugmail) from comment #14)
> (In reply to Jonas Sicking (:sicking) from comment #13)
> 
> > > Webapps:Uninstall
> > 
> > This one we should lock down more tightly. We should only allow this call
> > from the app itself, and from apps which have "manage-apps" privilege.
> 
> When you say "from the app itself", you mean that only "twitter" (or the
> homescreen that has manage-webapps privilege) could uninstall "twitter"?

Yes.

> This is slightly different from the current behavior where we let anyone
> that got a app object call methods on this object. So for instance, a store
> could provide a "uninstall" button in its own UI, which seems a valid use
> case.

Yeah. I know we discussed this a bunch a few months back where we decided that it didn't make sense for anyone that to be able get hole of an app object to be able to uninstall that app. But it appears that no bugs got filed and my memory is very hazy.

I'm honestly not sure it makes sense for anyone but the home screen to uninstall an app. I can see the use-case for marketplace doing so, but I'm not sure it's worth the hassle. But I guess that if we're tracking the appid/browserflag that installed the app, then limiting it to that might be ok. In addition to anyone with manage-apps of course.

I'm honestly not even sure it makes sense for apps themselves to be able to uninstall. I've never seen an app do that and it exposes additional risk if someone manages to hack the app.

> > > Webapps:GetBasePath
> > 
> > This one is not a bigger information leak than Webapps:CheckInstalled I
> > would think?
> 
> This lets the attacker know the location on disk where the application
> manifest and package are. Not sure if that's exploitable now that we have
> remoted the app:// protocol and use proper permissions.

Yeah, I'd say let's not worry about it.

The only real concern here is fingerprinting and the privacy implications of figuring out which apps a user has installed. But as long as we have checkInstalled we'll have that problem. So let's not worry about it for v1.

> > > Webapps:GetList
> > 
> > I think this one could be restricted to apps which have "manage-webapps"
> > permission?
> 
> It's only used internally by other chrome code, so if we can check that the
> caller has a system principal let's do that.

We can't reliably know if it's chrome-code in the child process vs. something else in the child process.

Or do you mean that it's only used by chrome code in the parent process?

> > > Webapps:RegisterForMessages
> > 
> > Isn't this one just system message handling which I thought we did have
> > decent checks in the parent for?
> 
> No, this one is for internal management of message broadcasting. I'm not
> sure yet what to do here, and if it's exploitable meaningfully.

Ok. Maybe file a followup on this so that we can figure out what needs to be done?

> > Ultimately I think the only real blocker here is adding checks to
> > Webapps:Uninstall. But maybe Webapps:Install/Package needs more sanity
> > checks in the parent too?
> 
> I mostly agree, but note that Webapps:Uninstall also triggers a confirmation
> dialog, so there's no way to wipe all apps without user consent.

I still think that we should limit who can call uninstall. It's definitely scary that the only thing preventing very bad dataloss is a single security dialog.

Especially when the use-cases aren't terribly strong since uninstalling through the homescreen is quite easy.

Comment 17

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

(In reply to Gene Lian [:gene] from comment #15)
> (In reply to Jonas Sicking (:sicking) from comment #13)
> > Here are the messages that I think we can reasonably check in the parent
> > right now:
> > 
> > > Webapps:Install
> > > Webapps:InstallPackage
> > 
> > We should check more things in the parent than we are it sounds like. But
> > ultimately we'll have to allow calls to this one from any child process.
> 
> If we'll allow any child process to do these, why do we need to add extra
> checks in parent for now? What do you suggest for the checks in parent if we
> really need to do so?

See the issues listed in comment 1. Paul was worried that the following things might be possible.

- Passing a manifestURL which doesnt match the manifest object. 
- Request an app type of privileged or certified (should be web only)
- Supply an arbitrary appid, which may overwrite an existing app, or potentially allow arbitray file overwrites (since id is used for a directory name)

I haven't looked at the code closely enough to see if there's a problem here. It would be great if you could help with that.

> > > Webapps:Uninstall
> > 
> > This one we should lock down more tightly. We should only allow this call
> > from the app itself, and from apps which have "manage-apps" privilege.
> 
> We might be able to pass the child's app ID and check it in the parent to
> ensure this is only called from the app itself. But why do we need to
> consider other apps since the current design is only to uninstall the app
> itself?

Other apps than the app itself should be able to uninstall an app. At the very least the homescreen app needs to be able to uninstall any app, right?

> > > Webapps:GetSelf
> > 
> > The parent process should be able to check that this one is only called from
> > the app itself. However I also don't think it's a blocker to add parent
> > checks for this one since you can derive the same information from other
> > messages.
> 
> Do you mean the hacked child could get other apps' objects from other
> messages (like Webapps:CheckInstalled?) so it doesn't help a lot to do extra
> checks for Webapps:GetSelf?

Exactly.

> > > Webapps:CheckInstalled
> > 
> > This one must be permitted from any child process. I don't know that there
> > are any checks that makes sense to do in the parent.
> 
> But how to prevent the hacked child sending a fake manifestURL that Paul
> mentioned in comment #1?

Which specific part of Paul's comment are you referring to?

> > > Webapps:GetInstalled
> > 
> > Currently we just track which origin is used to install an app. If we
> > instead tracked origin+appid+browserflag for who installed an app then we
> > could in the parent enforce that this function is only called to get the
> > list of apps installed form that appid.
> 
> I think I can pass the app ID and check it in the parent.

Not sure what you mean?

> > > Webapps:Launch
> > > Not check in parent that origin of message matches origin. A compromised
> > > child which knows app ids and origins, can launch arbitrary apps.
> > 
> > This one must be permitted from any child process. I don't know that there
> > are any checks that makes sense to do in the parent.
> 
> So we don't need to worry that the hacked child process could launch
> arbitrary apps? Why? Because the activities mechanism already allow us to do
> so?

No, because any app is allowed to launch() an app. Any app can create an <iframe> to any origin, the contents of the <iframe> will run in the same process. That iframe can then call checkInstalled() and then .launch() on the resulting app. 

So there is no way in the parent process to limit who can call this function.

Also, launching arbitrary apps isn't really a big problem.

> Btw, it there any reason why we don't need to examine the following messages?
> 
>   Webapps:GetAppInfo
>   Webapps:Download
>   Webapps:cancelDownload
>   Webapps:CheckForUpdate

Paul did an audit and commented on what looked wrong. If you think additional things might be wrong then we should absolutely fix those too.

> Sorry for bothering you guys with so many questions at a time!

If you don't think you are the right person to fix this, then definitely don't hesitate to let someone else take it.

Comment 18

[[:fabrice] Fabrice Desré]

What about adding to nsFrameMessageManager.cpp an interface to get the appId from the child side (we added assertPermission in /content/base/public/nsIMessageManager.idl).

Then we could use this child_appId to know what the content is allowed to do on the current app object.

Comment 19

[Gene Lian (I already quit Mozilla)]

Attachment: Part 1, provide .assertContainAppId()

Changes are trivial to provide a new nsIMessageManager.assertContainAppId() for internal use, which redirects the check to manifest URL, thus sharing lots of common logic.

Comment 20

[Gene Lian (I already quit Mozilla)]

Attachment: Part 2, check child process

Hi Fabrice,

Based on previous comments, I think we should check if the child process has the privilege to get the app object, which is permitted only when the app ID is matched (the calling child process is exactly itself). This needs to be done for:

  "Webapps:Install",
  "Webapps:GetSelf",
  "Webapps:CheckInstalled",
  "Webapps:GetInstalled",
  "Webapps:InstallPackage"

For the "Webapps:Uninstall", since an app can uninstall itself and can also uninstall others with the "webapps-manage" permission, we need to check both manifest URL and permission.

Honestly, I didn't follow very well with the previous comments. Could you please return some feedback to me based on this on-going patch? Thanks!

Comment 21

[[:fabrice] Fabrice Desré]

Gene, I don't think your current patches are doing what we need.

What we want to protect against is this kind of scenario:
A hacked content process sends an IPC request to undelete app_1. To do this, it will send a message with app_1's origin. What we need to check is if the app running in the content process may have accessed this app legitimately. So we have to make sure that:
- the content app id is the same as app_1's app id or
- app_1 could have been installed by the content app (so app_1 installOrigin is the origin of the content app) or
- the content process has the webapps-manage permission.

Does this make sense to you?

Comment 22

[Gene Lian (I already quit Mozilla)]

Attachment: Part 1, provide .checkContainApp()/.assertOrigin()/.checkOrigin()

Hi Fabrice,

The following functions in nsIMessageManager.idl can assert if a child process has certain capabilities (if no, then kill that child process):

  assertPermission()
  assertContainApp()
  assertOrigin() (new)

Also, the following functions can check if a child process has certain capabilities (just returning true/false without killing the child process):

  checkPermission() (new)
  checkContainApp() (new)
  checkOrigin() (new)

Please see part-2 patch for how to use the new functions. Thanks!

Comment 23

[Gene Lian (I already quit Mozilla)]

Attachment: Part 2, check child process's capabilities

Following the comment #21, to prevent the hacked child process from sending commands to parent to uninstall the app, we need to check the process's:

  - manifest URL to see if it is uninstalling the app itself.
  - install origin to see if it is uninstalling the app intalled by itself.
  - permission to see if it has 'webapps-manage' to manage the app.

Please let me know if it doesn't make sense to you. :)

Comment 24

[[:fabrice] Fabrice Desré]

Comment on attachment 699711 [details] [diff] [review]
Part 1, provide .checkContainApp()/.assertOrigin()/.checkOrigin()

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

That look fine to me, but please ask for review to someone that touched this code before.

Comment 25

[[:fabrice] Fabrice Desré]

Comment on attachment 699714 [details] [diff] [review]
Part 2, check child process's capabilities

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

Looks good to me. Please run the tests, as I fear they will break because the desktop implementation doesn't use the mozapps stuff yet.

Comment 26

[Gene Lian (I already quit Mozilla)]

Attachment: Part 1, provide .assertAppOrigin(), V2

Hi Jonas,

Fabrice already had feedback+ on this but we need you to take a second review since you used to have experiences on these codes. Summaries:

1. Provided a new |.assertAppOrigin()| branch for checking the app's origin of child process.

2. Provided a backward-compatible parameter |aKillProc| to let callers have the flexibility of whether to kill the child process if its capability is invalid. We need this because we will check multiple capabilities for a child process, which means we cannot directly kill the child process when its first capability is not passed.

Please let me know if it doesn't make sense to you. Thanks so much for your review again. :)

Comment 27

[Gene Lian (I already quit Mozilla)]

Attachment: Part 2, check child process's capabilities, V2

An optional parameter |aKillProc| is used so that we don't have to create 2 separate functions for checking the capability. For example, let's combine:

  assertAppOrigin(aOrigin)
  checkAppOrigin(aOrigin)

into

  assertAppOrigin(aOrigin, aKillProc)

Thanks for the review!

Comment 28

[Gene Lian (I already quit Mozilla)]

Try: https://tbpl.mozilla.org/?tree=Try&rev=3965c28b279e. Looks fine!

Comment 29

[Gene Lian (I already quit Mozilla)]

Sorry Overholt... It's really late now and I'm afraid I have to leave. As discussed on the IRC, if you're rushing to get this done, please feel free to reassign this to someone else to take over the remaining tasks. I'm totally fine with that. :)

Comment 30

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

Comment on attachment 700188 [details] [diff] [review]
Part 2, check child process's capabilities, V2

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

See comment below for why this is wrong.

I wonder if we should simply keep it simple for now and only permit applications with webapps-manage to uninstall applications.

That way the check in the parent process becomes a simple .assertPermission.

We'd also have to change the child such that calling .uninstall() returns an error event if the caller doesn't have webapps-manage permission. Ideally we would even move the .uninstall() call to the mozApps.mgmt object, but that would require some gaia changes.

::: dom/apps/src/Webapps.jsm
@@ +2092,5 @@
> +      //   - manifest URL to see if it's uninstalling the app itself.
> +      //   - install origin to see if it's uninstalling the app installed by itself.
> +      //   - permission to see if it has 'webapps-manage' to manage the app.
> +      if (!aMm.assertContainApp(app.manifestURL, false) &&
> +          !aMm.assertAppOrigin(app.installOrigin, false) &&

This assertAppOrigin call isn't right.

The origin of the page that called .install(), and thus is the .installOrigin, doesn't neccessarily need to be the origin of the app that called .install().

Consider for example if the user browses to http://mystore.com in the browser app. If this page calls .install() and installs an app, the .installOrigin for that app will be http://mystore.com.

However the app installing the app is the browser app, which obviously does not have http://mystore.com as app-origin.

Comment 31

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

We discussed what to do for this bug and came to the conclusion that the simplest thing is to limit the ability to uninstall apps to *only* applications that have webapps-manage permission. So we should do the following:

A) Simply add a aMm.assertPermission("webapps-manage") call in the parent process.

This fixes the most critical part of this bug. Additionally we should either do

B) Additionally add a call to the Application.uninstall() function which makes the
   function return an error if the caller doesn't have webapps-manage permission.
   You can do that by doing something similar to [1].

or even better

C) Remove the Application.uninstall() function and move it to
   mozIDOMApplicationMgmt. This should be quite easy by making the implementation
   similar to the one for mozIDOMApplicationMgmt.applyUpdate. I.e. make the
   function take an application object and send the same data to the parent
   process as is currently sent.
   The only part that is tricky about this is that we need to change the settings
   and homescreen apps so that they call uninstall the new way.


[1] http://mxr.mozilla.org/mozilla-central/source/dom/apps/src/Webapps.js#595

Comment 32

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

Comment on attachment 700186 [details] [diff] [review]
Part 1, provide .assertAppOrigin(), V2

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

This won't be needed with the simpler/safer strategy.

Comment 33

[Gene Lian (I already quit Mozilla)]

Thanks Jonas so much for the detailed comment. I'll come back with the new patch ASAP. Please stay tuned.

Comment 34

[Gene Lian (I already quit Mozilla)]

Attachment: Part 1, move .uninstall() from mozIDOMApplication to mozIDOMApplicationMgmt

Hi Jonas,

Thanks for your detailed suggestion! This patch does the following change:

  1. Move .uninstall() from mozIDOMApplication to mozIDOMApplicationMgmt.
  2. .uninstall(in mozIDOMApplication app) eats an app object now.
  3. Since the original .uninstall() returns a DOMRequest, we still need to fire success/error for the DOMRequest of mozApps.mgmt which was calling the .uninstall().
  4. Add permission check for .uninstall() coming from mozApps.mgmt.
  5. Please see part-2 patch later for the corresponding Gaia changes.

Thanks a lot for your time for the reviews!

Comment 35

[Gene Lian (I already quit Mozilla)]

Attachment: Part 2, call .uninstall() from mozIDOMApplicationMgmt in Gaia

This patch is the corresponding change in Gaia. Needs to be checked in as soon as the part-1 patch is checked in. Please let me know if you think I need to find a right person to review this part. Thanks again!

Comment 36

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

Comment on attachment 700868 [details] [diff] [review]
Part 1, move .uninstall() from mozIDOMApplication to mozIDOMApplicationMgmt

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

This looks good, but we also need to call .assertPermission("webapps-manage") somewhere. I think that's something that's missing for all mgmt-related messages. Could you attach a separate patch to add that?

Comment 37

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

Comment on attachment 700872 [details]
Part 2, call .uninstall() from mozIDOMApplicationMgmt in Gaia

The gaia team should review this

Comment 38

[Gene Lian (I already quit Mozilla)]

(In reply to Jonas Sicking (:sicking) from comment #36)
> Comment on attachment 700868 [details] [diff] [review]
> Part 1, move .uninstall() from mozIDOMApplication to mozIDOMApplicationMgmt
> 
> Review of attachment 700868 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> This looks good, but we also need to call
> .assertPermission("webapps-manage") somewhere. I think that's something
> that's missing for all mgmt-related messages. Could you attach a separate
> patch to add that?

Hi Jonas,

I've already added it. ;) Please let me know if I misunderstood. Thanks!

     // We need to check permissions for calls coming from mozApps.mgmt.
-    // These are: getAll(), getNotInstalled() and applyDownload()
+    // These are: getAll(), getNotInstalled(), applyDownload() and uninstall().
     if (["Webapps:GetAll",
          "Webapps:GetNotInstalled",
-         "Webapps:ApplyDownload"].indexOf(aMessage.name) != -1) {
+         "Webapps:ApplyDownload",
+         "Webapps:Uninstall"].indexOf(aMessage.name) != -1) {
       if (!aMessage.target.assertPermission("webapps-manage")) {
         debug("mozApps message " + aMessage.name +
         " from a content process with no 'webapps-manage' privileges.");
         return null;
       }
     }

Comment 39

[Etienne Segonzac (:etienne)]

Comment on attachment 700872 [details]
Part 2, call .uninstall() from mozIDOMApplicationMgmt in Gaia

The code looks good!
But I didn't made a custom build to test it.
If you already did that we're all good, if not, we should :)

Comment 40

[[:fabrice] Fabrice Desré]

Gene, we also need to fix the tests that uninstall apps. Let me know if you don't have time to do it.

Comment 41

[Gene Lian (I already quit Mozilla)]

Attachment: Part 3, test cases

Hi Fabrice,

I'm working on the test cases. Lots of cases need to be changed... As my best understanding, we don't need to worry about adding "webapps-manage" permission for *.xul, since it must be loaded on the chrome process. Right?

Except for the browser_webapps_permissions.js, others test cases have already included |navigator.mozApps.mgmt.xxx| (ex, .getAll()), thus having "webapps-manage" permission, so I believe we can directly apply |navigator.mozApps.mgmt.uninstall()| without worrying about the permission issue.

I'm still waiting on the final try result at: https://tbpl.mozilla.org/?tree=Try&rev=ff4dfc0d612a. Before that, could you please give me some feedbacks at the same time? Thanks!

Comment 42

[[:fabrice] Fabrice Desré]

Comment on attachment 700944 [details] [diff] [review]
Part 3, test cases

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

Looks good. Let's hope we're not missing any.

Comment 43

[Gene Lian (I already quit Mozilla)]

Another try: https://tbpl.mozilla.org/?tree=Try&rev=808c9eb15630

Comment 44

[Gene Lian (I already quit Mozilla)]

Another try: https://tbpl.mozilla.org/?tree=Try&rev=11976f11226e

Comment 45

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

(In reply to Paul Theriault [:pauljt] from comment #1)
> Webapps:RegisterForMessages
> Webapps.js uses this to register for specific messages, but there is no
> check in the parent as to what messages are being listened for. I am not
> sure if this is an issue - would there be any benefit to the child to be
> able add itself as a listener for arbitrary messages (perhaps to intercept
> sensitive messages?)

Ah, now I see what's going on here. I *think* that we are fine here as long as all message listening is managed on a per-childprocess basis (or per-application basis).

But might be worth filing a separate bug on the RegisterForMessages and UnregisterForMessages messages and audit those separately.

Comment 46

[Gene Lian (I already quit Mozilla)]

Fixed more test cases...

Another try: https://tbpl.mozilla.org/?tree=Try&rev=bb65023ca673

Comment 47

[Gene Lian (I already quit Mozilla)]

Attachment: Part 1, move .uninstall() from mozIDOMApplication to mozIDOMApplicationMgmt, V2

Comment 48

[Gene Lian (I already quit Mozilla)]

Attachment: Part 3, test cases, V1.1

Comment 49

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

https://hg.mozilla.org/releases/mozilla-b2g18/rev/77579969377c

Comment 50

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

Too trigger-happy. Also

https://hg.mozilla.org/releases/mozilla-b2g18/rev/56debcc19f14

Comment 51

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

https://hg.mozilla.org/integration/mozilla-inbound/rev/ddd84bce57fb
https://hg.mozilla.org/integration/mozilla-inbound/rev/a87848d15ab4

Comment 52

[Gene Lian (I already quit Mozilla)]

(In reply to Jonas Sicking (:sicking) from comment #45)
> But might be worth filing a separate bug on the RegisterForMessages and
> UnregisterForMessages messages and audit those separately.

Fire Bug 829619 to keep track of that.

Btw, does anyone know who should (me?) and where to update the spec document if this bug is specified by "dev-doc-needed"? Is that at [1]? However, that page hasn't kept updated at all for a long time. Lots of function descriptions are wrong, though.

[1] https://developer.mozilla.org/en-US/docs/Apps/Apps_JavaScript_API?redirectlocale=en-US&redirectslug=OpenWebApps%2FThe_JavaScript_API

Comment 53

[[:fabrice] Fabrice Desré]

(In reply to Gene Lian [:gene] from comment #52)

> Btw, does anyone know who should (me?) and where to update the spec document
> if this bug is specified by "dev-doc-needed"? Is that at [1]? However, that
> page hasn't kept updated at all for a long time. Lots of function
> descriptions are wrong, though.

I set the dev-doc-needed so that the documentation team notice they need to make a change. I'll ping them anyway early next week.

Comment 54

[Jason Smith [:jsmith]]

Mark Giffin usually does the updating. And yeah, the JS API pieces are quite out of date. Post Jan 15th, I might want to sit down with Janet and Mark with some other relevant parties to figure out what dev docs need updating.

Comment 55

[Jason Smith [:jsmith]]

Hmm...so we implemented the fix here for Gaia, but I'm wondering if this is going to break uninstalling web apps on Android.

Wes - Do you know?

I'll quickly check when I get an FF Android Nightly build with this patch.

Comment 56

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/a87848d15ab4
https://hg.mozilla.org/mozilla-central/rev/ddd84bce57fb

Comment 57

[[:fabrice] Fabrice Desré]

(In reply to Jason Smith [:jsmith] from comment #55)
> Hmm...so we implemented the fix here for Gaia, but I'm wondering if this is
> going to break uninstalling web apps on Android.
> 
> Wes - Do you know?
> 
> I'll quickly check when I get an FF Android Nightly build with this patch.

The patch fixes about:apps on Android, so we should be good there.

Comment 58

[Jason Smith [:jsmith]]

Okay sounds good. Thanks for double checking that.