- tor-commits - lists.torproject.org

[tor/master] fold in changes files so far
by arma＠torproject.org 26 Nov '11

26 Nov '11

commit 58d1aa44023e8b458450c4c00d5828f768c0adb5 Author: Roger Dingledine <arma(a)torproject.org> Date: Fri Nov 25 19:24:42 2011 -0500 fold in changes files so far --- ChangeLog | 77 +++++++++++++++++++++++++++++++++++ changes/UserspaceIOCPBuffers | 7 --- changes/aes_hackery | 10 ----- changes/bug2474 | 5 -- changes/bug3472 | 7 --- changes/bug3953 | 6 --- changes/bug4230 | 5 -- changes/bug4312 | 11 ----- changes/bug4554 | 4 -- changes/bug4574 | 4 -- changes/checkSpaces | 5 -- changes/disable_debugger_attachment | 16 ------- changes/make_version | 4 -- changes/pure_removal | 7 --- 14 files changed, 77 insertions(+), 91 deletions(-) diff --git a/ChangeLog b/ChangeLog index 4b9d9f0..9a7d50a 100644 --- a/ChangeLog +++ b/ChangeLog @@ -1,3 +1,80 @@ +Changes in version 0.2.3.9-alpha - 2011-1?-?? + o Major features: + - When using OpenSSL 1.0.0 or later, use OpenSSL's counter mode + implementation. It makes AES_CTR about 7% faster than our old one + (which was about 10% faster than the one OpenSSL used to provide). + Resolves ticket 4526. + - Tor clients and bridges can now be easily configured to use a + separate 'transport' proxy. This approach helps to resist + censorship by allowing bridges to use protocol obfuscation + plugins. It implements the 'managed proxy' part of proposal + 180. Implements ticket 3472. + - Block excess renegotiations even if they are RFC5746 compliant. + This security fix 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 + ticket 4312. + + o Major bugfixes: + - Teach Tor how to notice excess renegotiation attempts before it + receives the first data SSL record. Fixes part of ticket 4312. + - Only use the EVP interface when AES acceleration is enabled, + to avoid a 5-7% performance regression. Resolves issue 4525; + bugfix on 0.2.3.8-alpha. + + o Minor features: + - Experimental support for running on Windows with IOCP and no + kernel-space socket buffers. This feature is controlled by a new + UserspaceIOCPBuffers feature (off by default), which has no + effect unless Tor has been built with support for bufferevents, + is running on Windows, and has enabled IOCP. This may, in the + long run, help solve or mitigate bug 98. + - Try to make the introductory warning message that Tor prints on + startup more useful for actually finding help and information. + Resolves ticket 2474. + - Running "make version" now displays the version of Tor that + we're about to build. Idea from katmagic; resolves issue 4400. + - If set to 1, Tor will attempt to prevent basic debugging + attachment attempts by other processes. It has no impact for + users who wish to attach if they have CAP_SYS_PTRACE or if they + are root. We believe that this feature works on modern + Gnu/Linux distributions, and that it may also work on OSX and + some *BSD systems (untested). Some modern Gnu/Linux systems + such as Ubuntu have the kernel.yama.ptrace_scope sysctl and by + default enable it as an attempt to limit the PTRACE scope for + all user processes by default. This feature will attempt to + limit the PTRACE scope for Tor specifically - it will not + attempt to alter the system wide ptrace scope as it may not even + exist. If you wish to attach to Tor with a debugger such as gdb + or strace you will want to set this to 0 for the duration of + your debugging. Normal users should leave it on. (Default: 1) + + o Minor bugfixes: + - Resolve an integer overflow bug in smartlist_ensure_capacity(). + Fixes bug 4230; bugfix on Tor 0.1.0.1-rc. Based on a patch by + Mansour Moufid. + - Fix a compile warning in tor_inet_pton(). Bugfix on 0.2.3.8-alpha; + fixes bug 4554. + - Fix a minor formatting issue in one of tor-gencert's error messages. + Fixes bug 4574. + - Prevent a false positive from the check-spaces script, by disabling + the "whitespace between function name and (" check for functions + named 'op()'. + + o Build fixes: + - Properly handle the case where the build-tree is not the same + as the source tree when generating src/common/common_sha1.i, + src/or/micro-revision.i, and src/or/or_sha1.i. Fixes bug 3953; + bugfix on 0.2.0.1-alpha. + + o Code simplifications and refactorings: + - Remove the pure attribute from all functions that used it + previously. In many cases we assigned it incorrectly, because the + functions might assert or call impure functions, and we don't have + evidence that keeping the pure attribute is worthwhile. Implements + changes suggested in ticket 4421. + + Changes in version 0.2.3.8-alpha - 2011-11-22 Tor 0.2.3.8-alpha fixes some crash and assert bugs, including a socketpair-related bug that has been bothering Windows users. It adds diff --git a/changes/UserspaceIOCPBuffers b/changes/UserspaceIOCPBuffers deleted file mode 100644 index 7115b5f..0000000 --- a/changes/UserspaceIOCPBuffers +++ /dev/null @@ -1,7 +0,0 @@ - o Minor features: - - Experimental support for running on Windows with IOCP and no - kernel-space socket buffers. This feature is controlled by a new - UserspaceIOCPBuffers feature (off by default), which has no - effect unless Tor has been built with support for bufferevents, - is running on Windows, and has enabled IOCP. This may, in the - long run, help solve or mitigate bug 98. diff --git a/changes/aes_hackery b/changes/aes_hackery deleted file mode 100644 index 8b2ef1a..0000000 --- a/changes/aes_hackery +++ /dev/null @@ -1,10 +0,0 @@ - o Major features (AES performance): - - When using OpenSSL 1.0.0 or later, use OpenSSL's counter mode - implementation; it makes AES_CTR about 7% faster than our old one - (which was about 10% faster than the one OpenSSL used to provide). - Resolves issue #4526. - - o Major bugfixes (AES performance): - - Only use the EVP interface when AES acceleration is enabled, - to avoid a 5-7% performance regression. Resolves issue #4525, - bugfix on 0.2.3.8-alpha. diff --git a/changes/bug2474 b/changes/bug2474 deleted file mode 100644 index 02d3eb7..0000000 --- a/changes/bug2474 +++ /dev/null @@ -1,5 +0,0 @@ - o Minor features - - Try to make the introductory warning message that Tor prints on - startup more useful for actually finding help and information. - Resolves bug2474. - diff --git a/changes/bug3472 b/changes/bug3472 deleted file mode 100644 index d99d840..0000000 --- a/changes/bug3472 +++ /dev/null @@ -1,7 +0,0 @@ - o Major features: - - - Tor clients and bridges can now be easily configured to use a - separate 'transport' proxy. This approach helps to resist - censorship by allowing bridges to use protocol obfuscation - plugins. It implements the 'managed proxy' part of proposal - 180. Implements ticket #3472. diff --git a/changes/bug3953 b/changes/bug3953 deleted file mode 100644 index 5b3919f..0000000 --- a/changes/bug3953 +++ /dev/null @@ -1,6 +0,0 @@ - o Build fixes: - - Properly handle the case where the build-tree is not the same - as the source tree when generating src/common/common_sha1.i, - src/or/micro-revision.i and src/or/or_sha1.i. Fixes bug 3953; - bugfix on 0.2.0.1-alpha. - diff --git a/changes/bug4230 b/changes/bug4230 deleted file mode 100644 index c1ba584..0000000 --- a/changes/bug4230 +++ /dev/null @@ -1,5 +0,0 @@ - o Minor bugfixes: - - Resolve an integer overflow bug in smartlist_ensure_capacity. - Fixes bug 4230; bugfix on Tor 0.1.0.1-rc. Based on a patch by - Mansour Moufid. - diff --git a/changes/bug4312 b/changes/bug4312 deleted file mode 100644 index f8647d3..0000000 --- a/changes/bug4312 +++ /dev/null @@ -1,11 +0,0 @@ - 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/changes/bug4554 b/changes/bug4554 deleted file mode 100644 index e4754c2..0000000 --- a/changes/bug4554 +++ /dev/null @@ -1,4 +0,0 @@ - o Minor bugfixes: - - Fix a compile warning in tor_inet_pton(). Bugfix on 0.2.3.8-alpha; - fixes bug 4554. - diff --git a/changes/bug4574 b/changes/bug4574 deleted file mode 100644 index 0a57957..0000000 --- a/changes/bug4574 +++ /dev/null @@ -1,4 +0,0 @@ - o Minor bugfixes: - - Fix a minor formatting issue in one of tor-gencert's error messages. - Fixes bug 4574. - diff --git a/changes/checkSpaces b/changes/checkSpaces deleted file mode 100644 index 91f79ed..0000000 --- a/changes/checkSpaces +++ /dev/null @@ -1,5 +0,0 @@ - o Minor bugfixes: - - Prevent a false positive from the check-spaces script by disabling - the "whitespace between function name and (" check for functions - named 'op()'. - diff --git a/changes/disable_debugger_attachment b/changes/disable_debugger_attachment deleted file mode 100644 index b413788..0000000 --- a/changes/disable_debugger_attachment +++ /dev/null @@ -1,16 +0,0 @@ - o Minor features: - - If set to 1, Tor will attempt to prevent basic debugging - attachment attempts by other processes. It has no impact for - users who wish to attach if they have CAP_SYS_PTRACE or if they - are root. We believe that this feature works on modern - Gnu/Linux distributions, and that it may also work on OSX and - some *BSD systems (untested). Some modern Gnu/Linux systems - such as Ubuntu have the kernel.yama.ptrace_scope sysctl and by - default enable it as an attempt to limit the PTRACE scope for - all user processes by default. This feature will attempt to - limit the PTRACE scope for Tor specifically - it will not - attempt to alter the system wide ptrace scope as it may not even - exist. If you wish to attach to Tor with a debugger such as gdb - or strace you will want to set this to 0 for the duration of - your debugging. Normal users should leave it on. (Default: 1) - diff --git a/changes/make_version b/changes/make_version deleted file mode 100644 index b736a87..0000000 --- a/changes/make_version +++ /dev/null @@ -1,4 +0,0 @@ - o Minor features (build): - - Running "make version" now displays the version of Tor that - we're about to build. Idea from katmagic; resolves issue 4400. - diff --git a/changes/pure_removal b/changes/pure_removal deleted file mode 100644 index 4a3343c..0000000 --- a/changes/pure_removal +++ /dev/null @@ -1,7 +0,0 @@ - o Code simplifications and refactorings: - - Remove the pure attribute from all functions that used it previously. - In many cases this we assigned it incorrectly, because the functions - might assert or call impure functions, and we don't have evidence - that keeping the pure attribute is worthwhile. Implements changes - suggested in ticket 4421. -

