commit 5bcf9522618307ca4829ee7349a482a869766d6e Author: Nick Mathewson <nickm@torproject.org> Date: Sun Nov 2 13:04:44 2014 -0500 Check more thoroughly for unlogged OpenSSL errors --- changes/bug13319 | 4 ++++ src/common/tortls.c | 55 ++++++++++++++++++++++++++++++++++++++++----------- 2 files changed, 47 insertions(+), 12 deletions(-) diff --git a/changes/bug13319 b/changes/bug13319 new file mode 100644 index 0000000..eee95c8 --- /dev/null +++ b/changes/bug13319 @@ -0,0 +1,4 @@ + o Minor bugfixes: + - Check more thoroughly throughout the TLS code for possible unlogged + TLS errors. Possible diagnostic or fix for bug 13319. + diff --git a/src/common/tortls.c b/src/common/tortls.c index cca2d42..4f588de 100644 --- a/src/common/tortls.c +++ b/src/common/tortls.c @@ -452,6 +452,8 @@ tor_tls_get_error(tor_tls_t *tls, int r, int extra, static void tor_tls_init(void) { + check_no_tls_errors(); + if (!tls_library_is_initialized) { long version; SSL_library_init(); @@ -550,6 +552,8 @@ tor_tls_init(void) void tor_tls_free_all(void) { + check_no_tls_errors(); + if (server_tls_context) { tor_tls_context_t *ctx = server_tls_context; server_tls_context = NULL; @@ -861,29 +865,33 @@ tor_cert_decode(const uint8_t *certificate, size_t certificate_len) const unsigned char *cp = (const unsigned char *)certificate; tor_cert_t *newcert; tor_assert(certificate); + check_no_tls_errors(); if (certificate_len > INT_MAX) - return NULL; + goto err; x509 = d2i_X509(NULL, &cp, (int)certificate_len); if (!x509) - return NULL; /* Couldn't decode */ + goto err; /* Couldn't decode */ if (cp - certificate != (int)certificate_len) { X509_free(x509); - return NULL; /* Didn't use all the bytes */ + goto err; /* Didn't use all the bytes */ } newcert = tor_cert_new(x509); if (!newcert) { - return NULL; + goto err; } if (newcert->encoded_len != certificate_len || fast_memneq(newcert->encoded, certificate, certificate_len)) { /* Cert wasn't in DER */ tor_cert_free(newcert); - return NULL; + goto err; } return newcert; + err: + tls_log_errors(NULL, LOG_INFO, LD_CRYPTO, "decoding a certificate"); + return NULL; } /** Set *<b>encoded_out</b> and *<b>size_out</b> to <b>cert</b>'s encoded DER @@ -1025,21 +1033,24 @@ tor_tls_cert_is_valid(int severity, const tor_cert_t *signing_cert, int check_rsa_1024) { + check_no_tls_errors(); + EVP_PKEY *cert_key; EVP_PKEY *signing_key = X509_get_pubkey(signing_cert->cert); int r, key_ok = 0; + if (!signing_key) - return 0; + goto bad; r = X509_verify(cert->cert, signing_key); EVP_PKEY_free(signing_key); if (r <= 0) - return 0; + goto bad; /* okay, the signature checked out right. Now let's check the check the * lifetime. */ if (check_cert_lifetime_internal(severity, cert->cert, 48*60*60, 30*24*60*60) < 0) - return 0; + goto bad; cert_key = X509_get_pubkey(cert->cert); if (check_rsa_1024 && cert_key) { @@ -1059,11 +1070,14 @@ tor_tls_cert_is_valid(int severity, } EVP_PKEY_free(cert_key); if (!key_ok) - return 0; + goto bad; /* XXXX compare DNs or anything? */ return 1; + bad: + tls_log_errors(NULL, LOG_INFO, LD_CRYPTO, "checking a certificate"); + return 0; } /** Increase the reference count of <b>ctx</b>. */ @@ -1090,6 +1104,7 @@ tor_tls_context_init(unsigned flags, int rv1 = 0; int rv2 = 0; const int is_public_server = flags & TOR_TLS_CTX_IS_PUBLIC_SERVER; + check_no_tls_errors(); if (is_public_server) { tor_tls_context_t *new_ctx; @@ -1134,6 +1149,7 @@ tor_tls_context_init(unsigned flags, 1); } + tls_log_errors(NULL, LOG_WARN, LD_CRYPTO, "constructing a TLS context"); return MIN(rv1, rv2); } @@ -1857,11 +1873,12 @@ tor_tls_new(int sock, int isServer) client_tls_context; result->magic = TOR_TLS_MAGIC; + check_no_tls_errors(); tor_assert(context); /* make sure somebody made it first */ if (!(result->ssl = SSL_new(context->ctx))) { tls_log_errors(NULL, LOG_WARN, LD_NET, "creating SSL object"); tor_free(result); - return NULL; + goto err; } #ifdef SSL_set_tlsext_host_name @@ -1881,7 +1898,7 @@ tor_tls_new(int sock, int isServer) #endif SSL_free(result->ssl); tor_free(result); - return NULL; + goto err; } if (!isServer) rectify_client_ciphers(&result->ssl->cipher_list); @@ -1894,7 +1911,7 @@ tor_tls_new(int sock, int isServer) #endif SSL_free(result->ssl); tor_free(result); - return NULL; + goto err; } { int set_worked = @@ -1928,6 +1945,10 @@ tor_tls_new(int sock, int isServer) if (isServer) tor_tls_setup_session_secret_cb(result); + goto done; + err: + result = NULL; + done: /* Not expected to get called. */ tls_log_errors(NULL, LOG_WARN, LD_NET, "creating tor_tls_t object"); return result; @@ -2175,6 +2196,7 @@ int tor_tls_finish_handshake(tor_tls_t *tls) { int r = TOR_TLS_DONE; + check_no_tls_errors(); if (tls->isServer) { SSL_set_info_callback(tls->ssl, NULL); SSL_set_verify(tls->ssl, SSL_VERIFY_PEER, always_accept_verify_cb); @@ -2220,6 +2242,7 @@ tor_tls_finish_handshake(tor_tls_t *tls) r = TOR_TLS_ERROR_MISC; } } + tls_log_errors(NULL, LOG_WARN, LD_NET, "finishing the handshake"); return r; } @@ -2250,6 +2273,8 @@ tor_tls_renegotiate(tor_tls_t *tls) /* We could do server-initiated renegotiation too, but that would be tricky. * Instead of "SSL_renegotiate, then SSL_do_handshake until done" */ tor_assert(!tls->isServer); + + check_no_tls_errors(); if (tls->state != TOR_TLS_ST_RENEGOTIATE) { int r = SSL_renegotiate(tls->ssl); if (r <= 0) { @@ -2278,6 +2303,7 @@ tor_tls_shutdown(tor_tls_t *tls) char buf[128]; tor_assert(tls); tor_assert(tls->ssl); + check_no_tls_errors(); while (1) { if (tls->state == TOR_TLS_ST_SENTCLOSE) { @@ -2465,6 +2491,7 @@ tor_tls_verify(int severity, tor_tls_t *tls, crypto_pk_t **identity_key) RSA *rsa; int r = -1; + check_no_tls_errors(); *identity_key = NULL; try_to_extract_certs_from_tls(severity, tls, &cert, &id_cert); @@ -2705,6 +2732,8 @@ dn_indicates_v3_cert(X509_NAME *name) int tor_tls_received_v3_certificate(tor_tls_t *tls) { + check_no_tls_errors(); + X509 *cert = SSL_get_peer_certificate(tls->ssl); EVP_PKEY *key = NULL; X509_NAME *issuer_name, *subject_name; @@ -2737,6 +2766,8 @@ tor_tls_received_v3_certificate(tor_tls_t *tls) } done: + tls_log_errors(tls, LOG_WARN, LD_NET, "checking for a v3 cert"); + if (key) EVP_PKEY_free(key); if (cert)