[tor-commits] [tor/release-0.3.4] Detect openssl bug 7712 and work around it.

[nickm at torproject.org]

commit bf413829cb30e5b5ac0cf9273b5b57a50e257141
Author: Nick Mathewson <nickm at torproject.org>
Date:   Wed Jan 2 14:23:42 2019 -0500

    Detect openssl bug 7712 and work around it.
    
    In theory it would be better to detect this bug in advance, but this
    approach is much simpler, and therefore safer to backport.
    
    This closes tor issue 28973.
---
 changes/ticket28973    |  6 ++++++
 src/common/tortls.c    | 46 +++++++++++++++++++++++++++++++++++++++++++++-
 src/or/connection_or.c | 17 ++++++++++-------
 3 files changed, 61 insertions(+), 8 deletions(-)

diff --git a/changes/ticket28973 b/changes/ticket28973
new file mode 100644
index 000000000..b1d208ee5
--- /dev/null
+++ b/changes/ticket28973
@@ -0,0 +1,6 @@
+  o Minor features (OpenSSL bug workaround):
+    - Work around a bug in OpenSSL 1.1.1a, which prevented the TLS 1.3
+      key export function from handling long labels.  When this bug
+      is detected, Tor will disable TLS 1.3.  We recommend upgrading to
+      a version of OpenSSL without this bug when it becomes available.
+      Closes ticket 28973.
diff --git a/src/common/tortls.c b/src/common/tortls.c
index ceb614723..5f1930d88 100644
--- a/src/common/tortls.c
+++ b/src/common/tortls.c
@@ -89,6 +89,9 @@ ENABLE_GCC_WARNING(redundant-decls)
 #define SSL3_FLAGS_ALLOW_UNSAFE_LEGACY_RENEGOTIATION 0x0010
 #endif
 
+/** Set to true iff openssl bug 7712 has been detected. */
+static int openssl_bug_7712_is_present = 0;
+
 /** Return values for tor_tls_classify_client_ciphers.
  *
  * @{
@@ -1701,6 +1704,13 @@ tor_tls_new(int sock, int isServer)
   }
 #endif /* defined(SSL_set_tlsext_host_name) */
 
+#ifdef SSL_CTRL_SET_MAX_PROTO_VERSION
+  if (openssl_bug_7712_is_present) {
+    /* We can't actually use TLS 1.3 until this bug is fixed. */
+    SSL_set_max_proto_version(result->ssl, TLS1_2_VERSION);
+  }
+#endif
+
   if (!SSL_set_cipher_list(result->ssl,
                      isServer ? SERVER_CIPHER_LIST : CLIENT_CIPHER_LIST)) {
     tls_log_errors(NULL, LOG_WARN, LD_NET, "setting ciphers");
@@ -2586,7 +2596,8 @@ tor_tls_get_tlssecrets,(tor_tls_t *tls, uint8_t *secrets_out))
  * provided <b>context</b> (<b>context_len</b> bytes long) and
  * <b>label</b> (a NUL-terminated string), compute a 32-byte secret in
  * <b>secrets_out</b> that only the parties to this TLS session can
- * compute.  Return 0 on success and -1 on failure.
+ * compute.  Return 0 on success; -1 on failure; and -2 on failure
+ * caused by OpenSSL bug 7712.
  */
 MOCK_IMPL(int,
 tor_tls_export_key_material,(tor_tls_t *tls, uint8_t *secrets_out,
@@ -2601,6 +2612,39 @@ tor_tls_export_key_material,(tor_tls_t *tls, uint8_t *secrets_out,
                                      secrets_out, DIGEST256_LEN,
                                      label, strlen(label),
                                      context, context_len, 1);
+
+  if (r != 1) {
+    int severity = openssl_bug_7712_is_present ? LOG_WARN : LOG_DEBUG;
+    tls_log_errors(tls, severity, LD_NET, "exporting keying material");
+  }
+
+#ifdef TLS1_3_VERSION
+  if (r != 1 &&
+      strlen(label) > 12 &&
+      SSL_version(tls->ssl) >= TLS1_3_VERSION) {
+
+    if (! openssl_bug_7712_is_present) {
+      /* We might have run into OpenSSL issue 7712, which caused OpenSSL
+       * 1.1.1a to not handle long labels.  Let's test to see if we have.
+       */
+      r = SSL_export_keying_material(tls->ssl, secrets_out, DIGEST256_LEN,
+                                     "short", 5, context, context_len, 1);
+      if (r == 1) {
+        /* A short label succeeds, but a long label fails. This was openssl
+         * issue 7712. */
+        openssl_bug_7712_is_present = 1;
+        log_warn(LD_GENERAL, "Detected OpenSSL bug 7712: disabling TLS 1.3 on "
+                 "future connections. A fix is expected to appear in OpenSSL "
+                 "1.1.1b.");
+      }
+    }
+    if (openssl_bug_7712_is_present)
+      return -2;
+    else
+      return -1;
+  }
+#endif
+
   return (r == 1) ? 0 : -1;
 }
 
diff --git a/src/or/connection_or.c b/src/or/connection_or.c
index 221b8dc8a..c2ca7078e 100644
--- a/src/or/connection_or.c
+++ b/src/or/connection_or.c
@@ -2843,9 +2843,15 @@ connection_or_compute_authenticate_cell_body(or_connection_t *conn,
     char label[128];
     tor_snprintf(label, sizeof(label),
                  "EXPORTER FOR TOR TLS CLIENT BINDING %s", authtype_str);
-    tor_tls_export_key_material(conn->tls, auth->tlssecrets,
-                                auth->cid, sizeof(auth->cid),
-                                label);
+    int r = tor_tls_export_key_material(conn->tls, auth->tlssecrets,
+                                        auth->cid, sizeof(auth->cid),
+                                        label);
+    if (r < 0) {
+      if (r != -2)
+        log_warn(LD_BUG, "TLS key export failed for unknown reason.");
+      // If r == -2, this was openssl bug 7712.
+      goto err;
+    }
   }
 
   /* 8 octets were reserved for the current time, but we're trying to get out
@@ -2973,14 +2979,11 @@ connection_or_send_authenticate_cell,(or_connection_t *conn, int authtype))
                                                  get_current_auth_keypair(),
                                                  0 /* not server */);
   if (! cell) {
-    /* LCOV_EXCL_START */
-    log_warn(LD_BUG, "Unable to compute authenticate cell!");
+    log_fn(LOG_PROTOCOL_WARN, LD_NET, "Unable to compute authenticate cell!");
     return -1;
-    /* LCOV_EXCL_STOP */
   }
   connection_or_write_var_cell_to_buf(cell, conn);
   var_cell_free(cell);
 
   return 0;
 }
-