fix TLS implementation quarks (#398)

marty1885 · web-flow · commit efd0beb8a457 · 2026-04-08T16:34:52.000+08:00
diff --git a/trantor/net/inner/Socket.cc b/trantor/net/inner/Socket.cc
@@ -108,6 +108,7 @@ void Socket::closeWrite()
 #endif
     {
         LOG_SYSERR << "sockets::shutdownWrite";
+        abort();
     }
 }
 int Socket::read(char *buffer, uint64_t len)
diff --git a/trantor/net/inner/tlsprovider/BotanTLSProvider.cc b/trantor/net/inner/tlsprovider/BotanTLSProvider.cc
@@ -474,8 +474,6 @@ SSLContextPtr trantor::newSSLContext(const TLSPolicy &policy, bool server)
             ctx->certStore =
                 std::make_shared<Botan::Flatfile_Certificate_Store>(
                     policy.getCaPath());
-            if (server)
-                ctx->requireClientCert = true;
         }
         else if (policy.getUseSystemCertStore())
         {
@@ -484,6 +482,8 @@ SSLContextPtr trantor::newSSLContext(const TLSPolicy &policy, bool server)
             ctx->certStore = systemCertStore;
         }
     }
+    if (server && policy.getValidate() && !policy.getCaPath().empty())
+        ctx->requireClientCert = true;
 
     if (policy.getUseOldTLS())
         LOG_WARN << "SSLPloicy have set useOldTLS to true. BUt Botan does not "
diff --git a/trantor/net/inner/tlsprovider/OpenSSLProvider.cc b/trantor/net/inner/tlsprovider/OpenSSLProvider.cc
@@ -8,12 +8,10 @@
 #include <openssl/bio.h>
 #include <openssl/x509v3.h>
 
-#include <fstream>
 #include <memory>
 #include <mutex>
 #include <list>
 #include <unordered_map>
-#include <array>
 #include <limits>
 #include "callbacks.h"
 
@@ -70,62 +68,6 @@ inline bool loadWindowsSystemCert(X509_STORE *store)
 }
 #endif
 
-inline bool verifyCommonName(X509 *cert, const std::string &hostname)
-{
-    X509_NAME *subjectName = X509_get_subject_name(cert);
-
-    if (subjectName != nullptr)
-    {
-        std::array<char, BUFSIZ> name;
-        auto length = X509_NAME_get_text_by_NID(subjectName,
-                                                NID_commonName,
-                                                name.data(),
-                                                (int)name.size());
-        if (length == -1)
-            return false;
-
-        return utils::verifySslName(std::string(name.begin(),
-                                                name.begin() + length),
-                                    hostname);
-    }
-
-    return false;
-}
-
-inline bool verifyAltName(X509 *cert, const std::string &hostname)
-{
-    bool good = false;
-    auto altNames = static_cast<const struct stack_st_GENERAL_NAME *>(
-        X509_get_ext_d2i(cert, NID_subject_alt_name, nullptr, nullptr));
-
-    if (altNames)
-    {
-        int numNames = sk_GENERAL_NAME_num(altNames);
-
-        for (int i = 0; i < numNames && !good; i++)
-        {
-            auto val = sk_GENERAL_NAME_value(altNames, i);
-            if (val->type != GEN_DNS)
-            {
-                LOG_WARN << "Name using IP addresses are not supported. Open "
-                            "an issue if you need that feature";
-                continue;
-            }
-#if (OPENSSL_VERSION_NUMBER >= 0x10100000L)
-            auto name = (const char *)ASN1_STRING_get0_data(val->d.ia5);
-#else
-            auto name = (const char *)ASN1_STRING_data(val->d.ia5);
-#endif
-            auto name_len = (size_t)ASN1_STRING_length(val->d.ia5);
-            good = utils::verifySslName(std::string(name, name + name_len),
-                                        hostname);
-        }
-    }
-
-    GENERAL_NAMES_free((STACK_OF(GENERAL_NAME) *)altNames);
-    return good;
-}
-
 static bool validatePeerCertificate(SSL *ssl,
                                     X509 *cert,
                                     const std::string &hostname,
@@ -136,12 +78,16 @@ static bool validatePeerCertificate(SSL *ssl,
     assert(cert != nullptr);
     LOG_TRACE << "Validating peer certificate";
 
-    if (isServer)
+    if (!isServer)
     {
-        bool domainIsValid =
-            verifyCommonName(cert, hostname) || verifyAltName(cert, hostname);
-        if (!domainIsValid)
+        const int rc =
+            X509_check_host(cert, hostname.data(), hostname.size(), 0, nullptr);
+        if (rc != 1)
+        {
+            LOG_TRACE << "Peer certificate does not match hostname: "
+                      << hostname;
             return false;
+        }
     }
 
     auto result = SSL_get_verify_result(ssl);
@@ -423,16 +369,20 @@ class SessionManager
 #endif
     }
 
+    // Returns a session with an additional reference held by the caller.
+    // Caller must SSL_SESSION_free() when done. Required because the entry
+    // in sessionMap_ may be evicted/replaced/expired by another thread the
+    // moment we release the mutex, so the SessionManager's reference is not
+    // a stable ownership root for the returned pointer.
     SSL_SESSION *get(const std::string &hostname, InetAddress peerAddr)
     {
         std::lock_guard<std::mutex> lock(mutex_);
-        auto key = toKey(hostname, peerAddr);
-        auto it = sessionMap_.find(key);
-        if (it != sessionMap_.end())
-        {
-            return it->second->session;
-        }
-        return nullptr;
+        auto it = sessionMap_.find(toKey(hostname, peerAddr));
+        if (it == sessionMap_.end())
+            return nullptr;
+        SSL_SESSION *s = it->second->session;
+        SSL_SESSION_up_ref(s);
+        return s;
     }
 
     void removeExcessSession()
@@ -529,7 +479,9 @@ struct OpenSSLProvider : public TLSProvider, public NonCopyable
                                    conn_->peerAddr());
             if (cachedSession)
             {
+                // SSL_set_session takes its own reference; release ours.
                 SSL_set_session(ssl_, cachedSession);
+                SSL_SESSION_free(cachedSession);
             }
             SSL_set_connect_state(ssl_);
         }
