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 <levitte@openssl.org>
(Merged from https://github.com/openssl/openssl/pull/4778)

diff --git a/crypto/err/openssl.txt b/crypto/err/openssl.txt
index 601f1e7c01612675a2903c53f1f0f302261d6107..1220d3a8eeb4a86a5d574fb0c7490d021f3733b8 100644 (file)
--- 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 9774f9ff1938701c39a7ffe2a16e19c0219cb68a..6662f667e6f3daa892d9b6202b96d24edb259499 100644 (file)
--- 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 88e741e9fd1c2d479f6e512605b6081d28d8b3a9..b9bef5aa9575b9db73e741530beff4c78bbd5479 100644 (file)
--- 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 db2de6e3bf714ffdfaab5397f287ad195c39176d..5c158fa24d209b669946abe9dc4179f216a61245 100644 (file)
--- 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;
         }
     }