From 3375e053c40dacf62a7eac02d52438a43398c053 Mon Sep 17 00:00:00 2001 From: Phil Pennock Date: Sun, 20 May 2012 21:49:40 -0400 Subject: 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. --- src/README.UPDATING | 24 +++++++++++++++ src/src/functions.h | 1 + src/src/globals.c | 4 +++ src/src/globals.h | 1 + src/src/readconf.c | 76 ++++++++++++++++++++++++++++++++++++++++++++++ src/src/tls-gnu.c | 67 +++++++++++++++++++++++++++++++++++++++++ src/src/tls-openssl.c | 83 ++++++++++++++++++++++++++++++++++++++++++++++++--- 7 files changed, 252 insertions(+), 4 deletions(-) (limited to 'src') 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 }, @@ -2771,6 +2772,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); @@ -1829,6 +1838,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); @@ -1497,6 +1506,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. * *************************************************/ -- cgit v1.2.3