Bug 162910 – [SOUP] Move global TLS errors handling from ResourceHandle to SoupNetworkSession

• Component: Platform
• Status: RESOLVED FIXED

[Carlos Garcia Campos]

So that it will be shared with network session code.

[Carlos Garcia Campos]

Created attachment 290607 [details]
Patch

[Michael Catanzaro]

Comment on attachment 290607 [details]
Patch

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

I know this is a preexisting problem, but please fix it first: either use a secure hash algorithm in HostTLSCertificateSet (e.g. SHA-256 from CryptoDigest.h), or else just stop using hashes there and copy the full certificates into the hash table. This is a sufficiently-minor issue that I don't think we need to get a CVE for it, but it needs fixed.

> Source/WebCore/platform/network/soup/SoupNetworkSession.cpp:67
> +    bool contains(GTlsCertificate* certificate)

Should be const (preexisting issue)

> Source/WebCore/platform/network/soup/SoupNetworkSession.h:68
> +    static void setShouldIgnoreTLSErrors(bool);
> +    static void checkTLSErrors(SoupRequest*, SoupMessage*, std::function<void (const ResourceError&)>&&);
> +    static void allowSpecificHTTPSCertificateForHost(const CertificateInfo&, const String& host);

Why make these static? Doesn't it defeat the point of moving this code to SoupNetworkSession? I would expect different network sessions to handle these independently. If you were planning to change this in a future patch, it'd be better to do it now by using SoupNetworkSession::defaultSession() in NetworkProcessSoup.cpp, and fix up NetworkProcessSoup.cpp in a future patch.

[Carlos Garcia Campos]

