From d486c6cf74db6bce80689c17c7427a6e3a055f93 Mon Sep 17 00:00:00 2001 From: Nick Mathewson Date: Fri, 26 Jun 2020 10:45:42 -0400 Subject: [PATCH 1/5] Store errors from TLS write and handshake in the tls_error field. Previously, we would only update this field when the error happened during a read. This will improves our reporting for our bootstrap status, and help to address #32622. The problem is not completely solved by this patch, however: too many errors are still lumped into "MISC". --- src/core/mainloop/connection.c | 1 + src/core/or/connection_or.c | 4 +++- 2 files changed, 4 insertions(+), 1 deletion(-) diff --git a/src/core/mainloop/connection.c b/src/core/mainloop/connection.c index a8417e46d9e..792cdbf6869 100644 --- a/src/core/mainloop/connection.c +++ b/src/core/mainloop/connection.c @@ -4205,6 +4205,7 @@ connection_handle_write_impl(connection_t *conn, int force) switch (result) { CASE_TOR_TLS_ERROR_ANY: case TOR_TLS_CLOSE: + or_conn->tls_error = result; log_info(LD_NET, result != TOR_TLS_CLOSE ? "tls error. breaking.":"TLS connection closed on flush"); /* Don't flush; connection is dead. */ diff --git a/src/core/or/connection_or.c b/src/core/or/connection_or.c index 5d71b363f86..4cb83f45a9e 100644 --- a/src/core/or/connection_or.c +++ b/src/core/or/connection_or.c @@ -1692,7 +1692,8 @@ connection_tls_continue_handshake(or_connection_t *conn) switch (result) { CASE_TOR_TLS_ERROR_ANY: - log_info(LD_OR,"tls error [%s]. breaking connection.", + conn->tls_error = result; + log_info(LD_OR,"tls error [%s]. breaking connection.", tor_tls_err_to_string(result)); return -1; case TOR_TLS_DONE: @@ -1724,6 +1725,7 @@ connection_tls_continue_handshake(or_connection_t *conn) log_debug(LD_OR,"wanted read"); return 0; case TOR_TLS_CLOSE: + conn->tls_error = result; log_info(LD_OR,"tls closed. breaking connection."); return -1; } From e429ceb2667787750af070929d0e64cbc7a6dda4 Mon Sep 17 00:00:00 2001 From: Nick Mathewson Date: Fri, 26 Jun 2020 10:59:59 -0400 Subject: [PATCH 2/5] Add a TLS_ERROR bootstrap failure reason. If our TLS connection fails for a "misc" reason, we don't need to say that the reason is "misc" -- we can at least localize it to the TLS module. Part of a fix for #32622. --- src/core/or/or.h | 3 ++- src/core/or/reasons.c | 4 ++++ 2 files changed, 6 insertions(+), 1 deletion(-) diff --git a/src/core/or/or.h b/src/core/or/or.h index 5b35cbe7f1e..8758a2ec6f6 100644 --- a/src/core/or/or.h +++ b/src/core/or/or.h @@ -220,7 +220,8 @@ struct curve25519_public_key_t; #define END_OR_CONN_REASON_IO_ERROR 7 /* read/write error */ #define END_OR_CONN_REASON_RESOURCE_LIMIT 8 /* sockets, buffers, etc */ #define END_OR_CONN_REASON_PT_MISSING 9 /* PT failed or not available */ -#define END_OR_CONN_REASON_MISC 10 +#define END_OR_CONN_REASON_TLS_ERROR 10 /* Problem in TLS protocol */ +#define END_OR_CONN_REASON_MISC 11 /* Reasons why we (or a remote OR) might close a stream. See tor-spec.txt for * documentation of these. The values must match. */ diff --git a/src/core/or/reasons.c b/src/core/or/reasons.c index 7da7843cab8..708f43a6899 100644 --- a/src/core/or/reasons.c +++ b/src/core/or/reasons.c @@ -244,6 +244,8 @@ orconn_end_reason_to_control_string(int r) return "IOERROR"; case END_OR_CONN_REASON_RESOURCE_LIMIT: return "RESOURCELIMIT"; + case END_OR_CONN_REASON_TLS_ERROR: + return "TLS_ERROR"; case END_OR_CONN_REASON_MISC: return "MISC"; case END_OR_CONN_REASON_PT_MISSING: @@ -276,6 +278,8 @@ tls_error_to_orconn_end_reason(int e) case TOR_TLS_CLOSE: case TOR_TLS_DONE: return END_OR_CONN_REASON_DONE; + case TOR_TLS_ERROR_MISC: + return END_OR_CONN_REASON_TLS_ERROR; default: return END_OR_CONN_REASON_MISC; } From 5127880aa10c56a45a4e96820501f9611483b464 Mon Sep 17 00:00:00 2001 From: Nick Mathewson Date: Fri, 26 Jun 2020 11:18:38 -0400 Subject: [PATCH 3/5] Carry TLS error strings forward to controller when reporting them. Now instead of saying "DONE, DONE" or "MISC, MISC" or "TLS_ERROR, TLS_ERROR", we can finally give a nice sensible "TLS_ERROR, wrong version number" which should help debug a great deal. Closes ticket 32622. --- src/core/or/connection_or.c | 14 ++++++++++---- src/lib/tls/tortls.h | 1 + src/lib/tls/tortls_nss.c | 13 +++++++++++++ src/lib/tls/tortls_openssl.c | 20 ++++++++++++++++++++ src/lib/tls/tortls_st.h | 3 +++ 5 files changed, 47 insertions(+), 4 deletions(-) diff --git a/src/core/or/connection_or.c b/src/core/or/connection_or.c index 4cb83f45a9e..b88d1b6afb3 100644 --- a/src/core/or/connection_or.c +++ b/src/core/or/connection_or.c @@ -745,10 +745,16 @@ connection_or_about_to_close(or_connection_t *or_conn) int reason = tls_error_to_orconn_end_reason(or_conn->tls_error); connection_or_event_status(or_conn, OR_CONN_EVENT_FAILED, reason); - if (!authdir_mode_tests_reachability(options)) - control_event_bootstrap_prob_or( - orconn_end_reason_to_control_string(reason), - reason, or_conn); + if (!authdir_mode_tests_reachability(options)) { + const char *warning = NULL; + if (reason == END_OR_CONN_REASON_TLS_ERROR && or_conn->tls) { + warning = tor_tls_get_last_error_msg(or_conn->tls); + } + if (warning == NULL) { + warning = orconn_end_reason_to_control_string(reason); + } + control_event_bootstrap_prob_or(warning, reason, or_conn); + } } } } else if (conn->hold_open_until_flushed) { diff --git a/src/lib/tls/tortls.h b/src/lib/tls/tortls.h index e8dbbf5279b..517cdc17dd8 100644 --- a/src/lib/tls/tortls.h +++ b/src/lib/tls/tortls.h @@ -81,6 +81,7 @@ void tor_tls_free_all(void); void tor_tls_init(void); void tls_log_errors(tor_tls_t *tls, int severity, int domain, const char *doing); +const char *tor_tls_get_last_error_msg(const tor_tls_t *tls); int tor_tls_context_init(unsigned flags, crypto_pk_t *client_identity, crypto_pk_t *server_identity, diff --git a/src/lib/tls/tortls_nss.c b/src/lib/tls/tortls_nss.c index 62e82621159..84f7e3c1569 100644 --- a/src/lib/tls/tortls_nss.c +++ b/src/lib/tls/tortls_nss.c @@ -369,6 +369,8 @@ tls_log_errors(tor_tls_t *tls, int severity, int domain, (void)tls; PRErrorCode code = PORT_GetError(); + if (tls) + tls->last_error = code; const char *addr = tls ? tls->address : NULL; const char *string = PORT_ErrorToString(code); @@ -391,6 +393,17 @@ tls_log_errors(tor_tls_t *tls, int severity, int domain, with, addr); } } +const char * +tor_tls_get_last_error_msg(const tor_tls_t *tls) +{ + IF_BUG_ONCE(!tls) { + return NULL; + } + if (tls->last_error == 0) { + return NULL; + } + return PORT_ErrorToString(tls->last_error); +} tor_tls_t * tor_tls_new(tor_socket_t sock, int is_server) diff --git a/src/lib/tls/tortls_openssl.c b/src/lib/tls/tortls_openssl.c index 68d6e2aa505..22697141412 100644 --- a/src/lib/tls/tortls_openssl.c +++ b/src/lib/tls/tortls_openssl.c @@ -245,10 +245,30 @@ tls_log_errors(tor_tls_t *tls, int severity, int domain, const char *doing) unsigned long err; while ((err = ERR_get_error()) != 0) { + if (tls) + tls->last_error = err; tor_tls_log_one_error(tls, err, severity, domain, doing); } } +/** + * Return a string representing more detail about the last error received + * on TLS. + * + * May return null if no error was found. + **/ +const char * +tor_tls_get_last_error_msg(const tor_tls_t *tls) +{ + IF_BUG_ONCE(!tls) { + return NULL; + } + if (tls->last_error == 0) { + return NULL; + } + return (const char*)ERR_reason_error_string(tls->last_error); +} + #define CATCH_SYSCALL 1 #define CATCH_ZERO 2 diff --git a/src/lib/tls/tortls_st.h b/src/lib/tls/tortls_st.h index 925896d4934..922ad2f08b9 100644 --- a/src/lib/tls/tortls_st.h +++ b/src/lib/tls/tortls_st.h @@ -67,6 +67,8 @@ struct tor_tls_t { */ unsigned long last_write_count; unsigned long last_read_count; + /** Most recent error value from ERR_get_error(). */ + unsigned long last_error; /** If set, a callback to invoke whenever the client tries to renegotiate * the handshake. */ void (*negotiated_callback)(tor_tls_t *tls, void *arg); @@ -77,6 +79,7 @@ struct tor_tls_t { /** Last values retried from tor_get_prfiledesc_byte_counts(). */ uint64_t last_write_count; uint64_t last_read_count; + PRErrorCode last_error; #endif }; From 010dfdf834a642d749e0ab9944fc0005ddaf4126 Mon Sep 17 00:00:00 2001 From: Nick Mathewson Date: Fri, 26 Jun 2020 11:21:53 -0400 Subject: [PATCH 4/5] Changes file for #32622. --- changes/ticket32622 | 5 +++++ 1 file changed, 5 insertions(+) create mode 100644 changes/ticket32622 diff --git a/changes/ticket32622 b/changes/ticket32622 new file mode 100644 index 00000000000..1c663567fdc --- /dev/null +++ b/changes/ticket32622 @@ -0,0 +1,5 @@ + o Minor features (bootstrap reporting): + - Report more detailed reasons for bootstrap failure when the failure + happens due to a TLS error. Previously we would just call these errors + "MISC" when they happened during read, and "DONE" when they + happened during any other TLS operation. Closes ticket 32622. From 87f19c2d6b32da576bbc4e900897e2b70db127fc Mon Sep 17 00:00:00 2001 From: Nick Mathewson Date: Fri, 26 Jun 2020 15:01:17 -0400 Subject: [PATCH 5/5] fixup! Carry TLS error strings forward to controller when reporting them. Avoid using PRErrorCode in a header, since we try not to use NSPR headers outside of the crypto layer. --- src/lib/tls/tortls_nss.c | 2 +- src/lib/tls/tortls_st.h | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/src/lib/tls/tortls_nss.c b/src/lib/tls/tortls_nss.c index 84f7e3c1569..a9ec731c0a9 100644 --- a/src/lib/tls/tortls_nss.c +++ b/src/lib/tls/tortls_nss.c @@ -402,7 +402,7 @@ tor_tls_get_last_error_msg(const tor_tls_t *tls) if (tls->last_error == 0) { return NULL; } - return PORT_ErrorToString(tls->last_error); + return PORT_ErrorToString((PRErrorCode)tls->last_error); } tor_tls_t * diff --git a/src/lib/tls/tortls_st.h b/src/lib/tls/tortls_st.h index 922ad2f08b9..34abe52ee31 100644 --- a/src/lib/tls/tortls_st.h +++ b/src/lib/tls/tortls_st.h @@ -79,7 +79,7 @@ struct tor_tls_t { /** Last values retried from tor_get_prfiledesc_byte_counts(). */ uint64_t last_write_count; uint64_t last_read_count; - PRErrorCode last_error; + long last_error; #endif };