Attachment 339692 Details for Bug 185366 – Patch

[patch] Patch

bug-185366-20180506150357.patch (text/plain), 18.36 KB, created by Daniel Bates on 2018-05-06 15:03:58 PDT

(hide)

Description:

Filename:

MIME Type:

Creator: Daniel Bates

Created: 2018-05-06 15:03:58 PDT

Size: 18.36 KB

patch

obsolete

>Subversion Revision: 231394
>diff --git a/Source/WebCore/ChangeLog b/Source/WebCore/ChangeLog
>index 40a821d6d7214302469f2c466666303b268af894..c926ccd4da8df4011bab79368d2c7d12a95ca036 100644
>--- a/Source/WebCore/ChangeLog
>+++ b/Source/WebCore/ChangeLog
>@@ -1,3 +1,46 @@
>+2018-05-06  Daniel Bates  <dabates@apple.com>
>+
>+        CSP status-code incorrect for document blocked due to violation of its frame-ancestors directive
>+        https://bugs.webkit.org/show_bug.cgi?id=185366
>+
>+        Reviewed by NOBODY (OOPS!).
>+
>+        Fixes an issue where the status-code in the sent CSP report for an HTTP document blocked because
>+        its frame-ancestors directive was violated would be the status code of the previously loaded
>+        document in the frame. If the previously loaded document was about:blank then this would be 0.
>+
>+        Currently whenever we send a CSP report we ask the document's loader (Document::loader()) for the
>+        HTTP status code for the last response. Document::loader() returns the loader for the last committed
>+        document its frame. For a frame-ancestors violation, a CSP report is sent before the document
>+        that had the frame-ancestors directive has been committed and after it has been associate with a frame.
>+        As a result we are in are in a transient transition state for the frame and hence the last response
>+        for new document's loader (Document::loader()) is actually the last response of the previously loaded
>+        document in the frame. Instead we need to take care to tell CSP about the HTTP status code for the
>+        response associated with the document the CSP came from.
>+
>+        * dom/Document.cpp:
>+        (WebCore::Document::processHttpEquiv):
>+        (WebCore::Document::initSecurityContext):
>+        Pass the HTTP status code to CSP.
>+
>+        * page/csp/ContentSecurityPolicy.cpp:
>+        (WebCore::ContentSecurityPolicy::copyStateFrom):
>+        (WebCore::ContentSecurityPolicy::responseHeaders const):
>+        (WebCore::ContentSecurityPolicy::didReceiveHeaders):
>+        (WebCore::ContentSecurityPolicy::didReceiveHeader):
>+        (WebCore::ContentSecurityPolicy::reportViolation const):
>+        * page/csp/ContentSecurityPolicy.h:
>+        Modify existing functions to take the HTTP status code, store it in a instance variable,
>+        and reference this variable when reporting a violation.
>+
>+        * page/csp/ContentSecurityPolicyResponseHeaders.cpp:
>+        (WebCore::ContentSecurityPolicyResponseHeaders::ContentSecurityPolicyResponseHeaders):
>+        (WebCore::ContentSecurityPolicyResponseHeaders::isolatedCopy const):
>+        * page/csp/ContentSecurityPolicyResponseHeaders.h:
>+        (WebCore::ContentSecurityPolicyResponseHeaders::encode const):
>+        (WebCore::ContentSecurityPolicyResponseHeaders::decode):
>+        Store the HTTP status code along with the response headers.
>+
> 2018-05-06  Daniel Bates  <dabates@apple.com>
> 
>         CSP should only notify Inspector to pause the debugger on the first policy to violate a directive
>diff --git a/Source/WebCore/dom/Document.cpp b/Source/WebCore/dom/Document.cpp
>index 7dc8907cf0c78e73dc2a17a6e01e1528afb739a1..387fc057082f15dd17a47270dc1b509854f12e0b 100644
>--- a/Source/WebCore/dom/Document.cpp
>+++ b/Source/WebCore/dom/Document.cpp
>@@ -3317,6 +3317,7 @@ void Document::processHttpEquiv(const String& equiv, const String& content, bool
>     }
> 
>     Frame* frame = this->frame();
>+    auto* documentLoader = frame ? frame->loader().documentLoader() : nullptr;
> 
>     HTTPHeaderName headerName;
>     if (!findHTTPHeaderName(equiv, headerName))
>@@ -3384,12 +3385,12 @@ void Document::processHttpEquiv(const String& equiv, const String& content, bool
> 
>     case HTTPHeaderName::ContentSecurityPolicy:
>         if (isInDocumentHead)
>-            contentSecurityPolicy()->didReceiveHeader(content, ContentSecurityPolicyHeaderType::Enforce, ContentSecurityPolicy::PolicyFrom::HTTPEquivMeta);
>+            contentSecurityPolicy()->didReceiveHeader(content, ContentSecurityPolicyHeaderType::Enforce, ContentSecurityPolicy::PolicyFrom::HTTPEquivMeta, documentLoader ? documentLoader->response().httpStatusCode() : 0);
>         break;
> 
>     case HTTPHeaderName::XWebKitCSP:
>         if (isInDocumentHead)
>-            contentSecurityPolicy()->didReceiveHeader(content, ContentSecurityPolicyHeaderType::PrefixedEnforce, ContentSecurityPolicy::PolicyFrom::HTTPEquivMeta);
>+            contentSecurityPolicy()->didReceiveHeader(content, ContentSecurityPolicyHeaderType::PrefixedEnforce, ContentSecurityPolicy::PolicyFrom::HTTPEquivMeta, documentLoader ? documentLoader->response().httpStatusCode() : 0);
>         break;
> 
>     default:
>@@ -5511,18 +5512,17 @@ void Document::initSecurityContext()
>     if (shouldEnforceContentDispositionAttachmentSandbox())
>         applyContentDispositionAttachmentSandbox();
> 
>+    auto* loader = m_frame->loader().documentLoader();
>     bool isSecurityOriginUnique = isSandboxed(SandboxOrigin);
>-    if (!isSecurityOriginUnique) {
>-        auto* loader = m_frame->loader().documentLoader();
>+    if (!isSecurityOriginUnique)
>         isSecurityOriginUnique = loader && loader->response().tainting() == ResourceResponse::Tainting::Opaque;
>-    }
> 
>     setSecurityOriginPolicy(SecurityOriginPolicy::create(isSecurityOriginUnique ? SecurityOrigin::createUnique() : SecurityOrigin::create(m_url)));
>     setContentSecurityPolicy(std::make_unique<ContentSecurityPolicy>(*this));
> 
>     String overrideContentSecurityPolicy = m_frame->loader().client().overrideContentSecurityPolicy();
>     if (!overrideContentSecurityPolicy.isNull())
>-        contentSecurityPolicy()->didReceiveHeader(overrideContentSecurityPolicy, ContentSecurityPolicyHeaderType::Enforce, ContentSecurityPolicy::PolicyFrom::API);
>+        contentSecurityPolicy()->didReceiveHeader(overrideContentSecurityPolicy, ContentSecurityPolicyHeaderType::Enforce, ContentSecurityPolicy::PolicyFrom::API, loader ? loader->response().httpStatusCode() : 0);
> 
> #if USE(QUICK_LOOK)
>     if (shouldEnforceQuickLookSandbox())
>diff --git a/Source/WebCore/page/csp/ContentSecurityPolicy.cpp b/Source/WebCore/page/csp/ContentSecurityPolicy.cpp
>index b579e96d22446ff83c2d48ef37b9d45f6cbc89f7..d5d8b2aceaaeaa7fc5067ffe279ce70f3a8dde3b 100644
>--- a/Source/WebCore/page/csp/ContentSecurityPolicy.cpp
>+++ b/Source/WebCore/page/csp/ContentSecurityPolicy.cpp
>@@ -112,6 +112,7 @@ void ContentSecurityPolicy::copyStateFrom(const ContentSecurityPolicy* other)
>     ASSERT(m_policies.isEmpty());
>     for (auto& policy : other->m_policies)
>         didReceiveHeader(policy->header(), policy->headerType(), ContentSecurityPolicy::PolicyFrom::Inherited);
>+    m_httpStatusCode = other->m_httpStatusCode;
> }
> 
> void ContentSecurityPolicy::copyUpgradeInsecureRequestStateFrom(const ContentSecurityPolicy& other)
>@@ -166,6 +167,7 @@ ContentSecurityPolicyResponseHeaders ContentSecurityPolicy::responseHeaders() co
>         result.m_headers.reserveInitialCapacity(m_policies.size());
>         for (auto& policy : m_policies)
>             result.m_headers.uncheckedAppend({ policy->header(), policy->headerType() });
>+        result.m_httpStatusCode = m_httpStatusCode;
>         m_cachedResponseHeaders = WTFMove(result);
>     }
>     return *m_cachedResponseHeaders;
>@@ -176,13 +178,16 @@ void ContentSecurityPolicy::didReceiveHeaders(const ContentSecurityPolicyRespons
>     SetForScope<bool> isReportingEnabled(m_isReportingEnabled, reportParsingErrors == ReportParsingErrors::Yes);
>     for (auto& header : headers.m_headers)
>         didReceiveHeader(header.first, header.second, ContentSecurityPolicy::PolicyFrom::HTTPHeader);
>+    m_httpStatusCode = headers.m_httpStatusCode;
> }
> 
>-void ContentSecurityPolicy::didReceiveHeader(const String& header, ContentSecurityPolicyHeaderType type, ContentSecurityPolicy::PolicyFrom policyFrom)
>+void ContentSecurityPolicy::didReceiveHeader(const String& header, ContentSecurityPolicyHeaderType type, ContentSecurityPolicy::PolicyFrom policyFrom, int httpStatusCode)
> {
>     if (m_hasAPIPolicy)
>         return;
> 
>+    m_httpStatusCode = httpStatusCode;
>+
>     if (policyFrom == PolicyFrom::API) {
>         ASSERT(m_policies.isEmpty());
>         m_hasAPIPolicy = true;
>@@ -665,9 +670,8 @@ void ContentSecurityPolicy::reportViolation(const String& effectiveViolatedDirec
>     String violatedDirectiveText = violatedDirective;
>     String originalPolicy = violatedDirectiveList.header();
>     String referrer = document.referrer();
>-    ASSERT(document.loader());
>     // FIXME: Is it policy to not use the status code for HTTPS, or is that a bug?
>-    unsigned short statusCode = document.url().protocolIs("http") && document.loader() ? document.loader()->response().httpStatusCode() : 0;
>+    unsigned short statusCode = m_selfSourceProtocol == "http" ? m_httpStatusCode : 0;
> 
>     String sourceFile;
>     int lineNumber = 0;
>diff --git a/Source/WebCore/page/csp/ContentSecurityPolicy.h b/Source/WebCore/page/csp/ContentSecurityPolicy.h
>index 293d756b17ab6f06169c4f32d0780325b47e84cf..3d4195da6677b6d6299d3f1acf769c1a2c516c79 100644
>--- a/Source/WebCore/page/csp/ContentSecurityPolicy.h
>+++ b/Source/WebCore/page/csp/ContentSecurityPolicy.h
>@@ -82,7 +82,7 @@ public:
>     WEBCORE_EXPORT ContentSecurityPolicyResponseHeaders responseHeaders() const;
>     enum ReportParsingErrors { No, Yes };
>     WEBCORE_EXPORT void didReceiveHeaders(const ContentSecurityPolicyResponseHeaders&, ReportParsingErrors = ReportParsingErrors::Yes);
>-    void didReceiveHeader(const String&, ContentSecurityPolicyHeaderType, ContentSecurityPolicy::PolicyFrom);
>+    void didReceiveHeader(const String&, ContentSecurityPolicyHeaderType, ContentSecurityPolicy::PolicyFrom, int httpStatusCode = 0);
> 
>     bool allowScriptWithNonce(const String& nonce, bool overrideContentSecurityPolicy = false) const;
>     bool allowStyleWithNonce(const String& nonce, bool overrideContentSecurityPolicy = false) const;
>@@ -216,6 +216,7 @@ private:
>     bool m_isReportingEnabled { true };
>     bool m_upgradeInsecureRequests { false };
>     bool m_hasAPIPolicy { false };
>+    int m_httpStatusCode { 0 };
>     OptionSet<ContentSecurityPolicyHashAlgorithm> m_hashAlgorithmsForInlineScripts;
>     OptionSet<ContentSecurityPolicyHashAlgorithm> m_hashAlgorithmsForInlineStylesheets;
>     HashSet<SecurityOriginData> m_insecureNavigationRequestsToUpgrade;
>diff --git a/Source/WebCore/page/csp/ContentSecurityPolicyResponseHeaders.cpp b/Source/WebCore/page/csp/ContentSecurityPolicyResponseHeaders.cpp
>index 0b67b91d424f20aee9ccbcc7023687a4a88e51b6..30021884b146f6dcf8b73e0592f1c661f563fada 100644
>--- a/Source/WebCore/page/csp/ContentSecurityPolicyResponseHeaders.cpp
>+++ b/Source/WebCore/page/csp/ContentSecurityPolicyResponseHeaders.cpp
>@@ -48,6 +48,8 @@ ContentSecurityPolicyResponseHeaders::ContentSecurityPolicyResponseHeaders(const
>     policyValue = response.httpHeaderField(HTTPHeaderName::XWebKitCSPReportOnly);
>     if (!policyValue.isEmpty())
>         m_headers.append({ policyValue, ContentSecurityPolicyHeaderType::PrefixedReport });
>+
>+    m_httpStatusCode = response.httpStatusCode();
> }
> 
> ContentSecurityPolicyResponseHeaders ContentSecurityPolicyResponseHeaders::isolatedCopy() const
>@@ -56,6 +58,7 @@ ContentSecurityPolicyResponseHeaders ContentSecurityPolicyResponseHeaders::isola
>     isolatedCopy.m_headers.reserveInitialCapacity(m_headers.size());
>     for (auto& header : m_headers)
>         isolatedCopy.m_headers.uncheckedAppend({ header.first.isolatedCopy(), header.second });
>+    isolatedCopy.m_httpStatusCode = m_httpStatusCode;
>     return isolatedCopy;
> }
> 
>diff --git a/Source/WebCore/page/csp/ContentSecurityPolicyResponseHeaders.h b/Source/WebCore/page/csp/ContentSecurityPolicyResponseHeaders.h
>index 002e0f96987db5af765c310b1bd9257e04a51b38..4323fe668b6d5c49f45fdba46d2581ca1e95572a 100644
>--- a/Source/WebCore/page/csp/ContentSecurityPolicyResponseHeaders.h
>+++ b/Source/WebCore/page/csp/ContentSecurityPolicyResponseHeaders.h
>@@ -54,6 +54,7 @@ private:
>     friend class ContentSecurityPolicy;
> 
>     Vector<std::pair<String, ContentSecurityPolicyHeaderType>> m_headers;
>+    int m_httpStatusCode { 0 };
> };
> 
> template <class Encoder>
>@@ -64,6 +65,7 @@ void ContentSecurityPolicyResponseHeaders::encode(Encoder& encoder) const
>         encoder << pair.first;
>         encoder.encodeEnum(pair.second);
>     }
>+    encoder << m_httpStatusCode;
> }
> 
> template <class Decoder>
>@@ -83,6 +85,9 @@ bool ContentSecurityPolicyResponseHeaders::decode(Decoder& decoder, ContentSecur
>         headers.m_headers.append(std::make_pair(header, headerType));
>     }
> 
>+    if (!decoder.decode(headers.m_httpStatusCode))
>+        return false;
>+
>     return true;
> }
> 
>diff --git a/LayoutTests/ChangeLog b/LayoutTests/ChangeLog
>index 06b0d56bb95b3b9e92ab17177f857c52e99a6045..f725a0d9da4a841e5f4626c092306f329b64f53f 100644
>--- a/LayoutTests/ChangeLog
>+++ b/LayoutTests/ChangeLog
>@@ -1,3 +1,15 @@
>+2018-05-06  Daniel Bates  <dabates@apple.com>
>+
>+        CSP status-code incorrect for document blocked due to violation of its frame-ancestors directive
>+        https://bugs.webkit.org/show_bug.cgi?id=185366
>+
>+        Reviewed by NOBODY (OOPS!).
>+
>+        Update existing test results now that we send the HTTP status code for the correct document.
>+
>+        * http/tests/security/contentSecurityPolicy/1.1/frame-ancestors/report-frame-ancestors-cross-origin-expected.txt:
>+        * http/tests/security/contentSecurityPolicy/1.1/frame-ancestors/report-frame-ancestors-same-origin-expected.txt:
>+
> 2018-05-02  Daniel Bates  <dabates@apple.com>
> 
>         Add tests to ensure Same-Site cookies are included when performing a top-level redirect
>diff --git a/LayoutTests/http/tests/security/contentSecurityPolicy/1.1/frame-ancestors/report-frame-ancestors-cross-origin-expected.txt b/LayoutTests/http/tests/security/contentSecurityPolicy/1.1/frame-ancestors/report-frame-ancestors-cross-origin-expected.txt
>index 303dace0c94fb2f71b3bab918f18875eb4977093..12fc99bc32e4f07351062f065bdd79a981cda545 100644
>--- a/LayoutTests/http/tests/security/contentSecurityPolicy/1.1/frame-ancestors/report-frame-ancestors-cross-origin-expected.txt
>+++ b/LayoutTests/http/tests/security/contentSecurityPolicy/1.1/frame-ancestors/report-frame-ancestors-cross-origin-expected.txt
>@@ -5,4 +5,4 @@ HTTP_HOST: localhost:8000
> REQUEST_METHOD: POST
> REQUEST_URI: /security/contentSecurityPolicy/resources/save-report.php?test=/security/contentSecurityPolicy/1.1/report-frame-ancestors-cross-origin.html
> === POST DATA ===
>-{"csp-report":{"document-uri":"http://localhost:8000/security/contentSecurityPolicy/resources/echo-intertag.pl?header=Content-Security-Policy%3A+frame-ancestors+%27none%27%3B+report-uri+save-report.php%3Ftest%3D/security/contentSecurityPolicy/1.1/report-frame-ancestors-cross-origin.html&q=FAIL","referrer":"","violated-directive":"frame-ancestors 'none'","effective-directive":"frame-ancestors","original-policy":"frame-ancestors 'none'; report-uri save-report.php?test=/security/contentSecurityPolicy/1.1/report-frame-ancestors-cross-origin.html","blocked-uri":"http://localhost:8000/security/contentSecurityPolicy/resources/echo-intertag.pl?header=Content-Security-Policy%3A+frame-ancestors+%27none%27%3B+report-uri+save-report.php%3Ftest%3D/security/contentSecurityPolicy/1.1/report-frame-ancestors-cross-origin.html&q=FAIL","status-code":0}}
>+{"csp-report":{"document-uri":"http://localhost:8000/security/contentSecurityPolicy/resources/echo-intertag.pl?header=Content-Security-Policy%3A+frame-ancestors+%27none%27%3B+report-uri+save-report.php%3Ftest%3D/security/contentSecurityPolicy/1.1/report-frame-ancestors-cross-origin.html&q=FAIL","referrer":"","violated-directive":"frame-ancestors 'none'","effective-directive":"frame-ancestors","original-policy":"frame-ancestors 'none'; report-uri save-report.php?test=/security/contentSecurityPolicy/1.1/report-frame-ancestors-cross-origin.html","blocked-uri":"http://localhost:8000/security/contentSecurityPolicy/resources/echo-intertag.pl?header=Content-Security-Policy%3A+frame-ancestors+%27none%27%3B+report-uri+save-report.php%3Ftest%3D/security/contentSecurityPolicy/1.1/report-frame-ancestors-cross-origin.html&q=FAIL","status-code":200}}
>diff --git a/LayoutTests/http/tests/security/contentSecurityPolicy/1.1/frame-ancestors/report-frame-ancestors-same-origin-expected.txt b/LayoutTests/http/tests/security/contentSecurityPolicy/1.1/frame-ancestors/report-frame-ancestors-same-origin-expected.txt
>index b56c952f5005d974a150de13fad8b6527956486e..35c6da996dce1bb24e44f8149a0418a15984d12b 100644
>--- a/LayoutTests/http/tests/security/contentSecurityPolicy/1.1/frame-ancestors/report-frame-ancestors-same-origin-expected.txt
>+++ b/LayoutTests/http/tests/security/contentSecurityPolicy/1.1/frame-ancestors/report-frame-ancestors-same-origin-expected.txt
>@@ -5,4 +5,4 @@ HTTP_HOST: 127.0.0.1:8000
> REQUEST_METHOD: POST
> REQUEST_URI: /security/contentSecurityPolicy/resources/save-report.php?test=/security/contentSecurityPolicy/1.1/report-frame-ancestors-same-origin.html
> === POST DATA ===
>-{"csp-report":{"document-uri":"http://127.0.0.1:8000/security/contentSecurityPolicy/resources/echo-intertag.pl?header=Content-Security-Policy%3A+frame-ancestors+%27none%27%3B+report-uri+save-report.php%3Ftest%3D/security/contentSecurityPolicy/1.1/report-frame-ancestors-same-origin.html&q=FAIL","referrer":"","violated-directive":"frame-ancestors 'none'","effective-directive":"frame-ancestors","original-policy":"frame-ancestors 'none'; report-uri save-report.php?test=/security/contentSecurityPolicy/1.1/report-frame-ancestors-same-origin.html","blocked-uri":"http://127.0.0.1:8000/security/contentSecurityPolicy/resources/echo-intertag.pl?header=Content-Security-Policy%3A+frame-ancestors+%27none%27%3B+report-uri+save-report.php%3Ftest%3D/security/contentSecurityPolicy/1.1/report-frame-ancestors-same-origin.html&q=FAIL","status-code":0}}
>+{"csp-report":{"document-uri":"http://127.0.0.1:8000/security/contentSecurityPolicy/resources/echo-intertag.pl?header=Content-Security-Policy%3A+frame-ancestors+%27none%27%3B+report-uri+save-report.php%3Ftest%3D/security/contentSecurityPolicy/1.1/report-frame-ancestors-same-origin.html&q=FAIL","referrer":"","violated-directive":"frame-ancestors 'none'","effective-directive":"frame-ancestors","original-policy":"frame-ancestors 'none'; report-uri save-report.php?test=/security/contentSecurityPolicy/1.1/report-frame-ancestors-same-origin.html","blocked-uri":"http://127.0.0.1:8000/security/contentSecurityPolicy/resources/echo-intertag.pl?header=Content-Security-Policy%3A+frame-ancestors+%27none%27%3B+report-uri+save-report.php%3Ftest%3D/security/contentSecurityPolicy/1.1/report-frame-ancestors-same-origin.html&q=FAIL","status-code":200}}

Flags:

bfulgham: review+

Actions: View | Formatted Diff | Diff

Attachments on bug 185366: 339691 | 339692 | 339699