From 70c22888c1648fe8652e77107f3c74bf2212de36 Mon Sep 17 00:00:00 2001 From: Emilia Kasper Date: Mon, 4 Jul 2016 20:32:28 +0200 Subject: [PATCH] Fix two bugs in clienthello processing - Always process ALPN (previously there was an early return in the certificate status handling) - Don't send a duplicate alert. Previously, both ssl_check_clienthello_tlsext_late and its caller would send an alert. Consolidate alert sending code in the caller. Reviewed-by: Rich Salz --- ssl/ssl_locl.h | 2 +- ssl/statem/statem_srvr.c | 2 +- ssl/t1_lib.c | 87 ++++++++++++++++------------------------ 3 files changed, 37 insertions(+), 54 deletions(-) diff --git a/ssl/ssl_locl.h b/ssl/ssl_locl.h index 1cc63aa819..25cd312c95 100644 --- a/ssl/ssl_locl.h +++ b/ssl/ssl_locl.h @@ -2004,7 +2004,7 @@ __owur unsigned char *ssl_add_serverhello_tlsext(SSL *s, unsigned char *buf, __owur int ssl_parse_clienthello_tlsext(SSL *s, PACKET *pkt); void ssl_set_default_md(SSL *s); __owur int tls1_set_server_sigalgs(SSL *s); -__owur int ssl_check_clienthello_tlsext_late(SSL *s); +__owur int ssl_check_clienthello_tlsext_late(SSL *s, int *al); __owur int ssl_parse_serverhello_tlsext(SSL *s, PACKET *pkt); __owur int ssl_prepare_clienthello_tlsext(SSL *s); __owur int ssl_prepare_serverhello_tlsext(SSL *s); diff --git a/ssl/statem/statem_srvr.c b/ssl/statem/statem_srvr.c index a45acbd30c..b5cfc4f220 100644 --- a/ssl/statem/statem_srvr.c +++ b/ssl/statem/statem_srvr.c @@ -1454,7 +1454,7 @@ WORK_STATE tls_post_process_client_hello(SSL *s, WORK_STATE wst) /* Handles TLS extensions that we couldn't check earlier */ if (s->version >= SSL3_VERSION) { - if (ssl_check_clienthello_tlsext_late(s) <= 0) { + if (!ssl_check_clienthello_tlsext_late(s, &al)) { SSLerr(SSL_F_TLS_POST_PROCESS_CLIENT_HELLO, SSL_R_CLIENTHELLO_TLSEXT); goto f_err; diff --git a/ssl/t1_lib.c b/ssl/t1_lib.c index e938d87de7..2418c65862 100644 --- a/ssl/t1_lib.c +++ b/ssl/t1_lib.c @@ -1678,11 +1678,10 @@ static int tls1_alpn_handle_client_hello(SSL *s, PACKET *pkt, int *al) /* * Process the ALPN extension in a ClientHello. - * ret: a pointer to the TLSEXT return value: SSL_TLSEXT_ERR_* * al: a pointer to the alert value to send in the event of a failure. - * returns 1 on success, 0 + * returns 1 on success, 0 on error. */ -static int tls1_alpn_handle_client_hello_late(SSL *s, int *ret, int *al) +static int tls1_alpn_handle_client_hello_late(SSL *s, int *al) { const unsigned char *selected = NULL; unsigned char selected_len = 0; @@ -1698,7 +1697,6 @@ static int tls1_alpn_handle_client_hello_late(SSL *s, int *ret, int *al) s->s3->alpn_selected = OPENSSL_memdup(selected, selected_len); if (s->s3->alpn_selected == NULL) { *al = SSL_AD_INTERNAL_ERROR; - *ret = SSL_TLSEXT_ERR_ALERT_FATAL; return 0; } s->s3->alpn_selected_len = selected_len; @@ -1708,7 +1706,6 @@ static int tls1_alpn_handle_client_hello_late(SSL *s, int *ret, int *al) #endif } else { *al = SSL_AD_NO_APPLICATION_PROTOCOL; - *ret = SSL_TLSEXT_ERR_ALERT_FATAL; return 0; } } @@ -2661,10 +2658,13 @@ int tls1_set_server_sigalgs(SSL *s) return 0; } -int ssl_check_clienthello_tlsext_late(SSL *s) +/* + * 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) { - int ret = SSL_TLSEXT_ERR_OK; - int al = SSL_AD_INTERNAL_ERROR; + s->tlsext_status_expected = 0; /* * If status request then ask callback what to do. Note: this must be @@ -2673,58 +2673,41 @@ int ssl_check_clienthello_tlsext_late(SSL *s) * influence which certificate is sent */ if ((s->tlsext_status_type != -1) && s->ctx && s->ctx->tlsext_status_cb) { - int r; + int ret; CERT_PKEY *certpkey; certpkey = ssl_get_server_send_pkey(s); /* If no certificate can't return certificate status */ - if (certpkey == NULL) { - s->tlsext_status_expected = 0; - return 1; - } - /* - * Set current certificate to one we will use so SSL_get_certificate - * et al can pick it up. - */ - s->cert->key = certpkey; - r = s->ctx->tlsext_status_cb(s, s->ctx->tlsext_status_arg); - switch (r) { - /* 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; - else + 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; - /* something bad happened */ - case SSL_TLSEXT_ERR_ALERT_FATAL: - ret = SSL_TLSEXT_ERR_ALERT_FATAL; - al = SSL_AD_INTERNAL_ERROR; - goto err; + 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; + } } - } else - s->tlsext_status_expected = 0; - - if (!tls1_alpn_handle_client_hello_late(s, &ret, &al)) { - goto err; } - err: - 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; - - default: - return 1; + if (!tls1_alpn_handle_client_hello_late(s, al)) { + return 0; } + + return 1; } int ssl_check_serverhello_tlsext(SSL *s) -- 2.34.1