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 [email protected] Review URL: https://codereview.chromium.org/1607433007 Cr-Commit-Position: refs/heads/master@{#370098}
Bysmyyr · Jan 19, 2016 · 2cd11de · 2cd11de
1 parent 95eaa0c
commit 2cd11de
Show file tree

Hide file tree

Showing 6 changed files with 60 additions and 20 deletions.
diff --git 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
@@ -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
@@ -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
@@ -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<SecurityOrigin> 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
@@ -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
@@ -0,0 +1,9 @@
+<!DOCTYPE html>
+<html>
+<head>
+    <title>Document with a frame from `https://example.test`.</title>
+</head>
+<body>
+    <iframe src="https://example.test:0/first_party/empty.html"></iframe>
+</body>
+</html>