Security/Reviews/Gaia/clock: Difference between revisions
Jump to navigation
Jump to search
Ptheriault (talk | contribs) (Created page with "=== App Review Details === * App: * Review Date: * Review Lead: === Overview === ===Architecture=== ====Components==== ====Relevant Source Code==== ====Permissions==...") |
|||
| (4 intermediate revisions by the same user not shown) | |||
| Line 1: | Line 1: | ||
=== App Review Details === | === App Review Details === | ||
* App: | * App: clock | ||
* Review Date: | * Review Date: 2013-09-23 [https://github.com/mozilla-b2g/gaia/commit/3408cc3f6b190c8cd31832fbb8cd2ae571041f29 (commit)] | ||
* Review Lead: | * Review Lead: Frederik Braun (:freddyb) | ||
* Review Bug: [https://bugzilla.mozilla.org/show_bug.cgi?id=754737 bug 754737] | |||
=== Overview === | === Overview === | ||
=== | ====Components==== | ||
Menu to view current time, date and currently set alarms. | |||
Interaction via add-alarm button or change-alarm on an existing alarm. | |||
Events set to trigger an alarm. | |||
====Relevant Source Code==== | ====Relevant Source Code==== | ||
[https://github.com/mozilla-b2g/gaia/tree/master/apps/clock Source code available on GitHub] | |||
Reviewed all JavaScript code in js/ | |||
====Permissions==== | ====Permissions==== | ||
These permissions are set in the manifest: | |||
* "storage": indexeddb/appcache without size limitations | |||
* "alarms": self-explanatory | |||
* "settings": required to add/remove alarms | |||
* "attention": required to allow alarm window to be placed on top | |||
* "audio-channel-alarm": self-explanatory | |||
The app has full read/write permissions to the Settings in order to add and remove alarms. | |||
It also stores whether the clock is shown as a digital or an analog clock. | |||
It might be desirable to have more granular settings capabilities in general, [https://bugzilla.mozilla.org/show_bug.cgi?id=841071 bug 841071] | |||
====Web Activity Handlers ==== | ====Web Activity Handlers ==== | ||
in ./active_alarm.js:20, to handle all alarm messages, | |||
====Web Activity Usage ==== | ====Web Activity Usage ==== | ||
None | |||
==== Notable Event Handlers ==== | ==== Notable Event Handlers ==== | ||
None | |||
===Code Review Notes=== | ===Code Review Notes=== | ||
====1. XSS & HTML Injection attacks==== | ====1. XSS & HTML Injection attacks==== | ||
The name of an alarm is escaped using a temporary span element and then setting and extracting it's textContent. | |||
No other text input is being handled. | |||
====2. Secure Communications ==== | ====2. Secure Communications ==== | ||
Communication with internal alarm handler code happens, but it's verification process as follows (dchan pointed this out correctly): | |||
# Child process / app calls AlarmsManager.js add(). [http://mxr.mozilla.org/mozilla-central/source/dom/alarm/AlarmsManager.js#46 [1]] This sends a message to the jsm file. The message includes the manifestURL of the current app | |||
# AlarmsService.jsm handles the message, checking that the sender has the alarms permission and that the sender is actually the correct app for the specified manifestURL [http://mxr.mozilla.org/mozilla-central/source/dom/alarm/AlarmService.jsm#81 [2]] | |||
# The alarm is pushed onto a queue. AlarmHALService is reponsible for triggering the alarms at the correct time [http://mxr.mozilla.org/mozilla-central/source/dom/alarm/AlarmHalService.cpp [3]] | |||
# A system message is fired using the data from the alarmDB. This sends a message to the manifestURL that was originally stored [http://mxr.mozilla.org/mozilla-central/source/dom/alarm/AlarmService.jsm#274 [4]] | |||
# More checks that the app registerd to listen for the alarm message [http://mxr.mozilla.org/mozilla-central/source/dom/messages/SystemMessageInternal.js#73 [5]] [http://mxr.mozilla.org/mozilla-central/source/dom/messages/SystemMessageInternal.js#433 [6]] | |||
====3. Secure data storage ==== | ====3. Secure data storage ==== | ||
No storage of sensible data. | |||
====4. Denial of Service ==== | ====4. Denial of Service ==== | ||
None | |||
====5. Use of Privileged APIs ==== | ====5. Use of Privileged APIs ==== | ||
Settings | |||
====6. Interfaces with other Apps/Content==== | ====6. Interfaces with other Apps/Content==== | ||
None | |||
=== Security Risks & Mitigating Controls === | |||
=== Actions & Recommendations === | === Actions & Recommendations === | ||
Latest revision as of 14:59, 23 September 2013
App Review Details
- App: clock
- Review Date: 2013-09-23 (commit)
- Review Lead: Frederik Braun (:freddyb)
- Review Bug: bug 754737
Overview
Components
Menu to view current time, date and currently set alarms. Interaction via add-alarm button or change-alarm on an existing alarm.
Events set to trigger an alarm.
Relevant Source Code
Source code available on GitHub
Reviewed all JavaScript code in js/
Permissions
These permissions are set in the manifest:
- "storage": indexeddb/appcache without size limitations
- "alarms": self-explanatory
- "settings": required to add/remove alarms
- "attention": required to allow alarm window to be placed on top
- "audio-channel-alarm": self-explanatory
The app has full read/write permissions to the Settings in order to add and remove alarms. It also stores whether the clock is shown as a digital or an analog clock.
It might be desirable to have more granular settings capabilities in general, bug 841071
Web Activity Handlers
in ./active_alarm.js:20, to handle all alarm messages,
Web Activity Usage
None
Notable Event Handlers
None
Code Review Notes
1. XSS & HTML Injection attacks
The name of an alarm is escaped using a temporary span element and then setting and extracting it's textContent. No other text input is being handled.
2. Secure Communications
Communication with internal alarm handler code happens, but it's verification process as follows (dchan pointed this out correctly):
- Child process / app calls AlarmsManager.js add(). [1] This sends a message to the jsm file. The message includes the manifestURL of the current app
- AlarmsService.jsm handles the message, checking that the sender has the alarms permission and that the sender is actually the correct app for the specified manifestURL [2]
- The alarm is pushed onto a queue. AlarmHALService is reponsible for triggering the alarms at the correct time [3]
- A system message is fired using the data from the alarmDB. This sends a message to the manifestURL that was originally stored [4]
- More checks that the app registerd to listen for the alarm message [5] [6]
3. Secure data storage
No storage of sensible data.
4. Denial of Service
None
5. Use of Privileged APIs
Settings
6. Interfaces with other Apps/Content
None