From b1b4b543ee531606cddb5df9d56b17b27d4ac60d Mon Sep 17 00:00:00 2001 From: Matt Caswell Date: Mon, 31 Oct 2016 13:11:17 +0000 Subject: [PATCH] Fix various style issues in the extension parsing refactor Based on review feedback received. Reviewed-by: Kurt Roeckx Reviewed-by: Rich Salz --- ssl/statem/statem_lib.c | 25 +++++++------ ssl/statem/statem_locl.h | 2 +- ssl/statem/statem_srvr.c | 47 +++++++++++------------- ssl/t1_lib.c | 78 +++++++++++++++++++--------------------- 4 files changed, 71 insertions(+), 81 deletions(-) diff --git a/ssl/statem/statem_lib.c b/ssl/statem/statem_lib.c index 3d2e3f319a..120b277932 100644 --- a/ssl/statem/statem_lib.c +++ b/ssl/statem/statem_lib.c @@ -156,12 +156,13 @@ 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; + + 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; } diff --git a/ssl/statem/statem_locl.h b/ssl/statem/statem_locl.h index 9c1def78cd..740595bba0 100644 --- a/ssl/statem/statem_locl.h +++ b/ssl/statem/statem_locl.h @@ -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 */ diff --git a/ssl/statem/statem_srvr.c b/ssl/statem/statem_srvr.c index ca7f5afe4b..68958264bb 100644 --- a/ssl/statem/statem_srvr.c +++ b/ssl/statem/statem_srvr.c @@ -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. */ - 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 */ + 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; } + /* SSLv3/TLS */ + s->client_version = clienthello.version; } /* * 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; } diff --git a/ssl/t1_lib.c b/ssl/t1_lib.c index e8357afe0d..ea876cf135 100644 --- a/ssl/t1_lib.c +++ b/ssl/t1_lib.c @@ -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; } } -- 2.39.2