Bug 184804 (CVE-2018-11712) - REGRESSION(r228088): [SOUP] Certificate verification no longer performed for secure WebSocket connections
Summary: REGRESSION(r228088): [SOUP] Certificate verification no longer performed for ...
Alias: CVE-2018-11712
Product: Security
Classification: Unclassified
Component: Security (show other bugs)
Version: WebKit Nightly Build
Hardware: PC Linux
: P2 Normal
Assignee: WebKit Security Group
Keywords: InRadar
Depends on: 184480
  Show dependency treegraph
Reported: 2018-04-19 20:00 PDT by Michael Catanzaro
Modified: 2018-06-04 08:09 PDT (History)
7 users (show)

See Also:

Patch (49.57 KB, patch)
2018-04-19 20:09 PDT, Michael Catanzaro
no flags Details | Formatted Diff | Diff
Simpler patch (12.12 KB, patch)
2018-04-20 03:05 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 2018-04-19 20:00:01 PDT
r228088 changes SocketStreamHandleImplSoup to use the new soup_session_connection_async() API, which Carlos had just created for use by SocketStreamHandleImplSoup in bug #126384. Problem is this turned off certificate verification, since we disable SOUP_SESSION_SSL_STRICT.

For normal connections, we work around this using SoupMessage (see bug #184480 for why that is not quite right), but there is no possibility of doing that with soup_session_connect_async(). It's impossible to fix without changes in libsoup. We need to either (a) change soup_session_connect_async() to somehow force ssl-strict on the SoupConnection and SoupSocket it creates, or (b) add API for getting the SoupSocket so WebKit can force it on manually. I even tried finding some way to block the accept-certificate handler that SoupSocket is using to accept the certificate, since we want to reimplement that, but that would be a nasty hack and I don't think it's possible anyway.

We should probably add some generic API in libsoup to wrap accept-certificate, so it would be useful for bug #184480 as well. But I'm not quite sure what it would look like. A SoupMessage API seems the most natural, but that's insufficient because SoupMessage is not exposed at all by soup_session_connect_async(). A SoupSession API seems easiest, I guess. Thoughts?

Here is a WIP patch with a testcase. The test found some other longstanding problems here, too: we were ignoring the TLS error policy set by webkit_web_context_set_tls_errors_policy(), and ignoring any overrides set with webkit_web_context_allow_tls_certificate_for_host(). These aren't security problems, though, since they only ever caused the connection to fail when it shouldn't have. Anyway, it was because we were instead checking an insane DeprecatedGlobalSettings::allowsAnySSLCertificate setting, which is used exclusively for web sockets. This patch solves this issue and renames all of its uses in Source/ to indicate it's used only for testing (and a couple service worker functions too!). I didn't change its usage in LayoutTests, though.
Comment 1 Radar WebKit Bug Importer 2018-04-19 20:00:17 PDT
Comment 2 Michael Catanzaro 2018-04-19 20:09:41 PDT
Created attachment 338389 [details]
Comment 3 Michael Catanzaro 2018-04-19 20:10:34 PDT
Comment on attachment 338389 [details]

(It's not ready for review, because it doesn't fix the bug. Necessary, not sufficient.)
Comment 4 Michael Catanzaro 2018-04-19 20:23:43 PDT
Comment on attachment 338389 [details]

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

> Source/WebCore/testing/InternalSettings.cpp:-844
> -#if USE(SOUP)
> -    SoupNetworkSession::setShouldIgnoreTLSErrors(allowsAnyCertificate);
> -#endif

(If it breaks layout tests, this would probably be why.)
Comment 5 Carlos Garcia Campos 2018-04-20 03:01:58 PDT
Comment on attachment 338389 [details]

There's too much noise in this patch to be a security fix, note that I'll have to backport it to 2.20 branch. So, I would simply fix the bug here, and then do the renames, cleanups and update jhbuild in follow up patches.
Comment 6 Carlos Garcia Campos 2018-04-20 03:05:00 PDT
Created attachment 338404 [details]
Simpler patch

This patch fixes the bug if applied on top of patch attached to bug #184480. I've fixed the bug and copied the test case added by Michael in the previous patch. I also updated the test to move the asserts from the fixture to the test and simplify it a bit.
Comment 7 Carlos Garcia Campos 2018-04-20 03:29:53 PDT
Michael, if you are going to bump libsoup version in jhbuild, remember to include a patch for this commit:

Comment 8 Michael Catanzaro 2018-04-20 08:51:38 PDT
Comment on attachment 338404 [details]
Simpler patch

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

I see you simplified several aspects of my code. Excellent!

I'll create another bug to deal with the insane testing setting.

> Source/WebCore/ChangeLog:3
> +        REGRESSION(r228088): [SOUP] Check TLS errors for WebSockets right after TLS handshacking event


> Source/WebCore/platform/network/soup/SocketStreamHandleImplSoup.cpp:92
> +        reinterpret_cast<SoupSessionConnectProgressCallback>(connectProgressCallback),

I would change the ternary condition to url.protocolIs("wss") instead of removing it entirely, to avoid the unnecessary progress callbacks otherwise.

> Tools/TestWebKitAPI/Tests/WebKitGLib/TestSSL.cpp:348
> +        DidReachServer = 1 << 0,

This flag is misnamed. Of course the request will always reach the server, because the client won't be able to check for TLS errors if the server has not sent it anything. Rename it, perhaps: "DidServerCompleteHandshake"
Comment 9 Michael Catanzaro 2018-04-20 08:53:49 PDT
I'm going to request a CVE for this one.

Bug #184480 is really minor since we don't support TLS 1.3 or fast open, so that doesn't need a CVE.
Comment 10 Carlos Garcia Campos 2018-04-20 23:36:26 PDT
Committed r230886: <https://trac.webkit.org/changeset/230886>
Comment 11 Michael Catanzaro 2018-06-03 07:56:03 PDT
(In reply to Michael Catanzaro from comment #9)
> Bug #184480 is really minor since we don't support TLS 1.3 or fast open, so
> that doesn't need a CVE.

But note this fix depends on that one.
Comment 12 Michael Catanzaro 2018-06-03 08:12:39 PDT
(In reply to Michael Catanzaro from comment #9)
> I'm going to request a CVE for this one.
> Bug #184480 is really minor since we don't support TLS 1.3 or fast open, so
> that doesn't need a CVE.

It's been six weeks without a response from the DWF project. I've asked them to not issue the CVE after all, and requested one from Debian instead. Hopefully we don't wind up with a duplicate.
Comment 13 Michael Catanzaro 2018-06-04 08:06:08 PDT
And MITRE responds in one day... phenomenal, really.

I don't understand what DWF is trying to accomplish, they are just getting in the way.
Comment 14 Michael Catanzaro 2018-06-04 08:08:00 PDT
(In reply to Michael Catanzaro from comment #13)
> And MITRE responds in one day... phenomenal, really.

Oh yeah, Debian directed me to apply from MITRE.