September 2018 - tor-commits - lists.torproject.org

[tor/master] Change log_test_helpers macros to use printf, not pasting
by nickm＠torproject.org 05 Sep '18

05 Sep '18

commit 8148c0717dc6b05b0cda18ade94e03ed947bc7a3 Author: Nick Mathewson <nickm(a)torproject.org> Date: Wed Aug 22 10:31:44 2018 -0400 Change log_test_helpers macros to use printf, not pasting This ensures that our test failure messages actually tell us what strings Tor was expecting. I will need this to debug some test failures. --- src/test/log_test_helpers.h | 35 ++++++++++++++++++----------------- src/test/test_shared_random.c | 2 +- 2 files changed, 19 insertions(+), 18 deletions(-) diff --git a/src/test/log_test_helpers.h b/src/test/log_test_helpers.h index fc9768c12..330da3811 100644 --- a/src/test/log_test_helpers.h +++ b/src/test/log_test_helpers.h @@ -32,7 +32,7 @@ void mock_dump_saved_logs(void); #define assert_log_predicate(predicate, failure_msg) \ do { \ if (!(predicate)) { \ - tt_fail_msg((failure_msg)); \ + TT_FAIL(failure_msg); \ mock_dump_saved_logs(); \ TT_EXIT_TEST_FUNCTION; \ } \ @@ -40,70 +40,71 @@ void mock_dump_saved_logs(void); #define expect_log_msg(str) \ assert_log_predicate(mock_saved_log_has_message(str), \ - "expected log to contain " # str); + ("expected log to contain \"%s\"", str)); #define expect_log_msg_containing(str) \ assert_log_predicate(mock_saved_log_has_message_containing(str), \ - "expected log to contain " # str); + ("expected log to contain \"%s\"", str)); #define expect_log_msg_containing_either(str1, str2) \ assert_log_predicate(mock_saved_log_has_message_containing(str1) || \ mock_saved_log_has_message_containing(str2), \ - "expected log to contain " # str1 " or " # str2); + ("expected log to contain \"%s\" or \"%s\"", str1, str2)); #define expect_log_msg_containing_either3(str1, str2, str3) \ assert_log_predicate(mock_saved_log_has_message_containing(str1) || \ mock_saved_log_has_message_containing(str2) || \ mock_saved_log_has_message_containing(str3), \ - "expected log to contain " # str1 " or " # str2 \ - " or " # str3); + ("expected log to contain \"%s\" or \"%s\" or \"%s\"", \ + str1, str2, str3)) #define expect_log_msg_containing_either4(str1, str2, str3, str4) \ assert_log_predicate(mock_saved_log_has_message_containing(str1) || \ mock_saved_log_has_message_containing(str2) || \ mock_saved_log_has_message_containing(str3) || \ mock_saved_log_has_message_containing(str4), \ - "expected log to contain " # str1 " or " # str2 \ - " or " # str3 " or " # str4); + ("expected log to contain \"%s\" or \"%s\" or \"%s\" or \"%s\"", \ + str1, str2, str3, str4)) #define expect_single_log_msg(str) \ do { \ \ assert_log_predicate(mock_saved_log_has_message_containing(str) && \ mock_saved_log_n_entries() == 1, \ - "expected log to contain exactly 1 message " # str); \ + ("expected log to contain exactly 1 message \"%s\"", \ + str)); \ } while (0); #define expect_single_log_msg_containing(str) \ do { \ assert_log_predicate(mock_saved_log_has_message_containing(str)&& \ mock_saved_log_n_entries() == 1 , \ - "expected log to contain 1 message, containing " # str); \ + ("expected log to contain 1 message, containing \"%s\"",\ + str)); \ } while (0); #define expect_no_log_msg(str) \ assert_log_predicate(!mock_saved_log_has_message(str), \ - "expected log to not contain " # str); + ("expected log to not contain \"%s\"",str)); #define expect_no_log_msg_containing(str) \ assert_log_predicate(!mock_saved_log_has_message_containing(str), \ - "expected log to not contain " # str); + ("expected log to not contain \"%s\"", str)); #define expect_log_severity(severity) \ assert_log_predicate(mock_saved_log_has_severity(severity), \ - "expected log to contain severity " # severity); + ("expected log to contain severity " # severity)); #define expect_no_log_severity(severity) \ assert_log_predicate(!mock_saved_log_has_severity(severity), \ - "expected log to not contain severity " # severity); + ("expected log to not contain severity " # severity)); #define expect_log_entry() \ assert_log_predicate(mock_saved_log_has_entry(), \ - "expected log to contain entries"); + ("expected log to contain entries")); #define expect_no_log_entry() \ assert_log_predicate(!mock_saved_log_has_entry(), \ - "expected log to not contain entries"); + ("expected log to not contain entries")); #endif /* !defined(TOR_LOG_TEST_HELPERS_H) */ - diff --git a/src/test/test_shared_random.c b/src/test/test_shared_random.c index d2defdf68..4137877e6 100644 --- a/src/test/test_shared_random.c +++ b/src/test/test_shared_random.c @@ -1284,7 +1284,7 @@ test_keep_commit(void *arg) expect_log_msg_containing("doesn't match the commit value."); expect_log_msg_containing("has an invalid reveal value."); assert_log_predicate(mock_saved_log_n_entries() == 2, - "expected 2 log entries"); + ("expected 2 log entries")); teardown_capture_of_logs(); memcpy(commit->hashed_reveal, place_holder.hashed_reveal, sizeof(commit->hashed_reveal));

1 0

[tor/master] Remove tor_tls_shutdown()
by nickm＠torproject.org 05 Sep '18

05 Sep '18

