Closed Bug 491810 Opened 10 years ago Closed 10 years ago

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

Categories

(Firefox :: General, defect)

3.5 Branch
defect
Not set

Tracking

()

RESOLVED FIXED
Firefox 3.6a1

People

(Reporter: zeniko, Assigned: dougt)

Details

(Keywords: fixed1.9.1, privacy)

Attachments

(2 files)

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.
Flags: blocking-firefox3.5?
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.
Status: NEW → RESOLVED
Closed: 10 years ago
Resolution: --- → INVALID
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).
Status: RESOLVED → REOPENED
Resolution: INVALID → ---
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.
Flags: wanted-firefox3.5+
Flags: blocking-firefox3.5?
Flags: blocking-firefox3.5-
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!
(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).
Attached patch patch v.1Splinter Review
Assignee: nobody → doug.turner
Attachment #376265 - Flags: review?(vladimir)
s/have/has
> 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 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.
Attachment #376265 - Flags: review?(vladimir) → review+
(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?
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.
http://hg.mozilla.org/mozilla-central/rev/02cfcc7837a5

Marking fixed.
Status: REOPENED → RESOLVED
Closed: 10 years ago10 years ago
Resolution: --- → FIXED
Attachment #376265 - Flags: approval1.9.1?
(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!
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.
(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).
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.
(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 on attachment 376265 [details] [diff] [review]
patch v.1

a191=beltzner
Attachment #376265 - Flags: approval1.9.1? → approval1.9.1+
Keywords: checkin-needed
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
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Attached patch follow-up fixSplinter Review
Attachment #377411 - Flags: review?(doug.turner)
Comment on attachment 377411 [details] [diff] [review]
follow-up fix

how embarrassing.
Attachment #377411 - Flags: review?(doug.turner) → review+
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).
Status: REOPENED → RESOLVED
Closed: 10 years ago10 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 3.6a1
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.
Flags: in-testsuite?
Flags: in-litmus?
@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.
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).
Flags: in-testsuite? → in-testsuite-
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
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.
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.
Flags: in-litmus? → in-litmus+
You need to log in before you can comment on or make changes to this bug.