Added tls_dh_max_bits & check tls_require_ciphers early.

Janne Snabb tracked down the GnuTLS 2.12 vs NSS (Thunderbird) interop
problems to a hard-coded limit of 2236 bits for DH in NSS while GnuTLS
was suggesting 2432 bits as normal.

Added new global option tls_dh_max_bits to clamp all DH values (client
or server); unexpanded integer.  Default value to 2236.  Apply to both
GnuTLS and OpenSSL (which requires tls_dh_params for this).

Tired of debugging "SMTP fails TLS" error messages in mailing-lists
caused by OpenSSL library/include clashes, and of finding out I typo'd
in tls_require_ciphers only at the STARTTLS handshake.  During readconf,
fork/drop-privs/initialise-TLS-library.  In that, if tls_require_ciphers
is set, then validate it.

The validation child will panic if it can't initialise or if
tls_require_ciphers can't be parsed, else it exits 0.  If the child
exits anything other than 0, the main Exim process will exit.

7 files changed, 252 insertions, 4 deletions

diff --git a/src/README.UPDATING b/src/README.UPDATING
index 7ce35dff8..e685b8ec3 100644
--- a/src/README.UPDATING
+++ b/src/README.UPDATING
@@ -66,6 +66,11 @@ Exim version 4.80
    security for compatibility.  Exim is now defaulting to higher security and
    rewarding more modern clients.
 
+   If the option tls_dhparams is set and the parameters loaded from the file
+   have a bit-count greater than the new option tls_dh_max_bits, then the file
+   will now be ignored.  If this affects you, raise the tls_dh_max_bits limit.
+   We suspect that most folks are using dated defaults and will not be affected.
+
  * Ldap lookups returning multi-valued attributes now separate the attributes
    with only a comma, not a comma-space sequence.  Also, an actual comma within
    a returned attribute is doubled.  This makes it possible to parse the
@@ -111,6 +116,25 @@ Exim version 4.80
    support for SNI and other features more readily.  We regret that it wasn't
    feasible to retain the three dropped options.
 
+ * If built with TLS support, then Exim will now validate the value of
+   the main section tls_require_ciphers option at start-up.  Before, this
+   would cause a STARTTLS 4xx failure, now it causes a failure to start.
+   Running with a broken configuration which causes failures that may only
+   be left in the logs has been traded off for something more visible.  This
+   change makes an existing problem more prominent, but we do not believe
+   anyone would deliberately be running with an invalid tls_require_ciphers
+   option.
+
+   This also means that library linkage issues caused by conflicts of some
+   kind might take out the main daemon, not just the delivery or receiving
+   process.  Conceivably some folks might prefer to continue delivering
+   mail plaintext when their binary is broken in this way, if there is a
+   server that is a candidate to receive such mails that does not advertise
+   STARTTLS.  Note that Exim is typically a setuid root binary and given
+   broken linkage problems that cause segfaults, we feel it is safer to
+   fail completely.  (The check is not done as root, to ensure that problems
+   here are not made worse by the check).
+
 
 Exim version 4.77
 -----------------
diff --git a/src/src/functions.h b/src/src/functions.h
index cf8c54fe9..160f5133e 100644
--- a/src/src/functions.h
+++ b/src/src/functions.h
@@ -33,6 +33,7 @@ extern int     tls_server_start(const uschar *);
 extern BOOL    tls_smtp_buffered(void);
 extern int     tls_ungetc(int);
 extern int     tls_write(const uschar *, size_t);
+extern uschar *tls_validate_require_cipher(void);
 extern void    tls_version_report(FILE *);
 #ifndef USE_GNUTLS
 extern BOOL    tls_openssl_options_parse(uschar *, long *);
diff --git a/src/src/globals.c b/src/src/globals.c
index e69667609..824175fab 100644
--- a/src/src/globals.c
+++ b/src/src/globals.c
@@ -111,6 +111,10 @@ const pcre *regex_STARTTLS     = NULL;
 uschar *tls_advertise_hosts    = NULL;    /* This is deliberate */
 uschar *tls_certificate        = NULL;
 uschar *tls_crl                = NULL;
