ssl-params: Added ssl_dh_parameters_length & removed ssl_parameters_regenerate setting.

ssl_parameters_regenerate was based on some text from GNUTLS documentation a
long time ago, but there's really not much point in doing it.

Ideally we should also support "openssl dhparam" input files, but for now
there's the ssl_dh_parameters_length setting that can be used to specify the
wanted DH parameters length. If the current ssl-parameters.dat has a
different length, it's regenerated.

We should probably at some point support also built-in DH parameters which
are returned while the ssl-params runs.

diff --git a/doc/example-config/conf.d/10-ssl.conf b/doc/example-config/conf.d/10-ssl.conf
index cab9423f6a1073b81a343efc6ed93bb606d3a040..7ae6b7a2bbca23628a421b6576930505d90271b0 100644 (file)
--- a/doc/example-config/conf.d/10-ssl.conf
+++ b/doc/example-config/conf.d/10-ssl.conf
@@ -42,10 +42,8 @@ ssl_key = </etc/ssl/private/dovecot.pem
 # auth_ssl_username_from_cert=yes.
 #ssl_cert_username_field = commonName
 
-# How often to regenerate the SSL parameters file. Generation is quite CPU
-# intensive operation. The value is in hours, 0 disables regeneration
-# entirely.
-#ssl_parameters_regenerate = 168
+# DH parameters length to use.
+#ssl_dh_parameters_length = 1024
 
 # SSL protocols to use
 #ssl_protocols = !SSLv2

diff --git a/src/login-common/ssl-proxy-openssl.c b/src/login-common/ssl-proxy-openssl.c
index 3f5a6c161c5095e88ece69274a91a30133c2b4f4..4ebbfd3dc2f40d8f2499ecd30c80031f5df05e41 100644 (file)
--- a/src/login-common/ssl-proxy-openssl.c
+++ b/src/login-common/ssl-proxy-openssl.c
@@ -86,7 +86,7 @@ struct ssl_parameters {
        time_t last_refresh;
        int fd;
 
-       DH *dh_512, *dh_1024;
+       DH *dh_512, *dh_default;
 };
 
 struct ssl_server_context {
@@ -163,9 +163,9 @@ static int ssl_server_context_cmp(const struct ssl_server_context *ctx1,
        return ctx1->verify_client_cert == ctx2->verify_client_cert ? 0 : 1;
 }
 
-static void ssl_params_corrupted(void)
+static void ssl_params_corrupted(const char *reason)
 {
-       i_fatal("Corrupted SSL parameters file in state_dir: ssl-parameters.dat");
+       i_fatal("Corrupted SSL ssl-parameters.dat in state_dir: %s", reason);
 }
 
 static void read_next(struct ssl_parameters *params, void *data, size_t size)
@@ -175,7 +175,7 @@ static void read_next(struct ssl_parameters *params, void *data, size_t size)
        if ((ret = read_full(params->fd, data, size)) < 0)
                i_fatal("read(%s) failed: %m", params->path);
        if (ret == 0)
-               ssl_params_corrupted();
+               ssl_params_corrupted("Truncated file");
 }
 
 static bool read_dh_parameters_next(struct ssl_parameters *params)
@@ -194,7 +194,7 @@ static bool read_dh_parameters_next(struct ssl_parameters *params)
        /* read data size. */
        read_next(params, &len, sizeof(len));
        if (len > 1024*100) /* should be enough? */
-               ssl_params_corrupted();
+               ssl_params_corrupted("File too large");
 
        buf = i_malloc(len);
        read_next(params, buf, len);
@@ -202,13 +202,15 @@ static bool read_dh_parameters_next(struct ssl_parameters *params)
        cbuf = buf;
        switch (bits) {
        case 512:
+               if (params->dh_512 != NULL)
+                       ssl_params_corrupted("Duplicate 512bit parameters");
                params->dh_512 = d2i_DHparams(NULL, &cbuf, len);
                break;
-       case 1024:
-               params->dh_1024 = d2i_DHparams(NULL, &cbuf, len);
-               break;
        default:
-               ssl_params_corrupted();
+               if (params->dh_default != NULL)
+                       ssl_params_corrupted("Duplicate default parameters");
+               params->dh_default = d2i_DHparams(NULL, &cbuf, len);
+               break;
        }
 
        i_free(buf);
@@ -221,9 +223,9 @@ static void ssl_free_parameters(struct ssl_parameters *params)
                DH_free(params->dh_512);
                 params->dh_512 = NULL;
        }
-       if (params->dh_1024 != NULL) {
-               DH_free(params->dh_1024);
-                params->dh_1024 = NULL;
+       if (params->dh_default != NULL) {
+               DH_free(params->dh_default);
+                params->dh_default = NULL;
        }
 }
 
@@ -250,7 +252,7 @@ static void ssl_refresh_parameters(struct ssl_parameters *params)
                i_fatal("read(%s) failed: %m", params->path);
        else if (ret != 0) {
                /* more data than expected */
-               ssl_params_corrupted();
+               ssl_params_corrupted("More data than expected");
        }
 
        if (close(params->fd) < 0)
