Security/Reviews/Gaia/sms: Difference between revisions

From MozillaWiki
< Security‎ | Reviews‎ | Gaia
Jump to navigation Jump to search
No edit summary
m (update link to previous review notes)
 
(8 intermediate revisions by 2 users not shown)
Line 1: Line 1:
=== App Review Details ===
=== App Review Details ===
* App: SMS
* App: SMS
* Review Date: 4th June 2013
* Review Date: 3rd March 2014
* Review Lead: Paul Theriault
* Latest Commit: https://github.com/mozilla-b2g/gaia/commit/db3b537b4a29be4849f45fb7ec27b77bb778a2a6
* Branch Reviewed : Master
* Reviewers : Stéphanie Ouillon & Paul Theriault


=== Overview ===
=== Overview ===
SMS app provides the ability to send and recieve SMS messages.
SMS app provides the ability to send and receive SMS/MMS messages. SMS was reviewed previously in jun 2013.  


The main focus of this re-review was to include the addition of subject support in MMS ({{bug|919966}}) and delivery report support ({{bug|919977}}). The prior review can be found [https://wiki.mozilla.org/index.php?title=Security/Reviews/Gaia/sms&oldid=703745 here].


===Architecture===


====Components====
====Components====
SMS app is structed as a single HTML page. This page displays the list of SMS messages, UI for composing sms, viewing threads, deleting messages etc. There is also web activity handlers so that other apps can ask the SMS app to send an SMS.
SMS app is structed as a single HTML page. This page displays the list of SMS messages, UI for composing sms,  
viewing threads, deleting messages etc.


Inputs to the SMS app: SMS events from system, web activity requests, data from contacts, user input
A message is composed of a phone number and optional body, contact and threadId.
There is also web activity handlers so that other apps can ask the SMS app to send a message.
 
Inputs to the SMS app: SMS events from system, web activity requests, data from contacts, user input to write the SMS.
 
At startup, 4 components are initialized:
* ActivityHandler: application's global Activity handler to delegate to specific handler based on the Activity name; action when receiving a sms/mms.
* ThreadUI: UI actions (listeners, view initialization, message templating...)
* MessageManager: message actions (send, receive...)
* Settings: set the default mms message size limitation


Note that navigation in the app is performed in some cases via modify


====Relevant Source Code====
====Relevant Source Code====
Source code is viewable here: http://mxr.mozilla.org/gaia/source/apps/sms/js/
This review based on the snapshot of the v1-train branch:
v1-train      055ff9c Merge pull request #8773 from davidflanagan/bug847060
v1.0.1 was also examined during this review.


====Permissions====
UI:
The sms app reuires the following permisisons:
* action_menu.js
* attachment_menu.js
* compose.js - handle UI specifics of message composition
* dialog.js - generic confirm screen
* fixed_header.js
* information.js - Information view (delivery report)
* thread_list_ui.js - track current number of rendered threads
* thread_ui.js - manage threads
* waiting_screen.js
 
Actions:
* activity_handler.js - main activity handler
* activity_picker.js - to perform actions such as calling/adding/editing a contact, sending a sms/mms/email
* attachments.js - create an attachment object (media source and text)
* contacts.js - find/filter contacts
* blacklist.js - get the blacklisted numbers
* link_action_helper.js - link url/email/phone data in a message
* message_manager.js - send/received messages
* notify.js - notification on incoming message
* recipients.js - manage recipients list
* smil.js - handle SMIL document (layout of MMS message)
* startup.js
 
Settings:
* settings - default MMS size limitation if no operator specification
 
Util:
* link_helper.js - replace input strings by anchor links for url, phone, email
* utils.js - time, contact details, font size, carrier tag, etc.
* wbmp.js - decode the Wireless Bitmap image format (.wbmp is Wireless Application Protocol Bitmap Format)
 
 
Shared code:
* shared/js/l10n.js
* shared/js/lazy_loader.js
 
The *_mock.js files are not reviewed since they are test files.
 
 
===Permissions===
The sms app requires the following permissions:
 
   "permissions": {
   "permissions": {
     "sms":{},
     "sms":{},
    "storage":{},
    "mobileconnection": {},
     "contacts":{ "access": "readonly" },
     "contacts":{ "access": "readonly" },
    "mobileconnection":{},
     "settings":{ "access": "readonly" },
     "settings":{ "access": "readonly" },
     "audio-channel-notification":{},
     "audio-channel-notification":{},
     "desktop-notification":{}
     "desktop-notification":{},
   },
    "phonenumberservice":{}
   }


The SMS app does not currently appear to use mobileconnection API. This should be removed if not in use.
There are two new permissions:
* mobileconnection: used in settings.js for listening to mobile connection datachange before switching between SIMs .
* phonenumberservice: used for  normalizing and comparing phone numbers


====Web Activity Handlers ====
====Web Activity Handlers ====
The SMS app handles one activity:
The SMS app handles two Activities, defined in activity_handler.js:


   "activities": {
   "activities": {
Line 42: Line 96:
       "filters": {
       "filters": {
         "type": "websms/sms",
         "type": "websms/sms",
         "number": { "regexp":"/^[\\w\\s+#*().-]{0,50}$/" }
         "number": {
          "pattern":"[\\w\\s+#*().-]{0,50}"
        }
       },
       },
       "disposition": "window"
       "disposition": "window"
    },
    "share": {
      "filters": {
        "type": ["image/*", "audio/*", "video/*"],
        "number": 1
      },
      "disposition": "window",
      "returnValue": true
     }
     }
   }
   }
