813758 - Need additional security checks for the "geolocation" permission

Status: RESOLVED FIXED

(Core :: DOM: Geolocation, defect, P1)

Description

[Ben Turner (not reading bugmail, use the needinfo flag!)]

geolocation:
  prompt in parent (nsIContentPermissionPrompt), can be bypassed
  thereafter no security checks at all

Need some additional checks to ensure that geolocation data is not delivered to processes without the geolocation permission.

Comment 1

[Doug Turner (:dougt)]

Attachment: patch v.1

Comment 2

[Ben Turner (not reading bugmail, use the needinfo flag!)]

Comment on attachment 685308 [details] [diff] [review]
patch v.1

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

::: dom/ipc/ContentParent.cpp
@@ +1822,5 @@
>  
>  bool
>  ContentParent::RecvAddGeolocationListener()
>  {
> +  AssertAppProcessPermission(this, "geolocation");

This (and the others below) should be:

  if (!AssertAppProcessPermission(...)) {
    return false;
  }

Comment 3

[Doug Turner (:dougt)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/2cf359411d7d

Comment 4

[Ed Morley [:emorley]]

Backed out for Windows mochitest-other crashes due to:
{
3717 INFO TEST-START | chrome://mochitests/content/chrome/dom/ipc/tests/test_process_error.xul
Security problem: Content process does not have `geolocation' permission.  It will be killed.
}
(and a few other variations on the theme)

See:
https://tbpl.mozilla.org/?tree=Mozilla-Inbound&jobname=mochitest-other&tochange=051d3e0fa048&fromchange=a6b604916694

Backout:
https://hg.mozilla.org/integration/mozilla-inbound/rev/051d3e0fa048

Comment 5

[Doug Turner (:dougt)]

so, we call RecvRemoveGeolocationListener() from ContentParent::ActorDestroy.  :(

Comment 6

[Doug Turner (:dougt)]

Attachment: patch v.2

so, our IPC chrome tests basically do not have any permission bits set on them at all.  This is expected.

This patch protects RecvRemoveGeolocationListener from asserting if mGeolocationWatchID was never set.

Comment 7

[Doug Turner (:dougt)]

Attachment: patch v.2

Comment 8

[Andrew Overholt [:overholt]]

Setting priority based on triage discussions.  Feel free to decrease priority if you disagree.

Comment 9

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/ebaa0a4d35a3

Comment 10

[Doug Turner (:dougt)]

https://hg.mozilla.org/releases/mozilla-aurora/rev/c25efb1b7480
https://hg.mozilla.org/releases/mozilla-beta/rev/019f1b94cdd9

Comment 11

[Ryan VanderMeulen [:RyanVM]]

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

Comment 12

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/6fa6e55a93b2

Comment 13

[Ryan VanderMeulen [:RyanVM]]

(Comment 12 was a backout due to bug 817758)

Comment 14

[Ryan VanderMeulen [:RyanVM]]

Backed out on Aurora/Beta.
https://hg.mozilla.org/releases/mozilla-aurora/rev/f34cc14adb9a
https://hg.mozilla.org/releases/mozilla-beta/rev/e26ed80208c8

Comment 15

[Alex Keybl [:akeybl]]

Mass Modify: All un-milestoned, unresolved blocking-basecamp+ bugs are being moved into the C3 milestone. Note that the target milestone does not mean that these bugs can't be resolved prior to 12/10, rather C2 bugs should be prioritized ahead of C3 bugs.

Comment 16

[Dietrich Ayala (:dietrich)]

What's the situation here? Stuck on the branches, but not on m-c?

Comment 17

[Doug Turner (:dougt)]

Attachment: patch v.3

Comment 18

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Comment on attachment 694191 [details] [diff] [review]
patch v.3

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

r=me with that fixed.

::: dom/ipc/ContentParent.cpp
@@ +1873,5 @@
>  {
> +#ifdef MOZ_PERMISSIONS
> +  // We need to ensure that this permission has been set.
> +  // If it hasn't, just noop
> +  nsCOMPtr<nsIPermissionManager> pm = do_GetService(NS_PERMISSIONMANAGER_CONTRACTID);

You need to verify that the received principal isn't a complete lie too. To do this you can use code similar to [1].

So call ManagedPBrowserParent to get a list of PBrowserParent. Then on each of them call GetOwnOrContainingApp on it to get an nsIApplication. Then call GetLocalId on each nsIApplication (localid is just a different name for appid :( ). The appid of the principal needs to match the localId of one of the apps, otherwise the principal is a lie!

[1] http://mxr.mozilla.org/mozilla-central/source/dom/ipc/AppProcessPermissions.cpp#53

Comment 19

[Doug Turner (:dougt)]

sicking/bent - maybe we should do this as part of the principal serialization?

Comment 20

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Yes, I think we should!

Originally I had thought of doing something similar, but more complicated, but I think doing these checks on the principal deserializer would work great!

Comment 21

[Doug Turner (:dougt)]

Attachment: patch v.4

Comment 22

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Comment on attachment 696121 [details] [diff] [review]
patch v.4

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

r=me with that fixed.

::: dom/ipc/ContentParent.cpp
@@ +1934,5 @@
> +      }
> +  }
> +
> +  if (!found) {
> +    return true;

Call KillHard() here before returning.

Comment 23

[Mozilla RelEng Bot]

Try run for 39536974b3dd is complete.
Detailed breakdown of the results available here:
    https://tbpl.mozilla.org/?tree=Try&rev=39536974b3dd
Results (out of 315 total builds):
    success: 287
    warnings: 25
    failure: 3
Builds (or logs if builds failed) available at:
http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/[e-mail]-39536974b3dd

Comment 24

[Doug Turner (:dougt)]

https://hg.mozilla.org/mozilla-central/rev/8c3bd23a1567
https://hg.mozilla.org/releases/mozilla-aurora/rev/31d464929f7c
https://hg.mozilla.org/releases/mozilla-b2g18/rev/f73abd268bad

Comment 25

[Doug Turner (:dougt)]

Backed out due to X Orange.  Good news -- the patches work.  Bad news, some of the X tests need to be updated.

https://hg.mozilla.org/mozilla-central/rev/33064e13c3fd
https://hg.mozilla.org/releases/mozilla-aurora/rev/5652a8a40668
https://hg.mozilla.org/releases/mozilla-b2g18/rev/42aac34da660

Comment 26

[Mozilla RelEng Bot]

Try run for 39536974b3dd is complete.
Detailed breakdown of the results available here:
    https://tbpl.mozilla.org/?tree=Try&rev=39536974b3dd
Results (out of 316 total builds):
    success: 287
    warnings: 26
    failure: 3
Builds (or logs if builds failed) available at:
http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/[e-mail]-39536974b3dd

Comment 27

[Doug Turner (:dougt)]

https://hg.mozilla.org/mozilla-central/rev/13c24710fbac
https://hg.mozilla.org/releases/mozilla-b2g18/rev/a7088ed31774

Comment 28

[Mozilla RelEng Bot]

Try run for cc21ed6ca7e4 is complete.
Detailed breakdown of the results available here:
    https://tbpl.mozilla.org/?tree=Try&rev=cc21ed6ca7e4
Results (out of 308 total builds):
    exception: 2
    success: 287
    warnings: 17
    failure: 2
Builds (or logs if builds failed) available at:
http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/[e-mail]-cc21ed6ca7e4