Shuffle DH params parsing and pre-loading to libsecurity

diff --git a/src/anyp/PortCfg.cc b/src/anyp/PortCfg.cc
index 1a38f83f783f2bfd862e429a99412e4f0fa7aafd..1377d489d95fb3ccc1ab0759df55a7cc6eedd471 100644 (file)
--- a/src/anyp/PortCfg.cc
+++ b/src/anyp/PortCfg.cc
@@ -55,8 +55,7 @@ AnyP::PortCfg::PortCfg() :
     certsToChain(),
     untrustedSigningCert(),
     untrustedSignPkey(),
-    clientCA(),
-    dhParams()
+    clientCA()
 #endif
 {
     memset(&tcp_keepalive, 0, sizeof(tcp_keepalive));
@@ -148,9 +147,6 @@ AnyP::PortCfg::configureSslServerContext()
 
     secure.updateTlsVersionLimits();
 
-    if (!secure.dhParamsFile.isEmpty())
-        dhParams.reset(Ssl::readDHParams(secure.dhParamsFile.c_str()));
-
     staticSslContext.reset(sslCreateServerContext(*this));
 
     if (!staticSslContext) {

diff --git a/src/anyp/PortCfg.h b/src/anyp/PortCfg.h
index 9364cf9271ab90566455f80748f5032261d15262..7c77e506eb6243496856a1f38a7227528fd5939c 100644 (file)
--- a/src/anyp/PortCfg.h
+++ b/src/anyp/PortCfg.h
@@ -86,7 +86,6 @@ public:
     Ssl::EVP_PKEY_Pointer untrustedSignPkey; ///< private key for signing untrusted generated certificates
 
     Ssl::X509_NAME_STACK_Pointer clientCA; ///< CA certificates to use when verifying client certificates
-    Ssl::DH_Pointer dhParams; ///< DH parameters for temporary/ephemeral DH key exchanges
 #endif
 };
 

diff --git a/src/security/PeerOptions.h b/src/security/PeerOptions.h
index 9045d481682c7fd7e6be29e71466cca3245e5c12..aff0810e4932249a1c83aa5c0c00ff4cda6ee8ad 100644 (file)
--- a/src/security/PeerOptions.h
+++ b/src/security/PeerOptions.h
@@ -30,7 +30,7 @@ public:
     virtual void parse(const char *);
 
     /// reset the configuration details to default
-    void clear() {*this = PeerOptions();}
+    virtual void clear() {*this = PeerOptions();}
 
     /// generate a security client-context from these configured options
     Security::ContextPointer createClientContext(bool setOptions);

diff --git a/src/security/ServerOptions.cc b/src/security/ServerOptions.cc
index 8df08048b62d4d5092e4e6e5d2f199f8932e2edf..37e774cc3b61e754b9bea6a8f4ff3abf765fb3ab 100644 (file)
--- a/src/security/ServerOptions.cc
+++ b/src/security/ServerOptions.cc
@@ -21,7 +21,8 @@
 Security::ServerOptions::ServerOptions(const Security::ServerOptions &s) :
     dh(s.dh),
     dhParamsFile(s.dhParamsFile),
-    eecdhCurve(s.eecdhCurve)
+    eecdhCurve(s.eecdhCurve),
+    parsedDhParams(s.parsedDhParams)
 {
 }
 
@@ -54,6 +55,8 @@ Security::ServerOptions::parse(const char *token)
             }
         }
 