1 0

[torspec/master] minor typo
by arma＠torproject.org 26 Nov '11

26 Nov '11

commit 0c1a4a7bee9dabe2d7b9ecdc765807c43253fa0d Author: Roger Dingledine <arma(a)torproject.org> Date: Fri Nov 25 19:14:05 2011 -0500 minor typo --- proposals/178-param-voting.txt | 2 +- 1 files changed, 1 insertions(+), 1 deletions(-) diff --git a/proposals/178-param-voting.txt b/proposals/178-param-voting.txt index df68ad4..dddfaff 100644 --- a/proposals/178-param-voting.txt +++ b/proposals/178-param-voting.txt @@ -84,7 +84,7 @@ The following should be added to the bottom of section 3.4.: The following line should be added to the bottom of section 3.4.1.: - "12" -- Params are only included if a enough auths voted for them + "12" -- Params are only included if enough auths voted for them Compatibility:

1 0

[tor/master] use event_free() wrapper; fix bug 4582
by nickm＠torproject.org 26 Nov '11

26 Nov '11

commit 9a88c0cd32df53116a6bbb6b961650943755061c Author: Nick Mathewson <nickm(a)torproject.org> Date: Fri Nov 25 19:09:48 2011 -0500 use event_free() wrapper; fix bug 4582 --- src/common/compat_libevent.c | 2 +- 1 files changed, 1 insertions(+), 1 deletions(-) diff --git a/src/common/compat_libevent.c b/src/common/compat_libevent.c index c3a4746..3a754be 100644 --- a/src/common/compat_libevent.c +++ b/src/common/compat_libevent.c @@ -573,7 +573,7 @@ run_runnable_cb(evutil_socket_t s, short what, void *arg) void *cb_arg = r->arg; (void)what; (void)s; - event_free(r->ev); + tor_event_free(r->ev); tor_free(r); cb(cb_arg);

