Bug 187611

Summary: [Curl] Disable CURLOPT_SSL_VERIFYPEER and CURLOPT_SSL_VERIFYHOST specified by setAllowsAnyHTTPSCertificate.
Product: WebKit Reporter: Basuke Suzuki <basuke>
Component: PlatformAssignee: Basuke Suzuki <basuke>
Status: RESOLVED FIXED    
Severity: Normal CC: achristensen, basuke, bfulgham, commit-queue, ews-watchlist, fujii.hironori, galpeter, webkit-bug-importer, youennf
Priority: P2 Keywords: InRadar
Version: WebKit Nightly Build   
Hardware: Unspecified   
OS: Unspecified   
Bug Depends on:    
Bug Blocks: 187679    
Attachments:
Description Flags
WIP Patch
fujii.hironori: review-, ews-watchlist: commit-queue-
Archive of layout-test-results from ews206 for win-future
none
PATCH
fujii.hironori: review-
FIX
fujii.hironori: review-
FIX
none
Archive of layout-test-results from ews200 for win-future none

Basuke Suzuki
Reported 2018-07-12 11:57:20 PDT
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.
Attachments
WIP Patch (11.19 KB, patch)
2018-07-12 14:05 PDT, Basuke Suzuki 		fujii.hironori: review-
ews-watchlist: commit-queue-
DetailsFormatted DiffDiff
Archive of layout-test-results from ews206 for win-future (12.78 MB, application/zip)
2018-07-12 18:27 PDT, EWS Watchlist 		no flags
Details
PATCH (15.05 KB, patch)
2018-07-13 14:27 PDT, Basuke Suzuki 		fujii.hironori: review-
DetailsFormatted DiffDiff
FIX (13.03 KB, patch)
2018-07-14 12:07 PDT, Basuke Suzuki 		fujii.hironori: review-
DetailsFormatted DiffDiff
FIX (12.88 KB, patch)
2018-07-17 13:56 PDT, Basuke Suzuki 		no flags
DetailsFormatted DiffDiff
Archive of layout-test-results from ews200 for win-future (12.88 MB, application/zip)
2018-07-17 15:01 PDT, EWS Watchlist 		no flags
Details
Show Obsolete (4) View All Add attachment proposed patch, testcase, etc.
Basuke Suzuki
Comment 1 2018-07-12 14:05:06 PDT
Created attachment 344881 [details] WIP Patch
EWS Watchlist
Comment 2 2018-07-12 14:07:51 PDT
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
Comment 3 2018-07-12 14:58:12 PDT
I wanna confirm whether the direction is right or wrong.
EWS Watchlist
Comment 4 2018-07-12 18:27:32 PDT
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
Comment 5 2018-07-12 18:27:45 PDT
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
Comment 6 2018-07-13 00:53:21 PDT
(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 7 2018-07-13 02:37:24 PDT
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
Comment 8 2018-07-13 10:36:49 PDT
(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 9 2018-07-13 12:18:49 PDT
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 10 2018-07-13 14:18:37 PDT
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
Comment 11 2018-07-13 14:27:52 PDT
Created attachment 344981 [details] PATCH
Fujii Hironori
Comment 12 2018-07-13 17:54:20 PDT
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 13 2018-07-14 11:22:10 PDT
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
Comment 14 2018-07-14 12:07:19 PDT
Created attachment 345038 [details] FIX Remove extended part. Only patches for fixing the current implementation.
Fujii Hironori
Comment 15 2018-07-16 21:12:54 PDT
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 16 2018-07-17 13:51:07 PDT
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
Comment 17 2018-07-17 13:56:47 PDT
Created attachment 345189 [details] FIX
EWS Watchlist
Comment 18 2018-07-17 15:00:59 PDT
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
Comment 19 2018-07-17 15:01:11 PDT
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 20 2018-07-17 18:22:18 PDT
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 21 2018-07-18 11:35:24 PDT
Comment on attachment 345189 [details] FIX Clearing flags on attachment: 345189 Committed r233919: <https://trac.webkit.org/changeset/233919>
WebKit Commit Bot
Comment 22 2018-07-18 11:35:26 PDT
All reviewed patches have been landed. Closing bug.
Radar WebKit Bug Importer
Comment 23 2018-07-18 11:38:03 PDT
<rdar://problem/42344372>
Note You need to log in before you can comment on or make changes to this bug.