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.

(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?)

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.

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?

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....?)

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.

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

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?

(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! :)

(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.

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

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.

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

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?

(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.

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!

(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.

(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.

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.

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.

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!

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?

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!

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 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 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.

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. :)

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!

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

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 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.

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 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.

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

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!

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 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 on attachment 700872 [details] Part 2, call .uninstall() from mozIDOMApplicationMgmt in Gaia The gaia team should review this

(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 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 :)

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

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 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.

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

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

(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.

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

Too trigger-happy. Also https://hg.mozilla.org/releases/mozilla-b2g18/rev/56debcc19f14

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

(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

(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.

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.

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.

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

(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.

Okay sounds good. Thanks for double checking that.