httpd-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Joe Orton <jor...@redhat.com>
Subject Re: Memory leak in mod_ssl ssl_callback_TmpDH
Date Wed, 28 May 2014 13:42:40 GMT
On Wed, May 28, 2014 at 09:05:06AM +0200, Kaspar Brand wrote:
> Agree, CPU time is not a concern, get_dh_XXX() is only about populating
> DH with builtin constants (DH_generate_key happens at connection time,
> and is fast anyway).

Ah, I did not realise.  That is even more reason to do this at startup 
then, surely.  Users do complain (tho rarely) about modules which leak 
across reloads, so it is worth caring about that too, and we can stay 
with globals rather than polluting the module structure.

Any objections?  This seems cleaner than fussing around with atomics and 
ignoring the race/leak.

Index: modules/ssl/ssl_engine_init.c
===================================================================
--- modules/ssl/ssl_engine_init.c	(revision 1597999)
+++ modules/ssl/ssl_engine_init.c	(working copy)
@@ -47,6 +47,73 @@
 #define KEYTYPES "RSA or DSA"
 #endif
 
+/*
+ * Grab well-defined DH parameters from OpenSSL, see <openssl/bn.h>
+ * (get_rfc*) for all available primes.
+ */
+static DH *make_dh_params(BIGNUM *(*prime)(BIGNUM *), const char *gen)
+{
+    DH *dh = NULL;
+    DH *dh_tmp;
+
+    if (dh) {
+        return dh;
+    }
+    if (!(dh_tmp = DH_new())) {
+        return NULL;
+    }
+    dh_tmp->p = prime(NULL);
+    BN_dec2bn(&dh_tmp->g, gen);
+    if (!dh_tmp->p || !dh_tmp->g) {
+        DH_free(dh_tmp);
+        return NULL;
+    }
+    dh = dh_tmp;
+    return dh;
+}
+
+static DH *dhparam1024, *dhparam2048, *dhparam3072, *dhparam4096;
+
+static void init_dh_params(void)
+{
+    /*
+     * Prepare DH parameters from 1024 to 4096 bits, in 1024-bit increments
+     */
+    dhparam1024 = make_dh_params(get_rfc2409_prime_1024, "2");
+    dhparam2048 = make_dh_params(get_rfc3526_prime_2048, "2");
+    dhparam3072 = make_dh_params(get_rfc3526_prime_3072, "2");
+    dhparam4096 = make_dh_params(get_rfc3526_prime_4096, "2");
+}
+
+static void free_dh_params(void)
+{
+    DH_free(dhparam1024);
+    DH_free(dhparam2048);
+    DH_free(dhparam3072);
+    DH_free(dhparam4096);
+    dhparam1024 = dhparam2048 = dhparam3072 = dhparam4096 = NULL;
+}
+
+/* Hand out the same DH structure though once generated as we leak
+ * memory otherwise and freeing the structure up after use would be
+ * hard to track and in fact is not needed at all as it is safe to
+ * use the same parameters over and over again security wise (in
+ * contrast to the keys itself) and code safe as the returned structure
+ * is duplicated by OpenSSL anyway. Hence no modification happens
+ * to our copy. */
+DH *modssl_get_dh_params(unsigned keylen)
+{
+    if (keylen >= 4096)
+        return dhparam4096;
+    else if (keylen >= 3072)
+        return dhparam3072;
+    else if (keylen >= 2048)
+        return dhparam2048;
+    else
+        return dhparam1024;
+   
+}
+
 static void ssl_add_version_components(apr_pool_t *p,
                                        server_rec *s)
 {
@@ -277,6 +344,8 @@
 
     SSL_init_app_data2_idx(); /* for SSL_get_app_data2() at request time */
 
+    init_dh_params();
+
     return OK;
 }
 
@@ -1706,5 +1775,7 @@
         ssl_init_ctx_cleanup(sc->server);
     }
 
+    free_dh_params();
+
     return APR_SUCCESS;
 }
Index: modules/ssl/ssl_engine_kernel.c
===================================================================
--- modules/ssl/ssl_engine_kernel.c	(revision 1597999)
+++ modules/ssl/ssl_engine_kernel.c	(working copy)
@@ -1311,47 +1311,6 @@
 */
 
 /*
- * Grab well-defined DH parameters from OpenSSL, see <openssl/bn.h>
- * (get_rfc*) for all available primes.
- * Hand out the same DH structure though once generated as we leak
- * memory otherwise and freeing the structure up after use would be
- * hard to track and in fact is not needed at all as it is safe to
- * use the same parameters over and over again security wise (in
- * contrast to the keys itself) and code safe as the returned structure
- * is duplicated by OpenSSL anyway. Hence no modification happens
- * to our copy.
- */
-#define make_get_dh(rfc,size,gen) \
-static DH *get_dh##size(void) \
-{ \
-    static DH *dh = NULL; \
-    DH *dh_tmp; \
-\
-    if (dh) { \
-        return dh; \
-    } \
-    if (!(dh_tmp = DH_new())) { \
-        return NULL; \
-    } \
-    dh_tmp->p = get_##rfc##_prime_##size(NULL); \
-    BN_dec2bn(&dh_tmp->g, #gen); \
-    if (!dh_tmp->p || !dh_tmp->g) { \
-        DH_free(dh_tmp); \
-        return NULL; \
-    } \
-    dh = dh_tmp; \
-    return dh; \
-}
-
-/*
- * Prepare DH parameters from 1024 to 4096 bits, in 1024-bit increments
- */
-make_get_dh(rfc2409, 1024, 2)
-make_get_dh(rfc3526, 2048, 2)
-make_get_dh(rfc3526, 3072, 2)
-make_get_dh(rfc3526, 4096, 2)
-
-/*
  * Hand out standard DH parameters, based on the authentication strength
  */
 DH *ssl_callback_TmpDH(SSL *ssl, int export, int keylen)
@@ -1390,14 +1349,8 @@
     ap_log_cerror(APLOG_MARK, APLOG_TRACE2, 0, c,
                   "handing out built-in DH parameters for %d-bit authenticated connection",
keylen);
 
-    if (keylen >= 4096)
-        return get_dh4096();
-    else if (keylen >= 3072)
-        return get_dh3072();
-    else if (keylen >= 2048)
-        return get_dh2048();
-    else
-        return get_dh1024();
+    return modssl_get_dh_params(keylen);
+
 }
 
 /*
Index: modules/ssl/ssl_private.h
===================================================================
--- modules/ssl/ssl_private.h	(revision 1597999)
+++ modules/ssl/ssl_private.h	(working copy)
@@ -931,6 +931,11 @@
                                             conn_rec *c, apr_pool_t *p);
 #endif
 
+/* Retrieve DH parameters for given key length.  Return value should
+ * be treated as unmutable, since it is stored in process-global
+ * memory. */
+DH *modssl_get_dh_params(unsigned keylen);
+
 #if HAVE_VALGRIND
 extern int ssl_running_on_valgrind;
 #endif

Mime
View raw message