From 4ff65f77b62df12ad75ec232b38627c5fe131041 Mon Sep 17 00:00:00 2001 From: Matt Caswell Date: Wed, 18 Jan 2017 09:38:53 +0000 Subject: [PATCH] Add support for client side parsing of the PSK extension Requires a refactor of the ServerHello parsing, so that we parse first and then subsequently process. This is because the resumption information is held in the extensions block which is parsed last - but we need to know that information earlier. Reviewed-by: Rich Salz (Merged from https://github.com/openssl/openssl/pull/2259) --- include/openssl/ssl.h | 1 + ssl/ssl_err.c | 2 + ssl/statem/extensions.c | 2 +- ssl/statem/extensions_clnt.c | 23 ++++++ ssl/statem/statem_clnt.c | 144 +++++++++++++++++++---------------- ssl/statem/statem_locl.h | 1 + 6 files changed, 108 insertions(+), 65 deletions(-) diff --git a/include/openssl/ssl.h b/include/openssl/ssl.h index e528689c70..52be4064fa 100644 --- a/include/openssl/ssl.h +++ b/include/openssl/ssl.h @@ -2326,6 +2326,7 @@ int ERR_load_SSL_strings(void); # define SSL_F_TLS_PARSE_CTOS_RENEGOTIATE 464 # define SSL_F_TLS_PARSE_CTOS_USE_SRTP 465 # define SSL_F_TLS_PARSE_STOC_KEY_SHARE 445 +# define SSL_F_TLS_PARSE_STOC_PSK 502 # define SSL_F_TLS_PARSE_STOC_RENEGOTIATE 448 # define SSL_F_TLS_PARSE_STOC_USE_SRTP 446 # define SSL_F_TLS_POST_PROCESS_CLIENT_HELLO 378 diff --git a/ssl/ssl_err.c b/ssl/ssl_err.c index 2186f799fc..9b02c58737 100644 --- a/ssl/ssl_err.c +++ b/ssl/ssl_err.c @@ -373,6 +373,7 @@ static ERR_STRING_DATA SSL_str_functs[] = { "tls_parse_ctos_renegotiate"}, {ERR_FUNC(SSL_F_TLS_PARSE_CTOS_USE_SRTP), "tls_parse_ctos_use_srtp"}, {ERR_FUNC(SSL_F_TLS_PARSE_STOC_KEY_SHARE), "tls_parse_stoc_key_share"}, + {ERR_FUNC(SSL_F_TLS_PARSE_STOC_PSK), "tls_parse_stoc_psk"}, {ERR_FUNC(SSL_F_TLS_PARSE_STOC_RENEGOTIATE), "tls_parse_stoc_renegotiate"}, {ERR_FUNC(SSL_F_TLS_PARSE_STOC_USE_SRTP), "tls_parse_stoc_use_srtp"}, @@ -454,6 +455,7 @@ static ERR_STRING_DATA SSL_str_reasons[] = { {ERR_REASON(SSL_R_BAD_PACKET_LENGTH), "bad packet length"}, {ERR_REASON(SSL_R_BAD_PROTOCOL_VERSION_NUMBER), "bad protocol version number"}, + {ERR_REASON(SSL_R_BAD_PSK_IDENTITY), "bad psk identity"}, {ERR_REASON(SSL_R_BAD_RECORD_TYPE), "bad record type"}, {ERR_REASON(SSL_R_BAD_RSA_ENCRYPT), "bad rsa encrypt"}, {ERR_REASON(SSL_R_BAD_SIGNATURE), "bad signature"}, diff --git a/ssl/statem/extensions.c b/ssl/statem/extensions.c index 5837720eb7..4c66b3362f 100644 --- a/ssl/statem/extensions.c +++ b/ssl/statem/extensions.c @@ -277,7 +277,7 @@ static const EXTENSION_DEFINITION ext_defs[] = { TLSEXT_TYPE_psk, EXT_CLIENT_HELLO | EXT_TLS1_3_SERVER_HELLO | EXT_TLS_IMPLEMENTATION_ONLY | EXT_TLS1_3_ONLY, - NULL, NULL, NULL, NULL, tls_construct_ctos_psk, NULL + NULL, NULL, tls_parse_stoc_psk, NULL, tls_construct_ctos_psk, NULL } }; diff --git a/ssl/statem/extensions_clnt.c b/ssl/statem/extensions_clnt.c index 366462ee85..04dbea11fd 100644 --- a/ssl/statem/extensions_clnt.c +++ b/ssl/statem/extensions_clnt.c @@ -1279,3 +1279,26 @@ int tls_parse_stoc_key_share(SSL *s, PACKET *pkt, X509 *x, size_t chainidx, return 1; } + +int tls_parse_stoc_psk(SSL *s, PACKET *pkt, X509 *x, size_t chainidx, int *al) +{ +#ifndef OPENSSL_NO_TLS1_3 + unsigned int identity; + + if (!PACKET_get_net_2(pkt, &identity) || PACKET_remaining(pkt) != 0) { + *al = SSL_AD_HANDSHAKE_FAILURE; + SSLerr(SSL_F_TLS_PARSE_STOC_PSK, SSL_R_LENGTH_MISMATCH); + return 0; + } + + if (s->session->ext.tick_identity != (int)identity) { + *al = SSL_AD_HANDSHAKE_FAILURE; + SSLerr(SSL_F_TLS_PARSE_STOC_PSK, SSL_R_BAD_PSK_IDENTITY); + return 0; + } + + s->hit = 1; +#endif + + return 1; +} diff --git a/ssl/statem/statem_clnt.c b/ssl/statem/statem_clnt.c index 3bcd5902c1..3695c7568c 100644 --- a/ssl/statem/statem_clnt.c +++ b/ssl/statem/statem_clnt.c @@ -1130,6 +1130,7 @@ MSG_PROCESS_RETURN tls_process_server_hello(SSL *s, PACKET *pkt) goto f_err; } + /* We do this immediately so we know what format the ServerHello is in */ protverr = ssl_choose_client_version(s, sversion); if (protverr != 0) { al = SSL_AD_PROTOCOL_VERSION; @@ -1145,8 +1146,6 @@ MSG_PROCESS_RETURN tls_process_server_hello(SSL *s, PACKET *pkt) goto f_err; } - s->hit = 0; - /* Get the session-id. */ if (!SSL_IS_TLS13(s)) { if (!PACKET_get_length_prefixed_1(pkt, &session_id)) { @@ -1173,63 +1172,103 @@ MSG_PROCESS_RETURN tls_process_server_hello(SSL *s, PACKET *pkt) goto f_err; } - /* - * Check if we can resume the session based on external pre-shared secret. - * EAP-FAST (RFC 4851) supports two types of session resumption. - * Resumption based on server-side state works with session IDs. - * Resumption based on pre-shared Protected Access Credentials (PACs) - * works by overriding the SessionTicket extension at the application - * layer, and does not send a session ID. (We do not know whether EAP-FAST - * servers would honour the session ID.) Therefore, the session ID alone - * is not a reliable indicator of session resumption, so we first check if - * we can resume, and later peek at the next handshake message to see if the - * server wants to resume. - */ - if (s->version >= TLS1_VERSION && !SSL_IS_TLS13(s) - && s->ext.session_secret_cb != NULL && s->session->ext.tick) { - const SSL_CIPHER *pref_cipher = NULL; + if (!SSL_IS_TLS13(s)) { + if (!PACKET_get_1(pkt, &compression)) { + SSLerr(SSL_F_TLS_PROCESS_SERVER_HELLO, SSL_R_LENGTH_MISMATCH); + al = SSL_AD_DECODE_ERROR; + goto f_err; + } + } else { + compression = 0; + } + + /* TLS extensions */ + 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_LENGTH); + goto f_err; + } + + context = SSL_IS_TLS13(s) ? EXT_TLS1_3_SERVER_HELLO + : EXT_TLS1_2_SERVER_HELLO; + if (!tls_collect_extensions(s, &extpkt, context, &extensions, &al)) + goto f_err; + + s->hit = 0; + + if (SSL_IS_TLS13(s)) { + /* This will set s->hit if we are resuming */ + if (!tls_parse_extension(s, TLSEXT_IDX_psk, + EXT_TLS1_3_SERVER_HELLO, + extensions, NULL, 0, &al)) + goto f_err; + } else { /* - * s->session->master_key_length is a size_t, but this is an int for - * backwards compat reasons + * Check if we can resume the session based on external pre-shared + * secret. EAP-FAST (RFC 4851) supports two types of session resumption. + * Resumption based on server-side state works with session IDs. + * Resumption based on pre-shared Protected Access Credentials (PACs) + * works by overriding the SessionTicket extension at the application + * layer, and does not send a session ID. (We do not know whether + * EAP-FAST servers would honour the session ID.) Therefore, the session + * ID alone is not a reliable indicator of session resumption, so we + * first check if we can resume, and later peek at the next handshake + * message to see if the server wants to resume. */ - int master_key_length; - master_key_length = sizeof(s->session->master_key); - if (s->ext.session_secret_cb(s, s->session->master_key, - &master_key_length, - NULL, &pref_cipher, - s->ext.session_secret_cb_arg) - && master_key_length > 0) { - s->session->master_key_length = master_key_length; - s->session->cipher = pref_cipher ? - pref_cipher : ssl_get_cipher_by_char(s, cipherchars); - } else { - SSLerr(SSL_F_TLS_PROCESS_SERVER_HELLO, ERR_R_INTERNAL_ERROR); - al = SSL_AD_INTERNAL_ERROR; - goto f_err; + if (s->version >= TLS1_VERSION + && s->ext.session_secret_cb != NULL && s->session->ext.tick) { + const SSL_CIPHER *pref_cipher = NULL; + /* + * s->session->master_key_length is a size_t, but this is an int for + * backwards compat reasons + */ + int master_key_length; + master_key_length = sizeof(s->session->master_key); + if (s->ext.session_secret_cb(s, s->session->master_key, + &master_key_length, + NULL, &pref_cipher, + s->ext.session_secret_cb_arg) + && master_key_length > 0) { + s->session->master_key_length = master_key_length; + s->session->cipher = pref_cipher ? + pref_cipher : ssl_get_cipher_by_char(s, cipherchars); + } else { + SSLerr(SSL_F_TLS_PROCESS_SERVER_HELLO, ERR_R_INTERNAL_ERROR); + al = SSL_AD_INTERNAL_ERROR; + goto f_err; + } } + + if (session_id_len != 0 + && session_id_len == s->session->session_id_length + && memcmp(PACKET_data(&session_id), s->session->session_id, + session_id_len) == 0) + s->hit = 1; } - if (session_id_len != 0 && session_id_len == s->session->session_id_length - && memcmp(PACKET_data(&session_id), s->session->session_id, - session_id_len) == 0) { + if (s->hit) { if (s->sid_ctx_length != s->session->sid_ctx_length - || memcmp(s->session->sid_ctx, s->sid_ctx, s->sid_ctx_length)) { + || memcmp(s->session->sid_ctx, s->sid_ctx, s->sid_ctx_length)) { /* actually a client application bug */ al = SSL_AD_ILLEGAL_PARAMETER; SSLerr(SSL_F_TLS_PROCESS_SERVER_HELLO, SSL_R_ATTEMPT_TO_REUSE_SESSION_IN_DIFFERENT_CONTEXT); goto f_err; } - s->hit = 1; } else { /* * If we were trying for session-id reuse but the server - * didn't echo the ID, make a new SSL_SESSION. + * didn't resume, make a new SSL_SESSION. * In the case of EAP-FAST and PAC, we do not send a session ID, * so the PAC-based session secret is always preserved. It'll be * overwritten if the server refuses resumption. */ - if (s->session->session_id_length > 0) { + if (s->session->session_id_length > 0 + || (SSL_IS_TLS13(s) + && s->session->ext.tick_identity + != TLSEXT_PSK_BAD_IDENTITY)) { s->ctx->stats.sess_miss++; if (!ssl_get_new_session(s, 0)) { goto f_err; @@ -1299,17 +1338,6 @@ MSG_PROCESS_RETURN tls_process_server_hello(SSL *s, PACKET *pkt) goto f_err; } s->s3->tmp.new_cipher = c; - /* lets get the compression algorithm */ - /* COMPRESSION */ - if (!SSL_IS_TLS13(s)) { - if (!PACKET_get_1(pkt, &compression)) { - SSLerr(SSL_F_TLS_PROCESS_SERVER_HELLO, SSL_R_LENGTH_MISMATCH); - al = SSL_AD_DECODE_ERROR; - goto f_err; - } - } else { - compression = 0; - } #ifdef OPENSSL_NO_COMP if (compression != 0) { @@ -1353,19 +1381,7 @@ MSG_PROCESS_RETURN tls_process_server_hello(SSL *s, PACKET *pkt) } #endif - /* TLS extensions */ - 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_LENGTH); - goto f_err; - } - - context = SSL_IS_TLS13(s) ? EXT_TLS1_3_SERVER_HELLO - : EXT_TLS1_2_SERVER_HELLO; - if (!tls_collect_extensions(s, &extpkt, context, &extensions, &al) - || !tls_parse_all_extensions(s, context, extensions, NULL, 0, &al)) + if (!tls_parse_all_extensions(s, context, extensions, NULL, 0, &al)) goto f_err; #ifndef OPENSSL_NO_SCTP diff --git a/ssl/statem/statem_locl.h b/ssl/statem/statem_locl.h index 67858057f7..99f67e51ad 100644 --- a/ssl/statem/statem_locl.h +++ b/ssl/statem/statem_locl.h @@ -321,3 +321,4 @@ int tls_parse_stoc_etm(SSL *s, PACKET *pkt, X509 *x, size_t chainidx, int *al); int tls_parse_stoc_ems(SSL *s, PACKET *pkt, X509 *x, size_t chainidx, int *al); int tls_parse_stoc_key_share(SSL *s, PACKET *pkt, X509 *x, size_t chainidx, int *al); +int tls_parse_stoc_psk(SSL *s, PACKET *pkt, X509 *x, size_t chainidx, int *al); -- 2.34.1