Closed Bug 967653 Opened 6 years ago Closed 6 years ago

[Inter-App Communication API] parent process security checks

Categories

(Core Graveyard :: DOM: Apps, defect)

defect
Not set

Tracking

(blocking-b2g:1.4+, firefox27 disabled, firefox28 disabled, firefox29 disabled, firefox30+ fixed, firefox31 fixed, firefox-esr24 unaffected, b2g18 unaffected, b2g-v1.1hd unaffected, b2g-v1.2 fixed, b2g-v1.3 fixed, b2g-v1.3T fixed, b2g-v1.4 fixed, b2g-v2.0 fixed)

RESOLVED FIXED
mozilla31
blocking-b2g 1.4+
Tracking Status
firefox27 --- disabled
firefox28 --- disabled
firefox29 --- disabled
firefox30 + fixed
firefox31 --- fixed
firefox-esr24 --- unaffected
b2g18 --- unaffected
b2g-v1.1hd --- unaffected
b2g-v1.2 --- fixed
b2g-v1.3 --- fixed
b2g-v1.3T --- fixed
b2g-v1.4 --- fixed
b2g-v2.0 --- fixed

People

(Reporter: rfletcher, Assigned: airpingu)

References

Details

(Keywords: csectype-priv-escalation, sec-high, Whiteboard: [qa-])

Attachments

(1 file, 1 obsolete file)

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
Depends on: 967104
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.
blocking-b2g: --- → 1.3?
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?
Flags: needinfo?(gene.lian)
Keywords: sec-criticalsec-high
Yes, I can take this.
Assignee: nobody → gene.lian
Flags: needinfo?(gene.lian)
blocking-b2g: 1.4? → 1.4+
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
Flags: needinfo?(fabrice)
Yep, I can do that.
Assignee: nobody → fabrice
Flags: needinfo?(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 [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.
blocking-b2g: 1.4+ → 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
Attached patch Patch (WIP) (obsolete) — Splinter Review
Attached patch PatchSplinter Review
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(...).
Attachment #8408730 - Attachment is obsolete: true
Attachment #8408741 - Flags: review?(fabrice)
Comment on attachment 8408741 [details] [diff] [review]
Patch

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

Thanks Gene!
Attachment #8408741 - Flags: review?(fabrice) → review+
Just a reminder for myself. We shouldn't expose any info about the security issues in the commit message for landing.
Is this disabled on 30?
https://hg.mozilla.org/mozilla-central/rev/8e54082157d0
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla31
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.
Whiteboard: [qa-]
Group: core-security
Product: Core → Core Graveyard
You need to log in before you can comment on or make changes to this bug.