Closed Bug 940045 Opened 6 years ago Closed 6 years ago

The origin displayed in the persistent notification matches the location bar, not content

Categories

(Core :: WebRTC: Audio/Video, defect)

26 Branch
ARM
Gonk (Firefox OS)
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla28
blocking-b2g koi+
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)

91 bytes, text/html
Details
8.89 KB, patch
schien
: review+
Details | Diff | Splinter Review
8.40 KB, patch
schien
: review+
Details | Diff | Splinter Review
6.56 KB, patch
schien
: review+
Details | Diff | Splinter Review
6.95 KB, patch
schien
: review+
Details | Diff | Splinter Review
9.12 KB, patch
Details | Diff | Splinter Review
8.43 KB, patch
Details | Diff | Splinter Review
6.55 KB, patch
Details | Diff | Splinter Review
6.95 KB, 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.
blocking-b2g: --- → koi?
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.
Blocks: 940075
(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.
Knowing the increasing impact mentioned above, could the sec rating here be higher than sec-moderate? What do you think Paul?
Flags: needinfo?(ptheriault)
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)
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)
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)
I'm investigating this bug right now.
Assignee: nobody → schien
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
(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?

(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?
idk.  Jason, can you answer comment 11?
Flags: needinfo?(jduell.mcbugs)
blocking-b2g: koi? → koi+
Blocking on this for the spoofing risk on a new 1.2 feature
This is a proof-of-concept that sending request URL from content process can show the correct origin of gum request in notification.
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)
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
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
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
Attachment #8337579 - Attachment mime type: text/plain → text/html
Attachment #8337569 - Flags: review? → review?(mhabicher)
Attachment #8337571 - Flags: review? → review?(21)
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+
(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.
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.
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)
aggregate common code for gUM and Camera.
Attachment #8337570 - Attachment is obsolete: true
Attachment #8338448 - Flags: review?(rjesup)
aggregate common code for gUM and Camera.
Attachment #8337569 - Attachment is obsolete: true
Attachment #8337569 - Flags: review?(mhabicher)
Attachment #8338450 - Flags: review?(mhabicher)
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)
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)
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)
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)
Attachment #8338457 - Flags: review?(rjesup) → review+
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 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+
(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)
update according to comment 33, carry r+.
Attachment #8338458 - Attachment is obsolete: true
Attachment #8339046 - Flags: review+
update according to comment 34 (including deleting empty process record), carry r+.
Attachment #8338459 - Attachment is obsolete: true
Attachment #8339047 - Flags: review+
(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+
rebase to m-c tip, carry r+.
Attachment #8338456 - Attachment is obsolete: true
Attachment #8341500 - Flags: review+
rebase to m-c tip, carry r+.
Attachment #8338457 - Attachment is obsolete: true
Attachment #8341501 - Flags: review+
rebase to m-c tip, carry r+.
Attachment #8339046 - Attachment is obsolete: true
Attachment #8341502 - Flags: review+
rebase to m-c tip, carry r+.
Attachment #8339047 - Attachment is obsolete: true
Attachment #8341503 - Flags: review+
Keywords: verifyme
QA Contact: jsmith
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.
@Ryan, please backout my patch (see comment 52).
Status: RESOLVED → REOPENED
Flags: needinfo?(ryanvm)
Resolution: FIXED → ---
Keywords: verifyme
Can we just spotfix?
Flags: needinfo?(ryanvm) → needinfo?(schien)
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)
Keywords: verifyme
(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?
https://hg.mozilla.org/mozilla-central/rev/b857284946e0
Status: REOPENED → RESOLVED
Closed: 6 years ago6 years ago
Resolution: --- → FIXED
(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.
Verified on 12/12/2013 Buri 1.2 build - attached test case & comment 0 test case works fine.
Whiteboard: [adv-main28+]
Is this issue FirefoxOS only?
Flags: needinfo?(jsmith)
(In reply to Al Billings [:abillings] from comment #60)
> Is this issue FirefoxOS only?

Yes.
Flags: needinfo?(jsmith)
Thanks. That means we haven't shipped this problem anywhere yet, as far as Dan and I can tell.
Whiteboard: [adv-main28+] → [adv-main28-]
Group: core-security
You need to log in before you can comment on or make changes to this bug.