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>
diff --git a/ssl/statem/extensions_srvr.c b/ssl/statem/extensions_srvr.c index 50b42c0..1b9a820 100644 --- a/ssl/statem/extensions_srvr.c +++ b/ssl/statem/extensions_srvr.c
@@ -521,7 +521,13 @@ 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 @@ 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 0ec2353..23341d6 100644 --- a/ssl/statem/statem_locl.h +++ b/ssl/statem/statem_locl.h
@@ -182,8 +182,6 @@ 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 bfdd438..a346f54 100644 --- a/ssl/statem/statem_srvr.c +++ b/ssl/statem/statem_srvr.c
@@ -1062,24 +1062,6 @@ 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 @@ #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 @@ 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 @@ 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 @@ * 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;