491810 - Geolocation "cookie" isn't cleared at exit for network.cookie.lifetimePolicy == 2

Status: RESOLVED FIXED

(Firefox :: General, defect)

Description

[Simon Bünzli]

Geolocation's access_token are supposed to behave like a cookie. However, with network.cookie.lifetimePolicy set to 2 (clear all cookies at the end of the session), this nonetheless token persists.

Comment 1

[Doug Turner (:dougt)]

The token isn't clear that way by design.  To clear the token, you can do one if the following:

1) wait two weeks.  It will expire.
2) clear cookies
3) enter and exit private browsing mode.

Comment 2

[Simon Bünzli]

Would you mind pointing me to the design spec?

From http://dougt.wordpress.com/2009/04/30/geolocation-in-firefox-35-and-fennec/
> The access token basically acts like a two week cookie, and if you clear
> cookies in the browser, this value is deleted and a new one is used.

Note that I effectively _do_ clear cookies in the browser at every shutdown (resp. I tell it to ignore their two-week-in-the-future-expiry) which isn't honored by geolocation's access token.

BTW: What's that token needed for at all? If a new token gives me the same result as an old one, my guess would be that it's rather for Google's statistical tracking - which I pretty explicitly opted out of through the "Keep cookies ... until Firefox is closed" option (in the options' Privacy section).

Comment 3

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

