This is an automated email from the git hooks/post-receive script.
pierov pushed a commit to branch tor-browser-102.2.0esr-12.0-1 in repository tor-browser.
commit bb8996196e4d1f2dcc8302aa218e55d5a1a18a0e Author: Tom Schuster evilpies@gmail.com AuthorDate: Wed Jun 15 14:51:16 2022 +0000
Bug 1763073 - Add telemetry (and messaging) for SameSite cookies blocked due to redirects. r=freddyb,dveditz a=pascalc
Differential Revision: https://phabricator.services.mozilla.com/D143034 --- netwerk/cookie/Cookie.h | 4 + netwerk/cookie/CookieCommons.cpp | 7 +- netwerk/cookie/CookieCommons.h | 7 +- netwerk/cookie/CookieService.cpp | 112 +++++++++++++++++++++------ netwerk/cookie/CookieService.h | 4 +- netwerk/cookie/CookieServiceChild.cpp | 7 +- netwerk/cookie/CookieServiceParent.cpp | 15 ++-- netwerk/cookie/CookieServiceParent.h | 3 +- netwerk/cookie/PCookieService.ipdl | 1 + netwerk/locales/en-US/necko.properties | 3 + toolkit/components/telemetry/Histograms.json | 11 +++ 11 files changed, 138 insertions(+), 36 deletions(-)
diff --git a/netwerk/cookie/Cookie.h b/netwerk/cookie/Cookie.h index 51d9d1673c703..890589c1cf5ae 100644 --- a/netwerk/cookie/Cookie.h +++ b/netwerk/cookie/Cookie.h @@ -84,6 +84,10 @@ class Cookie final : public nsICookie { } inline int32_t SameSite() const { return mData.sameSite(); } inline int32_t RawSameSite() const { return mData.rawSameSite(); } + inline bool IsDefaultSameSite() const { + return SameSite() == nsICookie::SAMESITE_LAX && + RawSameSite() == nsICookie::SAMESITE_NONE; + } inline uint8_t SchemeMap() const { return mData.schemeMap(); }
// setters diff --git a/netwerk/cookie/CookieCommons.cpp b/netwerk/cookie/CookieCommons.cpp index 5c6f6933b718a..7ecbadef868b0 100644 --- a/netwerk/cookie/CookieCommons.cpp +++ b/netwerk/cookie/CookieCommons.cpp @@ -519,7 +519,10 @@ bool IsSameSiteSchemeEqual(const nsACString& aFirstScheme, return aFirstScheme.Equals(aSecondScheme); }
-bool CookieCommons::IsSameSiteForeign(nsIChannel* aChannel, nsIURI* aHostURI) { +bool CookieCommons::IsSameSiteForeign(nsIChannel* aChannel, nsIURI* aHostURI, + bool* aHadCrossSiteRedirects) { + *aHadCrossSiteRedirects = false; + if (!aChannel) { return false; } @@ -596,6 +599,7 @@ bool CookieCommons::IsSameSiteForeign(nsIChannel* aChannel, nsIURI* aHostURI) { rv = redirectPrincipal->IsThirdPartyChannel(aChannel, &isForeign); // if at any point we encounter a cross-origin redirect we can return. if (NS_FAILED(rv) || isForeign) { + *aHadCrossSiteRedirects = true; return true; }
@@ -604,6 +608,7 @@ bool CookieCommons::IsSameSiteForeign(nsIChannel* aChannel, nsIURI* aHostURI) { if (!IsSameSiteSchemeEqual(redirectScheme, hostScheme)) { // If the two schemes are not of the same http(s) scheme then we // consider the request as foreign. + *aHadCrossSiteRedirects = true; return true; } } diff --git a/netwerk/cookie/CookieCommons.h b/netwerk/cookie/CookieCommons.h index aaecfa1a2dd45..b7b338a623aa8 100644 --- a/netwerk/cookie/CookieCommons.h +++ b/netwerk/cookie/CookieCommons.h @@ -127,8 +127,11 @@ class CookieCommons final {
// Returns true if the channel is a foreign with respect to the host-uri. // For loads of TYPE_DOCUMENT, this function returns true if it's a cross - // origin navigation. - static bool IsSameSiteForeign(nsIChannel* aChannel, nsIURI* aHostURI); + // site navigation. + // `aHadCrossSiteRedirects` will be true iff the channel had a cross-site + // redirect before the final URI. + static bool IsSameSiteForeign(nsIChannel* aChannel, nsIURI* aHostURI, + bool* aHadCrossSiteRedirects); };
} // namespace net diff --git a/netwerk/cookie/CookieService.cpp b/netwerk/cookie/CookieService.cpp index 489123e8cbbe4..da1aee326a17f 100644 --- a/netwerk/cookie/CookieService.cpp +++ b/netwerk/cookie/CookieService.cpp @@ -152,22 +152,25 @@ bool ProcessSameSiteCookieForForeignRequest(nsIChannel* aChannel, Cookie* aCookie, bool aIsSafeTopLevelNav, bool aLaxByDefault) { - int32_t sameSiteAttr = 0; - aCookie->GetSameSite(&sameSiteAttr); - - // it if's a cross origin request and the cookie is same site only (strict) - // don't send it - if (sameSiteAttr == nsICookie::SAMESITE_STRICT) { + // If it's a cross-site request and the cookie is same site only (strict) + // don't send it. + if (aCookie->SameSite() == nsICookie::SAMESITE_STRICT) { return false; }
+ // Explicit SameSite=None cookies are always processed. When laxByDefault + // is OFF then so are default cookies. + if (aCookie->SameSite() == nsICookie::SAMESITE_NONE || + (!aLaxByDefault && aCookie->IsDefaultSameSite())) { + return true; + } + int64_t currentTimeInUsec = PR_Now();
// 2 minutes of tolerance for 'SameSite=Lax by default' for cookies set // without a SameSite value when used for unsafe http methods. - if (StaticPrefs::network_cookie_sameSite_laxPlusPOST_timeout() > 0 && - aLaxByDefault && sameSiteAttr == nsICookie::SAMESITE_LAX && - aCookie->RawSameSite() == nsICookie::SAMESITE_NONE && + if (aLaxByDefault && aCookie->IsDefaultSameSite() && + StaticPrefs::network_cookie_sameSite_laxPlusPOST_timeout() > 0 && currentTimeInUsec - aCookie->CreationTime() <= (StaticPrefs::network_cookie_sameSite_laxPlusPOST_timeout() * PR_USEC_PER_SEC) && @@ -175,9 +178,11 @@ bool ProcessSameSiteCookieForForeignRequest(nsIChannel* aChannel, return true; }
- // if it's a cross origin request, the cookie is same site lax, but it's not a - // top-level navigation, don't send it - return sameSiteAttr != nsICookie::SAMESITE_LAX || aIsSafeTopLevelNav; + MOZ_ASSERT((aLaxByDefault && aCookie->IsDefaultSameSite()) || + aCookie->SameSite() == nsICookie::SAMESITE_LAX); + // We only have SameSite=Lax or lax-by-default cookies at this point. These + // are processed only if it's a top-level navigation + return aIsSafeTopLevelNav; }
} // namespace @@ -486,7 +491,9 @@ CookieService::GetCookieStringFromHttp(nsIURI* aHostURI, nsIChannel* aChannel, aChannel, attrs, StoragePrincipalHelper::eStorageAccessPrincipal);
bool isSafeTopLevelNav = CookieCommons::IsSafeTopLevelNav(aChannel); - bool isSameSiteForeign = CookieCommons::IsSameSiteForeign(aChannel, aHostURI); + bool hadCrossSiteRedirects = false; + bool isSameSiteForeign = CookieCommons::IsSameSiteForeign( + aChannel, aHostURI, &hadCrossSiteRedirects);
AutoTArray<Cookie*, 8> foundCookieList; GetCookiesForURI( @@ -494,8 +501,8 @@ CookieService::GetCookieStringFromHttp(nsIURI* aHostURI, nsIChannel* aChannel, result.contains(ThirdPartyAnalysis::IsThirdPartyTrackingResource), result.contains(ThirdPartyAnalysis::IsThirdPartySocialTrackingResource), result.contains(ThirdPartyAnalysis::IsStorageAccessPermissionGranted), - rejectedReason, isSafeTopLevelNav, isSameSiteForeign, true, attrs, - foundCookieList); + rejectedReason, isSafeTopLevelNav, isSameSiteForeign, + hadCrossSiteRedirects, true, attrs, foundCookieList);
ComposeCookieString(foundCookieList, aCookieString);
@@ -895,12 +902,26 @@ CookieService::RemoveNative(const nsACString& aHost, const nsACString& aName, return NS_OK; }
+enum class CookieProblem : uint32_t { + None = 0, + // Same-Site cookies (default or explicit) blocked due to redirect + RedirectDefault = 1 << 0, + RedirectExplicit = 1 << 1, + // Special case for googleads Same-Site cookies + RedirectGoogleAds = 1 << 2, + // Blocked due to other reasons + OtherDefault = 1 << 3, + OtherExplicit = 1 << 4 +}; +MOZ_MAKE_ENUM_CLASS_BITWISE_OPERATORS(CookieProblem) + void CookieService::GetCookiesForURI( nsIURI* aHostURI, nsIChannel* aChannel, bool aIsForeign, bool aIsThirdPartyTrackingResource, bool aIsThirdPartySocialTrackingResource, bool aStorageAccessPermissionGranted, uint32_t aRejectedReason, - bool aIsSafeTopLevelNav, bool aIsSameSiteForeign, bool aHttpBound, + bool aIsSafeTopLevelNav, bool aIsSameSiteForeign, + bool aHadCrossSiteRedirects, bool aHttpBound, const OriginAttributes& aOriginAttrs, nsTArray<Cookie*>& aCookieList) { NS_ASSERTION(aHostURI, "null host!");
@@ -1003,6 +1024,13 @@ void CookieService::GetCookiesForURI( !nsContentUtils::IsURIInPrefList( aHostURI, "network.cookie.sameSite.laxByDefault.disabledHosts");
+ // We are counting blocked SameSite cookies to get an idea of + // potential website breakage before we reintroduce "laxByDefault". + // Special attention is paid to redirects where our behavior + // differs from Chrome (bug 1763073, crbug/1221316). + // + CookieProblem sameSiteProblems = CookieProblem::None; + // iterate the cookies! for (Cookie* cookie : *cookies) { // check the host, since the base domain lookup is conservative. @@ -1015,12 +1043,6 @@ void CookieService::GetCookiesForURI( continue; }
- if (aHttpBound && aIsSameSiteForeign && - !ProcessSameSiteCookieForForeignRequest( - aChannel, cookie, aIsSafeTopLevelNav, laxByDefault)) { - continue; - } - // if the cookie is httpOnly and it's not going directly to the HTTP // connection, don't send it if (cookie->IsHttpOnly() && !aHttpBound) { @@ -1037,6 +1059,49 @@ void CookieService::GetCookiesForURI( continue; }
+ if (aHttpBound && aIsSameSiteForeign) { + bool blockCookie = !ProcessSameSiteCookieForForeignRequest( + aChannel, cookie, aIsSafeTopLevelNav, laxByDefault); + + // Record telemetry for blocked sameSite cookies. If laxByDefault is off, + // re-run the check to see if we would get different results with it + // turned on. + if (blockCookie || (!laxByDefault && cookie->IsDefaultSameSite() && + !ProcessSameSiteCookieForForeignRequest( + aChannel, cookie, aIsSafeTopLevelNav, true))) { + if (aHadCrossSiteRedirects) { + // Count the known problematic Google cookies from + // adservice.google.{com, de} etc. separately. This is not an exact + // domain match, but good enough for telemetry. + if (StringBeginsWith(hostFromURI, "adservice.google."_ns)) { + sameSiteProblems |= CookieProblem::RedirectGoogleAds; + } else { + if (cookie->IsDefaultSameSite()) { + sameSiteProblems |= CookieProblem::RedirectDefault; + } else { + sameSiteProblems |= CookieProblem::RedirectExplicit; + } + } + } else { + if (cookie->IsDefaultSameSite()) { + sameSiteProblems |= CookieProblem::OtherDefault; + } else { + sameSiteProblems |= CookieProblem::OtherExplicit; + } + } + } + + if (blockCookie) { + CookieLogging::LogMessageToConsole( + crc, aHostURI, nsIScriptError::warningFlag, + CONSOLE_REJECTION_CATEGORY, "CookieBlockedCrossSiteRedirect"_ns, + AutoTArray<nsString, 1>{ + NS_ConvertUTF8toUTF16(cookie->Name()), + }); + continue; + } + } + // all checks passed - add to list and check if lastAccessed stamp needs // updating aCookieList.AppendElement(cookie); @@ -1045,6 +1110,9 @@ void CookieService::GetCookiesForURI( } }
+ Telemetry::Accumulate(Telemetry::COOKIE_RETRIEVAL_SAMESITE_PROBLEM, + static_cast<uint32_t>(sameSiteProblems)); + if (aCookieList.IsEmpty()) { return; } diff --git a/netwerk/cookie/CookieService.h b/netwerk/cookie/CookieService.h index 15de4170a810c..caec5b785bef2 100644 --- a/netwerk/cookie/CookieService.h +++ b/netwerk/cookie/CookieService.h @@ -84,8 +84,8 @@ class CookieService final : public nsICookieService, bool aIsThirdPartySocialTrackingResource, bool aStorageAccessPermissionGranted, uint32_t aRejectedReason, bool aIsSafeTopLevelNav, - bool aIsSameSiteForeign, bool aHttpBound, - const OriginAttributes& aOriginAttrs, + bool aIsSameSiteForeign, bool aHadCrossSiteRedirects, + bool aHttpBound, const OriginAttributes& aOriginAttrs, nsTArray<Cookie*>& aCookieList);
/** diff --git a/netwerk/cookie/CookieServiceChild.cpp b/netwerk/cookie/CookieServiceChild.cpp index fe869303511b8..a4e2d2523dd31 100644 --- a/netwerk/cookie/CookieServiceChild.cpp +++ b/netwerk/cookie/CookieServiceChild.cpp @@ -145,13 +145,16 @@ void CookieServiceChild::TrackCookieLoad(nsIChannel* aChannel) { aChannel, attrs);
bool isSafeTopLevelNav = CookieCommons::IsSafeTopLevelNav(aChannel); - bool isSameSiteForeign = CookieCommons::IsSameSiteForeign(aChannel, uri); + bool hadCrossSiteRedirects = false; + bool isSameSiteForeign = + CookieCommons::IsSameSiteForeign(aChannel, uri, &hadCrossSiteRedirects); SendPrepareCookieList( uri, result.contains(ThirdPartyAnalysis::IsForeign), result.contains(ThirdPartyAnalysis::IsThirdPartyTrackingResource), result.contains(ThirdPartyAnalysis::IsThirdPartySocialTrackingResource), result.contains(ThirdPartyAnalysis::IsStorageAccessPermissionGranted), - rejectedReason, isSafeTopLevelNav, isSameSiteForeign, attrs); + rejectedReason, isSafeTopLevelNav, isSameSiteForeign, + hadCrossSiteRedirects, attrs); }
IPCResult CookieServiceChild::RecvRemoveAll() { diff --git a/netwerk/cookie/CookieServiceParent.cpp b/netwerk/cookie/CookieServiceParent.cpp index 71411c574fc7a..8389eb342c0dd 100644 --- a/netwerk/cookie/CookieServiceParent.cpp +++ b/netwerk/cookie/CookieServiceParent.cpp @@ -82,7 +82,9 @@ void CookieServiceParent::TrackCookieLoad(nsIChannel* aChannel) { nsCOMPtr<nsILoadInfo> loadInfo = aChannel->LoadInfo(); OriginAttributes attrs = loadInfo->GetOriginAttributes(); bool isSafeTopLevelNav = CookieCommons::IsSafeTopLevelNav(aChannel); - bool aIsSameSiteForeign = CookieCommons::IsSameSiteForeign(aChannel, uri); + bool hadCrossSiteRedirects = false; + bool isSameSiteForeign = + CookieCommons::IsSameSiteForeign(aChannel, uri, &hadCrossSiteRedirects);
StoragePrincipalHelper::PrepareEffectiveStoragePrincipalOriginAttributes( aChannel, attrs); @@ -101,8 +103,8 @@ void CookieServiceParent::TrackCookieLoad(nsIChannel* aChannel) { result.contains(ThirdPartyAnalysis::IsThirdPartyTrackingResource), result.contains(ThirdPartyAnalysis::IsThirdPartySocialTrackingResource), result.contains(ThirdPartyAnalysis::IsStorageAccessPermissionGranted), - rejectedReason, isSafeTopLevelNav, aIsSameSiteForeign, false, attrs, - foundCookieList); + rejectedReason, isSafeTopLevelNav, isSameSiteForeign, + hadCrossSiteRedirects, false, attrs, foundCookieList); nsTArray<CookieStruct> matchingCookiesList; SerialializeCookieList(foundCookieList, matchingCookiesList); Unused << SendTrackCookiesLoad(matchingCookiesList, attrs); @@ -129,7 +131,8 @@ IPCResult CookieServiceParent::RecvPrepareCookieList( const bool& aIsThirdPartySocialTrackingResource, const bool& aStorageAccessPermissionGranted, const uint32_t& aRejectedReason, const bool& aIsSafeTopLevelNav, - const bool& aIsSameSiteForeign, const OriginAttributes& aAttrs) { + const bool& aIsSameSiteForeign, const bool& aHadCrossSiteRedirects, + const OriginAttributes& aAttrs) { // Send matching cookies to Child. if (!aHost) { return IPC_FAIL(this, "aHost must not be null"); @@ -142,8 +145,8 @@ IPCResult CookieServiceParent::RecvPrepareCookieList( mCookieService->GetCookiesForURI( aHost, nullptr, aIsForeign, aIsThirdPartyTrackingResource, aIsThirdPartySocialTrackingResource, aStorageAccessPermissionGranted, - aRejectedReason, aIsSafeTopLevelNav, aIsSameSiteForeign, false, aAttrs, - foundCookieList); + aRejectedReason, aIsSafeTopLevelNav, aIsSameSiteForeign, + aHadCrossSiteRedirects, false, aAttrs, foundCookieList); nsTArray<CookieStruct> matchingCookiesList; SerialializeCookieList(foundCookieList, matchingCookiesList); Unused << SendTrackCookiesLoad(matchingCookiesList, aAttrs); diff --git a/netwerk/cookie/CookieServiceParent.h b/netwerk/cookie/CookieServiceParent.h index e8ac54671ece3..af48756e0ee0a 100644 --- a/netwerk/cookie/CookieServiceParent.h +++ b/netwerk/cookie/CookieServiceParent.h @@ -56,7 +56,8 @@ class CookieServiceParent : public PCookieServiceParent { const bool& aIsThirdPartySocialTrackingResource, const bool& aStorageAccessPermissionGranted, const uint32_t& aRejectedReason, const bool& aIsSafeTopLevelNav, - const bool& aIsSameSiteForeign, const OriginAttributes& aAttrs); + const bool& aIsSameSiteForeign, const bool& aHadCrossSiteRedirects, + const OriginAttributes& aAttrs);
static void SerialializeCookieList(const nsTArray<Cookie*>& aFoundCookieList, nsTArray<CookieStruct>& aCookiesList); diff --git a/netwerk/cookie/PCookieService.ipdl b/netwerk/cookie/PCookieService.ipdl index 4197982fae838..35c807ce5f271 100644 --- a/netwerk/cookie/PCookieService.ipdl +++ b/netwerk/cookie/PCookieService.ipdl @@ -46,6 +46,7 @@ parent: uint32_t rejectedReason, bool isSafeTopLevelNav, bool isSameSiteForeign, + bool hadCrossSiteRedirects, OriginAttributes attrs);
async __delete__(); diff --git a/netwerk/locales/en-US/necko.properties b/netwerk/locales/en-US/necko.properties index 8ff47ab185416..2e9ab3a901de6 100644 --- a/netwerk/locales/en-US/necko.properties +++ b/netwerk/locales/en-US/necko.properties @@ -85,5 +85,8 @@ CookieRejectedExpired=Cookie “%1$S” has been rejected because it is already # LOCALIZATION NOTE (CookieRejectedForNonSameSiteness): %1$S is the cookie name. CookieRejectedForNonSameSiteness=Cookie “%1$S” has been rejected because it is in a cross-site context and its “SameSite” is “Lax” or “Strict”.
+# LOCALIZATION NOTE (CookieBlockedCrossSiteRedirect): %1$S is the cookie name. Do not translate "SameSite", "Lax" or "Strict". +CookieBlockedCrossSiteRedirect=Cookie “%1$S” with the “SameSite” attribute value “Lax” or “Strict” was omitted because of a cross-site redirect. + # LOCALIZATION NOTE (APIDeprecationWarning): %1$S is the deprecated API; %2$S is the API function that should be used. APIDeprecationWarning=Warning: ‘%1$S’ deprecated, please use ‘%2$S’ diff --git a/toolkit/components/telemetry/Histograms.json b/toolkit/components/telemetry/Histograms.json index 2720b5e1a5683..999ef7fd7b255 100644 --- a/toolkit/components/telemetry/Histograms.json +++ b/toolkit/components/telemetry/Histograms.json @@ -16532,6 +16532,17 @@ "n_buckets": 56, "description": "How much time (in hours) passed between the current cookie purging activity and the one before that (cookie purging is run on 'daily idle')" }, + "COOKIE_RETRIEVAL_SAMESITE_PROBLEM": { + "record_in_processes": ["main", "content"], + "products": ["firefox"], + "alert_emails": ["fbraun@mozilla.com", "tschuster@mozilla.com", "seceng-telemetry@mozilla.com"], + "bug_numbers": [1763367], + "expires_in_version": "106", + "kind" : "enumerated", + "n_values": 32, + "description": "Whether a cookie was skipped in GetCookiesForURI because of a same-site issue. This is a bit field.", + "releaseChannelCollection": "opt-out" + }, "REFERRER_POLICY_COUNT" : { "products": ["firefox"], "record_in_processes": ["main"],