1 0

[torspec/master] Fix proposal 178: add missing section, formatting
by nickm＠torproject.org 25 Nov '11

25 Nov '11

commit 8ecb3498db18ecd4f30e7d88b4dd0dd4913f2a94 Author: Sebastian Hahn <sebastian(a)torproject.org> Date: Mon May 2 11:19:43 2011 +0200 Fix proposal 178: add missing section, formatting --- proposals/178-param-voting.txt | 9 +++++++-- 1 files changed, 7 insertions(+), 2 deletions(-) diff --git a/proposals/178-param-voting.txt b/proposals/178-param-voting.txt index 2c4abf6..8c38708 100644 --- a/proposals/178-param-voting.txt +++ b/proposals/178-param-voting.txt @@ -60,11 +60,15 @@ It is proposed that the above is changed to: Entries are given on the "params" line for every keyword on which a majority of authorities (total authorities, not just those participating this vote) voted on. The values given are the - low-median of all votes on that keyword. XXX note previous behaviour. + low-median of all votes on that keyword. + + Consensus methods 11 and before, entries are given on the "params" + line for every keyword on which any authority voted, the value given + being the low-median of all votes on that keyword. The following should be added to the bottom of section 3.4.: - * If consensus method 12 or later is used, only consensus + * If consensus method 12 or later is used, only consensus parameters that more than half of the total number of authorities voted for are included in the consensus. @@ -84,3 +88,4 @@ Implementation: An example implementation of this feature can be found in https://gitweb.torproject.org/sebastian/tor.git, branch safer_params. +