+/* This default matches NSS DH_MAX_P_BITS value at current time (2012), because
+that's the interop problem which has been observed: GnuTLS suggesting a higher
+bit-count as "NORMAL" (2432) and Thunderbird dropping connection. */
+int     tls_dh_max_bits        = 2236;
 uschar *tls_dhparam            = NULL;
 #if defined(EXPERIMENTAL_OCSP) && !defined(USE_GNUTLS)
 uschar *tls_ocsp_file          = NULL;
diff --git a/src/src/globals.h b/src/src/globals.h
index ac6aa15d1..fbbec3230 100644
--- a/src/src/globals.h
+++ b/src/src/globals.h
@@ -93,6 +93,7 @@ extern uschar *tls_advertise_hosts;    /* host for which TLS is advertised */
 extern uschar *tls_certificate;        /* Certificate file */
 extern uschar *tls_channelbinding_b64; /* string of base64 channel binding */
 extern uschar *tls_crl;                /* CRL File */
+extern int     tls_dh_max_bits;        /* don't accept higher lib suggestions */
 extern uschar *tls_dhparam;            /* DH param file */
 #if defined(EXPERIMENTAL_OCSP) && !defined(USE_GNUTLS)
 extern uschar *tls_ocsp_file;          /* OCSP stapling proof file */
