1610371 - Exceptions of cookies, must only apply to the main domain, must not include third parties.

Status: RESOLVED FIXED

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

Description

[Iceberg]

User Agent: Mozilla/5.0 (X11; Linux i686; rv:73.0) Gecko/20100101 Firefox/73.0

Steps to reproduce:

Set: block All third-party cookies (or All cookies).
Allow a site by placing it as an exception in managing permissions.

Actual results:

The main site, the one we have put in exceptions, and all third party sites related to it (dozens of third parties in some cases) are authorized to save their cookies.

Expected results:

This should not be the behavior of this setting.
Only the main site must be authorized to save cookies. Only the site present in the exceptions and no third party.

Comment 1

[Andy Grover [:grover]]

Michal can you reproduce this?

Comment 2

[Michal Novotny [:michal]]

I can reproduce it. I've blocked all cookies, added an exception for one domain and cookies for more domains were stored. Honza, you're cookie peer. Can you have a look?

Comment 3

[Honza Bambas (:mayhemer)]

(In reply to Michal Novotny [:michal] from comment #2)

I can reproduce it. I've blocked all cookies, added an exception for one domain and cookies for more domains were stored. Honza, you're cookie peer. Can you have a look?

I need more info about how you reproduced this. I built a simple local web test on two different domains (1P doc with a 3P iframe both setting a cookie) with the 1P domain allowed to store, but blocking all other cookies. the 3P domain served iframe doesn't store anything unless I allow all cookies.

Comment 4

[Iceberg]

(In reply to Honza Bambas (:mayhemer) from comment #3)

I need more info about how you reproduced this. I built a simple local web test on two different domains (1P doc with a 3P iframe both setting a cookie) with the 1P domain allowed to store, but blocking all other cookies. the 3P domain served iframe doesn't store anything unless I allow all cookies.

Excuse me, but are not so many complications necessary to have a confirmation of this bug.
Simply, for example:

Set: block all cookies.
Set: allow site https://www.tim.it/ in managing permissions.
Visit site: https://www.tim.it/

Result: cookies have been accepted and saved by:
tim.it [OK]
blue.mynsystem.com [NOT OK]
demdex.net [NOT OK]

Comment 5

[Honza Bambas (:mayhemer)]

This is exactly the example I needed, thanks.

I'm not that deeply familiar with these parts of the code, but what happens for e.g. the https://dpm.demdex.net/id?... request is the following (m-c@b0f071e99da0):

 	xul.dll!mozilla::net::LoadInfo::FindPrincipalToInherit(0x000001ff65fb4050) Line 674	C++
 	xul.dll!nsScriptSecurityManager::GetChannelResultPrincipal(0x000001ff65fb4050, 0x0000000100bfd240, false) Line 318	C++
 	xul.dll!nsScriptSecurityManager::GetChannelResultPrincipal(0x000001ff65fb4050, 0x0000000100bfd240) Line 232	C++
 	xul.dll!mozilla::AntiTrackingCommon::IsFirstPartyStorageAccessGrantedFor(0x000001ff65fb4050, 0x000001ff65fbd100, 0x0000000100bfd67c) Line 1737	C++
 	xul.dll!ThirdPartyUtil::AnalyzeChannel(0x000001ff65fb4050, false, 0x000001ff65fbd100, 0x0000000000000000, 0x0000000100bfd67c) Line 494	C++
 	xul.dll!nsCookieService::SetCookieStringCommon(0x000001ff65fbd100, {...}, {...}, 0x000001ff65fb4050, true) Line 2139	C++

where in nsScriptSecurityManager::GetChannelResultPrincipal the forceInherit = loadInfo->GetForceInheritPrincipal(); is true which makes nsScriptSecurityManager::GetChannelResultPrincipal return channel's mLoadInfo->mTriggeringPrincipal instead of the content principal of the channel itself. That principal is then passed to CheckCookiePermissionForPrincipal which returns 1 == nsICookiePermission::ACCESS_ALLOW, because we check against the principal for which we have the exception set. This then sets the ThirdPartyAnalysis::IsFirstPartyStorageAccessGranted flag back in ThirdPartyUtil::AnalyzeChannel that makes us later in nsCookieService::CheckPrefs, called from nsCookieService::SetCookieStringInternal, accept (or better say not reject) the cookie at this line, we don't pass the condition because aFirstPartyStorageAccessGranted is true:

  // No cookies allowed if this request comes from a tracker, in a 3rd party
  // context, when anti-tracking protection is enabled and when we don't have
  // access to the first-party cookie jar.
  if (aIsForeign && aIsTrackingResource && !aFirstPartyStorageAccessGranted &&
      aCookieSettings->GetRejectThirdPartyTrackers()) {
    ...
    return STATUS_REJECTED;
  }

Inheritance is not clear to me. The request that stores the cookie is a CORS XHR with allowed credentials:

request:

GET
Host: dpm.demdex.net
Referer: https://www.tim.it/
Content-Type: application/x-www-form-urlencoded
Origin: https://www.tim.it

response:

Access-Control-Allow-Credentials: true
Access-Control-Allow-Origin: https://www.tim.it
Set-Cookie: demdex=73402411689983527281636480407948318677;Path=/;Domain=.demdex.net;Expires=Sat, 08-Aug-2020 15:40:35 GMT;Max-Age=15552000;Secure;SameSite=None

Ehsan or Baku, can you please push me a bit forward with what is happening here? I'd like to grasp the understanding here a bit more. I'm not even sure if this is an actual bug or not.

Comment 6

[(no longer active)]

Hmm, it may not be the correct thing to do to use the channel result principal here at all (that is subject to sandboxing and inheriting rules). I wonder if we should directly use the channel URI principal instead... This may be a regression from bug 1480780 which as far as I can tell is when we started to use the channel result principal for checking the cookie permission.

Comment 7

[Andrea Marchesini [:baku]]

Yes, it seems a good reason. Honza, are you going to work on this issue? Otherwise I can take it.

Comment 8

[Honza Bambas (:mayhemer)]

(In reply to Andrea Marchesini [:baku] from comment #7)

Yes, it seems a good reason. Honza, are you going to work on this issue? Otherwise I can take it.

Yes, I actually have a functioning try run. I believe we should have a test for this as well, that is the tricky part here.

https://treeherder.mozilla.org/#/jobs?repo=try&revision=0665c25af9a47f258cbb9fd877234b69065f357e

Comment 9

[Honza Bambas (:mayhemer)]

Attachment: Bug 1610371 - Use ChannelURIPrincipal instead of ChannelResultPrincipal for first-party-storage allowance decision, r=ehsan

Comment 10

[Honza Bambas (:mayhemer)]

Attachment: Bug 1610371 - Test, r=ehsan

Comment 11

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/1d8888515502
Use ChannelURIPrincipal instead of ChannelResultPrincipal for first-party-storage allowance decision, r=Ehsan

Comment 12

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/e8f0380d2c95
Test, r=Ehsan

Comment 13

[Razvan Maries]

https://hg.mozilla.org/mozilla-central/rev/1d8888515502
https://hg.mozilla.org/mozilla-central/rev/e8f0380d2c95

Comment 14

[Underpass]

Excuse me, don't you think that this bug is serious enough to have it fixed even in ESR, Fx 73 and Fx 74?

Thanks.

Comment 15

[Honza Bambas (:mayhemer)]

Let it ride. This needs baking on Nightly and there is no point uplifting to Beta now (3 weeks to merge, 4 to release). Not sure this is ESR-qualified, but could be possibly considered, if found not causing any other regressions.