1 0

[torspec/master] Update prop 178 to reflect tor-dev discussion
by nickm＠torproject.org 25 Nov '11

25 Nov '11

commit db9a429d005ae05ad0841e6026941594d5740b0b Author: Sebastian Hahn <sebastian(a)torproject.org> Date: Fri Nov 25 19:05:53 2011 +0100 Update prop 178 to reflect tor-dev discussion --- proposals/178-param-voting.txt | 42 ++++++++++++++++++++++++--------------- 1 files changed, 26 insertions(+), 16 deletions(-) diff --git a/proposals/178-param-voting.txt b/proposals/178-param-voting.txt index 8c38708..df68ad4 100644 --- a/proposals/178-param-voting.txt +++ b/proposals/178-param-voting.txt @@ -18,35 +18,44 @@ that paramater. The value of the parameter will be the low-median of all the votes for this parameter. This proposal aims at changing this voting process to be more secure -against tampering by a non-majority of directory authorities. +against tampering by a small fraction of directory authorities. Motivation: -To prevent a minority of the directory authorities from influencing -the value of a parameter unduly, the majority of directory authorities -has to vote for that parameter. This is not currently happening, and -it was in fact not uncommon for a single authority to govern the value -of a consensus parameter. +To prevent a small fraction of the directory authorities from +influencing the value of a parameter unduly, a big enough fraction +of all directory authorities authorities has to vote for that +parameter. This is not currently happening, and it is in fact not +uncommon for a single authority to govern the value of a consensus +parameter. Design: When the consensus is generated, the directory authorities ensure that -a param is only included in the list of params if at least half of the -total number of authorities votes for that param. The value chosen is -the low-median of all the votes. We don't mandate that the authorities -have to vote on exactly the same value for it to be included because -some consensus parameters could be the result of active measurements -that individual authorities make. +a param is only included in the list of params if at least three of the +authorities (or a simple majority, whichever is the smaller number) +votes for that param. The value chosen is the low-median of all the +votes. We don't mandate that the authorities have to vote on exactly +the same value for it to be included because some consensus parameters +could be the result of active measurements that individual authorities +make. Security implications: This change is aimed at improving the security of Tor nodes against -attacks carried out by a minority of directory authorities. It is -possible that a consensus parameter that would be helpful to the +attacks carried out by a small fraction of directory authorities. It +is possible that a consensus parameter that would be helpful to the network is not included because not enough directory authorities voted for it, but since clients are required to have sane defaults in case the parameter is absent this does not carry a security risk. +This proposal makes a security vs coordination effort tradeoff. When +considering only the security of the design, it would be better to +require a simple majority of directory authorities to agree on +voting on a parameter, but it would involve requiring more +directory authority operators to coordinate their actions to set the +parameter successfully. + Specification: dir-spec section 3.4 currently says: @@ -59,7 +68,8 @@ It is proposed that the above is changed to: Entries are given on the "params" line for every keyword on which a majority of authorities (total authorities, not just those - participating this vote) voted on. The values given are the + participating in this vote) voted on, or if at least three + authorities voted for that parameter. The values given are the low-median of all votes on that keyword. Consensus methods 11 and before, entries are given on the "params" @@ -74,7 +84,7 @@ The following should be added to the bottom of section 3.4.: The following line should be added to the bottom of section 3.4.1.: - "12" -- Params are only included if a majority voted for them + "12" -- Params are only included if a enough auths voted for them Compatibility:

1 0

[tor/master] Detect renegotiation when it actually happens.
by nickm＠torproject.org 25 Nov '11

25 Nov '11

commit 4fd79f9def28996552b5739792f428c2514de1f6 Author: George Kadianakis <desnacked(a)gmail.com> Date: Wed Oct 26 03:09:22 2011 +0200 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 files changed, 19 insertions(+), 10 deletions(-) diff --git a/src/common/tortls.c b/src/common/tortls.c index 6757cfa..79b6d2c 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(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; - } -#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;

1 0

[tor/master] Refactor the SSL_set_info_callback() callbacks.
by nickm＠torproject.org 25 Nov '11

25 Nov '11

commit 69a821ea1c9357acdd5aa1c9e23fd030b01cb5a9 Author: George Kadianakis <desnacked(a)gmail.com> Date: Wed Oct 26 02:05:45 2011 +0200 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 files changed, 44 insertions(+), 53 deletions(-) diff --git a/src/common/tortls.c b/src/common/tortls.c index 9a3c02b..6757cfa 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: - * <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; - - 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

1 0

[tor/master] Detect and deny excess renegotiations attempts.
by nickm＠torproject.org 25 Nov '11

25 Nov '11

commit ecd239e3b577705e0669d47293a2e755cf93cec0 Author: George Kadianakis <desnacked(a)gmail.com> Date: Wed Oct 26 03:12:18 2011 +0200 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 files changed, 15 insertions(+), 5 deletions(-) diff --git a/src/common/tortls.c b/src/common/tortls.c index 79b6d2c..7269785 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

1 0

[tor/master] Get rid of tor_tls_block_renegotiation().
by nickm＠torproject.org 25 Nov '11

25 Nov '11

commit 340809dd224b244675496e301d3ba154a6fe68d0 Author: George Kadianakis <desnacked(a)gmail.com> Date: Wed Oct 26 13:16:14 2011 +0200 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, 0 insertions(+), 16 deletions(-) diff --git a/src/common/tortls.c b/src/common/tortls.c index 7269785..1150c42 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 90e76e4..1219b80 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 bcae075..6c56a61 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;

1 0

[tor/master] Also handle needless renegotiations in SSL_write().
by nickm＠torproject.org 25 Nov '11

25 Nov '11

commit e2b3527106e0747f652e2f28fa087d9874e0e2ce Author: George Kadianakis <desnacked(a)gmail.com> Date: Wed Oct 26 13:36:30 2011 +0200 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 files changed, 24 insertions(+), 8 deletions(-) diff --git a/src/common/tortls.c b/src/common/tortls.c index 1150c42..4235de7 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; }

1 0