Doug, I think we need to consider the user setting "clear cookies at shutdown" through the cookie options (which is the setting Simon's talking about) to be equivalent to removing all cookies.

Not sure I'd block on it, as there's still a way to get rid of it and the token only exists if a user chooses to use the geolocation information, and then that token is only again used at the next geolocation lookup, but we should expedite a fix.

Comment 4

[Doug Turner (:dougt)]

Hi Simon,

Sorry for the confusion.  When explaining to a layperson what this token is, it is sometime easier to say that it is a cookie because people more-or-less already understand what those do.  However, it is clear that people who are more informed, like yourself, see that this is a weak metaphor.

The way this is built, we do not use any cookies (rfc2109).  The connection to the network geolocation provider (eg google) strips most http headers. The access token is part of a json string that is sent over ssl to google.  Since there are no cookies, this value really isn't subject to the network.cookie.lifetimePolicy preference.

We could build in some support for this, but I don't think I would like to do that given that we aren't really using cookies.  Instead, there is a different way to clear cookies which many people already use.

In the Firefox UI there is a "Clear Recent History..." feature that allows you to delete your history/cache/cookies immediately or at shutdown.  Looking at this feature, we thought it was an overkill to add a new item just for geolocation.  When looking at where exactly to put this, we settled on using the "cookie" type:

http://mxr.mozilla.org/mozilla-central/source/browser/base/content/sanitize.js#155

I could consider adding network.cookie.lifetimePolicy == 2 because as you suggest you are using it with the intention of clearing this sort of token.  Out of curiosity, are you using the "Clear Recent History..." stuff at shutdown at all?

Thanks!

Comment 5

[Simon Bünzli]

(In reply to comment #4)
> The way this is built, we do not use any cookies (rfc2109).

On a side-note: Why not? Sure they can't be transmitted in http headers, but they could still be stored through the cookie manager (with e.g. an otherwise invalid "<geolocation>" host), couldn't they? Then you wouldn't need to spread your "do as if"s everywhere...

> The access token is part of a json string that is sent over ssl to google.

To repeat my question: Why do we do that at all?

> Out of curiosity, are you using the "Clear Recent History..." stuff at
> shutdown at all?

No (and I'm aware that sites could track me e.g. through History or the Cache, however the sites I usually frequent don't - so convenience trumps privacy).

Comment 6

[Doug Turner (:dougt)]

Attachment: patch v.1

Comment 7

[Doug Turner (:dougt)]

s/have/has

Comment 8

[Doug Turner (:dougt)]

> On a side-note: Why not? 

we don't use cookies because, and this is a guess, that Google using their Location Service to not only support Firefox, but to support devices that do not have http cookie support (eg maybe some handsets using some crap OS restrict setting cookies in their application).

> To repeat my question: Why do we do that at all?

The Access Token is used to protect against common attacks like DoS.

Comment 9

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

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

This look ok, provided you add a TODO to the comment saying that to be accurate, we should be clearing the token on browser shutdown, not every time the provider is shut down.

Or, at the very least, a comment describing what a "session" is for geolocation.

Comment 10

[Simon Bünzli]

(In reply to comment #8)
> we don't use cookies because, and this is a guess, that Google using their
> Location Service to not only support Firefox

My question actually didn't have anything to do with Google but just with how the access token was stored inside Firefox: Why in a pref and not through the cookies manager?

> The Access Token is used to protect against common attacks like DoS.

From what? DoS from web pages should be prevented by our own implementation (no need to send anything to Google, just enforce the limits client side) and DoS from an implementor won't be prevented by something the implementor can choose freely itself. Since I guess Google won't be blindly trusting implementors, won't they rather rely on IP addresses than access tokens?

Comment 11

[Doug Turner (:dougt)]

Hi Simon,
You are probably right in that it could have possibly been stored as a cookie even though it isn't a HTTP cookie.  I suppose that it was much easier to just use preferences similar to what we do with safe browsing than to attempt to use the cookie manager.  On the DoS stuff.  IP doesn't work very well for DoS style attacks when there are many people sitting behind a proxy.

Comment 12

[Doug Turner (:dougt)]

http://hg.mozilla.org/mozilla-central/rev/02cfcc7837a5

Marking fixed.

Comment 13

[Simon Bünzli]

(In reply to comment #11)
> I suppose that it was much easier to just use preferences similar to what
> we do with safe browsing than to attempt to use the cookie manager.

Considering all the additional code you've had to include to clear the access token, wouldn't it make sense to reconsider that easiness? ;-)

> IP doesn't work very well for DoS style attacks

I didn't say that IP addresses worked well - just that they're still more reliable than an access token (thus making the access token pointless from the server's point of view - except for statistical purposes).

Anyway, thanks for the speedy fix for this issue!

Comment 14

[Doug Turner (:dougt)]

Hi Simon, no problem on the speedy fix -- I totally appreciate your feedback and I think because if it, we have a better Firefox!

So, regarding why we didn't use the cookie manager -- We basically didn't want this access token to be sent ask a cookie when ever you visited the site in the browser.  For example, if you used Geolocation and we set this access token as a cookie, then when you visited the location provider's home page in the browser, we would then send the access token back to them.  This break the sandboxing we were hoping for as it uses the access token is for much more than its current limited purpose.  I hope this helps.

Comment 15

[Simon Bünzli]

(In reply to comment #14)
> We basically didn't want this access token to be sent ask a cookie when ever
> you visited the site in the browser.

That's why you'd have set the cookie's host e.g. to "<geolocation>" (which isn't a valid host name, so the cookie'd never actually be added to any http request).

Comment 16

[Doug Turner (:dougt)]

simon, if you like, file a seperate bug on this issue.  I think we both can agree that this bug can be solved without converting to using the cookie manager, that if we did convert to the cookie manager we would have to clean up alot of stuff, and we would also have to look for edge cases where the host might not be checked for being valid (e.g <geolocation> somehow match an actual host).  Thanks for the suggestion btw.

Comment 17

[Simon Bünzli]

(In reply to comment #16)
> this bug can be solved without converting to using the cookie manager,

This bug's already FIXED. I've filed bug 492043 for the more general issue.

Comment 18

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

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

a191=beltzner

Comment 19

[Doug Turner (:dougt)]

http://hg.mozilla.org/releases/mozilla-1.9.1/rev/61c7b51b829e

Comment 20

[Simon Bünzli]

Rhetorical question: Has this code path ever been tested?

Error: prefService.getBranch is not a function
Source: file:///C:/Programme/Mozilla%20Firefox/components/NetworkGeolocationProvider.js
Line: 176

Comment 21

[Simon Bünzli]

Attachment: follow-up fix

Comment 22

[Doug Turner (:dougt)]

Comment on attachment 377411 [details] [diff] [review]
follow-up fix

how embarrassing.

Comment 23

[Doug Turner (:dougt)]

http://hg.mozilla.org/mozilla-central/rev/216ddaba3328

Comment 24

[Simon Bünzli]

This also needs check-in on the 1.9.1 branch (I guess approval carries over, considering that this is pretty much the same patch as has already gone in).

Comment 25

[Nochum Sossonko [:Natch]]

Perhaps a unit test or at least a litmus test would be appropriate, let me know if you would like me to hack at it.

Comment 26

[Doug Turner (:dougt)]

@natch, that would be awesome.  also, we should add litmus tests for "clear recent history...", enter/exit private browsing mode, changing permissions via page info.  If you like, I can file separate bugs?

@zeniko, thanks again.  i will push to 1.9.1 tomorrow after I can verify the fix on the trunk.

Comment 27

[Nochum Sossonko [:Natch]]

Actually, since this deals with session cookies, this can't be tested automatically now since mochitests don't support restarting the browser atm. Should go in litmus though. I'll file another bug (or you can point me at the other bugs) to deal with testing pb and clear recent history as those can be tested automatically (if they aren't already).

Comment 28

[Doug Turner (:dougt)]

the test case follow bug is 493122.

simon, thanks for cleaning my mess up!

http://hg.mozilla.org/releases/mozilla-1.9.1/rev/50be518fe1c0

Comment 29

[Marcia Knous [:marcia]]

I have one Litmus test case already for the PB scenario: https://litmus.mozilla.org/show_test.cgi?id=9321

I will check to see if I have the other scenarios covered.

(In reply to comment #26)
> @natch, that would be awesome.  also, we should add litmus tests for "clear
> recent history...", enter/exit private browsing mode, changing permissions via
> page info.  If you like, I can file separate bugs?
> 
> @zeniko, thanks again.  i will push to 1.9.1 tomorrow after I can verify the
> fix on the trunk.

Comment 30

[Marcia Knous [:marcia]]

I created a Litmus test case for the Page Info scenario - https://litmus.mozilla.org/show_test.cgi?id=9585. There is also a generic test case which covers CRH - https://litmus.mozilla.org/show_test.cgi?id=9351. So I think we have the main areas covered here from a Litmus standpoint. Would be willing to hear other suggestions.