commit dd04fc35c665976f9fc9ff586cbf7fe34d9cc241 Author: Nick Mathewson <nickm(a)torproject.org> Date: Thu Aug 23 09:32:20 2018 -0400 Remove tor_tls_shutdown() This function was supposed to implement a half-duplex mode for our TLS connections. However, nothing in Tor actually uses it (besides some unit tests), and the implementation looks really questionable to me. It's probably best to remove it. We can add a tested one later if we need one in the future. --- src/lib/tls/tortls.h | 1 - src/lib/tls/tortls_nss.c | 9 ---- src/lib/tls/tortls_openssl.c | 63 --------------------------- src/test/test_tortls_openssl.c | 96 ------------------------------------------ 4 files changed, 169 deletions(-) diff --git a/src/lib/tls/tortls.h b/src/lib/tls/tortls.h index 7bbb42b2f..a8bc7370a 100644 --- a/src/lib/tls/tortls.h +++ b/src/lib/tls/tortls.h @@ -111,7 +111,6 @@ int tor_tls_finish_handshake(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); size_t tor_tls_get_forced_write_size(tor_tls_t *tls); diff --git a/src/lib/tls/tortls_nss.c b/src/lib/tls/tortls_nss.c index d2b81ad08..671c01847 100644 --- a/src/lib/tls/tortls_nss.c +++ b/src/lib/tls/tortls_nss.c @@ -534,15 +534,6 @@ tor_tls_assert_renegotiation_unblocked(tor_tls_t *tls) } int -tor_tls_shutdown(tor_tls_t *tls) -{ - tor_assert(tls); - /* XXXX This is not actually used, so I'm not implementing it. We can - * XXXX remove this function entirely someday. */ - return -1; -} - -int tor_tls_get_pending_bytes(tor_tls_t *tls) { tor_assert(tls); diff --git a/src/lib/tls/tortls_openssl.c b/src/lib/tls/tortls_openssl.c index c4e9e7770..a9bab67a0 100644 --- a/src/lib/tls/tortls_openssl.c +++ b/src/lib/tls/tortls_openssl.c @@ -1323,69 +1323,6 @@ tor_tls_finish_handshake(tor_tls_t *tls) return r; } -/** Shut down an open tls connection tls. When finished, returns - * TOR_TLS_DONE. On failure, returns TOR_TLS_ERROR, TOR_TLS_WANTREAD, - * or TOR_TLS_WANTWRITE. - */ -int -tor_tls_shutdown(tor_tls_t *tls) -{ - int r, err; - char buf[128]; - tor_assert(tls); - tor_assert(tls->ssl); - check_no_tls_errors(); - - while (1) { - if (tls->state == TOR_TLS_ST_SENTCLOSE) { - /* If we've already called shutdown once to send a close message, - * we read until the other side has closed too. - */ - do { - r = SSL_read(tls->ssl, buf, 128); - } while (r>0); - err = tor_tls_get_error(tls, r, CATCH_ZERO, "reading to shut down", - LOG_INFO, LD_NET); - if (err == TOR_TLS_ZERORETURN_) { - tls->state = TOR_TLS_ST_GOTCLOSE; - /* fall through... */ - } else { - return err; - } - } - - r = SSL_shutdown(tls->ssl); - if (r == 1) { - /* If shutdown returns 1, the connection is entirely closed. */ - tls->state = TOR_TLS_ST_CLOSED; - return TOR_TLS_DONE; - } - err = tor_tls_get_error(tls, r, CATCH_SYSCALL|CATCH_ZERO, "shutting down", - LOG_INFO, LD_NET); - if (err == TOR_TLS_SYSCALL_) { - /* The underlying TCP connection closed while we were shutting down. */ - tls->state = TOR_TLS_ST_CLOSED; - return TOR_TLS_DONE; - } else if (err == TOR_TLS_ZERORETURN_) { - /* The TLS connection says that it sent a shutdown record, but - * isn't done shutting down yet. Make sure that this hasn't - * happened before, then go back to the start of the function - * and try to read. - */ - if (tls->state == TOR_TLS_ST_GOTCLOSE || - tls->state == TOR_TLS_ST_SENTCLOSE) { - log_warn(LD_NET, - "TLS returned \"half-closed\" value while already half-closed"); - return TOR_TLS_ERROR_MISC; - } - tls->state = TOR_TLS_ST_SENTCLOSE; - /* fall through ... */ - } else { - return err; - } - } /* end loop */ -} - /** Return true iff this TLS connection is authenticated. */ int diff --git a/src/test/test_tortls_openssl.c b/src/test/test_tortls_openssl.c index 12a05b303..b7e28f376 100644 --- a/src/test/test_tortls_openssl.c +++ b/src/test/test_tortls_openssl.c @@ -1710,102 +1710,6 @@ dummy_handshake_func(SSL *s) return 1; } -static void -test_tortls_shutdown(void *ignored) -{ - (void)ignored; - int ret; - tor_tls_t *tls; - SSL_METHOD *method = give_me_a_test_method(); - setup_capture_of_logs(LOG_WARN); - - tls = tor_malloc_zero(sizeof(tor_tls_t)); - tls->ssl = tor_malloc_zero(sizeof(SSL)); - tls->ssl->method = method; - method->ssl_read = fixed_ssl_read; - method->ssl_shutdown = fixed_ssl_shutdown; - - ret = tor_tls_shutdown(tls); - tt_int_op(ret, OP_EQ, -9); - - tls->state = TOR_TLS_ST_SENTCLOSE; - fixed_ssl_read_result_index = 0; - fixed_ssl_read_result[0] = 10; - fixed_ssl_read_result[1] = -1; - ret = tor_tls_shutdown(tls); - tt_int_op(ret, OP_EQ, -9); - -#ifndef LIBRESSL_VERSION_NUMBER - tls->ssl->handshake_func = dummy_handshake_func; - - fixed_ssl_read_result_index = 0; - fixed_ssl_read_result[0] = 10; - fixed_ssl_read_result[1] = 42; - fixed_ssl_read_result[2] = 0; - fixed_ssl_shutdown_result = 1; - ERR_clear_error(); - tls->ssl->version = SSL2_VERSION; - ret = tor_tls_shutdown(tls); - tt_int_op(ret, OP_EQ, TOR_TLS_DONE); - tt_int_op(tls->state, OP_EQ, TOR_TLS_ST_CLOSED); - - fixed_ssl_read_result_index = 0; - fixed_ssl_read_result[0] = 10; - fixed_ssl_read_result[1] = 42; - fixed_ssl_read_result[2] = 0; - fixed_ssl_shutdown_result = 0; - ERR_clear_error(); - tls->ssl->version = 0; - ret = tor_tls_shutdown(tls); - tt_int_op(ret, OP_EQ, TOR_TLS_DONE); - tt_int_op(tls->state, OP_EQ, TOR_TLS_ST_CLOSED); - - fixed_ssl_read_result_index = 0; - fixed_ssl_read_result[0] = 10; - fixed_ssl_read_result[1] = 42; - fixed_ssl_read_result[2] = 0; - fixed_ssl_shutdown_result = 0; - ERR_clear_error(); - tls->ssl->version = 0; - method->ssl_shutdown = setting_version_ssl_shutdown; - ret = tor_tls_shutdown(tls); - tt_int_op(ret, OP_EQ, TOR_TLS_ERROR_MISC); - - fixed_ssl_read_result_index = 0; - fixed_ssl_read_result[0] = 10; - fixed_ssl_read_result[1] = 42; - fixed_ssl_read_result[2] = 0; - fixed_ssl_shutdown_result = 0; - fixed_tls = tls; - fixed_ssl_state_to_set = TOR_TLS_ST_GOTCLOSE; - ERR_clear_error(); - tls->ssl->version = 0; - method->ssl_shutdown = setting_version_and_state_ssl_shutdown; - ret = tor_tls_shutdown(tls); - tt_int_op(ret, OP_EQ, TOR_TLS_ERROR_MISC); - - fixed_ssl_read_result_index = 0; - fixed_ssl_read_result[0] = 10; - fixed_ssl_read_result[1] = 42; - fixed_ssl_read_result[2] = 0; - fixed_ssl_read_result[3] = -1; - fixed_ssl_shutdown_result = 0; - fixed_tls = tls; - fixed_ssl_state_to_set = 0; - ERR_clear_error(); - tls->ssl->version = 0; - method->ssl_shutdown = setting_version_and_state_ssl_shutdown; - ret = tor_tls_shutdown(tls); - tt_int_op(ret, OP_EQ, TOR_TLS_ERROR_MISC); -#endif /* !defined(LIBRESSL_VERSION_NUMBER) */ - - done: - teardown_capture_of_logs(); - tor_free(method); - tor_free(tls->ssl); - tor_free(tls); -} - static int negotiated_callback_called; static void

1 0

[tor/master] Avoid spurious error logs when using NSS
by nickm＠torproject.org 05 Sep '18

05 Sep '18

commit 52d5f4da12cf4a9647a896395209121cbf9483c4 Author: Nick Mathewson <nickm(a)torproject.org> Date: Thu Aug 23 10:13:32 2018 -0400 Avoid spurious error logs when using NSS The tls_log_errors() function now behaves differently for NSS than it did for OpenSSL, so we need to tweak it a bit. --- src/lib/tls/tortls.c | 13 +++++++++++-- src/lib/tls/tortls_nss.c | 6 ++++-- 2 files changed, 15 insertions(+), 4 deletions(-) diff --git a/src/lib/tls/tortls.c b/src/lib/tls/tortls.c index cc9738599..edf421b4d 100644 --- a/src/lib/tls/tortls.c +++ b/src/lib/tls/tortls.c @@ -189,6 +189,9 @@ tor_tls_context_init(unsigned flags, if (old_ctx != NULL) { tor_tls_context_decref(old_ctx); } + } else { + tls_log_errors(NULL, LOG_WARN, LD_CRYPTO, + "constructing a TLS context"); } } else { if (server_identity != NULL) { @@ -197,6 +200,9 @@ tor_tls_context_init(unsigned flags, key_lifetime, flags, 0); + if (rv1 < 0) + tls_log_errors(NULL, LOG_WARN, LD_CRYPTO, + "constructing a server TLS context"); } else { tor_tls_context_t *old_ctx = server_tls_context; server_tls_context = NULL; @@ -211,9 +217,11 @@ tor_tls_context_init(unsigned flags, key_lifetime, flags, 1); + if (rv2 < 0) + tls_log_errors(NULL, LOG_WARN, LD_CRYPTO, + "constructing a client TLS context"); } - tls_log_errors(NULL, LOG_WARN, LD_CRYPTO, "constructing a TLS context"); return MIN(rv1, rv2); } @@ -451,8 +459,9 @@ tor_tls_check_lifetime(int severity, tor_tls_t *tls, r = 0; done: tor_x509_cert_free(cert); - /* Not expected to get invoked */ +#ifdef ENABLE_OPENSSL tls_log_errors(tls, LOG_WARN, LD_NET, "checking certificate lifetime"); +#endif return r; } diff --git a/src/lib/tls/tortls_nss.c b/src/lib/tls/tortls_nss.c index 671c01847..40a98dd87 100644 --- a/src/lib/tls/tortls_nss.c +++ b/src/lib/tls/tortls_nss.c @@ -323,8 +323,10 @@ void tls_log_errors(tor_tls_t *tls, int severity, int domain, const char *doing) { - /* XXXX This implementation isn't right for NSS -- it logs the last error - whether anything actually failed or not. */ + /* This implementation is a little different for NSS than it is for OpenSSL + -- it logs the last error whether anything actually failed or not. So we + have to only call it when something has gone wrong and we have a real + error to report. */ (void)tls; PRErrorCode code = PORT_GetError();

1 0

[tor/master] Remove tor_x509_get_cert_impl as unneeded.
by nickm＠torproject.org 05 Sep '18

05 Sep '18

commit 02086a216f15fd8c45e603a8d9bab482f60753f4 Author: Nick Mathewson <nickm(a)torproject.org> Date: Thu Aug 23 13:05:38 2018 -0400 Remove tor_x509_get_cert_impl as unneeded. --- src/lib/tls/tortls_openssl.c | 2 +- src/lib/tls/x509.c | 8 -------- src/lib/tls/x509_internal.h | 2 -- 3 files changed, 1 insertion(+), 11 deletions(-) diff --git a/src/lib/tls/tortls_openssl.c b/src/lib/tls/tortls_openssl.c index e455693df..d10e50a97 100644 --- a/src/lib/tls/tortls_openssl.c +++ b/src/lib/tls/tortls_openssl.c @@ -480,7 +480,7 @@ tor_tls_cert_matches_key,(const tor_tls_t *tls, const tor_x509_cert_t *cert)) if (!peercert) return 0; link_key = X509_get_pubkey(peercert); - cert_key = X509_get_pubkey((X509 *)tor_x509_cert_get_impl(cert)); + cert_key = X509_get_pubkey(cert->cert); result = link_key && cert_key && EVP_PKEY_cmp(cert_key, link_key) == 1; diff --git a/src/lib/tls/x509.c b/src/lib/tls/x509.c index d2270f910..cff1c1302 100644 --- a/src/lib/tls/x509.c +++ b/src/lib/tls/x509.c @@ -52,14 +52,6 @@ tor_tls_pick_certificate_lifetime(time_t now, *end_time_out = end_time; } -/** Return the underlying implementation for cert */ -const tor_x509_cert_impl_t * -tor_x509_cert_get_impl(const tor_x509_cert_t *cert) -{ - tor_assert(cert); - return cert->cert; -} - /** Return a set of digests for the public key in cert, or NULL if this * cert's public key is not one we know how to take the digest of. */ const common_digests_t * diff --git a/src/lib/tls/x509_internal.h b/src/lib/tls/x509_internal.h index 4b49f1dec..0e186ec84 100644 --- a/src/lib/tls/x509_internal.h +++ b/src/lib/tls/x509_internal.h @@ -33,8 +33,6 @@ MOCK_DECL(tor_x509_cert_impl_t *, tor_tls_create_certificate, unsigned int cert_lifetime)); MOCK_DECL(tor_x509_cert_t *, tor_x509_cert_new, (tor_x509_cert_impl_t *x509_cert)); -const tor_x509_cert_impl_t *tor_x509_cert_get_impl( - const tor_x509_cert_t *cert); int tor_x509_check_cert_lifetime_internal(int severity, const tor_x509_cert_impl_t *cert,

1 0

[tor/master] changes file for our NSS support
by nickm＠torproject.org 05 Sep '18

05 Sep '18

commit c18967d6c80f6a40acc78556490a4f40bc0e7003 Author: Nick Mathewson <nickm(a)torproject.org> Date: Thu Aug 23 11:45:34 2018 -0400 changes file for our NSS support --- changes/NSS | 7 +++++++ 1 file changed, 7 insertions(+) diff --git a/changes/NSS b/changes/NSS new file mode 100644 index 000000000..fc13867b4 --- /dev/null +++ b/changes/NSS @@ -0,0 +1,7 @@ + o Major features (portability, cryptography, experimental, TLS): + - Tor now has the option to compile with the NSS library instead of + OpenSSL. This feature is experimental, and we expect that bugs may + remain. It is mainly intended for environments where Tor's performance + is not CPU-bound, and where NSS is already known to be installed. + To try it out, configure Tor with the --enable-nss flag. + Closes ticket 26631.

1 0

[tor/master] Avoid double-close on TCP sockets under NSS.
by nickm＠torproject.org 05 Sep '18

05 Sep '18

commit b5fddbd24144a94580e50886cd07a82968a1b86c Author: Nick Mathewson <nickm(a)torproject.org> Date: Thu Aug 23 13:02:12 2018 -0400 Avoid double-close on TCP sockets under NSS. --- src/core/mainloop/connection.c | 25 +++++++++++++++++-------- src/lib/tls/tortls_openssl.c | 2 +- 2 files changed, 18 insertions(+), 9 deletions(-) diff --git a/src/core/mainloop/connection.c b/src/core/mainloop/connection.c index ed789d520..91ae41074 100644 --- a/src/core/mainloop/connection.c +++ b/src/core/mainloop/connection.c @@ -449,6 +449,20 @@ connection_new(int type, int socket_family) } } +static void +connection_close_and_invalidate_socket(connection_t *conn) +{ + if (connection_speaks_cells(conn)) { + or_connection_t *or_conn = TO_OR_CONN(conn); + tor_tls_free(or_conn->tls); + or_conn->tls = NULL; + or_conn->base_.s = TOR_INVALID_SOCKET; + } else { + tor_close_socket(conn->s); + conn->s = TOR_INVALID_SOCKET; + } +} + /** Initializes conn. (you must call connection_add() to link it into the main * array). * @@ -614,9 +628,8 @@ connection_free_minimal(connection_t *conn) tor_free(conn->address); if (connection_speaks_cells(conn)) { + connection_close_and_invalidate_socket(conn); or_connection_t *or_conn = TO_OR_CONN(conn); - tor_tls_free(or_conn->tls); - or_conn->tls = NULL; or_handshake_state_free(or_conn->handshake_state); or_conn->handshake_state = NULL; tor_free(or_conn->nickname); @@ -692,9 +705,7 @@ connection_free_minimal(connection_t *conn) } if (SOCKET_OK(conn->s)) { - log_debug(LD_NET,"closing fd %d.",(int)conn->s); - tor_close_socket(conn->s); - conn->s = TOR_INVALID_SOCKET; + connection_close_and_invalidate_socket(conn); } if (conn->type == CONN_TYPE_OR && @@ -820,9 +831,7 @@ connection_close_immediate(connection_t *conn) conn->read_blocked_on_bw = 0; conn->write_blocked_on_bw = 0; - if (SOCKET_OK(conn->s)) - tor_close_socket(conn->s); - conn->s = TOR_INVALID_SOCKET; + connection_close_and_invalidate_socket(conn); if (conn->linked) conn->linked_conn_is_closed = 1; if (conn->outbuf) diff --git a/src/lib/tls/tortls_openssl.c b/src/lib/tls/tortls_openssl.c index a9bab67a0..e455693df 100644 --- a/src/lib/tls/tortls_openssl.c +++ b/src/lib/tls/tortls_openssl.c @@ -1039,7 +1039,7 @@ tor_tls_new(int sock, int isServer) goto err; } result->socket = sock; - bio = BIO_new_socket(sock, BIO_NOCLOSE); + bio = BIO_new_socket(sock, 0); if (! bio) { tls_log_errors(NULL, LOG_WARN, LD_NET, "opening BIO"); #ifdef SSL_set_tlsext_host_name

1 0

[tor/master] Several unit tests to improve test coverage of x509*.c
by nickm＠torproject.org 05 Sep '18

05 Sep '18

commit 7163389b550a36fa017f700713405fc3c89dc234 Author: Nick Mathewson <nickm(a)torproject.org> Date: Thu Aug 23 14:03:00 2018 -0400 Several unit tests to improve test coverage of x509*.c --- src/lib/crypt_ops/crypto_digest.c | 4 +- src/lib/crypt_ops/crypto_digest.h | 3 +- src/lib/tls/x509.c | 3 +- src/lib/tls/x509_nss.c | 4 +- src/lib/tls/x509_openssl.c | 8 +- src/test/include.am | 1 + src/test/test.c | 1 + src/test/test.h | 1 + src/test/test_x509.c | 204 ++++++++++++++++++++++++++++++++++++++ 9 files changed, 221 insertions(+), 8 deletions(-) diff --git a/src/lib/crypt_ops/crypto_digest.c b/src/lib/crypt_ops/crypto_digest.c index 2bc5f0834..77cf18dca 100644 --- a/src/lib/crypt_ops/crypto_digest.c +++ b/src/lib/crypt_ops/crypto_digest.c @@ -116,8 +116,8 @@ library_supports_digest(digest_algorithm_t alg) * m. Write the DIGEST_LEN byte result into digest. * Return 0 on success, -1 on failure. */ -int -crypto_digest(char *digest, const char *m, size_t len) +MOCK_IMPL(int, +crypto_digest,(char *digest, const char *m, size_t len)) { tor_assert(m); tor_assert(digest); diff --git a/src/lib/crypt_ops/crypto_digest.h b/src/lib/crypt_ops/crypto_digest.h index 59713d2b9..204f1aaff 100644 --- a/src/lib/crypt_ops/crypto_digest.h +++ b/src/lib/crypt_ops/crypto_digest.h @@ -16,6 +16,7 @@ #include "lib/cc/torint.h" #include "lib/defs/digest_sizes.h" #include "lib/malloc/malloc.h" +#include "lib/testsupport/testsupport.h" /** Length of a sha1 message digest when encoded in base32 with trailing = * signs removed. */ @@ -75,7 +76,7 @@ typedef struct crypto_xof_t crypto_xof_t; struct smartlist_t; /* SHA-1 and other digests */ -int crypto_digest(char *digest, const char *m, size_t len); +MOCK_DECL(int, crypto_digest,(char *digest, const char *m, size_t len)); int crypto_digest256(char *digest, const char *m, size_t len, digest_algorithm_t algorithm); int crypto_digest512(char *digest, const char *m, size_t len, diff --git a/src/lib/tls/x509.c b/src/lib/tls/x509.c index cff1c1302..c88298b6c 100644 --- a/src/lib/tls/x509.c +++ b/src/lib/tls/x509.c @@ -118,6 +118,7 @@ tor_x509_cert_new,(tor_x509_cert_impl_t *x509_cert)) crypto_pk_t *pk = tor_tls_cert_get_key(cert); if (pk) { if (crypto_pk_get_common_digests(pk, &cert->pkey_digests) < 0) { + log_warn(LD_CRYPTO, "unable to compute digests of certificate key"); crypto_pk_free(pk); goto err; } @@ -128,10 +129,8 @@ tor_x509_cert_new,(tor_x509_cert_impl_t *x509_cert)) return cert; err: - /* LCOV_EXCL_START for the same reason as the exclusion above */ tor_free(cert); log_err(LD_CRYPTO, "Couldn't wrap encoded X509 certificate."); tor_x509_cert_impl_free_(x509_cert); return NULL; - /* LCOV_EXCL_STOP */ } diff --git a/src/lib/tls/x509_nss.c b/src/lib/tls/x509_nss.c index 35b3d2542..68aebbb69 100644 --- a/src/lib/tls/x509_nss.c +++ b/src/lib/tls/x509_nss.c @@ -62,9 +62,11 @@ tor_tls_create_certificate_internal(crypto_pk_t *rsa, validity = CERT_CreateValidity(((PRTime)start_time) * PRTIME_PER_SEC, ((PRTime)end_time) * PRTIME_PER_SEC); - if (! validity) { + if (BUG(! validity)) { + /* LCOV_EXCL_START */ crypto_nss_log_errors(LOG_WARN, "creating a validity object"); goto err; + /* LCOV_EXCL_STOP */ } unsigned long serial_number; diff --git a/src/lib/tls/x509_openssl.c b/src/lib/tls/x509_openssl.c index 28a30b66e..f315b88f3 100644 --- a/src/lib/tls/x509_openssl.c +++ b/src/lib/tls/x509_openssl.c @@ -327,11 +327,15 @@ tor_tls_cert_is_valid(int severity, if (check_rsa_1024 && cert_key) { RSA *rsa = EVP_PKEY_get1_RSA(cert_key); #ifdef OPENSSL_1_1_API - if (rsa && RSA_bits(rsa) == 1024) + if (rsa && RSA_bits(rsa) == 1024) { #else - if (rsa && BN_num_bits(rsa->n) == 1024) + if (rsa && BN_num_bits(rsa->n) == 1024) { #endif key_ok = 1; + } else { + log_fn(severity, LD_CRYPTO, "Invalid certificate: Key is not RSA1024."); + } + if (rsa) RSA_free(rsa); } else if (cert_key) { diff --git a/src/test/include.am b/src/test/include.am index 05149b865..69680f969 100644 --- a/src/test/include.am +++ b/src/test/include.am @@ -178,6 +178,7 @@ src_test_test_SOURCES += \ src/test/test_util_format.c \ src/test/test_util_process.c \ src/test/test_voting_schedule.c \ + src/test/test_x509.c \ src/test/test_helpers.c \ src/test/test_dns.c \ src/test/testing_common.c \ diff --git a/src/test/test.c b/src/test/test.c index 962344305..df586fc76 100644 --- a/src/test/test.c +++ b/src/test/test.c @@ -924,6 +924,7 @@ struct testgroup_t testgroups[] = { #ifndef ENABLE_NSS { "tortls/openssl/", tortls_openssl_tests }, #endif + { "tortls/x509/", x509_tests }, { "util/", util_tests }, { "util/format/", util_format_tests }, { "util/logging/", logging_tests }, diff --git a/src/test/test.h b/src/test/test.h index 94aa9443d..a46fedf3e 100644 --- a/src/test/test.h +++ b/src/test/test.h @@ -265,6 +265,7 @@ extern struct testcase_t voting_schedule_tests[]; extern struct testcase_t dns_tests[]; extern struct testcase_t handle_tests[]; extern struct testcase_t sr_tests[]; +extern struct testcase_t x509_tests[]; extern struct testcase_t slow_crypto_tests[]; extern struct testcase_t slow_util_tests[]; diff --git a/src/test/test_x509.c b/src/test/test_x509.c new file mode 100644 index 000000000..9163977bd --- /dev/null +++ b/src/test/test_x509.c @@ -0,0 +1,204 @@ +/* Copyright (c) 2010-2018, The Tor Project, Inc. */ +/* See LICENSE for licensing information */ + +#define TOR_X509_PRIVATE +#include "orconfig.h" + +#ifdef _WIN32 +#include <winsock2.h> +#endif +#include <math.h> +#include <stddef.h> + +#include "lib/cc/compat_compiler.h" + +#include "core/or/or.h" +#include "lib/log/log.h" +#include "app/config/config.h" +#include "lib/tls/x509.h" +#include "lib/tls/x509_internal.h" +#include "app/config/or_state_st.h" + +#include "test/test.h" +#include "test/log_test_helpers.h" + +#include "tinytest.h" + +/* A mock replacement for crypto_digest that always fails. */ +static int +mock_failing_digest(char *digest, const char *m, size_t len) +{ + (void)digest; + (void)m; + (void)len; + return -1; +} + +static void +test_x509_cert_new_failing_digest(void *arg) +{ + (void)arg; + crypto_pk_t *pk1=NULL, *pk2=NULL; + tor_x509_cert_impl_t *impl = NULL; + tor_x509_cert_t *cert = NULL; + pk1 = pk_generate(0); + pk2 = pk_generate(1); + + impl = tor_tls_create_certificate(pk1, pk2, "hello", "world", 86400*100); + tt_assert(impl); + MOCK(crypto_digest, mock_failing_digest); + + setup_full_capture_of_logs(LOG_WARN); + cert = tor_x509_cert_new(impl); + tt_assert(!cert); + expect_log_msg_containing("Couldn't wrap encoded X509 certificate"); + expect_log_msg_containing("unable to compute digests of certificate key"); + + done: + crypto_pk_free(pk1); + crypto_pk_free(pk2); + if (impl) + tor_x509_cert_impl_free_(impl); + UNMOCK(crypto_digest); + teardown_capture_of_logs(); +} + +static tor_x509_cert_t * +cert_from_der64(const char *der64) +{ + size_t der64len = strlen(der64); + unsigned char *der = tor_malloc_zero(der64len); + int derlen; + tor_x509_cert_t *cert = NULL; + + derlen = base64_decode((char*)der, der64len, + der64, der64len); + if (derlen >= 0) + cert = tor_x509_cert_decode(der, derlen); + tor_free(der); + return cert; +} + +static void +test_x509_consume_ec_cert(void *arg) +{ + (void)arg; + /* This is a small self-signed EC certificate. */ + const char certificate[] = + "MIIBEzCBugIJAIdl5svgOZ0OMAoGCCqGSM49BAMCMBIxEDAOBgNVBAMMB1Rlc3Rp\n" + "bmcwHhcNMTgwODIzMTcyMzI1WhcNMTkwODIzMTcyMzI1WjASMRAwDgYDVQQDDAdU\n" + "ZXN0aW5nMFkwEwYHKoZIzj0CAQYIKoZIzj0DAQcDQgAExMDpnRc0Btic3tIyCKNE\n" + "iNY4j4gzcaYzS2sTYRoVK3RAukG29Qg6/c8e8XcnsSquU4fItYxDRbi/3nhYk4CP\n" + "GDAKBggqhkjOPQQDAgNIADBFAiA0h1q03C2xlONUgAOonJLrlV1SUtMeKDxNsxsU\n" + "+FSPvQIhAM7kY9Tlt0ELmyMnORPp1VJieXn/qhL5VoxGxSedTbny\n"; + const time_t now = 1535045321; /* when I'm writing this test. */ + tor_x509_cert_t *cert = cert_from_der64(certificate); + tt_assert(cert); + + tt_ptr_op(NULL, OP_EQ, tor_tls_cert_get_key(cert)); + + /* It's a self-signed cert -- make sure it signed itself. */ + tt_assert(tor_tls_cert_is_valid(LOG_ERR, cert, cert, now, 0)); + + /* Make sure we detect its key as non-RSA1024 */ + setup_capture_of_logs(LOG_INFO); + tt_assert(! tor_tls_cert_is_valid(LOG_INFO, cert, cert, now, 1)); + expect_log_msg_containing("Key is not RSA1024"); + + done: + tor_x509_cert_free(cert); + teardown_capture_of_logs(); +} + +static void +test_x509_reject_tiny_keys(void *arg) +{ + (void)arg; + const char *certificates[] = { + /* Self-signed RSA512 */ + "MIIBXDCCAQYCCQDKikjJYZI5uDANBgkqhkiG9w0BAQsFADA1MRUwEwYDVQQHDAxE\n" + "ZWZhdWx0IENpdHkxHDAaBgNVBAoME0RlZmF1bHQgQ29tcGFueSBMdGQwHhcNMTgw\n" + "ODIzMTczNjQ4WhcNMTkwODIzMTczNjQ4WjA1MRUwEwYDVQQHDAxEZWZhdWx0IENp\n" + "dHkxHDAaBgNVBAoME0RlZmF1bHQgQ29tcGFueSBMdGQwXDANBgkqhkiG9w0BAQEF\n" + "AANLADBIAkEAqOvVKzrSpmKOTNqDzBG/iZrUdhCrMRsymFXyIScJcdsyn7jB8RMy\n" + "fbHqG8EqB8HHLU/eqt/+zhh2w08Lx3+5QwIDAQABMA0GCSqGSIb3DQEBCwUAA0EA\n" + "RSCq0sNbD9uWfcBqF0U4MtfFjU5x+RQQCeBVtAzwC9bggSILKZfB9XUvtGh6vqig\n", + /* Self-signed secp112r2 */ + "MIIBLTCB+QIJAI0LtN9uWxy3MAoGCCqGSM49BAMCMEUxCzAJBgNVBAYTAkFVMRMw\n" + "EQYDVQQIDApTb21lLVN0YXRlMSEwHwYDVQQKDBhJbnRlcm5ldCBXaWRnaXRzIFB0\n" + "eSBMdGQwHhcNMTgwODIzMTc0MTQ4WhcNMTkwODIzMTc0MTQ4WjBFMQswCQYDVQQG\n" + "EwJBVTETMBEGA1UECAwKU29tZS1TdGF0ZTEhMB8GA1UECgwYSW50ZXJuZXQgV2lk\n" + "Z2l0cyBQdHkgTHRkMDIwEAYHKoZIzj0CAQYFK4EEAAcDHgAEf7dFHo7xhCtIcgyo\n" + "Px+IDcUUlntZCtar6V4O0zAKBggqhkjOPQQDAgMjADAgAg4yhBJMEmpkNbZU95Zf\n" + "uwIOJAan4J1ETxUII1RrGmw=\n" + }; + const time_t now = 1535046182; + tor_x509_cert_t *cert = NULL; + + unsigned i; + for (i = 0; i < ARRAY_LENGTH(certificates); ++i) { + cert = cert_from_der64(certificates[i]); + /* It might parse okay, depending on our version of NSS or OpenSSL. */ + if (cert == NULL) + continue; + /* But it should not validate. */ + tt_assert(! tor_tls_cert_is_valid(LOG_INFO, cert, cert, now, 0)); + tor_x509_cert_free(cert); + } + + done: + tor_x509_cert_free(cert); +} + +static void +test_x509_expiration(void *arg) +{ + (void)arg; + /* a 365-day RSA2048 cert, created between 0 and 60 minutes before "now" */ + const char certificate[] = + "MIICzjCCAbYCCQDxIONWIQ9OGDANBgkqhkiG9w0BAQsFADApMQswCQYDVQQGEwJV\n" + "UzEaMBgGA1UEAwwRSW50ZXJlc3RpbmcgdGltZXMwHhcNMTgwODIzMTc1NTE4WhcN\n" + "MTkwODIzMTc1NTE4WjApMQswCQYDVQQGEwJVUzEaMBgGA1UEAwwRSW50ZXJlc3Rp\n" + "bmcgdGltZXMwggEiMA0GCSqGSIb3DQEBAQUAA4IBDwAwggEKAoIBAQD0Blz1fBii\n" + "OffpFlzMrmfPah/vkPcNrwoyx5YiosbHErYUpqdCtfNb7rbBM5xcac1LmF9kjnOQ\n" + "uAw1jsCNE82QHwWMlXOqaZCEJsnttNo0Y7yaSR/ChbGJ54XCp+Lx2acyTeH9cBWU\n" + "de8/sKAQ4NqpbEP01pBH4+1mPu2MYWjVWVicUxmw0mJ3cfkJCWUzt0nC4ls8+Itk\n" + "7XliKb216Z9uQXu/zD/JGkxAljnFs1jXCX4NyWz46xnJFzXbYCeyQnBz0tUbAvgg\n" + "uRdryYtHzD46hd8LTXH6oK2gV64ILAhDnRb1aBjnCXxbex24XoW3hjSrKGTdNsXA\n" + "RMWU/8QZaoiBAgMBAAEwDQYJKoZIhvcNAQELBQADggEBAFIYDBcbit2kOMrHECZK\n" + "ctem40A3s+0ZifzZ2KLhW8dTr/2Zb6DnlqVm2iUOV4cG/o1RAn/HzkQQuWEq+oBG\n" + "yOPVHudvCyGs+2ZQWudgAv9xq8N7KtZwJhnn42c2YSoreqRXDQgJqGFatyr+XdR7\n" + "gdQapLI4BFbZToeXp49Nl+q9330hKaSmIYmWEZ7R/33R64PU2el7X9/apYEcuZQT\n" + "+FjEqcO1lJ8/dTwM/2C1BJZqUeFTAu+ac1M+4//qyJRUUc6xSJLhiens8atWaxwL\n" + "eBCT8fCY8oPOwA1eImc/yWWmWXpv8bBWVe8OeLCMKM/OZoIdFqQpqSdcyGoh/kIW\n" + "Dws=\n"; + const time_t now = 1535046996; + + tor_x509_cert_t *cert = cert_from_der64(certificate); + tt_assert(cert); + + tt_assert(tor_tls_cert_is_valid(LOG_ERR, cert, cert, now, 0)); + + tt_assert(tor_tls_cert_is_valid(LOG_ERR, cert, cert, + now-TOR_X509_FUTURE_SLOP, 0)); + tt_assert(tor_tls_cert_is_valid(LOG_ERR, cert, cert, + now+365*86400+TOR_X509_PAST_SLOP - 3600, 0)); + + tt_assert(! tor_tls_cert_is_valid(LOG_INFO, cert, cert, + now-TOR_X509_FUTURE_SLOP - 3600, 0)); + tt_assert(! tor_tls_cert_is_valid(LOG_INFO, cert, cert, + now+365*86400+TOR_X509_FUTURE_SLOP, 0)); + + done: + tor_x509_cert_free(cert); +} + +#define TEST(name) { #name, test_x509_ ## name, TT_FORK, 0, NULL } + +struct testcase_t x509_tests[] = { + TEST(cert_new_failing_digest), + TEST(consume_ec_cert), + TEST(reject_tiny_keys), + TEST(expiration), + END_OF_TESTCASES +};

1 0

[tor/master] Do not leak a reference to "slot" when decoding private key.
by nickm＠torproject.org 05 Sep '18

05 Sep '18

commit f46a7eafb88494922f331d198500f94dee0a6d8e Author: Nick Mathewson <nickm(a)torproject.org> Date: Wed Aug 29 15:17:47 2018 -0400 Do not leak a reference to "slot" when decoding private key. --- src/lib/crypt_ops/crypto_rsa_nss.c | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-) diff --git a/src/lib/crypt_ops/crypto_rsa_nss.c b/src/lib/crypt_ops/crypto_rsa_nss.c index 517faa5c7..b6d8bb647 100644 --- a/src/lib/crypt_ops/crypto_rsa_nss.c +++ b/src/lib/crypt_ops/crypto_rsa_nss.c @@ -683,6 +683,9 @@ crypto_pk_asn1_decode_private(const char *str, size_t len) { tor_assert(str); tor_assert(len < INT_MAX); + PK11SlotInfo *slot = PK11_GetBestSlot(CKM_RSA_PKCS, NULL); + if (!slot) + return NULL; SECKEYPrivateKeyInfo info = { .algorithm = { @@ -699,7 +702,6 @@ crypto_pk_asn1_decode_private(const char *str, size_t len) } }; - PK11SlotInfo *slot = PK11_GetBestSlot(CKM_RSA_PKCS, NULL); SECStatus s; SECKEYPrivateKey *seckey = NULL; @@ -722,5 +724,8 @@ crypto_pk_asn1_decode_private(const char *str, size_t len) crypto_nss_log_errors(LOG_WARN, "decoding an RSA private key"); } + if (slot) + PK11_FreeSlot(slot); + return output; }

