InterAppCommService.js is the parent process file and does checking to ensure inter app communication messages are valid. There are several fields that it is necessary to validate: manifestURL, appStatus, and installOrigin. receiveMessage() does check the manifestURL at [1]. We can see aMessage.json.manifestURL is passed to assertContainApp() which verfies that aMessage.json.manifestURL is the same manifest URL of the application that sent the message. This prevents an application from sending a message with a spoofed manifest URL. installOrigin is checked at [2], since we have validated aManifestURL (aMessage.json.manifestURL). appStatus is never validated and is used in a security check at [3]. This means a compromised child process, that belongs to a 'web' application could spoof its aMessage.json.appStatus to 3 [4] and be viewed as a certified app. This bug is to address two things: 1. The parent process does not validate appStatus. 2. The more important question, why are we trusting anything sent from the child in the message? Can't we just do all checking in the parent process without needing strings sent from the child? [1] http://mxr.mozilla.org/mozilla-central/source/dom/apps/src/InterAppCommService.js#823 [2] http://mxr.mozilla.org/mozilla-central/source/dom/apps/src/InterAppCommService.js#312 [3] http://mxr.mozilla.org/mozilla-central/source/dom/apps/src/InterAppCommService.js#351 [4] http://mxr.mozilla.org/mozilla-central/source/caps/idl/nsIPrincipal.idl#174

Yes, indeed. We cannot trust the string sent from the message. Instead, we should need a mechanism to directly retrieve the info of that content process. Something similar to what assertContainApp() does.

A possible solution is just to add a utility to get the app status, similar to appsService.getAppByManifestURL(aManifestURL).installOrigin.

To be clear, this is a defense in depth control so I don't think its sec-critical (you would still need an existing code exec bug before you could exploit this. And we don't use this in existing builds afaik (think the plan is for 1.4) I would argue to have it as a 1.4 blocker though as without it hamstrings our sandbox. but on its own I don't think this is a sec-critical bug. Maybe Im splitting hairs here.

Gene, is this something you'll be able to work on?

Yes, I can take this.

Any progress here?

Sorry... I'm burned out in this recent work week with partners. I'm afraid I have to release the ownership first. If no one can help with this. I'll retake this by myself in the next Wed (sigh...). The solution is supposed to be comment #2. Hi Fabrice, is that possible for you to find someone to enable the appsService to get the app status?

Yep, I can do that.

Thank you so much, Fabrice! Just some side notes to support you as below. If we can get appStatus by something like: appsService.getAppByManifestURL(aManifestURL).appStatus; (not yet supported) then we don't need to pass the appStatus through the IPC [1][2]. Also, we don't need to pass it around in _matchRules(...) and _matchMinimumAccessLevel(...). [1] http://mxr.mozilla.org/mozilla-central/source/dom/apps/src/Webapps.js#489 [2] http://mxr.mozilla.org/mozilla-central/source/dom/apps/src/InterAppCommService.js#455

Renom - we've already shipped with this in a previous release, so we might be able to unblock this for 1.4.

Fabrice, any progress here?

(In reply to Andrew McCreight [:mccr8] from comment #11) > Fabrice, any progress here? Unfortunately not. I'll do that next week

Hi Fabrice, thank you for covering this. I have bandwidth now and I can retake this one if you don't mind. ;)

Hi Fabrice, In this patch I did: 1. We shouldn't rely on the IPC to pass appStatus, which could open a security hole addressing this issue. 2. Instead, we should rely on the appsService.getAppByManifestURL(aManifestURL).appStatus to retrieve that. 3. For symmetry, I don't save the appStatus during registration either. Since we already have app's manifestURL, we can retrieve all the app's info we want, including the appStatus. 4. I also refactor the _matchInstallOrigins(...) a bit since we have to get the application object earlier for _matchMinimumAccessLevel(...).

Comment on attachment 8408741 [details] [diff] [review] Patch Review of attachment 8408741 [details] [diff] [review]: ----------------------------------------------------------------- Thanks Gene!

Wait for try: https://tbpl.mozilla.org/?tree=Try&rev=be2b73fe1305

Just a reminder for myself. We shouldn't expose any info about the security issues in the commit message for landing.

Try looks good. Landing. https://hg.mozilla.org/integration/b2g-inbound/rev/8e54082157d0

Is this disabled on 30?

https://hg.mozilla.org/releases/mozilla-aurora/rev/54c701123c52 https://hg.mozilla.org/releases/mozilla-b2g28_v1_3/rev/cad97e180a42 https://hg.mozilla.org/releases/mozilla-b2g26_v1_2/rev/9a17be2de70f

Isn't this bug a V1.4+ only? I just wonder why we land it to other branches as well? Hope we won't encounter other side-effects in the last minute. I'd appreciate if QA can help verify all the branches are still working well. Testing the System-Tray and Music App interaction should be sufficient because that's our first and main application of IAC.

Just tested V1.2 and V1.3 by myself. V1.2 - There are no apps using IAC yet. V1.3 - The System-Tray/Lock-Screen and Music App interaction looks great. I think all the branches are working well. :)

Marking [qa-] on the desktop side, w/r/t verification. If B2G QA wishes to test in this area, they will do so as they see fit.