[tbb-commits] [tor-browser] 15/179: Bug 1763073 - Add telemetry (and messaging) for SameSite cookies blocked due to redirects. r=freddyb, dveditz a=pascalc
gitolite role
git at cupani.torproject.org
Fri Aug 19 08:35:12 UTC 2022
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 at 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 at mozilla.com", "tschuster at mozilla.com", "seceng-telemetry at 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"],
--
To stop receiving notification emails like this one, please contact
the administrator of this repository.
More information about the tbb-commits
mailing list