491759 - Clear geolocation token when exiting private browsing

Status: RESOLVED FIXED

(Firefox :: Private Browsing, defect, P2)

Description

[:Gavin Sharp [email: [e-mail]]]

All the token could potentially reveal that you used a site that used geolocation, which is pretty minor, but it probably doesn't really hurt to also clear it in this case.

Comment 2

[Doug Turner (:dougt)]

Attachment: patch v.1

Comment 3

[:Gavin Sharp [email: [e-mail]]]

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

>diff --git a/browser/components/privatebrowsing/src/nsPrivateBrowsingService.js b/browser/components/privatebrowsing/src/nsPrivateBrowsingService.js

>   removeDataFromDomain: function PBS_removeDataFromDomain(aDomain)
>   {
>+
>+    // clear any and all network geolocation provider sessions
>+    var psvc = Cc["@mozilla.org/preferences-service;1"].getService(Ci.nsIPrefService);
>+    try {
>+        var branch = psvc.getBranch("geo.wifi.access_token.");
>+        branch.deleteBranch("");
>+    } catch (e) {}

Should be able to use:
this._prefs.deleteBranch("geo.wifi.access_token.");

instead, r=me with that.

Comment 4

[Nochum Sossonko [:Natch]]

(In reply to comment #2)
> Created an attachment (id=376097) [details]
> patch v.1

Does this actually remove it? I don't think that's the way to do it, I think you want to listen to "private-browsing" in the geolocation module and remove it there. Seems like I'm wrong though, given your r+...

Comment 5

[:Gavin Sharp [email: [e-mail]]]

Er, this patch is just in the wrong bug. It was meant for 491756.

Comment 6

[Doug Turner (:dougt)]

Attachment: patch v.1

Comment 7

[(no longer active)]

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

>diff --git a/dom/src/geolocation/NetworkGeolocationProvider.js b/dom/src/geolocation/NetworkGeolocationProvider.js
>+    observe: function (aSubject, aTopic, aData) {
>+        if (aTopic == "private-browsing") {

Please check aData here and only delete the pref branch if it's either "enter" or "exit".  In the future, we might have other values for aData representing other things, and I prefer to keep checking aData everywhere if possible for now.

Other than that, it looks great to me.

Comment 8

[Doug Turner (:dougt)]

Attachment: patch v.2

Comment 9

[Doug Turner (:dougt)]

Comment on attachment 376248 [details] [diff] [review]
patch v.2

incorporates ehsan.akhgari's comment.

Comment 10

[Doug Turner (:dougt)]

http://hg.mozilla.org/mozilla-central/rev/c1b04224d6cf

Comment 11

[Marcia Knous [:marcia]]

dougt: Can you suggest a good way for qa to verify this?  Thanks.

Comment 12

[Doug Turner (:dougt)]

yes.

1) new profile
2) enter into private browsing mode
3) do a geolocation request
4) edit private browsing mode
5) go to about:config
6) filter "geo.wifi.access_token"
7) ensure that nothing is in preferences

Comment 13

[Marcia Knous [:marcia]]

Verified fixed using Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; en-US; rv:1.9.2a1pre) Gecko/20090508 Minefield/3.6a1pre. I followed the steps in Comment 12 to verify.

Comment 14

[Mike Beltzner [:beltzner, not reading bugmail]]

Comment on attachment 376248 [details] [diff] [review]
patch v.2

a191=beltzner

Comment 15

[Doug Turner (:dougt)]

http://hg.mozilla.org/releases/mozilla-1.9.1/rev/22365d11b28c

Comment 16

[Marcia Knous [:marcia]]

Verified fixed on the 1.9.1 branch using Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; en-US; rv:1.9.1b5pre) Gecko/20090515 Shiretoko/3.5b5pre. Will add a litmus test case for this as well.

Comment 17

[Marcia Knous [:marcia]]

https://litmus.mozilla.org/show_test.cgi?id=7718 added to Litmus

Comment 18

[Henrik Skupin [:whimboo][⌚️UTC+2]]

This should better be an automated test. Request its addition.

Comment 19

[Nochum Sossonko [:Natch]]

adding tests for this in bug 493122.

Comment 20

[Nochum Sossonko [:Natch]]

