777602 - Audit message manager protocols

Status: RESOLVED FIXED

(Core :: IPC, defect, P1)

Comment 1

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

Note bug 777188 and bug 776825, which are basically prereqs for this.

Comment 2

[Andreas Gal :gal]

Renom if you think we can't ship a v1 without this.

Comment 3

[Jason Smith [:jsmith]]

Per IRC conversations with a few other folks, I think the best course of action if there is disagreement on whether this blocks or not is to do the following:

- Move the blocking-basecamp flag to ? for re-evaluation
- Indicate a rationale for why you disagree

Comment 4

[Andrew Overholt [:overholt]]

Olli, is this something you can do?

Comment 5

[Olli Pettay [:smaug][[e-mail]]]

Some of this is done in https://bug776652.bugzilla.mozilla.org/attachment.cgi?id=651156

Comment 6

[Doug Turner (:dougt)]

smaug, are we done with this task?  what are the next steps?

Comment 7

[Olli Pettay [:smaug][[e-mail]]]

I don't know if I can do anything more. More messageManager usage have been added, I suppose, 
and now that we have the permission manager and new API (did it land already?) in message manager
to assert permissions, we should "just" add the necessary checks.
I believe the owner of the each component which uses mm is the right person to add permission asserts.

Comment 8

[Alex Keybl [:akeybl]]

Assigning to Lucas to find somebody to resolve this bug. Giving this a P1 priority since we don't know what issues are lying behind this bug.

Comment 9

[Andrew Overholt [:overholt]]

Paul, Lucas said you'd love to do this :)  If you can't take it on, just un-assign yourself.  Thanks!

Comment 10

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

Note that what we really need here is penn-testing. We should assume that all the data that's coming in to the message manager is hostile data and then see where that data flows to see if it can be used to hack other processes.

Comment 11

[Andrew Overholt [:overholt]]

Have you had a chance to look at this yet, Paul?

Comment 12

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

Andrew, just looking at this now. I don't think I will have the bandwidth to do this, but I will try to recruit help from the security assurance team.

Comment 13

[Alex Keybl [:akeybl]]

Marking for C2, given this meets the criteria of known P1/P2 bugs at the end of C1.

Comment 14

[Dietrich Ayala (:dietrich)]

Paul, Daniel, Curtis: What's the ETA for this?

Comment 15

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

This is the top of my list after 815105. I should have it completed by the end of the week.

Comment 16

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

I think I misinterpreted this bug from earlier comments - I thought the goal was audit permission checks based on comment 5 above. But I see that there is already bug 776834  (Apply security checks to sensitive DOM APIs) and bug 776652 (Apply appropriate security checks for sensitive IPDL protocols)

So I am trying to work out what the threat scenario is here: is it a compromised child process (e.g. chrome JavaScript execution?) that then seeks to escalate privileges via messages? I.e. APIs which have security checks or data validation only in the child not the parent, or where the parent blindly trusts that data received?

