Bug 187611 – [Curl] Disable CURLOPT_SSL_VERIFYPEER and CURLOPT_SSL_VERIFYHOST specified by setAllowsAnyHTTPSCertificate.

• Status: RESOLVED
• Resolution: FIXED
• Component: Platform

[Basuke Suzuki]

Current interface for TLS certificate validation for Curl port are as follows:

    WEBCORE_EXPORT void setHostAllowsAnyHTTPSCertificate(const String&);
    bool isAllowedHTTPSCertificateHost(const String&);
    bool canIgnoredHTTPSCertificate(const String&, const Vector<CertificateInfo::Certificate>&);

First one registers a host to be ignored for any certificate check. Once it is registered, no further certificate validation check is executed.
Second one checks the host is registered in the list above.
Third one is weird. The method signature implies it checks the certificate for the host and detect whether we can ignore this certificate for the host, but actually it  just check only the host and register the certificate into the vector. Then in the next request for the host, the certificate will be checked with the previously stored certificate.

It's hard to understand, but in short, 
- We can register a host as an exception for TLS certificate validation.
- But only certificate arrived first is ignored, not all certificates for the host (which is rare, but possible for mis configured web cluster).

This behavior is incomplete and more over it is very different from other ports.

This should be changed following other ports:
- allowSpecificHTTPSCertificateForHost(const CertificateInfo& certificateInfo, const String& host)
- canIgnoreSpecificHTTPSCertificateForHost(const CertificateInfo& certificateInfo, const String& host)
    (actually check its certificates against registered certificates for the host)

This will be used for standard SSL error interface such as: "Do you allow this certificates for host?" checkbox in the SSL error dialog.

Also for the original purpose to ignore ALL certificates of the host:
- allowAllHTTPSCertificatesForHost(const String& host)
- canIgnoreAllHTTPSCertificatesForHost(const String& host)

This is required to access to the host which has different SNI name in the certificates in our platform.

[Basuke Suzuki]

Created attachment 344881 [details]
WIP Patch

[EWS Watchlist]

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


ERROR: Source/WebCore/platform/network/curl/CurlSSLVerifier.cpp:92:  preverify_ok is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
Total errors found: 1 in 7 files


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

[Basuke Suzuki]

I wanna confirm whether the direction is right or wrong.

[EWS Watchlist]

Comment on attachment 344881 [details]
WIP Patch

Attachment 344881 [details] did not pass win-ews (win):
Output: https://webkit-queues.webkit.org/results/8521535

New failing tests:
http/tests/security/canvas-remote-read-remote-video-localhost.html
http/tests/preload/onload_event.html

[EWS Watchlist]

Created attachment 344912 [details]
Archive of layout-test-results from ews206 for win-future

The attached test failures were seen while running run-webkit-tests on the win-ews.
Bot: ews206  Port: win-future  Platform: CYGWIN_NT-6.1-2.9.0-0.318-5-3-x86_64-64bit

[Fujii Hironori]

