Merge remote-tracking branch 'asn/bug4312'

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.

src/common/compat_libevent.c

@@ -558,6 +558,15 @@ tor_check_libevent_header_compatibility(void)
 #endif
 }
 
+/** Wrapper around libevent's event_base_once(). Sets a
+ * timeout-triggered event with no associated file descriptor. */
+int
+tor_event_base_once(void (*cb)(evutil_socket_t, short, void *),
+                    void *arg, struct timeval *timer)
+{
+  return event_base_once(tor_libevent_get_base(), -1, EV_TIMEOUT, cb, arg, timer);
+}
+
 /*
   If possible, we're going to try to use Libevent's periodic timer support,
   since it does a pretty good job of making sure that periodic events get

src/common/compat_libevent.h

@@ -46,6 +46,9 @@ void tor_event_free(struct event *ev);
 
 typedef struct periodic_timer_t periodic_timer_t;
 
+int tor_event_base_once(void (*cb)(evutil_socket_t, short, void *),
+                        void *arg, struct timeval *timer);
+
 periodic_timer_t *periodic_timer_new(struct event_base *base,
              const struct timeval *tv,
              void (*cb)(periodic_timer_t *timer, void *data),

src/common/tortls.c

@@ -52,7 +52,6 @@
 #include <event2/bufferevent_ssl.h>
 #include <event2/buffer.h>
 #include <event2/event.h>
-#include "compat_libevent.h"
 #endif
 
 #define CRYPTO_PRIVATE /* to import prototypes from crypto.h */
@@ -64,6 +63,7 @@
 #include "torlog.h"
 #include "container.h"
 #include <string.h>
+#include "compat_libevent.h"
 
 /* Enable the "v2" TLS handshake.
  */
@@ -146,7 +146,7 @@ struct tor_tls_t {
   /** True iff we should call negotiated_callback when we're done reading. */
   unsigned int got_renegotiate:1;
   /** Incremented every time we start the server side of a handshake. */
-  uint8_t server_handshake_count;
+  unsigned int server_handshake_count:2;
   size_t wantwrite_n; /**< 0 normally, >0 if we returned wantwrite last
                        * time. */
   /** Last values retrieved from BIO_number_read()/write(); see
@@ -157,6 +157,11 @@ struct tor_tls_t {
   /** If set, a callback to invoke whenever the client tries to renegotiate
    * the handshake. */
   void (*negotiated_callback)(tor_tls_t *tls, void *arg);
+
+  /** Callback to invoke whenever a client tries to renegotiate more
+      than once. */
+  void (*excess_renegotiations_callback)(evutil_socket_t, short, void *);
+
   /** Argument to pass to negotiated_callback. */
   void *callback_arg;
 };
@@ -1297,55 +1302,49 @@ tor_tls_client_is_using_v2_ciphers(const SSL *ssl, const char *address)
   return 1;
 }
 
