OpenSSL fixes and backwards compat break.

Drop SSL_clear() after SSL_new() which causes protocol negotiation failures for TLS1.0 vs TLS1.1/1.2 in OpenSSL 1.0.1b.

Remove SSL_OP_DONT_INSERT_EMPTY_FRAGMENTS (+dont_insert_empty_fragments) from default of openssl_options.

2 files changed, 31 insertions, 5 deletions

diff --git a/src/README.UPDATING b/src/README.UPDATING
index 3dff7c094..5b6bea869 100644
--- a/src/README.UPDATING
+++ b/src/README.UPDATING
@@ -47,6 +47,22 @@ Exim version 4.78
 
    "openssl_options" gains "no_tlsv1_1", "no_tlsv1_2" and "no_compression".
 
+   COMPATIBILITY WARNING: The default value of "openssl_options" is no longer
+   "+dont_insert_empty_fragments".  We default to unset.  That old default was
+   grandfathered in from before openssl_options became a configuration option.
+   Empty fragments are inserted by default through TLS1.0, to partially defend
+   against certain attacks; TLS1.1+ change the protocol so that this is not
+   needed.  The DIEF SSL option was required for some old releases of mail
+   clients which did not gracefully handle the empty fragments, and was
+   initially set in Exim release 4.31 (see ChangeLog, item 37).
+
+   If you still have affected mail-clients, and you see SSL protocol failures
+   with this release of Exim, set:
+     openssl_options = +dont_insert_empty_fragments
+   in the main section of your Exim configuration file.  You're trading off
+   security for compatibility.  Exim is now defaulting to higher security and
+   rewarding more modern clients.
+
  * 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
diff --git a/src/src/tls-openssl.c b/src/src/tls-openssl.c
index e2e150c0a..5e8c804e5 100644
--- a/src/src/tls-openssl.c
+++ b/src/src/tls-openssl.c
@@ -695,7 +695,19 @@ else if (verify_check_host(&tls_try_verify_hosts) == OK)
 /* Prepare for new connection */
 
 if ((ssl = SSL_new(ctx)) == NULL) return tls_error(US"SSL_new", NULL, NULL);
-SSL_clear(ssl);
+
+/* Warning: we used to SSL_clear(ssl) here, it was removed.
+ *
+ * With the SSL_clear(), we get strange interoperability bugs with
+ * OpenSSL 1.0.1b and TLS1.1/1.2.  It looks as though this may be a bug in
+ * OpenSSL itself, as a clear should not lead to inability to follow protocols.
+ *
+ * The SSL_clear() call is to let an existing SSL* be reused, typically after
+ * session shutdown.  In this case, we have a brand new object and there's no
+ * obvious reason to immediately clear it.  I'm guessing that this was
+ * originally added because of incomplete initialisation which the clear fixed,
+ * in some historic release.
+ */
 
 /* Set context and tell client to go ahead, except in the case of TLS startup
 on connection, where outputting anything now upsets the clients and tends to
@@ -1332,10 +1344,8 @@ uschar keep_c;
 BOOL adding, item_parsed;
 
 result = 0L;
-/* We grandfather in as default the one option which we used to set always. */
-#ifdef SSL_OP_DONT_INSERT_EMPTY_FRAGMENTS
-result |= SSL_OP_DONT_INSERT_EMPTY_FRAGMENTS;
-#endif
+/* Prior to 4.78 we or'd in SSL_OP_DONT_INSERT_EMPTY_FRAGMENTS; removed
+ * from default because it increases BEAST susceptibility. */
 
 if (option_spec == NULL)
   {