Simplify ClientHello extension parsing

Remove some functions that are no longer needed now that we have the new
extension framework.

Perl changes reviewed by Richard Levitte. Non-perl changes reviewed by Rich
Salz

Reviewed-by: Rich Salz <rsalz@openssl.org>
Reviewed-by: Richard Levitte <levitte@openssl.org>

4 files changed, 71 insertions, 105 deletions

diff --git a/ssl/ssl_locl.h b/ssl/ssl_locl.h
index e068fd102e..7412ba6603 100644
--- a/ssl/ssl_locl.h
+++ b/ssl/ssl_locl.h
@@ -2079,7 +2079,6 @@ __owur  int tls1_get_curvelist(SSL *s, int sess, const unsigned char **pcurves,
 
 void ssl_set_default_md(SSL *s);
 __owur int tls1_set_server_sigalgs(SSL *s);
-__owur int ssl_check_clienthello_tlsext_late(SSL *s, int *al);
 __owur int ssl_parse_serverhello_tlsext(SSL *s, PACKET *pkt);
 __owur RAW_EXTENSION *tls_get_extension_by_type(RAW_EXTENSION *exts,
                                                 size_t numexts,
diff --git a/ssl/statem/extensions_srvr.c b/ssl/statem/extensions_srvr.c
index 50b42c0e68..1b9a820c98 100644
--- a/ssl/statem/extensions_srvr.c
+++ b/ssl/statem/extensions_srvr.c
@@ -521,7 +521,13 @@ int tls_parse_client_key_share(SSL *s, PACKET *pkt, int *al)
         return 0;
     }
 
-    /* Get the clients list of supported curves */
+    /*
+     * Get the clients list of supported curves. Note: We rely on the fact
+     * extension parsing happens in order of type. supported_groups has a lower
+     * type than key_share so will have been processed first.
+     * TODO(TLS1.3): We should validate that we actually received
+     * supported_groups!
+     */
     if (!tls1_get_curvelist(s, 1, &clntcurves, &clnt_num_curves)) {
         *al = SSL_AD_INTERNAL_ERROR;
         SSLerr(SSL_F_TLS_PARSE_CLIENT_KEY_SHARE, ERR_R_INTERNAL_ERROR);
@@ -648,78 +654,6 @@ int tls_parse_client_ems(SSL *s, PACKET *pkt, int *al)
     return 1;
 }
 
-/*
- * Process all remaining ClientHello extensions that we collected earlier and
- * haven't already processed.
- *
- * Behaviour upon resumption is extension-specific. If the extension has no
- * effect during resumption, it is parsed (to verify its format) but otherwise
- * ignored. Returns 1 on success and 0 on failure. Upon failure, sets |al| to
- * the appropriate alert.
- */
-int tls_scan_clienthello_tlsext(SSL *s, CLIENTHELLO_MSG *hello, int *al)
-{
-    /*
-     * We process the supported_groups extension first so that is done before
-     * we get to key_share which needs to use the information in it.
-     */
-    if (!tls_parse_extension(s, TLSEXT_TYPE_supported_groups, EXT_CLIENT_HELLO,
-                             hello->pre_proc_exts, hello->num_extensions, al)) {
-        return 0;
-    }
-
-    return tls_parse_all_extensions(s, EXT_CLIENT_HELLO, hello->pre_proc_exts,
-                                    hello->num_extensions, al);
-}
-
-/*
- * Upon success, returns 1.
- * Upon failure, returns 0 and sets |al| to the appropriate fatal alert.
- */
-int ssl_check_clienthello_tlsext_late(SSL *s, int *al)
-{
-    s->tlsext_status_expected = 0;
-
-    /*
-     * If status request then ask callback what to do. Note: this must be
-     * called after servername callbacks in case the certificate has changed,
-     * and must be called after the cipher has been chosen because this may
-     * influence which certificate is sent
-     */
-    if ((s->tlsext_status_type != -1) && s->ctx && s->ctx->tlsext_status_cb) {
-        int ret;
-        CERT_PKEY *certpkey;
-        certpkey = ssl_get_server_send_pkey(s);
-        /* If no certificate can't return certificate status */
-        if (certpkey != NULL) {
-            /*
-             * Set current certificate to one we will use so SSL_get_certificate
-             * et al can pick it up.
-             */
-            s->cert->key = certpkey;
-            ret = s->ctx->tlsext_status_cb(s, s->ctx->tlsext_status_arg);
-            switch (ret) {
-                /* We don't want to send a status request response */
-            case SSL_TLSEXT_ERR_NOACK:
-                s->tlsext_status_expected = 0;
-                break;
-                /* status request response should be sent */
-            case SSL_TLSEXT_ERR_OK:
-                if (s->tlsext_ocsp_resp)
-                    s->tlsext_status_expected = 1;
-                break;
-                /* something bad happened */
-            case SSL_TLSEXT_ERR_ALERT_FATAL:
-            default:
-                *al = SSL_AD_INTERNAL_ERROR;
-                return 0;
-            }
-        }
-    }
-
-    return 1;
-}
-
 /* Add the server's renegotiation binding */
 int tls_construct_server_renegotiate(SSL *s, WPACKET *pkt, int *al)
 {
diff --git a/ssl/statem/statem_locl.h b/ssl/statem/statem_locl.h
index 0ec2353626..23341d68de 100644
--- a/ssl/statem/statem_locl.h
+++ b/ssl/statem/statem_locl.h
@@ -182,8 +182,6 @@ int tls_parse_client_etm(SSL *s, PACKET *pkt, int *al);
 int tls_parse_client_key_share(SSL *s, PACKET *pkt, int *al);
 int tls_parse_client_ems(SSL *s, PACKET *pkt, int *al);
 
-int tls_scan_clienthello_tlsext(SSL *s, CLIENTHELLO_MSG *hello, int *al);
-
 int tls_construct_server_renegotiate(SSL *s, WPACKET *pkt, int *al);
 int tls_construct_server_server_name(SSL *s, WPACKET *pkt, int *al);
 #ifndef OPENSSL_NO_EC
diff --git a/ssl/statem/statem_srvr.c b/ssl/statem/statem_srvr.c
index bfdd4383f1..a346f54a61 100644
--- a/ssl/statem/statem_srvr.c
+++ b/ssl/statem/statem_srvr.c
@@ -1062,24 +1062,6 @@ int dtls_construct_hello_verify_request(SSL *s, WPACKET *pkt)
     return 1;
 }
 
-/*
- * Parse the extensions in the ClientHello that were collected earlier. Returns
- * 1 for success or 0 for failure.
- */
-static int tls_parse_clienthello_tlsext(SSL *s, CLIENTHELLO_MSG *hello)
-{
-    int al = -1;
-
-
-
-    if (tls_scan_clienthello_tlsext(s, hello, &al) <= 0) {
-        ssl3_send_alert(s, SSL3_AL_FATAL, al);
-        return 0;
-    }
-
-    return 1;
-}
-
 #ifndef OPENSSL_NO_EC
 /*-
  * ssl_check_for_safari attempts to fingerprint Safari using OS X
@@ -1525,9 +1507,11 @@ MSG_PROCESS_RETURN tls_process_client_hello(SSL *s, PACKET *pkt)
 #endif                          /* !OPENSSL_NO_EC */
 
     /* TLS extensions */
-    if (!tls_parse_clienthello_tlsext(s, &clienthello)) {
+    if (!tls_parse_all_extensions(s, EXT_CLIENT_HELLO,
+                                  clienthello.pre_proc_exts,
+                                  clienthello.num_extensions, &al)) {
         SSLerr(SSL_F_TLS_PROCESS_CLIENT_HELLO, SSL_R_PARSE_TLSEXT);
-        goto err;
+        goto f_err;
     }
 
     /* Check we've got a key_share for TLSv1.3 */
@@ -1711,6 +1695,54 @@ MSG_PROCESS_RETURN tls_process_client_hello(SSL *s, PACKET *pkt)
     return MSG_PROCESS_ERROR;
 }
 
+/*
+ * Call the status request callback if needed. Upon success, returns 1.
+ * Upon failure, returns 0 and sets |al| to the appropriate fatal alert.
+ */
+static int tls_handle_status_request(SSL *s, int *al)
+{
+    s->tlsext_status_expected = 0;
+
+    /*
+     * If status request then ask callback what to do. Note: this must be
+     * called after servername callbacks in case the certificate has changed,
+     * and must be called after the cipher has been chosen because this may
+     * influence which certificate is sent
+     */
+    if ((s->tlsext_status_type != -1) && s->ctx && s->ctx->tlsext_status_cb) {
+        int ret;
+        CERT_PKEY *certpkey;
+        certpkey = ssl_get_server_send_pkey(s);
+        /* If no certificate can't return certificate status */
+        if (certpkey != NULL) {
+            /*
+             * Set current certificate to one we will use so SSL_get_certificate
+             * et al can pick it up.
+             */
+            s->cert->key = certpkey;
+            ret = s->ctx->tlsext_status_cb(s, s->ctx->tlsext_status_arg);
+            switch (ret) {
+                /* We don't want to send a status request response */
+            case SSL_TLSEXT_ERR_NOACK:
+                s->tlsext_status_expected = 0;
+                break;
+                /* status request response should be sent */
+            case SSL_TLSEXT_ERR_OK:
+                if (s->tlsext_ocsp_resp)
+                    s->tlsext_status_expected = 1;
+                break;
+                /* something bad happened */
+            case SSL_TLSEXT_ERR_ALERT_FATAL:
+            default:
+                *al = SSL_AD_INTERNAL_ERROR;
+                return 0;
+            }
+        }
+    }
+
+    return 1;
+}
+
 WORK_STATE tls_post_process_client_hello(SSL *s, WORK_STATE wst)
 {
     int al = SSL_AD_HANDSHAKE_FAILURE;
@@ -1744,8 +1776,10 @@ WORK_STATE tls_post_process_client_hello(SSL *s, WORK_STATE wst)
             s->s3->tmp.new_cipher = cipher;
             /* check whether we should disable session resumption */
             if (s->not_resumable_session_cb != NULL)
-                s->session->not_resumable = s->not_resumable_session_cb(s,
-                                                                        ((cipher->algorithm_mkey & (SSL_kDHE | SSL_kECDHE)) != 0));
+                s->session->not_resumable =
+                    s->not_resumable_session_cb(s, ((cipher->algorithm_mkey
+                                                    & (SSL_kDHE | SSL_kECDHE))
+                                                   != 0));
             if (s->session->not_resumable)
                 /* do not send a session ticket */
                 s->tlsext_ticket_expected = 0;
@@ -1773,13 +1807,14 @@ WORK_STATE tls_post_process_client_hello(SSL *s, WORK_STATE wst)
          * s->s3->tmp.new_cipher- the new cipher to use.
          */
 
-        /* Handles TLS extensions that we couldn't check earlier */
-        if (s->version >= SSL3_VERSION) {
-            if (!ssl_check_clienthello_tlsext_late(s, &al)) {
-                SSLerr(SSL_F_TLS_POST_PROCESS_CLIENT_HELLO,
-                       SSL_R_CLIENTHELLO_TLSEXT);
-                goto f_err;
-            }
+        /*
+         * Call status_request callback if needed. Has to be done after the
+         * certificate callbacks etc above.
+         */
+        if (!tls_handle_status_request(s, &al)) {
+            SSLerr(SSL_F_TLS_POST_PROCESS_CLIENT_HELLO,
+                   SSL_R_CLIENTHELLO_TLSEXT);
+            goto f_err;
         }
 
         wst = WORK_MORE_B;