805616 - Double-setting cookies (once w/bad 3rd party check) under e10s

Status: RESOLVED DUPLICATE of bug 1049299

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

Description

[Jason Duell]

So in bug 570867 we deliberately avoided calling SetCookie in parent for e10s channels and called it on child process instead because 3rd party checks sometimes require the window object which lives on child.

Then in bug 558624 patch 4 (aka attachment 480186 [details] [diff] [review]) to "optimize" cookies we removed the check (so we now always set in parent), but didn't remove the SetCookie in child:

  http://mxr.mozilla.org/mozilla-central/source/netwerk/protocol/http/HttpChannelChild.cpp#282

The result:  we are now setting cookies twice, once in parent (with a bad 3rd party check that may return early/badly because no Window is found) and once in child.

This definitely breaks http-on-examine observers in the child (not sure there are any) who want to modify/block a cookie before it's set, as the cookie will be set on the parent before they are called.  I'm not sure if it has any other bad effects (the 3rd party check defaults to 3rdparty=true: is it bad to set a cookie as 3rd party then set it again with 3rdparty=false?  We might send out a bogus rejection notification.).

Anyway, it seems to me that we ought to restore mRemoteChannel and once again use it to not SetCookie on parent.

Comment 1

[Jason Duell]

I'm not seeing anything here that needs to block this for basecamp, but if anyone can think of a way that this messes up cookies more than I think it does, please comment and we can mark it blocking.

Comment 2

[Josh Matthews [:jdm]]

Attachment: Ensure e10s HTTP channels don't attempt to set cookies in the parent process.

I realized that testing this is going to be a complicated affair, since we can only do the third-party stuff in mochitest (I presume), wherein we have little to no e10s testing facilities. I figured I would at least put this up for your perusal; I'm a bit worried about the implications for redirect channels and whether they need special handing with regards to remoteness or not.

Comment 3

[Josh Matthews [:jdm]]

I've considered comment 0 further, and here's my analysis. The 3rd-party check in the parent is conservative and defaults to marking the cookie as foreign. That means that a cookie will only be set in the event that at least one of the following is true:
* forceEnableThirdPartyCookies is turned on
* gecko is configured to accept all cookies

Any other situations will cause a deny notification to occur; these have the potential of being spurious. With regards to the child http-on-examine-response observers, in bug 558624 we discussed breaking them, and comment 12 indicates that there's no expectation for them to work from that point onwards.

I don't believe there's any observable functional difference here, though, outside of the deny notifications. In no circumstance can the parent set a cookie when the child would deny it, and at worst a cookie could be observed being set unexpectedly if some child process raced direct cookie manager access against the incoming IPC HTTP messages.

Comment 4

[Josh Matthews [:jdm]]

Attachment: * * *

This took far longer to get working than it should have. Concerns about redirects still apply.

Comment 5

[Honza Bambas (:mayhemer)]

Comment on attachment 675689 [details] [diff] [review]
* * *

Review of attachment 675689 [details] [diff] [review]:
-----------------------------------------------------------------

::: netwerk/protocol/http/nsHttpChannel.h
@@ +342,5 @@
>      // the cache entry's expiration time. Otherwise, it is not(see bug 567360).
>      uint32_t                          mRequestTimeInitialized : 1;
>  
> +    // True if this channel is the parent side of an IPC channel.
> +    uint32_t                          mRemoteChannel : 1;

I thought we were already tracking this info on nsHttpChannel...

Comment 6

[Josh Matthews [:jdm]]

Not since http://hg.mozilla.org/mozilla-central/rev/7a1028b9d42f, as far as I know.

Comment 7

[Jason Duell]

Comment on attachment 675689 [details] [diff] [review]
* * *

Review of attachment 675689 [details] [diff] [review]:
-----------------------------------------------------------------

::: netwerk/protocol/http/HttpBaseChannel.cpp
@@ +1483,5 @@
> +  nsICookieService *cs = gHttpHandler->GetCookieService();
> +  if (cs) {
> +    cs->GetCookieStringFromHttp(mURI,
> +                                nullptr,
> +                                this, getter_Copies(cookie));

As discussed on IRC, the problem with this change is that GetCookieString winds up doing a blocking sync IPDL call to the parent to get cookies.  We don't want to do that for every HTTP channel.

I think the way forward here is to change our code so that we do the 3rd party check in the channel on the child, then ship that as a bool to the parent, and the cookie service will then call a new function, nsIHttpChannel.IsThirdParty, instead of using the mozIThirdPartyUtil directly.  That way we'll always do the the check on the child, where it belongs, but without the extra IPDL traffic.

re: the original bug here:  if I'm right about bug 806753, we can remove the code that sets/gets cookies in the child and just do it on the parent.

Comment 8

[Josh Matthews [:jdm]]

Attachment: Ensure third party checks for e10s HTTP channels occur in the child, but set all cookies in the parent process.

This passes all of the tests I have thrown at it.

Comment 9

[Jason Duell]

Comment on attachment 676660 [details] [diff] [review]
Ensure third party checks for e10s HTTP channels occur in the child, but set all cookies in the parent process.

Review of attachment 676660 [details] [diff] [review]:
-----------------------------------------------------------------

Patch looks almost ready: one fix and a few nits.

::: extensions/cookie/test/test_e10s_thirdparty.html
@@ +5,5 @@
> +  <script type="text/javascript" src="/tests/SimpleTest/SimpleTest.js"></script>
> +  <link rel="stylesheet" type="text/css" href="/tests/SimpleTest/test.css" />
> +</head>
> +<body>
> +  <siframe onload="finishUp()" mozbrowser="true" sid="testFrame"></iframe>

As explained on IRC, this 'siframe' is jdm's way of commenting out tags in his HTML.  Very confusing if you don't see the typo.  Remove it :)

