MozillaWiki

Security/Reviews/Gaia/Contacts: Difference between revisions

Page Discussion

Security/Reviews/Gaia/Contacts (view source)

Revision as of 15:16, 19 November 2013

3,414 bytes added ,  19 November 2013
new sec review
(new sec review)
Line 2: Line 2:


* App: Contacts, which is part of the Communications application
* App: Contacts, which is part of the Communications application
* Review Date: March 4th, 2013
* Review Date: November 19th, 2013
* Review Lead: Stefan Arentz
* Review Lead: Stéphanie Ouillon
* Review Bug: {{bug|754738}} [Security Review] B2G Gaia - Communications / Contacts
* Branch reviewed: master
* Dependency Tree: https://bugzilla.mozilla.org/showdependencytree.cgi?id=754738&hide_resolved=0
* Latest commit: https://github.com/mozilla-b2g/gaia/commit/6b8572b10572059a359ff0fc68a0e0b83eda250b
 
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 [https://wiki.mozilla.org/Security/Reviews/Gaia/FacebookIntegration review].
Also, a separate review focuses on the contacts import/export features of the app: [url to add]


This review does *not* cover the Facebook integration that is also part of the communications/contacts code. See separate review.


=== Overview ===
=== 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===
===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 [https://wiki.mozilla.org/WebAPI/DataStore Datastore API] would be the first step to deal with it.
Permissions and privacy concerns: see [https://wiki.mozilla.org/WebAPI/ContactsAPI#permissions_and_privacy here].


====Components====
====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.
====Relevant Source Code====
 
Source code can be found at https://github.com/mozilla-b2g/gaia/tree/master/apps/communications/contacts
 


ISSUE: Contacts should be separated from contacts and maybe even facebook
OAuth2:
* oauth2/js/flow.js - the logic to obtain an access token through OAuth2
* oauth2/js/oauth2.js -


* {{bug|847417}} Contacts should be turned into minimal standalone application
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


====Relevant Source Code====
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)


Source code can be found at https://github.com/mozilla-b2g/gaia/tree/v1-train/apps/communications/contacts
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


Reviewed code:


* contacts/js/activities.js
* contacts/js/confirm_dialog.js
* contacts/js/contacts.js
* contacts/js/contacts_details.js
* contacts/js/contacts_form.js
* contacts/js/contacts_list.js
* contacts/js/contacts_settings.js
* contacts/js/contacts_shortcuts.js
* contacts/js/fb_extensions.js
* contacts/js/fb_loader.js
* contacts/js/fixed_header.js
* contacts/js/navigation.js
* contacts/js/search.js
* contacts/js/sms_integration.js
* contacts/js/value_selector.js
* contacts/js/utilities/binary_search.js
* contacts/js/utilities/config.js
* contacts/js/utilities/event_listeners.js
* contacts/js/utilities/image_loader.js
* contacts/js/utilities/image_square.js
* contacts/js/utilities/import_sim_contacts.js
* contacts/js/utilities/normalizer.js
* contacts/js/utilities/overlay.js
* contacts/js/utilities/responsive.js
* contacts/js/utilities/status.js
* contacts/js/utilities/templates.js


====Permissions====
====Permissions====


The (communications) app has the following permissions:
The (communications) app has the following permissions (App permissions are documented on [https://developer.mozilla.org/en-US/Apps/Developing/App_permissions?redirectlocale=en-US&redirectslug=Web%2FApps%2FApp_permissions 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": {},


*  "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.
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 ====
====Web Activity Handlers ====
Line 81: Line 153:
All activities are handled in https://github.com/mozilla-b2g/gaia/blob/v1-train/apps/communications/contacts/js/activities.js
All activities are handled in https://github.com/mozilla-b2g/gaia/blob/v1-train/apps/communications/contacts/js/activities.js


===== new =====
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.
The new activity is used to create a new contact. It opens the contact editor screen.
Line 94: Line 170:
     },
     },


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


ISSUE: 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. 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.
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..


* {{bug|847650}} Contacts' "new" activity is also "edit" in disguise
"pick": {
* {{bug|847649}} Contacts' "new" activity does not validate parameters
      "filters": {
        "type": {
          "required": true,
          "value": ["webcontacts/contact","webcontacts/email","webcontacts/tel"]
        }
      },
      "disposition": "inline",
      "href": "/contacts/index.html?pick",
      "returnValue": true
    },


===== pick =====
Import a vcard from bluetooth:
    "import": {
      "filters": {
        "type": "text/vcard"
      },
      "disposition": "inline",
      "href": "/contacts/index.html?open",
      "returnValue": true
    },


The pick activity is used to choose a contact. It simply opens a contact picker and returns a contact object.
Open a contact:
    "open": {
      "filters": {
        "type": "webcontacts/contact"
      },
      "disposition": "inline",
      "href": "/contacts/index.html?open",
      "returnValue": true
    },


    "pick": {
Update a contact object:
      "update": {
       "filters": {
       "filters": {
         "type": ["webcontacts/contact","webcontacts/email"]
         "type": "webcontacts/contact"
      },
      },
       "disposition": "inline",
       "disposition": "inline",
       "href": "/contacts/index.html?pick",
       "href": "/contacts/index.html?update",
       "returnValue": true
       "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 ====
==== System Messages ====
Line 125: Line 235:
     { "telephony-new-call": "/dialer/index.html#keyboard-view" },
     { "telephony-new-call": "/dialer/index.html#keyboard-view" },
     { "ussd-received": "/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.
The contacts app does not handle any of these messages. They are all for the Dialer and Facebook integration.
Line 133: Line 244:
The <code>alarm</code> message is used by the facebook integration code to schedule periodic updates of the Facebook contact data.
The <code>alarm</code> 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
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 ====
==== Post Messages ====


The contacts app does not use <code>postMessage()</code>
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 ====
====Web Activity Usage ====
Line 159: Line 273:
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.
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 <code>utils.templates.render()</code> is used.
There a few of these cases in views/details.js where <code>utils.templates.render()</code> is used.


====2. Secure Communications ====
====2. Secure Communications ====


The app only talks to Facebook through the Facebook Contacts integration. This is covered in the Facebook code review.
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 ====
====3. (Secure) data storage ====
Line 170: Line 284:


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 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 being moved to use the Datastore API. This is to be covered in the next Facebook integration review.


====4. Denial of Service ====
====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 ====
====5. Use of Privileged APIs ====


* mozContacts
* mozContacts - store contacts
* mozMobileConnection
* mozMobileConnection - detect SIM card changes
* mozTelephony (via TelephonyHelper)
* mozTelephony (via TelephonyHelper) - initiate calls
 
* mozIccManager - Export/import SIM contacts
The use of <code>mozContacts</code> is obvious.
 
The use of <code>mozMobileConnection</code> is to detect SIM card changes.


The use of mozTelephony is to initiate calls.


====6. Interfaces with other Apps/Content====
====6. Interfaces with other Apps/Content====
Line 201: Line 315:
* {{bug|841071}}  Settings are globally shared between applications
* {{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:
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
* {{bug|847417}} Contacts should be turned into minimal standalone application
Arroway
Confirmed users
152

edits

Retrieved from "https://wiki.allizom.org/Special:MobileDiff/765403"