1 0

[tor/master] Do not leave a certificate allocated after testing dirvote_add()
by nickm＠torproject.org 05 Sep '18

05 Sep '18

commit b892133fb9307aba2b9e7931472b9ad9a1628bb2 Author: Nick Mathewson <nickm(a)torproject.org> Date: Wed Aug 29 16:00:49 2018 -0400 Do not leave a certificate allocated after testing dirvote_add() --- src/test/test_dir_handle_get.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/test/test_dir_handle_get.c b/src/test/test_dir_handle_get.c index 571b0386e..09799a0e5 100644 --- a/src/test/test_dir_handle_get.c +++ b/src/test/test_dir_handle_get.c @@ -2102,6 +2102,7 @@ test_dir_handle_get_status_vote_d(void* data) clear_dir_servers(); dirvote_free_all(); + routerlist_free_all(); } static void @@ -2638,4 +2639,3 @@ struct testcase_t dir_handle_get_tests[] = { DIR_HANDLE_CMD(parse_accept_encoding, 0), END_OF_TESTCASES }; -

1 0

[tor/master] Make some additional RSA functions const
by nickm＠torproject.org 05 Sep '18

05 Sep '18

commit edbb5ef5b258db15459407f8964b83b6dd4c3d22 Author: Nick Mathewson <nickm(a)torproject.org> Date: Tue Sep 4 09:51:03 2018 -0400 Make some additional RSA functions const --- src/lib/crypt_ops/crypto_rsa.h | 4 ++-- src/lib/crypt_ops/crypto_rsa_nss.c | 4 ++-- src/lib/crypt_ops/crypto_rsa_openssl.c | 4 ++-- 3 files changed, 6 insertions(+), 6 deletions(-) diff --git a/src/lib/crypt_ops/crypto_rsa.h b/src/lib/crypt_ops/crypto_rsa.h index dd25bc31b..007964b26 100644 --- a/src/lib/crypt_ops/crypto_rsa.h +++ b/src/lib/crypt_ops/crypto_rsa.h @@ -64,7 +64,7 @@ int crypto_pk_read_private_key_from_string(crypto_pk_t *env, int crypto_pk_write_private_key_to_filename(crypto_pk_t *env, const char *fname); -int crypto_pk_is_valid_private_key(crypto_pk_t *env); +int crypto_pk_is_valid_private_key(const crypto_pk_t *env); int crypto_pk_cmp_keys(const crypto_pk_t *a, const crypto_pk_t *b); int crypto_pk_eq_keys(const crypto_pk_t *a, const crypto_pk_t *b); size_t crypto_pk_keysize(const crypto_pk_t *env); @@ -72,7 +72,7 @@ int crypto_pk_num_bits(crypto_pk_t *env); crypto_pk_t *crypto_pk_dup_key(crypto_pk_t *orig); crypto_pk_t *crypto_pk_copy_full(crypto_pk_t *orig); int crypto_pk_key_is_private(const crypto_pk_t *key); -int crypto_pk_public_exponent_ok(crypto_pk_t *env); +int crypto_pk_public_exponent_ok(const crypto_pk_t *env); int crypto_pk_obsolete_public_hybrid_encrypt(crypto_pk_t *env, char *to, size_t tolen, const char *from, size_t fromlen, diff --git a/src/lib/crypt_ops/crypto_rsa_nss.c b/src/lib/crypt_ops/crypto_rsa_nss.c index 90e9c34d3..dc282d7c9 100644 --- a/src/lib/crypt_ops/crypto_rsa_nss.c +++ b/src/lib/crypt_ops/crypto_rsa_nss.c @@ -247,7 +247,7 @@ crypto_pk_generate_key_with_bits,(crypto_pk_t *key, int bits)) /** Return true iff env is a valid private key. */ int -crypto_pk_is_valid_private_key(crypto_pk_t *key) +crypto_pk_is_valid_private_key(const crypto_pk_t *key) { /* We don't need to do validation here, since unlike OpenSSL, NSS won't let * us load private keys without validating them. */ @@ -258,7 +258,7 @@ crypto_pk_is_valid_private_key(crypto_pk_t *key) * equals 65537. */ int -crypto_pk_public_exponent_ok(crypto_pk_t *key) +crypto_pk_public_exponent_ok(const crypto_pk_t *key) { return key && key->pubkey && diff --git a/src/lib/crypt_ops/crypto_rsa_openssl.c b/src/lib/crypt_ops/crypto_rsa_openssl.c index f203a0683..df81c963e 100644 --- a/src/lib/crypt_ops/crypto_rsa_openssl.c +++ b/src/lib/crypt_ops/crypto_rsa_openssl.c @@ -186,7 +186,7 @@ crypto_pk_generate_key_with_bits,(crypto_pk_t *env, int bits)) /** Return true if env has a valid key; false otherwise. */ int -crypto_pk_is_valid_private_key(crypto_pk_t *env) +crypto_pk_is_valid_private_key(const crypto_pk_t *env) { int r; tor_assert(env); @@ -204,7 +204,7 @@ crypto_pk_is_valid_private_key(crypto_pk_t *env) * equals TOR_RSA_EXPONENT. */ int -crypto_pk_public_exponent_ok(crypto_pk_t *env) +crypto_pk_public_exponent_ok(const crypto_pk_t *env) { tor_assert(env); tor_assert(env->key);

1 0