Mozilla/5.0 (Windows; U; Windows NT 6.0; en-US; rv:1.9.2a1pre) Gecko/20090515 Minefield/3.6a1pre (official nightly)
(In reply to comment #16)
> Verified fixed on the 1.9.1 branch using Mozilla/5.0 (Macintosh; U; Intel Mac
> OS X 10.5; en-US; rv:1.9.1b5pre) Gecko/20090515 Shiretoko/3.5b5pre. Will add a
> litmus test case for this as well.

How was this verified, I cannot seem to verify this and in fact the automated test I'm trying to add to bug 493122 is failing. Note: you must check that the access_cookie is set before private browsing mode as sometimes there is no cookie at all.

Comment 21

[Henrik Skupin [:whimboo][⌚️UTC+2]]

As already said, I cannot verify this too. Even when the access token is created in PB mode. Leaving PB mode the token still exists. That happens on trunk and recent Shiretoko builds.

=> Reopen bug.

Comment 22

[Nochum Sossonko [:Natch]]

requesting blocking to get this sorted out.

Comment 23

[Doug Turner (:dougt)]

@whimboo, could you let me know if geolocation was running when you exited.  I think what you may be seeing is if geolocation shuts down before the PB exist, we don't get a chance to clean up.

Comment 24

[(no longer active)]

I don't think that this should be done in the geolocation service.  The best place for this code to live is inside the private browsing service, which would last for the whole duration of the browsing session.

Doug, can you please provide an alternate patch which undoes attachment 376248 [details] [diff] [review] and moves this code to nsPrivateBrowsingService instead?

Comment 25

[Simon Bünzli]

Attachment: keep clean-up code together

(In reply to comment #24)
> I don't think that this should be done in the geolocation service.

OTOH, I'm opposed to spreading even more geolocation code around our code base if we can help it.

Comment 26

[Simon Bünzli]

Attachment: keep clean-up code together even tighter

Comment 27

[(no longer active)]

Comment on attachment 378007 [details] [diff] [review]
keep clean-up code together even tighter

If I'm reading the code correctly, this still doesn't solve the case in which the following sequence of events happen:

1. A geolocation token is saved.
2. Browser is restarted.
3. Private browsing mode is started (without first visiting a page with geolocation service).
4. A page with geolocation is opened, the previously stored token should still be used.

Is that right?

Comment 28

[Simon Bünzli]

Attachment: keep clean-up code together and always ready

(In reply to comment #27)
You're indeed right. This patch now even also solves the case where you set cookies to expire at the end of the session, use Geolocation, crash Firefox, restart and quit (previously, this could have persisted the token as well).

Comment 29

[Simon Bünzli]

Comment on attachment 378012 [details] [diff] [review]
keep clean-up code together and always ready

>+    QueryInterface:    XPCOMUtils.generateQI([Ci.nsIObserver, Ci.nsISupportsWeakReference])

When applying this patch, please manually add a comma at the end of this line...

Comment 30

[Henrik Skupin [:whimboo][⌚️UTC+2]]

Simon, could you start a tryserver build? So I could check the behavior right before the review of Doug.

(In reply to comment #23)
> @whimboo, could you let me know if geolocation was running when you exited.  I
> think what you may be seeing is if geolocation shuts down before the PB exist,
> we don't get a chance to clean up.

Steps I did were the following:
1. New profile
2. Start Private Browsing mode
3. Open http://people.mozilla.org/~dougt/geo.html
4. Leave Private Browsing mode

After step 4 the access token was still existent. I believe this will be fixed by Simon's patch.

Comment 31

[(no longer active)]

Comment on attachment 378012 [details] [diff] [review]
keep clean-up code together and always ready

Looks great.  r=me on the privatebrowsing part.  :-)

Comment 32

[Doug Turner (:dougt)]

i like the approach a bunch better.  can we make it so that WifiGeoCleanupService doesn't get started each time the browser starts up, but instead only starts when geo starts?

Comment 33

[(no longer active)]

(In reply to comment #32)
> i like the approach a bunch better.  can we make it so that
> WifiGeoCleanupService doesn't get started each time the browser starts up, but
> instead only starts when geo starts?

Doesn't that cause problems such as comment 27 again?

Comment 34

[Doug Turner (:dougt)]

depends how it is done.  i am just worry about having a startup hit for something that might not at all be used.

Comment 35

[Doug Turner (:dougt)]

we could test to see if we are in private browsing mode and just clear the access token when we startup geolocation, i suppose.  This will resolve comment 27, correct?

Comment 36

[(no longer active)]

(In reply to comment #35)
> we could test to see if we are in private browsing mode and just clear the
> access token when we startup geolocation, i suppose.  This will resolve comment
> 27, correct?

Based on my understanding of the geolocation stuff, yes, it should.

Comment 37

[(no longer active)]

Actually that won't work.  In comment 27, replace step 3 with "Used Forget About This Site on a history entry" to get a set of steps which would lead to the geolocation token not being cleared when it should be.

Comment 38

[Mike Beltzner [:beltzner, not reading bugmail]]

I don't think this blocks, though it's definitely wanted.

Comment 39

[Nochum Sossonko [:Natch]]

The clearing *must* work whether geolocation has been used this session or not. Suppose a user uses the geo feature, shuts down the browser, restarts it and goes directly into pb mode, the access_token must be cleared then as well. So there must be a startup service for this, if only just to observe pb notifications. Alternatively, this can be handled directly in the pb service, as are other items (eg clearing nsIConsoleService).

Comment 40

[Simon Bünzli]

(In reply to comment #34)
> i am just worry about having a startup hit for
> something that might not at all be used.

The easiest solution would of course be to never store the token in the preferences but only inside the WifiGeoPositionProvider (which would be the same behavior as setting network.cookie.lifetimePolicy to 2).

Comment 41

[Doug Turner (:dougt)]

Lets see if this has any Ts impact by pushing to try.  If it does, maybe we should move this into the nsBrowserGlue that already gets started up?

Comment 42

[Doug Turner (:dougt)]

pushed to try; awaiting results.

gavin, can you take a look at this patch?

Comment 43

[Simon Bünzli]

(In reply to comment #42)
> pushed to try; awaiting results.

Note: In case you haven't read comment #29, you might want to push again...

(In reply to comment #41)
> should move this into the nsBrowserGlue that already gets started up?

nsBrowserGlue.js same as nsPrivateBrowsing.js are both Firefox specific, while your provider and the "private-browsing" notification itself are not (SeaMonkey might have its own implementation).

Comment 44

[Mike Beltzner [:beltzner, not reading bugmail]]

Comment on attachment 376248 [details] [diff] [review]
patch v.2

Revoking approval. We're cutting back on potential churn here. We can try again for 3.5.1

You can leave the non-working patch in 191 for now, but can't carry over approvals.

Comment 46

[Doug Turner (:dougt)]

I am more or less unhappy with having to startup a new service to monitor these events.  However, i really like how everything is managed in one place.

Instead of creating a startup listener, could we start this service the first time geo is use?  If so, I think everyone would be happy.

Comment 47

[Simon Bünzli]

(In reply to comment #46)
> Instead of creating a startup listener, could we start this service the first
> time geo is use?

Comment #39 explains why this isn't really an option. On Trunk, you really want to use conventional cookies (which already behave as expected, except for the missing chrome-only flag). And for the Branch, let me ask you again: why can't you just always clear that token when your service shuts down (which AFAICT it also does when enabling/disabling Private Browsing mode)?

Comment 48

[Doug Turner (:dougt)]

so, I am not sure that we should be starting this up exclusively to monitor something that might never be used.  I agree to wait on the chrome cookies.

The token is suppose to live for more or less a 2 week period in normal usage.  When a person enters or exists private browsing mode, getting a new token is fine.

Comment 49

[Simon Bünzli]

Then why don't you add the cleaning directly to nsPrivateBrowsingService.js to PBS_observe right after where we clear the Error Console?

(In reply to comment #48)
> When a person enters or exists private browsing mode, getting a new token is
> fine.

And the paranoid me keeps asking: And why isn't it otherwise fine? ;-)

Comment 50

[Doug Turner (:dougt)]

simon, that would be fine, i suppose.  I did like your idea of keeping all of our stuff together.

Right, paranoia is fine.  Basically, google uses this token for detection of DDoS style attacks.   I suppose we could restore the access token when existing out of private browsing mode if one existed.  However, I didn't think it was that big of a deal.

Comment 51

[:Gavin Sharp [email: [e-mail]]]

I don't see how the token helps against DDoS attacks - if it really is distributed, you'd get multiple IDs anyways, and it's unlikely that such attackers would use an unmodified Firefox client to perform the attack rather than a client that avoids sending the token altogether.

Comment 52

[Jonathan McPhie]

Hi there,

My name is Jonathan McPhie, I'm a PM on Google's location platform.

The client identifier that gets sent with a location request to Google Location Service is used to protect our service against common attacks such as DoS.  While we understand that this is not a perfect signal, we have found it to be very valuable in helping us to protect our service and users from malicious use.  

As a reminder, the client identifier is not used to identify individual users of the service, and Firefox resets this identifier periodically.

If you have further questions, please feel free to contact me.

Thanks,
Jonathan

Comment 53

[Nochum Sossonko [:Natch]]

This got lost...

Comment 54

[Doug Turner (:dougt)]

natch, for the trunk, i think we want to wait for chrome-only cookies.  We should mark this bug dependent on that work. Anyone have a bug number?

Comment 55

[(no longer active)]

Comment on attachment 378012 [details] [diff] [review]
keep clean-up code together and always ready

I think we need to land this patch on trunk, get it baked and land it on 1.9.2, and then file a new bug to revert this change from trunk and use a chrome cookie for implementing the token on trunk.  Doug, what do you think?

Comment 56

[Doug Turner (:dougt)]

ehsan, i would rather just do the chrome-only cookies approach.  is that something we can do for 3.6?

Comment 57

[(no longer active)]

(In reply to comment #56)
> ehsan, i would rather just do the chrome-only cookies approach.  is that
> something we can do for 3.6?

I'm not sure, Dan is the person to ask here.

Comment 58

[[e-mail]]

Pretty late in the game for 3.6, and I don't have cycles to put into it atm - but if someone picked it up, I could certainly help out.

Comment 59

[(no longer active)]

Doug, bug 511933 was minused as a blocker for 3.6, so I think we must take the less ideal approach in attachment 378012 [details] [diff] [review] for 3.6.  What do you think?

Comment 60

[Doug Turner (:dougt)]

Ehsan, in comment #34 i mentioned that I am worried about startup perf.  Loading another component that may never be used doesn't sound like the best idea.  Do we have any data that shows that this doesn't hurt anyone (esp. mobile)?

Comment 61

[(no longer active)]

(In reply to comment #60)
> Ehsan, in comment #34 i mentioned that I am worried about startup perf. 
> Loading another component that may never be used doesn't sound like the best
> idea.  Do we have any data that shows that this doesn't hurt anyone (esp.
> mobile)?

You pushed a try server build in comment 42, what results did you get?

Comment 62

[Nochum Sossonko [:Natch]]

As an interim fix it should be possible to just deleteBranch in the pb service (since it's already an app-startup service) here:

http://mxr.mozilla.org/mozilla-central/source/browser/components/privatebrowsing/src/nsPrivateBrowsingService.js#356

Comment 63

[(no longer active)]

(In reply to comment #62)
> As an interim fix it should be possible to just deleteBranch in the pb service
> (since it's already an app-startup service) here:
> 
> http://mxr.mozilla.org/mozilla-central/source/browser/components/privatebrowsing/src/nsPrivateBrowsingService.js#356

Hmm, I'm fine with taking that fix for 3.6, and then reverting it when chrome cookies are available.  Doug, what do you think?

Natch: if Doug feels comfortable with it as well, would you mind writing a quick patch for it with a quick review from me guaranteed? :-)

Comment 64

[Doug Turner (:dougt)]

sounds like a fine approach.

Comment 65

[[e-mail]]

If you guys could file a bug for changing this behavior if & when we get chrome-only cookies, and make it dependent on the chrome-only bug, that would be awesome.

Comment 66

[(no longer active)]

Filed bug 524790.

Comment 67

[Nochum Sossonko [:Natch]]

Attachment: proposed patch w/test

This is the proposed patch with a test.

The observer is being removed from the geolocation module so there's no need for it to implement nsISupportsWeakRef

Comment 68

[Nochum Sossonko [:Natch]]

Comment on attachment 408703 [details] [diff] [review]
proposed patch w/test

Keep forgetting Ehsan's new bug mail address ;)

Comment 69

[(no longer active)]

Comment on attachment 408703 [details] [diff] [review]
proposed patch w/test

The patch looks good to me overall, but I have a few comments.

>diff --git a/browser/components/privatebrowsing/src/nsPrivateBrowsingService.js b/browser/components/privatebrowsing/src/nsPrivateBrowsingService.js
>--- a/browser/components/privatebrowsing/src/nsPrivateBrowsingService.js
>+++ b/browser/components/privatebrowsing/src/nsPrivateBrowsingService.js
>@@ -360,16 +360,20 @@ PrivateBrowsingService.prototype = {
>+        try {
>+          this.__prefs.deleteBranch("geo.wifi.access_token.");

s/__prefs/_prefs/

>+        } catch (ex) {}
>+


>diff --git a/dom/tests/mochitest/geolocation/test_geoPrivateBrowsing.html b/dom/tests/mochitest/geolocation/test_geoPrivateBrowsing.html

Please move this test to the privatebrowsing module.

Also, is there any reason why you wrote this as a mochitest?  I think it could easily be converted into an xpcshell test, which is quicker to run and hence preferred if the test doesn't touch the UI.

Comment 70

[Nochum Sossonko [:Natch]]

Attachment: address comments [checkin: comment 72]

Made the test an xpcshell test, never really done those before, but works just as well.

Comment 71

[(no longer active)]

Comment on attachment 409960 [details] [diff] [review]
address comments [checkin: comment 72]

>diff --git a/browser/components/privatebrowsing/test/unit/test_geoClearCookie.js b/browser/components/privatebrowsing/test/unit/test_geoClearCookie.js
>+ * ***** END LICENSE BLOCK ***** */

Please add a short comment here specifying what this test does (see the other unit tests in that directory for samples).

>+const accessToken = '{"location":{"latitude":51.5090332,"longitude":-0.1212726,"accuracy":150.0},"access_token":"2:jVhRZJ-j6PiRchH_:RGMrR0W1BiwdZs12"}'
>+function run_test() {

Something that we do in PB tests is to run each test twice, once with the js PB service and once with the C++ wrapper.  For this test, all you need to do is to rename run_test to run_test_on_service, use PRIVATEBROWSING_CONTRACT_ID instead of "@mozilla.org/privatebrowsing;1", and define a run_tests function like this:


// Support running tests on both the service itself and its wrapper
function run_test() {
  run_test_on_all_services();
}

Please see test_transition_nooffline.js for a real sample.

>+  if (!("@mozilla.org/privatebrowsing;1" in Components.classes)) {
>+    do_check_true(true);
>+    return;
>+  }

No need to do this, because if we're in this directory, then the PB service should exist (and we have tests which make sure it does!).

>+  var prefBranch = Components.classes["@mozilla.org/preferences-service;1"]
>+                             .getService(Components.interfaces.nsIPrefBranch2);
>+  var pb = Components.classes["@mozilla.org/privatebrowsing;1"]
>+                     .getService(Components.interfaces.nsIPrivateBrowsingService);

Use PRIVATEBROWSING_CONTRACT_ID like I said above, and also please stick to Cc and Ci.  These are all just nits to make this test look more like the rest of pb test suite.  :-)

r=me with the above nits addressed.

The rest should be a very quick and easy review for Doug.  Thanks for the patch, Natch!

Comment 72

[(no longer active)]

http://hg.mozilla.org/mozilla-central/rev/c19d7df7b28a

Comment 73

[Nochum Sossonko [:Natch]]

Ehsan, I didn't get a chance to address your nits from comment 71, and the cset you pushed doesn't seem to address them either...

Comment 74

[(no longer active)]

(In reply to comment #73)
> Ehsan, I didn't get a chance to address your nits from comment 71, and the cset
> you pushed doesn't seem to address them either...

Oh, I'm so sorry I didn't pay enough attention...  Could you please attach a patch which addresses the nits based on the current trunk?  I'll push it and then push both patches combined on 1.9.2.

Comment 75

[Nochum Sossonko [:Natch]]

Attachment: update to comments

Done.

Comment 76

[(no longer active)]

Comment on attachment 411131 [details] [diff] [review]
update to comments

r=me.  Thanks!

Comment 77

[(no longer active)]

Landed the test fixes as http://hg.mozilla.org/mozilla-central/rev/53dfe82e7fff.

Comment 78

[(no longer active)]

Pushed both patches combined as http://hg.mozilla.org/releases/mozilla-1.9.2/rev/14191c0ab88d.

Comment 79

[Nochum Sossonko [:Natch]]

Attachment: refreshed for 191

Updated roll-up patch for 1.9.1

Comment 80

[Daniel Veditz [:dveditz]]

Comment on attachment 411233 [details] [diff] [review]
refreshed for 191

Would like to see more testing/baking with nightly users, will have to catch this one next time.

Comment 81

[Daniel Veditz [:dveditz]]

Comment on attachment 411233 [details] [diff] [review]
refreshed for 191

Approved for 1.9.1.8, a=dveditz for release-drivers

Comment 82

[(no longer active)]

Landed on 1.9.1: http://hg.mozilla.org/releases/mozilla-1.9.1/rev/918f199fc1b2