@@ +12,5 @@
> +      SpecialPowers.addPermission("browser", true, document);
> +      SpecialPowers.pushPrefEnv({
> +        "set": [
> +          ["dom.ipc.browser_frames.oop_by_default", true],
> +          ["dom.mozBrowserFramesEnabled", true],

I'm trusting that this recipe for e10s mochitest setup was cribbed from an existing test that's already been reviewed?

@@ +44,5 @@
> +
> +      var iframe = document.createElement('iframe');
> +      iframe.mozbrowser = true;
> +      iframe.addEventListener("mozbrowserloadend", finishUp);
> +      iframe.src = "http://mochi.test:8888/tests/extensions/cookie/test/file_setcookie.sjs";

Test coverage needs expansion.  This just tests a 1st party cookie with the "reject foreign cookies" setting on.  We definitely also want to have an actual 3rd party cookie get rejected (and even more, have one get approved).  Pretty simple: just use one of the various other domains that mochitest supports to get a 3rd party cookie--it can be the same sjs script as the resource:

  https://developer.mozilla.org/en-US/docs/Mochitest#How_do_I_test_issues_which_only_show_up_when_tests_are_run_across_domains.3F

I'd also like to add a redirect test, if possible.  It looks like it ought to be just a matter of having the sjs file redirect to another sjs file which also sets a cookie.

If you want a prize, make the test table-driven so we can test all relevant permutations of 1) cookiebehavior; 2) 1st or 3rd party cookie; 3) redirected or not; and 4) whether 'forceAllow3rdPartyCookie' is set on channel.

::: netwerk/protocol/http/HttpChannelChild.cpp
@@ +1052,5 @@
>  
> +  // Determine whether the request is foreign. Failure is acceptable.
> +  bool isForeign = true;
> +  nsCOMPtr<mozIThirdPartyUtil> thirdParty = do_GetService(THIRDPARTYUTIL_CONTRACTID);
> +  thirdParty->IsThirdPartyChannel(this, mURI, &isForeign);

I'm not sure what to do with GetService of a service that should really never fail.  (nsCookieService fails Init if it can't get the 3rdpartyUtil.)  Maybe put "if (thirdparty)" in here just to be superstitious?

Bigger issue: When we do a redirect, we're not doing the 3rd party check on the child for the new channel.  It looks to me like we should do the same IsThirdPartyChannel check during HttpChannelChild::Redirect1Begin.  Logically I think it belongs in SetupReplacementChannel, so override that in HttpChannelChild, call the HttpBaseChannel version, and then do the 3rdparty check (and stuff it into a member variable).  Then send it across IPDL in Redirect2Verify and have the parent set it on the new channel. We don't need to care whether the new channel is the one that actually gets used or not (its harmless to set it if the redirect gets vetoed).

::: netwerk/protocol/http/nsIHttpChannelInternal.idl
@@ +161,5 @@
> +     * @ param overriddenValue the value to use if true.
> +     *
> +     * @see mozIThirdPartyUtil
> +     */
> +    boolean isThirdPartyOverridden(out boolean overriddenValue);

It's confusing to have both "forceAllowThirdPartyCookie" and "isThirdPartyOverridden" in the same IDL.   I thought of merging the two into one 'override' API, but that gets funky quickly, as we need to preserve 'forced' 3rd party across redirects, but need to toss and refresh the 'override'.

I think the easiest thing to do is just to pick a name that's less similar to 'force'.  I'm thinking

  boolean hasChildThirdPartyValue(out boolean childValue); 

I'm open to other naming ideas if you have any.

Comment 10

[Josh Matthews [:jdm]]

It occurs to me that fixing this could have positive perf implications for B2G, as it would eliminate an async child->parent message for each cookie received as part of an HTTP response.

Comment 11

[Brian Smith (:briansmith, :bsmith, use NEEDINFO?)]

This patch seems to be good progress in that direction of bug 828221.

Since you removed the call to SetCookie in HttpChannelChild, couldn't you also now move the SetCookie method from HttpBaseChannel back to nsHttpChannel?

Comment 12

[Jason Duell]

> eliminate an async child->parent message for each cookie

Sure, that's nice, though async msgs aren't that bad.   The security architecture win is probably bigger.  Either way let's do it.  Are you good with my comments on the last patch?

Comment 13

[Jason Duell]

jdm: ping?

Comment 14

[Josh Matthews [:jdm]]

I obviously haven't been working on this, and while it's on my todo list I'd be happy to mentor someone else through the process instead. This would not be good for someone new to Gecko code; experience writing both Firefox C++ code and xpcshell tests would be ideal. Updating the existing patch to compile on trunk and addressing the review in comment 9 are the tasks at hand.

Comment 15

[Chris Peterson [:cpeterson]]

Moving to M3 milestone. These bugs affect e10s dogfooding but Brad and Gavin did not think they were M2 blockers.

Comment 16

[Nick Lebedev [:nl]]

Attachment: Ensure third party checks for e10s HTTP channels occur in the child, but set all cookies in the parent process.

I would like to take this bug. I've update patch to build in the current version of mozilla-central. Is it possible to proceed working on this bug right now without fixing the 'depends on' bugs?

Comment 17

[Josh Matthews [:jdm]]

It does seem like implementing the proposal in bug 1042377 would solve half of this bug (ie. the cookie set in the parent would have the correct third-party-ness information available). It's probably worth doing that first.

Comment 18

[Josh Matthews [:jdm]]

It appears likely that bug 1042377 will end up fixing all the problems associated with this one, in a generally cleaner fashion than my old patch here. It's probably worth turning your attention elsewhere, but thanks for the effort!