+        loadDhParams();
+
     } else if (strncmp(token, "dhparams=", 9) == 0) {
         if (!eecdhCurve.isEmpty()) {
             debugs(83, DBG_PARSE_NOTE(1), "UPGRADE WARNING: EECDH settings in tls-dh= override dhparams=");
@@ -65,6 +68,8 @@ Security::ServerOptions::parse(const char *token)
         dh.append(token + 9);
         dhParamsFile = dh;
 
+        loadDhParams();
+
     } else {
         // parse generic TLS options
         Security::PeerOptions::parse(token);
@@ -86,35 +91,74 @@ Security::ServerOptions::dumpCfg(Packable *p, const char *pfx) const
 }
 
 void
-Security::ServerOptions::updateContextEecdh(Security::ContextPointer &ctx)
+Security::ServerOptions::loadDhParams()
 {
-    if (eecdhCurve.isEmpty())
+    if (dhParamsFile.isEmpty())
         return;
 
-    debugs(83, 9, "Setting Ephemeral ECDH curve to " << eecdhCurve << ".");
-
-#if USE_OPENSSL && OPENSSL_VERSION_NUMBER >= 0x0090800fL && !defined(OPENSSL_NO_ECDH)
-    int nid = OBJ_sn2nid(eecdhCurve.c_str());
-    if (!nid) {
-        debugs(83, DBG_CRITICAL, "ERROR: Unknown EECDH curve '" << eecdhCurve << "'");
-        return;
+#if USE_OPENSSL
+    DH *dhp = nullptr;
+    if (FILE *in = fopen(dhParamsFile.c_str(), "r")) {
+        dhp = PEM_read_DHparams(in, NULL, NULL, NULL);
+        fclose(in);
     }
 
-    auto ecdh = EC_KEY_new_by_curve_name(nid);
-    if (!ecdh) {
-        auto ssl_error = ERR_get_error();
-        debugs(83, DBG_CRITICAL, "ERROR: Unable to configure Ephemeral ECDH: " << ERR_error_string(ssl_error, NULL));
+    if (!dhp) {
+        debugs(83, DBG_IMPORTANT, "WARNING: Failed to read DH parameters '" << dhParamsFile << "'");
         return;
     }
 
-    if (SSL_CTX_set_tmp_ecdh(ctx, ecdh) != 0) {
-        auto ssl_error = ERR_get_error();
-        debugs(83, DBG_CRITICAL, "ERROR: Unable to set Ephemeral ECDH: " << ERR_error_string(ssl_error, NULL));
+    int codes;
+    if (DH_check(dhp, &codes) == 0) {
+        if (codes) {
+            debugs(83, DBG_IMPORTANT, "WARNING: Failed to verify DH parameters '" << dhParamsFile << "' (" << std::hex << codes << ")");
+            DH_free(dhp);
+            dhp = nullptr;
+        }
     }
-    EC_KEY_free(ecdh);
+
+    parsedDhParams.reset(dhp);
+#endif
+}
+
+void
+Security::ServerOptions::updateContextEecdh(Security::ContextPointer &ctx)
+{
+    // set Elliptic Curve details into the server context
+    if (!eecdhCurve.isEmpty()) {
+        debugs(83, 9, "Setting Ephemeral ECDH curve to " << eecdhCurve << ".");
+
+#if USE_OPENSSL && OPENSSL_VERSION_NUMBER >= 0x0090800fL && !defined(OPENSSL_NO_ECDH)
+        int nid = OBJ_sn2nid(eecdhCurve.c_str());
+        if (!nid) {
+            debugs(83, DBG_CRITICAL, "ERROR: Unknown EECDH curve '" << eecdhCurve << "'");
+            return;
+        }
+
+        auto ecdh = EC_KEY_new_by_curve_name(nid);
+        if (!ecdh) {
+            auto ssl_error = ERR_get_error();
+            debugs(83, DBG_CRITICAL, "ERROR: Unable to configure Ephemeral ECDH: " << ERR_error_string(ssl_error, NULL));
+            return;
+        }
+
+        if (SSL_CTX_set_tmp_ecdh(ctx, ecdh) != 0) {
+            auto ssl_error = ERR_get_error();
+            debugs(83, DBG_CRITICAL, "ERROR: Unable to set Ephemeral ECDH: " << ERR_error_string(ssl_error, NULL));
+        }
+        EC_KEY_free(ecdh);
+
 #else
-    debugs(83, DBG_CRITICAL, "ERROR: EECDH is not available in this build." <<
-           " Please link against OpenSSL>=0.9.8 and ensure OPENSSL_NO_ECDH is not set.");
+        debugs(83, DBG_CRITICAL, "ERROR: EECDH is not available in this build." <<
+               " Please link against OpenSSL>=0.9.8 and ensure OPENSSL_NO_ECDH is not set.");
+#endif
+    }
+
+    // set DH parameters into the server context
+#if USE_OPENSSL
+    if (parsedDhParams.get()) {
+        SSL_CTX_set_tmp_dh(ctx, parsedDhParams.get());
+    }
 #endif
 }
 

diff --git a/src/security/ServerOptions.h b/src/security/ServerOptions.h
index 5b24a211379232da70dafb8614762ddc095412a7..9c09ea21f5ef896f8a4fa86e85cb4a948394218e 100644 (file)
--- a/src/security/ServerOptions.h
+++ b/src/security/ServerOptions.h
@@ -30,10 +30,17 @@ public:
     /// update the context with DH, EDH, EECDH settings
     void updateContextEecdh(Security::ContextPointer &);
 
-public:
+private:
+    void loadDhParams();
+
+//public:
     SBuf dh;            ///< Diffi-Helman cipher config
+
+private:
     SBuf dhParamsFile;  ///< Diffi-Helman ciphers parameter file
     SBuf eecdhCurve;    ///< Elliptic curve for ephemeral EC-based DH key exchanges
+
+    Security::DhePointer parsedDhParams; ///< DH parameters for temporary/ephemeral DH key exchanges
 };
 
 } // namespace Security

diff --git a/src/security/forward.h b/src/security/forward.h
index 2a0522f1cef528362439a60408ae71918b4a9bb8..9286f836dc3b4c9668e18752263b4ae19be19e7d 100644 (file)
--- a/src/security/forward.h
+++ b/src/security/forward.h
@@ -76,6 +76,13 @@ typedef void *CrlPointer;
 
 typedef std::list<Security::CrlPointer> CertRevokeList;
 
+#if USE_OPENSSL
+CtoCpp1(DH_free, DH *);
+typedef Security::LockingPointer<DH, DH_free_cpp, CRYPTO_LOCK_DH> DhePointer;
+#else
+typedef void *DhePointer;
+#endif
+
 } // namespace Security
 
 #endif /* SQUID_SRC_SECURITY_FORWARD_H */

diff --git a/src/ssl/gadgets.h b/src/ssl/gadgets.h
index 366538ca152bb078b2df7c645bf023a189fa4318..c5c44e2370d8e83e85cd2abcf157fbaa54958e12 100644 (file)
--- a/src/ssl/gadgets.h
+++ b/src/ssl/gadgets.h
@@ -72,9 +72,6 @@ typedef TidyPointer<SSL_CTX, SSL_CTX_free_cpp> SSL_CTX_Pointer;
 CtoCpp1(SSL_free, SSL *)
 typedef TidyPointer<SSL, SSL_free_cpp> SSL_Pointer;
 
-CtoCpp1(DH_free, DH *);
-typedef TidyPointer<DH, DH_free_cpp> DH_Pointer;
-
 sk_free_wrapper(sk_X509_NAME, STACK_OF(X509_NAME) *, X509_NAME_free)
 typedef TidyPointer<STACK_OF(X509_NAME), sk_X509_NAME_free_wrapper> X509_NAME_STACK_Pointer;
 

diff --git a/src/ssl/support.cc b/src/ssl/support.cc
index 6860a86534551ef657645b06dca5c8efcf869c61..2ee7e21c80e11e82e432ef79ba93afca20b263e6 100644 (file)
--- a/src/ssl/support.cc
+++ b/src/ssl/support.cc
@@ -471,30 +471,6 @@ ssl_initialize(void)
     ssl_ex_index_ssl_validation_counter = SSL_get_ex_new_index(0, (void *) "ssl_validation_counter", NULL, NULL, &ssl_free_int);
 }
 
