Closed
Bug 845487
Opened 12 years ago
Closed 12 years ago
Dialer responds to cross-origin messages without verifying the source (exploitable)
Categories
(Firefox OS Graveyard :: Gaia::Dialer, defect)
Tracking
(blocking-b2g:tef+, firefox-esr17 unaffected, b2g18 fixed, b2g18-v1.0.1 fixed)
RESOLVED
FIXED
blocking-b2g | tef+ |
Tracking | Status | |
---|---|---|
firefox-esr17 | --- | unaffected |
b2g18 | --- | fixed |
b2g18-v1.0.1 | --- | fixed |
People
(Reporter: st3fan, Assigned: gtorodelvalle)
References
Details
(Keywords: sec-high)
Attachments
(3 files)
The dialer (and the other code in the communications app) depends on window.postMessage() and setEventHandler('message',...) to send and receive cross origin messages. Usually between different pages in the same app, like dialer/index.html and dialer/oncall.html but also for remote sites like for example when we integrate with Facebook.
As these messages can originate from multiple sources it is highly recommended that the event handler checks the origin of these messages. This to prevent messages from malicious sources.
This is outlined on MDN at https://developer.mozilla.org/en-US/docs/DOM/window.postMessage#Security_concerns
EXPLOIT:
This exploit shows how Facebook can send Missed Call notifications to the phone from any of the integration points that we have with them:
1) Open the dialer
2) Click the contacts button (middle on bottom of screen)
3) Flip the Facebook slider so that the FB login page appears
4) Use the fxos-repl to connect to the facebook page
2) Execute window.opener.parent.parent.postMessage({type:"notification", number:"1-800-BACONATOR"}, "*");
See the attached screenshot of the resulting Missed Call notification
MITIGATION:
1) Enable mozbrowser property on iframes that load foreign content
2) Do strict event source checking in every "message" handler as described on the MDN page for window.PostMessage.
ADDITIONAL:
Note that although I am executing the exploit code from the facebook page manually using a javascript console, it is also possible for attackers to inject this code into facebook scripts due to bug 835983 (Facebook assets are loaded over HTTP)
Comment 1•12 years ago
|
||
The frame hierarchy for the pages as provided by stefan is
1 app://communications.gaiamobile.org/dialer/index.html#contacts-view
2-- app://communications.gaiamobile.org/contacts/index.html
3---- app://communications.gaiamobile.org/facebook/fb_oauth.html
4------ http://m.facebook.com
We discussed this a little over IRC and believe that multiple fixes are needed.
1) fetch the https:// page for m.facebook.com to stop MITM attacks
2) add origin checks to postMessage in communications app.
2) change iframe 3 to have the mozbrowser attribute to stop iframe 4 from moving up the frame hierarchy.
Implementing 3) also requires that iframe 2 handles iframe 3's load event to initiate postMessage communication (event.source) . There also needs to be checks between the iframe 2/3 communication to check for allow messages.
Comment 2•12 years ago
|
||
Sec-High or Sec-Moderate for this?
Reporter | ||
Comment 3•12 years ago
|
||
high if we are not able to fix bug 835983 on time
Comment 4•12 years ago
|
||
We don't base ratings on whether we're fixing another bug on schedule or not. :-)
Either this is a sec-high because it is high, or it is sec-moderate.
Comment 5•12 years ago
|
||
Seems like a must-fix for TEF regardless of the WAP bug. To clarify, is the attack surface strictly between dialer and FB, or any app that has dialer permission like contacts?
Borja, are you the best person to take this on?
Assignee: nobody → fbsc
blocking-b2g: --- → tef?
Reporter | ||
Comment 6•12 years ago
|
||
The problem is limited to anything happening in the communications app. So that mean contacts, dialer, first run.
Applications invoking contacts through MozActivity cannot trigger this. So it is somewhat limited.
Updated•12 years ago
|
Assignee: fbsc → gtorodelvalle
Comment 7•12 years ago
|
||
I think the problem can be easily solved by checking the source property in the 'message' event handlers and avoid postMessage(obj,'*') calls.
Status: NEW → ASSIGNED
Assignee | ||
Comment 8•12 years ago
|
||
Attachment #726715 -
Flags: review?(jmcf)
Reporter | ||
Comment 9•12 years ago
|
||
The pull request looks good. Can someone with the right powers mark it for review?
Updated•12 years ago
|
Attachment #726715 -
Flags: review?(jmcf) → review?(crdlc)
Comment 10•12 years ago
|
||
Comment on attachment 726715 [details]
Associated PR.
The code is ok for me although we need to test a lot these changes because we are touching in several places before landing, ok?
Attachment #726715 -
Flags: review?(crdlc) → review+
Assignee | ||
Comment 11•12 years ago
|
||
I did test almost every execution path I could think of :-) but Cristian is retesting it again ;-) Waiting for your final OK for the merge ;-) Thanks!
Comment 12•12 years ago
|
||
Merge, I've tested and everything is OK on my device
Assignee | ||
Comment 13•12 years ago
|
||
Assignee | ||
Updated•12 years ago
|
Status: ASSIGNED → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
Comment 14•12 years ago
|
||
Hi, unfortunately we had to back this one out due to a failed travis build. Your R+ will carry over, please update the code so it passes the linter and we can land it. You can also run 'make lint' from the project root to ensure that there are no problems.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Comment 15•12 years ago
|
||
Sorry about that, did not see bug #853041 filed as I did not have access to this bug until we made the backout.
Comment 16•12 years ago
|
||
German please add some comment on your code, squash the new commit and then push -f your branch again. Thanks for your work!
Assignee | ||
Comment 17•12 years ago
|
||
Attachment #727602 -
Flags: review?(crdlc)
Comment 18•12 years ago
|
||
Comment on attachment 727602 [details]
New PR without Lint errors.
perfect, amigo!
Attachment #727602 -
Flags: review?(crdlc) → review+
Assignee | ||
Comment 19•12 years ago
|
||
Merging it! Thanks to everybody involved! :-)
Comment 20•12 years ago
|
||
Status: REOPENED → RESOLVED
Closed: 12 years ago → 12 years ago
Resolution: --- → FIXED
Assignee | ||
Comment 21•12 years ago
|
||
We just detected thanks to José Manuel that we went too far and broke the Facebook synchronization :-O I created a new bug to fix it ;-)
Comment 22•12 years ago
|
||
jhford, when uplifting, take into account that those three bugs should be uplifted in order to avoid regressions:
bug 845487
bug 853799
bug 856584
so it would be good do all of them in a row
Updated•12 years ago
|
Flags: needinfo?(jhford)
Comment 23•12 years ago
|
||
(In reply to Daniel Coloma:dcoloma from comment #22)
> jhford, when uplifting, take into account that those three bugs should be
> uplifted in order to avoid regressions:
>
> bug 845487
> bug 853799
> bug 856584
>
> so it would be good do all of them in a row
Hey, I just tried to do this uplift and the commit in this bug does not apply cleanly to v1-train or v1.0.1.
To confirm, I have the following commits:
1) 845487 -- 169a4bfb2e19a3f015373c059b7ff90af4965eed
2) 853799 -- 9718acef2680ac58b83160bdb5804fcbb3993a19
3) 856584 -- 4fbf53318c32e27cdbf79ee23955298860ed42e7
Commit 1) does not apply cleanly to v1-train or v1.0.1. Commit 2) does apply cleanly to v1-train but not v1.0.1 . Commit 3) generates a zero-length diff.
Flags: needinfo?(jhford) → needinfo?(dcoloma)
Comment 24•12 years ago
|
||
Please German take a look to this. If you have some problem with the last commit please tell me
Flags: needinfo?(dcoloma) → needinfo?(gtorodelvalle)
Assignee | ||
Comment 25•12 years ago
|
||
Hi guys!
1) 845487 -- 169a4bfb2e19a3f015373c059b7ff90af4965eed
- v1-train: https://github.com/mozilla-b2g/gaia/commit/226541671ecd9fbd2266af8c278b89732fce1fa3
2) 853799 -- 9718acef2680ac58b83160bdb5804fcbb3993a19
- v1-train: https://github.com/mozilla-b2g/gaia/commit/9233eec3063dfd826efb9b59b648af1e1b53e383
3) 856584 -- 4fbf53318c32e27cdbf79ee23955298860ed42e7
- v1-train: https://github.com/mozilla-b2g/gaia/commit/da97273e1dc6bbfde20f026834bd0388cde56b91
Doing the same for v1.0.1 right now.
Flags: needinfo?(gtorodelvalle)
Comment 26•12 years ago
|
||
Thanks!
Setting the status-b2g18 flag on this and the linked bugs so that we keep the bug status consistent.
Assignee | ||
Comment 27•12 years ago
|
||
Hi again, now the commit for v1.0.1.
1) 845487 -- 169a4bfb2e19a3f015373c059b7ff90af4965eed
- v1.0.1: https://github.com/mozilla-b2g/gaia/commit/f0bccacb4324ca862d10b819281168302e66e610
2) 853799 -- 9718acef2680ac58b83160bdb5804fcbb3993a19
- v1.0.1: https://github.com/mozilla-b2g/gaia/commit/b269790fa1c8d908334a07e7eb71d5cf2007f60d
3) 856584 -- 4fbf53318c32e27cdbf79ee23955298860ed42e7
- v1.0.1: https://github.com/mozilla-b2g/gaia/commit/1173b27f66b9c23ab9b02955230ba7bbdc8e015f
Assignee | ||
Updated•12 years ago
|
Updated•12 years ago
|
status-firefox-esr17:
--- → unaffected
Updated•9 years ago
|
Group: core-security
You need to log in
before you can comment on or make changes to this bug.
Description
•