Attachment 361034 Details for Bug 194208 – Patch

[patch] Patch

bug-194208-20190203211608.patch (text/plain), 6.53 KB, created by Michael Catanzaro on 2019-02-03 19:16:09 PST

(hide)

Description:

Filename:

MIME Type:

Creator: Michael Catanzaro

Created: 2019-02-03 19:16:09 PST

Size: 6.53 KB

patch

obsolete

>Subversion Revision: 240850
>diff --git a/Source/WebKit/ChangeLog b/Source/WebKit/ChangeLog
>index f789ec2ed670cea373589ba818df877c7a95944f..00d68c777511ebe70240af6b95d99a98db9e6819 100644
>--- a/Source/WebKit/ChangeLog
>+++ b/Source/WebKit/ChangeLog
>@@ -1,3 +1,33 @@
>+2019-02-03  Michael Catanzaro  <mcatanzaro@igalia.com>
>+
>+        (CVE-2019-6251) [WPE][GTK] URI issue when JS redirects page to something that takes a long time to load
>+        https://bugs.webkit.org/show_bug.cgi?id=194208
>+        <rdar://problem/47776021>
>+
>+        Reviewed by NOBODY (OOPS!).
>+
>+        We must not allow URI updates between committed and finished, because this is when JS could
>+        trigger redirects that will not ever complete, for the purpose of displaying problematic
>+        content with a trusted URI in the address bar. The user's only indication that something
>+        might be wrong would be a spinner.
>+
>+        I tried writing an API test for this, but it's not possible because we want to unfreeze the
>+        URI immediately before emitting finished, of course, but then a test has no way to know if
>+        it's happening too soon or not, because the test just knows the load hasn't finished yet.
>+
>+        My solution relies on the fact that finished is now always emitted even if the load never
>+        finishes, but this is also a hindrance, because the problematic page could circumvent this
>+        new protection by just starting a new load (causing the previous one to "finish"), so we
>+        also need to notify the web view when a new load occurs before the previous one finished,
>+        and reset the URI in this case. (This part might be testable, but I haven't tried.)
>+
>+        * UIProcess/API/glib/WebKitNavigationClient.cpp:
>+        (NavigationClient::NavigationClient):
>+        * UIProcess/API/glib/WebKitWebView.cpp:
>+        (webkitWebViewLoadChanged):
>+        (webkitWebViewResetToLastCommittedURI):
>+        * UIProcess/API/glib/WebKitWebViewPrivate.h:
>+
> 2019-02-03  Michael Catanzaro  <mcatanzaro@igalia.com>
> 
>         [WPE][GTK] Load events may occur in unexpected order when JS redirects page before subresource load finishes
>diff --git a/Source/WebKit/UIProcess/API/glib/WebKitNavigationClient.cpp b/Source/WebKit/UIProcess/API/glib/WebKitNavigationClient.cpp
>index fc1c943123c5d2e48d7fd7cbdac8749f7e1b478a..a5d0de83b44e5eb9c89396eec807276b711a3aef 100644
>--- a/Source/WebKit/UIProcess/API/glib/WebKitNavigationClient.cpp
>+++ b/Source/WebKit/UIProcess/API/glib/WebKitNavigationClient.cpp
>@@ -101,7 +101,13 @@ class NavigationClient : public API::NavigationClient {
> public:
>     explicit NavigationClient(WebKitWebView* webView)
>         : m_webView(webView)
>-        , m_navigationStateTracker([this] { webkitWebViewLoadChanged(m_webView, WEBKIT_LOAD_FINISHED); })
>+        , m_navigationStateTracker([this] {
>+            // We have to reset the URI here because a malicious page could
>+            // attempt to spoof a URI on a legitimate host if it knows that
>+            // the load won't complete.
>+            webkitWebViewResetToLastCommittedURI(m_webView);
>+            webkitWebViewLoadChanged(m_webView, WEBKIT_LOAD_FINISHED);
>+        })
>     {
>     }
> 
>diff --git a/Source/WebKit/UIProcess/API/glib/WebKitWebView.cpp b/Source/WebKit/UIProcess/API/glib/WebKitWebView.cpp
>index 2979b8a5238c7e8d635bdb440f52948c5fc8534f..775db5594c466e2613084fcc7be8a45373711ec7 100644
>--- a/Source/WebKit/UIProcess/API/glib/WebKitWebView.cpp
>+++ b/Source/WebKit/UIProcess/API/glib/WebKitWebView.cpp
>@@ -246,9 +246,11 @@ struct _WebKitWebViewPrivate {
>     CString title;
>     CString customTextEncoding;
>     CString activeURI;
>-    bool isLoading;
>-    bool isEphemeral;
>-    bool isControlledByAutomation;
>+    CString lastCommittedURI;
>+    bool uriUpdatesAreBlocked : 1;
>+    bool isLoading : 1;
>+    bool isEphemeral : 1;
>+    bool isControlledByAutomation : 1;
> 
>     std::unique_ptr<PageLoadStateObserver> loadObserver;
> 
>@@ -360,7 +362,8 @@ private:
>     void didChangeActiveURL() override
>     {
>         m_webView->priv->activeURI = getPage(m_webView).pageLoadState().activeURL().utf8();
>-        g_object_notify(G_OBJECT(m_webView), "uri");
>+        if (!m_webView->priv->uriUpdatesAreBlocked)
>+            g_object_notify(G_OBJECT(m_webView), "uri");
>         g_object_thaw_notify(G_OBJECT(m_webView));
>     }
> 
>@@ -2070,16 +2073,20 @@ void webkitWebViewLoadChanged(WebKitWebView* webView, WebKitLoadEvent loadEvent)
>         priv->loadingResourcesMap.clear();
>         priv->mainResource = nullptr;
>         break;
>-#if PLATFORM(GTK)
>     case WEBKIT_LOAD_COMMITTED: {
>+#if PLATFORM(GTK)
>         WebKitFaviconDatabase* database = webkit_web_context_get_favicon_database(priv->context.get());
>         GUniquePtr<char> faviconURI(webkit_favicon_database_get_favicon_uri(database, priv->activeURI.data()));
>         webkitWebViewUpdateFaviconURI(webView, faviconURI.get());
>+#endif
>+        priv->uriUpdatesAreBlocked = true;
>         break;
>     }
>-#endif
>     case WEBKIT_LOAD_FINISHED:
>         webkitWebViewCancelAuthenticationRequest(webView);
>+        priv->uriUpdatesAreBlocked = false;
>+        if (priv->activeURI != priv->lastCommittedURI)
>+            g_object_notify(G_OBJECT(webView), "uri");
>         break;
>     default:
>         break;
>@@ -2088,6 +2095,11 @@ void webkitWebViewLoadChanged(WebKitWebView* webView, WebKitLoadEvent loadEvent)
>     g_signal_emit(webView, signals[LOAD_CHANGED], 0, loadEvent);
> }
> 
>+void webkitWebViewResetToLastCommittedURI(WebKitWebView* webView)
>+{
>+    webView->priv->activeURI = webView->priv->lastCommittedURI;
>+}
>+
> void webkitWebViewLoadFailed(WebKitWebView* webView, WebKitLoadEvent loadEvent, const char* failingURI, GError *error)
> {
>     webkitWebViewCancelAuthenticationRequest(webView);
>diff --git a/Source/WebKit/UIProcess/API/glib/WebKitWebViewPrivate.h b/Source/WebKit/UIProcess/API/glib/WebKitWebViewPrivate.h
>index e9ef60c577c4541b872092d732444a7eabad9a4d..fb032439e1a99312c0d6fee1e0cce1818b77f178 100644
>--- a/Source/WebKit/UIProcess/API/glib/WebKitWebViewPrivate.h
>+++ b/Source/WebKit/UIProcess/API/glib/WebKitWebViewPrivate.h
>@@ -42,6 +42,7 @@
> void webkitWebViewCreatePage(WebKitWebView*, Ref<API::PageConfiguration>&&);
> WebKit::WebPageProxy& webkitWebViewGetPage(WebKitWebView*);
> void webkitWebViewLoadChanged(WebKitWebView*, WebKitLoadEvent);
>+void webkitWebViewResetToLastCommittedURI(WebKitWebView*);
> void webkitWebViewLoadFailed(WebKitWebView*, WebKitLoadEvent, const char* failingURI, GError*);
> void webkitWebViewLoadFailedWithTLSErrors(WebKitWebView*, const char* failingURI, GError*, GTlsCertificateFlags, GTlsCertificate*);
> #if PLATFORM(GTK)

Actions: View | Formatted Diff | Diff

Attachments on bug 194208: 361034 | 365721 | 365808