From 332eb3908883fcaac8483dcc895571b0a3c2813a Mon Sep 17 00:00:00 2001 From: Matt Caswell Date: Mon, 28 Nov 2016 16:15:51 +0000 Subject: [PATCH] Move ServerHello extension parsing into the new extension framework Perl changes reviewed by Richard Levitte. Non-perl changes reviewed by Rich Salz Reviewed-by: Rich Salz Reviewed-by: Richard Levitte --- include/openssl/ssl.h | 3 + ssl/ssl_err.c | 4 + ssl/ssl_locl.h | 1 - ssl/statem/extensions.c | 145 ++++++++++++++++++++++++++---- ssl/statem/extensions_clnt.c | 168 ----------------------------------- ssl/statem/statem_clnt.c | 37 +++++--- ssl/statem/statem_locl.h | 1 - 7 files changed, 163 insertions(+), 196 deletions(-) diff --git a/include/openssl/ssl.h b/include/openssl/ssl.h index 9dd87d16e5..e2f61a5d12 100644 --- a/include/openssl/ssl.h +++ b/include/openssl/ssl.h @@ -2308,6 +2308,9 @@ int ERR_load_SSL_strings(void); # define SSL_F_TLS_CONSTRUCT_SERVER_SESSION_TICKET 460 # define SSL_F_TLS_CONSTRUCT_SERVER_STATUS_REQUEST 461 # define SSL_F_TLS_CONSTRUCT_SERVER_USE_SRTP 462 +# define SSL_F_TLS_EXT_FINAL_ 484 +# define SSL_F_TLS_EXT_FINAL_EC_PT_FORMATS 485 +# define SSL_F_TLS_EXT_FINAL_EMS 486 # define SSL_F_TLS_EXT_FINAL_RENEGOTIATE 483 # define SSL_F_TLS_GET_MESSAGE_BODY 351 # define SSL_F_TLS_GET_MESSAGE_HEADER 387 diff --git a/ssl/ssl_err.c b/ssl/ssl_err.c index 11c63c7390..ca96c72bb5 100644 --- a/ssl/ssl_err.c +++ b/ssl/ssl_err.c @@ -338,6 +338,10 @@ static ERR_STRING_DATA SSL_str_functs[] = { "tls_construct_server_status_request"}, {ERR_FUNC(SSL_F_TLS_CONSTRUCT_SERVER_USE_SRTP), "tls_construct_server_use_srtp"}, + {ERR_FUNC(SSL_F_TLS_EXT_FINAL_), "tls_ext_final_ems"}, + {ERR_FUNC(SSL_F_TLS_EXT_FINAL_EC_PT_FORMATS), + "tls_ext_final_ec_pt_formats"}, + {ERR_FUNC(SSL_F_TLS_EXT_FINAL_EMS), "tls_ext_final_ems"}, {ERR_FUNC(SSL_F_TLS_EXT_FINAL_RENEGOTIATE), "tls_ext_final_renegotiate"}, {ERR_FUNC(SSL_F_TLS_GET_MESSAGE_BODY), "tls_get_message_body"}, {ERR_FUNC(SSL_F_TLS_GET_MESSAGE_HEADER), "tls_get_message_header"}, diff --git a/ssl/ssl_locl.h b/ssl/ssl_locl.h index 3fc5b5f93c..e2a2ff16b1 100644 --- a/ssl/ssl_locl.h +++ b/ssl/ssl_locl.h @@ -2103,7 +2103,6 @@ __owur int tls1_get_curvelist(SSL *s, int sess, const unsigned char **pcurves, void ssl_set_default_md(SSL *s); __owur int tls1_set_server_sigalgs(SSL *s); -__owur int ssl_parse_serverhello_tlsext(SSL *s, PACKET *pkt); __owur RAW_EXTENSION *tls_get_extension_by_type(RAW_EXTENSION *exts, size_t numexts, unsigned int type); diff --git a/ssl/statem/extensions.c b/ssl/statem/extensions.c index e8bd3be8a4..261ee2ef38 100644 --- a/ssl/statem/extensions.c +++ b/ssl/statem/extensions.c @@ -15,7 +15,14 @@ static int tls_ext_final_renegotiate(SSL *s, unsigned int context, int sent, static int tls_ext_init_server_name(SSL *s, unsigned int context); static int tls_ext_final_server_name(SSL *s, unsigned int context, int sent, int *al); +#ifndef OPENSSL_NO_EC +static int tls_ext_final_ec_pt_formats(SSL *s, unsigned int context, int sent, + int *al); +#endif +static int tls_ext_init_session_ticket(SSL *s, unsigned int context); static int tls_ext_init_status_request(SSL *s, unsigned int context); +static int tls_ext_final_status_request(SSL *s, unsigned int context, int sent, + int *al); #ifndef OPENSSL_NO_NEXTPROTONEG static int tls_ext_init_npn(SSL *s, unsigned int context); #endif @@ -26,6 +33,8 @@ static int tls_ext_init_sig_algs(SSL *s, unsigned int context); static int tls_ext_init_srp(SSL *s, unsigned int context); #endif static int tls_ext_init_etm(SSL *s, unsigned int context); +static int tls_ext_init_ems(SSL *s, unsigned int context); +static int tls_ext_final_ems(SSL *s, unsigned int context, int sent, int *al); #ifndef OPENSSL_NO_SRTP static int tls_ext_init_srtp(SSL *s, unsigned int context); #endif @@ -122,7 +131,7 @@ static const EXTENSION_DEFINITION ext_defs[] = { tls_parse_server_ec_pt_formats, tls_construct_server_ec_pt_formats, tls_construct_client_ec_pt_formats, - NULL, + tls_ext_final_ec_pt_formats, EXT_CLIENT_HELLO | EXT_TLS1_2_AND_BELOW_ONLY }, { @@ -138,7 +147,7 @@ static const EXTENSION_DEFINITION ext_defs[] = { #endif { TLSEXT_TYPE_session_ticket, - NULL, + tls_ext_init_session_ticket, tls_parse_client_session_ticket, tls_parse_server_session_ticket, tls_construct_server_session_ticket, @@ -164,7 +173,7 @@ static const EXTENSION_DEFINITION ext_defs[] = { tls_parse_server_status_request, tls_construct_server_status_request, tls_construct_client_status_request, - NULL, + tls_ext_final_status_request, EXT_CLIENT_HELLO | EXT_TLS1_2_SERVER_HELLO | EXT_TLS1_3_CERTIFICATE }, @@ -239,12 +248,12 @@ static const EXTENSION_DEFINITION ext_defs[] = { #endif { TLSEXT_TYPE_extended_master_secret, - NULL, + tls_ext_init_ems, tls_parse_client_ems, tls_parse_server_ems, tls_construct_server_ems, tls_construct_client_ems, - NULL, + tls_ext_final_ems, EXT_CLIENT_HELLO | EXT_TLS1_2_SERVER_HELLO | EXT_TLS1_2_AND_BELOW_ONLY }, { @@ -697,8 +706,22 @@ int tls_construct_extensions(SSL *s, WPACKET *pkt, unsigned int context, static int tls_ext_final_renegotiate(SSL *s, unsigned int context, int sent, int *al) { - if (!s->server) + if (!s->server) { + /* + * Check if we can connect to a server that doesn't support safe + * renegotiation + */ + if (!(s->options & SSL_OP_LEGACY_SERVER_CONNECT) + && !(s->options & SSL_OP_ALLOW_UNSAFE_LEGACY_RENEGOTIATION) + && !sent) { + *al = SSL_AD_HANDSHAKE_FAILURE; + SSLerr(SSL_F_TLS_EXT_FINAL_RENEGOTIATE, + SSL_R_UNSAFE_LEGACY_RENEGOTIATION_DISABLED); + return 0; + } + return 1; + } /* Need RI if renegotiating */ if (s->renegotiate @@ -710,6 +733,7 @@ static int tls_ext_final_renegotiate(SSL *s, unsigned int context, int sent, return 0; } + return 1; } @@ -727,9 +751,6 @@ static int tls_ext_final_server_name(SSL *s, unsigned int context, int sent, int ret = SSL_TLSEXT_ERR_NOACK; int altmp = SSL_AD_UNRECOGNIZED_NAME; - if (!s->server) - return 1; - if (s->ctx != NULL && s->ctx->tlsext_servername_callback != 0) ret = s->ctx->tlsext_servername_callback(s, &altmp, s->ctx->tlsext_servername_arg); @@ -756,6 +777,58 @@ static int tls_ext_final_server_name(SSL *s, unsigned int context, int sent, } } +#ifndef OPENSSL_NO_EC +static int tls_ext_final_ec_pt_formats(SSL *s, unsigned int context, int sent, + int *al) +{ + unsigned long alg_k, alg_a; + + if (s->server) + return 1; + + alg_k = s->s3->tmp.new_cipher->algorithm_mkey; + alg_a = s->s3->tmp.new_cipher->algorithm_auth; + + /* + * If we are client and using an elliptic curve cryptography cipher + * suite, then if server returns an EC point formats lists extension it + * must contain uncompressed. + */ + if ((s->tlsext_ecpointformatlist != NULL) + && (s->tlsext_ecpointformatlist_length > 0) + && (s->session->tlsext_ecpointformatlist != NULL) + && (s->session->tlsext_ecpointformatlist_length > 0) + && ((alg_k & SSL_kECDHE) || (alg_a & SSL_aECDSA))) { + /* we are using an ECC cipher */ + size_t i; + unsigned char *list; + int found_uncompressed = 0; + list = s->session->tlsext_ecpointformatlist; + for (i = 0; i < s->session->tlsext_ecpointformatlist_length; i++) { + if (*(list++) == TLSEXT_ECPOINTFORMAT_uncompressed) { + found_uncompressed = 1; + break; + } + } + if (!found_uncompressed) { + SSLerr(SSL_F_TLS_EXT_FINAL_EC_PT_FORMATS, + SSL_R_TLS_INVALID_ECPOINTFORMAT_LIST); + return 0; + } + } + + return 1; +} +#endif + +static int tls_ext_init_session_ticket(SSL *s, unsigned int context) +{ + if (!s->server) + s->tlsext_ticket_expected = 0; + + return 1; +} + static int tls_ext_init_status_request(SSL *s, unsigned int context) { if (s->server) @@ -764,11 +837,27 @@ static int tls_ext_init_status_request(SSL *s, unsigned int context) return 1; } +static int tls_ext_final_status_request(SSL *s, unsigned int context, int sent, + int *al) +{ + if (s->server) + return 1; + + /* + * Ensure we get sensible values passed to tlsext_status_cb in the event + * that we don't receive a status message + */ + OPENSSL_free(s->tlsext_ocsp_resp); + s->tlsext_ocsp_resp = NULL; + s->tlsext_ocsp_resplen = 0; + + return 1; +} + #ifndef OPENSSL_NO_NEXTPROTONEG static int tls_ext_init_npn(SSL *s, unsigned int context) { - if (s->server) - s->s3->next_proto_neg_seen = 0; + s->s3->next_proto_neg_seen = 0; return 1; } @@ -776,15 +865,14 @@ static int tls_ext_init_npn(SSL *s, unsigned int context) static int tls_ext_init_alpn(SSL *s, unsigned int context) { + OPENSSL_free(s->s3->alpn_selected); + s->s3->alpn_selected = NULL; if (s->server) { - OPENSSL_free(s->s3->alpn_selected); - s->s3->alpn_selected = NULL; s->s3->alpn_selected_len = 0; OPENSSL_free(s->s3->alpn_proposed); s->s3->alpn_proposed = NULL; s->s3->alpn_proposed_len = 0; } - return 1; } @@ -844,8 +932,33 @@ static int tls_ext_init_srp(SSL *s, unsigned int context) static int tls_ext_init_etm(SSL *s, unsigned int context) { - if (s->server) - s->s3->flags &= ~TLS1_FLAGS_ENCRYPT_THEN_MAC; + s->s3->flags &= ~TLS1_FLAGS_ENCRYPT_THEN_MAC; + + return 1; +} + +static int tls_ext_init_ems(SSL *s, unsigned int context) +{ + if (!s->server) + s->s3->flags &= ~TLS1_FLAGS_RECEIVED_EXTMS; + + return 1; +} + +static int tls_ext_final_ems(SSL *s, unsigned int context, int sent, int *al) +{ + if (!s->server && 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_TLS_EXT_FINAL_EMS, SSL_R_INCONSISTENT_EXTMS); + return 0; + } + } return 1; } diff --git a/ssl/statem/extensions_clnt.c b/ssl/statem/extensions_clnt.c index 93d4178ed9..df12969c43 100644 --- a/ssl/statem/extensions_clnt.c +++ b/ssl/statem/extensions_clnt.c @@ -1033,171 +1033,3 @@ int tls_parse_server_key_share(SSL *s, PACKET *pkt, int *al) return 1; } - -static int ssl_scan_serverhello_tlsext(SSL *s, PACKET *pkt, int *al) -{ - RAW_EXTENSION *extensions = NULL; - PACKET extpkt; - -#ifndef OPENSSL_NO_NEXTPROTONEG - s->s3->next_proto_neg_seen = 0; -#endif - s->tlsext_ticket_expected = 0; - - OPENSSL_free(s->s3->alpn_selected); - s->s3->alpn_selected = NULL; - - s->s3->flags &= ~TLS1_FLAGS_ENCRYPT_THEN_MAC; - - s->s3->flags &= ~TLS1_FLAGS_RECEIVED_EXTMS; - - if (!PACKET_as_length_prefixed_2(pkt, &extpkt)) { - /* Extensions block may be completely absent in SSLv3 */ - if (s->version != SSL3_VERSION || PACKET_remaining(pkt) != 0) { - *al = SSL_AD_DECODE_ERROR; - SSLerr(SSL_F_SSL_SCAN_SERVERHELLO_TLSEXT, SSL_R_BAD_LENGTH); - return 0; - } - PACKET_null_init(&extpkt); - } - - /* - * TODO(TLS1.3): We give multiple contexts for now until we're ready to - * give something more specific - */ - - if (!tls_collect_extensions(s, &extpkt, EXT_TLS1_2_SERVER_HELLO - | EXT_TLS1_3_SERVER_HELLO - | EXT_TLS1_3_ENCRYPTED_EXTENSIONS - | EXT_TLS1_3_CERTIFICATE, - &extensions, al)) - return 0; - - /* - * Determine if we need to see RI. Strictly speaking if we want to avoid - * an attack we should *always* see RI even on initial server hello - * because the client doesn't see any renegotiation during an attack. - * However this would mean we could not connect to any server which - * doesn't support RI so for the immediate future tolerate RI absence - */ - if (!(s->options & SSL_OP_LEGACY_SERVER_CONNECT) - && !(s->options & SSL_OP_ALLOW_UNSAFE_LEGACY_RENEGOTIATION) - && !extensions[TLSEXT_IDX_renegotiate].present) { - *al = SSL_AD_HANDSHAKE_FAILURE; - SSLerr(SSL_F_SSL_SCAN_SERVERHELLO_TLSEXT, - SSL_R_UNSAFE_LEGACY_RENEGOTIATION_DISABLED); - return 0; - } - - if (!tls_parse_all_extensions(s, EXT_TLS1_2_SERVER_HELLO - | EXT_TLS1_3_SERVER_HELLO - | EXT_TLS1_3_ENCRYPTED_EXTENSIONS - | EXT_TLS1_3_CERTIFICATE, - extensions,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; -} - -static int ssl_check_serverhello_tlsext(SSL *s) -{ - int ret = SSL_TLSEXT_ERR_NOACK; - int al = SSL_AD_UNRECOGNIZED_NAME; - -#ifndef OPENSSL_NO_EC - /* - * If we are client and using an elliptic curve cryptography cipher - * suite, then if server returns an EC point formats lists extension it - * must contain uncompressed. - */ - unsigned long alg_k = s->s3->tmp.new_cipher->algorithm_mkey; - unsigned long alg_a = s->s3->tmp.new_cipher->algorithm_auth; - if ((s->tlsext_ecpointformatlist != NULL) - && (s->tlsext_ecpointformatlist_length > 0) - && (s->session->tlsext_ecpointformatlist != NULL) - && (s->session->tlsext_ecpointformatlist_length > 0) - && ((alg_k & SSL_kECDHE) || (alg_a & SSL_aECDSA))) { - /* we are using an ECC cipher */ - size_t i; - unsigned char *list; - int found_uncompressed = 0; - list = s->session->tlsext_ecpointformatlist; - for (i = 0; i < s->session->tlsext_ecpointformatlist_length; i++) { - if (*(list++) == TLSEXT_ECPOINTFORMAT_uncompressed) { - found_uncompressed = 1; - break; - } - } - if (!found_uncompressed) { - SSLerr(SSL_F_SSL_CHECK_SERVERHELLO_TLSEXT, - SSL_R_TLS_INVALID_ECPOINTFORMAT_LIST); - return -1; - } - } - ret = SSL_TLSEXT_ERR_OK; -#endif /* OPENSSL_NO_EC */ - - if (s->ctx != NULL && s->ctx->tlsext_servername_callback != 0) - ret = - s->ctx->tlsext_servername_callback(s, &al, - s->ctx->tlsext_servername_arg); - else if (s->initial_ctx != NULL - && s->initial_ctx->tlsext_servername_callback != 0) - ret = - s->initial_ctx->tlsext_servername_callback(s, &al, - s-> - initial_ctx->tlsext_servername_arg); - - /* - * Ensure we get sensible values passed to tlsext_status_cb in the event - * that we don't receive a status message - */ - OPENSSL_free(s->tlsext_ocsp_resp); - s->tlsext_ocsp_resp = NULL; - s->tlsext_ocsp_resplen = 0; - - 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; - - case SSL_TLSEXT_ERR_NOACK: - s->servername_done = 0; - default: - return 1; - } -} - -int ssl_parse_serverhello_tlsext(SSL *s, PACKET *pkt) -{ - int al = -1; - if (s->version < SSL3_VERSION) - return 1; - if (ssl_scan_serverhello_tlsext(s, pkt, &al) <= 0) { - ssl3_send_alert(s, SSL3_AL_FATAL, al); - return 0; - } - - if (ssl_check_serverhello_tlsext(s) <= 0) { - SSLerr(SSL_F_SSL_PARSE_SERVERHELLO_TLSEXT, SSL_R_SERVERHELLO_TLSEXT); - return 0; - } - return 1; -} diff --git a/ssl/statem/statem_clnt.c b/ssl/statem/statem_clnt.c index cded5b96ec..aa1e6e1a89 100644 --- a/ssl/statem/statem_clnt.c +++ b/ssl/statem/statem_clnt.c @@ -1058,13 +1058,14 @@ MSG_PROCESS_RETURN tls_process_server_hello(SSL *s, PACKET *pkt) { STACK_OF(SSL_CIPHER) *sk; const SSL_CIPHER *c; - PACKET session_id; + PACKET session_id, extpkt; size_t session_id_len; const unsigned char *cipherchars; int i, al = SSL_AD_INTERNAL_ERROR; unsigned int compression; unsigned int sversion; int protverr; + RAW_EXTENSION *extensions = NULL; #ifndef OPENSSL_NO_COMP SSL_COMP *comp; #endif @@ -1297,17 +1298,34 @@ MSG_PROCESS_RETURN tls_process_server_hello(SSL *s, PACKET *pkt) #endif /* TLS extensions */ - if (!ssl_parse_serverhello_tlsext(s, pkt)) { - SSLerr(SSL_F_TLS_PROCESS_SERVER_HELLO, SSL_R_PARSE_TLSEXT); - goto err; - } - - if (PACKET_remaining(pkt) != 0) { - /* wrong packet length */ + 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_PACKET_LENGTH); + SSLerr(SSL_F_TLS_PROCESS_SERVER_HELLO, SSL_R_BAD_LENGTH); goto f_err; } + + /* + * TODO(TLS1.3): We give multiple contexts for now until we're ready to + * give something more specific + */ + + if (!tls_collect_extensions(s, &extpkt, EXT_TLS1_2_SERVER_HELLO + | EXT_TLS1_3_SERVER_HELLO + | EXT_TLS1_3_ENCRYPTED_EXTENSIONS + | EXT_TLS1_3_CERTIFICATE, + &extensions, &al)) + goto f_err; + + + if (!tls_parse_all_extensions(s, EXT_TLS1_2_SERVER_HELLO + | EXT_TLS1_3_SERVER_HELLO + | EXT_TLS1_3_ENCRYPTED_EXTENSIONS + | EXT_TLS1_3_CERTIFICATE, + extensions, &al)) + goto f_err; + #ifndef OPENSSL_NO_SCTP if (SSL_IS_DTLS(s) && s->hit) { unsigned char sctpauthkey[64]; @@ -1350,7 +1368,6 @@ MSG_PROCESS_RETURN tls_process_server_hello(SSL *s, PACKET *pkt) return MSG_PROCESS_CONTINUE_READING; f_err: ssl3_send_alert(s, SSL3_AL_FATAL, al); - err: ossl_statem_set_error(s); return MSG_PROCESS_ERROR; } diff --git a/ssl/statem/statem_locl.h b/ssl/statem/statem_locl.h index 1984087c1b..b0c77ce640 100644 --- a/ssl/statem/statem_locl.h +++ b/ssl/statem/statem_locl.h @@ -266,4 +266,3 @@ int tls_parse_server_use_srtp(SSL *s, PACKET *pkt, int *al); int tls_parse_server_etm(SSL *s, PACKET *pkt, int *al); int tls_parse_server_ems(SSL *s, PACKET *pkt, int *al); int tls_parse_server_key_share(SSL *s, PACKET *pkt, int *al); -int ssl_parse_serverhello_tlsext(SSL *s, PACKET *pkt); -- 2.34.1