As an example, webapps.js checks the app type (installed/privileged/certified) requested prior to install, and only allows web unless the device in developer mode. A spurious message could just request 'certified' and as far as I can tell, this isnt chekced in the parent webapps.jsm (http://mxr.mozilla.org/mozilla-central/source/dom/apps/src/Webapps.jsm#677) So, with user interaction maybe a higher privileged app could be installed. Or maybe a directory name control with the app id parameter (used here http://mxr.mozilla.org/mozilla-central/source/dom/apps/src/Webapps.jsm#1160)

Am I on the right path? Would this sort of issue be basecamp blocker? (I feel like I should be prioritizing reviews of APIs exposed to content and work on the permissions API rather than defence in depth at this point). 

Not sure if I am understanding this bug correctly though - if someone could spare 20 mins Monday afternoon to give me a bit of a grounding in message managers that would be helpful.

Comment 17

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

(In reply to Paul Theriault [:pauljt] from comment #16)
> I think I misinterpreted this bug from earlier comments - I thought the goal
> was audit permission checks based on comment 5 above. But I see that there
> is already bug 776834  (Apply security checks to sensitive DOM APIs) and bug
> 776652 (Apply appropriate security checks for sensitive IPDL protocols)
> 
> So I am trying to work out what the threat scenario is here: is it a
> compromised child process (e.g. chrome JavaScript execution?) that then
> seeks to escalate privileges via messages?

Exactly. The threat is an app which uses javascript to create a buffer overflow or similar to break the javascript sandbox and execute binary code directly against the CPU.

B2G uses a sandbox which prevents on an OS level child processes from accessing almost any data, other than that it can use IPDL or message manager to send messages to the parent process. These messages could have side effects or return sensitive data.

Hence we should in all cases assume that the child process has been hacked and not trust any data that it sends us. I.e. we should only allow child processes to take actions that we want to allow the apps in those child processes to take.

> I.e. APIs which have security
> checks or data validation only in the child not the parent, or where the
> parent blindly trusts that data received?

Exactly. For APIs that we only have security checks in the child process, we effectively have no security checks at all if the child process is hacked.

Now, this might not be a huge deal. If we leak some non-sensitive data in this scenario it's something we can live with for the v1 release. For example if a hacker can fingerprint the user after hacking a child process, for example by enumerating the set of apps installed, it might not be a blocker.

But it's something that we'd want to know about.

> As an example, webapps.js checks the app type
> (installed/privileged/certified) requested prior to install, and only allows
> web unless the device in developer mode. A spurious message could just
> request 'certified' and as far as I can tell, this isnt chekced in the
> parent webapps.jsm
> (http://mxr.mozilla.org/mozilla-central/source/dom/apps/src/Webapps.jsm#677)

Isn't this code run in the parent process?

> So, with user interaction maybe a higher privileged app could be installed.
> Or maybe a directory name control with the app id parameter (used here
> http://mxr.mozilla.org/mozilla-central/source/dom/apps/src/Webapps.jsm#1160)

If these checks indeed happen in the child process, then that's a bad thing.

> Am I on the right path? Would this sort of issue be basecamp blocker? (I
> feel like I should be prioritizing reviews of APIs exposed to content and
> work on the permissions API rather than defence in depth at this point). 
> 
> Not sure if I am understanding this bug correctly though - if someone could
> spare 20 mins Monday afternoon to give me a bit of a grounding in message
> managers that would be helpful.

It sounds like you are on the right path indeed.

Comment 18

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

(In reply to Jonas Sicking (:sicking) from comment #17)
> (In reply to Paul Theriault [:pauljt] from comment #16)
> > I think I misinterpreted this bug from earlier comments - I thought the goal
> > was audit permission checks based on comment 5 above. But I see that there
> > is already bug 776834  (Apply security checks to sensitive DOM APIs) and bug
> > 776652 (Apply appropriate security checks for sensitive IPDL protocols)
> > 
> > So I am trying to work out what the threat scenario is here: is it a
> > compromised child process (e.g. chrome JavaScript execution?) that then
> > seeks to escalate privileges via messages?
> 
> Exactly. The threat is an app which uses javascript to create a buffer
> overflow or similar to break the javascript sandbox and execute binary code
> directly against the CPU.
> 
> B2G uses a sandbox which prevents on an OS level child processes from
> accessing almost any data, other than that it can use IPDL or message
> manager to send messages to the parent process. These messages could have
> side effects or return sensitive data.
> 
> Hence we should in all cases assume that the child process has been hacked
> and not trust any data that it sends us. I.e. we should only allow child
> processes to take actions that we want to allow the apps in those child
> processes to take.
> 
> > I.e. APIs which have security
> > checks or data validation only in the child not the parent, or where the
> > parent blindly trusts that data received?
> 
> Exactly. For APIs that we only have security checks in the child process, we
> effectively have no security checks at all if the child process is hacked.
> 
> Now, this might not be a huge deal. If we leak some non-sensitive data in
> this scenario it's something we can live with for the v1 release. For
> example if a hacker can fingerprint the user after hacking a child process,
> for example by enumerating the set of apps installed, it might not be a
> blocker.
> 
> But it's something that we'd want to know about.
> 
> > As an example, webapps.js checks the app type
> > (installed/privileged/certified) requested prior to install, and only allows
> > web unless the device in developer mode. A spurious message could just
> > request 'certified' and as far as I can tell, this isnt chekced in the
> > parent webapps.jsm
> > (http://mxr.mozilla.org/mozilla-central/source/dom/apps/src/Webapps.jsm#677)
> 
> Isn't this code run in the parent process?

I meant the checks were in webapps.js, and not the webapps.jsm. Webapps.jsm seems to run in both parent and child, since it has ppmm and cpmm, but for the install flow it seems to be the parent, hence the checks dont seem to be in the parent where they belong. Thats what I meant.

> 
> > So, with user interaction maybe a higher privileged app could be installed.
> > Or maybe a directory name control with the app id parameter (used here
> > http://mxr.mozilla.org/mozilla-central/source/dom/apps/src/Webapps.jsm#1160)
> 
> If these checks indeed happen in the child process, then that's a bad thing.
> 
> > Am I on the right path? Would this sort of issue be basecamp blocker? (I
> > feel like I should be prioritizing reviews of APIs exposed to content and
> > work on the permissions API rather than defence in depth at this point). 
> > 
> > Not sure if I am understanding this bug correctly though - if someone could
> > spare 20 mins Monday afternoon to give me a bit of a grounding in message
> > managers that would be helpful.
> 
> It sounds like you are on the right path indeed.

Comment 19

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

Raised 820202 to track the issues identified as part of this review. At a guess I am about 50% way through reviewing these messages.

Comment 21

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

Attachment: Notes v2.

I have finished a first pass through all of the APIs. The only ones I haven't looked deeply at are the RIL:* messages, since they have permissions checks and that is the main protection in this API. Making attachment public since the bugs raised are public, and these are not directly exploitable security vulnerabilities.

Comment 22

[Alex Keybl [:akeybl]]

(In reply to Paul Theriault [:pauljt] from comment #21)
> Created attachment 692264 [details]
> Notes v2.
> 
> I have finished a first pass through all of the APIs. The only ones I
> haven't looked deeply at are the RIL:* messages, since they have permissions
> checks and that is the main protection in this API. Making attachment public
> since the bugs raised are public, and these are not directly exploitable
> security vulnerabilities.

Can we call this b-b+ meta bug resolved in that case? All mentioned bugs that we feel are needed for v1 should be b-b+ instead of this bug.

Comment 23

[Lucas Adamski [:ladamski]]

(In reply to Alex Keybl [:akeybl] from comment #22)
> 
> Can we call this b-b+ meta bug resolved in that case? All mentioned bugs
> that we feel are needed for v1 should be b-b+ instead of this bug.

I'd be comfortable resolving this as such unless Paul feels like there's major work still outstanding.

Comment 24

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

Yes I think this is fine to close. I raised any issues or question under 820202 and the children of this bug should be reviewed on a case by case basis for blocking basecamp (although as I commented above, all of this is defence in depth controls, lower priority than directly exposed API security). 

Note that if it isn't in the spreadsheet, I didnt look at it - ie for any new system messages we should keep an eye on.

Comment 25

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

Paul: We shouldn't audit just the implementations of the APIs listed in the permissions matrix. We need to audit every user of the message manager. It's quite likely that there are non-B2G-specific pieces of code that uses the message manager and do harmful things in the parent process since most non-B2G code did not consider the child process as untrusted.

I.e. the message manager was originally written for e10s desktop Firefox and e10s XUL fennec. Neither of which was running the child process in a sandbox.

Comment 26

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

The approach I used was to search for all files which have the string |parentprocessmessagemanager| to match the contractid, ie
https://mxr.mozilla.org/mozilla-central/search?string=parentprocessmessagemanager

I then went through each of these files and looked for where .addMessageListener was called to come up with a list message that would be listened for in the parent process. Does this sound like a comprehensive list of messages (again see the spreadsheet for detail).

This included two places in Fennec:
/mobile/xul/chrome/content/CapturePickerUI.js
/mobile/xul/chrome/content/WebappsUI.js

I didn't think these would be present in B2G so I hadn't had a strong look at them yet. I will do that now.


(reopened for now, close when sure this is comprehensive.)

Comment 27

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

More info:
/mobile/xul/chrome/content/CapturePickerUI.js

This just load chrome://browser/content/CaptureDialog.xul which I dont think is present on B2G. In any case, only the title appears to be used, and it looks safe to me.

/mobile/xul/chrome/content/WebappsUI.js
This file references OpenWebapps.jsm which doesn't seem to be in the tree any more following bug 697383. If it IS still present this has the similar issues to bug 820206.

Comment 28

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

Yup, that sounds perfect. Sirry to cause a stir.

Comment 29

[David Chan [:dchan]]

Removing myself from the sec-review flag. I think Paul finished this though

Comment 30

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

This was complete last year as above. I didn't close this since the dependent bugs were closed, but we can probably just keep 820202 open to track that work, so I am closing this.