Closed Bug 845487 Opened 9 years ago Closed 9 years ago
Dialer responds to cross-origin messages without verifying the source (exploitable)
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
9 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.
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.
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
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: 9 years ago
Resolution: --- → FIXED
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!
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: 9 years ago → 9 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 ;-)
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
(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.
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
You need to log in before you can comment on or make changes to this bug.