Bug 194208 (CVE-2019-6251) - [WPE][GTK] URI spoofing when JS redirects page to something that takes a long time to load
Summary: [WPE][GTK] URI spoofing when JS redirects page to something that takes a long...
Status: RESOLVED FIXED
Alias: CVE-2019-6251
Product: Security
Classification: Unclassified
Component: Security (show other bugs)
Version: WebKit Nightly Build
Hardware: PC Linux
: P2 Normal
Assignee: WebKit Security Group
URL: https://gitlab.gnome.org/GNOME/epipha...
Keywords: InRadar
Depends on: 194131
Blocks:
  Show dependency treegraph
 
Reported: 2019-02-03 17:43 PST by Michael Catanzaro
Modified: 2019-05-02 16:23 PDT (History)
9 users (show)

See Also:


Attachments
Patch (6.53 KB, patch)
2019-02-03 19:16 PST, Michael Catanzaro
no flags Details | Formatted Diff | Diff
Patch (2.74 KB, patch)
2019-03-22 06:41 PDT, Carlos Garcia Campos
mcatanzaro: review-
Details | Formatted Diff | Diff
Patch (4.23 KB, patch)
2019-03-23 02:47 PDT, Carlos Garcia Campos
mcatanzaro: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Michael Catanzaro 2019-02-03 17:43:03 PST
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.
Comment 1 Radar WebKit Bug Importer 2019-02-03 17:45:26 PST
<rdar://problem/47776021>
Comment 2 Radar WebKit Bug Importer 2019-02-03 17:48:31 PST
<rdar://problem/47776040>
Comment 3 Michael Catanzaro 2019-02-03 19:16:09 PST
Created attachment 361034 [details]
Patch
Comment 4 Michael Catanzaro 2019-02-03 19:16:53 PST
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.
Comment 6 Michael Catanzaro 2019-03-15 09:55:29 PDT
(This is already public elsewhere, but Security-Sensitive tag should avoid spam.)
Comment 7 Carlos Garcia Campos 2019-03-22 06:41:41 PDT
Created attachment 365721 [details]
Patch
Comment 8 Michael Catanzaro 2019-03-22 09:16:57 PDT
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.
Comment 9 Michael Catanzaro 2019-03-22 09:19:06 PDT
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/
Comment 10 Carlos Garcia Campos 2019-03-23 02:41:02 PDT
(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
Comment 11 Carlos Garcia Campos 2019-03-23 02:47:48 PDT
Created attachment 365808 [details]
Patch
Comment 12 Michael Catanzaro 2019-03-24 10:47:35 PDT
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, ...
Comment 13 Carlos Garcia Campos 2019-03-25 02:12:04 PDT
Committed r243434: <https://trac.webkit.org/changeset/243434>