From 4be3a7c7aa8bc93ba68253638030d2e5a92bc946 Mon Sep 17 00:00:00 2001 From: Matt Caswell Date: Wed, 16 Aug 2017 12:50:32 +0100 Subject: [PATCH 1/1] Client side sanity check of ALPN after server has accepted early_data Reviewed-by: Ben Kaduk (Merged from https://github.com/openssl/openssl/pull/3926) --- ssl/statem/extensions.c | 23 +++++++++++++++++++++-- ssl/statem/extensions_clnt.c | 31 ++++++++++++++++++++----------- ssl/statem/statem_srvr.c | 26 ++++++++++++++++++++------ 3 files changed, 61 insertions(+), 19 deletions(-) diff --git a/ssl/statem/extensions.c b/ssl/statem/extensions.c index 9d32366e12..b8ab5c880e 100644 --- a/ssl/statem/extensions.c +++ b/ssl/statem/extensions.c @@ -844,7 +844,7 @@ static int final_server_name(SSL *s, unsigned int context, int sent, case SSL_TLSEXT_ERR_NOACK: s->servername_done = 0; - if (s->session->ext.hostname != NULL) + if (s->server && s->session->ext.hostname != NULL) s->ext.early_data_ok = 0; return 1; @@ -945,6 +945,9 @@ static int init_alpn(SSL *s, unsigned int context) static int final_alpn(SSL *s, unsigned int context, int sent, int *al) { + if (!s->server && !sent && s->session->ext.alpn_selected != NULL) + s->ext.early_data_ok = 0; + if (!s->server || !SSL_IS_TLS13(s)) return 1; @@ -1387,8 +1390,24 @@ int tls_psk_do_binder(SSL *s, const EVP_MD *md, const unsigned char *msgstart, static int final_early_data(SSL *s, unsigned int context, int sent, int *al) { - if (!s->server || !sent) + if (!sent) + return 1; + + if (!s->server) { + if (context == SSL_EXT_TLS1_3_ENCRYPTED_EXTENSIONS + && sent + && !s->ext.early_data_ok) { + /* + * If we get here then the server accepted our early_data but we + * later realised that it shouldn't have done (e.g. inconsistent + * ALPN) + */ + *al = SSL_AD_ILLEGAL_PARAMETER; + return 0; + } + return 1; + } if (s->max_early_data == 0 || !s->hit diff --git a/ssl/statem/extensions_clnt.c b/ssl/statem/extensions_clnt.c index cb7b211f7c..bcbcbac873 100644 --- a/ssl/statem/extensions_clnt.c +++ b/ssl/statem/extensions_clnt.c @@ -771,6 +771,7 @@ EXT_RETURN tls_construct_ctos_early_data(SSL *s, WPACKET *pkt, * extension, we set it to accepted. */ s->ext.early_data = SSL_EARLY_DATA_REJECTED; + s->ext.early_data_ok = 1; return EXT_RETURN_SENT; } @@ -1400,15 +1401,22 @@ int tls_parse_stoc_alpn(SSL *s, PACKET *pkt, unsigned int context, X509 *x, } s->s3->alpn_selected_len = len; - /* We also put a copy in the session */ - OPENSSL_free(s->session->ext.alpn_selected); - s->session->ext.alpn_selected = OPENSSL_memdup(s->s3->alpn_selected, - s->s3->alpn_selected_len); - s->session->ext.alpn_selected_len = s->s3->alpn_selected_len; - - if (s->session->ext.alpn_selected == NULL) { - *al = SSL_AD_INTERNAL_ERROR; - return 0; + if (s->session->ext.alpn_selected != NULL + && (s->session->ext.alpn_selected_len != len + || memcmp(s->session->ext.alpn_selected, s->s3->alpn_selected, + len) != 0)) { + /* ALPN not consistent with the old session so cannot use early_data */ + s->ext.early_data_ok = 0; + } + if (!s->hit) { + /* If a new session then update it with the selected ALPN */ + s->session->ext.alpn_selected = + OPENSSL_memdup(s->s3->alpn_selected, s->s3->alpn_selected_len); + if (s->session->ext.alpn_selected == NULL) { + *al = SSL_AD_INTERNAL_ERROR; + return 0; + } + s->session->ext.alpn_selected_len = s->s3->alpn_selected_len; } return 1; @@ -1637,12 +1645,13 @@ int tls_parse_stoc_early_data(SSL *s, PACKET *pkt, unsigned int context, return 0; } - if (s->ext.early_data != SSL_EARLY_DATA_REJECTED + if (!s->ext.early_data_ok || !s->hit || s->session->ext.tick_identity != 0) { /* * If we get here then we didn't send early data, or we didn't resume - * using the first identity so the server should not be accepting it. + * using the first identity, or the SNI/ALPN is not consistent so the + * server should not be accepting it. */ *al = SSL_AD_ILLEGAL_PARAMETER; return 0; diff --git a/ssl/statem/statem_srvr.c b/ssl/statem/statem_srvr.c index 8c2a4e97ac..a3a6bdf6cb 100644 --- a/ssl/statem/statem_srvr.c +++ b/ssl/statem/statem_srvr.c @@ -1962,14 +1962,26 @@ int tls_handle_alpn(SSL *s, int *al) s->s3->npn_seen = 0; #endif - /* Check ALPN is consistent with early_data */ - if (s->ext.early_data_ok - && (s->session->ext.alpn_selected == NULL + /* Check ALPN is consistent with session */ + if (s->session->ext.alpn_selected == NULL || selected_len != s->session->ext.alpn_selected_len || memcmp(selected, s->session->ext.alpn_selected, - selected_len) != 0)) + selected_len) != 0) { + /* Not consistent so can't be used for early_data */ s->ext.early_data_ok = 0; + if (!s->hit) { + /* If a new session update it with the new ALPN value */ + s->session->ext.alpn_selected = OPENSSL_memdup(selected, + selected_len); + if (s->session->ext.alpn_selected == NULL) { + *al = SSL_AD_INTERNAL_ERROR; + return 0; + } + s->session->ext.alpn_selected_len = selected_len; + } + } + return 1; } else if (r != SSL_TLSEXT_ERR_NOACK) { *al = SSL_AD_NO_APPLICATION_PROTOCOL; @@ -1981,9 +1993,11 @@ int tls_handle_alpn(SSL *s, int *al) */ } - /* Check ALPN is consistent with early_data */ - if (s->ext.early_data_ok && s->session->ext.alpn_selected != NULL) + /* Check ALPN is consistent with session */ + if (s->session->ext.alpn_selected != NULL) { + /* Not consistent so can't be used for early_data */ s->ext.early_data_ok = 0; + } return 1; } -- 2.34.1