From 9916a76026270737d179f9348b30b7617c58347d Mon Sep 17 00:00:00 2001 From: Hugo Landau Date: Thu, 3 Aug 2023 11:56:12 +0100 Subject: [PATCH] QUIC TLS: Rethink error handling --- crypto/err/err_mark.c | 19 ++++++++ crypto/err/err_save.c | 65 +++++++++++++++++++++++++++ crypto/err/openssl.txt | 1 + doc/man3/ERR_set_mark.pod | 12 ++++- doc/man3/OSSL_ERR_STATE_save.pod | 25 ++++++++--- include/internal/quic_channel.h | 12 +++++ include/internal/quic_tls.h | 4 +- include/internal/ssl.h | 2 + include/openssl/err.h.in | 2 + include/openssl/sslerr.h | 1 + ssl/quic/quic_channel.c | 24 +++++----- ssl/quic/quic_tls.c | 77 +++++++++++++++++++++++++------- ssl/ssl_err.c | 2 + ssl/ssl_lib.c | 7 ++- util/libcrypto.num | 2 + 15 files changed, 217 insertions(+), 38 deletions(-) diff --git a/crypto/err/err_mark.c b/crypto/err/err_mark.c index cc1baba7a389d0..1395e944dd23df 100644 --- a/crypto/err/err_mark.c +++ b/crypto/err/err_mark.c @@ -46,6 +46,25 @@ int ERR_pop_to_mark(void) return 1; } +int ERR_count_to_mark(void) +{ + ERR_STATE *es; + int count = 0, top; + + es = ossl_err_get_state_int(); + if (es == NULL) + return 0; + + top = es->top; + while (es->bottom != top + && es->err_marks[top] == 0) { + ++count; + top = top > 0 ? top - 1 : ERR_NUM_ERRORS - 1; + } + + return count; +} + int ERR_clear_last_mark(void) { ERR_STATE *es; diff --git a/crypto/err/err_save.c b/crypto/err/err_save.c index a471b22682ad92..3ca059adc336b0 100644 --- a/crypto/err/err_save.c +++ b/crypto/err/err_save.c @@ -43,6 +43,71 @@ void OSSL_ERR_STATE_save(ERR_STATE *es) memset(thread_es, 0, sizeof(*thread_es)); } +void OSSL_ERR_STATE_save_to_mark(ERR_STATE *es) +{ + size_t i, j, count; + int top; + ERR_STATE *thread_es; + + if (es == NULL) + return; + + thread_es = ossl_err_get_state_int(); + if (thread_es == NULL) { + for (i = 0; i < ERR_NUM_ERRORS; ++i) + err_clear(es, i, 1); + + es->top = es->bottom = 0; + return; + } + + /* Determine number of errors we are going to move. */ + for (count = 0, top = thread_es->top; + thread_es->bottom != top + && thread_es->err_marks[top] == 0; + ++count) + top = top > 0 ? top - 1 : ERR_NUM_ERRORS - 1; + + /* Move the errors, preserving order. */ + for (i = 0, j = top; i < count; ++i) { + j = (j + 1) % ERR_NUM_ERRORS; + + err_clear(es, i, 1); + + /* Move the error entry to the given ERR_STATE. */ + es->err_flags[i] = thread_es->err_flags[j]; + es->err_marks[i] = 0; + es->err_buffer[i] = thread_es->err_buffer[j]; + es->err_data[i] = thread_es->err_data[j]; + es->err_data_size[i] = thread_es->err_data_size[j]; + es->err_data_flags[i] = thread_es->err_data_flags[j]; + es->err_file[i] = thread_es->err_file[j]; + es->err_line[i] = thread_es->err_line[j]; + es->err_func[i] = thread_es->err_func[j]; + + thread_es->err_flags[j] = 0; + thread_es->err_buffer[j] = 0; + thread_es->err_data[j] = NULL; + thread_es->err_data_size[j] = 0; + thread_es->err_file[j] = NULL; + thread_es->err_line[j] = 0; + thread_es->err_func[j] = NULL; + } + + if (i > 0) { + /* If we moved anything, es's stack always starts at [0]. */ + es->top = i - 1; + es->bottom = ERR_NUM_ERRORS - 1; + } else { + /* Didn't move anything - empty stack */ + es->top = es->bottom = 0; + } + + /* Erase extra space as a precaution. */ + for (; i < ERR_NUM_ERRORS; ++i) + err_clear(es, i, 1); +} + void OSSL_ERR_STATE_restore(const ERR_STATE *es) { size_t i; diff --git a/crypto/err/openssl.txt b/crypto/err/openssl.txt index 0be223b974f590..70c1645a4f82d9 100644 --- a/crypto/err/openssl.txt +++ b/crypto/err/openssl.txt @@ -1498,6 +1498,7 @@ SSL_R_PROTOCOL_IS_SHUTDOWN:207:protocol is shutdown SSL_R_PSK_IDENTITY_NOT_FOUND:223:psk identity not found SSL_R_PSK_NO_CLIENT_CB:224:psk no client cb SSL_R_PSK_NO_SERVER_CB:225:psk no server cb +SSL_R_QUIC_HANDSHAKE_LAYER_ERROR:393:quic handshake layer error SSL_R_QUIC_NETWORK_ERROR:387:quic network error SSL_R_QUIC_PROTOCOL_ERROR:382:quic protocol error SSL_R_READ_BIO_NOT_SET:211:read bio not set diff --git a/doc/man3/ERR_set_mark.pod b/doc/man3/ERR_set_mark.pod index e61d5c61edd402..add9b232c09df0 100644 --- a/doc/man3/ERR_set_mark.pod +++ b/doc/man3/ERR_set_mark.pod @@ -2,8 +2,8 @@ =head1 NAME -ERR_set_mark, ERR_clear_last_mark, ERR_pop_to_mark -- set mark, clear mark and pop errors until mark +ERR_set_mark, ERR_clear_last_mark, ERR_pop_to_mark, ERR_count_to_mark - set +mark, clear mark and pop errors until mark =head1 SYNOPSIS @@ -12,6 +12,7 @@ ERR_set_mark, ERR_clear_last_mark, ERR_pop_to_mark int ERR_set_mark(void); int ERR_pop_to_mark(void); int ERR_clear_last_mark(void); + int ERR_count_to_mark(void); =head1 DESCRIPTION @@ -23,6 +24,10 @@ The mark is then removed. If there is no mark, the whole stack is removed. ERR_clear_last_mark() removes the last mark added if there is one. +ERR_count_to_mark() returns the number of entries on the error stack above the +most recently marked entry, not including that entry. If there is no mark in the +error stack, the number of entries in the error stack is returned. + =head1 RETURN VALUES ERR_set_mark() returns 0 if the error stack is empty, otherwise 1. @@ -30,6 +35,9 @@ ERR_set_mark() returns 0 if the error stack is empty, otherwise 1. ERR_clear_last_mark() and ERR_pop_to_mark() return 0 if there was no mark in the error stack, which implies that the stack became empty, otherwise 1. +ERR_count_to_mark() returns the number of error stack entries found above the +most recent mark, if any, or the total number of error stack entries. + =head1 COPYRIGHT Copyright 2003-2021 The OpenSSL Project Authors. All Rights Reserved. diff --git a/doc/man3/OSSL_ERR_STATE_save.pod b/doc/man3/OSSL_ERR_STATE_save.pod index 79f3aba5f84bba..05c738fc552a33 100644 --- a/doc/man3/OSSL_ERR_STATE_save.pod +++ b/doc/man3/OSSL_ERR_STATE_save.pod @@ -2,8 +2,8 @@ =head1 NAME -OSSL_ERR_STATE_new, OSSL_ERR_STATE_save, OSSL_ERR_STATE_restore, -OSSL_ERR_STATE_free - saving and restoring error state +OSSL_ERR_STATE_new, OSSL_ERR_STATE_save, OSSL_ERR_STATE_save_to_mark, +OSSL_ERR_STATE_restore, OSSL_ERR_STATE_free - saving and restoring error state =head1 SYNOPSIS @@ -11,6 +11,7 @@ OSSL_ERR_STATE_free - saving and restoring error state ERR_STATE *OSSL_ERR_STATE_new(void); void OSSL_ERR_STATE_save(ERR_STATE *es); + void OSSL_ERR_STATE_save_to_mark(ERR_STATE *es); void OSSL_ERR_STATE_restore(const ERR_STATE *es); void OSSL_ERR_STATE_free(ERR_STATE *es); @@ -26,6 +27,18 @@ OSSL_ERR_STATE_save() saves the thread error state to I. It subsequently clears the thread error state. Any previously saved state in I is cleared prior to saving the new state. +OSSL_ERR_STATE_save_to_mark() is similar to OSSL_ERR_STATE_save() but only saves +ERR entries up to the most recent mark on the ERR stack. These entries are moved +to I and removed from the thread error state. However, the most recent +marked ERR and any ERR state before it remains part of the thread error state +and is not moved to the ERR_STATE. The mark is not cleared and must be cleared +explicitly after a call to this function using L or +L. (Since a call to OSSL_ERR_STATE_save_to_mark() leaves +the marked ERR as the top error, either of these functions will have the same +effect.) If there is no marked ERR in the thread local error state, all ERR +entries are copied and the effect is the same as for a call to +OSSL_ERR_STATE_save(). + OSSL_ERR_STATE_restore() adds all the error entries from the saved state I to the thread error state. Existing entries in the thread error state are not affected if there is enough space @@ -39,13 +52,13 @@ OSSL_ERR_STATE_free() frees the saved error state I. OSSL_ERR_STATE_new() returns a pointer to the allocated ERR_STATE structure or NULL on error. -OSSL_ERR_STATE_save(), OSSL_ERR_STATE_restore(), OSSL_ERR_STATE_free() -do not return any values. +OSSL_ERR_STATE_save(), OSSL_ERR_STATE_save_to_mark(), OSSL_ERR_STATE_restore(), +OSSL_ERR_STATE_free() do not return any values. =head1 NOTES -OSSL_ERR_STATE_save() cannot fail as it takes over any allocated -data from the thread error state. +OSSL_ERR_STATE_save() and OSSL_ERR_STATE_save_to_mark() cannot fail as it takes +over any allocated data from the thread error state. OSSL_ERR_STATE_restore() is a best effort function. The only failure that can happen during its operation is when memory allocation fails. diff --git a/include/internal/quic_channel.h b/include/internal/quic_channel.h index 98546d462e63b1..46fca2eb21140a 100644 --- a/include/internal/quic_channel.h +++ b/include/internal/quic_channel.h @@ -221,6 +221,7 @@ void ossl_quic_channel_raise_protocol_error_loc(QUIC_CHANNEL *ch, uint64_t error_code, uint64_t frame_type, const char *reason, + ERR_STATE *err_state, const char *src_file, int src_line, const char *src_func); @@ -229,10 +230,21 @@ void ossl_quic_channel_raise_protocol_error_loc(QUIC_CHANNEL *ch, ossl_quic_channel_raise_protocol_error_loc((ch), (error_code), \ (frame_type), \ (reason), \ + NULL, \ OPENSSL_FILE, \ OPENSSL_LINE, \ OPENSSL_FUNC) +#define ossl_quic_channel_raise_protocol_error_state(ch, error_code, frame_type, reason, state) \ + ossl_quic_channel_raise_protocol_error_loc((ch), (error_code), \ + (frame_type), \ + (reason), \ + (state), \ + OPENSSL_FILE, \ + OPENSSL_LINE, \ + OPENSSL_FUNC) + + /* * Returns 1 if permanent net error was detected on the QUIC_CHANNEL, * 0 otherwise. diff --git a/include/internal/quic_tls.h b/include/internal/quic_tls.h index 13da5882c96504..770c698d315ddb 100644 --- a/include/internal/quic_tls.h +++ b/include/internal/quic_tls.h @@ -98,8 +98,6 @@ int ossl_quic_tls_set_transport_params(QUIC_TLS *qtls, int ossl_quic_tls_get_error(QUIC_TLS *qtls, uint64_t *error_code, const char **error_msg, - const char **error_src_file, - int *error_src_line, - const char **error_src_func); + ERR_STATE **error_state); #endif diff --git a/include/internal/ssl.h b/include/internal/ssl.h index 3bf9bf9e34d09d..46146a9e7ebf86 100644 --- a/include/internal/ssl.h +++ b/include/internal/ssl.h @@ -16,4 +16,6 @@ typedef void (*ossl_msg_cb)(int write_p, int version, int content_type, const void *buf, size_t len, SSL *ssl, void *arg); +int ossl_ssl_get_error(const SSL *s, int i, int check_err); + #endif diff --git a/include/openssl/err.h.in b/include/openssl/err.h.in index 075d683f8d4ac3..a28afa885fe232 100644 --- a/include/openssl/err.h.in +++ b/include/openssl/err.h.in @@ -485,9 +485,11 @@ int ERR_get_next_error_library(void); int ERR_set_mark(void); int ERR_pop_to_mark(void); int ERR_clear_last_mark(void); +int ERR_count_to_mark(void); ERR_STATE *OSSL_ERR_STATE_new(void); void OSSL_ERR_STATE_save(ERR_STATE *es); +void OSSL_ERR_STATE_save_to_mark(ERR_STATE *es); void OSSL_ERR_STATE_restore(const ERR_STATE *es); void OSSL_ERR_STATE_free(ERR_STATE *es); diff --git a/include/openssl/sslerr.h b/include/openssl/sslerr.h index b330b90be4a88b..30e7e86db38836 100644 --- a/include/openssl/sslerr.h +++ b/include/openssl/sslerr.h @@ -230,6 +230,7 @@ # define SSL_R_PSK_IDENTITY_NOT_FOUND 223 # define SSL_R_PSK_NO_CLIENT_CB 224 # define SSL_R_PSK_NO_SERVER_CB 225 +# define SSL_R_QUIC_HANDSHAKE_LAYER_ERROR 393 # define SSL_R_QUIC_NETWORK_ERROR 387 # define SSL_R_QUIC_PROTOCOL_ERROR 382 # define SSL_R_READ_BIO_NOT_SET 211 diff --git a/ssl/quic/quic_channel.c b/ssl/quic/quic_channel.c index 71c1296d394ae2..e693f227ab3b12 100644 --- a/ssl/quic/quic_channel.c +++ b/ssl/quic/quic_channel.c @@ -1612,8 +1612,8 @@ static void ch_tick(QUIC_TICK_RESULT *res, void *arg, uint32_t flags) QUIC_CHANNEL *ch = arg; int channel_only = (flags & QUIC_REACTOR_TICK_FLAG_CHANNEL_ONLY) != 0; uint64_t error_code; - const char *error_msg, *error_src_file, *error_src_func; - int error_src_line; + const char *error_msg; + ERR_STATE *error_state = NULL; /* * When we tick the QUIC connection, we do everything we need to do @@ -1670,15 +1670,9 @@ static void ch_tick(QUIC_TICK_RESULT *res, void *arg, uint32_t flags) ossl_quic_tls_tick(ch->qtls); if (ossl_quic_tls_get_error(ch->qtls, &error_code, &error_msg, - &error_src_file, - &error_src_line, - &error_src_func)) { - ossl_quic_channel_raise_protocol_error_loc(ch, error_code, 0, - error_msg, - error_src_file, - error_src_line, - error_src_func); - } + &error_state)) + ossl_quic_channel_raise_protocol_error_state(ch, error_code, 0, + error_msg, error_state); } /* @@ -2838,6 +2832,7 @@ void ossl_quic_channel_raise_protocol_error_loc(QUIC_CHANNEL *ch, uint64_t error_code, uint64_t frame_type, const char *reason, + ERR_STATE *err_state, const char *src_file, int src_line, const char *src_func) @@ -2856,6 +2851,13 @@ void ossl_quic_channel_raise_protocol_error_loc(QUIC_CHANNEL *ch, err_str_sfx = ""; } + /* + * If we were provided an underlying error state, restore it and then append + * our ERR on top as a "cover letter" error. + */ + if (err_state != NULL) + OSSL_ERR_STATE_restore(err_state); + if (frame_type != 0) { ft_str = ossl_quic_frame_type_to_string(frame_type); if (ft_str == NULL) { diff --git a/ssl/quic/quic_tls.c b/ssl/quic/quic_tls.c index d8770bf9f7d145..fd24026fa38351 100644 --- a/ssl/quic/quic_tls.c +++ b/ssl/quic/quic_tls.c @@ -29,6 +29,8 @@ struct quic_tls_st { const unsigned char *local_transport_params; size_t local_transport_params_len; + ERR_STATE *error_state; + /* * QUIC error code (usually in the TLS Alert-mapped CRYPTO_ERR range). Valid * only if inerror is 1. @@ -41,10 +43,6 @@ struct quic_tls_st { */ const char *error_msg; - const char *error_src_file; - const char *error_src_func; - int error_src_line; - /* Whether our SSL object for TLS has been configured for use in QUIC */ unsigned int configured : 1; @@ -636,12 +634,18 @@ QUIC_TLS *ossl_quic_tls_new(const QUIC_TLS_ARGS *args) if (qtls == NULL) return NULL; + if ((qtls->error_state = OSSL_ERR_STATE_new()) == NULL) { + OPENSSL_free(qtls); + return NULL; + } + qtls->args = *args; return qtls; } void ossl_quic_tls_free(QUIC_TLS *qtls) { + OSSL_ERR_STATE_free(qtls->error_state); OPENSSL_free(qtls); } @@ -651,12 +655,28 @@ static int raise_error(QUIC_TLS *qtls, uint64_t error_code, int src_line, const char *src_func) { + /* + * When QTLS fails, add a "cover letter" error with information, potentially + * with any underlying libssl errors underneath it (but our cover error may + * be the only error in some cases). Then capture this into an ERR_STATE so + * we can report it later if need be when the QUIC_CHANNEL asks for it. + */ + ERR_new(); + ERR_set_debug(src_file, src_line, src_func); + ERR_set_error(ERR_LIB_SSL, SSL_R_QUIC_HANDSHAKE_LAYER_ERROR, + "handshake layer error, error code %zu (\"%s\")", + error_code, error_msg); + OSSL_ERR_STATE_save_to_mark(qtls->error_state); + + /* + * We record the error information reported via the QUIC protocol + * separately. + */ qtls->error_code = error_code; qtls->error_msg = error_msg; - qtls->error_src_file = src_file; - qtls->error_src_line = src_line; - qtls->error_src_func = src_func; qtls->inerror = 1; + + ERR_pop_to_mark(); return 0; } @@ -669,7 +689,7 @@ static int raise_error(QUIC_TLS *qtls, uint64_t error_code, int ossl_quic_tls_tick(QUIC_TLS *qtls) { - int ret; + int ret, err; const unsigned char *alpn; unsigned int alpnlen; @@ -683,6 +703,29 @@ int ossl_quic_tls_tick(QUIC_TLS *qtls) if (qtls->inerror) return 0; + /* + * SSL_get_error does not truly know what the cause of an SSL_read failure + * is and to some extent guesses based on contextual information. In + * particular, if there is _any_ ERR on the error stack, SSL_ERROR_SSL or + * SSL_ERROR_SYSCALL will be returned no matter what and there is no + * possibility of SSL_ERROR_WANT_READ/WRITE being returned, even if that was + * the actual cause of the SSL_read() failure. + * + * This means that ordinarily, the below code might not work right if the + * application has any ERR on the error stack. In order to make this code + * perform correctly regardless of prior ERR state, we use a variant of + * SSL_get_error() which ignores the error stack. However, some ERRs are + * raised by SSL_read() and actually indicate that something has gone wrong + * during the call to SSL_read(). We therefore adopt a strategy of marking + * the ERR stack and seeing if any errors get appended during the call to + * SSL_read(). If they are, we assume SSL_read() has raised an error and + * that we should use normal SSL_get_error() handling. + * + * NOTE: Ensure all escape paths from this function call + * ERR_clear_to_mark(). The RAISE macros handle this in failure cases. + */ + ERR_set_mark(); + if (!qtls->configured) { SSL_CONNECTION *sc = SSL_CONNECTION_FROM_SSL(qtls->args.s); SSL_CTX *sctx = SSL_CONNECTION_GET_CTX(sc); @@ -745,11 +788,17 @@ int ossl_quic_tls_tick(QUIC_TLS *qtls) ret = SSL_read(qtls->args.s, NULL, 0); else ret = SSL_do_handshake(qtls->args.s); + if (ret <= 0) { - switch (SSL_get_error(qtls->args.s, ret)) { + err = ossl_ssl_get_error(qtls->args.s, ret, + /*check_err=*/ERR_count_to_mark() > 0); + + switch (err) { case SSL_ERROR_WANT_READ: case SSL_ERROR_WANT_WRITE: + ERR_pop_to_mark(); return 1; + default: return RAISE_INTERNAL_ERROR(qtls); } @@ -763,9 +812,11 @@ int ossl_quic_tls_tick(QUIC_TLS *qtls) "no application protocol negotiated"); qtls->complete = 1; + ERR_pop_to_mark(); return qtls->args.handshake_complete_cb(qtls->args.handshake_complete_cb_arg); } + ERR_pop_to_mark(); return 1; } @@ -781,16 +832,12 @@ int ossl_quic_tls_set_transport_params(QUIC_TLS *qtls, int ossl_quic_tls_get_error(QUIC_TLS *qtls, uint64_t *error_code, const char **error_msg, - const char **error_src_file, - int *error_src_line, - const char **error_src_func) + ERR_STATE **error_state) { if (qtls->inerror) { *error_code = qtls->error_code; *error_msg = qtls->error_msg; - *error_src_file = qtls->error_src_file; - *error_src_line = qtls->error_src_line; - *error_src_func = qtls->error_src_func; + *error_state = qtls->error_state; } return qtls->inerror; diff --git a/ssl/ssl_err.c b/ssl/ssl_err.c index 9d24bc8014bf97..55694569d6cf1f 100644 --- a/ssl/ssl_err.c +++ b/ssl/ssl_err.c @@ -355,6 +355,8 @@ static const ERR_STRING_DATA SSL_str_reasons[] = { "psk identity not found"}, {ERR_PACK(ERR_LIB_SSL, 0, SSL_R_PSK_NO_CLIENT_CB), "psk no client cb"}, {ERR_PACK(ERR_LIB_SSL, 0, SSL_R_PSK_NO_SERVER_CB), "psk no server cb"}, + {ERR_PACK(ERR_LIB_SSL, 0, SSL_R_QUIC_HANDSHAKE_LAYER_ERROR), + "quic handshake layer error"}, {ERR_PACK(ERR_LIB_SSL, 0, SSL_R_QUIC_NETWORK_ERROR), "quic network error"}, {ERR_PACK(ERR_LIB_SSL, 0, SSL_R_QUIC_PROTOCOL_ERROR), "quic protocol error"}, diff --git a/ssl/ssl_lib.c b/ssl/ssl_lib.c index ce093b18cd641c..d092f69e2ac7bc 100644 --- a/ssl/ssl_lib.c +++ b/ssl/ssl_lib.c @@ -4559,6 +4559,11 @@ int SSL_set_ssl_method(SSL *s, const SSL_METHOD *meth) } int SSL_get_error(const SSL *s, int i) +{ + return ossl_ssl_get_error(s, i, /*check_err=*/1); +} + +int ossl_ssl_get_error(const SSL *s, int i, int check_err) { int reason; unsigned long l; @@ -4583,7 +4588,7 @@ int SSL_get_error(const SSL *s, int i) * Make things return SSL_ERROR_SYSCALL when doing SSL_do_handshake etc, * where we do encode the error */ - if ((l = ERR_peek_error()) != 0) { + if (check_err && (l = ERR_peek_error()) != 0) { if (ERR_GET_LIB(l) == ERR_LIB_SYS) return SSL_ERROR_SYSCALL; else diff --git a/util/libcrypto.num b/util/libcrypto.num index 9871821a6f6bce..b9354559740477 100644 --- a/util/libcrypto.num +++ b/util/libcrypto.num @@ -5528,3 +5528,5 @@ OSSL_ERR_STATE_new ? 3_2_0 EXIST::FUNCTION: OSSL_ERR_STATE_save ? 3_2_0 EXIST::FUNCTION: OSSL_ERR_STATE_restore ? 3_2_0 EXIST::FUNCTION: OSSL_ERR_STATE_free ? 3_2_0 EXIST::FUNCTION: +ERR_count_to_mark ? 3_2_0 EXIST::FUNCTION: +OSSL_ERR_STATE_save_to_mark ? 3_2_0 EXIST::FUNCTION: