Bug (CVE-2019-6251) – [WPE][GTK] URI spoofing when JS redirects page to something that takes a long time to load

• Component: Security
• Status: RESOLVED FIXED

[Michael Catanzaro]

Epiphany is vulnerable to a URI spoofing attack when JS redirects the page to something that takes a long time to load. See https://gitlab.gnome.org/GNOME/epiphany/issues/532 for full details.

The solution is to just not display any URI changes that occur after LOAD_COMMITTED until we have hit LOAD_FINISHED. We could easily solve this at the Epiphany level, but any application using the WPE/GTK API would be affected, so it seems better to handle it in WebKit instead.

[Radar WebKit Bug Importer]

<rdar://problem/47776021>

[Radar WebKit Bug Importer]

<rdar://problem/47776040>

[Michael Catanzaro]

Created attachment 361034 [details]
Patch

[Michael Catanzaro]

Not really happy with this, since it desyncs our URI from WebCore's... but this is the best I was able to come up with.

[Michael Catanzaro]

(This is already public elsewhere, but Security-Sensitive tag should avoid spam.)

[Carlos Garcia Campos]

Created attachment 365721 [details]
Patch

[Michael Catanzaro]

Comment on attachment 365721 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=365721&action=review

I was working on this last night and tried several things, but none were correct. I think what you have here might be on the right track, but when the URI is un-blocked, the URI change is not effected. Just browse a few pages and notice that URI is never updated except on API requests or HTTP redirects. So we need to update the URI somewhere. Now, if we do that update after LOAD_STARTED, where you unblock it in this patch, then the spoofing will succeed and this change will fail, so the right place to do the update must be after LOAD_COMMITTED, right? That might look bad, though, because now users won't be able to see redirections happen, but I think, with that changed, this will still be better than my attempts.

> Source/WebKit/UIProcess/API/glib/WebKitWebView.cpp:2086
> +    // Ignore the active URI changes happening before WEBKIT_LOAD_STARTED. In case of API request,
> +    // the active URI is already the pending API request URL.

I think the comment needs more explanation to make us less likely to reintroduce this bug. E.g.:

// Ignore the active URI changes happening before WEBKIT_LOAD_STARTED. If they are not user-initiated,
// they could be a malicious attempt to trick users by loading an invalid URI on a trusted host, with the load
// intended to stall, or perhaps be repeated. If we trust the URI here and display it to the user, then the user's
// only indication that something is wrong would be a page loading indicator. If the load request is not
// user-initiated, we must not trust it until WEBKIT_LOAD_COMMITTED. If the load is triggered by API
// request, then the active URI is already the pending API request URL, so the blocking is harmless and the
// client application will still see the URI update immediately. Otherwise, the URI update will be delayed a bit.

Note my comment matches my suggestion above, but not what you've implemented.

[Michael Catanzaro]

E.g. load https://www.igalia.com/ by API request. Then click on Services in the top bar.

Expected URI is: https://www.igalia.com/services/
But actual URI is still: https://www.igalia.com/

[Carlos Garcia Campos]

(In reply to Michael Catanzaro from comment #8)
> Comment on attachment 365721 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=365721&action=review
> 
> I was working on this last night and tried several things, but none were
> correct. I think what you have here might be on the right track, but when
> the URI is un-blocked, the URI change is not effected. Just browse a few
> pages and notice that URI is never updated except on API requests or HTTP
> redirects.

Right, I assumed it was going to be updated on committed, but the url hasn't changed in the page load state, so we need to check it manually. HTTP redirects should work, though, because the URL will change in the page load state.

> So we need to update the URI somewhere. Now, if we do that update
> after LOAD_STARTED, where you unblock it in this patch, then the spoofing
> will succeed and this change will fail, so the right place to do the update
> must be after LOAD_COMMITTED, right?

Yes, committed is the right place.

> That might look bad, though, because
> now users won't be able to see redirections happen, but I think, with that
> changed, this will still be better than my attempts.

No, redirections should work because they happen after provisional load started and the URL changes in the page load state.

> > Source/WebKit/UIProcess/API/glib/WebKitWebView.cpp:2086
> > +    // Ignore the active URI changes happening before WEBKIT_LOAD_STARTED. In case of API request,
> > +    // the active URI is already the pending API request URL.
> 
> I think the comment needs more explanation to make us less likely to
> reintroduce this bug. E.g.:
> 
> // Ignore the active URI changes happening before WEBKIT_LOAD_STARTED. If
> they are not user-initiated,
> // they could be a malicious attempt to trick users by loading an invalid
> URI on a trusted host, with the load
> // intended to stall, or perhaps be repeated. If we trust the URI here and
> display it to the user, then the user's
> // only indication that something is wrong would be a page loading
> indicator. If the load request is not
> // user-initiated, we must not trust it until WEBKIT_LOAD_COMMITTED. If the
> load is triggered by API
> // request, then the active URI is already the pending API request URL, so
> the blocking is harmless and the
> // client application will still see the URI update immediately. Otherwise,
> the URI update will be delayed a bit.
> 
> Note my comment matches my suggestion above, but not what you've implemented.

Ok, thanks

[Carlos Garcia Campos]

Created attachment 365808 [details]
Patch

[Michael Catanzaro]

Comment on attachment 365808 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=365808&action=review

OK, so the trick is to unblock during LOAD_STARTED, then restore during LOAD_COMMITTED. Really good job. I don't immediately notice any regressions.

> Source/WebKit/ChangeLog:4
> +        Need the bug URL (OOPS!).

You'll have to fix the URL before it lands. Our scripts don't like security bugs.

> Source/WebKit/UIProcess/API/glib/WebKitWebView.cpp:2120
> +        // Active URL is trusted now, if it's different to our active URI, due to the

Active URL is trusted now. If it's different to our active URI, ...

[Carlos Garcia Campos]

Committed r243434: <https://trac.webkit.org/changeset/243434>

[Michael Catanzaro]

Finally, I've noticed multiple regressions. Look at Epiphany's load_changed_cb:

    case WEBKIT_LOAD_STARTED: {
      const char *loading_uri = NULL;

      view->load_failed = FALSE;

      if (view->snapshot_timeout_id) {
        g_source_remove (view->snapshot_timeout_id);
        view->snapshot_timeout_id = 0;
      }

      loading_uri = webkit_web_view_get_uri (web_view); // PROBLEM!

      if (ephy_embed_utils_is_no_show_address (loading_uri))
        ephy_web_view_freeze_history (view);

      if (view->address == NULL || view->address[0] == '\0')
        ephy_web_view_set_address (view, loading_uri);

      ephy_web_view_set_loading_message (view, loading_uri);


      if (!view->reader_loading) {
        g_clear_pointer (&view->reader_byline, g_free);
        g_clear_pointer (&view->reader_content, g_free);
        view->reader_active = FALSE;
      }

      g_object_notify_by_pspec (G_OBJECT (view), obj_properties[PROP_READER_MODE]);

      break;
    }

Notice the line I marked with // PROBLEM! After this commit, the URI there can now be stale. It has two undesirable effects:

 * We freeze the history database if the *current* page is not supposed to go into history, not if the *loading* page is. So, when starting from about:overview and clicking on any of the overview thumbnails, the load will not enter history because about:overview is not supposed to enter history. Epiphany thinks it is loading about:overview again.
 * The loading message is set incorrectly. E.g. load https://duckduckgo.com/about, scroll down to the DuckDuckGo Blog section, and click any of the links that go do https://spreadprivacy.com. The loading message (in Epiphany's floating status bar) should say "Loading https://spreadprivacy.com" but instead says "Loading https://duckduckgo.com", which is incorrect.

But this all is an unavoidable result of this change: delaying the URI update is the only plausible way to fix the security bug, after all. So I think all we can do is update our documentation to warn developers that URI must not be relied on until LOAD_COMMITTED.

It also makes me wonder whether it's possible to do much of use in LOAD_REDIRECTED anymore.

[Michael Catanzaro]

https://gitlab.gnome.org/GNOME/epiphany/merge_requests/403

I recommend an update to the webkit_web_view_get_uri() documentation at least, and probably also WebKitWebView::load-changed since this is a significant footgun.

[Adrian Perez]

I think that the regressions observed by Michael very likely
have the same root cause as bug #200341 — Carlos García was
already looking into it, we should probably check with him.

[Michael Catanzaro]

Looks to me like two different issues with similar symptoms.