diff --git a/src/src/readconf.c b/src/src/readconf.c
index 7e347881a..bddb74c0a 100644
--- a/src/src/readconf.c
+++ b/src/src/readconf.c
@@ -415,6 +415,7 @@ static optionlist optionlist_config[] = {
   { "tls_advertise_hosts",      opt_stringptr,   &tls_advertise_hosts },
   { "tls_certificate",          opt_stringptr,   &tls_certificate },
   { "tls_crl",                  opt_stringptr,   &tls_crl },
+  { "tls_dh_max_bits",          opt_int,         &tls_dh_max_bits },
   { "tls_dhparam",              opt_stringptr,   &tls_dhparam },
 #if defined(EXPERIMENTAL_OCSP) && !defined(USE_GNUTLS)
   { "tls_ocsp_file",            opt_stringptr,   &tls_ocsp_file },
@@ -2772,6 +2773,69 @@ log_write(0, LOG_MAIN|LOG_PANIC_DIE, "malformed ratelimit data: %s", s);
 
 
 /*************************************************
+*       Drop privs for checking TLS config      *
+*************************************************/
+
+/* We want to validate TLS options during readconf, but do not want to be
+root when we call into the TLS library, in case of library linkage errors
+which cause segfaults; before this check, those were always done as the Exim
+runtime user and it makes sense to continue with that.
+
+Assumes:  tls_require_ciphers has been set, if it will be
+          exim_user has been set, if it will be
+          exim_group has been set, if it will be
+
+Returns:  bool for "okay"; false will cause caller to immediately exit.
+*/
+
+#ifdef SUPPORT_TLS
+static BOOL
+tls_dropprivs_validate_require_cipher(void)
+{
+const uschar *errmsg;
+pid_t pid;
+int rc, status;
+void (*oldsignal)(int);
+
+oldsignal = signal(SIGCHLD, SIG_DFL);
+
+fflush(NULL);
+if ((pid = fork()) < 0)
+  log_write(0, LOG_MAIN|LOG_PANIC_DIE, "fork failed for TLS check");
+
+if (pid == 0)
+  {
+  exim_setugid(exim_uid, exim_gid, FALSE,
+      US"calling tls_validate_require_cipher");
+
+  errmsg = tls_validate_require_cipher();
+  if (errmsg)
+    {
+    log_write(0, LOG_PANIC_DIE|LOG_CONFIG,
+        "tls_require_ciphers invalid: %s", errmsg);
+    }
+  fflush(NULL);
+  _exit(0);
+  }
+
+do {
+  rc = waitpid(pid, &status, 0);
+} while (rc < 0 && errno == EINTR);
+
+DEBUG(D_all)
+  debug_printf("tls_validate_require_cipher child %d ended: status=0x%x\n",
+      (int)pid, status);
+
+signal(SIGCHLD, oldsignal);
+
+return status == 0;
+}
+#endif /* SUPPORT_TLS */
+
+
+
+
+/*************************************************
 *         Read main configuration options        *
 *************************************************/
 
@@ -3221,6 +3285,18 @@ if ((tls_verify_hosts != NULL || tls_try_verify_hosts != NULL) &&
     "tls_%sverify_hosts is set, but tls_verify_certificates is not set",
     (tls_verify_hosts != NULL)? "" : "try_");
 
+/* This also checks that the library linkage is working and we can call
+routines in it, so call even if tls_require_ciphers is unset */
+if (!tls_dropprivs_validate_require_cipher())
+  exit(1);
+
+/* Magic number: at time of writing, 1024 has been the long-standing value
+used by so many clients, and what Exim used to use always, that it makes
+sense to just min-clamp this max-clamp at that. */
+if (tls_dh_max_bits < 1024)
+  log_write(0, LOG_PANIC_DIE|LOG_CONFIG,
+      "tls_dh_max_bits is too small, must be at least 1024 for interop");
+
 /* If openssl_options is set, validate it */
 if (openssl_options != NULL)
   {
diff --git a/src/src/tls-gnu.c b/src/src/tls-gnu.c
index 9d121f96f..3ea02bd52 100644
--- a/src/src/tls-gnu.c
+++ b/src/src/tls-gnu.c
@@ -395,6 +395,15 @@ DEBUG(D_tls)
       dh_bits);
 #endif
 
+/* Some clients have hard-coded limits. */
+if (dh_bits > tls_dh_max_bits)
+  {
+  DEBUG(D_tls)
+    debug_printf("tls_dh_max_bits clamping override, using %d bits instead.\n",
+        tls_dh_max_bits);
+  dh_bits = tls_dh_max_bits;
+  }
+
 if (!string_format(filename, sizeof(filename),
       "%s/gnutls-params-%d", spool_directory, dh_bits))
   return tls_error(US"overlong filename", NULL, NULL);
@@ -1830,6 +1839,64 @@ vaguely_random_number(int max)
 
 
 /*************************************************
+*  Let tls_require_ciphers be checked at startup *
+*************************************************/
+
+/* The tls_require_ciphers option, if set, must be something which the
+library can parse.
+
+Returns:     NULL on success, or error message
+*/
+
+uschar *
+tls_validate_require_cipher(void)
+{
+int rc;
+uschar *expciphers = NULL;
+gnutls_priority_t priority_cache;
+const char *errpos;
+
+#define validate_check_rc(Label) do { \
+  if (rc != GNUTLS_E_SUCCESS) { if (exim_gnutls_base_init_done) gnutls_global_deinit(); \
+  return string_sprintf("%s failed: %s", (Label), gnutls_strerror(rc)); } } while (0)
+#define return_deinit(Label) do { gnutls_global_deinit(); return (Label); } while (0)
+
+if (exim_gnutls_base_init_done)
+  log_write(0, LOG_MAIN|LOG_PANIC,
+      "already initialised GnuTLS, Exim developer bug");
+
+rc = gnutls_global_init();
+validate_check_rc(US"gnutls_global_init()");
+exim_gnutls_base_init_done = TRUE;
+
+if (!(tls_require_ciphers && *tls_require_ciphers))
+  return_deinit(NULL);
+
+if (!expand_check(tls_require_ciphers, US"tls_require_ciphers", &expciphers))
+  return_deinit(US"failed to expand tls_require_ciphers");
+
+if (!(expciphers && *expciphers))
+  return_deinit(NULL);
+
+DEBUG(D_tls)
+  debug_printf("tls_require_ciphers expands to \"%s\"\n", expciphers);
+
+rc = gnutls_priority_init(&priority_cache, CS expciphers, &errpos);
+validate_check_rc(string_sprintf(
+      "gnutls_priority_init(%s) failed at offset %ld, \"%.8s..\"",
+      expciphers, errpos - CS expciphers, errpos));
+
+#undef return_deinit
+#undef validate_check_rc
+gnutls_global_deinit();
+
+return NULL;
+}
+
+
+
+
+/*************************************************
 *         Report the library versions.           *
 *************************************************/
 
