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.

10 files changed, 293 insertions, 5 deletions

diff --git a/doc/doc-docbook/spec.xfpt b/doc/doc-docbook/spec.xfpt
index c4c181ef1..da97d4082 100644
--- a/doc/doc-docbook/spec.xfpt
+++ b/doc/doc-docbook/spec.xfpt
@@ -12694,6 +12694,7 @@ listed in more than one group.
 .row &%tls_advertise_hosts%&         "advertise TLS to these hosts"
 .row &%tls_certificate%&             "location of server certificate"
 .row &%tls_crl%&                     "certificate revocation list"
+.row &%tls_dh_max_bits%&             "clamp D-H bit count suggestion"
 .row &%tls_dhparam%&                 "DH parameters for server"
 .row &%tls_on_connect_ports%&        "specify SSMTP (SMTPS) ports"
 .row &%tls_privatekey%&              "location of server private key"
@@ -15680,6 +15681,25 @@ See &<<SECTtlssni>>& for discussion of when this option might be re-expanded.
 .wen
 
 
+.new
+.option tls_dh_max_bits main integer 2236
+.cindex "TLS" "D-H bit count"
+The number of bits used for Diffie-Hellman key-exchange may be suggested by
+the chosen TLS library.  That value might prove to be too high for
+interoperability.  This option provides a maximum clamp on the value
+suggested, trading off security for interoperability.
+
+The value must be at least 1024.
+
+The value 2236 was chosen because, at time of adding the option, it was the
+hard-coded maximum value supported by the NSS cryptographic library, as used
+by Thunderbird, while GnuTLS was suggesting 2432 bits as normal.
+
+If you prefer more security and are willing to break some clients, raise this
+number.
+.wen
+
+
 .option tls_dhparam main string&!! unset
 .cindex "TLS" "D-H parameters for server"
 The value of this option is expanded, and must then be the absolute path to
@@ -15687,6 +15707,11 @@ a file which contains the server's DH parameter values.
 This is used only for OpenSSL. When Exim is linked with GnuTLS, this option is
 ignored. See section &<<SECTopenvsgnu>>& for further details.
 
+.new
+If the DH bit-count from loading the file is greater than tls_dh_max_bits then
+it will be ignored.
+.end
+
 
 .option tls_on_connect_ports main "string list" unset
 This option specifies a list of incoming SSMTP (aka SMTPS) ports that should
@@ -24565,7 +24590,7 @@ made that any particular new authentication mechanism will be supported
 without code changes in Exim.
 
 
-.option server_channelbinding gsasl bool false
+.option server_channelbinding gsasl boolean false
 Some authentication mechanisms are able to use external context at both ends
 of the session to bind the authentication to that context, and fail the
 authentication process if that context differs.  Specifically, some TLS
@@ -24940,6 +24965,9 @@ name of a directory (for OpenSSL it can be either).
 The &%tls_dhparam%& option is ignored, because early versions of GnuTLS had no
 facility for varying its Diffie-Hellman parameters. I understand that this has
 changed, but Exim has not been updated to provide this facility.
+.new
+Instead, the GnuTLS support will use a file from the spool directory.
+.wen
 .next
 .vindex "&$tls_peerdn$&"
 Distinguished Name (DN) strings reported by the OpenSSL library use a slash for
diff --git a/doc/doc-txt/ChangeLog b/doc/doc-txt/ChangeLog
index 3d0f5c255..9db1c3823 100644
--- a/doc/doc-txt/ChangeLog
+++ b/doc/doc-txt/ChangeLog
@@ -137,6 +137,14 @@ PP/31 %D in printf continues to cause issues (-Wformat=security), so for
 PP/32 GnuTLS was always using default tls_require_ciphers, due to a missing
       assignment on my part.  Fixed.
 
+PP/33 Added tls_dh_max_bits option, defaulting to current hard-coded limit
+      of NSS, for GnuTLS/NSS interop.  Problem root cause diagnosis by
+      Janne Snabb (who went above and beyond: thank you).
+
+PP/34 Validate tls_require_ciphers on startup, since debugging an invalid
+      string otherwise requires a connection and a bunch more work and it's
+      relatively easy to get wrong.
+
 
 Exim version 4.77
 -----------------
diff --git a/doc/doc-txt/NewStuff b/doc/doc-txt/NewStuff
index 36b85d1ba..59994448f 100644
--- a/doc/doc-txt/NewStuff
+++ b/doc/doc-txt/NewStuff
@@ -96,6 +96,10 @@ Version 4.80
 
 14. New expansion variable $tod_epoch_l for higher-precision time.
 
+15. New global option tls_dh_max_bits, defaulting to current value of NSS
+    hard-coded limit of DH ephemeral bits, to fix interop problems caused by
+    GnuTLS 2.12 library recommending a bit count higher than NSS supports.
+
 
 Version 4.77
 ------------
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.           *
 *************************************************/