Attachment 338918 Details for Bug 184892 – WIP

[patch] WIP

bug-184892-20180426144709.patch (text/plain), 11.46 KB, created by youenn fablet on 2018-04-26 14:47:10 PDT

(hide)

Description:

Filename:

MIME Type:

Creator: youenn fablet

Created: 2018-04-26 14:47:10 PDT

Size: 11.46 KB

patch

obsolete

>Subversion Revision: 230995
>diff --git a/Source/WebCore/ChangeLog b/Source/WebCore/ChangeLog
>index 2066193eec12cfa333572f23b64f4552202005b6..7620f0f74054a1ccd504049b8ce51c521ef135d6 100644
>--- a/Source/WebCore/ChangeLog
>+++ b/Source/WebCore/ChangeLog
>@@ -1,3 +1,19 @@
>+2018-04-26  Youenn Fablet  <youenn@apple.com>
>+
>+        Use NetworkLoadChecker for navigation loads
>+        https://bugs.webkit.org/show_bug.cgi?id=184892
>+        <rdar://problem/39652686>
>+
>+        Reviewed by NOBODY (OOPS!).
>+
>+        Adding finer grained sanitization based on the response tainting, now that NetworkLoadChecker is used
>+        and sets the response tainting.
>+
>+        * platform/network/ResourceResponseBase.cpp:
>+        (WebCore::ResourceResponseBase::sanitizeHTTPHeaderFieldsAccordingTainting):
>+        (WebCore::ResourceResponseBase::sanitizeHTTPHeaderFields):
>+        * platform/network/ResourceResponseBase.h:
>+
> 2018-04-26  Youenn Fablet  <youenn@apple.com>
> 
>         Use NetworkLoadChecker for XHR/fetch loads
>diff --git a/Source/WebKit/ChangeLog b/Source/WebKit/ChangeLog
>index 02215cea044c94d9672f957b6f932b991e05547d..94fda7387e7642315923a78d8e6a1d5daa46811d 100644
>--- a/Source/WebKit/ChangeLog
>+++ b/Source/WebKit/ChangeLog
>@@ -1,3 +1,18 @@
>+2018-04-26  Youenn Fablet  <youenn@apple.com>
>+
>+        Use NetworkLoadChecker for navigation loads
>+        https://bugs.webkit.org/show_bug.cgi?id=184892
>+        <rdar://problem/39652686>
>+
>+        Reviewed by NOBODY (OOPS!).
>+
>+        * NetworkProcess/NetworkResourceLoader.cpp:
>+        (WebKit::NetworkResourceLoader::sanitizeResponseIfPossible):
>+        * WebProcess/Network/WebLoaderStrategy.cpp:
>+        (WebKit::WebLoaderStrategy::scheduleLoadFromNetworkProcess):
>+        (WebKit::WebLoaderStrategy::isDoingLoadingSecurityChecks const):
>+        * WebProcess/Network/WebLoaderStrategy.h:
>+
> 2018-04-26  Youenn Fablet  <youenn@apple.com>
> 
>         Use NetworkLoadChecker for XHR/fetch loads
>diff --git a/Source/WebCore/platform/network/ResourceResponseBase.cpp b/Source/WebCore/platform/network/ResourceResponseBase.cpp
>index da83b610961f348f609bba05a945d4a490af651a..7a346785acbfc984f85f2e2d525615b57bb57b72 100644
>--- a/Source/WebCore/platform/network/ResourceResponseBase.cpp
>+++ b/Source/WebCore/platform/network/ResourceResponseBase.cpp
>@@ -389,6 +389,46 @@ static bool isSafeCrossOriginResponseHeader(HTTPHeaderName name)
>         || name == HTTPHeaderName::XXSSProtection;
> }
> 
>+void ResourceResponseBase::sanitizeHTTPHeaderFieldsAccordingTainting()
>+{
>+    switch (m_tainting) {
>+    case ResourceResponse::Tainting::Basic:
>+        return;
>+    case ResourceResponse::Tainting::Cors: {
>+        HTTPHeaderMap filteredHeaders;
>+        for (auto& header : m_httpHeaderFields.commonHeaders()) {
>+            if (isSafeCrossOriginResponseHeader(header.key))
>+                filteredHeaders.add(header.key, WTFMove(header.value));
>+        }
>+        if (auto corsSafeHeaderSet = parseAccessControlAllowList(httpHeaderField(HTTPHeaderName::AccessControlExposeHeaders))) {
>+            for (auto& headerName : *corsSafeHeaderSet) {
>+                if (!filteredHeaders.contains(headerName)) {
>+                    auto value = m_httpHeaderFields.get(headerName);
>+                    if (!value.isNull())
>+                        filteredHeaders.add(headerName, value);
>+                }
>+            }
>+        }
>+        m_httpHeaderFields = WTFMove(filteredHeaders);
>+        return;
>+    }
>+    case ResourceResponse::Tainting::Opaque: {
>+        HTTPHeaderMap filteredHeaders;
>+        for (auto& header : m_httpHeaderFields.commonHeaders()) {
>+            if (isSafeCrossOriginResponseHeader(header.key))
>+                filteredHeaders.add(header.key, WTFMove(header.value));
>+        }
>+        m_httpHeaderFields = WTFMove(filteredHeaders);
>+        return;
>+    }
>+    case ResourceResponse::Tainting::Opaqueredirect: {
>+        auto location = httpHeaderField(HTTPHeaderName::Location);
>+        m_httpHeaderFields.clear();
>+        m_httpHeaderFields.add(HTTPHeaderName::Location, WTFMove(location));
>+    }
>+    }
>+}
>+
> void ResourceResponseBase::sanitizeHTTPHeaderFields(SanitizationType type)
> {
>     lazyInit(AllFields);
>@@ -408,23 +448,8 @@ void ResourceResponseBase::sanitizeHTTPHeaderFields(SanitizationType type)
>         m_httpHeaderFields.uncommonHeaders().clear();
>         return;
>     }
>-    case SanitizationType::CrossOriginSafe: {
>-        HTTPHeaderMap filteredHeaders;
>-        for (auto& header : m_httpHeaderFields.commonHeaders()) {
>-            if (isSafeCrossOriginResponseHeader(header.key))
>-                filteredHeaders.add(header.key, WTFMove(header.value));
>-        }
>-        if (auto corsSafeHeaderSet = parseAccessControlAllowList(httpHeaderField(HTTPHeaderName::AccessControlExposeHeaders))) {
>-            for (auto& headerName : *corsSafeHeaderSet) {
>-                if (!filteredHeaders.contains(headerName)) {
>-                    auto value = m_httpHeaderFields.get(headerName);
>-                    if (!value.isNull())
>-                        filteredHeaders.add(headerName, value);
>-                }
>-            }
>-        }
>-        m_httpHeaderFields = WTFMove(filteredHeaders);
>-    }
>+    case SanitizationType::CrossOriginSafe:
>+        sanitizeHTTPHeaderFieldsAccordingTainting();
>     }
> }
> 
>diff --git a/Source/WebCore/platform/network/ResourceResponseBase.h b/Source/WebCore/platform/network/ResourceResponseBase.h
>index 858faa1b68cab839215b45c6d39f9a78083461e9..0469573024cc21d596eb4905be5ac96f2910c6e9 100644
>--- a/Source/WebCore/platform/network/ResourceResponseBase.h
>+++ b/Source/WebCore/platform/network/ResourceResponseBase.h
>@@ -199,6 +199,7 @@ protected:
> private:
>     void parseCacheControlDirectives() const;
>     void updateHeaderParsedState(HTTPHeaderName);
>+    void sanitizeHTTPHeaderFieldsAccordingTainting();
> 
> protected:
>     bool m_isNull;
>diff --git a/Source/WebKit/NetworkProcess/NetworkResourceLoader.cpp b/Source/WebKit/NetworkProcess/NetworkResourceLoader.cpp
>index 5289943004c7e2d988b412c4aa1f4cd60f9ea501..17ce96cdf03dc0490c7dcea6ab579e515b79ec0a 100644
>--- a/Source/WebKit/NetworkProcess/NetworkResourceLoader.cpp
>+++ b/Source/WebKit/NetworkProcess/NetworkResourceLoader.cpp
>@@ -600,16 +600,9 @@ void NetworkResourceLoader::continueWillSendRedirectedRequest(WebCore::ResourceR
> 
> ResourceResponse NetworkResourceLoader::sanitizeResponseIfPossible(ResourceResponse&& response, ResourceResponse::SanitizationType type)
> {
>-    if (m_parameters.shouldRestrictHTTPResponseAccess) {
>-        if (type == ResourceResponse::SanitizationType::CrossOriginSafe) {
>-            // We reduce filtering when it would otherwise be visible to scripts.
>-            // FIXME: We should use response tainting once computed in Network Process.
>-            bool isSameOrigin = m_parameters.sourceOrigin ? m_parameters.sourceOrigin->canRequest(response.url()) : protocolHostAndPortAreEqual(response.url(), m_parameters.request.url());
>-            if (isSameOrigin && m_parameters.options.destination == FetchOptions::Destination::EmptyString)
>-                type = ResourceResponse::SanitizationType::RemoveCookies;
>-        }
>+    if (m_parameters.shouldRestrictHTTPResponseAccess)
>         response.sanitizeHTTPHeaderFields(type);
>-    }
>+
>     return WTFMove(response);
> }
> 
>diff --git a/Source/WebKit/WebProcess/Network/WebLoaderStrategy.cpp b/Source/WebKit/WebProcess/Network/WebLoaderStrategy.cpp
>index 86b9509f44290ccae2ff33a9ef985686902557df..f46d57c3e0695e60c63226b7ab13178208fcfd69 100644
>--- a/Source/WebKit/WebProcess/Network/WebLoaderStrategy.cpp
>+++ b/Source/WebKit/WebProcess/Network/WebLoaderStrategy.cpp
>@@ -301,20 +301,20 @@ void WebLoaderStrategy::scheduleLoadFromNetworkProcess(ResourceLoader& resourceL
>     }
> #endif
> 
>+    // FIXME: All loaders should provide their origin if navigation mode is cors/no-cors/same-origin.
>+    // As a temporary approach, we use the document origin if available or the HTTP Origin header otherwise.
>+    if (resourceLoader.isSubresourceLoader())
>+        loadParameters.sourceOrigin = static_cast<SubresourceLoader&>(resourceLoader).origin();
>+
>+    if (!loadParameters.sourceOrigin && document)
>+        loadParameters.sourceOrigin = &document->securityOrigin();
>+    if (!loadParameters.sourceOrigin) {
>+        auto origin = request.httpOrigin();
>+        if (!origin.isNull())
>+            loadParameters.sourceOrigin = SecurityOrigin::createFromString(origin);
>+    }
>+
>     if (loadParameters.options.mode != FetchOptions::Mode::Navigate) {
>-        // FIXME: All loaders should provide their origin if navigation mode is cors/no-cors/same-origin.
>-        // As a temporary approach, we use the document origin if available or the HTTP Origin header otherwise.
>-        if (resourceLoader.isSubresourceLoader())
>-            loadParameters.sourceOrigin = static_cast<SubresourceLoader&>(resourceLoader).origin();
>-
>-        auto* document = resourceLoader.frame() ? resourceLoader.frame()->document() : nullptr;
>-        if (!loadParameters.sourceOrigin && document)
>-            loadParameters.sourceOrigin = &document->securityOrigin();
>-        if (!loadParameters.sourceOrigin) {
>-            auto origin = request.httpOrigin();
>-            if (!origin.isNull())
>-                loadParameters.sourceOrigin = SecurityOrigin::createFromString(origin);
>-        }
>         ASSERT(loadParameters.sourceOrigin);
>         if (!loadParameters.sourceOrigin) {
>             scheduleInternallyFailedLoad(resourceLoader);
>@@ -322,8 +322,7 @@ void WebLoaderStrategy::scheduleLoadFromNetworkProcess(ResourceLoader& resourceL
>         }
>     }
> 
>-    // FIXME: We should also sanitize redirect response for navigations.
>-    loadParameters.shouldRestrictHTTPResponseAccess = RuntimeEnabledFeatures::sharedFeatures().restrictedHTTPResponseAccess() && resourceLoader.options().mode != FetchOptions::Mode::Navigate;
>+    loadParameters.shouldRestrictHTTPResponseAccess = RuntimeEnabledFeatures::sharedFeatures().restrictedHTTPResponseAccess();
> 
>     bool isMainFrameNavigation = resourceLoader.frame() && resourceLoader.frame()->isMainFrame() && resourceLoader.options().mode == FetchOptions::Mode::Navigate;
> 
>@@ -663,4 +662,9 @@ NetworkLoadMetrics WebLoaderStrategy::networkMetricsFromResourceLoadIdentifier(u
>     return networkMetrics;
> }
> 
>+bool WebLoaderStrategy::isDoingLoadingSecurityChecks() const
>+{
>+    return RuntimeEnabledFeatures::sharedFeatures().restrictedHTTPResponseAccess();
>+}
>+
> } // namespace WebKit
>diff --git a/Source/WebKit/WebProcess/Network/WebLoaderStrategy.h b/Source/WebKit/WebProcess/Network/WebLoaderStrategy.h
>index 65786330c2a8bcce174ae499488522df8e3e9524..14814a92e794fdbf7f88084073a62e45c2aadebb 100644
>--- a/Source/WebKit/WebProcess/Network/WebLoaderStrategy.h
>+++ b/Source/WebKit/WebProcess/Network/WebLoaderStrategy.h
>@@ -83,8 +83,6 @@ public:
>     void addOnlineStateChangeListener(Function<void(bool)>&&) final;
>     void setOnLineState(bool);
> 
>-    bool isDoingLoadingSecurityChecks() const final { return true; }
>-
> private:
>     void scheduleLoad(WebCore::ResourceLoader&, WebCore::CachedResource*, bool shouldClearReferrerOnHTTPSToHTTPRedirect);
>     void scheduleInternallyFailedLoad(WebCore::ResourceLoader&);
>@@ -95,6 +93,8 @@ private:
>     WebCore::ResourceResponse responseFromResourceLoadIdentifier(uint64_t resourceLoadIdentifier) final;
>     WebCore::NetworkLoadMetrics networkMetricsFromResourceLoadIdentifier(uint64_t resourceLoadIdentifier) final;
> 
>+    bool isDoingLoadingSecurityChecks() const final;
>+
>     HashSet<RefPtr<WebCore::ResourceLoader>> m_internallyFailedResourceLoaders;
>     RunLoop::Timer<WebLoaderStrategy> m_internallyFailedLoadTimer;
>

Actions: View | Formatted Diff | Diff

Attachments on bug 184892: 338918 | 338935 | 338944 | 339020 | 339033 | 339045 | 339118 | 339121 | 339128 | 339145 | 339327