Commit 47e2ee07 authored by Matt Caswell's avatar Matt Caswell
Browse files

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

crypto/err/openssl.txt

+1 −0
Original line number Diff line number Diff line
@@ -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

include/openssl/sslerr.h

+1 −0
Original line number Diff line number Diff line
@@ -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

ssl/ssl_err.c

+1 −0
Original line number Diff line number Diff line
@@ -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),

ssl/statem/statem.c

+44 −7
Original line number Diff line number Diff line
@@ -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;

        }

    }