From 2cd11de2ba88e31413b0a38649f039d41206470b Mon Sep 17 00:00:00 2001 From: mkwst Date: Tue, 19 Jan 2016 04:16:33 -0800 Subject: [PATCH] Add 'OriginAccessEntry::matchDomain'. We need to ignore protocol shifts when calculating the first party for cookies, as we're otherwise breaking sites that embed secure login forms into insecure pages. It's better to weaken the check than to force those sites to put everything into plaintext. BUG=534749 R=jochen@chromium.org Review URL: https://codereview.chromium.org/1607433007 Cr-Commit-Position: refs/heads/master@{#370098} --- .../WebKit/Source/core/dom/Document.cpp | 4 +- .../platform/weborigin/OriginAccessEntry.cpp | 7 +++- .../platform/weborigin/OriginAccessEntry.h | 4 ++ .../weborigin/OriginAccessEntryTest.cpp | 39 ++++++++++--------- .../Source/web/tests/WebDocumentTest.cpp | 17 ++++++++ .../first_party/nested-originSecureA.html | 9 +++++ 6 files changed, 60 insertions(+), 20 deletions(-) create mode 100644 third_party/WebKit/Source/web/tests/data/first_party/nested-originSecureA.html diff --git a/third_party/WebKit/Source/core/dom/Document.cpp b/third_party/WebKit/Source/core/dom/Document.cpp index 303f98e0c89b4..d5f65bbc78e69 100644 --- a/third_party/WebKit/Source/core/dom/Document.cpp +++ b/third_party/WebKit/Source/core/dom/Document.cpp @@ -4160,7 +4160,9 @@ const KURL& Document::firstPartyForCookies() const currentDocument = currentDocument->parentDocument(); ASSERT(currentDocument); - if (accessEntry.matchesOrigin(*currentDocument->securityOrigin()) == OriginAccessEntry::DoesNotMatchOrigin) + // We use 'matchesDomain' here, as it turns out that some folks embed HTTPS login forms + // into HTTP pages; we should allow this kind of upgrade. + if (accessEntry.matchesDomain(*currentDocument->securityOrigin()) == OriginAccessEntry::DoesNotMatchOrigin) return SecurityOrigin::urlWithUniqueSecurityOrigin(); currentDocument = currentDocument->parentDocument(); diff --git a/third_party/WebKit/Source/platform/weborigin/OriginAccessEntry.cpp b/third_party/WebKit/Source/platform/weborigin/OriginAccessEntry.cpp index 820402d994942..a97e957ef9969 100644 --- a/third_party/WebKit/Source/platform/weborigin/OriginAccessEntry.cpp +++ b/third_party/WebKit/Source/platform/weborigin/OriginAccessEntry.cpp @@ -108,12 +108,17 @@ OriginAccessEntry::OriginAccessEntry(const String& protocol, const String& host, OriginAccessEntry::MatchResult OriginAccessEntry::matchesOrigin(const SecurityOrigin& origin) const { - ASSERT(origin.host() == origin.host().lower()); ASSERT(origin.protocol() == origin.protocol().lower()); if (m_protocol != origin.protocol()) return DoesNotMatchOrigin; + return matchesDomain(origin); +} + +OriginAccessEntry::MatchResult OriginAccessEntry::matchesDomain(const SecurityOrigin& origin) const +{ + ASSERT(origin.host() == origin.host().lower()); // Special case: Include subdomains and empty host means "all hosts, including ip addresses". if (m_subdomainSettings != DisallowSubdomains && m_host.isEmpty()) return MatchesOrigin; diff --git a/third_party/WebKit/Source/platform/weborigin/OriginAccessEntry.h b/third_party/WebKit/Source/platform/weborigin/OriginAccessEntry.h index d364702af2806..1d2846f314bfe 100644 --- a/third_party/WebKit/Source/platform/weborigin/OriginAccessEntry.h +++ b/third_party/WebKit/Source/platform/weborigin/OriginAccessEntry.h @@ -62,7 +62,11 @@ class PLATFORM_EXPORT OriginAccessEntry { // If host is empty string and SubdomainSetting is not DisallowSubdomains, the entry will match all domains in the specified protocol. // IPv6 addresses must include brackets (e.g. '[2001:db8:85a3::8a2e:370:7334]', not '2001:db8:85a3::8a2e:370:7334'). OriginAccessEntry(const String& protocol, const String& host, SubdomainSetting); + + // 'matchesOrigin' requires a protocol match (e.g. 'http' != 'https'). 'matchesDomain' + // relaxes this constraint. MatchResult matchesOrigin(const SecurityOrigin&) const; + MatchResult matchesDomain(const SecurityOrigin&) const; const String& protocol() const { return m_protocol; } const String& host() const { return m_host; } diff --git a/third_party/WebKit/Source/platform/weborigin/OriginAccessEntryTest.cpp b/third_party/WebKit/Source/platform/weborigin/OriginAccessEntryTest.cpp index 72d97a374d652..4136c98b0cfc6 100644 --- a/third_party/WebKit/Source/platform/weborigin/OriginAccessEntryTest.cpp +++ b/third_party/WebKit/Source/platform/weborigin/OriginAccessEntryTest.cpp @@ -91,24 +91,26 @@ TEST(OriginAccessEntryTest, AllowSubdomainsTest) const char* protocol; const char* host; const char* origin; - OriginAccessEntry::MatchResult expected; + OriginAccessEntry::MatchResult expectedOrigin; + OriginAccessEntry::MatchResult expectedDomain; } inputs[] = { - { "http", "example.com", "http://example.com/", OriginAccessEntry::MatchesOrigin }, - { "http", "example.com", "http://www.example.com/", OriginAccessEntry::MatchesOrigin }, - { "http", "example.com", "http://www.www.example.com/", OriginAccessEntry::MatchesOrigin }, - { "http", "www.example.com", "http://example.com/", OriginAccessEntry::DoesNotMatchOrigin }, - { "http", "www.example.com", "http://www.example.com/", OriginAccessEntry::MatchesOrigin }, - { "http", "www.example.com", "http://www.www.example.com/", OriginAccessEntry::MatchesOrigin }, - { "http", "com", "http://example.com/", OriginAccessEntry::MatchesOriginButIsPublicSuffix }, - { "http", "com", "http://www.example.com/", OriginAccessEntry::MatchesOriginButIsPublicSuffix }, - { "http", "com", "http://www.www.example.com/", OriginAccessEntry::MatchesOriginButIsPublicSuffix }, - { "https", "example.com", "http://example.com/", OriginAccessEntry::DoesNotMatchOrigin }, - { "https", "example.com", "http://www.example.com/", OriginAccessEntry::DoesNotMatchOrigin }, - { "https", "example.com", "http://www.www.example.com/", OriginAccessEntry::DoesNotMatchOrigin }, - { "http", "example.com", "http://beispiel.de/", OriginAccessEntry::DoesNotMatchOrigin }, - { "http", "", "http://example.com/", OriginAccessEntry::MatchesOrigin }, - { "http", "", "http://beispiel.de/", OriginAccessEntry::MatchesOrigin }, - { "https", "", "http://beispiel.de/", OriginAccessEntry::DoesNotMatchOrigin }, + { "http", "example.com", "http://example.com/", OriginAccessEntry::MatchesOrigin, OriginAccessEntry::MatchesOrigin }, + { "http", "example.com", "http://www.example.com/", OriginAccessEntry::MatchesOrigin, OriginAccessEntry::MatchesOrigin }, + { "http", "example.com", "http://www.www.example.com/", OriginAccessEntry::MatchesOrigin, OriginAccessEntry::MatchesOrigin }, + { "http", "www.example.com", "http://example.com/", OriginAccessEntry::DoesNotMatchOrigin, OriginAccessEntry::DoesNotMatchOrigin }, + { "http", "www.example.com", "http://www.example.com/", OriginAccessEntry::MatchesOrigin, OriginAccessEntry::MatchesOrigin }, + { "http", "www.example.com", "http://www.www.example.com/", OriginAccessEntry::MatchesOrigin, OriginAccessEntry::MatchesOrigin }, + { "http", "com", "http://example.com/", OriginAccessEntry::MatchesOriginButIsPublicSuffix, OriginAccessEntry::MatchesOriginButIsPublicSuffix }, + { "http", "com", "http://www.example.com/", OriginAccessEntry::MatchesOriginButIsPublicSuffix, OriginAccessEntry::MatchesOriginButIsPublicSuffix }, + { "http", "com", "http://www.www.example.com/", OriginAccessEntry::MatchesOriginButIsPublicSuffix, OriginAccessEntry::MatchesOriginButIsPublicSuffix }, + { "https", "example.com", "http://example.com/", OriginAccessEntry::DoesNotMatchOrigin, OriginAccessEntry::MatchesOrigin }, + { "https", "example.com", "http://www.example.com/", OriginAccessEntry::DoesNotMatchOrigin, OriginAccessEntry::MatchesOrigin }, + { "https", "example.com", "http://www.www.example.com/", OriginAccessEntry::DoesNotMatchOrigin, OriginAccessEntry::MatchesOrigin }, + { "http", "example.com", "http://beispiel.de/", OriginAccessEntry::DoesNotMatchOrigin, OriginAccessEntry::DoesNotMatchOrigin }, + { "http", "example.com", "https://beispiel.de/", OriginAccessEntry::DoesNotMatchOrigin, OriginAccessEntry::DoesNotMatchOrigin }, + { "http", "", "http://example.com/", OriginAccessEntry::MatchesOrigin, OriginAccessEntry::MatchesOrigin }, + { "http", "", "http://beispiel.de/", OriginAccessEntry::MatchesOrigin, OriginAccessEntry::MatchesOrigin }, + { "https", "", "http://beispiel.de/", OriginAccessEntry::DoesNotMatchOrigin, OriginAccessEntry::MatchesOrigin }, }; OriginAccessEntryTestPlatform platform; @@ -118,7 +120,8 @@ TEST(OriginAccessEntryTest, AllowSubdomainsTest) SCOPED_TRACE(testing::Message() << "Host: " << test.host << ", Origin: " << test.origin); RefPtr originToTest = SecurityOrigin::createFromString(test.origin); OriginAccessEntry entry1(test.protocol, test.host, OriginAccessEntry::AllowSubdomains); - EXPECT_EQ(test.expected, entry1.matchesOrigin(*originToTest)); + EXPECT_EQ(test.expectedOrigin, entry1.matchesOrigin(*originToTest)); + EXPECT_EQ(test.expectedDomain, entry1.matchesDomain(*originToTest)); } } diff --git a/third_party/WebKit/Source/web/tests/WebDocumentTest.cpp b/third_party/WebKit/Source/web/tests/WebDocumentTest.cpp index 60be6bbd6507b..b5b63ea26895c 100644 --- a/third_party/WebKit/Source/web/tests/WebDocumentTest.cpp +++ b/third_party/WebKit/Source/web/tests/WebDocumentTest.cpp @@ -139,11 +139,13 @@ namespace { const char* baseURLOriginA = "http://example.test:0/"; const char* baseURLOriginSubA = "http://subdomain.example.test:0/"; +const char* baseURLOriginSecureA = "https://example.test:0/"; const char* baseURLOriginB = "http://not-example.test:0/"; const char* emptyFile = "first_party/empty.html"; const char* nestedData = "first_party/nested-data.html"; const char* nestedOriginA = "first_party/nested-originA.html"; const char* nestedOriginSubA = "first_party/nested-originSubA.html"; +const char* nestedOriginSecureA = "first_party/nested-originSecureA.html"; const char* nestedOriginAInOriginA = "first_party/nested-originA-in-originA.html"; const char* nestedOriginAInOriginB = "first_party/nested-originA-in-originB.html"; const char* nestedOriginB = "first_party/nested-originB.html"; @@ -161,6 +163,11 @@ KURL toOriginSubA(const char* file) return toKURL(std::string(baseURLOriginSubA) + file); } +KURL toOriginSecureA(const char* file) +{ + return toKURL(std::string(baseURLOriginSecureA) + file); +} + KURL toOriginB(const char* file) { return toKURL(std::string(baseURLOriginB) + file); @@ -184,6 +191,7 @@ void WebDocumentFirstPartyTest::SetUpTestCase() URLTestHelpers::registerMockedURLLoad(toOriginA(nestedData), WebString::fromUTF8(nestedData)); URLTestHelpers::registerMockedURLLoad(toOriginA(nestedOriginA), WebString::fromUTF8(nestedOriginA)); URLTestHelpers::registerMockedURLLoad(toOriginA(nestedOriginSubA), WebString::fromUTF8(nestedOriginSubA)); + URLTestHelpers::registerMockedURLLoad(toOriginA(nestedOriginSecureA), WebString::fromUTF8(nestedOriginSecureA)); URLTestHelpers::registerMockedURLLoad(toOriginA(nestedOriginAInOriginA), WebString::fromUTF8(nestedOriginAInOriginA)); URLTestHelpers::registerMockedURLLoad(toOriginA(nestedOriginAInOriginB), WebString::fromUTF8(nestedOriginAInOriginB)); URLTestHelpers::registerMockedURLLoad(toOriginA(nestedOriginB), WebString::fromUTF8(nestedOriginB)); @@ -192,6 +200,7 @@ void WebDocumentFirstPartyTest::SetUpTestCase() URLTestHelpers::registerMockedURLLoad(toOriginA(nestedSrcDoc), WebString::fromUTF8(nestedSrcDoc)); URLTestHelpers::registerMockedURLLoad(toOriginSubA(emptyFile), WebString::fromUTF8(emptyFile)); + URLTestHelpers::registerMockedURLLoad(toOriginSecureA(emptyFile), WebString::fromUTF8(emptyFile)); URLTestHelpers::registerMockedURLLoad(toOriginB(emptyFile), WebString::fromUTF8(emptyFile)); URLTestHelpers::registerMockedURLLoad(toOriginB(nestedOriginA), WebString::fromUTF8(nestedOriginA)); @@ -236,6 +245,14 @@ TEST_F(WebDocumentFirstPartyTest, NestedOriginSubA) ASSERT_EQ(toOriginA(nestedOriginSubA), nestedDocument()->firstPartyForCookies()); } +TEST_F(WebDocumentFirstPartyTest, NestedOriginSecureA) +{ + load(nestedOriginSecureA); + + ASSERT_EQ(toOriginA(nestedOriginSecureA), topDocument()->firstPartyForCookies()); + ASSERT_EQ(toOriginA(nestedOriginSecureA), nestedDocument()->firstPartyForCookies()); +} + TEST_F(WebDocumentFirstPartyTest, NestedOriginAInOriginA) { load(nestedOriginAInOriginA); diff --git a/third_party/WebKit/Source/web/tests/data/first_party/nested-originSecureA.html b/third_party/WebKit/Source/web/tests/data/first_party/nested-originSecureA.html new file mode 100644 index 0000000000000..b3efe96a309d1 --- /dev/null +++ b/third_party/WebKit/Source/web/tests/data/first_party/nested-originSecureA.html @@ -0,0 +1,9 @@ + + + + Document with a frame from `https://example.test`. + + + + +