1484026 - Add IPC Checks to Get/SetCookie methods

Status: NEW

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

Description

[Tom Ritter [:tjr] (OOTO until 4/30 at least)]

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] (OOTO until 4/30 at least)]

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.