Security/Reviews/Gaia/Contacts: Difference between revisions
(Created page with "=== App Review Details === * App: Contacts, which is part of the Communications application * Review Date: March 4th, 2013 * Review Lead: Stefan Arentz * Review Bug: {{bug|75473...") |
No edit summary |
||
| Line 87: | Line 87: | ||
==== Post Messages ==== | ==== Post Messages ==== | ||
ISSUE: None of the handlers verify that the message originated from a trusted/expected source. | ISSUE: None of the handlers verify that the message originated from a trusted/expected source. | ||
| Line 111: | Line 97: | ||
====Web Activity Usage ==== | ====Web Activity Usage ==== | ||
==== Notable Event Handlers ==== | ==== Notable Event Handlers ==== | ||
| Line 119: | Line 103: | ||
====1. XSS & HTML Injection attacks==== | ====1. XSS & HTML Injection attacks==== | ||
====2. Secure Communications ==== | ====2. Secure Communications ==== | ||
===== Remote Services ===== | ===== Remote Services ===== | ||
===== BlueTooth ===== | ===== BlueTooth ===== | ||
====3. (Secure) data storage ==== | ====3. (Secure) data storage ==== | ||
====4. Denial of Service ==== | ====4. Denial of Service ==== | ||
====5. Use of Privileged APIs ==== | ====5. Use of Privileged APIs ==== | ||
====6. Interfaces with other Apps/Content==== | ====6. Interfaces with other Apps/Content==== | ||
==== 7. Oddities ==== | ==== 7. Oddities ==== | ||
==== 8. Input Validation Problems ==== | ==== 8. Input Validation Problems ==== | ||
=== Security Risks & Mitigating Controls === | === Security Risks & Mitigating Controls === | ||
| Line 176: | Line 130: | ||
* {{bug|841071}} Settings are globally shared between applications | * {{bug|841071}} Settings are globally shared between applications | ||
The | The contacts app is embedded in a bigger app, which is not great from a security pov: | ||
* {{bug| | * {{bug|847417}} Contacts should be turned into minimal standalone application | ||
Revision as of 16:33, 4 March 2013
App Review Details
- App: Contacts, which is part of the Communications application
- Review Date: March 4th, 2013
- Review Lead: Stefan Arentz
- Review Bug: bug 754738 [Security Review] B2G Gaia - Communications / Contacts
- Dependency Tree: https://bugzilla.mozilla.org/showdependencytree.cgi?id=754738&hide_resolved=0
This review also covers the Facebook integration because that is mostly integrated in the Contacts application.
Overview
Architecture
Components
The contacts app is part of a the communications application. This is an umbrella app that also contains the dialer app, the connect-to-facebook app and the first-time-setup app. I say 'app' here, but they are really just pages as part of the single communications app.
This is not great from a security point of view and should ideally be fixed in a future version. The best scenario is that these are three different applications. If they have to talk to eachother they should use WebActivities. This greatly reduces the attack surface.
ISSUE: Contacts should be separated from contacts and maybe even facebook
- bug 847417 Contacts should be turned into minimal standalone application
Relevant Source Code
Source code can be found at https://github.com/mozilla-b2g/gaia/tree/v1-train/apps/communications/contacts
TODO List relevant files
Permissions
The (communications) app has the following permissions:
* "telephony":{},
* "voicemail":{},
* "contacts":{ "access": "readwrite" },
* "mobileconnection":{},
* "attention":{},
* "settings":{ "access": "readwrite" },
* "desktop-notification":{},
* "alarms": {},
* "systemXHR": {},
* "wifi-manage":{},
* "time": {},
* "audio-channel-telephony":{},
* "audio-channel-ringer":{},
* "browser":{}
ISSUE: Because the communications app is an 'umbrella app' for dialer, contacts, first-time setup and some generic facebook code, it has more permissions than it needs to have. Ideally we separate the three applications in the communications app into three separate apps.
Web Activity Handlers
new
TODO
update
TODO
pick
TODO
System Messages
The (communications) app listens to the following system messages:
"messages": [
{ "alarm": "/facebook/fb_sync.html" },
{ "bluetooth-dialer-command": "/dialer/index.html#keyboard-view" },
{ "headset-button": "/dialer/index.html#keyboard-view" },
{ "notification": "/dialer/index.html#keyboard-view" },
{ "telephony-new-call": "/dialer/index.html#keyboard-view" },
{ "ussd-received": "/dialer/index.html#keyboard-view" }
]
The contacts app does not handle any of these messages. They are all for the Dialer and Facebook integration.
alarm
The alarm message is used by the facebook integration code to schedule periodic updates of the Facebook contact data.
The code for this can be found in https://github.com/mozilla-b2g/gaia/blob/master/apps/communications/facebook/js/fb_sync_init.js#L22
Post Messages
ISSUE: None of the handlers verify that the message originated from a trusted/expected source.
I was able to exploit this and let remote content post messages to the Dialer to trigger Missed Calls notifications to appear.
ACTION: Add strict checking of event sources as described on MDN at https://developer.mozilla.org/en-US/docs/DOM/window.postMessage#Security_concerns
- bug 845487 Dialer responds to cross-origin messages without verifying the source (exploitable)
Web Activity Usage
Notable Event Handlers
Code Review Notes
1. XSS & HTML Injection attacks
2. Secure Communications
Remote Services
BlueTooth
3. (Secure) data storage
4. Denial of Service
5. Use of Privileged APIs
6. Interfaces with other Apps/Content
7. Oddities
8. Input Validation Problems
Security Risks & Mitigating Controls
Actions & Recommendations
The dialer unnecessarily has access to all system settings. This is an issue with the Settings API that should be improved in a future version of Firefox OS:
- bug 841071 Settings are globally shared between applications
The contacts app is embedded in a bigger app, which is not great from a security pov:
- bug 847417 Contacts should be turned into minimal standalone application