Fix issues pointed out by nickm.

- Rename tor_tls_got_server_hello() to tor_tls_got_client_hello().
- Replaced some aggressive asserts with LD_BUG logging.

  They were the innocent "I believe I understand how these callbacks
  work, and this assert proves it" type of callbacks, and not the "If
  this statement is not true, computer is exploding." type of
  callbacks.
- Added a changes file.

2 files changed, 27 insertions, 10 deletions

diff --git a/changes/bug4312 b/changes/bug4312
new file mode 100644
index 000000000..f8647d3c7
--- /dev/null
+++ b/changes/bug4312
@@ -0,0 +1,11 @@
+  o Security fixes:
+
+    - Block excess renegotiations even if they are RFC5746 compliant.
+      This mitigates potential SSL Denial of Service attacks that use
+      SSL renegotiation as a way of forcing the server to perform
+      unneeded computationally expensive SSL handshakes. Implements
+      #4312.
+
+    - Fix a bug where tor would not notice excess renegotiation
+      attempts before it received the first data SSL record. Fixes
+      part of #4312.
diff --git a/src/common/tortls.c b/src/common/tortls.c
index 4235de70f..111a7f624 100644
--- a/src/common/tortls.c
+++ b/src/common/tortls.c
@@ -1291,17 +1291,21 @@ tor_tls_client_is_using_v2_ciphers(const SSL *ssl, const char *address)
   return 1;
 }
 
-/** We sent the ServerHello part of an SSL handshake.  This might mean
- * that we completed a renegotiation and appropriate actions must be
- * taken. */
+/** We got an SSL ClientHello message.  This might mean that the
+ * client wants to initiate a renegotiation and appropriate actions
+ * must be taken. */
 static void
-tor_tls_got_server_hello(tor_tls_t *tls)
+tor_tls_got_client_hello(tor_tls_t *tls)
 {
   if (tls->server_handshake_count < 3)
     ++tls->server_handshake_count;
 
   if (tls->server_handshake_count == 2) {
-    tor_assert(tls->negotiated_callback);
+    if (!tls->negotiated_callback) {
+      log_warn(LD_BUG, "Got a renegotiation request but we don't"
+               " have a renegotiation callback set!");
+    }
+
     tls->got_renegotiate = 1;
   }
 
@@ -1344,8 +1348,8 @@ tor_tls_state_changed_callback(const SSL *ssl, int type, int val)
   if (type == SSL_CB_ACCEPT_LOOP &&
       ssl->state == SSL3_ST_SW_SRVR_HELLO_A) {
 
-    /* Call tor_tls_got_server_hello() for every SSL ServerHello we
-       send. */
+    /* Call tor_tls_got_client_hello() for every SSL ClientHello we
+       receive. */
 
     tor_tls_t *tls = tor_tls_get_by_ssl(ssl);
     if (!tls) {
@@ -1353,7 +1357,7 @@ tor_tls_state_changed_callback(const SSL *ssl, int type, int val)
       return;
     }
 
-    tor_tls_got_server_hello(tls);
+    tor_tls_got_client_hello(tls);
   }
 #endif
 
@@ -1624,8 +1628,10 @@ tor_tls_read(tor_tls_t *tls, char *cp, size_t len)
 
 #ifdef V2_HANDSHAKE_SERVER
   if (tls->got_renegotiate) {
-    tor_assert(tls->server_handshake_count == 2);
-    /* XXX tor_assert(err == TOR_TLS_WANTREAD); */
+    if (tls->server_handshake_count != 2) {
+      log_warn(LD_BUG, "We did not notice renegotiation in a timely fashion (%u)!",
+               tls->server_handshake_count);
+    }
 
     /* Renegotiation happened! */
     log_info(LD_NET, "Got a TLS renegotiation from %s", ADDR(tls));