From e7f0d9210c4a421e6306cd9a0c486c3e96be4d25 Mon Sep 17 00:00:00 2001 From: "Dr. Stephen Henson" Date: Fri, 4 Dec 2015 19:48:15 +0000 Subject: [PATCH] Extended master secret fixes and checks. Add new flag TLS1_FLAGS_RECEIVED_EXTMS which is set when the peer sends the extended master secret extension. Server now sends extms if and only if the client sent extms. Check consistency of extms extension when resuming sessions following (where practical) RFC7627. Reviewed-by: Matt Caswell --- include/openssl/ssl3.h | 2 ++ ssl/ssl_locl.h | 5 ++-- ssl/ssl_sess.c | 22 ++++++++++++-- ssl/t1_lib.c | 68 ++++++++++++++++++++++++++++++++---------- 4 files changed, 77 insertions(+), 20 deletions(-) diff --git a/include/openssl/ssl3.h b/include/openssl/ssl3.h index ef93c085a6..2a2974c3ca 100644 --- a/include/openssl/ssl3.h +++ b/include/openssl/ssl3.h @@ -369,6 +369,8 @@ extern "C" { /* Set if we encrypt then mac instead of usual mac then encrypt */ # define TLS1_FLAGS_ENCRYPT_THEN_MAC 0x0100 +/* Set if extended master secret extension received from peer */ +# define TLS1_FLAGS_RECEIVED_EXTMS 0x0200 # define SSL3_MT_HELLO_REQUEST 0 # define SSL3_MT_CLIENT_HELLO 1 diff --git a/ssl/ssl_locl.h b/ssl/ssl_locl.h index 34091d3aa6..1e1b26569c 100644 --- a/ssl/ssl_locl.h +++ b/ssl/ssl_locl.h @@ -2021,8 +2021,9 @@ __owur int tls1_process_heartbeat(SSL *s, unsigned char *p, unsigned int length) __owur int dtls1_process_heartbeat(SSL *s, unsigned char *p, unsigned int length); # endif -__owur int tls1_process_ticket(SSL *s, const PACKET *ext, - const PACKET *session_id, SSL_SESSION **ret); +__owur int tls_check_serverhello_tlsext_early(SSL *s, const PACKET *ext, + const PACKET *session_id, + SSL_SESSION **ret); __owur int tls12_get_sigandhash(unsigned char *p, const EVP_PKEY *pk, const EVP_MD *md); diff --git a/ssl/ssl_sess.c b/ssl/ssl_sess.c index 5265b15983..aff615e0e6 100644 --- a/ssl/ssl_sess.c +++ b/ssl/ssl_sess.c @@ -497,6 +497,10 @@ int ssl_get_new_session(SSL *s, int session) ss->ssl_version = s->version; ss->verify_result = X509_V_OK; + /* If client supports extended master secret set it in session */ + if (s->s3->flags & TLS1_FLAGS_RECEIVED_EXTMS) + ss->flags |= SSL_SESS_FLAG_EXTMS; + return (1); } @@ -533,8 +537,8 @@ int ssl_get_prev_session(SSL *s, const PACKET *ext, const PACKET *session_id) if (len == 0) try_session_cache = 0; - /* sets s->tlsext_ticket_expected */ - r = tls1_process_ticket(s, ext, session_id, &ret); + /* sets s->tlsext_ticket_expected and extended master secret flag */ + r = tls_check_serverhello_tlsext_early(s, ext, session_id, &ret); switch (r) { case -1: /* Error during processing */ fatal = 1; @@ -670,6 +674,20 @@ int ssl_get_prev_session(SSL *s, const PACKET *ext, const PACKET *session_id) goto err; } + /* Check extended master secret extension consistency */ + if (ret->flags & SSL_SESS_FLAG_EXTMS) { + /* If old session includes extms, but new does not: abort handshake */ + if (!(s->s3->flags & TLS1_FLAGS_RECEIVED_EXTMS)) { + SSLerr(SSL_F_SSL_GET_PREV_SESSION, SSL_R_INCONSISTENT_EXTMS); + ssl3_send_alert(s, SSL3_AL_FATAL, SSL_AD_HANDSHAKE_FAILURE); + fatal = 1; + goto err; + } + } else if (s->s3->flags & TLS1_FLAGS_RECEIVED_EXTMS) { + /* If new session includes extms, but old does not: do not resume */ + goto err; + } + s->session_ctx->stats.sess_hit++; SSL_SESSION_free(s->session); diff --git a/ssl/t1_lib.c b/ssl/t1_lib.c index a6f2502c72..83015e85c3 100644 --- a/ssl/t1_lib.c +++ b/ssl/t1_lib.c @@ -1699,7 +1699,7 @@ unsigned char *ssl_add_serverhello_tlsext(SSL *s, unsigned char *buf, } } #endif - if (!s->hit && s->session->flags & SSL_SESS_FLAG_EXTMS) { + if (s->s3->flags & TLS1_FLAGS_RECEIVED_EXTMS) { s2n(TLSEXT_TYPE_extended_master_secret, ret); s2n(0, ret); } @@ -2269,10 +2269,11 @@ static int ssl_scan_clienthello_tlsext(SSL *s, PACKET *pkt, int *al) else if (type == TLSEXT_TYPE_encrypt_then_mac) s->s3->flags |= TLS1_FLAGS_ENCRYPT_THEN_MAC; #endif - else if (type == TLSEXT_TYPE_extended_master_secret) { - if (!s->hit) - s->session->flags |= SSL_SESS_FLAG_EXTMS; - } + /* + * Note: extended master secret extension handled in + * tls_check_serverhello_tlsext_early() + */ + /* * If this ClientHello extension was unhandled and this is a * nonresumed connection, check whether the extension is a custom @@ -2366,6 +2367,8 @@ static int ssl_scan_serverhello_tlsext(SSL *s, PACKET *pkt, int *al) s->s3->flags &= ~TLS1_FLAGS_ENCRYPT_THEN_MAC; #endif + s->s3->flags &= ~TLS1_FLAGS_RECEIVED_EXTMS; + if (!PACKET_get_net_2(pkt, &length)) goto ri_check; @@ -2554,6 +2557,7 @@ static int ssl_scan_serverhello_tlsext(SSL *s, PACKET *pkt, int *al) } #endif else if (type == TLSEXT_TYPE_extended_master_secret) { + s->s3->flags |= TLS1_FLAGS_RECEIVED_EXTMS; if (!s->hit) s->session->flags |= SSL_SESS_FLAG_EXTMS; } @@ -2603,6 +2607,19 @@ static int ssl_scan_serverhello_tlsext(SSL *s, PACKET *pkt, int *al) return 0; } + if (s->hit) { + /* + * Check extended master secret extension is consistent with + * original session. + */ + if (!(s->s3->flags & TLS1_FLAGS_RECEIVED_EXTMS) != + !(s->session->flags & SSL_SESS_FLAG_EXTMS)) { + *al = SSL_AD_HANDSHAKE_FAILURE; + SSLerr(SSL_F_SSL_SCAN_SERVERHELLO_TLSEXT, SSL_R_INCONSISTENT_EXTMS); + return 0; + } + } + return 1; } @@ -2892,8 +2909,11 @@ int ssl_parse_serverhello_tlsext(SSL *s, PACKET *pkt) /*- * Since the server cache lookup is done early on in the processing of the - * ClientHello, and other operations depend on the result, we need to handle - * any TLS session ticket extension at the same time. + * ClientHello and other operations depend on the result some extensions + * need to be handled at the same time. + * + * Two extensions are currently handled, session ticket and extended master + * secret. * * session_id: ClientHello session ID. * ext: ClientHello extensions (including length prefix) @@ -2920,23 +2940,29 @@ int ssl_parse_serverhello_tlsext(SSL *s, PACKET *pkt) * a session ticket or we couldn't use the one it gave us, or if * s->ctx->tlsext_ticket_key_cb asked to renew the client's ticket. * Otherwise, s->tlsext_ticket_expected is set to 0. + * + * For extended master secret flag is set if the extension is present. + * */ -int tls1_process_ticket(SSL *s, const PACKET *ext, const PACKET *session_id, - SSL_SESSION **ret) +int tls_check_serverhello_tlsext_early(SSL *s, const PACKET *ext, + const PACKET *session_id, + SSL_SESSION **ret) { unsigned int i; PACKET local_ext = *ext; int retv = -1; + int have_ticket = 0; + int use_ticket = tls_use_ticket(s); + *ret = NULL; s->tlsext_ticket_expected = 0; + s->s3->flags &= ~TLS1_FLAGS_RECEIVED_EXTMS; /* * If tickets disabled behave as if no ticket present to permit stateful * resumption. */ - if (!tls_use_ticket(s)) - return 0; if ((s->version <= SSL3_VERSION)) return 0; @@ -2957,10 +2983,17 @@ int tls1_process_ticket(SSL *s, const PACKET *ext, const PACKET *session_id, retv = 0; goto end; } - if (type == TLSEXT_TYPE_session_ticket) { + if (type == TLSEXT_TYPE_session_ticket && use_ticket) { int r; unsigned char *etick; + /* Duplicate extension */ + if (have_ticket != 0) { + retv = -1; + goto end; + } + have_ticket = 1; + if (size == 0) { /* * The client will accept a ticket but doesn't currently have @@ -2968,7 +3001,7 @@ int tls1_process_ticket(SSL *s, const PACKET *ext, const PACKET *session_id, */ s->tlsext_ticket_expected = 1; retv = 1; - goto end; + continue; } if (s->tls_session_secret_cb) { /* @@ -2978,7 +3011,7 @@ int tls1_process_ticket(SSL *s, const PACKET *ext, const PACKET *session_id, * calculate the master secret later. */ retv = 2; - goto end; + continue; } if (!PACKET_get_bytes(&local_ext, &etick, size)) { /* Shouldn't ever happen */ @@ -3003,15 +3036,18 @@ int tls1_process_ticket(SSL *s, const PACKET *ext, const PACKET *session_id, retv = -1; break; } - goto end; + continue; } else { + if (type == TLSEXT_TYPE_extended_master_secret) + s->s3->flags |= TLS1_FLAGS_RECEIVED_EXTMS; if (!PACKET_forward(&local_ext, size)) { retv = -1; goto end; } } } - retv = 0; + if (have_ticket == 0) + retv = 0; end: return retv; } -- 2.34.1