1484026 - Add IPC Checks to Get/SetCookie methods

Status: RESOLVED FIXED

(Core :: Networking: Cookies, enhancement, P3)

Description

[Tom Ritter [:tjr]]

While reviewing the IPC for document.cookie, I used it as a guinea pig for where IPC layer checks could be inserted. 

I believe they need to be in CookieServiceParent::RecvPrepareCookieList which receives a Host and an OriginAttributes object from the content process, and looks up the cookies based on those values.

Inside of CookieServiceChild::GetCookieStringFromCookieHashTable there is at least one check that should become superfluous, and could be removed or turned into an Assert: if (cookie->IsSecure() && !isSecure),

Comment 1

[Alex Gaynor [:Alex_Gaynor]]

So, one way to structure this is as PCookieService currently does -- GetCookieString takes a host string, and looks things up for it. To make this fission-ipc-secure we would insert a check into RecvGetCookieString that verifies that the host is allowed for that pid.

However, an alternate way we could structure it is to add the host to the PCookieServiceParent constructor, and simply remove the host argument from GetCookieString. The child gets a PCookieServiceParent actor that only knows how to look things up for one host.

Comment 2

[Valentin Gosu [:valentin] (he/him)]

(In reply to Alex Gaynor [:Alex_Gaynor] from comment #1)
> So, one way to structure this is as PCookieService currently does --
> GetCookieString takes a host string, and looks things up for it. To make
> this fission-ipc-secure we would insert a check into RecvGetCookieString
> that verifies that the host is allowed for that pid.

For one thing, I think we're about to remove the sync GetCookieString anyway (bug 1483986).
Its use is preffed off anyway.

> However, an alternate way we could structure it is to add the host to the
> PCookieServiceParent constructor, and simply remove the host argument from
> GetCookieString. The child gets a PCookieServiceParent actor that only knows
> how to look things up for one host.

That's a really interesting approach. I think that could work. But to implement it we must be in a world where we only have one origin in each process.

Comment 3

[Alex Gaynor [:Alex_Gaynor]]

We could also provide one actor per origin-in-the-process to the child. The overhead for that might be higher than is acceptable though, I don't know off-hand how heavyweight child actors are (or how much refactoring would be required to make that work).

Comment 4

[Valentin Gosu [:valentin] (he/him)]

It's not how heavy they are, I assume not very. It's the way the nsICookieService works - it's a singleton; changing the API to allow for multiple instances of it would prove challenging.

Comment 5

[Tom Ritter [:tjr]]

Additionally, the async SetCookieString method needs to have checks on it's host/OriginAttributes also.

Comment 6

[Chris Peterson [:cpeterson]]

This bug is not a Fission MVP blocker.

Comment 7

[Andrea Marchesini [:baku]]

I fixed this issue as part of the CookieStore implementation. In case we want to accelerate, we can land the fix immediately without waiting for the entire CookieStore review process. Valentin, what do you think? The fix is here: https://phabricator.services.mozilla.com/D217734

Comment 8

[Valentin Gosu [:valentin] (he/him)]

I don't think this is super urgent. The cookieStore impl seems to have some momentum, so we can probably land it with bug 1475599.

Comment 9

[Andrea Marchesini [:baku]]

while working on CookieStore API, I implemented this extra IPC checks for Get/Set cookie methods. Currently we use IPC_FAIL only on nightly:
https://searchfox.org/mozilla-central/rev/9fa446ad77af13847a7da250135fc58b1a1bd5b9/netwerk/cookie/CookieServiceParent.cpp#311-315

I suggest removing the #ifdef bit.

Comment 10

[Andrea Marchesini [:baku]]

Attachment: Bug 1484026 - Enforce Cookie IPC checks in RecvSetCookies, r?valentin

Comment 11

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/26015048a84c
Enforce Cookie IPC checks in RecvSetCookies, r=valentin,cookie-reviewers

Comment 12

[Serban Stanca [:SerbanS]]

https://hg.mozilla.org/mozilla-central/rev/26015048a84c