(In reply to comment #2)
> Comment on attachment 290607 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=290607&action=review
> 
> I know this is a preexisting problem, but please fix it first: either use a
> secure hash algorithm in HostTLSCertificateSet (e.g. SHA-256 from
> CryptoDigest.h), or else just stop using hashes there and copy the full
> certificates into the hash table. This is a sufficiently-minor issue that I
> don't think we need to get a CVE for it, but it needs fixed.

What's the problem of using SHA1 here? What's the security problem exactly? We are using it just as a hash, for non private information, as an optimization to avoid having to do comparisons with large certificate contents. It's not the only place in WebKit that SHA1 is used, I don't mind if the algorithm is not secure enough for this particular use case.

> > Source/WebCore/platform/network/soup/SoupNetworkSession.cpp:67
> > +    bool contains(GTlsCertificate* certificate)
> 
> Should be const (preexisting issue)

Yes.

> > Source/WebCore/platform/network/soup/SoupNetworkSession.h:68
> > +    static void setShouldIgnoreTLSErrors(bool);
> > +    static void checkTLSErrors(SoupRequest*, SoupMessage*, std::function<void (const ResourceError&)>&&);
> > +    static void allowSpecificHTTPSCertificateForHost(const CertificateInfo&, const String& host);
> 
> Why make these static? Doesn't it defeat the point of moving this code to
> SoupNetworkSession? I would expect different network sessions to handle
> these independently. If you were planning to change this in a future patch,
> it'd be better to do it now by using SoupNetworkSession::defaultSession() in
> NetworkProcessSoup.cpp, and fix up NetworkProcessSoup.cpp in a future patch.

No, the point, as the ChangeLog says, is using this from NetworkSession that replaces ResourceHandle, SoupNetworkSession is a good place common in both code paths. I'm definitely going to rework the soup session handling, but I'm still not sure how exactly. The TLS policy is a network process global setting anyway, so it should be applied to any network session used. Note that there are only 3 possible sessions: default, private and testing. We don't support private browsing and testing is only used internally by layout tests, so from the users point of view there's only one global session which is the default one.

[Carlos Garcia Campos]

Created attachment 290695 [details]
Updated patch

This should apply now. I've just made the contains() const and replaced a NULL by nullptr. I'm still using SHA1 because I don't see any security implication (we are not signing anything nor hashing a password or private info). I'll update the patch to replace SHA1 if I'm wrong, of course.

[WebKit Commit Bot]

Attachment 290695 [details] did not pass style-queue:


ERROR: Source/WebCore/platform/network/soup/SoupNetworkSession.cpp:332:  Extra space before ( in function call  [whitespace/parens] [4]
ERROR: Source/WebCore/platform/network/soup/SoupNetworkSession.h:67:  Extra space before ( in function call  [whitespace/parens] [4]
Total errors found: 2 in 6 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Alex Christensen]

Comment on attachment 290695 [details]
Updated patch

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

Yep.  Moves the code.  Great.  r=me

> Source/WebCore/platform/network/soup/SoupNetworkSession.cpp:80
> +        SHA1 sha1;

You should probably migrate away from SHA1, it being theoretically not cryptographically secure and all...

> Source/WebKit2/NetworkProcess/soup/NetworkProcessSoup.cpp:86
> -    ResourceHandle::setIgnoreSSLErrors(ignoreTLSErrors);
> +    SoupNetworkSession::setShouldIgnoreTLSErrors(ignoreTLSErrors);

It seems like we should work towards removing this, too.  Very bad things can happen when we ignore TLS errors.

> Source/WebKit2/NetworkProcess/soup/NetworkProcessSoup.cpp:91
> -    ResourceHandle::setClientCertificate(host, certificateInfo.certificate());
> +    SoupNetworkSession::allowSpecificHTTPSCertificateForHost(certificateInfo, host);

We are also moving away from allowing specific TLS certificates.  I'm going to do this on Cocoa by using SecTrustEvaluateAsync with a few additional checks in the NetworkProcess after receiving the server trust evaluation challenge.  This will avoid IPC and allow us to quickly and asynchronously connect to most HTTPS servers that use modern TLS and valid certificates.  In the case where it fails such as on badssl.com we will send IPC to the UIProcess which has the option of responding and saying to trust the server even though it's probably unsafe.

[Alex Christensen]

(In reply to comment #3)
> What's the problem of using SHA1 here? What's the security problem exactly?
I see certificates and SHA1 and think "Somebody could theoretically create a collision and something unexpected could happen and that's really bad with certificates."  I'm not sure what the exact attack would look like, but it's up to you to decide whether that's important enough.

[Carlos Garcia Campos]

(In reply to comment #6)
> Comment on attachment 290695 [details]
> Updated patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=290695&action=review
> 
> Yep.  Moves the code.  Great.  r=me

Thanks!

> > Source/WebCore/platform/network/soup/SoupNetworkSession.cpp:80
> > +        SHA1 sha1;
> 
> You should probably migrate away from SHA1, it being theoretically not
> cryptographically secure and all...

I'm not security expert, but I don't see the security problem here yet, I can understand it's not safe to use SHA1 for signing something or hashing private info like a password, but here we just want to get a checksum to make comparisons.

> > Source/WebKit2/NetworkProcess/soup/NetworkProcessSoup.cpp:86
> > -    ResourceHandle::setIgnoreSSLErrors(ignoreTLSErrors);
> > +    SoupNetworkSession::setShouldIgnoreTLSErrors(ignoreTLSErrors);
> 
> It seems like we should work towards removing this, too.  Very bad things
> can happen when we ignore TLS errors.

This is disabled by default of course, and only enabled when explicitly set by API users. See GTK+ API for example:
 https://webkitgtk.org/reference/webkit2gtk/stable/WebKitWebContext.html#WebKitTLSErrorsPolicy

The default is fail, of course, and web browser applications should never change that setting.

> > Source/WebKit2/NetworkProcess/soup/NetworkProcessSoup.cpp:91
> > -    ResourceHandle::setClientCertificate(host, certificateInfo.certificate());
> > +    SoupNetworkSession::allowSpecificHTTPSCertificateForHost(certificateInfo, host);
> 
> We are also moving away from allowing specific TLS certificates.  I'm going
> to do this on Cocoa by using SecTrustEvaluateAsync with a few additional
> checks in the NetworkProcess after receiving the server trust evaluation
> challenge.  This will avoid IPC and allow us to quickly and asynchronously
> connect to most HTTPS servers that use modern TLS and valid certificates. 
> In the case where it fails such as on badssl.com we will send IPC to the
> UIProcess which has the option of responding and saying to trust the server
> even though it's probably unsafe.

I'll see if we can do something similar.

[Carlos Garcia Campos]

(In reply to comment #7)
> (In reply to comment #3)
> > What's the problem of using SHA1 here? What's the security problem exactly?
> I see certificates and SHA1 and think "Somebody could theoretically create a
> collision and something unexpected could happen and that's really bad with
> certificates."  I'm not sure what the exact attack would look like, but it's
> up to you to decide whether that's important enough.

hmm, I see, thanks for the clarification. I don't think it's so urgent to block this anyway, we can open a bug report for this and Michael can contribute a patch :-)

[Carlos Garcia Campos]

Committed r206807: <http://trac.webkit.org/changeset/206807>

[Michael Catanzaro]

> (In reply to comment #3)
> > What's the problem of using SHA1 here? What's the security problem exactly?
> I see certificates and SHA1 and think "Somebody could theoretically create a
> collision and something unexpected could happen and that's really bad with
> certificates."  I'm not sure what the exact attack would look like, but it's
> up to you to decide whether that's important enough.

Yeah, the problem is an attacker could theoretically get a totally different certificate to be accepted. There have been very similar practical attacks published recently that work on all implementations, no need to introduce a WebKit-specific problem too. Bug #162965.

(In reply to comment #6)
> It seems like we should work towards removing this, too.  Very bad things
> can happen when we ignore TLS errors.

Unfortunately it's exposed in our API, in case applications want to shoot themselves in the foot. I'm not aware of any applications that are using it, though.

> > Source/WebKit2/NetworkProcess/soup/NetworkProcessSoup.cpp:91
> > -    ResourceHandle::setClientCertificate(host, certificateInfo.certificate());
> > +    SoupNetworkSession::allowSpecificHTTPSCertificateForHost(certificateInfo, host);
> 
> We are also moving away from allowing specific TLS certificates.  I'm going
> to do this on Cocoa by using SecTrustEvaluateAsync with a few additional
> checks in the NetworkProcess after receiving the server trust evaluation
> challenge.  This will avoid IPC and allow us to quickly and asynchronously
> connect to most HTTPS servers that use modern TLS and valid certificates. 
> In the case where it fails such as on badssl.com we will send IPC to the
> UIProcess which has the option of responding and saying to trust the server
> even though it's probably unsafe.

This is exposed in our API too, but it's only used to implement the functionality you described, so I'm not sure what the problem is?

[Alex Christensen]

Cocoa still uses allowSpecificHTTPSCertificateForHost, so it's not ready for deprecation yet, but in the hopefully-not-too-distant-future we should work towards deprecating those APIs and having them do nothing.  Asynchronously responding to challenges in the UIProcess is the way of the future.

[Michael Catanzaro]

(In reply to comment #12)
> Cocoa still uses allowSpecificHTTPSCertificateForHost, so it's not ready for
> deprecation yet, but in the hopefully-not-too-distant-future we should work
> towards deprecating those APIs and having them do nothing.  Asynchronously
> responding to challenges in the UIProcess is the way of the future.

OK, I understand. Our existing API can actually be implemented that way, so it will be more code for us but not a problem.

[Jeremy Huddleston Sequoia]

Followup to address build failure:

https://bugs.webkit.org/show_bug.cgi?id=163340