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

Fix various style issues in the extension parsing refactor 



Based on review feedback received.

Reviewed-by: default avatarKurt Roeckx <kurt@openssl.org>
Reviewed-by: default avatarRich Salz <rsalz@openssl.org>
parent 6f8db4e6

ssl/statem/statem_lib.c

+14 −11
Original line number Diff line number Diff line
@@ -156,11 +156,12 @@ static int compare_extensions(const void *p1, const void *p2) 
{

    const RAW_EXTENSION *e1 = (const RAW_EXTENSION *)p1;

    const RAW_EXTENSION *e2 = (const RAW_EXTENSION *)p2;



    if (e1->type < e2->type)

        return -1;

    else if (e1->type > e2->type)

        return 1;

    else



    return 0;

}
@@ -174,7 +175,7 @@ static int compare_extensions(const void *p1, const void *p2) 
 * types, and 0 if the extensions contain duplicates, could not be successfully

 * parsed, or an internal error occurred.

 */

int tls_parse_raw_extensions(PACKET *packet, RAW_EXTENSION **res,

int tls_collect_extensions(PACKET *packet, RAW_EXTENSION **res,

                             size_t *numfound, int *ad)

{

    PACKET extensions = *packet;
@@ -185,20 +186,22 @@ int tls_parse_raw_extensions(PACKET *packet, RAW_EXTENSION **res, 
    while (PACKET_remaining(&extensions) > 0) {

        unsigned int type;

        PACKET extension;



        if (!PACKET_get_net_2(&extensions, &type) ||

            !PACKET_get_length_prefixed_2(&extensions, &extension)) {

            *ad = SSL_AD_DECODE_ERROR;

            goto done;

            goto err;

        }

        num_extensions++;

    }



    if (num_extensions > 0) {

        raw_extensions = OPENSSL_malloc(sizeof(RAW_EXTENSION) * num_extensions);

        raw_extensions = OPENSSL_malloc(sizeof(*raw_extensions)

                                        * num_extensions);

        if (raw_extensions == NULL) {

            *ad = SSL_AD_INTERNAL_ERROR;

            SSLerr(SSL_F_TLS_PARSE_RAW_EXTENSIONS, ERR_R_MALLOC_FAILURE);

            goto done;

            goto err;

        }



        /* Second pass: gather the extension types. */
@@ -209,22 +212,22 @@ int tls_parse_raw_extensions(PACKET *packet, RAW_EXTENSION **res, 
                /* This should not happen. */

                *ad = SSL_AD_INTERNAL_ERROR;

                SSLerr(SSL_F_TLS_PARSE_RAW_EXTENSIONS, ERR_R_INTERNAL_ERROR);

                goto done;

                goto err;

            }

        }



        if (PACKET_remaining(packet) != 0) {

            *ad = SSL_AD_DECODE_ERROR;

            SSLerr(SSL_F_TLS_PARSE_RAW_EXTENSIONS, SSL_R_LENGTH_MISMATCH);

            goto done;

            goto err;

        }

        /* Sort the extensions and make sure there are no duplicates. */

        qsort(raw_extensions, num_extensions, sizeof(RAW_EXTENSION),

        qsort(raw_extensions, num_extensions, sizeof(*raw_extensions),

              compare_extensions);

        for (i = 1; i < num_extensions; i++) {

            if (raw_extensions[i - 1].type == raw_extensions[i].type) {

                *ad = SSL_AD_DECODE_ERROR;

                goto done;

                goto err;

            }

        }

    }
@@ -233,7 +236,7 @@ int tls_parse_raw_extensions(PACKET *packet, RAW_EXTENSION **res, 
    *numfound = num_extensions;

    return 1;



 done:

 err:

    OPENSSL_free(raw_extensions);

    return 0;

}

ssl/statem/statem_locl.h

+1 −1
Original line number Diff line number Diff line
@@ -86,7 +86,7 @@ __owur int tls_construct_finished(SSL *s, WPACKET *pkt); 
__owur WORK_STATE tls_finish_handshake(SSL *s, WORK_STATE wst);

__owur WORK_STATE dtls_wait_for_dry(SSL *s);



int tls_parse_raw_extensions(PACKET *packet, RAW_EXTENSION **res,

int tls_collect_extensions(PACKET *packet, RAW_EXTENSION **res,

                             size_t *numfound, int *ad);



/* some client-only functions */

ssl/statem/statem_srvr.c

+20 −27
Original line number Diff line number Diff line
@@ -903,18 +903,15 @@ MSG_PROCESS_RETURN tls_process_client_hello(SSL *s, PACKET *pkt) 
    CLIENTHELLO_MSG clienthello;



    /*

     * First step is to parse the raw ClientHello data into the CLIENTHELLO_MSG

     * structure.

     * First, parse the raw ClientHello data into the CLIENTHELLO_MSG structure.

     */



    memset(&clienthello, 0, sizeof(clienthello));



    clienthello.isv2 = RECORD_LAYER_is_sslv2_record(&s->rlayer);



    PACKET_null_init(&cookie);



    if (clienthello.isv2) {

        unsigned int mt;



        /*-

         * An SSLv3/TLSv1 backwards-compatible CLIENT-HELLO in an SSLv2

         * header is sent directly on the wire, not wrapped as a TLS
@@ -988,11 +985,11 @@ MSG_PROCESS_RETURN tls_process_client_hello(SSL *s, PACKET *pkt) 
        }



        /* Load the client random and compression list. */

        challenge_len = challenge_len > SSL3_RANDOM_SIZE ? SSL3_RANDOM_SIZE :

            challenge_len;

        memset(clienthello.random, 0, SSL3_RANDOM_SIZE);

        challenge_len = challenge_len > sizeof(clienthello.random)

                        ? sizeof(clienthello.random) : challenge_len;

        memset(clienthello.random, 0, sizeof(clienthello.random));

        if (!PACKET_copy_bytes(&challenge,

                               clienthello.random + SSL3_RANDOM_SIZE -

                               clienthello.random + sizeof(clienthello.random) -

                               challenge_len, challenge_len)

            /* Advertise only null compression. */

            || !PACKET_buf_init(&compression, &null_compression, 1)) {
@@ -1069,9 +1066,9 @@ MSG_PROCESS_RETURN tls_process_client_hello(SSL *s, PACKET *pkt) 
        goto f_err;

    }



    /* We preserve the raw extensions PACKET for later use */

    /* Preserve the raw extensions PACKET for later use */

    extensions = clienthello.extensions;

    if (!tls_parse_raw_extensions(&extensions, &clienthello.pre_proc_exts,

    if (!tls_collect_extensions(&extensions, &clienthello.pre_proc_exts,

                                  &clienthello.num_extensions, &al)) {

        /* SSLerr already been called */

        goto f_err;
@@ -1085,18 +1082,18 @@ MSG_PROCESS_RETURN tls_process_client_hello(SSL *s, PACKET *pkt) 
    /* Choose the version */



    if (clienthello.isv2) {

        if (clienthello.version == 0x0002) {

            /* This is real SSLv2. We don't support it. */

        if (clienthello.version == SSL2_VERSION

                || (clienthello.version & 0xff00)

                   != (SSL3_VERSION_MAJOR << 8)) {

            /*

             * This is real SSLv2 or something complete unknown. We don't

             * support it.

             */

            SSLerr(SSL_F_TLS_PROCESS_CLIENT_HELLO, SSL_R_UNKNOWN_PROTOCOL);

            goto err;

        } else if ((clienthello.version & 0xff00) == (SSL3_VERSION_MAJOR << 8)) {

        }

        /* SSLv3/TLS */

        s->client_version = clienthello.version;

        } else {

            /* No idea what protocol this is */

            SSLerr(SSL_F_TLS_PROCESS_CLIENT_HELLO, SSL_R_UNKNOWN_PROTOCOL);

            goto err;

        }

    }

    /*

     * Do SSL/TLS version negotiation if applicable. For DTLS we just check
@@ -1114,10 +1111,7 @@ MSG_PROCESS_RETURN tls_process_client_hello(SSL *s, PACKET *pkt) 
    if (protverr) {

        SSLerr(SSL_F_TLS_PROCESS_CLIENT_HELLO, protverr);

        if ((!s->enc_write_ctx && !s->write_hash)) {

            /*

             * similar to ssl3_get_record, send alert using remote version

             * number

             */

            /* like ssl3_get_record, send alert using remote version number */

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

        }

        al = SSL_AD_PROTOCOL_VERSION;
@@ -1160,8 +1154,7 @@ MSG_PROCESS_RETURN tls_process_client_hello(SSL *s, PACKET *pkt) 
    s->hit = 0;



    /* We need to do this before getting the session */

    if (!tls_check_client_ems_support(s, &clienthello))

    {

    if (!tls_check_client_ems_support(s, &clienthello)) {

        /* Only fails if the extension is malformed */

        al = SSL_AD_DECODE_ERROR;

        SSLerr(SSL_F_TLS_PROCESS_CLIENT_HELLO, SSL_R_CLIENTHELLO_TLSEXT);
@@ -1212,7 +1205,7 @@ MSG_PROCESS_RETURN tls_process_client_hello(SSL *s, PACKET *pkt) 
        }

    }



    if (ssl_bytes_to_cipher_list(s, &clienthello.ciphersuites, &(ciphers),

    if (ssl_bytes_to_cipher_list(s, &clienthello.ciphersuites, &ciphers,

                                 clienthello.isv2, &al) == NULL) {

        goto f_err;

    }

ssl/t1_lib.c

+36 −42
Original line number Diff line number Diff line
@@ -1809,15 +1809,17 @@ static int ssl_scan_clienthello_tlsext(SSL *s, CLIENTHELLO_MSG *hello, int *al) 
     * resumption.

     */

    for (loop = 0; loop < hello->num_extensions; loop++) {

        RAW_EXTENSION *currext = &hello->pre_proc_exts[loop];



        if (s->tlsext_debug_cb)

            s->tlsext_debug_cb(s, 0, hello->pre_proc_exts[loop].type,

                               PACKET_data(&hello->pre_proc_exts[loop].data),

                               PACKET_remaining(&hello->pre_proc_exts[loop].data),

            s->tlsext_debug_cb(s, 0, currext->type,

                               PACKET_data(&currext->data),

                               PACKET_remaining(&currext->data),

                               s->tlsext_debug_arg);



        if (hello->pre_proc_exts[loop].type == TLSEXT_TYPE_renegotiate) {

        if (currext->type == TLSEXT_TYPE_renegotiate) {

            if (!ssl_parse_clienthello_renegotiate_ext(s,

                    &hello->pre_proc_exts[loop].data, al))

                    &currext->data, al))

                return 0;

            renegotiate_seen = 1;

        } else if (s->version == SSL3_VERSION) {
@@ -1847,12 +1849,11 @@ static int ssl_scan_clienthello_tlsext(SSL *s, CLIENTHELLO_MSG *hello, int *al) 
 *

 */



        else if (hello->pre_proc_exts[loop].type == TLSEXT_TYPE_server_name) {

        else if (currext->type == TLSEXT_TYPE_server_name) {

            unsigned int servname_type;

            PACKET sni, hostname;



            if (!PACKET_as_length_prefixed_2(&hello->pre_proc_exts[loop].data,

                                             &sni)

            if (!PACKET_as_length_prefixed_2(&currext->data, &sni)

                /* ServerNameList must be at least 1 byte long. */

                || PACKET_remaining(&sni) == 0) {

                return 0;
@@ -1904,11 +1905,10 @@ static int ssl_scan_clienthello_tlsext(SSL *s, CLIENTHELLO_MSG *hello, int *al) 
            }

        }

#ifndef OPENSSL_NO_SRP

        else if (hello->pre_proc_exts[loop].type == TLSEXT_TYPE_srp) {

        else if (currext->type == TLSEXT_TYPE_srp) {

            PACKET srp_I;



            if (!PACKET_as_length_prefixed_1(&hello->pre_proc_exts[loop].data,

                                             &srp_I))

            if (!PACKET_as_length_prefixed_1(&currext->data, &srp_I))

                return 0;



            if (PACKET_contains_zero_byte(&srp_I))
@@ -1926,11 +1926,10 @@ static int ssl_scan_clienthello_tlsext(SSL *s, CLIENTHELLO_MSG *hello, int *al) 
#endif



#ifndef OPENSSL_NO_EC

        else if (hello->pre_proc_exts[loop].type

                 == TLSEXT_TYPE_ec_point_formats) {

        else if (currext->type == TLSEXT_TYPE_ec_point_formats) {

            PACKET ec_point_format_list;



            if (!PACKET_as_length_prefixed_1(&hello->pre_proc_exts[loop].data,

            if (!PACKET_as_length_prefixed_1(&currext->data,

                                             &ec_point_format_list)

                || PACKET_remaining(&ec_point_format_list) == 0) {

                return 0;
@@ -1945,12 +1944,11 @@ static int ssl_scan_clienthello_tlsext(SSL *s, CLIENTHELLO_MSG *hello, int *al) 
                    return 0;

                }

            }

        } else if (hello->pre_proc_exts[loop].type

                   == TLSEXT_TYPE_elliptic_curves) {

        } else if (currext->type == TLSEXT_TYPE_elliptic_curves) {

            PACKET elliptic_curve_list;



            /* Each NamedCurve is 2 bytes and we must have at least 1. */

            if (!PACKET_as_length_prefixed_2(&hello->pre_proc_exts[loop].data,

            if (!PACKET_as_length_prefixed_2(&currext->data,

                                             &elliptic_curve_list)

                || PACKET_remaining(&elliptic_curve_list) == 0

                || (PACKET_remaining(&elliptic_curve_list) % 2) != 0) {
@@ -1968,21 +1966,19 @@ static int ssl_scan_clienthello_tlsext(SSL *s, CLIENTHELLO_MSG *hello, int *al) 
            }

        }

#endif                          /* OPENSSL_NO_EC */

        else if (hello->pre_proc_exts[loop].type

                 == TLSEXT_TYPE_session_ticket) {

        else if (currext->type == TLSEXT_TYPE_session_ticket) {

            if (s->tls_session_ticket_ext_cb &&

                !s->tls_session_ticket_ext_cb(s,

                    PACKET_data(&hello->pre_proc_exts[loop].data),

                    PACKET_remaining(&hello->pre_proc_exts[loop].data),

                    PACKET_data(&currext->data),

                    PACKET_remaining(&currext->data),

                    s->tls_session_ticket_ext_cb_arg)) {

                *al = TLS1_AD_INTERNAL_ERROR;

                return 0;

            }

        } else if (hello->pre_proc_exts[loop].type

                   == TLSEXT_TYPE_signature_algorithms) {

        } else if (currext->type == TLSEXT_TYPE_signature_algorithms) {

            PACKET supported_sig_algs;



            if (!PACKET_as_length_prefixed_2(&hello->pre_proc_exts[loop].data,

            if (!PACKET_as_length_prefixed_2(&currext->data,

                                             &supported_sig_algs)

                || (PACKET_remaining(&supported_sig_algs) % 2) != 0

                || PACKET_remaining(&supported_sig_algs) == 0) {
@@ -1995,9 +1991,8 @@ static int ssl_scan_clienthello_tlsext(SSL *s, CLIENTHELLO_MSG *hello, int *al) 
                    return 0;

                }

            }

        } else if (hello->pre_proc_exts[loop].type

                   == TLSEXT_TYPE_status_request) {

            if (!PACKET_get_1(&hello->pre_proc_exts[loop].data,

        } else if (currext->type == TLSEXT_TYPE_status_request) {

            if (!PACKET_get_1(&currext->data,

                              (unsigned int *)&s->tlsext_status_type)) {

                return 0;

            }
@@ -2006,7 +2001,7 @@ static int ssl_scan_clienthello_tlsext(SSL *s, CLIENTHELLO_MSG *hello, int *al) 
                const unsigned char *ext_data;

                PACKET responder_id_list, exts;

                if (!PACKET_get_length_prefixed_2

                    (&hello->pre_proc_exts[loop].data, &responder_id_list))

                    (&currext->data, &responder_id_list))

                    return 0;



                /*
@@ -2057,7 +2052,7 @@ static int ssl_scan_clienthello_tlsext(SSL *s, CLIENTHELLO_MSG *hello, int *al) 


                /* Read in request_extensions */

                if (!PACKET_as_length_prefixed_2(

                        &hello->pre_proc_exts[loop].data, &exts))

                        &currext->data, &exts))

                    return 0;



                if (PACKET_remaining(&exts) > 0) {
@@ -2082,12 +2077,11 @@ static int ssl_scan_clienthello_tlsext(SSL *s, CLIENTHELLO_MSG *hello, int *al) 
            }

        }

#ifndef OPENSSL_NO_HEARTBEATS

        else if (SSL_IS_DTLS(s)

                 && hello->pre_proc_exts[loop].type == TLSEXT_TYPE_heartbeat) {

        else if (SSL_IS_DTLS(s) && currext->type == TLSEXT_TYPE_heartbeat) {

            unsigned int hbtype;



            if (!PACKET_get_1(&hello->pre_proc_exts[loop].data, &hbtype)

                || PACKET_remaining(&hello->pre_proc_exts[loop].data)) {

            if (!PACKET_get_1(&currext->data, &hbtype)

                || PACKET_remaining(&currext->data)) {

                *al = SSL_AD_DECODE_ERROR;

                return 0;

            }
@@ -2106,7 +2100,7 @@ static int ssl_scan_clienthello_tlsext(SSL *s, CLIENTHELLO_MSG *hello, int *al) 
        }

#endif

#ifndef OPENSSL_NO_NEXTPROTONEG

        else if (hello->pre_proc_exts[loop].type == TLSEXT_TYPE_next_proto_neg

        else if (currext->type == TLSEXT_TYPE_next_proto_neg

                 && s->s3->tmp.finish_md_len == 0) {

            /*-

             * We shouldn't accept this extension on a
@@ -2129,24 +2123,24 @@ static int ssl_scan_clienthello_tlsext(SSL *s, CLIENTHELLO_MSG *hello, int *al) 
        }

#endif



        else if (hello->pre_proc_exts[loop].type

        else if (currext->type

                     == TLSEXT_TYPE_application_layer_protocol_negotiation

                 && s->s3->tmp.finish_md_len == 0) {

            if (!tls1_alpn_handle_client_hello(s,

                    &hello->pre_proc_exts[loop].data, al))

                    &currext->data, al))

                return 0;

        }



        /* session ticket processed earlier */

#ifndef OPENSSL_NO_SRTP

        else if (SSL_IS_DTLS(s) && SSL_get_srtp_profiles(s)

                 && hello->pre_proc_exts[loop].type == TLSEXT_TYPE_use_srtp) {

                 && currext->type == TLSEXT_TYPE_use_srtp) {

            if (ssl_parse_clienthello_use_srtp_ext(s,

                    &hello->pre_proc_exts[loop].data, al))

                    &currext->data, al))

                return 0;

        }

#endif

        else if (hello->pre_proc_exts[loop].type == TLSEXT_TYPE_encrypt_then_mac

        else if (currext->type == TLSEXT_TYPE_encrypt_then_mac

                 && !(s->options & SSL_OP_NO_ENCRYPT_THEN_MAC))

            s->s3->flags |= TLS1_FLAGS_ENCRYPT_THEN_MAC;

        /*
@@ -2162,9 +2156,9 @@ static int ssl_scan_clienthello_tlsext(SSL *s, CLIENTHELLO_MSG *hello, int *al) 
         * ServerHello may be later returned.

         */

        else if (!s->hit) {

            if (custom_ext_parse(s, 1, hello->pre_proc_exts[loop].type,

                    PACKET_data(&hello->pre_proc_exts[loop].data),

                    PACKET_remaining(&hello->pre_proc_exts[loop].data), al) <= 0)

            if (custom_ext_parse(s, 1, currext->type,

                    PACKET_data(&currext->data),

                    PACKET_remaining(&currext->data), al) <= 0)

                return 0;

        }

    }