diff --git a/src/src/tls-openssl.c b/src/src/tls-openssl.c
index de9c659a6..eeab9c130 100644
--- a/src/src/tls-openssl.c
+++ b/src/src/tls-openssl.c
@@ -308,10 +308,19 @@ else
     }
   else
     {
-    SSL_CTX_set_tmp_dh(ctx, dh);
-    DEBUG(D_tls)
-      debug_printf("Diffie-Hellman initialized from %s with %d-bit key\n",
-        dhexpanded, 8*DH_size(dh));
+    if ((8*DH_size(dh)) > tls_dh_max_bits)
+      {
+      DEBUG(D_tls)
+        debug_printf("dhparams file %d bits, is > tls_dh_max_bits limit of %d",
+            8*DH_size(dh), tls_dh_max_bits);
+      }
+    else
+      {
+      SSL_CTX_set_tmp_dh(ctx, dh);
+      DEBUG(D_tls)
+        debug_printf("Diffie-Hellman initialized from %s with %d-bit key\n",
+          dhexpanded, 8*DH_size(dh));
+      }
     DH_free(dh);
     }
   BIO_free(bio);
@@ -1498,6 +1507,72 @@ tls_active = -1;
 
 
 /*************************************************
+*  Let tls_require_ciphers be checked at startup *
+*************************************************/
+
+/* The tls_require_ciphers option, if set, must be something which the
+library can parse.
+
+Returns:     NULL on success, or error message
+*/
+
+uschar *
+tls_validate_require_cipher(void)
+{
+SSL_CTX *ctx;
+uschar *s, *expciphers, *err;
+
+/* this duplicates from tls_init(), we need a better "init just global
+state, for no specific purpose" singleton function of our own */
+
+SSL_load_error_strings();
+OpenSSL_add_ssl_algorithms();
+#if (OPENSSL_VERSION_NUMBER >= 0x0090800fL) && !defined(OPENSSL_NO_SHA256)
+/* SHA256 is becoming ever more popular. This makes sure it gets added to the
+list of available digests. */
+EVP_add_digest(EVP_sha256());
+#endif
+
+if (!(tls_require_ciphers && *tls_require_ciphers))
+  return NULL;
+
+if (!expand_check(tls_require_ciphers, US"tls_require_ciphers", &expciphers))
+  return US"failed to expand tls_require_ciphers";
+
+if (!(expciphers && *expciphers))
+  return NULL;
+
+/* normalisation ripped from above */
+s = expciphers;
+while (*s != 0) { if (*s == '_') *s = '-'; s++; }
+
+err = NULL;
+
+ctx = SSL_CTX_new(SSLv23_server_method());
+if (!ctx)
+  {
+  ERR_error_string(ERR_get_error(), ssl_errstring);
+  return string_sprintf("SSL_CTX_new() failed: %s", ssl_errstring);
+  }
+
+DEBUG(D_tls)
+  debug_printf("tls_require_ciphers expands to \"%s\"\n", expciphers);
+
+if (!SSL_CTX_set_cipher_list(ctx, CS expciphers))
+  {
+  ERR_error_string(ERR_get_error(), ssl_errstring);
+  err = string_sprintf("SSL_CTX_set_cipher_list(%s) failed", expciphers);
+  }
+
+SSL_CTX_free(ctx);
+
+return err;
+}
+
+
+
+
+/*************************************************
 *         Report the library versions.           *
 *************************************************/