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)

x86
macOS
defect
Not set
normal

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)

Attached image Exploit in action (deleted) —
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)
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.
Sec-High or Sec-Moderate for this?
high if we are not able to fix bug 835983 on time
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.
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?
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.
Assignee: fbsc → gtorodelvalle
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
Attached file Associated PR. (deleted) —
Attachment #726715 - Flags: review?(jmcf)
The pull request looks good. Can someone with the right powers mark it for review?
Attachment #726715 - Flags: review?(jmcf) → review?(crdlc)
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+
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!
Merge, I've tested and everything is OK on my device
Status: ASSIGNED → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
Blocks: 853041
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 → ---
Sorry about that, did not see bug #853041 filed as I did not have access to this bug until we made the backout.
German please add some comment on your code, squash the new commit and then push -f your branch again. Thanks for your work!
Attached file New PR without Lint errors. (deleted) —
Attachment #727602 - Flags: review?(crdlc)
Comment on attachment 727602 [details] New PR without Lint errors. perfect, amigo!
Attachment #727602 - Flags: review?(crdlc) → review+
Merging it! Thanks to everybody involved! :-)
Status: REOPENED → RESOLVED
Closed: 12 years ago12 years ago
Resolution: --- → FIXED
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 ;-)
Blocks: 853799
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
blocking-b2g: tef? → tef+
Flags: needinfo?(jhford)
(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)
Please German take a look to this. If you have some problem with the last commit please tell me
Flags: needinfo?(dcoloma) → needinfo?(gtorodelvalle)
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)
Thanks! Setting the status-b2g18 flag on this and the linked bugs so that we keep the bug status consistent.
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
Group: core-security
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: