Commit bf846a6d authored by Matt Caswell's avatar Matt Caswell
Browse files

Don't overwrite the alert value if there is no alert to send 



The function tls_early_post_process_client_hello() was overwriting the
passed "al" parameter even if it was successful. The caller of that
function, tls_post_process_client_hello(), sets "al" to a sensible default
(HANDSHAKE_FAILURE), but this was being overwritten to be INTERNAL_ERROR.
The result is a "no shared cipher" error (and probably other similar errors)
were being reported back to the client with an incorrect INTERNAL_ERROR
alert.

Reviewed-by: default avatarRich Salz <rsalz@openssl.org>
(Merged from https://github.com/openssl/openssl/pull/3314)
parent d91b7423

ssl/statem/statem_srvr.c

+21 −21
Original line number Diff line number Diff line
@@ -1444,10 +1444,10 @@ MSG_PROCESS_RETURN tls_process_client_hello(SSL *s, PACKET *pkt) 
    return MSG_PROCESS_ERROR;

}



static int tls_early_post_process_client_hello(SSL *s, int *al)

static int tls_early_post_process_client_hello(SSL *s, int *pal)

{

    unsigned int j;

    int i;

    int i, al = SSL_AD_INTERNAL_ERROR;

    int protverr;

    size_t loop;

    unsigned long id;
@@ -1460,13 +1460,12 @@ static int tls_early_post_process_client_hello(SSL *s, int *al) 
    CLIENTHELLO_MSG *clienthello = s->clienthello;

    DOWNGRADE dgrd = DOWNGRADE_NONE;



    *al = SSL_AD_INTERNAL_ERROR;

    /* Finished parsing the ClientHello, now we can start processing it */

    /* Give the early callback a crack at things */

    if (s->ctx->early_cb != NULL) {

        int code;

        /* A failure in the early callback terminates the connection. */

        code = s->ctx->early_cb(s, al, s->ctx->early_cb_arg);

        code = s->ctx->early_cb(s, &al, s->ctx->early_cb_arg);

        if (code == 0)

            goto err;

        if (code < 0) {
@@ -1513,13 +1512,13 @@ static int tls_early_post_process_client_hello(SSL *s, int *al) 
            /* like ssl3_get_record, send alert using remote version number */

            s->version = s->client_version = clienthello->legacy_version;

        }

        *al = SSL_AD_PROTOCOL_VERSION;

        al = SSL_AD_PROTOCOL_VERSION;

        goto err;

    }



    /* TLSv1.3 specifies that a ClientHello must end on a record boundary */

    if (SSL_IS_TLS13(s) && RECORD_LAYER_processed_read_pending(&s->rlayer)) {

        *al = SSL_AD_UNEXPECTED_MESSAGE;

        al = SSL_AD_UNEXPECTED_MESSAGE;

        SSLerr(SSL_F_TLS_EARLY_POST_PROCESS_CLIENT_HELLO,

               SSL_R_NOT_ON_RECORD_BOUNDARY);

        goto err;
@@ -1531,7 +1530,7 @@ static int tls_early_post_process_client_hello(SSL *s, int *al) 
            if (s->ctx->app_verify_cookie_cb != NULL) {

                if (s->ctx->app_verify_cookie_cb(s, clienthello->dtls_cookie,

                        clienthello->dtls_cookie_len) == 0) {

                    *al = SSL_AD_HANDSHAKE_FAILURE;

                    al = SSL_AD_HANDSHAKE_FAILURE;

                    SSLerr(SSL_F_TLS_EARLY_POST_PROCESS_CLIENT_HELLO,

                           SSL_R_COOKIE_MISMATCH);

                    goto err;
@@ -1541,7 +1540,7 @@ static int tls_early_post_process_client_hello(SSL *s, int *al) 
            } else if (s->d1->cookie_len != clienthello->dtls_cookie_len

                    || memcmp(clienthello->dtls_cookie, s->d1->cookie,

                              s->d1->cookie_len) != 0) {

                *al = SSL_AD_HANDSHAKE_FAILURE;

                al = SSL_AD_HANDSHAKE_FAILURE;

                SSLerr(SSL_F_TLS_EARLY_POST_PROCESS_CLIENT_HELLO, SSL_R_COOKIE_MISMATCH);

                goto err;

            }
@@ -1552,7 +1551,7 @@ static int tls_early_post_process_client_hello(SSL *s, int *al) 
            if (protverr != 0) {

                SSLerr(SSL_F_TLS_EARLY_POST_PROCESS_CLIENT_HELLO, protverr);

                s->version = s->client_version;

                *al = SSL_AD_PROTOCOL_VERSION;

                al = SSL_AD_PROTOCOL_VERSION;

                goto err;

            }

        }
@@ -1563,7 +1562,7 @@ static int tls_early_post_process_client_hello(SSL *s, int *al) 
    /* We need to do this before getting the session */

    if (!tls_parse_extension(s, TLSEXT_IDX_extended_master_secret,

                             SSL_EXT_CLIENT_HELLO,

                             clienthello->pre_proc_exts, NULL, 0, al)) {

                             clienthello->pre_proc_exts, NULL, 0, &al)) {

        SSLerr(SSL_F_TLS_EARLY_POST_PROCESS_CLIENT_HELLO, SSL_R_CLIENTHELLO_TLSEXT);

        goto err;

    }
@@ -1590,7 +1589,7 @@ static int tls_early_post_process_client_hello(SSL *s, int *al) 
        if (!ssl_get_new_session(s, 1))

            goto err;

    } else {

        i = ssl_get_prev_session(s, clienthello, al);

        i = ssl_get_prev_session(s, clienthello, &al);

        if (i == 1) {

            /* previous session */

            s->hit = 1;
@@ -1604,9 +1603,9 @@ static int tls_early_post_process_client_hello(SSL *s, int *al) 
    }



    if (!ssl_cache_cipherlist(s, &clienthello->ciphersuites,

                              clienthello->isv2, al) ||

                              clienthello->isv2, &al) ||

        !bytes_to_cipher_list(s, &clienthello->ciphersuites, &ciphers, &scsvs,

                             clienthello->isv2, al)) {

                             clienthello->isv2, &al)) {

        goto err;

    }
@@ -1620,7 +1619,7 @@ static int tls_early_post_process_client_hello(SSL *s, int *al) 
                    /* SCSV is fatal if renegotiating */

                    SSLerr(SSL_F_TLS_EARLY_POST_PROCESS_CLIENT_HELLO,

                           SSL_R_SCSV_RECEIVED_WHEN_RENEGOTIATING);

                    *al = SSL_AD_HANDSHAKE_FAILURE;

                    al = SSL_AD_HANDSHAKE_FAILURE;

                    goto err;

                }

                s->s3->send_connection_binding = 1;
@@ -1635,7 +1634,7 @@ static int tls_early_post_process_client_hello(SSL *s, int *al) 
                 */

                SSLerr(SSL_F_TLS_EARLY_POST_PROCESS_CLIENT_HELLO,

                       SSL_R_INAPPROPRIATE_FALLBACK);

                *al = SSL_AD_INAPPROPRIATE_FALLBACK;

                al = SSL_AD_INAPPROPRIATE_FALLBACK;

                goto err;

            }

        }
@@ -1665,7 +1664,7 @@ static int tls_early_post_process_client_hello(SSL *s, int *al) 
             * we need to have the cipher in the cipher list if we are asked

             * to reuse it

             */

            *al = SSL_AD_ILLEGAL_PARAMETER;

            al = SSL_AD_ILLEGAL_PARAMETER;

            SSLerr(SSL_F_TLS_EARLY_POST_PROCESS_CLIENT_HELLO,

                   SSL_R_REQUIRED_CIPHER_MISSING);

            goto err;
@@ -1679,7 +1678,7 @@ static int tls_early_post_process_client_hello(SSL *s, int *al) 


    if (loop >= clienthello->compressions_len) {

        /* no compress */

        *al = SSL_AD_DECODE_ERROR;

        al = SSL_AD_DECODE_ERROR;

        SSLerr(SSL_F_TLS_EARLY_POST_PROCESS_CLIENT_HELLO, SSL_R_NO_COMPRESSION_SPECIFIED);

        goto err;

    }
@@ -1691,7 +1690,7 @@ static int tls_early_post_process_client_hello(SSL *s, int *al) 


    /* TLS extensions */

    if (!tls_parse_all_extensions(s, SSL_EXT_CLIENT_HELLO,

                                  clienthello->pre_proc_exts, NULL, 0, al)) {

                                  clienthello->pre_proc_exts, NULL, 0, &al)) {

        SSLerr(SSL_F_TLS_EARLY_POST_PROCESS_CLIENT_HELLO, SSL_R_PARSE_TLSEXT);

        goto err;

    }
@@ -1736,7 +1735,7 @@ static int tls_early_post_process_client_hello(SSL *s, int *al) 
                pref_cipher = ssl3_choose_cipher(s, s->session->ciphers,

                                                 SSL_get_ciphers(s));

            if (pref_cipher == NULL) {

                *al = SSL_AD_HANDSHAKE_FAILURE;

                al = SSL_AD_HANDSHAKE_FAILURE;

                SSLerr(SSL_F_TLS_EARLY_POST_PROCESS_CLIENT_HELLO, SSL_R_NO_SHARED_CIPHER);

                goto err;

            }
@@ -1786,7 +1785,7 @@ static int tls_early_post_process_client_hello(SSL *s, int *al) 
                break;

        }

        if (k >= clienthello->compressions_len) {

            *al = SSL_AD_ILLEGAL_PARAMETER;

            al = SSL_AD_ILLEGAL_PARAMETER;

            SSLerr(SSL_F_TLS_EARLY_POST_PROCESS_CLIENT_HELLO,

                   SSL_R_REQUIRED_COMPRESSION_ALGORITHM_MISSING);

            goto err;
@@ -1836,7 +1835,7 @@ static int tls_early_post_process_client_hello(SSL *s, int *al) 
        sk_SSL_CIPHER_free(s->session->ciphers);

        s->session->ciphers = ciphers;

        if (ciphers == NULL) {

            *al = SSL_AD_INTERNAL_ERROR;

            al = SSL_AD_INTERNAL_ERROR;

            SSLerr(SSL_F_TLS_EARLY_POST_PROCESS_CLIENT_HELLO, ERR_R_INTERNAL_ERROR);

            goto err;

        }
@@ -1863,6 +1862,7 @@ static int tls_early_post_process_client_hello(SSL *s, int *al) 
    return 1;

 err:

    ossl_statem_set_error(s);

    *pal = al;



    sk_SSL_CIPHER_free(ciphers);

    sk_SSL_CIPHER_free(scsvs);