From 69a821ea1c9357acdd5aa1c9e23fd030b01cb5a9 Mon Sep 17 00:00:00 2001 From: George Kadianakis Date: Wed, 26 Oct 2011 02:05:45 +0200 Subject: Refactor the SSL_set_info_callback() callbacks. Introduce tor_tls_state_changed_callback(), which handles every SSL state change. The new function tor_tls_got_server_hello() is called every time we send a ServerHello during a v2 handshake, and plays the role of the previous tor_tls_server_info_callback() function. --- src/common/tortls.c | 97 ++++++++++++++++++++++++----------------------------- 1 file changed, 44 insertions(+), 53 deletions(-) (limited to 'src/common/tortls.c') diff --git a/src/common/tortls.c b/src/common/tortls.c index 9a3c02b5b3..6757cfa780 100644 --- a/src/common/tortls.c +++ b/src/common/tortls.c @@ -1280,55 +1280,29 @@ 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. */ static void -tor_tls_debug_state_callback(const SSL *ssl, int type, int val) +tor_tls_got_server_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); -} - -/** Invoked when we're accepting a connection on ssl, and the connection - * changes state. We use this: - * - */ -static void -tor_tls_server_info_callback(const SSL *ssl, int type, int val) -{ - tor_tls_t *tls; - (void) val; - - tor_tls_debug_state_callback(ssl, type, val); - - if (type != SSL_CB_ACCEPT_LOOP) - return; - if (ssl->state != SSL3_ST_SW_SRVR_HELLO_A) - return; - - 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; - } + /* 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; /* 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; @@ -1343,6 +1317,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_server_hello() for every SSL ServerHello we + send. */ + + 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_server_hello(tls); + } +#endif + +} + /** Replace *ciphers 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 @@ -1484,14 +1487,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"); @@ -1521,13 +1518,7 @@ tor_tls_set_renegotiate_callback(tor_tls_t *tls, tls->negotiated_callback = cb; 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 -- cgit v1.2.3-54-g00ecf From 4fd79f9def28996552b5739792f428c2514de1f6 Mon Sep 17 00:00:00 2001 From: George Kadianakis Date: Wed, 26 Oct 2011 03:09:22 +0200 Subject: Detect renegotiation when it actually happens. The renegotiation callback was called only when the first Application Data arrived, instead of when the renegotiation took place. This happened because SSL_read() returns -1 and sets the error to SSL_ERROR_WANT_READ when a renegotiation happens instead of reading data [0]. I also added a commented out aggressive assert that I won't enable yet because I don't feel I understand SSL_ERROR_WANT_READ enough. [0]: Look at documentation of SSL_read(), SSL_get_error() and SSL_CTX_set_mode() (SSL_MODE_AUTO_RETRY section). --- src/common/tortls.c | 29 +++++++++++++++++++---------- 1 file changed, 19 insertions(+), 10 deletions(-) (limited to 'src/common/tortls.c') diff --git a/src/common/tortls.c b/src/common/tortls.c index 6757cfa780..79b6d2c3c9 100644 --- a/src/common/tortls.c +++ b/src/common/tortls.c @@ -1606,19 +1606,28 @@ tor_tls_read(tor_tls_t *tls, char *cp, size_t len) tor_assert(tls->state == TOR_TLS_ST_OPEN); tor_assert(lenssl, 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; - } -#endif + if (tls->got_renegotiate) { + tor_assert(tls->server_handshake_count == 2); + /* XXX tor_assert(err == TOR_TLS_WANTREAD); */ + + /* 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; -- cgit v1.2.3-54-g00ecf From ecd239e3b577705e0669d47293a2e755cf93cec0 Mon Sep 17 00:00:00 2001 From: George Kadianakis Date: Wed, 26 Oct 2011 03:12:18 +0200 Subject: Detect and deny excess renegotiations attempts. Switch 'server_handshake_count' from a uint8_t to 2 unsigned int bits. Since we won't ever be doing more than 3 handshakes, we don't need the extra space. Toggle tor_tls_t.got_renegotiate based on the server_handshake_count. Also assert that when we've done two handshakes as a server (the initial SSL handshake, and the renegotiation handshake) we've just renegotiated. Finally, in tor_tls_read() return an error if we see more than 2 handshakes. --- src/common/tortls.c | 20 +++++++++++++++----- 1 file changed, 15 insertions(+), 5 deletions(-) (limited to 'src/common/tortls.c') diff --git a/src/common/tortls.c b/src/common/tortls.c index 79b6d2c3c9..72697850bd 100644 --- a/src/common/tortls.c +++ b/src/common/tortls.c @@ -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 @@ -1286,12 +1286,14 @@ tor_tls_client_is_using_v2_ciphers(const SSL *ssl, const char *address) static void tor_tls_got_server_hello(tor_tls_t *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*/ + if (tls->server_handshake_count < 3) ++tls->server_handshake_count; + if (tls->server_handshake_count == 2) { + tor_assert(tls->negotiated_callback); + tls->got_renegotiate = 1; + } + /* Now check the cipher list. */ if (tor_tls_client_is_using_v2_ciphers(tls->ssl, ADDR(tls))) { /*XXXX_TLS keep this from happening more than once! */ @@ -1625,6 +1627,14 @@ tor_tls_read(tor_tls_t *tls, char *cp, size_t len) tls->got_renegotiate = 0; return r; + } else if (tls->server_handshake_count > 2) { + /* If we get more than 2 handshakes, it means that our peer is + trying to re-renegotiate. Return an error. */ + tor_assert(tls->server_handshake_count == 3); + + log_info(LD_NET, "Detected excess renegotiation from %s!", ADDR(tls)); + + return TOR_TLS_ERROR_MISC; } #endif -- cgit v1.2.3-54-g00ecf From 340809dd224b244675496e301d3ba154a6fe68d0 Mon Sep 17 00:00:00 2001 From: George Kadianakis Date: Wed, 26 Oct 2011 13:16:14 +0200 Subject: Get rid of tor_tls_block_renegotiation(). Since we check for naughty renegotiations using tor_tls_t.server_handshake_count we don't need that semi-broken function (at least till there is a way to disable rfc5746 renegotiations too). --- src/common/tortls.c | 10 ---------- src/common/tortls.h | 1 - src/or/connection_or.c | 5 ----- 3 files changed, 16 deletions(-) (limited to 'src/common/tortls.c') diff --git a/src/common/tortls.c b/src/common/tortls.c index 72697850bd..1150c42327 100644 --- a/src/common/tortls.c +++ b/src/common/tortls.c @@ -1540,16 +1540,6 @@ tor_tls_unblock_renegotiation(tor_tls_t *tls) } } -/** If this version of openssl supports it, turn off renegotiation on - * tls. (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) { diff --git a/src/common/tortls.h b/src/common/tortls.h index 90e76e4a95..1219b80816 100644 --- a/src/common/tortls.h +++ b/src/common/tortls.h @@ -75,7 +75,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); diff --git a/src/or/connection_or.c b/src/or/connection_or.c index bcae075c56..6c56a61e54 100644 --- a/src/or/connection_or.c +++ b/src/or/connection_or.c @@ -1139,10 +1139,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. */ @@ -1529,7 +1525,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; -- cgit v1.2.3-54-g00ecf From e2b3527106e0747f652e2f28fa087d9874e0e2ce Mon Sep 17 00:00:00 2001 From: George Kadianakis Date: Wed, 26 Oct 2011 13:36:30 +0200 Subject: Also handle needless renegotiations in SSL_write(). SSL_read(), SSL_write() and SSL_do_handshake() can always progress the SSL protocol instead of their normal operation, this means that we must be checking for needless renegotiations after they return. Introduce tor_tls_got_excess_renegotiations() which makes the tls->server_handshake_count > 2 check for us, and use it in tor_tls_read() and tor_tls_write(). Cases that should not be handled: * SSL_do_handshake() is only called by tor_tls_renegotiate() which is a client-only function. * The SSL_read() in tor_tls_shutdown() does not need to be handled, since SSL_shutdown() will be called if SSL_read() returns an error. --- src/common/tortls.c | 32 ++++++++++++++++++++++++-------- 1 file changed, 24 insertions(+), 8 deletions(-) (limited to 'src/common/tortls.c') diff --git a/src/common/tortls.c b/src/common/tortls.c index 1150c42327..4235de70fd 100644 --- a/src/common/tortls.c +++ b/src/common/tortls.c @@ -1228,6 +1228,17 @@ tor_tls_context_new(crypto_pk_env_t *identity, unsigned int key_lifetime) return NULL; } +/** Return true if the tls object has completed more + * renegotiations than needed for the Tor protocol. */ +static INLINE int +tor_tls_got_excess_renegotiations(tor_tls_t *tls) +{ + /** The Tor v2 server handshake needs a single renegotiation after + the initial SSL handshake. This means that if we ever see more + than 2 handshakes, we raise the flag. */ + return (tls->server_handshake_count > 2) ? 1 : 0; +} + #ifdef V2_HANDSHAKE_SERVER /** Return true iff the cipher list suggested by the client for ssl is * a list that indicates that the client knows how to do the v2 TLS connection @@ -1605,6 +1616,12 @@ tor_tls_read(tor_tls_t *tls, char *cp, size_t len) err = tor_tls_get_error(tls, r, CATCH_ZERO, "reading", LOG_DEBUG, LD_NET); + if (tor_tls_got_excess_renegotiations(tls)) { + log_info(LD_NET, "Detected excess renegotiation from %s!", ADDR(tls)); + + return TOR_TLS_ERROR_MISC; + } + #ifdef V2_HANDSHAKE_SERVER if (tls->got_renegotiate) { tor_assert(tls->server_handshake_count == 2); @@ -1617,14 +1634,6 @@ tor_tls_read(tor_tls_t *tls, char *cp, size_t len) tls->got_renegotiate = 0; return r; - } else if (tls->server_handshake_count > 2) { - /* If we get more than 2 handshakes, it means that our peer is - trying to re-renegotiate. Return an error. */ - tor_assert(tls->server_handshake_count == 3); - - log_info(LD_NET, "Detected excess renegotiation from %s!", ADDR(tls)); - - return TOR_TLS_ERROR_MISC; } #endif @@ -1664,6 +1673,13 @@ 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 (tor_tls_got_excess_renegotiations(tls)) { + log_info(LD_NET, "Detected excess renegotiation from %s!", ADDR(tls)); + + return TOR_TLS_ERROR_MISC; + } + if (err == TOR_TLS_DONE) { return r; } -- cgit v1.2.3-54-g00ecf From e097bffaed72af6b19f7293722021196bb94de1e Mon Sep 17 00:00:00 2001 From: George Kadianakis Date: Thu, 3 Nov 2011 22:33:50 +0100 Subject: 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. --- changes/bug4312 | 11 +++++++++++ src/common/tortls.c | 26 ++++++++++++++++---------- 2 files changed, 27 insertions(+), 10 deletions(-) create mode 100644 changes/bug4312 (limited to 'src/common/tortls.c') diff --git a/changes/bug4312 b/changes/bug4312 new file mode 100644 index 0000000000..f8647d3c76 --- /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 4235de70fd..111a7f6243 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)); -- cgit v1.2.3-54-g00ecf From 406ae1ba5ad529a4d0e710229dab6ed645d42b50 Mon Sep 17 00:00:00 2001 From: George Kadianakis Date: Sun, 13 Nov 2011 14:47:11 +0100 Subject: Use callback-driven approach to block renegotiations. Also use this new approach in the bufferevents-enabled case. --- src/common/compat_libevent.c | 9 +++++++ src/common/compat_libevent.h | 3 +++ src/common/tortls.c | 57 ++++++++++++++++++++++---------------------- src/common/tortls.h | 4 +++- src/or/connection_or.c | 20 ++++++++++++++-- 5 files changed, 62 insertions(+), 31 deletions(-) (limited to 'src/common/tortls.c') diff --git a/src/common/compat_libevent.c b/src/common/compat_libevent.c index 3201738701..ea7f5d1bbf 100644 --- a/src/common/compat_libevent.c +++ b/src/common/compat_libevent.c @@ -518,6 +518,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 diff --git a/src/common/compat_libevent.h b/src/common/compat_libevent.h index 0247297177..897eddbbe0 100644 --- a/src/common/compat_libevent.h +++ b/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), diff --git a/src/common/tortls.c b/src/common/tortls.c index 111a7f6243..94aa4aeb3a 100644 --- a/src/common/tortls.c +++ b/src/common/tortls.c @@ -52,7 +52,6 @@ #include #include #include -#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 +#include "compat_libevent.h" /* Enable the "v2" TLS handshake. */ @@ -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; }; @@ -1228,17 +1233,6 @@ tor_tls_context_new(crypto_pk_env_t *identity, unsigned int key_lifetime) return NULL; } -/** Return true if the tls object has completed more - * renegotiations than needed for the Tor protocol. */ -static INLINE int -tor_tls_got_excess_renegotiations(tor_tls_t *tls) -{ - /** The Tor v2 server handshake needs a single renegotiation after - the initial SSL handshake. This means that if we ever see more - than 2 handshakes, we raise the flag. */ - return (tls->server_handshake_count > 2) ? 1 : 0; -} - #ifdef V2_HANDSHAKE_SERVER /** Return true iff the cipher list suggested by the client for ssl is * a list that indicates that the client knows how to do the v2 TLS connection @@ -1307,6 +1301,20 @@ tor_tls_got_client_hello(tor_tls_t *tls) } 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. */ + + struct timeval zero_seconds_timer = {0,0}; + + 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. */ @@ -1523,16 +1531,20 @@ tor_tls_set_logged_address(tor_tls_t *tls, const char *address) tls->address = tor_strdup(address); } -/** Set cb to be called with argument arg whenever tls - * next gets a client-side renegotiate in the middle of a read. Do not - * invoke this function until after initial handshaking is done! +/** Set cb to be called with argument arg whenever + * tls next gets a client-side renegotiate in the middle of a + * read. Set cb2 to be called with argument arg whenever + * tls gets excess renegotiation requests. Do not invoke this + * function until after 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; SSL_set_info_callback(tls->ssl, tor_tls_state_changed_callback); @@ -1592,6 +1604,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); @@ -1620,12 +1633,6 @@ tor_tls_read(tor_tls_t *tls, char *cp, size_t len) err = tor_tls_get_error(tls, r, CATCH_ZERO, "reading", LOG_DEBUG, LD_NET); - if (tor_tls_got_excess_renegotiations(tls)) { - log_info(LD_NET, "Detected excess renegotiation from %s!", ADDR(tls)); - - return TOR_TLS_ERROR_MISC; - } - #ifdef V2_HANDSHAKE_SERVER if (tls->got_renegotiate) { if (tls->server_handshake_count != 2) { @@ -1680,12 +1687,6 @@ 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 (tor_tls_got_excess_renegotiations(tls)) { - log_info(LD_NET, "Detected excess renegotiation from %s!", ADDR(tls)); - - return TOR_TLS_ERROR_MISC; - } - if (err == TOR_TLS_DONE) { return r; } diff --git a/src/common/tortls.h b/src/common/tortls.h index 1219b80816..f032acd5d7 100644 --- a/src/common/tortls.h +++ b/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); diff --git a/src/or/connection_or.c b/src/or/connection_or.c index 6c56a61e54..0b39ad5f03 100644 --- a/src/or/connection_or.c +++ b/src/or/connection_or.c @@ -1146,6 +1146,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 * conn to connection_tls_finish_handshake(). * @@ -1192,8 +1206,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)); @@ -1255,8 +1270,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; */ -- cgit v1.2.3-54-g00ecf