Closed Bug 1026538 Opened 10 years ago Closed 8 years ago

Cookie Manager can't delete cookies with appid != 0 [sandboxed cookies]

Categories

(Core :: Networking: Cookies, defect)

defect
Not set
normal

Tracking

()

RESOLVED WONTFIX

People

(Reporter: gcp, Unassigned)

References

Details

(Whiteboard: [necko-backlog])

https://bugzilla.mozilla.org/show_bug.cgi?id=1008706#c22

SafeBrowsing used a special sandboxed cookie with appid=-2. The cookie management window shows it, and if you try to delete it, it appears to work, but the cookie isn't actually deleted.
Is this a frontend problem (in the Firefox UI) or a backend problem (in how cookies are handled in general)? If the latter, the bug probably belongs in Core::Networking:Cookies
Someone will have to debug it to find out.
In the interests of helping anyone else who had the same problem as me but didn't know how to search - I raised a bug 1064219 before it became apparent this bug tracker was more applicable.
Flags: needinfo?(gijskruitbosch+bugs)
This is a cookie service bug; it hardcodes the default app ID in its remove() call which then silently fails.

http://mxr.mozilla.org/mozilla-central/source/netwerk/cookie/nsCookieService.cpp#2093
Component: Preferences → Networking: Cookies
Flags: needinfo?(gijskruitbosch+bugs)
Product: Firefox → Core
Jason, is there a particular reason cookies are not aware of their own appId/inBrowserElement status? It seems this is all kept in the same db (result) row (I think? Maybe I'm misreading the code...), but the only thing aware of it is the cookiekey which we use to select, which is "just" set to the default in many places in the cookie service.

While we could expose the more elaborate remove() version through IDL, that wouldn't be enough to be able to change the frontend call into remove() in order to fix this bug, because the frontend call has no idea about the appId and inBrowserElement flags (see also e.g. http://hg.mozilla.org/mozilla-central/annotate/4d1793da0b96/netwerk/cookie/nsCookieService.cpp#l4080 which faces a similar issue, although at least it knows the appId).
Flags: needinfo?(jduell.mcbugs)
SafeBrowsing used a special sandboxed cookie with appid=-2

appID's are unsigned ints.  (I don't think that breaks anything here, but just be aware that it's gonna get converted to the unsigned value).

So I think the answer here now is to either 1) stop showing the SafeBrowsing cookie, i.e. change the enumerator to only return cookies with appID==0 (note: expect the tinfoil hat and google-rules-mozilla conspiracy theorists to pipe up), or 2) Add some APIs to nsICookie2/nsICookieManager2 that allow you to get the appID for a cookie and to call delete with a specific appID/inBrowser.

Some explanation of the architecture here:

When we implemented appID stuff for cookies we assumed that all parent cookieSvc calls would have appID=0.  We also exported only a subset of nsICookieService to the child process, and only those calls are smart about requiring/getting an appID for a cookie.

The point of having appId/inBrowser info in the cookie database is so that there are separate logical "namespaces" (aka "cookie jars") for cookies.  So the same yahoo.com foo=bar1|bar2|bar3 can exist in different apps and have a different value.  This is why we don't want a cookie removal call without an appID to purge any old "foo" cookie from the database.

Getting the appID w/o breaking existing addons, etc, is tricky.  For calls that pass in an nsIChannel, we get the appID from the channel's nsILoadContext.  That was enough for B2G (at least so far), and we haven't until now found the need to allow addons and other main process code to pass in an appID when removing/setting, etc cookies.

We started to talk about how best to change the API for nsICookieManager in bug 777620.  That didn't go anywhere.  Off the top of my head I'd think we might just want to add a completely separate API that knows about appId/inBrowser, and deprecate the old APIs eventually.

There are security concerns with allowing random code to delete cookies with arbitrary appIDs--not something we'd want to allow in the child w/o some security checks.  But should be OK in the parent.
Flags: needinfo?(jduell.mcbugs)
And there's also issues here with being in PB mode: see bug 864150
Oh, and in case it's not obvious why safebrowsing cookies use a different appId than 0: we decided we don't want Google to see user's other google.com cookies when getting safebrowsing info.  See bug 368255.
(In reply to Jason Duell [:jduell] (needinfo? me) from comment #9)
> Oh, and in case it's not obvious why safebrowsing cookies use a different
> appId than 0: we decided we don't want Google to see user's other google.com
> cookies when getting safebrowsing info.  See bug 368255.

I always wonder who is "we" in sentences like this.

Thanks for mentioning bug 368255. It seems that I predicted there (bug 368255 comment #32 footnote [1]) exactly this problem almost 6 years ago...
>I always wonder who is "we" in sentences like this.

My understanding from reading the bugs (which you can do, too) is that it was eventually implemented to make 3rd party cookie blocking not automatically allow all Google cookies if you have SafeBrowsing enabled: https://bugzilla.mozilla.org/show_bug.cgi?id=368255#c57

If this bug is too hard to fix then perhaps as an alternate solution SafeBrowsing could delete its cookie if it notices it has been disabled.
Status: NEW → RESOLVED
Closed: 8 years ago
Resolution: --- → WONTFIX
Why is this wontfix, patrick? This isn't about 'ask me every time', it's specifically about the cookie manager UI that we provide through prefs.
Flags: needinfo?(mcmanus)
what is the appid != 0 relevance outside of b2g?
Flags: needinfo?(mcmanus)
(In reply to Patrick McManus [:mcmanus] from comment #16)
> what is the appid != 0 relevance outside of b2g?

It's used for sandboxed cookies. (I personally only know of SafeBrowsing as a user of those: sending specific google.com cookies for SafeBrowsing operation that don't reveal the "real" Google cookie of the user).

These cookies can't currently be removed due to this bug.
Flags: needinfo?(mcmanus)
(In reply to Gian-Carlo Pascutto [:gcp] from comment #17)
> (In reply to Patrick McManus [:mcmanus] from comment #16)
> > what is the appid != 0 relevance outside of b2g?
> 
> It's used for sandboxed cookies. (I personally only know of SafeBrowsing as

didn't understand that. thanks!
Status: RESOLVED → REOPENED
Flags: needinfo?(mcmanus)
Resolution: WONTFIX → ---
Whiteboard: [necko-backlog]
Summary: Cookie Manager can't delete cookies with appid != 0 → Cookie Manager can't delete cookies with appid != 0 [sandboxed cookies]
Sandboxing is now done via originAttributes rather than appId (Bug 1165267) so this isn't an issue anymore. The sandboxed safebrowsing NID cookie (formerly PREF) can be blocked and deleted in the Cookie Manager like a normal cookie so this bug can be marked as resolved.
Flags: needinfo?(mcmanus)
thanks kestrel
Status: REOPENED → RESOLVED
Closed: 8 years ago8 years ago
Flags: needinfo?(mcmanus)
Resolution: --- → WONTFIX
You need to log in before you can comment on or make changes to this bug.