[Curl] Disable CURLOPT_SSL_VERIFYPEER and CURLOPT_SSL_VERIFYHOST specified by setAllo...
authorBasuke.Suzuki@sony.com <Basuke.Suzuki@sony.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Wed, 18 Jul 2018 18:35:16 +0000 (18:35 +0000)
committerBasuke.Suzuki@sony.com <Basuke.Suzuki@sony.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Wed, 18 Jul 2018 18:35:16 +0000 (18:35 +0000)
https://bugs.webkit.org/show_bug.cgi?id=187611

Reviewed by Fujii Hironori.

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 any TLS certificate validation.
- But only certificate arrived first is ignored, not any certificates for the host
  (which is rare, but possible for mis configured web cluster).

This behavior is incomplete. To ignore any certificates of the host, these two methods are enough:

- void allowAnyHTTPSCertificatesForHost(const String&)
- bool canIgnoreAnyHTTPSCertificatesForHost(const String&)

No new tests. Covered by existing tests.

* platform/network/curl/CertificateInfo.h:
* platform/network/curl/CurlContext.cpp:
(WebCore::CurlHandle::enableSSLForHost): Ignore TLS verification for registered host.
* platform/network/curl/CurlSSLHandle.cpp:
(WebCore::CurlSSLHandle::allowAnyHTTPSCertificatesForHost): Added.
(WebCore::CurlSSLHandle::canIgnoreAnyHTTPSCertificatesForHost const): Ditto.
(WebCore::CurlSSLHandle::setClientCertificateInfo): Separate lock.
(WebCore::CurlSSLHandle::getSSLClientCertificate const): Ditto and add const.
(WebCore::CurlSSLHandle::setHostAllowsAnyHTTPSCertificate): Deleted.
(WebCore::CurlSSLHandle::isAllowedHTTPSCertificateHost): Deleted.
(WebCore::CurlSSLHandle::canIgnoredHTTPSCertificate): Deleted.
(WebCore::CurlSSLHandle::getSSLClientCertificate): Deleted.
* platform/network/curl/CurlSSLHandle.h:
* platform/network/curl/CurlSSLVerifier.cpp:
(WebCore::CurlSSLVerifier::CurlSSLVerifier):
(WebCore::CurlSSLVerifier::collectInfo): Renamed from verify.
(WebCore::CurlSSLVerifier::verifyCallback):
(WebCore::CurlSSLVerifier::verify): Renamed to collectInfo.
* platform/network/curl/CurlSSLVerifier.h:
* platform/network/curl/ResourceHandleCurl.cpp:
(WebCore::ResourceHandle::setHostAllowsAnyHTTPSCertificate): Rename calling method.

git-svn-id: https://svn.webkit.org/repository/webkit/trunk@233919 268f45cc-cd09-0410-ab3c-d52691b4dbfc

Source/WebCore/ChangeLog
Source/WebCore/platform/network/curl/CertificateInfo.h
Source/WebCore/platform/network/curl/CurlContext.cpp
Source/WebCore/platform/network/curl/CurlSSLHandle.cpp
Source/WebCore/platform/network/curl/CurlSSLHandle.h
Source/WebCore/platform/network/curl/CurlSSLVerifier.cpp
Source/WebCore/platform/network/curl/CurlSSLVerifier.h
Source/WebCore/platform/network/curl/ResourceHandleCurl.cpp

