Closed Bug 967653 Opened 6 years ago Closed 6 years ago
[Inter-App Communication API] parent process security checks
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 . 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 , since we have validated aManifestURL (aMessage.json.manifestURL). appStatus is never validated and is used in a security check at . This means a compromised child process, that belongs to a 'web' application could spoof its aMessage.json.appStatus to 3  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?  http://mxr.mozilla.org/mozilla-central/source/dom/apps/src/InterAppCommService.js#823  http://mxr.mozilla.org/mozilla-central/source/dom/apps/src/InterAppCommService.js#312  http://mxr.mozilla.org/mozilla-central/source/dom/apps/src/InterAppCommService.js#351  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.
blocking-b2g: 1.3? → 1.4?
Gene, is this something you'll be able to work on?
Yes, I can take this.
Assignee: nobody → gene.lian
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?
Assignee: gene.lian → nobody
Yep, I can do that.
Assignee: nobody → fabrice
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 . Also, we don't need to pass it around in _matchRules(...) and _matchMinimumAccessLevel(...).  http://mxr.mozilla.org/mozilla-central/source/dom/apps/src/Webapps.js#489  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.
blocking-b2g: 1.4+ → 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. ;)
Assignee: fabrice → gene.lian
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!
Attachment #8408741 - Flags: review?(fabrice) → review+
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?
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.
You need to log in before you can comment on or make changes to this bug.