diff --git a/trantor/unittests/CMakeLists.txt b/trantor/unittests/CMakeLists.txt
@@ -4,15 +4,13 @@ add_executable(inetaddress_unittest InetAddressUnittest.cc)
 add_executable(date_unittest DateUnittest.cc)
 add_executable(split_string_unittest splitStringUnittest.cc)
 add_executable(string_encoding_unittest stringEncodingUnittest.cc)
-add_executable(ssl_name_verify_unittest sslNameVerifyUnittest.cc)
 add_executable(hash_unittest HashUnittest.cc)
 set(UNITTEST_TARGETS
     msgbuffer_unittest
     inetaddress_unittest
     date_unittest
     split_string_unittest
     string_encoding_unittest
-    ssl_name_verify_unittest
     hash_unittest
 )
 set_property(TARGET ${UNITTEST_TARGETS} PROPERTY CXX_STANDARD 14)
diff --git a/trantor/unittests/sslNameVerifyUnittest.cc b/trantor/unittests/sslNameVerifyUnittest.cc
diff --git a/trantor/utils/Utilities.cc b/trantor/utils/Utilities.cc
@@ -228,120 +228,6 @@ std::string fromWidePath(const std::wstring &wstrPath)
     return toUtf8(srcPath);
 }
 
-bool verifySslName(const std::string &certName, const std::string &hostname)
-{
-    if (certName.find('*') == std::string::npos)
-    {
-        return certName == hostname;
-    }
-
-    size_t firstDot = certName.find('.');
-    size_t hostFirstDot = hostname.find('.');
-    size_t pos, len, hostPos, hostLen;
-
-    if (firstDot != std::string::npos)
-    {
-        pos = firstDot + 1;
-    }
-    else
-    {
-        firstDot = pos = certName.size();
-    }
-
-    len = certName.size() - pos;
-
-    if (hostFirstDot != std::string::npos)
-    {
-        hostPos = hostFirstDot + 1;
-    }
-    else
-    {
-        hostFirstDot = hostPos = hostname.size();
-    }
-
-    hostLen = hostname.size() - hostPos;
-
-    // *. in the beginning of the cert name
-    if (certName.compare(0, firstDot, "*") == 0)
-    {
-        return certName.compare(pos, len, hostname, hostPos, hostLen) == 0;
-    }
-    // * in the left most. but other chars in the right
-    else if (certName[0] == '*')
-    {
-        // compare if `hostname` ends with `certName` but without the leftmost
-        // should be fine as domain names can't be that long
-        intmax_t hostnameIdx = hostname.size() - 1;
-        intmax_t certNameIdx = certName.size() - 1;
-        while (hostnameIdx >= 0 && certNameIdx != 0)
-        {
-            if (hostname[hostnameIdx] != certName[certNameIdx])
-            {
-                return false;
-            }
-            hostnameIdx--;
-            certNameIdx--;
-        }
-        if (certNameIdx != 0)
-        {
-            return false;
-        }
-        return true;
-    }
-    // * in the right of the first dot
-    else if (firstDot != 0 && certName[firstDot - 1] == '*')
-    {
-        if (certName.compare(pos, len, hostname, hostPos, hostLen) != 0)
-        {
-            return false;
-        }
-        for (size_t i = 0;
-             i < hostFirstDot && i < firstDot && certName[i] != '*';
-             i++)
-        {
-            if (hostname[i] != certName[i])
-            {
-                return false;
-            }
-        }
-        return true;
-    }
-    // else there's a * in  the middle
-    else
-    {
-        if (certName.compare(pos, len, hostname, hostPos, hostLen) != 0)
-        {
-            return false;
-        }
-        for (size_t i = 0;
-             i < hostFirstDot && i < firstDot && certName[i] != '*';
-             i++)
-        {
-            if (hostname[i] != certName[i])
-            {
-                return false;
-            }
-        }
-        intmax_t hostnameIdx = hostFirstDot - 1;
-        intmax_t certNameIdx = firstDot - 1;
-        while (hostnameIdx >= 0 && certNameIdx >= 0 &&
-               certName[certNameIdx] != '*')
-        {
-            if (hostname[hostnameIdx] != certName[certNameIdx])
-            {
-                return false;
-            }
-            hostnameIdx--;
-            certNameIdx--;
-        }
-        return true;
-    }
-
-    assert(false && "This line should not be reached in verifySslName");
-    // should not reach
-    return certName == hostname;
-}
-
 #define STRINGIFY(x) #x
 #define TOSTRING(x) STRINGIFY(x)
 