-DH *
-Ssl::readDHParams(const char *dhfile)
-{
-    FILE *in = fopen(dhfile, "r");
-    DH *dh = NULL;
-    int codes;
-
-    if (in) {
-        dh = PEM_read_DHparams(in, NULL, NULL, NULL);
-        fclose(in);
-    }
-
-    if (!dh)
-        debugs(83, DBG_IMPORTANT, "WARNING: Failed to read DH parameters '" << dhfile << "'");
-    else if (dh && DH_check(dh, &codes) == 0) {
-        if (codes) {
-            debugs(83, DBG_IMPORTANT, "WARNING: Failed to verify DH parameters '" << dhfile  << "' (" << std::hex << codes  << ")");
-            DH_free(dh);
-            dh = NULL;
-        }
-    }
-    return dh;
-}
-
 #if defined(SSL3_FLAGS_NO_RENEGOTIATE_CIPHERS)
 static void
 ssl_info_cb(const SSL *ssl, int where, int ret)
@@ -571,10 +547,6 @@ configureSslContext(SSL_CTX *sslContext, AnyP::PortCfg &port)
         SSL_CTX_set_verify(sslContext, SSL_VERIFY_NONE, NULL);
     }
 
-    if (port.dhParams.get()) {
-        SSL_CTX_set_tmp_dh(sslContext, port.dhParams.get());
-    }
-
     if (port.secure.parsedFlags & SSL_FLAG_DONT_VERIFY_DOMAIN)
         SSL_CTX_set_ex_data(sslContext, ssl_ctx_ex_index_dont_verify_domain, (void *) -1);
 

