Move ServerHello extension parsing into the new extension framework

Perl changes reviewed by Richard Levitte. Non-perl changes reviewed by Rich
Salz
Reviewed-by: NRich Salz <rsalz@openssl.org>
Reviewed-by: NRichard Levitte <levitte@openssl.org>

include/openssl/ssl.h

@@ -2308,6 +2308,9 @@ int ERR_load_SSL_strings(void);
 # define SSL_F_TLS_CONSTRUCT_SERVER_SESSION_TICKET        460
 # define SSL_F_TLS_CONSTRUCT_SERVER_STATUS_REQUEST        461
 # define SSL_F_TLS_CONSTRUCT_SERVER_USE_SRTP              462
+# define SSL_F_TLS_EXT_FINAL_                             484
+# define SSL_F_TLS_EXT_FINAL_EC_PT_FORMATS                485
+# define SSL_F_TLS_EXT_FINAL_EMS                          486
 # define SSL_F_TLS_EXT_FINAL_RENEGOTIATE                  483
 # define SSL_F_TLS_GET_MESSAGE_BODY                       351
 # define SSL_F_TLS_GET_MESSAGE_HEADER                     387

ssl/ssl_err.c

@@ -338,6 +338,10 @@ static ERR_STRING_DATA SSL_str_functs[] = {
      "tls_construct_server_status_request"},
     {ERR_FUNC(SSL_F_TLS_CONSTRUCT_SERVER_USE_SRTP),
      "tls_construct_server_use_srtp"},
+    {ERR_FUNC(SSL_F_TLS_EXT_FINAL_), "tls_ext_final_ems"},
+    {ERR_FUNC(SSL_F_TLS_EXT_FINAL_EC_PT_FORMATS),
+     "tls_ext_final_ec_pt_formats"},
+    {ERR_FUNC(SSL_F_TLS_EXT_FINAL_EMS), "tls_ext_final_ems"},
     {ERR_FUNC(SSL_F_TLS_EXT_FINAL_RENEGOTIATE), "tls_ext_final_renegotiate"},
     {ERR_FUNC(SSL_F_TLS_GET_MESSAGE_BODY), "tls_get_message_body"},
     {ERR_FUNC(SSL_F_TLS_GET_MESSAGE_HEADER), "tls_get_message_header"},

ssl/ssl_locl.h

@@ -2103,7 +2103,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_parse_serverhello_tlsext(SSL *s, PACKET *pkt);
 __owur RAW_EXTENSION *tls_get_extension_by_type(RAW_EXTENSION *exts,
                                                 size_t numexts,
                                                 unsigned int type);

ssl/statem/extensions.c

