REGRESSION(r228088): [SOUP] Certificate verification no longer performed for secure WebSocket connections

• Component: Security
• Status: RESOLVED FIXED

[Michael Catanzaro]

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.

[Radar WebKit Bug Importer]

<rdar://problem/39585887>

[Michael Catanzaro]

Created attachment 338389 [details]
Patch

[Michael Catanzaro]

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 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 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]

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]

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 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]

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]

Committed r230886: <https://trac.webkit.org/changeset/230886>

[Michael Catanzaro]

(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]

(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]

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]

(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.