Security/Reviews/Gaia/Contacts

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 does *not* cover the Facebook integration that is also part of the communications/contacts code. See separate review.

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

All activities are handled in https://github.com/mozilla-b2g/gaia/blob/v1-train/apps/communications/contacts/js/activities.js

new

The new activity is used to create a new contact. It opens the contact editor screen.

   "new": {
     "filters": {
       "type": "webcontacts/contact"
     },
     "disposition": "inline",
     "href": "/contacts/index.html?new",
     "returnValue": true
   },

TODO The activity handler copies all parameters from activity.source.params to the request params. Is it possible to cause anything bad there? It seems to pass an 'id' and 'extras' parameter.

The actual contact editor is shown at https://github.com/mozilla-b2g/gaia/blob/v1-train/apps/communications/contacts/js/contacts.js#L68

If an id was passed as a request param then addExtrasToContact is called to process the extras param which is a JSON encoded object. The fields of the extras object are added to the existing contact.

TODO: It seems that it is possible to add any kind of field to a contact? What kind of side effects can this have?

TODO: How to obtain contact ids? Are they easily guessable?

pick

The pick activity is used to choose a contact. It simply opens a contact picker and returns ... TODO

   "pick": {
     "filters": {
       "type": ["webcontacts/contact","webcontacts/email"]
      },
     "disposition": "inline",
     "href": "/contacts/index.html?pick",
     "returnValue": true
   },

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

The contacts app does not use postMessage()

Web Activity Usage

The contacts app uses MozActivity for the following:

• Create a new email when you tap an email address
• Pick an image for the contact sheet
• Create a new websms/sms when to send an sms from a contact sheer

None of these do anything interesting except passing an email or phonenumber to the activity or grabbing a resulting image blob.

Code Review Notes

1. XSS & HTML Injection attacks

The app is pretty good at using templates and escaping html. There are however some cases where content is not escaped. These are all cases where the input is expected to be a number or a keyword with no possible side effects.

I recommend against using shortcuts like this. It is usually fine but it is better to escape as a rule than as an exception as experience shows that the best XSS exploits are when those 'known values' can somehow be modified in an unexpected way in an unexpected location.

There a few of these cases in contacts_details.js where utils.templates.render() is used.

3. (Secure) data storage

The contacts are stored using async_storage.js which is a wrapper around IndexedDB. The contacts data is therefore only available to the communications app. That does include the dialer and first time setup since those are the same app/domain.

5. Use of Privileged APIs

• mozContacts
• mozMobileConnection
• mozTelephony (via TelephonyHelper)

8. Input Validation Problems

No input validation is done in the activity handlers. Specifically the blind copying of data from the new activity to the page request params is troublesome as it looks like this is purely for internal usage.

Actions & Recommendations

The contacts app 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

Usage of innerHTML:

• bug 828751 Favor use of createElement and textContent instead of innerHTML in Gaia:Contacts