@@ -15,7 +15,14 @@ static int tls_ext_final_renegotiate(SSL *s, unsigned int context, int sent,
 static int tls_ext_init_server_name(SSL *s, unsigned int context);
 static int tls_ext_final_server_name(SSL *s, unsigned int context, int sent,
                                      int *al);
+#ifndef OPENSSL_NO_EC
+static int tls_ext_final_ec_pt_formats(SSL *s, unsigned int context, int sent,
+                                       int *al);
+#endif
+static int tls_ext_init_session_ticket(SSL *s, unsigned int context);
 static int tls_ext_init_status_request(SSL *s, unsigned int context);
+static int tls_ext_final_status_request(SSL *s, unsigned int context, int sent,
+                                        int *al);
 #ifndef OPENSSL_NO_NEXTPROTONEG
 static int tls_ext_init_npn(SSL *s, unsigned int context);
 #endif
@@ -26,6 +33,8 @@ static int tls_ext_init_sig_algs(SSL *s, unsigned int context);
 static int tls_ext_init_srp(SSL *s, unsigned int context);
 #endif
 static int tls_ext_init_etm(SSL *s, unsigned int context);
+static int tls_ext_init_ems(SSL *s, unsigned int context);
+static int tls_ext_final_ems(SSL *s, unsigned int context, int sent, int *al);
 #ifndef OPENSSL_NO_SRTP
 static int tls_ext_init_srtp(SSL *s, unsigned int context);
 #endif
@@ -122,7 +131,7 @@ static const EXTENSION_DEFINITION ext_defs[] = {
         tls_parse_server_ec_pt_formats,
         tls_construct_server_ec_pt_formats,
         tls_construct_client_ec_pt_formats,
-        NULL,
+        tls_ext_final_ec_pt_formats,
         EXT_CLIENT_HELLO | EXT_TLS1_2_AND_BELOW_ONLY
     },
     {
@@ -138,7 +147,7 @@ static const EXTENSION_DEFINITION ext_defs[] = {
 #endif
     {
         TLSEXT_TYPE_session_ticket,
-        NULL,
+        tls_ext_init_session_ticket,
         tls_parse_client_session_ticket,
         tls_parse_server_session_ticket,
         tls_construct_server_session_ticket,
@@ -164,7 +173,7 @@ static const EXTENSION_DEFINITION ext_defs[] = {
         tls_parse_server_status_request,
         tls_construct_server_status_request,
         tls_construct_client_status_request,
-        NULL,
+        tls_ext_final_status_request,
         EXT_CLIENT_HELLO | EXT_TLS1_2_SERVER_HELLO
         | EXT_TLS1_3_CERTIFICATE
     },
@@ -239,12 +248,12 @@ static const EXTENSION_DEFINITION ext_defs[] = {
 #endif
     {
         TLSEXT_TYPE_extended_master_secret,
-        NULL,
+        tls_ext_init_ems,
         tls_parse_client_ems,
         tls_parse_server_ems,
         tls_construct_server_ems,
         tls_construct_client_ems,
-        NULL,
+        tls_ext_final_ems,
         EXT_CLIENT_HELLO | EXT_TLS1_2_SERVER_HELLO | EXT_TLS1_2_AND_BELOW_ONLY
     },
     {
@@ -697,8 +706,22 @@ int tls_construct_extensions(SSL *s, WPACKET *pkt, unsigned int context,
 static int tls_ext_final_renegotiate(SSL *s, unsigned int context, int sent,
                                      int *al)
 {
-    if (!s->server)
+    if (!s->server) {
+        /*
+         * Check if we can connect to a server that doesn't support safe
+         * renegotiation
+         */
+        if (!(s->options & SSL_OP_LEGACY_SERVER_CONNECT)
+                && !(s->options & SSL_OP_ALLOW_UNSAFE_LEGACY_RENEGOTIATION)
+                && !sent) {
+            *al = SSL_AD_HANDSHAKE_FAILURE;
+            SSLerr(SSL_F_TLS_EXT_FINAL_RENEGOTIATE,
+                   SSL_R_UNSAFE_LEGACY_RENEGOTIATION_DISABLED);
+            return 0;
+        }
+
         return 1;
+    }
 
     /* Need RI if renegotiating */
     if (s->renegotiate
@@ -710,6 +733,7 @@ static int tls_ext_final_renegotiate(SSL *s, unsigned int context, int sent,
         return 0;
     }
 
+
     return 1;
 }
 
@@ -727,9 +751,6 @@ static int tls_ext_final_server_name(SSL *s, unsigned int context, int sent,
     int ret = SSL_TLSEXT_ERR_NOACK;
     int altmp = SSL_AD_UNRECOGNIZED_NAME;
 
-    if (!s->server)
-        return 1;
-
     if (s->ctx != NULL && s->ctx->tlsext_servername_callback != 0)
         ret = s->ctx->tlsext_servername_callback(s, &altmp,
                                                  s->ctx->tlsext_servername_arg);
@@ -756,6 +777,58 @@ static int tls_ext_final_server_name(SSL *s, unsigned int context, int sent,
     }
 }
 
+#ifndef OPENSSL_NO_EC
+static int tls_ext_final_ec_pt_formats(SSL *s, unsigned int context, int sent,
+                                       int *al)
+{
+    unsigned long alg_k, alg_a;
+
+    if (s->server)
+        return 1;
+
+    alg_k = s->s3->tmp.new_cipher->algorithm_mkey;
+    alg_a = s->s3->tmp.new_cipher->algorithm_auth;
+
+    /*
+     * If we are client and using an elliptic curve cryptography cipher
+     * suite, then if server returns an EC point formats lists extension it
+     * must contain uncompressed.
+     */
+    if ((s->tlsext_ecpointformatlist != NULL)
+        && (s->tlsext_ecpointformatlist_length > 0)
+        && (s->session->tlsext_ecpointformatlist != NULL)
+        && (s->session->tlsext_ecpointformatlist_length > 0)
+        && ((alg_k & SSL_kECDHE) || (alg_a & SSL_aECDSA))) {
+        /* we are using an ECC cipher */
+        size_t i;
+        unsigned char *list;
+        int found_uncompressed = 0;
+        list = s->session->tlsext_ecpointformatlist;
+        for (i = 0; i < s->session->tlsext_ecpointformatlist_length; i++) {
+            if (*(list++) == TLSEXT_ECPOINTFORMAT_uncompressed) {
+                found_uncompressed = 1;
+                break;
+            }
+        }
+        if (!found_uncompressed) {
+            SSLerr(SSL_F_TLS_EXT_FINAL_EC_PT_FORMATS,
+                   SSL_R_TLS_INVALID_ECPOINTFORMAT_LIST);
+            return 0;
+        }
+    }
+
+    return 1;
+}
+#endif
+
+static int tls_ext_init_session_ticket(SSL *s, unsigned int context)
+{
+    if (!s->server)
+        s->tlsext_ticket_expected = 0;
+
+    return 1;
+}
+
 static int tls_ext_init_status_request(SSL *s, unsigned int context)
 {
     if (s->server)
@@ -764,11 +837,27 @@ static int tls_ext_init_status_request(SSL *s, unsigned int context)
     return 1;
 }
 
+static int tls_ext_final_status_request(SSL *s, unsigned int context, int sent,
+                                        int *al)
+{
+    if (s->server)
+        return 1;
+
+    /*
+     * Ensure we get sensible values passed to tlsext_status_cb in the event
+     * that we don't receive a status message
+     */
+    OPENSSL_free(s->tlsext_ocsp_resp);
+    s->tlsext_ocsp_resp = NULL;
+    s->tlsext_ocsp_resplen = 0;
+
+    return 1;
+}
+
 #ifndef OPENSSL_NO_NEXTPROTONEG
 static int tls_ext_init_npn(SSL *s, unsigned int context)
 {
-    if (s->server)
-        s->s3->next_proto_neg_seen = 0;
+    s->s3->next_proto_neg_seen = 0;
 
     return 1;
 }
@@ -776,15 +865,14 @@ static int tls_ext_init_npn(SSL *s, unsigned int context)
 
 static int tls_ext_init_alpn(SSL *s, unsigned int context)
 {
+    OPENSSL_free(s->s3->alpn_selected);
+    s->s3->alpn_selected = NULL;
     if (s->server) {
-        OPENSSL_free(s->s3->alpn_selected);
-        s->s3->alpn_selected = NULL;
         s->s3->alpn_selected_len = 0;
         OPENSSL_free(s->s3->alpn_proposed);
         s->s3->alpn_proposed = NULL;
         s->s3->alpn_proposed_len = 0;
     }
-
     return 1;
 }
 
@@ -844,8 +932,33 @@ static int tls_ext_init_srp(SSL *s, unsigned int context)
 
 static int tls_ext_init_etm(SSL *s, unsigned int context)
 {
-    if (s->server)
-        s->s3->flags &= ~TLS1_FLAGS_ENCRYPT_THEN_MAC;
+    s->s3->flags &= ~TLS1_FLAGS_ENCRYPT_THEN_MAC;
+
+    return 1;
+}
+
+static int tls_ext_init_ems(SSL *s, unsigned int context)
+{
+    if (!s->server)
+        s->s3->flags &= ~TLS1_FLAGS_RECEIVED_EXTMS;
+
+    return 1;
+}
+
+static int tls_ext_final_ems(SSL *s, unsigned int context, int sent, int *al)
+{
+    if (!s->server && s->hit) {
+        /*
+         * Check extended master secret extension is consistent with
+         * original session.
+         */
+        if (!(s->s3->flags & TLS1_FLAGS_RECEIVED_EXTMS) !=
+            !(s->session->flags & SSL_SESS_FLAG_EXTMS)) {
+            *al = SSL_AD_HANDSHAKE_FAILURE;
+            SSLerr(SSL_F_TLS_EXT_FINAL_EMS, SSL_R_INCONSISTENT_EXTMS);
+            return 0;
+        }
+    }
 
     return 1;
 }

ssl/statem/extensions_clnt.c

@@ -1033,171 +1033,3 @@ int tls_parse_server_key_share(SSL *s, PACKET *pkt, int *al)
 
     return 1;
 }
-
-static int ssl_scan_serverhello_tlsext(SSL *s, PACKET *pkt, int *al)
-{
-    RAW_EXTENSION *extensions = NULL;
-    PACKET extpkt;
-
-#ifndef OPENSSL_NO_NEXTPROTONEG
-    s->s3->next_proto_neg_seen = 0;
-#endif
-    s->tlsext_ticket_expected = 0;
-
-    OPENSSL_free(s->s3->alpn_selected);
-    s->s3->alpn_selected = NULL;
-
-    s->s3->flags &= ~TLS1_FLAGS_ENCRYPT_THEN_MAC;
-
-    s->s3->flags &= ~TLS1_FLAGS_RECEIVED_EXTMS;
-
-    if (!PACKET_as_length_prefixed_2(pkt, &extpkt)) {
-        /* Extensions block may be completely absent in SSLv3 */
-        if (s->version != SSL3_VERSION || PACKET_remaining(pkt) != 0) {
-            *al = SSL_AD_DECODE_ERROR;
-            SSLerr(SSL_F_SSL_SCAN_SERVERHELLO_TLSEXT, SSL_R_BAD_LENGTH);
-            return 0;
-        }
-        PACKET_null_init(&extpkt);
-    }
-
-    /*
-     * TODO(TLS1.3): We give multiple contexts for now until we're ready to
-     * give something more specific
-     */
-
-    if (!tls_collect_extensions(s, &extpkt, EXT_TLS1_2_SERVER_HELLO
-                                            | EXT_TLS1_3_SERVER_HELLO
-                                            | EXT_TLS1_3_ENCRYPTED_EXTENSIONS
-                                            | EXT_TLS1_3_CERTIFICATE,
-                                &extensions, al))
-        return 0;
-
-    /*
-     * Determine if we need to see RI. Strictly speaking if we want to avoid
-     * an attack we should *always* see RI even on initial server hello
-     * because the client doesn't see any renegotiation during an attack.
-     * However this would mean we could not connect to any server which
-     * doesn't support RI so for the immediate future tolerate RI absence
-     */
-    if (!(s->options & SSL_OP_LEGACY_SERVER_CONNECT)
-            && !(s->options & SSL_OP_ALLOW_UNSAFE_LEGACY_RENEGOTIATION)
-            && !extensions[TLSEXT_IDX_renegotiate].present) {
-        *al = SSL_AD_HANDSHAKE_FAILURE;
-        SSLerr(SSL_F_SSL_SCAN_SERVERHELLO_TLSEXT,
-               SSL_R_UNSAFE_LEGACY_RENEGOTIATION_DISABLED);
-        return 0;
-    }
-
-    if (!tls_parse_all_extensions(s, EXT_TLS1_2_SERVER_HELLO
-                                     | EXT_TLS1_3_SERVER_HELLO
-                                     | EXT_TLS1_3_ENCRYPTED_EXTENSIONS
-                                     | EXT_TLS1_3_CERTIFICATE,
-                                  extensions,al))
-        return 0;
-
-    if (s->hit) {
-        /*
-         * Check extended master secret extension is consistent with
-         * original session.
-         */
-        if (!(s->s3->flags & TLS1_FLAGS_RECEIVED_EXTMS) !=
-            !(s->session->flags & SSL_SESS_FLAG_EXTMS)) {
-            *al = SSL_AD_HANDSHAKE_FAILURE;
-            SSLerr(SSL_F_SSL_SCAN_SERVERHELLO_TLSEXT, SSL_R_INCONSISTENT_EXTMS);
-            return 0;
-        }
-    }
-
-    return 1;
-}
-
-static int ssl_check_serverhello_tlsext(SSL *s)
-{
-    int ret = SSL_TLSEXT_ERR_NOACK;
-    int al = SSL_AD_UNRECOGNIZED_NAME;
-
-#ifndef OPENSSL_NO_EC
-    /*
-     * If we are client and using an elliptic curve cryptography cipher
-     * suite, then if server returns an EC point formats lists extension it
-     * must contain uncompressed.
-     */
-    unsigned long alg_k = s->s3->tmp.new_cipher->algorithm_mkey;
-    unsigned long alg_a = s->s3->tmp.new_cipher->algorithm_auth;
-    if ((s->tlsext_ecpointformatlist != NULL)
-        && (s->tlsext_ecpointformatlist_length > 0)
-        && (s->session->tlsext_ecpointformatlist != NULL)
-        && (s->session->tlsext_ecpointformatlist_length > 0)
-        && ((alg_k & SSL_kECDHE) || (alg_a & SSL_aECDSA))) {
-        /* we are using an ECC cipher */
-        size_t i;
-        unsigned char *list;
-        int found_uncompressed = 0;
-        list = s->session->tlsext_ecpointformatlist;
-        for (i = 0; i < s->session->tlsext_ecpointformatlist_length; i++) {
-            if (*(list++) == TLSEXT_ECPOINTFORMAT_uncompressed) {
-                found_uncompressed = 1;
-                break;
-            }
-        }
-        if (!found_uncompressed) {
-            SSLerr(SSL_F_SSL_CHECK_SERVERHELLO_TLSEXT,
-                   SSL_R_TLS_INVALID_ECPOINTFORMAT_LIST);
-            return -1;
-        }
-    }
-    ret = SSL_TLSEXT_ERR_OK;
-#endif                          /* OPENSSL_NO_EC */
-
-    if (s->ctx != NULL && s->ctx->tlsext_servername_callback != 0)
-        ret =
-            s->ctx->tlsext_servername_callback(s, &al,
-                                               s->ctx->tlsext_servername_arg);
-    else if (s->initial_ctx != NULL
-             && s->initial_ctx->tlsext_servername_callback != 0)
-        ret =
-            s->initial_ctx->tlsext_servername_callback(s, &al,
-                                                       s->
-                                                       initial_ctx->tlsext_servername_arg);
-
-    /*
-     * Ensure we get sensible values passed to tlsext_status_cb in the event
-     * that we don't receive a status message
-     */
-    OPENSSL_free(s->tlsext_ocsp_resp);
-    s->tlsext_ocsp_resp = NULL;
-    s->tlsext_ocsp_resplen = 0;
-
-    switch (ret) {
-    case SSL_TLSEXT_ERR_ALERT_FATAL:
-        ssl3_send_alert(s, SSL3_AL_FATAL, al);
-        return -1;
-
-    case SSL_TLSEXT_ERR_ALERT_WARNING:
-        ssl3_send_alert(s, SSL3_AL_WARNING, al);
-        return 1;
-
-    case SSL_TLSEXT_ERR_NOACK:
-        s->servername_done = 0;
-    default:
-        return 1;
-    }
-}
-
-int ssl_parse_serverhello_tlsext(SSL *s, PACKET *pkt)
-{
-    int al = -1;
-    if (s->version < SSL3_VERSION)
-        return 1;
-    if (ssl_scan_serverhello_tlsext(s, pkt, &al) <= 0) {
-        ssl3_send_alert(s, SSL3_AL_FATAL, al);
-        return 0;
-    }
-
-    if (ssl_check_serverhello_tlsext(s) <= 0) {
-        SSLerr(SSL_F_SSL_PARSE_SERVERHELLO_TLSEXT, SSL_R_SERVERHELLO_TLSEXT);
-        return 0;
-    }
-    return 1;
-}

ssl/statem/statem_clnt.c

@@ -1058,13 +1058,14 @@ MSG_PROCESS_RETURN tls_process_server_hello(SSL *s, PACKET *pkt)
 {
     STACK_OF(SSL_CIPHER) *sk;
     const SSL_CIPHER *c;
-    PACKET session_id;
+    PACKET session_id, extpkt;
     size_t session_id_len;
     const unsigned char *cipherchars;
     int i, al = SSL_AD_INTERNAL_ERROR;
     unsigned int compression;
     unsigned int sversion;
     int protverr;
+    RAW_EXTENSION *extensions = NULL;
 #ifndef OPENSSL_NO_COMP
     SSL_COMP *comp;
 #endif
@@ -1297,17 +1298,34 @@ MSG_PROCESS_RETURN tls_process_server_hello(SSL *s, PACKET *pkt)
 #endif
 
     /* TLS extensions */
-    if (!ssl_parse_serverhello_tlsext(s, pkt)) {
-        SSLerr(SSL_F_TLS_PROCESS_SERVER_HELLO, SSL_R_PARSE_TLSEXT);
-        goto err;
-    }
-
-    if (PACKET_remaining(pkt) != 0) {
-        /* wrong packet length */
+    if (PACKET_remaining(pkt) == 0) {
+        PACKET_null_init(&extpkt);
+    } else if (!PACKET_as_length_prefixed_2(pkt, &extpkt)) {
         al = SSL_AD_DECODE_ERROR;
-        SSLerr(SSL_F_TLS_PROCESS_SERVER_HELLO, SSL_R_BAD_PACKET_LENGTH);
+        SSLerr(SSL_F_TLS_PROCESS_SERVER_HELLO, SSL_R_BAD_LENGTH);
         goto f_err;
     }
+
+    /*
+     * TODO(TLS1.3): We give multiple contexts for now until we're ready to
+     * give something more specific
+     */
+
+    if (!tls_collect_extensions(s, &extpkt, EXT_TLS1_2_SERVER_HELLO
+                                            | EXT_TLS1_3_SERVER_HELLO
+                                            | EXT_TLS1_3_ENCRYPTED_EXTENSIONS
+                                            | EXT_TLS1_3_CERTIFICATE,
+                                &extensions, &al))
+        goto f_err;
+
+
+    if (!tls_parse_all_extensions(s, EXT_TLS1_2_SERVER_HELLO
+                                     | EXT_TLS1_3_SERVER_HELLO
+                                     | EXT_TLS1_3_ENCRYPTED_EXTENSIONS
+                                     | EXT_TLS1_3_CERTIFICATE,
+                                  extensions, &al))
+        goto f_err;
+
 #ifndef OPENSSL_NO_SCTP
     if (SSL_IS_DTLS(s) && s->hit) {
         unsigned char sctpauthkey[64];
@@ -1350,7 +1368,6 @@ MSG_PROCESS_RETURN tls_process_server_hello(SSL *s, PACKET *pkt)
     return MSG_PROCESS_CONTINUE_READING;
  f_err:
     ssl3_send_alert(s, SSL3_AL_FATAL, al);
- err:
     ossl_statem_set_error(s);
     return MSG_PROCESS_ERROR;
 }

ssl/statem/statem_locl.h

@@ -266,4 +266,3 @@ int tls_parse_server_use_srtp(SSL *s, PACKET *pkt, int *al);
 int tls_parse_server_etm(SSL *s, PACKET *pkt, int *al);
 int tls_parse_server_ems(SSL *s, PACKET *pkt, int *al);
 int tls_parse_server_key_share(SSL *s, PACKET *pkt, int *al);
-int ssl_parse_serverhello_tlsext(SSL *s, PACKET *pkt);