Security/Reviews/Gaia/Contacts

From MozillaWiki
< Security‎ | Reviews‎ | Gaia
Jump to: navigation, search

App Review Details

Please see page history for details of previous reviews.

This review does *not* cover the Facebook integration that is also part of the communications/contacts code. See separate review. A separate review focuses on the contacts import/export features of the app: /Import.

Contacts are currenlty being moved to use the DataStore API. Another review may be conducted to focus on this implementation.

Overview

The Contacts app allows you to browse through a list of Contacts, to add, edit and delete each one of them. A contact form allows you to edit several entries such as name, phone numbers, emails, addresses, and comments.

You can synchronize your contacts with Facebook, and import them from Google, Live, Facebook, your SIM card or your SD card.

You can also get a vCard by bluetooth. The Contacts API is based on the vCard file format. It implements the vCArd4/hCard 1.0 specifications. hCard is a microformat allowing a vCard to be embedded inside a HTML page. It identifies vCard properties to CSS class names.

Architecture

The Contact apps is more a module which is part of a bigger app Communications. bug 847417 points out security concerns about this architecture. The use of the new Datastore API would be the first step to deal with it.

Permissions and privacy concerns: see here.

Components

Relevant Source Code

Source code can be found at https://github.com/mozilla-b2g/gaia/tree/master/apps/communications/contacts


OAuth2:

  • oauth2/js/flow.js - the logic to obtain an access token through OAuth2
  • oauth2/js/oauth2.js -

UI elements:

  • elements/confirm.html
  • elements/details.html
  • elements/form.html
  • elements/overlay.html
  • elements/screenshot.html
  • elements/search.html
  • elements/settings.html
  • elements/status.html
  • elements/tag.html

contacts/js/:

  • js/activities.js- handling activities
  • js/contacts_cleaner.js - deleting contacts
  • js/contacts_importer.js - main file to import contacts from external services
  • js/contacts.js - displaying contact info
  • js/contacts_matcher.js
  • js/contacts_matching_controller.js
  • js/contacts_matching_ui.js
  • js/contacts_merger.js
  • js/contacts_shirtcuts.js
  • js/contacts_tag.js
  • js/fixed_header.js
  • js/importer_ui.js - Contacts import UI
  • js/import_init.js
  • js/import_utils.js
  • js/merger_adapter.js
  • js/navigation.js
  • js/service_extension.js - Connects to available external services to import contacts
  • js/sms_integration.js - send a sms from the contact page
  • js/value_selector.js

Facebook specific code:

  • js/fb_loader.js
  • js/fb_resolver.js
  • js/fb/fb_contact.js
  • js/fb/fb_contact_utils.js
  • js/fb/fb_data.js
  • js/fb/fb_init.js
  • js/fb/fb_link_init.js
  • js/fb/fb_link.js
  • js/fb/fb_messaging.js
  • js/fb/fb_query.js
  • js/fb/fb_utils.js
  • js/fb/friends_list.js

export/*: audited in the review of Import/Export Contacts

utilities:

  • js/utilities/binary_search.js
  • js/utilities/config.js
  • js/utilities/confirm.js
  • js/utilities/cookie.js - Cookies for local usage
  • js/utilities/dom.js
  • js/utilities/event_listeners.js
  • js/utilities/extract_params.js - function to extract params in a url separated by ‘&’
  • js/utilities/http_rest.js
  • js/utilities/image_loader.js
  • js/utilities/image_square.js
  • js/utilities/import_from_vcard.js - Read vcard file imported from bluetooth
  • js/utilities/import_sim_contacts.js - Import contacts from the SIM card
  • js/utilities/normalizer.js
  • js/utilities/overlay.js
  • js/utilities/sdcard.js
  • js/utilities/status.js
  • js/utilities/templates.js - HTML template
  • js/utilities/vcard_parser.js - Parse a vCard file (Quoted-Printable)

views:

  • js/views/details.js
  • js/views/form.js - Form to add/update a contact:q
  • js/views/list.js
  • js/views/search.js
  • js/views/settings.js


Permissions

The (communications) app has the following permissions (App permissions are documented on their MDN page):

Certified 

   "attention":{}, 
   "audio-channel-telephony":{},
   "audio-channel-ringer":{},
   "bluetooth":{},
   "idle":{},    
   "mobileconnection":{},
   "settings":{ "access": "readwrite" },
   "telephony":{},
   "time": {},
   "voicemail":{},
   "wifi-manage":{},

Privileged 

   "contacts":{ "access": "readwrite" },
   "device-storage:sdcard": { "access": "readcreate" }
   "systemXHR": {},

Hosted 

   "alarms": {},
   "desktop-notification":{},
   "storage": {},


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

Activities handled by the Contacts app are ‘new’, ‘pick’, ‘import’, ‘open’, and ‘update’. They may take as an argument/parameter the following objects:

  • webcontacts/contact
  • webcontacts/email
  • webcontacts/tel
  • text/vcard

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
   },


The pick activity is used to choose a contact. It simply opens a contact picker and returns a contact, an email or a tel object..

"pick": { 

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

Import a vcard from bluetooth: 

   "import": {
     "filters": {
       "type": "text/vcard"
     },
     "disposition": "inline",
     "href": "/contacts/index.html?open",
     "returnValue": true
   },

Open a contact: 

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

Update a contact object: 

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


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


The last review of the app raised concern about validating the ‘extras’ parameters, still valid :

  • bug 847649 Contacts' "new" activity does not validate parameters
  • bug 847650 Contacts' "new" activity is also "edit" in disguise


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#L23

Post Messages

postMessage is used in different places :

  • In oauth, see the Import/Export contacts review. There are messages between the facebook and the contacts components inside the communications app, so they’re internal messages.
  • Some messages are sent to fb.CONTACTS_APP_ORIGIN where CONTACTS_APP_ORIGIN = 'app://communications.gaiamobile.org’, so they’re for internal use also.


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.

Notable Event Handlers

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 views/details.js where utils.templates.render() is used.

2. Secure Communications

The app talks to Facebook, Google services, Live and BT Yahoo! through the Contacts integration. This is covered in the Facebook code review and the Import/Export feature review.

3. (Secure) data storage

The contacts are stored using the mozContacts API which uses IndexedDB under the covers. The app also uses async_storage, a wrapper around IndexedDB, to store settings and for example the Facebook oauth token.

The mozContacts data is global to every app with contacts permission while the IndexedDB with app specific data is only available to the communications app. (Dialer, FTU, Contacts)

The Facebook Contacts is currenlty being moved to use the Datastore API. 

 "datastores-owned": {
   "Gaia_Facebook_Friends": {
     "description": "Imported Facebook Friends"
   }   
 }


4. Denial of Service

The fields of a contact form are checked against very big chunks of data (it displayed “Infinity” in case of a really big string).

5. Use of Privileged APIs

  • mozContacts - store contacts
  • mozMobileConnection - detect SIM card changes
  • mozTelephony (via TelephonyHelper) - initiate calls
  • mozIccManager - Export/import SIM contacts


6. Interfaces with other Apps/Content

7. Oddities

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.

Security Risks & Mitigating Controls

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 point of view:

  • 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

It is not clear what the intent is of the "new" activity:

  • bug 847650 Contacts' "new" activity is also "edit" in disguise

The "new" activity blindly accepts incoming parameters - no validation

  • bug 847649 Contacts' "new" activity does not validate parameters
Retrieved from "https://wiki.mozilla.org/index.php?title=Security/Reviews/Gaia/Contacts&oldid=766767"