From 5626f634c31cfde48ccbbee243be29e0eb77171e Mon Sep 17 00:00:00 2001 From: Benjamin Kaduk Date: Tue, 1 Aug 2017 14:50:22 -0500 Subject: [PATCH] Move ALPN handling from finalizer to delayed call Commit 02f0274e8c0596dcf7e2d104250232a42c650b96 moved ALPN processing into an extension finalization function, as the only documented ordering requirement from previous commits was that ALPN processing occur after SNI processing, and SNI processing is performed before the extension finalization step. However, it is useful for applications' alpn_select callbacks to run after ciphersuite selection as well -- at least one application protocol specification (HTTP/2) imposes restrictions on which ciphersuites are usable with that protocol. Since it is generally more preferrable to have a successful TLS connection with a default application protocol than to fail the TLS connection and not be able to have the preferred application protocol, it is good to give the alpn_select callback information about the ciphersuite to be used, so that appropriate restrctions can be enforced in application code. Accordingly, split the ALPN handling out into a separate tls_handl_alpn() function akin to tls_handle_status_request(), called from tls_post_process_client_hello(). This is an alternative to resuscitating ssl_check_clienthello_tlsext_late(), something of an awkwward name itself. Reviewed-by: Matt Caswell (Merged from https://github.com/openssl/openssl/pull/4070) --- ssl/statem/extensions.c | 41 +--------------------------------- ssl/statem/statem_srvr.c | 48 ++++++++++++++++++++++++++++++++++++++++ 2 files changed, 49 insertions(+), 40 deletions(-) diff --git a/ssl/statem/extensions.c b/ssl/statem/extensions.c index 2268b271ce..a5dda45a96 100644 --- a/ssl/statem/extensions.c +++ b/ssl/statem/extensions.c @@ -28,7 +28,6 @@ static int init_status_request(SSL *s, unsigned int context); static int init_npn(SSL *s, unsigned int context); #endif static int init_alpn(SSL *s, unsigned int context); -static int final_alpn(SSL *s, unsigned int context, int sent, int *al); static int init_sig_algs(SSL *s, unsigned int context); static int init_certificate_authorities(SSL *s, unsigned int context); static EXT_RETURN tls_construct_certificate_authorities(SSL *s, WPACKET *pkt, @@ -207,7 +206,7 @@ static const EXTENSION_DEFINITION ext_defs[] = { SSL_EXT_CLIENT_HELLO | SSL_EXT_TLS1_2_SERVER_HELLO | SSL_EXT_TLS1_3_ENCRYPTED_EXTENSIONS, init_alpn, tls_parse_ctos_alpn, tls_parse_stoc_alpn, - tls_construct_stoc_alpn, tls_construct_ctos_alpn, final_alpn + tls_construct_stoc_alpn, tls_construct_ctos_alpn, NULL }, #ifndef OPENSSL_NO_SRTP { @@ -937,44 +936,6 @@ static int init_alpn(SSL *s, unsigned int context) return 1; } -static int final_alpn(SSL *s, unsigned int context, int sent, int *al) -{ - const unsigned char *selected = NULL; - unsigned char selected_len = 0; - - if (!s->server) - return 1; - - if (s->ctx->ext.alpn_select_cb != NULL && s->s3->alpn_proposed != NULL) { - int r = s->ctx->ext.alpn_select_cb(s, &selected, &selected_len, - s->s3->alpn_proposed, - (unsigned int)s->s3->alpn_proposed_len, - s->ctx->ext.alpn_select_cb_arg); - - if (r == SSL_TLSEXT_ERR_OK) { - OPENSSL_free(s->s3->alpn_selected); - s->s3->alpn_selected = OPENSSL_memdup(selected, selected_len); - if (s->s3->alpn_selected == NULL) { - *al = SSL_AD_INTERNAL_ERROR; - return 0; - } - s->s3->alpn_selected_len = selected_len; -#ifndef OPENSSL_NO_NEXTPROTONEG - /* ALPN takes precedence over NPN. */ - s->s3->npn_seen = 0; -#endif - } else if (r == SSL_TLSEXT_ERR_NOACK) { - /* Behave as if no callback was present. */ - return 1; - } else { - *al = SSL_AD_NO_APPLICATION_PROTOCOL; - return 0; - } - } - - return 1; -} - static int init_sig_algs(SSL *s, unsigned int context) { /* Clear any signature algorithms extension received */ diff --git a/ssl/statem/statem_srvr.c b/ssl/statem/statem_srvr.c index fad339adb2..8c5f77bce5 100644 --- a/ssl/statem/statem_srvr.c +++ b/ssl/statem/statem_srvr.c @@ -1934,6 +1934,45 @@ static int tls_handle_status_request(SSL *s, int *al) return 1; } +/* + * Call the alpn_select callback if needed. Upon success, returns 1. + * Upon failure, returns 0 and sets |*al| to the appropriate fatal alert. + */ +static int tls_handle_alpn(SSL *s, int *al) +{ + const unsigned char *selected = NULL; + unsigned char selected_len = 0; + + if (s->ctx->ext.alpn_select_cb != NULL && s->s3->alpn_proposed != NULL) { + int r = s->ctx->ext.alpn_select_cb(s, &selected, &selected_len, + s->s3->alpn_proposed, + (unsigned int)s->s3->alpn_proposed_len, + s->ctx->ext.alpn_select_cb_arg); + + if (r == SSL_TLSEXT_ERR_OK) { + OPENSSL_free(s->s3->alpn_selected); + s->s3->alpn_selected = OPENSSL_memdup(selected, selected_len); + if (s->s3->alpn_selected == NULL) { + *al = SSL_AD_INTERNAL_ERROR; + return 0; + } + s->s3->alpn_selected_len = selected_len; +#ifndef OPENSSL_NO_NEXTPROTONEG + /* ALPN takes precedence over NPN. */ + s->s3->npn_seen = 0; +#endif + } else if (r == SSL_TLSEXT_ERR_NOACK) { + /* Behave as if no callback was present. */ + return 1; + } else { + *al = SSL_AD_NO_APPLICATION_PROTOCOL; + return 0; + } + } + + return 1; +} + WORK_STATE tls_post_process_client_hello(SSL *s, WORK_STATE wst) { int al = SSL_AD_HANDSHAKE_FAILURE; @@ -2018,6 +2057,15 @@ WORK_STATE tls_post_process_client_hello(SSL *s, WORK_STATE wst) SSL_R_CLIENTHELLO_TLSEXT); goto f_err; } + /* + * Call alpn_select callback if needed. Has to be done after SNI and + * cipher negotiation (HTTP/2 restricts permitted ciphers). + */ + if (!tls_handle_alpn(s, &al)) { + SSLerr(SSL_F_TLS_POST_PROCESS_CLIENT_HELLO, + SSL_R_CLIENTHELLO_TLSEXT); + goto f_err; + } wst = WORK_MORE_C; } -- 2.25.1