+/** 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_debug_state_callback(const SSL *ssl, int type, int val)
+tor_tls_got_client_hello(tor_tls_t *tls)
 {
-  log_debug(LD_HANDSHAKE, "SSL %p is now in state %s [type=%d,val=%d].",
-            ssl, ssl_state_to_string(ssl->state), type, val);
-}
+  if (tls->server_handshake_count < 3)
+    ++tls->server_handshake_count;
 
-/** Invoked when we're accepting a connection on <b>ssl</b>, and the connection
- * changes state. We use this:
- * <ul><li>To alter the state of the handshake partway through, so we
- *         do not send or request extra certificates in v2 handshakes.</li>
- * <li>To detect renegotiation</li></ul>
- */
-static void
-tor_tls_server_info_callback(const SSL *ssl, int type, int val)
-{
-  tor_tls_t *tls;
-  (void) val;
+  if (tls->server_handshake_count == 2) {
+    if (!tls->negotiated_callback) {
+      log_warn(LD_BUG, "Got a renegotiation request but we don't"
+               " have a renegotiation callback set!");
+    }
 
-  tor_tls_debug_state_callback(ssl, type, val);
+    tls->got_renegotiate = 1;
+  } else if (tls->server_handshake_count > 2) {
+    /* We got more than one renegotiation requests. The Tor protocol
+       needs just one renegotiation; more than that probably means
+       They are trying to DoS us and we have to stop them. We can't
+       close their connection from in here since it's an OpenSSL
+       callback, so we set a libevent timer that triggers in the next
+       event loop and closes the connection. */
 
-  if (type != SSL_CB_ACCEPT_LOOP)
-    return;
-  if (ssl->state != SSL3_ST_SW_SRVR_HELLO_A)
-    return;
+    struct timeval zero_seconds_timer = {0,0};
 
-  tls = tor_tls_get_by_ssl(ssl);
-  if (tls) {
-    /* Check whether we're watching for renegotiates.  If so, this is one! */
-    if (tls->negotiated_callback)
-      tls->got_renegotiate = 1;
-    if (tls->server_handshake_count < 127) /*avoid any overflow possibility*/
-      ++tls->server_handshake_count;
-  } else {
-    log_warn(LD_BUG, "Couldn't look up the tls for an SSL*. How odd!");
-    return;
+    if (tor_event_base_once(tls->excess_renegotiations_callback,
+                            tls->callback_arg, &zero_seconds_timer) < 0) {
+      log_warn(LD_GENERAL, "Didn't manage to set a renegotiation limiting callback.");
+    }
   }
 
   /* Now check the cipher list. */
-  if (tor_tls_client_is_using_v2_ciphers(ssl, ADDR(tls))) {
+  if (tor_tls_client_is_using_v2_ciphers(tls->ssl, ADDR(tls))) {
     /*XXXX_TLS keep this from happening more than once! */
 
     /* Yes, we're casting away the const from ssl.  This is very naughty of us.
      * Let's hope openssl doesn't notice! */
 
     /* Set SSL_MODE_NO_AUTO_CHAIN to keep from sending back any extra certs. */
-    SSL_set_mode((SSL*) ssl, SSL_MODE_NO_AUTO_CHAIN);
+    SSL_set_mode((SSL*) tls->ssl, SSL_MODE_NO_AUTO_CHAIN);
     /* Don't send a hello request. */
-    SSL_set_verify((SSL*) ssl, SSL_VERIFY_NONE, NULL);
+    SSL_set_verify((SSL*) tls->ssl, SSL_VERIFY_NONE, NULL);
 
     if (tls) {
       tls->wasV2Handshake = 1;
@@ -1360,6 +1359,35 @@ tor_tls_server_info_callback(const SSL *ssl, int type, int val)
 }
 #endif
 
+/** This is a callback function for SSL_set_info_callback() and it
+ *  will be called in every SSL state change.
+ *  It logs the SSL state change, and executes any actions that must be
+ *  taken.  */
+static void
+tor_tls_state_changed_callback(const SSL *ssl, int type, int val)
+{
+  log_debug(LD_HANDSHAKE, "SSL %p is now in state %s [type=%d,val=%d].",
+            ssl, ssl_state_to_string(ssl->state), type, val);
+
+#ifdef V2_HANDSHAKE_SERVER
+  if (type == SSL_CB_ACCEPT_LOOP &&
+      ssl->state == SSL3_ST_SW_SRVR_HELLO_A) {
+
+    /* Call tor_tls_got_client_hello() for every SSL ClientHello we
+       receive. */
+
+    tor_tls_t *tls = tor_tls_get_by_ssl(ssl);
+    if (!tls) {
+      log_warn(LD_BUG, "Couldn't look up the tls for an SSL*. How odd!");
+      return;
+    }
+
+    tor_tls_got_client_hello(tls);
+  }
+#endif
+
+}
+
 /** Replace *<b>ciphers</b> with a new list of SSL ciphersuites: specifically,
  * a list designed to mimic a common web browser.  Some of the ciphers in the
  * list won't actually be implemented by OpenSSL: that's okay so long as the
@@ -1501,14 +1529,8 @@ tor_tls_new(int sock, int isServer)
     log_warn(LD_NET, "Newly created BIO has read count %lu, write count %lu",
              result->last_read_count, result->last_write_count);
   }
-#ifdef V2_HANDSHAKE_SERVER
-  if (isServer) {
-    SSL_set_info_callback(result->ssl, tor_tls_server_info_callback);
-  } else
-#endif
-  {
-    SSL_set_info_callback(result->ssl, tor_tls_debug_state_callback);
-  }
+
+  SSL_set_info_callback(result->ssl, tor_tls_state_changed_callback);
 
   /* Not expected to get called. */
   tls_log_errors(NULL, LOG_WARN, LD_NET, "creating tor_tls_t object");
@@ -1526,25 +1548,23 @@ tor_tls_set_logged_address(tor_tls_t *tls, const char *address)
   tls->address = tor_strdup(address);
 }
 
-/** Set <b>cb</b> to be called with argument <b>arg</b> whenever <b>tls</b>
- * next gets a client-side renegotiate in the middle of a read.  Do not
- * invoke this function until <em>after</em> initial handshaking is done!
+/** Set <b>cb</b> to be called with argument <b>arg</b> whenever
+ * <b>tls</b> next gets a client-side renegotiate in the middle of a
+ * read. Set <b>cb2</b> to be called with argument <b>arg</b> whenever
+ * <b>tls</b> gets excess renegotiation requests.  Do not invoke this
+ * function until <em>after</em> initial handshaking is done!
  */
 void
-tor_tls_set_renegotiate_callback(tor_tls_t *tls,
+tor_tls_set_renegotiate_callbacks(tor_tls_t *tls,
                                  void (*cb)(tor_tls_t *, void *arg),
+                                 void (*cb2)(evutil_socket_t, short, void *),
                                  void *arg)
 {
   tls->negotiated_callback = cb;
+  tls->excess_renegotiations_callback = cb2;
   tls->callback_arg = arg;
   tls->got_renegotiate = 0;
-#ifdef V2_HANDSHAKE_SERVER
-  if (cb) {
-    SSL_set_info_callback(tls->ssl, tor_tls_server_info_callback);
-  } else {
-    SSL_set_info_callback(tls->ssl, tor_tls_debug_state_callback);
-  }
-#endif
+  SSL_set_info_callback(tls->ssl, tor_tls_state_changed_callback);
 }
 
 /** If this version of openssl requires it, turn on renegotiation on
@@ -1564,16 +1584,6 @@ tor_tls_unblock_renegotiation(tor_tls_t *tls)
   }
 }
 
-/** If this version of openssl supports it, turn off renegotiation on
- * <b>tls</b>.  (Our protocol never requires this for security, but it's nice
- * to use belt-and-suspenders here.)
- */
-void
-tor_tls_block_renegotiation(tor_tls_t *tls)
-{
-  tls->ssl->s3->flags &= ~SSL3_FLAGS_ALLOW_UNSAFE_LEGACY_RENEGOTIATION;
-}
-
 void
 tor_tls_assert_renegotiation_unblocked(tor_tls_t *tls)
 {
@@ -1611,6 +1621,7 @@ tor_tls_free(tor_tls_t *tls)
   SSL_free(tls->ssl);
   tls->ssl = NULL;
   tls->negotiated_callback = NULL;
+  tls->excess_renegotiations_callback = NULL;
   if (tls->context)
     tor_tls_context_decref(tls->context);
   tor_free(tls->address);
@@ -1632,19 +1643,30 @@ tor_tls_read(tor_tls_t *tls, char *cp, size_t len)
   tor_assert(tls->state == TOR_TLS_ST_OPEN);
   tor_assert(len<INT_MAX);
   r = SSL_read(tls->ssl, cp, (int)len);
-  if (r > 0) {
+  if (r > 0) /* return the number of characters read */
+    return r;
+
+  /* If we got here, SSL_read() did not go as expected. */
+
+  err = tor_tls_get_error(tls, r, CATCH_ZERO, "reading", LOG_DEBUG, LD_NET);
+
 #ifdef V2_HANDSHAKE_SERVER
-    if (tls->got_renegotiate) {
-      /* Renegotiation happened! */
-      log_info(LD_NET, "Got a TLS renegotiation from %s", ADDR(tls));
-      if (tls->negotiated_callback)
-        tls->negotiated_callback(tls, tls->callback_arg);
-      tls->got_renegotiate = 0;
+  if (tls->got_renegotiate) {
+    if (tls->server_handshake_count != 2) {
+      log_warn(LD_BUG, "We did not notice renegotiation in a timely fashion (%u)!",
+               tls->server_handshake_count);
     }
-#endif
+
+    /* Renegotiation happened! */
+    log_info(LD_NET, "Got a TLS renegotiation from %s", ADDR(tls));
+    if (tls->negotiated_callback)
+      tls->negotiated_callback(tls, tls->callback_arg);
+    tls->got_renegotiate = 0;
+
     return r;
   }
-  err = tor_tls_get_error(tls, r, CATCH_ZERO, "reading", LOG_DEBUG, LD_NET);
+#endif
+
   if (err == _TOR_TLS_ZERORETURN || err == TOR_TLS_CLOSE) {
     log_debug(LD_NET,"read returned r=%d; TLS is closed",r);
     tls->state = TOR_TLS_ST_CLOSED;
@@ -1681,6 +1703,7 @@ tor_tls_write(tor_tls_t *tls, const char *cp, size_t n)
   }
   r = SSL_write(tls->ssl, cp, (int)n);
   err = tor_tls_get_error(tls, r, 0, "writing", LOG_INFO, LD_NET);
+
   if (err == TOR_TLS_DONE) {
     return r;
   }

src/common/tortls.h

@@ -13,6 +13,7 @@
 
 #include "crypto.h"
 #include "compat.h"
+#include "compat_libevent.h"
 
 /* Opaque structure to hold a TLS connection. */
 typedef struct tor_tls_t tor_tls_t;
@@ -60,8 +61,9 @@ int tor_tls_context_init(int is_public_server,
                          unsigned int key_lifetime);
 tor_tls_t *tor_tls_new(int sock, int is_server);
 void tor_tls_set_logged_address(tor_tls_t *tls, const char *address);
-void tor_tls_set_renegotiate_callback(tor_tls_t *tls,
+void tor_tls_set_renegotiate_callbacks(tor_tls_t *tls,
                                       void (*cb)(tor_tls_t *, void *arg),
+                                      void (*cb2)(evutil_socket_t, short, void *),
                                       void *arg);
 int tor_tls_is_server(tor_tls_t *tls);
 void tor_tls_free(tor_tls_t *tls);
@@ -77,7 +79,6 @@ int tor_tls_handshake(tor_tls_t *tls);
 int tor_tls_finish_handshake(tor_tls_t *tls);
 int tor_tls_renegotiate(tor_tls_t *tls);
 void tor_tls_unblock_renegotiation(tor_tls_t *tls);
-void tor_tls_block_renegotiation(tor_tls_t *tls);
 void tor_tls_assert_renegotiation_unblocked(tor_tls_t *tls);
 int tor_tls_shutdown(tor_tls_t *tls);
 int tor_tls_get_pending_bytes(tor_tls_t *tls);

src/or/connection_or.c

@@ -1146,10 +1146,6 @@ connection_or_tls_renegotiated_cb(tor_tls_t *tls, void *_conn)
   or_connection_t *conn = _conn;
   (void)tls;
 
-  /* Don't invoke this again. */
-  tor_tls_set_renegotiate_callback(tls, NULL, NULL);
-  tor_tls_block_renegotiation(tls);
-
   if (connection_tls_finish_handshake(conn) < 0) {
     /* XXXX_TLS double-check that it's ok to do this from inside read. */
     /* XXXX_TLS double-check that this verifies certificates. */
@@ -1157,6 +1153,20 @@ connection_or_tls_renegotiated_cb(tor_tls_t *tls, void *_conn)
   }
 }
 
+/** Invoked on the server side using a timer from inside
+ * tor_tls_got_client_hello() when the server receives excess
+ * renegotiation attempts; probably indicating a DoS. */
+static void
+connection_or_close_connection_cb(evutil_socket_t fd, short what, void *_conn)
+{
+  or_connection_t *conn = _conn;
+  (void) what;
+  (void) fd;
+
+  connection_stop_reading(TO_CONN(conn));
+  connection_mark_for_close(TO_CONN(conn));
+}
+
 /** Move forward with the tls handshake. If it finishes, hand
  * <b>conn</b> to connection_tls_finish_handshake().
  *
@@ -1203,8 +1213,9 @@ connection_tls_continue_handshake(or_connection_t *conn)
           /* v2/v3 handshake, but not a client. */
           log_debug(LD_OR, "Done with initial SSL handshake (server-side). "
                            "Expecting renegotiation or VERSIONS cell");
-          tor_tls_set_renegotiate_callback(conn->tls,
+          tor_tls_set_renegotiate_callbacks(conn->tls,
                                            connection_or_tls_renegotiated_cb,
+                                           connection_or_close_connection_cb,
                                            conn);
           conn->_base.state = OR_CONN_STATE_TLS_SERVER_RENEGOTIATING;
           connection_stop_writing(TO_CONN(conn));
@@ -1266,8 +1277,9 @@ connection_or_handle_event_cb(struct bufferevent *bufev, short event,
       } else if (tor_tls_get_num_server_handshakes(conn->tls) == 1) {
         /* v2 or v3 handshake, as a server. Only got one handshake, so
          * wait for the next one. */
-        tor_tls_set_renegotiate_callback(conn->tls,
+        tor_tls_set_renegotiate_callbacks(conn->tls,
                                          connection_or_tls_renegotiated_cb,
+                                         connection_or_close_connection_cb,
                                          conn);
         conn->_base.state = OR_CONN_STATE_TLS_SERVER_RENEGOTIATING;
         /* return 0; */
@@ -1536,7 +1548,6 @@ connection_tls_finish_handshake(or_connection_t *conn)
       connection_or_init_conn_from_address(conn, &conn->_base.addr,
                                            conn->_base.port, digest_rcvd, 0);
     }
-    tor_tls_block_renegotiation(conn->tls);
     return connection_or_set_state_open(conn);
   } else {
     conn->_base.state = OR_CONN_STATE_OR_HANDSHAKING_V2;