Fix various style issues in the extension parsing refactor

Based on review feedback received.
Reviewed-by: NKurt Roeckx <kurt@openssl.org>
Reviewed-by: NRich Salz <rsalz@openssl.org>

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;
 }

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 */

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;
     }

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;
         }
     }