@@ -840,12 +842,10 @@ static RSA *ssl_gen_rsa_key(SSL *ssl ATTR_UNUSED,
 static DH *ssl_tmp_dh_callback(SSL *ssl ATTR_UNUSED,
                               int is_export, int keylength)
 {
-       /* Well, I'm not exactly sure why the logic in here is this.
-          It's the same as in Postfix, so it can't be too wrong. */
        if (is_export && keylength == 512 && ssl_params.dh_512 != NULL)
                return ssl_params.dh_512;
 
-       return ssl_params.dh_1024;
+       return ssl_params.dh_default;
 }
 
 static void ssl_info_callback(const SSL *ssl, int where, int ret)

diff --git a/src/ssl-params/ssl-params-openssl.c b/src/ssl-params/ssl-params-openssl.c
index 3fc45f468baeeee84b3e6fcf74c95416c9c27474..206ab6c507f467d1a2962695436d3d85054e2c0f 100644 (file)
--- a/src/ssl-params/ssl-params-openssl.c
+++ b/src/ssl-params/ssl-params-openssl.c
@@ -13,8 +13,6 @@
    default.. */
 #define DH_GENERATOR 2
 
-static int dh_param_bitsizes[] = { 512, 1024 };
-
 static const char *ssl_last_error(void)
 {
        unsigned long err;
@@ -56,13 +54,12 @@ static void generate_dh_parameters(int bitsize, int fd, const char *fname)
        i_free(buf);
 }
 
-void ssl_generate_parameters(int fd, const char *fname)
+void ssl_generate_parameters(int fd, unsigned int dh_length, const char *fname)
 {
-       unsigned int i;
        int bits;
 
-       for (i = 0; i < N_ELEMENTS(dh_param_bitsizes); i++)
-               generate_dh_parameters(dh_param_bitsizes[i], fd, fname);
+       generate_dh_parameters(512, fd, fname);
+       generate_dh_parameters(dh_length, fd, fname);
        bits = 0;
        if (write_full(fd, &bits, sizeof(bits)) < 0)
                i_fatal("write_full() failed for file %s: %m", fname);

diff --git a/src/ssl-params/ssl-params-settings.c b/src/ssl-params/ssl-params-settings.c
index dda8e901d777d64b9b4509e0976e3e1b51e0331b..42cc210f4eec55bd06b43fc908c45b5b73e47e58 100644 (file)
--- a/src/ssl-params/ssl-params-settings.c
+++ b/src/ssl-params/ssl-params-settings.c
@@ -61,12 +61,14 @@ struct service_settings ssl_params_service_settings = {
 
 static const struct setting_define ssl_params_setting_defines[] = {
        DEF(SET_TIME, ssl_parameters_regenerate),
+       DEF(SET_UINT, ssl_dh_parameters_length),
 
        SETTING_DEFINE_LIST_END
 };
 
 static const struct ssl_params_settings ssl_params_default_settings = {
-       .ssl_parameters_regenerate = 3600*24*7
+       .ssl_parameters_regenerate = 0,
+       .ssl_dh_parameters_length = 1024
 };
 
 const struct setting_parser_info ssl_params_setting_parser_info = {

diff --git a/src/ssl-params/ssl-params-settings.h b/src/ssl-params/ssl-params-settings.h
index d539b77134e6650d9487b078a83469f796644a9b..951c6a05edb9ebe5ccfc9ae2a9bee74760a69596 100644 (file)
--- a/src/ssl-params/ssl-params-settings.h
+++ b/src/ssl-params/ssl-params-settings.h
@@ -5,6 +5,7 @@ struct master_service;
 
 struct ssl_params_settings {
        unsigned int ssl_parameters_regenerate;
+       unsigned int ssl_dh_parameters_length;
 };
 
 struct ssl_params_settings *

diff --git a/src/ssl-params/ssl-params.c b/src/ssl-params/ssl-params.c
index 751eb986d6daca6cb5712cc0223a06d81fd060ce..479249f58f86f2392ee9f4f91ef72f2915ac3f73 100644 (file)
--- a/src/ssl-params/ssl-params.c
+++ b/src/ssl-params/ssl-params.c
@@ -22,7 +22,7 @@
 #  include <sys/resource.h>
 #endif
 
-#define MAX_PARAM_FILE_SIZE 1024
+#define MAX_PARAM_FILE_SIZE 1024*1024
 #define SSL_BUILD_PARAM_TIMEOUT_SECS (60*30)
 #define SSL_PARAMS_PRIORITY 15
 
@@ -31,11 +31,11 @@ struct ssl_params {
        struct ssl_params_settings set;
 
        time_t last_mtime;
-       struct timeout *to_rebuild;
        ssl_params_callback_t *callback;
 };
 
-static void ssl_params_if_unchanged(const char *path, time_t mtime)
+static void ssl_params_if_unchanged(const char *path, time_t mtime,
+                                   unsigned int ssl_dh_parameters_length)
 {
        const char *temp_path;
        struct file_lock *lock;
@@ -99,7 +99,7 @@ static void ssl_params_if_unchanged(const char *path, time_t mtime)
 
        i_info("Generating SSL parameters");
 #ifdef HAVE_SSL
-       ssl_generate_parameters(fd, temp_path);
+       ssl_generate_parameters(fd, ssl_dh_parameters_length, temp_path);
 #endif
 
        if (rename(temp_path, path) < 0)
@@ -129,9 +129,6 @@ static void ssl_params_close_listeners(void)
 
 static void ssl_params_rebuild(struct ssl_params *param)
 {
-       if (param->to_rebuild != NULL)
-               timeout_remove(&param->to_rebuild);
-
        switch (fork()) {
        case -1:
                i_fatal("fork() failed: %m");
@@ -139,7 +136,8 @@ static void ssl_params_rebuild(struct ssl_params *param)
                /* child - close listener fds so a long-running ssl-params
                   doesn't cause Dovecot restart to fail */
                ssl_params_close_listeners();
-               ssl_params_if_unchanged(param->path, param->last_mtime);
+               ssl_params_if_unchanged(param->path, param->last_mtime,
+                                       param->set.ssl_dh_parameters_length);
                exit(0);
        default:
                /* parent */
@@ -147,27 +145,38 @@ static void ssl_params_rebuild(struct ssl_params *param)
        }
 }
 
-static void ssl_params_set_timeout(struct ssl_params *param)
+static bool
+ssl_params_verify(struct ssl_params *param,
+                 const unsigned char *data, size_t size)
 {
-       time_t next_rebuild, diff;
-
-       if (param->to_rebuild != NULL)
-               timeout_remove(&param->to_rebuild);
-       if (param->set.ssl_parameters_regenerate == 0)
-               return;
-
-       next_rebuild = param->last_mtime +
-               param->set.ssl_parameters_regenerate;
-
-       if (ioloop_time >= next_rebuild) {
-               ssl_params_rebuild(param);
-               return;
+       unsigned int bitsize, len;
+       bool found = FALSE;
+
+       /* <bitsize><length><data>... */
+       while (size >= sizeof(bitsize)) {
+               memcpy(&bitsize, data, sizeof(bitsize));
+               if (bitsize == 0) {
+                       if (found)
+                               return TRUE;
+                       i_warning("Regenerating %s for ssl_dh_parameters_length=%u",
+                                 param->path, param->set.ssl_dh_parameters_length);
+                       return FALSE;
+               }
+               data += sizeof(bitsize);
+               size -= sizeof(bitsize);
+               if (bitsize == param->set.ssl_dh_parameters_length)
+                       found = TRUE;
+
+               if (size < sizeof(len))
+                       break;
+               memcpy(&len, data, sizeof(len));
+               if (len > size - sizeof(len))
+                       break;
+               data += sizeof(bitsize) + len;
+               size -= sizeof(bitsize) + len;
        }
-
-       diff = next_rebuild - ioloop_time;
-       if (diff > INT_MAX / 1000)
-               diff = INT_MAX / 1000;
-       param->to_rebuild = timeout_add(diff * 1000, ssl_params_rebuild, param);
+       i_error("Corrupted %s", param->path);
+       return FALSE;
 }
 
 static int ssl_params_read(struct ssl_params *param)
@@ -188,6 +197,7 @@ static int ssl_params_read(struct ssl_params *param)
                i_close_fd(&fd);
                return -1;
        }
+       param->last_mtime = st.st_mtime;
        if (st.st_size == 0 || st.st_size > MAX_PARAM_FILE_SIZE) {
                i_error("Corrupted file: %s", param->path);
                i_close_fd(&fd);
@@ -202,9 +212,9 @@ static int ssl_params_read(struct ssl_params *param)
        else if (ret == 0) {
                i_error("File unexpectedly shrank: %s", param->path);
                ret = -1;
+       } else if (!ssl_params_verify(param, buffer, st.st_size)) {
+               ret = -1;
        } else {
-               param->last_mtime = st.st_mtime;
-               ssl_params_set_timeout(param);
                param->callback(buffer, st.st_size);
        }
 
@@ -238,8 +248,6 @@ void ssl_params_deinit(struct ssl_params **_param)
        struct ssl_params *param = *_param;
 
        *_param = NULL;
-       if (param->to_rebuild != NULL)
-               timeout_remove(&param->to_rebuild);
        i_free(param->path);
        i_free(param);
 }

diff --git a/src/ssl-params/ssl-params.h b/src/ssl-params/ssl-params.h
index e5eb070c0412b29266ee4e8f7308f71158e12294..19d8f6e9db6bc1112424a9aeff1a78a151b4b1d1 100644 (file)
--- a/src/ssl-params/ssl-params.h
+++ b/src/ssl-params/ssl-params.h
@@ -12,6 +12,6 @@ void ssl_params_deinit(struct ssl_params **param);
 
 void ssl_params_refresh(struct ssl_params *param);
 
-void ssl_generate_parameters(int fd, const char *fname);
+void ssl_generate_parameters(int fd, unsigned int dh_length, const char *fname);
 
 #endif