WebKit Bugzilla
New
Browse
Log In
×
Sign in with GitHub
or
Remember my login
Create Account
·
Forgot Password
Forgotten password account recovery
RESOLVED FIXED
184804
CVE-2018-11712
REGRESSION(
r228088
): [SOUP] Certificate verification no longer performed for secure WebSocket connections
https://bugs.webkit.org/show_bug.cgi?id=184804
Summary
REGRESSION(r228088): [SOUP] Certificate verification no longer performed for ...
Michael Catanzaro
Reported
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.
Attachments
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
Show Obsolete
(1)
View All
Add attachment
proposed patch, testcase, etc.
Radar WebKit Bug Importer
Comment 1
2018-04-19 20:00:17 PDT
<
rdar://problem/39585887
>
Michael Catanzaro
Comment 2
2018-04-19 20:09:41 PDT
Created
attachment 338389
[details]
Patch
Michael Catanzaro
Comment 3
2018-04-19 20:10:34 PDT
Comment on
attachment 338389
[details]
Patch (It's not ready for review, because it doesn't fix the bug. Necessary, not sufficient.)
Michael Catanzaro
Comment 4
2018-04-19 20:23:43 PDT
Comment on
attachment 338389
[details]
Patch 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.)
Carlos Garcia Campos
Comment 5
2018-04-20 03:01:58 PDT
Comment on
attachment 338389
[details]
Patch 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.
Carlos Garcia Campos
Comment 6
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.
Carlos Garcia Campos
Comment 7
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:
https://git.gnome.org/browse/libsoup/commit/?id=e8995d4e1d5cf984cf10327c59808976425b2f9c
Michael Catanzaro
Comment 8
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
handshaking
> 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"
Michael Catanzaro
Comment 9
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.
Carlos Garcia Campos
Comment 10
2018-04-20 23:36:26 PDT
Committed
r230886
: <
https://trac.webkit.org/changeset/230886
>
Michael Catanzaro
Comment 11
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.
Michael Catanzaro
Comment 12
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.
Michael Catanzaro
Comment 13
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.
Michael Catanzaro
Comment 14
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.
Note
You need to
log in
before you can comment on or make changes to this bug.
Top of Page
Format For Printing
XML
Clone This Bug