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

Status: RESOLVED FIXED

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

Description

[Jason Smith [:jsmith]]

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.

Comment 1

[Jason Smith [:jsmith]]

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.

Comment 2

[Jason Smith [:jsmith]]

(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.

Comment 3

[Jason Smith [:jsmith]]

Knowing the increasing impact mentioned above, could the sec rating here be higher than sec-moderate? What do you think Paul?

Comment 4

[Paul Theriault [:pauljt] (no longer reading bugmail)]

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.

Comment 5

[Randell Jesup [:jesup] (needinfo me)]

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

Comment 6

[Eric Rescorla (: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.

Comment 7

[Shih-Chiang Chien [:schien] (UTC+8) (use ni? plz)]

I'm investigating this bug right now.

Comment 8

[Shih-Chiang Chien [:schien] (UTC+8) (use ni? plz)]

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

Comment 9

[Jason Smith [:jsmith]]

(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.

Comment 10

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

Trusting the URL sent over from the content process seems wrong though.  What if it has been compromised?

Comment 11

[Eric Rescorla (:ekr)]

(In reply to Kyle Huey [:khuey] ([e-mail]) 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?

Comment 12

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

idk.  Jason, can you answer comment 11?

Comment 13

[Jason Smith [:jsmith]]

Blocking on this for the spoofing risk on a new 1.2 feature

Comment 14

[Shih-Chiang Chien [:schien] (UTC+8) (use ni? plz)]

Attachment: request-url-from-content-process.patch

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

[Jason Duell]

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).

Comment 16

[Shih-Chiang Chien [:schien] (UTC+8) (use ni? plz)]

Attachment: Part 1 - carry requestURL/isApp property from content process

Comment 17

[Shih-Chiang Chien [:schien] (UTC+8) (use ni? plz)]

Attachment: Part 2 - send requestURL/isApp for Camera API

Comment 18

[Shih-Chiang Chien [:schien] (UTC+8) (use ni? plz)]

Attachment: Part 3 - send requestURL/isApp for gUM API

Comment 19

[Shih-Chiang Chien [:schien] (UTC+8) (use ni? plz)]

Attachment: Part 4 - support multiple requestURL with one content process

Comment 20

[Shih-Chiang Chien [:schien] (UTC+8) (use ni? plz)]

Attachment: test case for multiple iframe and nested iframe

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 21

[Shih-Chiang Chien [:schien] (UTC+8) (use ni? plz)]

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>

Comment 22

[Shih-Chiang Chien [:schien] (UTC+8) (use ni? plz)]

Attachment: test case for multiple iframe and nested iframe

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 23

[Randell Jesup [:jesup] (needinfo me)]

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.

Comment 24

[Shih-Chiang Chien [:schien] (UTC+8) (use ni? plz)]

(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

[Randell Jesup [:jesup] (needinfo me)]

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.

Comment 26

[Shih-Chiang Chien [:schien] (UTC+8) (use ni? plz)]

Attachment: Part 1 - carry requestURL property from content process

1. Move RecordingDeviceEvent back to PContent
2. carry requestURL from content process

Comment 27

[Shih-Chiang Chien [:schien] (UTC+8) (use ni? plz)]

Attachment: Part 2 - send requestURL for gUM API

aggregate common code for gUM and Camera.

Comment 28

[Shih-Chiang Chien [:schien] (UTC+8) (use ni? plz)]

Attachment: Part 3 - send requestURL for Camera API

aggregate common code for gUM and Camera.

Comment 29

[Shih-Chiang Chien [:schien] (UTC+8) (use ni? plz)]

Attachment: Part 1 - carry requestURL property from content process

1. Move RecordingDeviceEvent back to PContent
2. carry requestURL from content process 
3. correct hg comment

Comment 30

[Shih-Chiang Chien [:schien] (UTC+8) (use ni? plz)]

Attachment: Part 2 - send requestURL for gUM API

1. aggregate common code for gUM and Camera.
2. remove tailing space
3. correct hg comment

Comment 31

[Shih-Chiang Chien [:schien] (UTC+8) (use ni? plz)]

Attachment: Part 3 - send requestURL for Camera API

1. aggregate common code for gUM and Camera.
2. correct hg comment

Comment 32

[Shih-Chiang Chien [:schien] (UTC+8) (use ni? plz)]

Attachment: Part 4 - support multiple requestURL with one content process

1. support multiple requestURL with one content process
2. correct hg comment

Comment 33

[Mike Habicher [:mikeh] (high bugzilla latency)]

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.

Comment 34

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

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?

Comment 35

[Shih-Chiang Chien [:schien] (UTC+8) (use ni? plz)]

(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.

Comment 36

[Shih-Chiang Chien [:schien] (UTC+8) (use ni? plz)]

Attachment: Part 3 - send requestURL for Camera API

update according to comment 33, carry r+.

Comment 37

[Shih-Chiang Chien [:schien] (UTC+8) (use ni? plz)]

Attachment: Part 4 - support multiple requestURL with one content process

update according to comment 34 (including deleting empty process record), carry r+.

Comment 38

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

(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.

Comment 39

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

Comment on attachment 8338456 [details] [diff] [review]
Part 1 - carry requestURL property from content process

Review of attachment 8338456 [details] [diff] [review]:
-----------------------------------------------------------------

r=me

Comment 40

[Shih-Chiang Chien [:schien] (UTC+8) (use ni? plz)]

Attachment: Part 1 - carry requestURL property from content process

rebase to m-c tip, carry r+.

Comment 41

[Shih-Chiang Chien [:schien] (UTC+8) (use ni? plz)]

Attachment: Part 2 - send requestURL for gUM API

rebase to m-c tip, carry r+.

Comment 42

[Shih-Chiang Chien [:schien] (UTC+8) (use ni? plz)]

Attachment: Part 3 - send requestURL for Camera API

rebase to m-c tip, carry r+.

Comment 43

[Shih-Chiang Chien [:schien] (UTC+8) (use ni? plz)]

Attachment: Part 4 - support multiple requestURL with one content process

rebase to m-c tip, carry r+.

Comment 44

[Shih-Chiang Chien [:schien] (UTC+8) (use ni? plz)]

https://tbpl.mozilla.org/?tree=Try&rev=aa6323add4c0

Comment 45

[Shih-Chiang Chien [:schien] (UTC+8) (use ni? plz)]

Attachment: [b2g26] part 1- request-url-from-content-process.patch

rebase to b2g26

Comment 46

[Shih-Chiang Chien [:schien] (UTC+8) (use ni? plz)]

Attachment: [b2g26] part 2 - gum.patch

rebase to b2g26

Comment 47

[Shih-Chiang Chien [:schien] (UTC+8) (use ni? plz)]

Attachment: [b2g26] part 3 - camera.patch

rebase to b2g26

Comment 48

[Shih-Chiang Chien [:schien] (UTC+8) (use ni? plz)]

Attachment: [b2g26] part 4 - shell-js.patch

rebase to b2g26.

Comment 49

[Ryan VanderMeulen [:RyanVM]]

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

Comment 50

[Ryan VanderMeulen [:RyanVM]]

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?

Comment 51

[Ryan VanderMeulen [:RyanVM]]

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

Comment 52

[Shih-Chiang Chien [:schien] (UTC+8) (use ni? plz)]

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.

Comment 53

[Shih-Chiang Chien [:schien] (UTC+8) (use ni? plz)]

@Ryan, please backout my patch (see comment 52).

Comment 54

[Ryan VanderMeulen [:RyanVM]]

Can we just spotfix?

Comment 55

[Ryan VanderMeulen [:RyanVM]]

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

Comment 56

[Ryan VanderMeulen [:RyanVM]]

(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

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/b857284946e0

Comment 58

[Shih-Chiang Chien [:schien] (UTC+8) (use ni? plz)]

(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.

Comment 59

[Jason Smith [:jsmith]]

Verified on 12/12/2013 Buri 1.2 build - attached test case & comment 0 test case works fine.

Comment 60

[Al Billings [:abillings - ex-MoCo]]

Is this issue FirefoxOS only?

Comment 61

[Jason Smith [:jsmith]]

(In reply to Al Billings [:abillings] from comment #60)
> Is this issue FirefoxOS only?

Yes.

Comment 62

[Al Billings [:abillings - ex-MoCo]]

Thanks. That means we haven't shipped this problem anywhere yet, as far as Dan and I can tell.