From 47e2ee072290db534720565318f0a8110a2e7d92 Mon Sep 17 00:00:00 2001 From: Matt Caswell Date: Thu, 23 Nov 2017 12:10:54 +0000 Subject: [PATCH] Add some sanity checks for the fatal error condition Sometimes at the top level of the state machine code we know we are supposed to be in a fatal error condition. This commit adds some sanity checks to ensure that SSLfatal() has been called. Reviewed-by: Richard Levitte (Merged from https://github.com/openssl/openssl/pull/4778) --- crypto/err/openssl.txt | 1 + include/openssl/sslerr.h | 1 + ssl/ssl_err.c | 1 + ssl/statem/statem.c | 51 ++++++++++++++++++++++++++++++++++------ 4 files changed, 47 insertions(+), 7 deletions(-) diff --git a/crypto/err/openssl.txt b/crypto/err/openssl.txt index 601f1e7c01..1220d3a8ee 100644 --- a/crypto/err/openssl.txt +++ b/crypto/err/openssl.txt @@ -2466,6 +2466,7 @@ SSL_R_LIBRARY_BUG:274:library bug SSL_R_LIBRARY_HAS_NO_CIPHERS:161:library has no ciphers SSL_R_MISSING_DSA_SIGNING_CERT:165:missing dsa signing cert SSL_R_MISSING_ECDSA_SIGNING_CERT:381:missing ecdsa signing cert +SSL_R_MISSING_FATAL:256:missing fatal SSL_R_MISSING_RSA_CERTIFICATE:168:missing rsa certificate SSL_R_MISSING_RSA_ENCRYPTING_CERT:169:missing rsa encrypting cert SSL_R_MISSING_RSA_SIGNING_CERT:170:missing rsa signing cert diff --git a/include/openssl/sslerr.h b/include/openssl/sslerr.h index 9774f9ff19..6662f667e6 100644 --- a/include/openssl/sslerr.h +++ b/include/openssl/sslerr.h @@ -547,6 +547,7 @@ int ERR_load_SSL_strings(void); # define SSL_R_LIBRARY_HAS_NO_CIPHERS 161 # define SSL_R_MISSING_DSA_SIGNING_CERT 165 # define SSL_R_MISSING_ECDSA_SIGNING_CERT 381 +# define SSL_R_MISSING_FATAL 256 # define SSL_R_MISSING_RSA_CERTIFICATE 168 # define SSL_R_MISSING_RSA_ENCRYPTING_CERT 169 # define SSL_R_MISSING_RSA_SIGNING_CERT 170 diff --git a/ssl/ssl_err.c b/ssl/ssl_err.c index 88e741e9fd..b9bef5aa95 100644 --- a/ssl/ssl_err.c +++ b/ssl/ssl_err.c @@ -877,6 +877,7 @@ static const ERR_STRING_DATA SSL_str_reasons[] = { "missing dsa signing cert"}, {ERR_PACK(ERR_LIB_SSL, 0, SSL_R_MISSING_ECDSA_SIGNING_CERT), "missing ecdsa signing cert"}, + {ERR_PACK(ERR_LIB_SSL, 0, SSL_R_MISSING_FATAL), "missing fatal"}, {ERR_PACK(ERR_LIB_SSL, 0, SSL_R_MISSING_RSA_CERTIFICATE), "missing rsa certificate"}, {ERR_PACK(ERR_LIB_SSL, 0, SSL_R_MISSING_RSA_ENCRYPTING_CERT), diff --git a/ssl/statem/statem.c b/ssl/statem/statem.c index db2de6e3bf..5c158fa24d 100644 --- a/ssl/statem/statem.c +++ b/ssl/statem/statem.c @@ -124,6 +124,19 @@ void ossl_statem_fatal(SSL *s, int al, int func, int reason, const char *file, ssl3_send_alert(s, SSL3_AL_FATAL, al); } +/* + * This macro should only be called if we are already expecting to be in + * a fatal error state. We verify that we are, and set it if not (this would + * indicate a bug). + */ +#define check_fatal(s, f) \ + do { \ + if (!ossl_assert((s)->statem.in_init \ + || (s)->statem.state != MSG_FLOW_ERROR)) \ + SSLfatal(s, SSL_AD_INTERNAL_ERROR, (f), \ + SSL_R_MISSING_FATAL); \ + } while (0) + /* * Discover whether the current connection is in the error state. * @@ -321,17 +334,21 @@ static int state_machine(SSL *s, int server) if (cb != NULL) cb(s, SSL_CB_HANDSHAKE_START, 1); + /* + * Fatal errors in this block don't send an alert because we have + * failed to even initialise properly. Sending an alert is probably + * doomed to failure. + */ + if (SSL_IS_DTLS(s)) { if ((s->version & 0xff00) != (DTLS1_VERSION & 0xff00) && (server || (s->version & 0xff00) != (DTLS1_BAD_VER & 0xff00))) { - /* We've failed to even initialise so no alert sent */ SSLfatal(s, SSL_AD_NO_ALERT, SSL_F_STATE_MACHINE, ERR_R_INTERNAL_ERROR); goto end; } } else { if ((s->version >> 8) != SSL3_VERSION_MAJOR) { - /* We've failed to even initialise so no alert sent */ SSLfatal(s, SSL_AD_NO_ALERT, SSL_F_STATE_MACHINE, ERR_R_INTERNAL_ERROR); goto end; @@ -339,7 +356,6 @@ static int state_machine(SSL *s, int server) } if (!ssl_security(s, SSL_SECOP_VERSION, 0, s->version, NULL)) { - /* We've failed to even initialise so no alert sent */ SSLfatal(s, SSL_AD_NO_ALERT, SSL_F_STATE_MACHINE, ERR_R_INTERNAL_ERROR); goto end; @@ -347,9 +363,13 @@ static int state_machine(SSL *s, int server) if (s->init_buf == NULL) { if ((buf = BUF_MEM_new()) == NULL) { + SSLfatal(s, SSL_AD_NO_ALERT, SSL_F_STATE_MACHINE, + ERR_R_INTERNAL_ERROR); goto end; } if (!BUF_MEM_grow(buf, SSL3_RT_MAX_PLAIN_LENGTH)) { + SSLfatal(s, SSL_AD_NO_ALERT, SSL_F_STATE_MACHINE, + ERR_R_INTERNAL_ERROR); goto end; } s->init_buf = buf; @@ -357,6 +377,8 @@ static int state_machine(SSL *s, int server) } if (!ssl3_setup_buffers(s)) { + SSLfatal(s, SSL_AD_NO_ALERT, SSL_F_STATE_MACHINE, + ERR_R_INTERNAL_ERROR); goto end; } s->init_num = 0; @@ -374,13 +396,17 @@ static int state_machine(SSL *s, int server) if (!SSL_IS_DTLS(s) || !BIO_dgram_is_sctp(SSL_get_wbio(s))) #endif if (!ssl_init_wbio_buffer(s)) { + SSLfatal(s, SSL_AD_NO_ALERT, SSL_F_STATE_MACHINE, + ERR_R_INTERNAL_ERROR); goto end; } if ((SSL_in_before(s)) || s->renegotiate) { - if (!tls_setup_handshake(s)) + if (!tls_setup_handshake(s)) { + /* SSLfatal() already called */ goto end; + } if (SSL_IS_FIRST_HANDSHAKE(s)) st->read_state_first_init = 1; @@ -413,8 +439,8 @@ static int state_machine(SSL *s, int server) } } else { /* Error */ - SSLfatal(s, SSL_AD_INTERNAL_ERROR, SSL_F_STATE_MACHINE, - ERR_R_INTERNAL_ERROR); + check_fatal(s, SSL_F_STATE_MACHINE); + SSLerr(SSL_F_STATE_MACHINE, ERR_R_INTERNAL_ERROR); goto end; } } @@ -557,6 +583,7 @@ static SUB_STATE_RETURN read_state_machine(SSL *s) * to that state if so */ if (!transition(s, mt)) { + check_fatal(s, SSL_F_READ_STATE_MACHINE); return SUB_STATE_ERROR; } @@ -602,6 +629,7 @@ static SUB_STATE_RETURN read_state_machine(SSL *s) switch (ret) { case MSG_PROCESS_ERROR: + check_fatal(s, SSL_F_READ_STATE_MACHINE); return SUB_STATE_ERROR; case MSG_PROCESS_FINISHED_READING: @@ -625,6 +653,8 @@ static SUB_STATE_RETURN read_state_machine(SSL *s) st->read_state_work = post_process_message(s, st->read_state_work); switch (st->read_state_work) { case WORK_ERROR: + check_fatal(s, SSL_F_READ_STATE_MACHINE); + /* Fall through */ case WORK_MORE_A: case WORK_MORE_B: case WORK_MORE_C: @@ -760,6 +790,7 @@ static SUB_STATE_RETURN write_state_machine(SSL *s) break; case WRITE_TRAN_ERROR: + check_fatal(s, SSL_F_WRITE_STATE_MACHINE); return SUB_STATE_ERROR; } break; @@ -767,6 +798,8 @@ static SUB_STATE_RETURN write_state_machine(SSL *s) case WRITE_STATE_PRE_WORK: switch (st->write_state_work = pre_work(s, st->write_state_work)) { case WORK_ERROR: + check_fatal(s, SSL_F_WRITE_STATE_MACHINE); + /* Fall through */ case WORK_MORE_A: case WORK_MORE_B: case WORK_MORE_C: @@ -798,7 +831,7 @@ static SUB_STATE_RETURN write_state_machine(SSL *s) } if (confunc != NULL && !confunc(s, &pkt)) { WPACKET_cleanup(&pkt); - /* SSLfatal() already called */ + check_fatal(s, SSL_F_WRITE_STATE_MACHINE); return SUB_STATE_ERROR; } if (!ssl_close_construct_packet(s, &pkt, mt) @@ -826,6 +859,8 @@ static SUB_STATE_RETURN write_state_machine(SSL *s) case WRITE_STATE_POST_WORK: switch (st->write_state_work = post_work(s, st->write_state_work)) { case WORK_ERROR: + check_fatal(s, SSL_F_WRITE_STATE_MACHINE); + /* Fall through */ case WORK_MORE_A: case WORK_MORE_B: case WORK_MORE_C: @@ -841,6 +876,8 @@ static SUB_STATE_RETURN write_state_machine(SSL *s) break; default: + SSLfatal(s, SSL_AD_INTERNAL_ERROR, SSL_F_WRITE_STATE_MACHINE, + ERR_R_INTERNAL_ERROR); return SUB_STATE_ERROR; } } -- 2.25.1