Closed
Bug 940045
Opened 11 years ago
Closed 11 years ago
The origin displayed in the persistent notification matches the location bar, not content
Categories
(Core :: WebRTC: Audio/Video, defect)
Tracking
()
Tracking | Status | |
---|---|---|
firefox26 | --- | wontfix |
firefox27 | --- | wontfix |
firefox28 | --- | fixed |
firefox-esr24 | --- | wontfix |
b2g18 | --- | unaffected |
b2g-v1.1hd | --- | unaffected |
b2g-v1.2 | --- | verified |
People
(Reporter: jsmith, Assigned: schien)
References
Details
(Keywords: csectype-spoof, sec-moderate, Whiteboard: [adv-main28-])
Attachments
(9 files, 15 obsolete files)
(deleted),
text/html
|
Details | |
(deleted),
patch
|
schien
:
review+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
schien
:
review+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
schien
:
review+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
schien
:
review+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
Details | Diff | Splinter Review | |
(deleted),
patch
|
Details | Diff | Splinter Review | |
(deleted),
patch
|
Details | Diff | Splinter Review | |
(deleted),
patch
|
Details | Diff | Splinter Review |
Build - 11/18/2013 1.2 Buri STR 1. Go to http://mozilla.github.io/qa-testcase-data/webapi/webrtc/iframemozqagum.html 2. Request gUM audio & grant permission 3. Check the persistent notification Expected The persistent notification should use mozqa.com as the origin. Actual The persistent notification uses mozilla.github.io as the origin. Additional Notes Security sensitive for the same reasons why bug 876044 was.
Reporter | ||
Updated•11 years ago
|
Reporter | ||
Comment 1•11 years ago
|
||
Strangely enough - if you happen to hit this bug, all subsequent requests on any origin in the browser will pretty much stay with the origin you ended up in the actual results until you kill the browser process.
Reporter | ||
Comment 2•11 years ago
|
||
(In reply to Jason Smith [:jsmith] from comment #1) > Strangely enough - if you happen to hit this bug, all subsequent requests on > any origin in the browser will pretty much stay with the origin you ended up > in the actual results until you kill the browser process. So there's a lot of bad things that happen when we get into state - the origin becomes wrong, the persistent notification could show when there's no recording present, and a shutdown crash is possible in bug 940075. We need to block on this.
Reporter | ||
Comment 3•11 years ago
|
||
Knowing the increasing impact mentioned above, could the sec rating here be higher than sec-moderate? What do you think Paul?
Flags: needinfo?(ptheriault)
Comment 4•11 years ago
|
||
The crash looks like a null-deference so no so worried about that part. I think this could be called a sec-high but I think its border-line. If I understand this correctly, a page that could manage to get itself framed by another site spoof permission requests from the other domain. There are probably many situations where this would be confusing to the user, but it doesn't sound as bad as say getting access to webrtc without any prompt at all.
Flags: needinfo?(ptheriault)
Comment 5•11 years ago
|
||
This is a significant privacy problem in that the ability for an evil-ad hosted on a trusted-site page can make it look like the trusted-site is requesting the permission to record. This may cause people to mistakenly put trust in evil-ad, but it would have to be a situation where they would expect a question. It's worse in that this is persistent, so I assume you've given evil-ad persistent permission and evil-ad can turn on the camera/mic at any time with no notification later. (Jason, is that correct?) This part is concerning as well, and it's hard for me to judge the impact in practice: "all subsequent requests on any origin in the browser will pretty much stay with the origin you ended up in the actual results until you kill the browser process." CC-ing ekr who wrote the webrtc security draft for comment
Flags: needinfo?(jsmith)
Flags: needinfo?(ekr)
Comment 6•11 years ago
|
||
This seems bad, obviously. I don't really know that it makes sense to debate the security level between high and medium or whatever, but we should certainly try to fix ASAP.
Flags: needinfo?(ekr)
Assignee | ||
Comment 8•11 years ago
|
||
I know where the problem is. The URL we get from FrameLoader is the out most window of child process. I can send the page URL through IPDL directly instead of query from TabParent. http://dxr.mozilla.org/mozilla-central/source/dom/ipc/TabParent.cpp?from=TabParent.cpp#1680
Reporter | ||
Comment 9•11 years ago
|
||
(In reply to Randell Jesup [:jesup] from comment #5) > This is a significant privacy problem in that the ability for an evil-ad > hosted on a trusted-site page can make it look like the trusted-site is > requesting the permission to record. This may cause people to mistakenly > put trust in evil-ad, but it would have to be a situation where they would > expect a question. It's worse in that this is persistent, so I assume > you've given evil-ad persistent permission and evil-ad can turn on the > camera/mic at any time with no notification later. (Jason, is that correct?) Mostly - the only piece I'd add to this is that the bug is happening with the persistent notification, not the permission prompt. However, I think the fact that the origin is incorrect in the persistent notification & right in the permission prompt will end up establishing a lack of user trust on whether their mic is at the hands of content he/she can trust or not, as a user might notice the inconsistency in the origin in the permission prompt vs. persistent notification.
Flags: needinfo?(jsmith)
Trusting the URL sent over from the content process seems wrong though. What if it has been compromised?
Comment 11•11 years ago
|
||
(In reply to Kyle Huey [:khuey] (khuey@mozilla.com) from comment #10) > Trusting the URL sent over from the content process seems wrong though. > What if it has been compromised? How does the process separation generally work? E.g., when the content process accesses the cookie store, where does the origin come from?
Updated•11 years ago
|
blocking-b2g: koi? → koi+
Assignee | ||
Comment 14•11 years ago
|
||
This is a proof-of-concept that sending request URL from content process can show the correct origin of gum request in notification.
Comment 15•11 years ago
|
||
When we access the cookie store, we trust the URI that the child process gives us: http://mxr.mozilla.org/mozilla-central/source/netwerk/cookie/PCookieService.ipdl#63 A given child process gets its own cookie database--it can't read cookies of other apps--but within an app there's nothing to prevent a compromised process from reading arbitrary cookies within that cookie-jar. If the process hasn't been compromised than my understanding is that the logic in nsHTMLDocument::GetCookie makes sure the origin of the "document principal" is used: http://mxr.mozilla.org/mozilla-central/source/content/html/document/src/nsHTMLDocument.cpp#1190 I'm not clear on how we're supposed to provide more security than the cookie-jar here. If we let a child process set cookies for various URIs within its cookie-jar, we also have to let it read them. But I'm not sure how much the cookie model applies to the bug here. (I'm on PTO till Monday BTW in case you need more info from me).
Flags: needinfo?(jduell.mcbugs)
Assignee | ||
Comment 16•11 years ago
|
||
Attachment #8335778 -
Attachment is obsolete: true
Attachment #8337567 -
Flags: review?(khuey)
Assignee | ||
Comment 20•11 years ago
|
||
This test case provides following scenarios: 1. iframe in the same origin 2. iframe in different origin 3. nested iframe in different origins 4. multiple iframe in different origins
Assignee | ||
Comment 21•11 years ago
|
||
Comment on attachment 8337575 [details] test case for multiple iframe and nested iframe <script> location.replace('http://people.mozilla.org/~schien/gum-test.html'); </script>
Attachment #8337575 -
Attachment mime type: text/plain → text/html
Assignee | ||
Comment 22•11 years ago
|
||
This test case provides following scenarios: 1. iframe in the same origin 2. iframe in different origin 3. nested iframe in different origins 4. multiple iframe in different origins
Attachment #8337575 -
Attachment is obsolete: true
Assignee | ||
Updated•11 years ago
|
Attachment #8337579 -
Attachment mime type: text/plain → text/html
Assignee | ||
Updated•11 years ago
|
Attachment #8337569 -
Flags: review? → review?(mhabicher)
Assignee | ||
Updated•11 years ago
|
Attachment #8337571 -
Flags: review? → review?(21)
Comment 23•11 years ago
|
||
Comment on attachment 8337570 [details] [diff] [review] Part 3 - send requestURL/isApp for gUM API Review of attachment 8337570 [details] [diff] [review]: ----------------------------------------------------------------- ::: dom/media/MediaManager.cpp @@ +1799,5 @@ > + return NotifyRecordingStatusChange(msg); > +} > + > +nsresult > +GetUserMediaNotificationEvent::NotifyRecordingStatusChange(nsString& aMsg) This appears to be duplicated in the Camera API (patch 2). Is there any easy way to instead share the code? If not, please add a comment to both stating there's a duplicate (or semi-duplicate) elsewhere, so someone modifying one doesn't forget to modify the other.
Attachment #8337570 -
Flags: review?(rjesup) → review+
Assignee | ||
Comment 24•11 years ago
|
||
(In reply to Randell Jesup [:jesup] from comment #23) > Comment on attachment 8337570 [details] [diff] [review] > Part 3 - send requestURL/isApp for gUM API > > Review of attachment 8337570 [details] [diff] [review]: > ----------------------------------------------------------------- > > ::: dom/media/MediaManager.cpp > @@ +1799,5 @@ > > + return NotifyRecordingStatusChange(msg); > > +} > > + > > +nsresult > > +GetUserMediaNotificationEvent::NotifyRecordingStatusChange(nsString& aMsg) > > This appears to be duplicated in the Camera API (patch 2). Is there any > easy way to instead share the code? > > If not, please add a comment to both stating there's a duplicate (or > semi-duplicate) elsewhere, so someone modifying one doesn't forget to modify > the other. The most reasonable common place I can find is nsPIDOMWindow. I can try move the NotifyRecordingStatusChange() into nsPIDOMWindow if you think it is a good idea.
Comment 25•11 years ago
|
||
It could be a static method on MediaManager, taking aWindowID (or aWindow), aMsg, aIsAudio, and aIsVideo as arguments. That might be simpler and less confusing. Not required, but overall cleaner and easier to maintain.
Assignee | ||
Comment 26•11 years ago
|
||
1. Move RecordingDeviceEvent back to PContent 2. carry requestURL from content process
Attachment #8337567 -
Attachment is obsolete: true
Attachment #8337567 -
Flags: review?(khuey)
Attachment #8338446 -
Flags: review?(khuey)
Assignee | ||
Comment 27•11 years ago
|
||
aggregate common code for gUM and Camera.
Attachment #8337570 -
Attachment is obsolete: true
Attachment #8338448 -
Flags: review?(rjesup)
Assignee | ||
Comment 28•11 years ago
|
||
aggregate common code for gUM and Camera.
Attachment #8337569 -
Attachment is obsolete: true
Attachment #8337569 -
Flags: review?(mhabicher)
Attachment #8338450 -
Flags: review?(mhabicher)
Assignee | ||
Comment 29•11 years ago
|
||
1. Move RecordingDeviceEvent back to PContent 2. carry requestURL from content process 3. correct hg comment
Attachment #8338446 -
Attachment is obsolete: true
Attachment #8338446 -
Flags: review?(khuey)
Attachment #8338456 -
Flags: review?(khuey)
Assignee | ||
Comment 30•11 years ago
|
||
1. aggregate common code for gUM and Camera. 2. remove tailing space 3. correct hg comment
Attachment #8338448 -
Attachment is obsolete: true
Attachment #8338448 -
Flags: review?(rjesup)
Attachment #8338457 -
Flags: review?(rjesup)
Assignee | ||
Comment 31•11 years ago
|
||
1. aggregate common code for gUM and Camera. 2. correct hg comment
Attachment #8338450 -
Attachment is obsolete: true
Attachment #8338450 -
Flags: review?(mhabicher)
Attachment #8338458 -
Flags: review?(mhabicher)
Assignee | ||
Comment 32•11 years ago
|
||
1. support multiple requestURL with one content process 2. correct hg comment
Attachment #8337571 -
Attachment is obsolete: true
Attachment #8337571 -
Flags: review?(21)
Attachment #8338459 -
Flags: review?(21)
Updated•11 years ago
|
Attachment #8338457 -
Flags: review?(rjesup) → review+
Comment 33•11 years ago
|
||
Comment on attachment 8338458 [details] [diff] [review] Part 3 - send requestURL for Camera API Review of attachment 8338458 [details] [diff] [review]: ----------------------------------------------------------------- r+ with the one issue below to be fixed. ::: dom/camera/DOMCameraControl.cpp @@ +579,5 @@ > return mCameraControl; > } > > +nsresult > +nsDOMCameraControl::NotifyRecordingStatusChange(bool aIsStart) { nit: opening brace goes on a new line.
Attachment #8338458 -
Flags: review?(mhabicher) → review+
Comment 34•11 years ago
|
||
Comment on attachment 8338459 [details] [diff] [review] Part 4 - support multiple requestURL with one content process Review of attachment 8338459 [details] [diff] [review]: ----------------------------------------------------------------- ::: b2g/chrome/content/shell.js @@ +1278,4 @@ > } > > + let commandHandler = function (requestURL, command) { > + let currentActive = gRecordingActiveProcesses[processId][requestURL]; nit: let also create a temporary variable: let currentProcess = gRecordingActiveProcesses[processId]; @@ +1303,5 @@ > + > + if (currentActive['count'] > 0) { > + gRecordingActiveProcesses[processId][requestURL] = currentActive; > + } else { > + delete gRecordingActiveProcesses[processId][requestURL]; Are we deleting gRecordingActiveProcesses[processId] at some point if there is not properties anymore?
Attachment #8338459 -
Flags: review?(21) → review+
Assignee | ||
Comment 35•11 years ago
|
||
(In reply to Vivien Nicolas (:vingtetun) (:21) (RTO - Review Time Off until 09/12/13) from comment #34) > > @@ +1303,5 @@ > > + > > + if (currentActive['count'] > 0) { > > + gRecordingActiveProcesses[processId][requestURL] = currentActive; > > + } else { > > + delete gRecordingActiveProcesses[processId][requestURL]; > > Are we deleting gRecordingActiveProcesses[processId] at some point if there > is not properties anymore? I was thinking to reduce create/delete gRecordingActiveProcesses[processId] too often, so I only delete gRecordingActiveProcesses[processId] while child process is killed. I can do that if you think gRecordingActiveProcesses should be kept as compact as possible.
Flags: needinfo?(21)
Assignee | ||
Comment 36•11 years ago
|
||
update according to comment 33, carry r+.
Attachment #8338458 -
Attachment is obsolete: true
Attachment #8339046 -
Flags: review+
Assignee | ||
Comment 37•11 years ago
|
||
update according to comment 34 (including deleting empty process record), carry r+.
Attachment #8338459 -
Attachment is obsolete: true
Attachment #8339047 -
Flags: review+
Comment 38•11 years ago
|
||
(In reply to Shih-Chiang Chien [:schien] from comment #35) > (In reply to Vivien Nicolas (:vingtetun) (:21) (RTO - Review Time Off until > 09/12/13) from comment #34) > > > > @@ +1303,5 @@ > > > + > > > + if (currentActive['count'] > 0) { > > > + gRecordingActiveProcesses[processId][requestURL] = currentActive; > > > + } else { > > > + delete gRecordingActiveProcesses[processId][requestURL]; > > > > Are we deleting gRecordingActiveProcesses[processId] at some point if there > > is not properties anymore? > I was thinking to reduce create/delete gRecordingActiveProcesses[processId] > too often, so I only delete gRecordingActiveProcesses[processId] while child > process is killed. I can do that if you think gRecordingActiveProcesses > should be kept as compact as possible. If you delete it when the process is killed that's fine.
Flags: needinfo?(21)
Comment on attachment 8338456 [details] [diff] [review] Part 1 - carry requestURL property from content process Review of attachment 8338456 [details] [diff] [review]: ----------------------------------------------------------------- r=me
Attachment #8338456 -
Flags: review?(khuey) → review+
Assignee | ||
Comment 40•11 years ago
|
||
rebase to m-c tip, carry r+.
Attachment #8338456 -
Attachment is obsolete: true
Attachment #8341500 -
Flags: review+
Assignee | ||
Comment 41•11 years ago
|
||
rebase to m-c tip, carry r+.
Attachment #8338457 -
Attachment is obsolete: true
Attachment #8341501 -
Flags: review+
Assignee | ||
Comment 42•11 years ago
|
||
rebase to m-c tip, carry r+.
Attachment #8339046 -
Attachment is obsolete: true
Attachment #8341502 -
Flags: review+
Assignee | ||
Comment 43•11 years ago
|
||
rebase to m-c tip, carry r+.
Attachment #8339047 -
Attachment is obsolete: true
Attachment #8341503 -
Flags: review+
Assignee | ||
Updated•11 years ago
|
Keywords: checkin-needed
Comment 49•11 years ago
|
||
https://hg.mozilla.org/integration/b2g-inbound/rev/51132887f014 https://hg.mozilla.org/integration/b2g-inbound/rev/7d02b81f691b https://hg.mozilla.org/integration/b2g-inbound/rev/f6675487604f https://hg.mozilla.org/integration/b2g-inbound/rev/09c1263cd190
Keywords: checkin-needed
Comment 50•11 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/51132887f014 https://hg.mozilla.org/mozilla-central/rev/7d02b81f691b https://hg.mozilla.org/mozilla-central/rev/f6675487604f https://hg.mozilla.org/mozilla-central/rev/09c1263cd190 Can we get a test for this?
Status: NEW → RESOLVED
Closed: 11 years ago
status-b2g-v1.2:
--- → affected
status-firefox26:
--- → wontfix
status-firefox27:
--- → wontfix
status-firefox28:
--- → fixed
Flags: in-testsuite?
Resolution: --- → FIXED
Target Milestone: --- → mozilla28
Comment 51•11 years ago
|
||
https://hg.mozilla.org/releases/mozilla-b2g26_v1_2/rev/cd7d22a8945e https://hg.mozilla.org/releases/mozilla-b2g26_v1_2/rev/b4eb9fd1b401 https://hg.mozilla.org/releases/mozilla-b2g26_v1_2/rev/9ca958d6020e https://hg.mozilla.org/releases/mozilla-b2g26_v1_2/rev/891244867a4d
Assignee | ||
Comment 52•11 years ago
|
||
Comment on attachment 8341503 [details] [diff] [review] Part 4 - support multiple requestURL with one content process Review of attachment 8341503 [details] [diff] [review]: ----------------------------------------------------------------- ::: b2g/chrome/content/shell.js @@ +1346,4 @@ > break; > case 'content-shutdown': > + // iterate through all the existing active processes > + Object.keys(gRecordingActiveProcesses[processId]).foreach(function(requestURL) { "foreach" needs to be "forEach"...This single character error makes recording icon cannot disappear after killing content process while active recording. I really hate to say so but please backout my patch. I feel really sorry about this.
Assignee | ||
Comment 53•11 years ago
|
||
@Ryan, please backout my patch (see comment 52).
Status: RESOLVED → REOPENED
Flags: needinfo?(ryanvm)
Resolution: FIXED → ---
Comment 55•11 years ago
|
||
I pushed a s/foreach/forEach fix to b-i and b2g26. If this isn't enough and you still want a backout, let me know and I will. https://hg.mozilla.org/integration/b2g-inbound/rev/b857284946e0 https://hg.mozilla.org/releases/mozilla-b2g26_v1_2/rev/5dd49ceed628
Flags: needinfo?(schien)
Comment 56•11 years ago
|
||
(In reply to Shih-Chiang Chien [:schien] from comment #52) > "foreach" needs to be "forEach"...This single character error makes > recording icon cannot disappear after killing content process while active > recording. I really hate to say so but please backout my patch. I feel > really sorry about this. Is there a way we could have automated tests to catch an issue like this?
Comment 57•11 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/b857284946e0
Status: REOPENED → RESOLVED
Closed: 11 years ago → 11 years ago
Resolution: --- → FIXED
Assignee | ||
Comment 58•11 years ago
|
||
(In reply to Ryan VanderMeulen [:RyanVM UTC-5] from comment #56) > (In reply to Shih-Chiang Chien [:schien] from comment #52) > > "foreach" needs to be "forEach"...This single character error makes > > recording icon cannot disappear after killing content process while active > > recording. I really hate to say so but please backout my patch. I feel > > really sorry about this. > > Is there a way we could have automated tests to catch an issue like this? I couldn't find a existing test framework for automatically testing shell.js so I'm planning to build one. I just file Bug 947010 for this task.
Reporter | ||
Comment 59•11 years ago
|
||
Verified on 12/12/2013 Buri 1.2 build - attached test case & comment 0 test case works fine.
Keywords: verifyme
Updated•11 years ago
|
status-firefox-esr24:
--- → wontfix
Updated•11 years ago
|
status-b2g18:
--- → unaffected
status-b2g-v1.1hd:
--- → unaffected
Updated•11 years ago
|
Whiteboard: [adv-main28+]
Reporter | ||
Comment 61•11 years ago
|
||
(In reply to Al Billings [:abillings] from comment #60) > Is this issue FirefoxOS only? Yes.
Flags: needinfo?(jsmith)
Comment 62•11 years ago
|
||
Thanks. That means we haven't shipped this problem anywhere yet, as far as Dan and I can tell.
Whiteboard: [adv-main28+] → [adv-main28-]
Updated•10 years ago
|
Group: core-security
You need to log in
before you can comment on or make changes to this bug.
Description
•