diff --git a/trantor/utils/Utilities.h b/trantor/utils/Utilities.h
@@ -171,15 +171,6 @@ inline std::string fromNativePath(const std::wstring &strPath)
     return fromWidePath(strPath);
 }
 
-/**
- * @brief Check if the name supplied by the SSL Cert matches a FQDN
- * @param certName The name supplied by the SSL Cert
- * @param hostName The FQDN to match
- *
- * @return true if matches. false otherwise
- */
-bool verifySslName(const std::string &certName, const std::string &hostName);
-
 /**
  * @brief Returns the TLS backend used by trantor. Could be "None", "OpenSSL" or
  * "Botan"

Original file line number	Diff line number	Diff line change
`@@ -108,6 +108,7 @@ void Socket::closeWrite()`
`108`	`108`	`#endif`
`109`	`109`	`{`
`110`	`110`	`LOG_SYSERR << "sockets::shutdownWrite";`
	`111`	`+ abort();`
`111`	`112`	`}`
`112`	`113`	`}`
`113`	`114`	`int Socket::read(char *buffer, uint64_t len)`
Original file line number	Diff line number	Diff line change
`@@ -474,8 +474,6 @@ SSLContextPtr trantor::newSSLContext(const TLSPolicy &policy, bool server)`
`474`	`474`	`ctx->certStore =`
`475`	`475`	`std::make_shared<Botan::Flatfile_Certificate_Store>(`
`476`	`476`	`policy.getCaPath());`
`477`		`- if (server)`
`478`		`- ctx->requireClientCert = true;`
`479`	`477`	`}`
`480`	`478`	`else if (policy.getUseSystemCertStore())`
`481`	`479`	`{`
`@@ -484,6 +482,8 @@ SSLContextPtr trantor::newSSLContext(const TLSPolicy &policy, bool server)`
`484`	`482`	`ctx->certStore = systemCertStore;`
`485`	`483`	`}`
`486`	`484`	`}`
	`485`	`+ if (server && policy.getValidate() && !policy.getCaPath().empty())`
	`486`	`+ ctx->requireClientCert = true;`
`487`	`487`
`488`	`488`	`if (policy.getUseOldTLS())`
`489`	`489`	`LOG_WARN << "SSLPloicy have set useOldTLS to true. BUt Botan does not "`