diff --git a/src/ssl/support.h b/src/ssl/support.h
index 75cb39ee246c07aa399ffcd29b354c63d64e4e80..949fea79a20746947f68ce4817b3d01ff934a30b 100644 (file)
--- a/src/ssl/support.h
+++ b/src/ssl/support.h
@@ -155,12 +155,6 @@ inline const char *bumpMode(int bm)
     return (0 <= bm && bm < Ssl::bumpEnd) ? Ssl::BumpModeStr[bm] : NULL;
 }
 
-/**
- \ingroup ServerProtocolSSLAPI
- * Load DH params from file
- */
-DH *readDHParams(const char *dhfile);
-
 /**
   \ingroup ServerProtocolSSLAPI
   * Generate a certificate to be used as untrusted signing certificate, based on a trusted CA

diff --git a/src/tests/stub_libsslsquid.cc b/src/tests/stub_libsslsquid.cc
index 401cb7afc033fd85660a100b671ff55b3159e1fa..b4ee0098ff237b249945afc9650ef7303384b4b5 100644 (file)
--- a/src/tests/stub_libsslsquid.cc
+++ b/src/tests/stub_libsslsquid.cc
@@ -72,7 +72,6 @@ namespace Ssl
 //GETX509ATTRIBUTE GetX509CAAttribute;
 //GETX509ATTRIBUTE GetX509Fingerprint;
 const char *BumpModeStr[] = {""};
-DH *readDHParams(const char *dhfile) STUB_RETVAL(NULL)
 bool generateUntrustedCert(Security::CertPointer & untrustedCert, EVP_PKEY_Pointer & untrustedPkey, Security::CertPointer const & cert, EVP_PKEY_Pointer const & pkey) STUB_RETVAL(false)
 SSL_CTX * generateSslContext(CertificateProperties const &properties, AnyP::PortCfg &port) STUB_RETVAL(NULL)
 bool verifySslCertificate(SSL_CTX * sslContext,  CertificateProperties const &properties) STUB_RETVAL(false)