(In reply to Basuke Suzuki from comment #3)
> I wanna confirm whether the direction is right or wrong.

This is a right direction. Curl port also should implement
NetworkProcess::allowSpecificHTTPSCertificateForHost of
NetworkProcess/curl/NetworkProcessCurl.cpp.

(In reply to Basuke Suzuki from comment #0)
> This behavior is incomplete and more over it is very different from other
> ports.

In such case, please summarize the implementations of other ports to reduce reviewer's tasks.
It took some time for me to check other ports.

> Also for the original purpose to ignore ALL certificates of the host:
> - allowAllHTTPSCertificatesForHost(const String& host)
> - canIgnoreAllHTTPSCertificatesForHost(const String& host)

You plan to rename, but 'all' sounds weird to me.
Mac port is using 'any' like 'setHostAllowsAnyHTTPSCertificate' or 'allowsAnyHTTPSCertificateHosts'.

BTW, CurlSSLHandle is not a handle, but a context. This is a confusing naming.

[Fujii Hironori]

Comment on attachment 344881 [details]
WIP Patch

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

> Source/WebCore/platform/network/curl/CurlSSLHandle.cpp:95
> +void CurlSSLHandle::allowSpecificHTTPSCertificateForHost(CertificateInfo::CertificateChain&& certificates, String&& host)

I don't think you should pass the lifetime of 'host' by calling this method. 'host' should be a lvalue reference.

> Source/WebCore/platform/network/curl/CurlSSLHandle.cpp:99
> +    auto found = m_allowedCertificatesForHost.find(host);

This is an iterator. found → iterator or iter

> Source/WebCore/platform/network/curl/CurlSSLHandle.cpp:101
> +        m_allowedCertificatesForHost.set(WTFMove(host), Vector<CertificateInfo::CertificateChain> { WTFMove(certificates) });

Do you need to specify the type "Vector<CertificateInfo::CertificateChain>"  in this case?
m_allowedCertificatesForHost.set(WTFMove(host), { WTFMove(certificates) });

> Source/WebCore/platform/network/curl/CurlSSLHandle.cpp:110
> +    auto found = m_allowedCertificatesForHost.find(host);

This is an iterator. found → iterator or iter

> Source/WebCore/platform/network/curl/CurlSSLHandle.cpp:115
> +    return allowedCertificates.contains(certificates);

return found->contains(certificates);

> Source/WebCore/platform/network/curl/CurlSSLVerifier.cpp:51
> +    if (auto data = WTF::get_if<CertificateInfo::Certificate>(sslHandle.getCACertInfo()))

'auto data' should be "auto& data" to avoid copy.

> Source/WebCore/platform/network/curl/CurlSSLVerifier.cpp:71
> +    if (auto error = m_certificateInfo.verificationError())

Do you need to define 'error' twice?
if (error)

> Source/WebCore/platform/network/curl/CurlSSLVerifier.cpp:78
> +    if (!m_certificateInfo.verificationError())

Why don't use 'error' you defined above?
if (!error)
This change is start to check 'preverify_ok'. Do you really need this check?

> Source/WebCore/platform/network/curl/CurlSSLVerifier.cpp:85
>              m_curlHandle->setSslVerifyPeer(CurlHandle::VerifyPeer::Disable);

Just out of curiosity, Why is this code needed?
When is m_curlHandle null?

> Source/WebCore/platform/network/curl/ResourceHandleCurl.cpp:173
> +    CurlContext::singleton().sslHandle().allowAnyHTTPSCertificatesForHost(WTFMove(hostCopy));

This code doesn't look good.
In this case, allowAnyHTTPSCertificatesForHost should take a value instead of a rvalue reference.
allowAnyHTTPSCertificatesForHost(String);
Then, only one copy is created by calling the method.

[Basuke Suzuki]

(In reply to Fujii Hironori from comment #6)
> (In reply to Basuke Suzuki from comment #3)
> > I wanna confirm whether the direction is right or wrong.
> 
> This is a right direction. Curl port also should implement
> NetworkProcess::allowSpecificHTTPSCertificateForHost of
> NetworkProcess/curl/NetworkProcessCurl.cpp.
> 
> (In reply to Basuke Suzuki from comment #0)
> > This behavior is incomplete and more over it is very different from other
> > ports.
> 
> In such case, please summarize the implementations of other ports to reduce
> reviewer's tasks.
> It took some time for me to check other ports.

Got it.


> > Also for the original purpose to ignore ALL certificates of the host:
> > - allowAllHTTPSCertificatesForHost(const String& host)
> > - canIgnoreAllHTTPSCertificatesForHost(const String& host)
> 
> You plan to rename, but 'all' sounds weird to me.
> Mac port is using 'any' like 'setHostAllowsAnyHTTPSCertificate' or
> 'allowsAnyHTTPSCertificateHosts'.

Right. The sent patch uses `any` instead of `all`. That's my mistake while writing that comment. Sorry for confusion.


> BTW, CurlSSLHandle is not a handle, but a context. This is a confusing
> naming.

I agree with that. It originally came from old curl/SSLHandle. I put the renaming this name to my to-do list.

[Basuke Suzuki]

Comment on attachment 344881 [details]
WIP Patch

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

>> Source/WebCore/platform/network/curl/CurlSSLVerifier.cpp:78
>> +    if (!m_certificateInfo.verificationError())
> 
> Why don't use 'error' you defined above?
> if (!error)
> This change is start to check 'preverify_ok'. Do you really need this check?

Right. I couldn't be confident about it, but after checking the source code of OpenSSL, we can assume if pre-verified is false, error is non-zero. (The opposite case can be possible, pre-verified but error.) We can delete this check.

>> Source/WebCore/platform/network/curl/ResourceHandleCurl.cpp:173
>> +    CurlContext::singleton().sslHandle().allowAnyHTTPSCertificatesForHost(WTFMove(hostCopy));
> 
> This code doesn't look good.
> In this case, allowAnyHTTPSCertificatesForHost should take a value instead of a rvalue reference.
> allowAnyHTTPSCertificatesForHost(String);
> Then, only one copy is created by calling the method.

If so, I prefer leaving this const reference and such copy is handled by caller.

[Basuke Suzuki]

Comment on attachment 344881 [details]
WIP Patch

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

>> Source/WebCore/platform/network/curl/CurlSSLHandle.cpp:101
>> +        m_allowedCertificatesForHost.set(WTFMove(host), Vector<CertificateInfo::CertificateChain> { WTFMove(certificates) });
> 
> Do you need to specify the type "Vector<CertificateInfo::CertificateChain>"  in this case?
> m_allowedCertificatesForHost.set(WTFMove(host), { WTFMove(certificates) });

It seems MSVC doesn't allow omitting type.

>> Source/WebCore/platform/network/curl/CurlSSLHandle.cpp:115
>> +    return allowedCertificates.contains(certificates);
> 
> return found->contains(certificates);

Do you mean `found->value.contains(certificates)` ?

>> Source/WebCore/platform/network/curl/CurlSSLVerifier.cpp:51
>> +    if (auto data = WTF::get_if<CertificateInfo::Certificate>(sslHandle.getCACertInfo()))
> 
> 'auto data' should be "auto& data" to avoid copy.

No, data is pointer. There's no copy overhead.

[Basuke Suzuki]

Created attachment 344981 [details]
PATCH

[Fujii Hironori]

Comment on attachment 344981 [details]
PATCH

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

> Source/WebCore/ChangeLog:3
> +        [Curl] Fix implementation error in handling Certificate exceptions.

I like a more informative subject.
But, it is difficult because you are doing two thinkgs in a single patch.
1. Add allowSpecificHTTPSCertificateForHost
2. Change how isAllowedHTTPSCertificateHost is used
Please split this patch into two.

> Source/WebCore/platform/network/curl/CurlContext.cpp:304
> +    if (sslHandle.canIgnoreAnyHTTPSCertificatesForHost(host) || sslHandle.shouldIgnoreSSLErrors()) {

What will happen in case of a redirection from http to https of different site?
This CurlHandle is reused in such case?

> Source/WebCore/platform/network/curl/CurlSSLHandle.cpp:95
> +void CurlSSLHandle::allowSpecificHTTPSCertificateForHost(CertificateInfo::CertificateChain&& certificates, const String& host)

You add CurlSSLHandle::allowSpecificHTTPSCertificateForHost, but not used anywhere in this change.

Why don't you implement NetworkProcess::allowSpecificHTTPSCertificateForHost?

> Source/WebCore/platform/network/curl/CurlSSLVerifier.cpp:82
> +    // whether the verification of the certificate in question was passed (preverified=1) or not (preverified=0)

This comment should be moved to the below checking.

> Source/WebCore/platform/network/curl/CurlSSLVerifier.cpp:88
> +    return preverified || verifier->verify();

if (preverified)
    // Put a good comment here.
    return 1;
return verifier->verify();

I think this looks better.

[Basuke Suzuki]

Comment on attachment 344981 [details]
PATCH

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

>> Source/WebCore/ChangeLog:3
>> +        [Curl] Fix implementation error in handling Certificate exceptions.
> 
> I like a more informative subject.
> But, it is difficult because you are doing two thinkgs in a single patch.
> 1. Add allowSpecificHTTPSCertificateForHost
> 2. Change how isAllowedHTTPSCertificateHost is used
> Please split this patch into two.

Got it. Without fixing the original bug, we cannot adding a new feature, allowing specific certificates. So I will fix the bug first. Then adding that feature with the implementation of WebKit Network layer.

>> Source/WebCore/platform/network/curl/CurlContext.cpp:304
>> +    if (sslHandle.canIgnoreAnyHTTPSCertificatesForHost(host) || sslHandle.shouldIgnoreSSLErrors()) {
> 
> What will happen in case of a redirection from http to https of different site?
> This CurlHandle is reused in such case?

No, any redirection wont reuse the handle. It used to be redirected internally, but not any more since this patch. https://bugs.webkit.org/show_bug.cgi?id=21242

>> Source/WebCore/platform/network/curl/CurlSSLHandle.cpp:95
>> +void CurlSSLHandle::allowSpecificHTTPSCertificateForHost(CertificateInfo::CertificateChain&& certificates, const String& host)
> 
> You add CurlSSLHandle::allowSpecificHTTPSCertificateForHost, but not used anywhere in this change.
> 
> Why don't you implement NetworkProcess::allowSpecificHTTPSCertificateForHost?

See you in the next patch.

>> Source/WebCore/platform/network/curl/CurlSSLVerifier.cpp:88
>> +    return preverified || verifier->verify();
> 
> if (preverified)
>     // Put a good comment here.
>     return 1;
> return verifier->verify();
> 
> I think this looks better.

For fixing the bug, extra verification is not required. I will just return `preverified` after collecting the certificates.

[Basuke Suzuki]

Created attachment 345038 [details]
FIX

Remove extended part. Only patches for fixing the current implementation.

[Fujii Hironori]

Comment on attachment 345038 [details]
FIX

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

> Source/WebCore/ChangeLog:3
> +        [Curl] Fix implementation error in handling Certificate exceptions.

In my understanding, this patch doesn't solve any problem.

isAllowedHTTPSCertificateHost is used if PLATFORM(WIN),
canIgnoredHTTPSCertificate is used otherwise.
But, Curl support is used only in WinCairo port in upstream WebKit.
This patch changes two things:
1. How isAllowedHTTPSCertificateHost is used.
2. Remove canIgnoredHTTPSCertificate.
#2 is a trivial change because this is an unused function in upstream ports.

I'd like to propose a following summaries:

[Curl] Disable CURLOPT_SSL_VERIFYPEER and CURLOPT_SSL_VERIFYHOST for hosts specified by setAllowsAnyHTTPSCertificate

or

[Curl] Change the implementation of setAllowsAnyHTTPSCertificate by disabling CURLOPT_SSL_VERIFYPEER and CURLOPT_SSL_VERIFYHOST

> Source/WebCore/ChangeLog:32
> +        No new tests. Covered by existing tests.

AFAIK, there is no test cases of ResourceHandle::setHostAllowsAnyHTTPSCertificate.
It's impossible to test unused a function even by manual testing.
It should be testable by using Windows MiniBrowser because Windows WK1 has an API WebMutableURLRequest::setAllowsAnyHTTPSCertificate.

> Source/WebCore/ChangeLog:34
> +        * platform/LocalizedStrings.cpp:

This patch doesn't change LocalizedStrings.cpp. Remove this.

> Source/WebCore/platform/network/curl/CertificateInfo.h:36
> +    using CertificateChain = Vector<Certificate>;

Remove "using CertificateChain = Vector<Certificate>;". This is not used yet.

[Basuke Suzuki]

Comment on attachment 345038 [details]
FIX

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

>> Source/WebCore/ChangeLog:3
>> +        [Curl] Fix implementation error in handling Certificate exceptions.
> 
> In my understanding, this patch doesn't solve any problem.
> 
> isAllowedHTTPSCertificateHost is used if PLATFORM(WIN),
> canIgnoredHTTPSCertificate is used otherwise.
> But, Curl support is used only in WinCairo port in upstream WebKit.
> This patch changes two things:
> 1. How isAllowedHTTPSCertificateHost is used.
> 2. Remove canIgnoredHTTPSCertificate.
> #2 is a trivial change because this is an unused function in upstream ports.
> 
> I'd like to propose a following summaries:
> 
> [Curl] Disable CURLOPT_SSL_VERIFYPEER and CURLOPT_SSL_VERIFYHOST for hosts specified by setAllowsAnyHTTPSCertificate
> 
> or
> 
> [Curl] Change the implementation of setAllowsAnyHTTPSCertificate by disabling CURLOPT_SSL_VERIFYPEER and CURLOPT_SSL_VERIFYHOST

Oh, good point. Sorry I was confusing the platform we develop and WinCairo. You're right. But still fixes a bug. The old implementation couldn't handle mis-configured web cluster which has different certificates as described below. So original title still works. So first proposed one is better description, I think.

>> Source/WebCore/ChangeLog:32
>> +        No new tests. Covered by existing tests.
> 
> AFAIK, there is no test cases of ResourceHandle::setHostAllowsAnyHTTPSCertificate.
> It's impossible to test unused a function even by manual testing.
> It should be testable by using Windows MiniBrowser because Windows WK1 has an API WebMutableURLRequest::setAllowsAnyHTTPSCertificate.

It is used in secure WebSocket client tests and actually it sets this flag.

>> Source/WebCore/ChangeLog:34
>> +        * platform/LocalizedStrings.cpp:
> 
> This patch doesn't change LocalizedStrings.cpp. Remove this.

Ouch. Sorry!

>> Source/WebCore/platform/network/curl/CertificateInfo.h:36
>> +    using CertificateChain = Vector<Certificate>;
> 
> Remove "using CertificateChain = Vector<Certificate>;". This is not used yet.

Right.

[Basuke Suzuki]

Created attachment 345189 [details]
FIX

[EWS Watchlist]

Comment on attachment 345189 [details]
FIX

Attachment 345189 [details] did not pass win-ews (win):
Output: https://webkit-queues.webkit.org/results/8567023

New failing tests:
http/tests/security/contentSecurityPolicy/userAgentShadowDOM/allow-video.html

[EWS Watchlist]

Created attachment 345195 [details]
Archive of layout-test-results from ews200 for win-future

The attached test failures were seen while running run-webkit-tests on the win-ews.
Bot: ews200  Port: win-future  Platform: CYGWIN_NT-6.1-2.9.0-0.318-5-3-x86_64-64bit

[Fujii Hironori]

Comment on attachment 345038 [details]
FIX

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

>>> Source/WebCore/ChangeLog:32
>>> +        No new tests. Covered by existing tests.
>> 
>> AFAIK, there is no test cases of ResourceHandle::setHostAllowsAnyHTTPSCertificate.
>> It's impossible to test unused a function even by manual testing.
>> It should be testable by using Windows MiniBrowser because Windows WK1 has an API WebMutableURLRequest::setAllowsAnyHTTPSCertificate.
> 
> It is used in secure WebSocket client tests and actually it sets this flag.

Oh, you are right. I didn't notice.
https://github.com/WebKit/webkit/blob/8764594d963414856d1f9b948e7cc46e9b5a82ff/Tools/DumpRenderTree/win/DumpRenderTree.cpp#L1235

[WebKit Commit Bot]

Comment on attachment 345189 [details]
FIX

Clearing flags on attachment: 345189

Committed r233919: <https://trac.webkit.org/changeset/233919>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Radar WebKit Bug Importer]

<rdar://problem/42344372>