index 72a9f9b..34a16ae 100644 (file)
@@ -1,3 +1,58 @@
+2018-07-18  Basuke Suzuki  <Basuke.Suzuki@sony.com>
+
+        [Curl] Disable CURLOPT_SSL_VERIFYPEER and CURLOPT_SSL_VERIFYHOST specified by setAllowsAnyHTTPSCertificate.
+        https://bugs.webkit.org/show_bug.cgi?id=187611
+
+        Reviewed by Fujii Hironori.
+
+        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 any TLS certificate validation.
+        - But only certificate arrived first is ignored, not any certificates for the host
+          (which is rare, but possible for mis configured web cluster).
+
+        This behavior is incomplete. To ignore any certificates of the host, these two methods are enough:
+
+        - void allowAnyHTTPSCertificatesForHost(const String&)
+        - bool canIgnoreAnyHTTPSCertificatesForHost(const String&)
+
+        No new tests. Covered by existing tests.
+
+        * platform/network/curl/CertificateInfo.h:
+        * platform/network/curl/CurlContext.cpp:
+        (WebCore::CurlHandle::enableSSLForHost): Ignore TLS verification for registered host.
+        * platform/network/curl/CurlSSLHandle.cpp:
+        (WebCore::CurlSSLHandle::allowAnyHTTPSCertificatesForHost): Added.
+        (WebCore::CurlSSLHandle::canIgnoreAnyHTTPSCertificatesForHost const): Ditto.
+        (WebCore::CurlSSLHandle::setClientCertificateInfo): Separate lock.
+        (WebCore::CurlSSLHandle::getSSLClientCertificate const): Ditto and add const.
+        (WebCore::CurlSSLHandle::setHostAllowsAnyHTTPSCertificate): Deleted.
+        (WebCore::CurlSSLHandle::isAllowedHTTPSCertificateHost): Deleted.
+        (WebCore::CurlSSLHandle::canIgnoredHTTPSCertificate): Deleted.
+        (WebCore::CurlSSLHandle::getSSLClientCertificate): Deleted.
+        * platform/network/curl/CurlSSLHandle.h:
+        * platform/network/curl/CurlSSLVerifier.cpp:
+        (WebCore::CurlSSLVerifier::CurlSSLVerifier):
+        (WebCore::CurlSSLVerifier::collectInfo): Renamed from verify.
+        (WebCore::CurlSSLVerifier::verifyCallback):
+        (WebCore::CurlSSLVerifier::verify): Renamed to collectInfo.
+        * platform/network/curl/CurlSSLVerifier.h:
+        * platform/network/curl/ResourceHandleCurl.cpp:
+        (WebCore::ResourceHandle::setHostAllowsAnyHTTPSCertificate): Rename calling method.
+
 2018-07-18  Myles C. Maxfield  <mmaxfield@apple.com>
 
         Rename WordBreak::Break to WordBreak::BreakWord