* new - to create a new message and pre-populate message details (phone number, body, contact).
* share - to allow to send a media in attachment via a SMS/MMS, launching the sms app from another app.


====Web Activity Usage ====
====Web Activity Usage ====
The SMS initiates outgoing activities for links. A pick activity is also used to retrieve a contact from the address book (or any app that provides the right activity).


* blacklist.js gets the blacklist file by an XHR request to 'js/backlist.json'.
* ActivityPicker can make calls to several activities with web usage: new (mail), view (url) 
* A pick Activity (with type ['image/*', 'audio/*', 'video/*']) is used to allow the user to create an attachment.
* A pick Activity (with type webontacts/contacts) is used to retrieve a Contact from the adress book.


==== Notable Event Handlers ====
==== Notable Event Handlers ====
window.addEventListener('hashchange'


If you could cause the sms app to navigate somehow, then this might be an issue, since hashchange causes this to happen in the UI. But you can't frame or link to app:// so I dont think this is an issue.
The 'hashchange' event handler is used in threads.js and activity_handler.js.
 
==== Datastore access ====
 
Access to Facebook contacts:
 
  "datastores-access": {
    "Gaia_Facebook_Friends": {
      "description": "Facebook Friends"
    } 
  }




Line 61: Line 140:


====1. XSS & HTML Injection attacks====
====1. XSS & HTML Injection attacks====
InnerHTML used dangerously  despite presence of templating system e.g:
http://mxr.mozilla.org/gaia/source/apps/sms/js/waiting_screen.js#22 (doesnt seem to be called though)


Data retrieved for delivery reports seems to be correctly rendered in the information panel. Timestamps come from Gecko.
There is a few "innerHTML" but there are used alongwith templating (TMPL.*.interpolate).


====2. Secure Communications ====
====2. Secure Communications ====
N/A Nothing loaded remotely
No comunication with any external services.


====3. Secure data storage ====
====3. Secure data storage ====
Line 73: Line 151:


====4. Denial of Service ====
====4. Denial of Service ====
No issues identified.
No issues.
Subject messages are limited to 140 characters.


====5. Use of Privileged APIs ====
====5. Use of Privileged APIs ====
The app uses privileged APIs in a sane manner - can't see any opportunity for hardening.
 


====6. Interfaces with other Apps/Content====
====6. Interfaces with other Apps/Content====
No issues other than web activity issues previously raised.
No issues.
 
 


=== Actions & Recommendations ===
=== Actions & Recommendations ===
Private issue: bug 879136
* See bug 912885
Remove mobileconnection permission since it isn't used.
 


[[Category:SecReview]]
[[Category:SecReview]]

Latest revision as of 10:28, 3 March 2014

App Review Details

Overview

SMS app provides the ability to send and receive SMS/MMS messages. SMS was reviewed previously in jun 2013.

The main focus of this re-review was to include the addition of subject support in MMS (bug 919966) and delivery report support (bug 919977). The prior review can be found here.

Architecture

Components

SMS app is structed as a single HTML page. This page displays the list of SMS messages, UI for composing sms, viewing threads, deleting messages etc.

A message is composed of a phone number and optional body, contact and threadId. There is also web activity handlers so that other apps can ask the SMS app to send a message.

Inputs to the SMS app: SMS events from system, web activity requests, data from contacts, user input to write the SMS.

At startup, 4 components are initialized:

  • ActivityHandler: application's global Activity handler to delegate to specific handler based on the Activity name; action when receiving a sms/mms.
  • ThreadUI: UI actions (listeners, view initialization, message templating...)
  • MessageManager: message actions (send, receive...)
  • Settings: set the default mms message size limitation


Relevant Source Code

UI:

  • action_menu.js
  • attachment_menu.js
  • compose.js - handle UI specifics of message composition
  • dialog.js - generic confirm screen
  • fixed_header.js
  • information.js - Information view (delivery report)
  • thread_list_ui.js - track current number of rendered threads
  • thread_ui.js - manage threads
  • waiting_screen.js

Actions:

  • activity_handler.js - main activity handler
  • activity_picker.js - to perform actions such as calling/adding/editing a contact, sending a sms/mms/email
  • attachments.js - create an attachment object (media source and text)
  • contacts.js - find/filter contacts
  • blacklist.js - get the blacklisted numbers
  • link_action_helper.js - link url/email/phone data in a message
  • message_manager.js - send/received messages
  • notify.js - notification on incoming message
  • recipients.js - manage recipients list
  • smil.js - handle SMIL document (layout of MMS message)
  • startup.js

Settings:

  • settings - default MMS size limitation if no operator specification

Util:

  • link_helper.js - replace input strings by anchor links for url, phone, email
  • utils.js - time, contact details, font size, carrier tag, etc.
  • wbmp.js - decode the Wireless Bitmap image format (.wbmp is Wireless Application Protocol Bitmap Format)


Shared code:

  • shared/js/l10n.js
  • shared/js/lazy_loader.js

The *_mock.js files are not reviewed since they are test files.


Permissions

The sms app requires the following permissions:

 "permissions": {
   "sms":{},
   "storage":{},
   "mobileconnection": {},
   "contacts":{ "access": "readonly" },
   "settings":{ "access": "readonly" },
   "audio-channel-notification":{},
   "desktop-notification":{},
   "phonenumberservice":{}
 }

There are two new permissions:

  • mobileconnection: used in settings.js for listening to mobile connection datachange before switching between SIMs .
  • phonenumberservice: used for normalizing and comparing phone numbers

Web Activity Handlers

The SMS app handles two Activities, defined in activity_handler.js:

 "activities": {
   "new": {
     "filters": {
       "type": "websms/sms",
       "number": {
         "pattern":"[\\w\\s+#*().-]{0,50}"
       }
      },
     "disposition": "window"
   },
   "share": {
     "filters": {
       "type": ["image/*", "audio/*", "video/*"],
       "number": 1
      },
     "disposition": "window",
     "returnValue": true
   }
 }
  • new - to create a new message and pre-populate message details (phone number, body, contact).
  • share - to allow to send a media in attachment via a SMS/MMS, launching the sms app from another app.

Web Activity Usage

  • blacklist.js gets the blacklist file by an XHR request to 'js/backlist.json'.
  • ActivityPicker can make calls to several activities with web usage: new (mail), view (url)
  • A pick Activity (with type ['image/*', 'audio/*', 'video/*']) is used to allow the user to create an attachment.
  • A pick Activity (with type webontacts/contacts) is used to retrieve a Contact from the adress book.

Notable Event Handlers

The 'hashchange' event handler is used in threads.js and activity_handler.js.

Datastore access

Access to Facebook contacts:

 "datastores-access": {
   "Gaia_Facebook_Friends": {
     "description": "Facebook Friends"
   }   
 }


Code Review Notes

1. XSS & HTML Injection attacks

Data retrieved for delivery reports seems to be correctly rendered in the information panel. Timestamps come from Gecko. There is a few "innerHTML" but there are used alongwith templating (TMPL.*.interpolate).

2. Secure Communications

No comunication with any external services.

3. Secure data storage

Sms data is stored in indexeddb, which is stored inside the profile, which is to be protected via file permissions.

4. Denial of Service

No issues. Subject messages are limited to 140 characters.

5. Use of Privileged APIs

6. Interfaces with other Apps/Content

No issues.

Actions & Recommendations

  • See bug 912885