Skip to content

Commit

Permalink
Do not warn about SameSite if cookie would be blocked anyway
Browse files Browse the repository at this point in the history 
This fixes a bug where cookies that would be rejected for other reasons
(e.g. domain mismatch) would still generate a SameSite warning message.
This led to misleading/alarming messages that implied that sites were
fetching resources from domains that they were not in fact making
requests to.

The fix is to not attach a warning to the cookie status if there are any
exclusion reasons other than the new SameSite ones.

Bug: 1027318
Change-Id: I77a26ee4679aa6010b0e6b604476ec364614ce96
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1929780
Commit-Queue: Lily Chen <chlily@chromium.org>
Reviewed-by: John Abd-El-Malek <jam@chromium.org>
Reviewed-by: Maksim Orlovich <morlovich@chromium.org>
Cr-Commit-Position: refs/heads/master@{#718805}
  • Loading branch information
@chlily1
chlily1 authored and Commit Bot committed Nov 25, 2019
1 parent 9f84b7b commit 51515c5
Show file tree
Hide file tree
Showing 6 changed files with 277 additions and 54 deletions.
27 changes: 27 additions & 0 deletions content/browser/frame_host/render_frame_host_impl_browsertest.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -2757,6 +2757,33 @@ IN_PROC_BROWSER_TEST_F(RenderFrameHostImplSameSiteByDefaultCookiesBrowserTest,
EXPECT_NE(console_observer.messages()[1], console_observer.messages()[2]);
}

// Test that the SameSite-by-default console warnings are not emitted
// if the cookie would have been rejected for other reasons.
// Regression test for https://crbug.com/1027318.
IN_PROC_BROWSER_TEST_F(RenderFrameHostImplSameSiteByDefaultCookiesBrowserTest,
NoMessagesIfCookieWouldBeRejectedForOtherReasons) {
WebContentsImpl* web_contents =
static_cast<WebContentsImpl*>(shell()->web_contents());
ConsoleObserverDelegate console_observer(web_contents, "*");
web_contents->SetDelegate(&console_observer);

GURL url = embedded_test_server()->GetURL(
"x.com", "/set-cookie?cookiewithpath=1;path=/set-cookie");
EXPECT_TRUE(NavigateToURL(shell(), url));
url = embedded_test_server()->GetURL("sub.x.com",
"/set-cookie?cookieforsubdomain=1");
EXPECT_TRUE(NavigateToURL(shell(), url));

ASSERT_EQ(0u, console_observer.messages().size());
url = embedded_test_server()->GetURL(
"a.com", "/cross_site_iframe_factory.html?a(x())");
EXPECT_TRUE(NavigateToURL(shell(), url));
// No messages appear even though x.com is accessed in a cross-site
// context, because the cookies would have been rejected for mismatching path
// or domain anyway.
EXPECT_EQ(0u, console_observer.messages().size());
}

IN_PROC_BROWSER_TEST_F(RenderFrameHostImplBrowserTest,
SchedulerTrackedFeatures) {
EXPECT_TRUE(
Expand Down
58 changes: 44 additions & 14 deletions net/cookies/canonical_cookie.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -133,6 +133,9 @@ void ApplySameSiteCookieWarningToStatus(
status->set_warning(
CanonicalCookie::CookieInclusionStatus::WARN_SAMESITE_NONE_INSECURE);
}
// If there are reasons to exclude the cookie other than the new SameSite
// rules, don't warn about the cookie at all.
status->MaybeClearSameSiteWarning();
}

} // namespace
Expand Down Expand Up @@ -554,10 +557,34 @@ CanonicalCookie::CookieInclusionStatus CanonicalCookie::IsSetPermittedInContext(
const CookieOptions& options,
CookieAccessSemantics access_semantics) const {
CookieInclusionStatus status;
IsSetPermittedInContext(options, access_semantics, &status);
return status;
}

void CanonicalCookie::IsSetPermittedInContext(
const CookieOptions& options,
CookieAccessSemantics access_semantics,
CookieInclusionStatus* status) const {
if (options.exclude_httponly() && IsHttpOnly()) {
DVLOG(net::cookie_util::kVlogSetCookies)
<< "HttpOnly cookie not permitted in script context.";
status.AddExclusionReason(CookieInclusionStatus::EXCLUDE_HTTP_ONLY);
status->AddExclusionReason(CookieInclusionStatus::EXCLUDE_HTTP_ONLY);
}

// If both SameSiteByDefaultCookies and CookiesWithoutSameSiteMustBeSecure
// are enabled, non-SameSite cookies without the Secure attribute will be
// rejected.
if (access_semantics != CookieAccessSemantics::LEGACY &&
cookie_util::IsCookiesWithoutSameSiteMustBeSecureEnabled() &&
SameSite() == CookieSameSite::NO_RESTRICTION && !IsSecure()) {
DVLOG(net::cookie_util::kVlogSetCookies)
<< "SetCookie() rejecting insecure cookie with SameSite=None.";
status->AddExclusionReason(
CanonicalCookie::CookieInclusionStatus::EXCLUDE_SAMESITE_NONE_INSECURE);
}
// Log whether a SameSite=None cookie is Secure or not.
if (SameSite() == CookieSameSite::NO_RESTRICTION) {
UMA_HISTOGRAM_BOOLEAN("Cookie.SameSiteNoneIsSecure", IsSecure());
}

CookieEffectiveSameSite effective_same_site =
Expand All @@ -572,7 +599,7 @@ CanonicalCookie::CookieInclusionStatus CanonicalCookie::IsSetPermittedInContext(
DVLOG(net::cookie_util::kVlogSetCookies)
<< "Trying to set a `SameSite=Strict` cookie from a "
"cross-site URL.";
status.AddExclusionReason(
status->AddExclusionReason(
CookieInclusionStatus::EXCLUDE_SAMESITE_STRICT);
}
break;
Expand All @@ -584,13 +611,13 @@ CanonicalCookie::CookieInclusionStatus CanonicalCookie::IsSetPermittedInContext(
DVLOG(net::cookie_util::kVlogSetCookies)
<< "Cookies with no known SameSite attribute being treated as "
"lax; attempt to set from a cross-site URL denied.";
status.AddExclusionReason(
status->AddExclusionReason(
CookieInclusionStatus::
EXCLUDE_SAMESITE_UNSPECIFIED_TREATED_AS_LAX);
} else {
DVLOG(net::cookie_util::kVlogSetCookies)
<< "Trying to set a `SameSite=Lax` cookie from a cross-site URL.";
status.AddExclusionReason(
status->AddExclusionReason(
CookieInclusionStatus::EXCLUDE_SAMESITE_LAX);
}
}
Expand All @@ -601,9 +628,9 @@ CanonicalCookie::CookieInclusionStatus CanonicalCookie::IsSetPermittedInContext(

ApplySameSiteCookieWarningToStatus(
SameSite(), effective_same_site, IsSecure(),
options.same_site_cookie_context(), &status);
options.same_site_cookie_context(), status);

if (status.IsInclude()) {
if (status->IsInclude()) {
UMA_HISTOGRAM_ENUMERATION("Cookie.IncludedResponseEffectiveSameSite",
effective_same_site,
CookieEffectiveSameSite::COUNT);
Expand All @@ -620,7 +647,6 @@ CanonicalCookie::CookieInclusionStatus CanonicalCookie::IsSetPermittedInContext(
}

// TODO(chlily): Log metrics.
return status;
}

std::string CanonicalCookie::DebugString() const {
Expand Down Expand Up @@ -843,20 +869,24 @@ bool CanonicalCookie::CookieInclusionStatus::HasExclusionReason(
void CanonicalCookie::CookieInclusionStatus::AddExclusionReason(
ExclusionReason reason) {
exclusion_reasons_ |= GetBitmask(reason);
}

void CanonicalCookie::CookieInclusionStatus::AddExclusionReasonsAndWarningIfAny(
const CookieInclusionStatus& other) {
exclusion_reasons_ |= other.exclusion_reasons_;
if (other.warning_ != DO_NOT_WARN)
warning_ = other.warning_;
// If the cookie would be excluded for reasons other than the new SameSite
// rules, don't bother warning about it.
MaybeClearSameSiteWarning();
}

void CanonicalCookie::CookieInclusionStatus::RemoveExclusionReason(
ExclusionReason reason) {
exclusion_reasons_ &= ~(GetBitmask(reason));
}

void CanonicalCookie::CookieInclusionStatus::MaybeClearSameSiteWarning() {
uint32_t samesite_reasons_mask =
GetBitmask(EXCLUDE_SAMESITE_UNSPECIFIED_TREATED_AS_LAX) |
GetBitmask(EXCLUDE_SAMESITE_NONE_INSECURE);
if (exclusion_reasons_ & ~samesite_reasons_mask)
set_warning(DO_NOT_WARN);
}

bool CanonicalCookie::CookieInclusionStatus::ShouldWarn() const {
return warning_ != DO_NOT_WARN;
}
Expand Down
15 changes: 10 additions & 5 deletions net/cookies/canonical_cookie.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -197,6 +197,11 @@ class NET_EXPORT CanonicalCookie {
CookieAccessSemantics access_semantics =
CookieAccessSemantics::UNKNOWN) const;

// Overload that updates an existing |status| rather than returning a new one.
void IsSetPermittedInContext(const CookieOptions& options,
CookieAccessSemantics access_semantics,
CookieInclusionStatus* status) const;

std::string DebugString() const;

static std::string CanonPathWithString(const GURL& url,
Expand Down Expand Up @@ -388,14 +393,14 @@ class NET_EXPORT CanonicalCookie::CookieInclusionStatus {
// Add an exclusion reason.
void AddExclusionReason(ExclusionReason status_type);

// Add all the exclusion reasons given in |other|. If there is a warning in
// |other| (other than DO_NOT_WARN), also apply that. This could overwrite the
// existing warning, so set the most important warnings last.
void AddExclusionReasonsAndWarningIfAny(const CookieInclusionStatus& other);

// Remove an exclusion reason.
void RemoveExclusionReason(ExclusionReason reason);

// If the cookie would have been excluded for reasons other than
// SAMESITE_UNSPECIFIED_TREATED_AS_LAX or SAMESITE_NONE_INSECURE, don't bother
// warning about it (clear the warning).
void MaybeClearSameSiteWarning();

// Whether the cookie should be warned about.
bool ShouldWarn() const;

Expand Down
36 changes: 20 additions & 16 deletions net/cookies/canonical_cookie_unittest.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -2312,11 +2312,31 @@ TEST(CookieInclusionStatusTest, NotValid) {

TEST(CookieInclusionStatusTest, AddExclusionReason) {
CanonicalCookie::CookieInclusionStatus status;
status.set_warning(CanonicalCookie::CookieInclusionStatus::
WARN_SAMESITE_UNSPECIFIED_LAX_ALLOW_UNSAFE);
status.AddExclusionReason(
CanonicalCookie::CookieInclusionStatus::EXCLUDE_UNKNOWN_ERROR);
EXPECT_TRUE(status.IsValid());
EXPECT_TRUE(status.HasExactlyExclusionReasonsForTesting(
{CanonicalCookie::CookieInclusionStatus::EXCLUDE_UNKNOWN_ERROR}));
// Adding an exclusion reason other than
// EXCLUDE_SAMESITE_UNSPECIFIED_TREATED_AS_LAX or
// EXCLUDE_SAMESITE_NONE_INSECURE should clear any SameSite warning.
EXPECT_EQ(CanonicalCookie::CookieInclusionStatus::DO_NOT_WARN,
status.warning());

status = CanonicalCookie::CookieInclusionStatus();
status.set_warning(CanonicalCookie::CookieInclusionStatus::
WARN_SAMESITE_UNSPECIFIED_CROSS_SITE_CONTEXT);
status.AddExclusionReason(CanonicalCookie::CookieInclusionStatus::
EXCLUDE_SAMESITE_UNSPECIFIED_TREATED_AS_LAX);
EXPECT_TRUE(status.IsValid());
EXPECT_TRUE(status.HasExactlyExclusionReasonsForTesting(
{CanonicalCookie::CookieInclusionStatus::
EXCLUDE_SAMESITE_UNSPECIFIED_TREATED_AS_LAX}));
EXPECT_EQ(CanonicalCookie::CookieInclusionStatus::
WARN_SAMESITE_UNSPECIFIED_CROSS_SITE_CONTEXT,
status.warning());
}

TEST(CookieInclusionStatusTest, RemoveExclusionReason) {
Expand All @@ -2342,20 +2362,4 @@ TEST(CookieInclusionStatusTest, RemoveExclusionReason) {
CanonicalCookie::CookieInclusionStatus::NUM_EXCLUSION_REASONS));
}

TEST(CookieInclusionStatusTest, AddExclusionReasonsAndWarningIfAny) {
CanonicalCookie::CookieInclusionStatus status1;
CanonicalCookie::CookieInclusionStatus status2;

status1.set_exclusion_reasons(0b00011111u);
status2.set_exclusion_reasons(0b11111000u);
status2.set_warning(
CanonicalCookie::CookieInclusionStatus::WARN_SAMESITE_NONE_INSECURE);

status1.AddExclusionReasonsAndWarningIfAny(status2);

EXPECT_EQ(0b11111111u, status1.exclusion_reasons());
EXPECT_EQ(CanonicalCookie::CookieInclusionStatus::WARN_SAMESITE_NONE_INSECURE,
status1.warning());
}

} // namespace net
21 changes: 2 additions & 19 deletions net/cookies/cookie_monster.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -1160,30 +1160,13 @@ void CookieMonster::SetCanonicalCookie(std::unique_ptr<CanonicalCookie> cc,
CanonicalCookie::CookieInclusionStatus::EXCLUDE_SECURE_ONLY);
}

status.AddExclusionReasonsAndWarningIfAny(
cc->IsSetPermittedInContext(options, GetAccessSemanticsForCookie(*cc)));

if (!IsCookieableScheme(scheme_lower)) {
status.AddExclusionReason(
CanonicalCookie::CookieInclusionStatus::EXCLUDE_NONCOOKIEABLE_SCHEME);
}

// If both SameSiteByDefaultCookies and CookiesWithoutSameSiteMustBeSecure
// are enabled, non-SameSite cookies without the Secure attribute will be
// rejected. A warning for this would have been added by
// IsSetPermittedInContext().
if (GetAccessSemanticsForCookie(*cc) != CookieAccessSemantics::LEGACY &&
cookie_util::IsCookiesWithoutSameSiteMustBeSecureEnabled() &&
cc->SameSite() == CookieSameSite::NO_RESTRICTION && !cc->IsSecure()) {
DVLOG(net::cookie_util::kVlogSetCookies)
<< "SetCookie() rejecting insecure cookie with SameSite=None.";
status.AddExclusionReason(
CanonicalCookie::CookieInclusionStatus::EXCLUDE_SAMESITE_NONE_INSECURE);
}
// Log whether a SameSite=None cookie is Secure or not.
if (cc->SameSite() == CookieSameSite::NO_RESTRICTION) {
UMA_HISTOGRAM_BOOLEAN("Cookie.SameSiteNoneIsSecure", cc->IsSecure());
}
cc->IsSetPermittedInContext(options, GetAccessSemanticsForCookie(*cc),
&status);

const std::string key(GetKey(cc->Domain()));

Expand Down
Loading

0 comments on commit 51515c5

Please sign in to comment.