index 4705f3b..faf1aba 100644 (file)
@@ -32,7 +32,7 @@ namespace WebCore {
 
 class CertificateInfo {
 public:
-    using Certificate = Vector<char>;
+    using Certificate = Vector<uint8_t>;
 
     CertificateInfo() = default;
     WEBCORE_EXPORT CertificateInfo(int verificationError, Vector<Certificate>&&);
index 95aaa89..d0a06ba 100644 (file)
@@ -301,7 +301,7 @@ void CurlHandle::enableSSLForHost(const String& host)
         setSslKeyPassword(sslClientCertificate->second.utf8().data());
     }
 
-    if (sslHandle.shouldIgnoreSSLErrors()) {
+    if (sslHandle.canIgnoreAnyHTTPSCertificatesForHost(host) || sslHandle.shouldIgnoreSSLErrors()) {
         setSslVerifyPeer(CurlHandle::VerifyPeer::Disable);
         setSslVerifyHost(CurlHandle::VerifyHost::LooseNameCheck);
     } else {
index 8574bb0..cea249a 100644 (file)
@@ -92,48 +92,30 @@ void CurlSSLHandle::clearCACertInfo()
     m_caCertInfo = WTF::Monostate { };
 }
 
-void CurlSSLHandle::setHostAllowsAnyHTTPSCertificate(const String& hostName)
+void CurlSSLHandle::allowAnyHTTPSCertificatesForHost(const String& host)
 {
-    LockHolder mutex(m_mutex);
+    LockHolder mutex(m_allowedHostsLock);
 
-    m_allowedHosts.set(hostName, Vector<CertificateInfo::Certificate> { });
+    m_allowedHosts.addVoid(host);
 }
 
-bool CurlSSLHandle::isAllowedHTTPSCertificateHost(const String& hostName)
+bool CurlSSLHandle::canIgnoreAnyHTTPSCertificatesForHost(const String& host) const
 {
-    LockHolder mutex(m_mutex);
+    LockHolder mutex(m_allowedHostsLock);
 
-    auto it = m_allowedHosts.find(hostName);
-    return (it != m_allowedHosts.end());
-}
-
-bool CurlSSLHandle::canIgnoredHTTPSCertificate(const String& hostName, const Vector<CertificateInfo::Certificate>& certificates)
-{
-    LockHolder mutex(m_mutex);
-
-    auto found = m_allowedHosts.find(hostName);
-    if (found == m_allowedHosts.end())
-        return false;
-
-    auto& value = found->value;
-    if (value.isEmpty()) {
-        value = certificates;
-        return true;
-    }
-
-    return std::equal(certificates.begin(), certificates.end(), value.begin());
+    return m_allowedHosts.contains(host);
 }
 
 void CurlSSLHandle::setClientCertificateInfo(const String& hostName, const String& certificate, const String& key)
 {
-    LockHolder mutex(m_mutex);
+    LockHolder mutex(m_allowedClientHostsLock);
 
     m_allowedClientHosts.set(hostName, ClientCertificate { certificate, key });
 }
 
-std::optional<CurlSSLHandle::ClientCertificate> CurlSSLHandle::getSSLClientCertificate(const String& hostName)
+std::optional<CurlSSLHandle::ClientCertificate> CurlSSLHandle::getSSLClientCertificate(const String& hostName) const
 {
-    LockHolder mutex(m_mutex);
+    LockHolder mutex(m_allowedClientHostsLock);
 
     auto it = m_allowedClientHosts.find(hostName);
     if (it == m_allowedClientHosts.end())
index f515de4..d195d80 100644 (file)
@@ -29,8 +29,8 @@
 #include "CertificateInfo.h"
 #include <openssl/crypto.h>
 #include <wtf/HashMap.h>
+#include <wtf/HashSet.h>
 #include <wtf/NeverDestroyed.h>
-#include <wtf/Noncopyable.h>
 #include <wtf/Variant.h>
 #include <wtf/text/StringHash.h>
 
@@ -69,12 +69,11 @@ public:
     WEBCORE_EXPORT void setCACertData(CertificateInfo::Certificate&&);
     WEBCORE_EXPORT void clearCACertInfo();
 
-    WEBCORE_EXPORT void setHostAllowsAnyHTTPSCertificate(const String&);
-    bool isAllowedHTTPSCertificateHost(const String&);
-    bool canIgnoredHTTPSCertificate(const String&, const Vector<CertificateInfo::Certificate>&);
+    WEBCORE_EXPORT void allowAnyHTTPSCertificatesForHost(const String& host);
+    bool canIgnoreAnyHTTPSCertificatesForHost(const String&) const;
 
     WEBCORE_EXPORT void setClientCertificateInfo(const String&, const String&, const String&);
-    std::optional<ClientCertificate> getSSLClientCertificate(const String&);
+    std::optional<ClientCertificate> getSSLClientCertificate(const String&) const;
 
 private:
 #if NEED_OPENSSL_THREAD_SUPPORT
@@ -113,8 +112,9 @@ private:
 
     bool m_ignoreSSLErrors { false };
 
-    Lock m_mutex;
-    HashMap<String, Vector<CertificateInfo::Certificate>, ASCIICaseInsensitiveHash> m_allowedHosts;
+    mutable Lock m_allowedHostsLock;
+    mutable Lock m_allowedClientHostsLock;
+    HashSet<String, ASCIICaseInsensitiveHash> m_allowedHosts;
     HashMap<String, ClientCertificate, ASCIICaseInsensitiveHash> m_allowedClientHosts;
 };
 
index 246cd17..922d6ff 100644 (file)
@@ -47,8 +47,8 @@ CurlSSLVerifier::CurlSSLVerifier(CurlHandle& curlHandle, void* sslCtx)
     SSL_CTX_set_verify(ctx, SSL_CTX_get_verify_mode(ctx), verifyCallback);
 
 #if defined(LIBRESSL_VERSION_NUMBER)
-    if (auto* data = WTF::get_if<Vector<char>>(sslHandle.getCACertInfo()))
-        SSL_CTX_load_verify_mem(ctx, static_cast<void*>(const_cast<char*>(data->data())), data->size());
+    if (auto data = WTF::get_if<CertificateInfo::Certificate>(sslHandle.getCACertInfo()))
+        SSL_CTX_load_verify_mem(ctx, static_cast<void*>(const_cast<uint8_t*>(data->data())), data->size());
 #endif
 
 #if (!defined(LIBRESSL_VERSION_NUMBER))
@@ -62,40 +62,23 @@ CurlSSLVerifier::CurlSSLVerifier(CurlHandle& curlHandle, void* sslCtx)
         SSL_CTX_set1_curves_list(ctx, curvesList.utf8().data());
 }
 
-bool CurlSSLVerifier::verify(X509StoreCTX* ctx)
+void CurlSSLVerifier::collectInfo(X509StoreCTX* ctx)
 {
-    // whether the verification of the certificate in question was passed (preverify_ok=1) or not (preverify_ok=0)
     m_certificateInfo = CertificateInfo { X509_STORE_CTX_get_error(ctx), pemDataFromCtx(ctx) };
 
-    auto error = m_certificateInfo.verificationError();
-    if (!error)
-        return 1;
-
-    m_sslErrors = static_cast<int>(convertToSSLCertificateFlags(error));
-
-#if PLATFORM(WIN)
-    bool ok = CurlContext::singleton().sslHandle().isAllowedHTTPSCertificateHost(m_curlHandle.url().host().toString());
-#else
-    bool ok = CurlContext::singleton().sslHandle().canIgnoredHTTPSCertificate(m_curlHandle.url().host().toString(), m_certificateInfo.certificateChain());
-#endif
-
-    if (ok) {
-        // if the host and the certificate are stored for the current handle that means is enabled,
-        // so don't need to curl verifies the authenticity of the peer's certificate
-        m_curlHandle.setSslVerifyPeer(CurlHandle::VerifyPeer::Disable);
-    }
-
-    return ok;
+    if (auto error = m_certificateInfo.verificationError())
+        m_sslErrors = static_cast<int>(convertToSSLCertificateFlags(error));
 }
 
-int CurlSSLVerifier::verifyCallback(int ok, X509StoreCTX* ctx)
+int CurlSSLVerifier::verifyCallback(int preverified, X509StoreCTX* ctx)
 {
     auto ssl = static_cast<SSL*>(X509_STORE_CTX_get_ex_data(ctx, SSL_get_ex_data_X509_STORE_CTX_idx()));
     auto sslCtx = SSL_get_SSL_CTX(ssl);
     auto verifier = static_cast<CurlSSLVerifier*>(SSL_CTX_get_app_data(sslCtx));
 
-    // whether the verification of the certificate in question was passed (preverify_ok=1) or not (preverify_ok=0)
-    return verifier->verify(ctx);
+    verifier->collectInfo(ctx);
+    // whether the verification of the certificate in question was passed (preverified=1) or not (preverified=0)
+    return preverified;
 }
 
 class StackOfX509 {
index d51e5fd..49b6b9d 100644 (file)
@@ -63,7 +63,7 @@ private:
     int m_sslErrors { 0 };
     CertificateInfo m_certificateInfo;
 
-    bool verify(X509StoreCTX*);
+    void collectInfo(X509StoreCTX*);
 };
 
 } // namespace WebCore
index 7cde247..6384d62 100644 (file)
@@ -169,7 +169,7 @@ void ResourceHandle::setHostAllowsAnyHTTPSCertificate(const String& host)
 {
     ASSERT(isMainThread());
 
-    CurlContext::singleton().sslHandle().setHostAllowsAnyHTTPSCertificate(host);
+    CurlContext::singleton().sslHandle().allowAnyHTTPSCertificatesForHost(host);
 }
 
 void ResourceHandle::setClientCertificateInfo(const String& host, const String& certificate, const String& key)