From 94ed2c6739754d13306fe510bb8bc19c2ad42749 Mon Sep 17 00:00:00 2001 From: Matt Caswell Date: Mon, 14 Nov 2016 14:53:31 +0000 Subject: [PATCH] Fixed various style issues in the key_share code Numerous style issues as well as references to TLS1_3_VERSION instead of SSL_IS_TLS13(s) Reviewed-by: Rich Salz --- include/openssl/ssl.h | 2 + ssl/ssl_err.c | 2 + ssl/statem/statem_clnt.c | 52 ++-- ssl/statem/statem_srvr.c | 57 ++-- ssl/t1_enc.c | 2 +- ssl/t1_lib.c | 455 ++++++++++++++++---------------- test/recipes/70-test_tlsextms.t | 4 +- 7 files changed, 288 insertions(+), 286 deletions(-) diff --git a/include/openssl/ssl.h b/include/openssl/ssl.h index f05ec9d0b4..66f7d8c489 100644 --- a/include/openssl/ssl.h +++ b/include/openssl/ssl.h @@ -2074,6 +2074,7 @@ int ERR_load_SSL_strings(void); /* Error codes for the SSL functions. */ /* Function codes. */ +# define SSL_F_ADD_CLIENT_KEY_SHARE_EXT 438 # define SSL_F_CHECK_SUITEB_CIPHER_LIST 331 # define SSL_F_CT_MOVE_SCTS 345 # define SSL_F_CT_STRICT 349 @@ -2105,6 +2106,7 @@ int ERR_load_SSL_strings(void); # define SSL_F_OSSL_STATEM_SERVER13_READ_TRANSITION 437 # define SSL_F_OSSL_STATEM_SERVER_CONSTRUCT_MESSAGE 431 # define SSL_F_OSSL_STATEM_SERVER_READ_TRANSITION 418 +# define SSL_F_PROCESS_KEY_SHARE_EXT 439 # define SSL_F_READ_STATE_MACHINE 352 # define SSL_F_SSL3_CHANGE_CIPHER_STATE 129 # define SSL_F_SSL3_CHECK_CERT_AND_ALGORITHM 130 diff --git a/ssl/ssl_err.c b/ssl/ssl_err.c index 4b4559d08e..235a53ccc8 100644 --- a/ssl/ssl_err.c +++ b/ssl/ssl_err.c @@ -19,6 +19,7 @@ # define ERR_REASON(reason) ERR_PACK(ERR_LIB_SSL,0,reason) static ERR_STRING_DATA SSL_str_functs[] = { + {ERR_FUNC(SSL_F_ADD_CLIENT_KEY_SHARE_EXT), "add_client_key_share_ext"}, {ERR_FUNC(SSL_F_CHECK_SUITEB_CIPHER_LIST), "check_suiteb_cipher_list"}, {ERR_FUNC(SSL_F_CT_MOVE_SCTS), "ct_move_scts"}, {ERR_FUNC(SSL_F_CT_STRICT), "ct_strict"}, @@ -61,6 +62,7 @@ static ERR_STRING_DATA SSL_str_functs[] = { "ossl_statem_server_construct_message"}, {ERR_FUNC(SSL_F_OSSL_STATEM_SERVER_READ_TRANSITION), "ossl_statem_server_read_transition"}, + {ERR_FUNC(SSL_F_PROCESS_KEY_SHARE_EXT), "process_key_share_ext"}, {ERR_FUNC(SSL_F_READ_STATE_MACHINE), "read_state_machine"}, {ERR_FUNC(SSL_F_SSL3_CHANGE_CIPHER_STATE), "ssl3_change_cipher_state"}, {ERR_FUNC(SSL_F_SSL3_CHECK_CERT_AND_ALGORITHM), diff --git a/ssl/statem/statem_clnt.c b/ssl/statem/statem_clnt.c index feda8d2119..f89d317ce7 100644 --- a/ssl/statem/statem_clnt.c +++ b/ssl/statem/statem_clnt.c @@ -112,14 +112,18 @@ static int key_exchange_expected(SSL *s) * server. The message type that the server has sent is provided in |mt|. The * current state is in |s->statem.hand_state|. * - * Return values are: - * 1: Success (transition allowed) - * 0: Error (transition not allowed) + * Return values are 1 for success (transition allowed) and 0 on error + * (transition not allowed) */ static int ossl_statem_client13_read_transition(SSL *s, int mt) { OSSL_STATEM *st = &s->statem; + /* + * TODO(TLS1.3): This is still based on the TLSv1.2 state machine. Over time + * we will update this to look more like real TLSv1.3 + */ + /* * Note: There is no case for TLS_ST_CW_CLNT_HELLO, because we haven't * yet negotiated TLSv1.3 at that point so that is handled by @@ -218,9 +222,8 @@ static int ossl_statem_client13_read_transition(SSL *s, int mt) * server. The message type that the server has sent is provided in |mt|. The * current state is in |s->statem.hand_state|. * - * Return values are: - * 1: Success (transition allowed) - * 0: Error (transition not allowed) + * Return values are 1 for success (transition allowed) and 0 on error + * (transition not allowed) */ int ossl_statem_client_read_transition(SSL *s, int mt) { @@ -387,16 +390,16 @@ int ossl_statem_client_read_transition(SSL *s, int mt) * ossl_statem_client13_write_transition() works out what handshake state to * move to next when the TLSv1.3 client is writing messages to be sent to the * server. - * - * Return values: - * WRITE_TRAN_ERROR - an error occurred - * WRITE_TRAN_CONTINUE - Successful transition, more writing to be done - * WRITE_TRAN_FINISHED - Successful transition, no more writing to be done */ static WRITE_TRAN ossl_statem_client13_write_transition(SSL *s) { OSSL_STATEM *st = &s->statem; + /* + * TODO(TLS1.3): This is still based on the TLSv1.2 state machine. Over time + * we will update this to look more like real TLSv1.3 + */ + /* * Note: There are no cases for TLS_ST_BEFORE or TLS_ST_CW_CLNT_HELLO, * because we haven't negotiated TLSv1.3 yet at that point. They are @@ -408,18 +411,14 @@ static WRITE_TRAN ossl_statem_client13_write_transition(SSL *s) return WRITE_TRAN_ERROR; case TLS_ST_CR_SRVR_DONE: - if (s->s3->tmp.cert_req) - st->hand_state = TLS_ST_CW_CERT; - else - st->hand_state = TLS_ST_CW_CHANGE; + st->hand_state = (s->s3->tmp.cert_req != 0) ? TLS_ST_CW_CERT + : TLS_ST_CW_CHANGE; return WRITE_TRAN_CONTINUE; case TLS_ST_CW_CERT: /* If a non-empty Certificate we also send CertificateVerify */ - if (s->s3->tmp.cert_req == 1) - st->hand_state = TLS_ST_CW_CERT_VRFY; - else - st->hand_state = TLS_ST_CW_CHANGE; + st->hand_state = (s->s3->tmp.cert_req == 1) ? TLS_ST_CW_CERT_VRFY + : TLS_ST_CW_CHANGE; return WRITE_TRAN_CONTINUE; case TLS_ST_CW_CERT_VRFY: @@ -435,30 +434,23 @@ static WRITE_TRAN ossl_statem_client13_write_transition(SSL *s) st->hand_state = TLS_ST_OK; ossl_statem_set_in_init(s, 0); return WRITE_TRAN_CONTINUE; - } else { - return WRITE_TRAN_FINISHED; } + return WRITE_TRAN_FINISHED; case TLS_ST_CR_FINISHED: if (s->hit) { st->hand_state = TLS_ST_CW_CHANGE; return WRITE_TRAN_CONTINUE; - } else { - st->hand_state = TLS_ST_OK; - ossl_statem_set_in_init(s, 0); - return WRITE_TRAN_CONTINUE; } + st->hand_state = TLS_ST_OK; + ossl_statem_set_in_init(s, 0); + return WRITE_TRAN_CONTINUE; } } /* * ossl_statem_client_write_transition() works out what handshake state to * move to next when the client is writing messages to be sent to the server. - * - * Return values: - * WRITE_TRAN_ERROR - an error occurred - * WRITE_TRAN_CONTINUE - Successful transition, more writing to be done - * WRITE_TRAN_FINISHED - Successful transition, no more writing to be done */ WRITE_TRAN ossl_statem_client_write_transition(SSL *s) { diff --git a/ssl/statem/statem_srvr.c b/ssl/statem/statem_srvr.c index c8c0b8e17a..3c4d6ee768 100644 --- a/ssl/statem/statem_srvr.c +++ b/ssl/statem/statem_srvr.c @@ -73,14 +73,18 @@ static STACK_OF(SSL_CIPHER) *ssl_bytes_to_cipher_list(SSL *s, * the client. The message type that the client has sent is provided in |mt|. * The current state is in |s->statem.hand_state|. * - * Valid return values are: - * 1: Success (transition allowed) - * 0: Error (transition not allowed) + * Return values are 1 for success (transition allowed) and 0 on error + * (transition not allowed) */ static int ossl_statem_server13_read_transition(SSL *s, int mt) { OSSL_STATEM *st = &s->statem; + /* + * TODO(TLS1.3): This is still based on the TLSv1.2 state machine. Over time + * we will update this to look more like real TLSv1.3 + */ + /* * Note: There is no case for TLS_ST_BEFORE because at that stage we have * not negotiated TLSv1.3 yet, so that case is handled by @@ -153,9 +157,8 @@ static int ossl_statem_server13_read_transition(SSL *s, int mt) * client. The message type that the client has sent is provided in |mt|. The * current state is in |s->statem.hand_state|. * - * Valid return values are: - * 1: Success (transition allowed) - * 0: Error (transition not allowed) + * Return values are 1 for success (transition allowed) and 0 on error + * (transition not allowed) */ int ossl_statem_server_read_transition(SSL *s, int mt) { @@ -390,16 +393,16 @@ static int send_certificate_request(SSL *s) * ossl_statem_server13_write_transition() works out what handshake state to * move to next when a TLSv1.3 server is writing messages to be sent to the * client. - * - * Return values: - * WRITE_TRAN_ERROR - an error occurred - * WRITE_TRAN_CONTINUE - Successful transition, more writing to be done - * WRITE_TRAN_FINISHED - Successful transition, no more writing to be done */ static WRITE_TRAN ossl_statem_server13_write_transition(SSL *s) { OSSL_STATEM *st = &s->statem; + /* + * TODO(TLS1.3): This is still based on the TLSv1.2 state machine. Over time + * we will update this to look more like real TLSv1.3 + */ + /* * No case for TLS_ST_BEFORE, because at that stage we have not negotiated * TLSv1.3 yet, so that is handled by ossl_statem_server_write_transition() @@ -415,14 +418,12 @@ static WRITE_TRAN ossl_statem_server13_write_transition(SSL *s) return WRITE_TRAN_CONTINUE; case TLS_ST_SW_SRVR_HELLO: - if (s->hit) { - if (s->tlsext_ticket_expected) - st->hand_state = TLS_ST_SW_SESSION_TICKET; - else - st->hand_state = TLS_ST_SW_CHANGE; - } else { + if (s->hit) + st->hand_state = s->tlsext_ticket_expected + ? TLS_ST_SW_SESSION_TICKET : TLS_ST_SW_CHANGE; + else st->hand_state = TLS_ST_SW_CERT; - } + return WRITE_TRAN_CONTINUE; case TLS_ST_SW_CERT: @@ -451,11 +452,10 @@ static WRITE_TRAN ossl_statem_server13_write_transition(SSL *s) st->hand_state = TLS_ST_OK; ossl_statem_set_in_init(s, 0); return WRITE_TRAN_CONTINUE; - } else if (s->tlsext_ticket_expected) { - st->hand_state = TLS_ST_SW_SESSION_TICKET; - } else { - st->hand_state = TLS_ST_SW_CHANGE; } + + st->hand_state = s->tlsext_ticket_expected ? TLS_ST_SW_SESSION_TICKET + : TLS_ST_SW_CHANGE; return WRITE_TRAN_CONTINUE; case TLS_ST_SW_SESSION_TICKET: @@ -467,9 +467,9 @@ static WRITE_TRAN ossl_statem_server13_write_transition(SSL *s) return WRITE_TRAN_CONTINUE; case TLS_ST_SW_FINISHED: - if (s->hit) { + if (s->hit) return WRITE_TRAN_FINISHED; - } + st->hand_state = TLS_ST_OK; ossl_statem_set_in_init(s, 0); return WRITE_TRAN_CONTINUE; @@ -479,11 +479,6 @@ static WRITE_TRAN ossl_statem_server13_write_transition(SSL *s) /* * ossl_statem_server_write_transition() works out what handshake state to move * to next when the server is writing messages to be sent to the client. - * - * Return values: - * WRITE_TRAN_ERROR - an error occurred - * WRITE_TRAN_CONTINUE - Successful transition, more writing to be done - * WRITE_TRAN_FINISHED - Successful transition, no more writing to be done */ WRITE_TRAN ossl_statem_server_write_transition(SSL *s) { @@ -1452,7 +1447,7 @@ MSG_PROCESS_RETURN tls_process_client_hello(SSL *s, PACKET *pkt) /* Check we've got a key_share for TLSv1.3 */ if (s->version == TLS1_3_VERSION && s->s3->peer_tmp == NULL && !s->hit) { /* No suitable share */ - /* TODO(1.3): Send a HelloRetryRequest */ + /* TODO(TLS1.3): Send a HelloRetryRequest */ al = SSL_AD_HANDSHAKE_FAILURE; SSLerr(SSL_F_TLS_PROCESS_CLIENT_HELLO, SSL_R_NO_SUITABLE_KEY_SHARE); goto f_err; @@ -3123,7 +3118,7 @@ MSG_PROCESS_RETURN tls_process_client_certificate(SSL *s, PACKET *pkt) * Freeze the handshake buffer. For version == TLS1_3_VERSION && !ssl3_digest_cached_records(s, 1)) { + if (SSL_IS_TLS13(s) && !ssl3_digest_cached_records(s, 1)) { al = SSL_AD_INTERNAL_ERROR; SSLerr(SSL_F_TLS_PROCESS_CLIENT_CERTIFICATE, ERR_R_INTERNAL_ERROR); goto f_err; diff --git a/ssl/t1_enc.c b/ssl/t1_enc.c index df2ce37030..8cb6dd5a3b 100644 --- a/ssl/t1_enc.c +++ b/ssl/t1_enc.c @@ -480,7 +480,7 @@ int tls1_generate_master_secret(SSL *s, unsigned char *out, unsigned char *p, * handshake has). This will need to be removed later */ if ((s->session->flags & SSL_SESS_FLAG_EXTMS) - && s->version != TLS1_3_VERSION) { + && SSL_IS_TLS13(s)) { unsigned char hash[EVP_MAX_MD_SIZE * 2]; size_t hashlen; /* diff --git a/ssl/t1_lib.c b/ssl/t1_lib.c index 6474c6dbc2..56b6f27e0a 100644 --- a/ssl/t1_lib.c +++ b/ssl/t1_lib.c @@ -1048,7 +1048,7 @@ int ssl_add_clienthello_tlsext(SSL *s, WPACKET *pkt, int *al) break; } } - } else if (s->version >= TLS1_3_VERSION) { + } else if (SSL_IS_TLS13(s)) { /* * TODO(TLS1.3): We always use ECC for TLSv1.3 at the moment. This will * change if we implement DH key shares @@ -1056,7 +1056,7 @@ int ssl_add_clienthello_tlsext(SSL *s, WPACKET *pkt, int *al) using_ecc = 1; } #else - if (s->version >= TLS1_3_VERSION) { + if (SSL_IS_TLS13(s)) { /* Shouldn't happen! */ SSLerr(SSL_F_SSL_ADD_CLIENTHELLO_TLSEXT, ERR_R_INTERNAL_ERROR); return 0; @@ -1423,57 +1423,57 @@ int ssl_add_clienthello_tlsext(SSL *s, WPACKET *pkt, int *al) * now, just send one */ for (i = 0; i < num_curves && sharessent < 1; i++, pcurves += 2) { - if (tls_curve_allowed(s, pcurves, SSL_SECOP_CURVE_SUPPORTED)) { - unsigned char *encodedPoint = NULL; - unsigned int curve_id = 0; - EVP_PKEY *key_share_key = NULL; - size_t encodedlen; - - if (s->s3->tmp.pkey != NULL) { - /* Shouldn't happen! */ - SSLerr(SSL_F_SSL_ADD_CLIENTHELLO_TLSEXT, - ERR_R_INTERNAL_ERROR); - return 0; - } - - /* Generate a key for this key_share */ - curve_id = (pcurves[0] << 8) | pcurves[1]; - key_share_key = ssl_generate_pkey_curve(curve_id); - if (key_share_key == NULL) { - SSLerr(SSL_F_SSL_ADD_CLIENTHELLO_TLSEXT, ERR_R_EVP_LIB); - return 0; - } + unsigned char *encodedPoint = NULL; + unsigned int curve_id = 0; + EVP_PKEY *key_share_key = NULL; + size_t encodedlen; + + if (!tls_curve_allowed(s, pcurves, SSL_SECOP_CURVE_SUPPORTED)) + continue; + + if (s->s3->tmp.pkey != NULL) { + /* Shouldn't happen! */ + SSLerr(SSL_F_SSL_ADD_CLIENTHELLO_TLSEXT, + ERR_R_INTERNAL_ERROR); + return 0; + } - /* Encode the public key. */ - encodedlen = EVP_PKEY_get1_tls_encodedpoint(key_share_key, - &encodedPoint); - if (encodedlen == 0) { - SSLerr(SSL_F_SSL_ADD_CLIENTHELLO_TLSEXT, ERR_R_EC_LIB); - EVP_PKEY_free(key_share_key); - return 0; - } + /* Generate a key for this key_share */ + curve_id = (pcurves[0] << 8) | pcurves[1]; + key_share_key = ssl_generate_pkey_curve(curve_id); + if (key_share_key == NULL) { + SSLerr(SSL_F_SSL_ADD_CLIENTHELLO_TLSEXT, ERR_R_EVP_LIB); + return 0; + } - /* Create KeyShareEntry */ - if (!WPACKET_put_bytes_u16(pkt, curve_id) - || !WPACKET_sub_memcpy_u16(pkt, encodedPoint, - encodedlen)) { - SSLerr(SSL_F_SSL_ADD_CLIENTHELLO_TLSEXT, - ERR_R_INTERNAL_ERROR); - EVP_PKEY_free(key_share_key); - OPENSSL_free(encodedPoint); - return 0; - } + /* Encode the public key. */ + encodedlen = EVP_PKEY_get1_tls_encodedpoint(key_share_key, + &encodedPoint); + if (encodedlen == 0) { + SSLerr(SSL_F_SSL_ADD_CLIENTHELLO_TLSEXT, ERR_R_EC_LIB); + EVP_PKEY_free(key_share_key); + return 0; + } - /* - * TODO(TLS1.3): When changing to send more than one key_share - * we're going to need to be able to save more than one EVP_PKEY - * For now we reuse the existing tmp.pkey - */ - s->s3->group_id = curve_id; - s->s3->tmp.pkey = key_share_key; - sharessent++; + /* Create KeyShareEntry */ + if (!WPACKET_put_bytes_u16(pkt, curve_id) + || !WPACKET_sub_memcpy_u16(pkt, encodedPoint, encodedlen)) { + SSLerr(SSL_F_SSL_ADD_CLIENTHELLO_TLSEXT, + ERR_R_INTERNAL_ERROR); + EVP_PKEY_free(key_share_key); OPENSSL_free(encodedPoint); + return 0; } + + /* + * TODO(TLS1.3): When changing to send more than one key_share we're + * going to need to be able to save more than one EVP_PKEY. For now + * we reuse the existing tmp.pkey + */ + s->s3->group_id = curve_id; + s->s3->tmp.pkey = key_share_key; + sharessent++; + OPENSSL_free(encodedPoint); } if (!WPACKET_close(pkt) || !WPACKET_close(pkt)) { SSLerr(SSL_F_SSL_ADD_CLIENTHELLO_TLSEXT, ERR_R_INTERNAL_ERROR); @@ -1516,6 +1516,59 @@ int ssl_add_clienthello_tlsext(SSL *s, WPACKET *pkt, int *al) return 1; } +/* + * Add the key_share extension. + * + * Returns 1 on success or 0 on failure. + */ +static int add_client_key_share_ext(SSL *s, WPACKET *pkt, int *al) +{ + unsigned char *encodedPoint; + size_t encoded_pt_len = 0; + EVP_PKEY *ckey = s->s3->peer_tmp, *skey = NULL; + + if (ckey == NULL) { + SSLerr(SSL_F_ADD_CLIENT_KEY_SHARE_EXT, ERR_R_INTERNAL_ERROR); + return 0; + } + + if (!WPACKET_put_bytes_u16(pkt, TLSEXT_TYPE_key_share) + || !WPACKET_start_sub_packet_u16(pkt) + || !WPACKET_put_bytes_u16(pkt, s->s3->group_id)) { + SSLerr(SSL_F_ADD_CLIENT_KEY_SHARE_EXT, ERR_R_INTERNAL_ERROR); + return 0; + } + + skey = ssl_generate_pkey(ckey); + + /* Generate encoding of server key */ + encoded_pt_len = EVP_PKEY_get1_tls_encodedpoint(skey, &encodedPoint); + if (encoded_pt_len == 0) { + SSLerr(SSL_F_ADD_CLIENT_KEY_SHARE_EXT, ERR_R_EC_LIB); + EVP_PKEY_free(skey); + return 0; + } + + if (!WPACKET_sub_memcpy_u16(pkt, encodedPoint, encoded_pt_len) + || !WPACKET_close(pkt)) { + SSLerr(SSL_F_ADD_CLIENT_KEY_SHARE_EXT, ERR_R_INTERNAL_ERROR); + EVP_PKEY_free(skey); + OPENSSL_free(encodedPoint); + return 0; + } + OPENSSL_free(encodedPoint); + + /* This causes the crypto state to be updated based on the derived keys */ + s->s3->tmp.pkey = skey; + if (ssl_derive(s, skey, ckey, 1) == 0) { + *al = SSL_AD_INTERNAL_ERROR; + SSLerr(SSL_F_ADD_CLIENT_KEY_SHARE_EXT, ERR_R_INTERNAL_ERROR); + return 0; + } + + return 1; +} + int ssl_add_serverhello_tlsext(SSL *s, WPACKET *pkt, int *al) { #ifndef OPENSSL_NO_NEXTPROTONEG @@ -1649,51 +1702,8 @@ int ssl_add_serverhello_tlsext(SSL *s, WPACKET *pkt, int *al) } #endif - if (s->version == TLS1_3_VERSION && !s->hit) { - unsigned char *encodedPoint; - size_t encoded_pt_len = 0; - EVP_PKEY *ckey = NULL, *skey = NULL; - - ckey = s->s3->peer_tmp; - if (ckey == NULL) { - SSLerr(SSL_F_SSL_ADD_SERVERHELLO_TLSEXT, ERR_R_INTERNAL_ERROR); - return 0; - } - - if (!WPACKET_put_bytes_u16(pkt, TLSEXT_TYPE_key_share) - || !WPACKET_start_sub_packet_u16(pkt) - || !WPACKET_put_bytes_u16(pkt, s->s3->group_id)) { - SSLerr(SSL_F_SSL_ADD_SERVERHELLO_TLSEXT, ERR_R_INTERNAL_ERROR); - return 0; - } - - skey = ssl_generate_pkey(ckey); - - /* Generate encoding of server key */ - encoded_pt_len = EVP_PKEY_get1_tls_encodedpoint(skey, &encodedPoint); - if (encoded_pt_len == 0) { - SSLerr(SSL_F_SSL_ADD_SERVERHELLO_TLSEXT, ERR_R_EC_LIB); - EVP_PKEY_free(skey); - return 0; - } - - if (!WPACKET_sub_memcpy_u16(pkt, encodedPoint, encoded_pt_len) - || !WPACKET_close(pkt)) { - SSLerr(SSL_F_SSL_ADD_SERVERHELLO_TLSEXT, ERR_R_INTERNAL_ERROR); - EVP_PKEY_free(skey); - OPENSSL_free(encodedPoint); - return 0; - } - OPENSSL_free(encodedPoint); - - s->s3->tmp.pkey = skey; - - if (ssl_derive(s, skey, ckey, 1) == 0) { - *al = SSL_AD_INTERNAL_ERROR; - SSLerr(SSL_F_SSL_ADD_SERVERHELLO_TLSEXT, ERR_R_INTERNAL_ERROR); - return 0; - } - } + if (SSL_IS_TLS13(s) && !s->hit && !add_client_key_share_ext(s, pkt, al)) + return 0; if (!custom_ext_add(s, 1, pkt, al)) { SSLerr(SSL_F_SSL_ADD_SERVERHELLO_TLSEXT, ERR_R_INTERNAL_ERROR); @@ -1890,9 +1900,7 @@ static void ssl_check_for_safari(SSL *s, const CLIENTHELLO_MSG *hello) * Process the supported_groups extension if present. Returns success if the * extension is absent, or if it has been successfully processed. * - * Returns - * 1 on success - * 0 on failure + * Returns 1 on success or 0 on failure */ static int tls_process_supported_groups(SSL *s, CLIENTHELLO_MSG *hello) { @@ -1926,11 +1934,8 @@ static int tls_process_supported_groups(SSL *s, CLIENTHELLO_MSG *hello) /* * Checks a list of |groups| to determine if the |group_id| is in it. If it is * and |checkallow| is 1 then additionally check if the group is allowed to be - * used. - * - * Returns: - * 1 if the group is in the list (and allowed if |checkallow| is 1) - * 0 otherwise + * used. Returns 1 if the group is in the list (and allowed if |checkallow| is + * 1) or 0 otherwise. */ static int check_in_list(SSL *s, unsigned int group_id, const unsigned char *groups, size_t num_groups, @@ -1943,6 +1948,7 @@ static int check_in_list(SSL *s, unsigned int group_id, for (i = 0; i < num_groups; i++, groups += 2) { unsigned int share_id = (groups[0] << 8) | (groups[1]); + if (group_id == share_id && (!checkallow || tls_curve_allowed(s, groups, SSL_SECOP_CURVE_CHECK))) { @@ -1950,11 +1956,130 @@ static int check_in_list(SSL *s, unsigned int group_id, } } - if (i == num_groups) { - /* Not in list */ + /* If i == num_groups then not in the list */ + return i < num_groups; +} + +/* + * Process a key_share extension received in the ClientHello. |pkt| contains + * the raw PACKET data for the extension. Returns 1 on success or 0 on failure. + * If a failure occurs then |*al| is set to an appropriate alert value. + */ +static int process_key_share_ext(SSL *s, PACKET *pkt, int *al) +{ + unsigned int group_id; + PACKET key_share_list, encoded_pt; + const unsigned char *curves; + size_t num_curves; + int group_nid, found = 0; + unsigned int curve_flags; + + /* Sanity check */ + if (s->s3->peer_tmp != NULL) { + *al = SSL_AD_INTERNAL_ERROR; + SSLerr(SSL_F_PROCESS_KEY_SHARE_EXT, ERR_R_INTERNAL_ERROR); + return 0; + } + + if (!PACKET_as_length_prefixed_2(pkt, &key_share_list)) { + *al = SSL_AD_HANDSHAKE_FAILURE; + SSLerr(SSL_F_PROCESS_KEY_SHARE_EXT, + SSL_R_LENGTH_MISMATCH); return 0; } + while (PACKET_remaining(&key_share_list) > 0) { + if (!PACKET_get_net_2(&key_share_list, &group_id) + || !PACKET_get_length_prefixed_2(&key_share_list, &encoded_pt) + || PACKET_remaining(&encoded_pt) == 0) { + *al = SSL_AD_HANDSHAKE_FAILURE; + SSLerr(SSL_F_PROCESS_KEY_SHARE_EXT, + SSL_R_LENGTH_MISMATCH); + return 0; + } + + /* + * If we already found a suitable key_share we loop through the + * rest to verify the structure, but don't process them. + */ + if (found) + continue; + + /* Check if this share is in supported_groups sent from client */ + if (!tls1_get_curvelist(s, 1, &curves, &num_curves)) { + *al = SSL_AD_INTERNAL_ERROR; + SSLerr(SSL_F_PROCESS_KEY_SHARE_EXT, + ERR_R_INTERNAL_ERROR); + return 0; + } + if (!check_in_list(s, group_id, curves, num_curves, 0)) { + *al = SSL_AD_HANDSHAKE_FAILURE; + SSLerr(SSL_F_PROCESS_KEY_SHARE_EXT, + SSL_R_BAD_KEY_SHARE); + return 0; + } + + /* Check if this share is for a group we can use */ + if (!tls1_get_curvelist(s, 0, &curves, &num_curves)) { + *al = SSL_AD_INTERNAL_ERROR; + SSLerr(SSL_F_PROCESS_KEY_SHARE_EXT, + ERR_R_INTERNAL_ERROR); + return 0; + } + if (!check_in_list(s, group_id, curves, num_curves, 1)) { + /* Share not suitable */ + continue; + } + + group_nid = tls1_ec_curve_id2nid(group_id, &curve_flags); + + if (group_nid == 0) { + *al = SSL_AD_INTERNAL_ERROR; + SSLerr(SSL_F_PROCESS_KEY_SHARE_EXT, + SSL_R_UNABLE_TO_FIND_ECDH_PARAMETERS); + return 0; + } + + if ((curve_flags & TLS_CURVE_TYPE) == TLS_CURVE_CUSTOM) { + /* Can happen for some curves, e.g. X25519 */ + EVP_PKEY *key = EVP_PKEY_new(); + + if (key == NULL || !EVP_PKEY_set_type(key, group_nid)) { + *al = SSL_AD_INTERNAL_ERROR; + SSLerr(SSL_F_PROCESS_KEY_SHARE_EXT, ERR_R_EVP_LIB); + EVP_PKEY_free(key); + return 0; + } + s->s3->peer_tmp = key; + } else { + /* Set up EVP_PKEY with named curve as parameters */ + EVP_PKEY_CTX *pctx = EVP_PKEY_CTX_new_id(EVP_PKEY_EC, NULL); + if (pctx == NULL + || EVP_PKEY_paramgen_init(pctx) <= 0 + || EVP_PKEY_CTX_set_ec_paramgen_curve_nid(pctx, + group_nid) <= 0 + || EVP_PKEY_paramgen(pctx, &s->s3->peer_tmp) <= 0) { + *al = SSL_AD_INTERNAL_ERROR; + SSLerr(SSL_F_PROCESS_KEY_SHARE_EXT, ERR_R_EVP_LIB); + EVP_PKEY_CTX_free(pctx); + return 0; + } + EVP_PKEY_CTX_free(pctx); + pctx = NULL; + } + s->s3->group_id = group_id; + + if (!EVP_PKEY_set1_tls_encodedpoint(s->s3->peer_tmp, + PACKET_data(&encoded_pt), + PACKET_remaining(&encoded_pt))) { + *al = SSL_AD_DECODE_ERROR; + SSLerr(SSL_F_PROCESS_KEY_SHARE_EXT, SSL_R_BAD_ECPOINT); + return 0; + } + + found = 1; + } + return 1; } @@ -2313,120 +2438,9 @@ static int ssl_scan_clienthello_tlsext(SSL *s, CLIENTHELLO_MSG *hello, int *al) && !(s->options & SSL_OP_NO_ENCRYPT_THEN_MAC)) { s->s3->flags |= TLS1_FLAGS_ENCRYPT_THEN_MAC; } else if (currext->type == TLSEXT_TYPE_key_share - && s->version == TLS1_3_VERSION && !s->hit) { - unsigned int group_id; - PACKET key_share_list, encoded_pt; - const unsigned char *curves; - size_t num_curves; - int group_nid, found = 0; - unsigned int curve_flags; - - /* Sanity check */ - if (s->s3->peer_tmp != NULL) { - *al = SSL_AD_INTERNAL_ERROR; - SSLerr(SSL_F_SSL_SCAN_CLIENTHELLO_TLSEXT, ERR_R_INTERNAL_ERROR); - return 0; - } - - if (!PACKET_as_length_prefixed_2(&currext->data, &key_share_list)) { - *al = SSL_AD_HANDSHAKE_FAILURE; - SSLerr(SSL_F_SSL_SCAN_CLIENTHELLO_TLSEXT, - SSL_R_LENGTH_MISMATCH); - return 0; - } - - while (PACKET_remaining(&key_share_list) > 0) { - if (!PACKET_get_net_2(&key_share_list, &group_id) - || !PACKET_get_length_prefixed_2(&key_share_list, - &encoded_pt) - || PACKET_remaining(&encoded_pt) == 0) { - *al = SSL_AD_HANDSHAKE_FAILURE; - SSLerr(SSL_F_SSL_SCAN_CLIENTHELLO_TLSEXT, - SSL_R_LENGTH_MISMATCH); - return 0; - } - - /* - * If we already found a suitable key_share we loop through the - * rest to verify the structure, but don't process them. - */ - if (found) - continue; - - /* Check this share is in supported_groups */ - if (!tls1_get_curvelist(s, 1, &curves, &num_curves)) { - *al = SSL_AD_INTERNAL_ERROR; - SSLerr(SSL_F_SSL_SCAN_CLIENTHELLO_TLSEXT, - ERR_R_INTERNAL_ERROR); - return 0; - } - if (!check_in_list(s, group_id, curves, num_curves, 0)) { - *al = SSL_AD_HANDSHAKE_FAILURE; - SSLerr(SSL_F_SSL_SCAN_CLIENTHELLO_TLSEXT, - SSL_R_BAD_KEY_SHARE); - return 0; - } - - /* Find a share that we can use */ - if (!tls1_get_curvelist(s, 0, &curves, &num_curves)) { - *al = SSL_AD_INTERNAL_ERROR; - SSLerr(SSL_F_SSL_SCAN_CLIENTHELLO_TLSEXT, - ERR_R_INTERNAL_ERROR); - return 0; - } - if (!check_in_list(s, group_id, curves, num_curves, 1)) { - /* Share not suitable */ - continue; - } - - group_nid = tls1_ec_curve_id2nid(group_id, &curve_flags); - - if (group_nid == 0) { - *al = SSL_AD_INTERNAL_ERROR; - SSLerr(SSL_F_SSL_SCAN_CLIENTHELLO_TLSEXT, - SSL_R_UNABLE_TO_FIND_ECDH_PARAMETERS); - return 0; - } - - if ((curve_flags & TLS_CURVE_TYPE) == TLS_CURVE_CUSTOM) { - /* Can happen for some curves, e.g. X25519 */ - EVP_PKEY *key = EVP_PKEY_new(); - - if (key == NULL || !EVP_PKEY_set_type(key, group_nid)) { - *al = SSL_AD_INTERNAL_ERROR; - SSLerr(SSL_F_SSL_SCAN_CLIENTHELLO_TLSEXT, ERR_R_EVP_LIB); - EVP_PKEY_free(key); - return 0; - } - s->s3->peer_tmp = key; - } else { - /* Set up EVP_PKEY with named curve as parameters */ - EVP_PKEY_CTX *pctx = EVP_PKEY_CTX_new_id(EVP_PKEY_EC, NULL); - if (pctx == NULL - || EVP_PKEY_paramgen_init(pctx) <= 0 - || EVP_PKEY_CTX_set_ec_paramgen_curve_nid(pctx, - group_nid) <= 0 - || EVP_PKEY_paramgen(pctx, &s->s3->peer_tmp) <= 0) { - *al = SSL_AD_INTERNAL_ERROR; - SSLerr(SSL_F_SSL_SCAN_CLIENTHELLO_TLSEXT, ERR_R_EVP_LIB); - EVP_PKEY_CTX_free(pctx); - return 0; - } - EVP_PKEY_CTX_free(pctx); - pctx = NULL; - } - s->s3->group_id = group_id; - - if (!EVP_PKEY_set1_tls_encodedpoint(s->s3->peer_tmp, - PACKET_data(&encoded_pt), - PACKET_remaining(&encoded_pt))) { - *al = SSL_AD_DECODE_ERROR; - SSLerr(SSL_F_SSL_SCAN_CLIENTHELLO_TLSEXT, SSL_R_BAD_ECPOINT); - return 0; - } - - found = 1; - } + && SSL_IS_TLS13(s) && !s->hit + && !process_key_share_ext(s, &currext->data, al)) { + return 0; } /* * Note: extended master secret extension handled in @@ -2717,12 +2731,12 @@ static int ssl_scan_serverhello_tlsext(SSL *s, PACKET *pkt, int *al) && s->s3->tmp.new_cipher->algorithm_enc != SSL_RC4) s->s3->flags |= TLS1_FLAGS_ENCRYPT_THEN_MAC; } else if (type == TLSEXT_TYPE_extended_master_secret && - (SSL_IS_DTLS(s) || s->version < TLS1_3_VERSION)) { + (SSL_IS_DTLS(s) || !SSL_IS_TLS13(s))) { s->s3->flags |= TLS1_FLAGS_RECEIVED_EXTMS; if (!s->hit) s->session->flags |= SSL_SESS_FLAG_EXTMS; } else if (type == TLSEXT_TYPE_key_share - && s->version == TLS1_3_VERSION) { + && SSL_IS_TLS13(s)) { unsigned int group_id; PACKET encoded_pt; EVP_PKEY *ckey = s->s3->tmp.pkey, *skey = NULL; @@ -2752,8 +2766,6 @@ static int ssl_scan_serverhello_tlsext(SSL *s, PACKET *pkt, int *al) return 0; } - skey = ssl_generate_pkey(ckey); - if (!PACKET_as_length_prefixed_2(&spkt, &encoded_pt) || PACKET_remaining(&encoded_pt) == 0) { *al = SSL_AD_DECODE_ERROR; @@ -2762,6 +2774,7 @@ static int ssl_scan_serverhello_tlsext(SSL *s, PACKET *pkt, int *al) return 0; } + skey = ssl_generate_pkey(ckey); if (!EVP_PKEY_set1_tls_encodedpoint(skey, PACKET_data(&encoded_pt), PACKET_remaining(&encoded_pt))) { *al = SSL_AD_DECODE_ERROR; diff --git a/test/recipes/70-test_tlsextms.t b/test/recipes/70-test_tlsextms.t index dd2a6a47f6..dc6cf75cbe 100644 --- a/test/recipes/70-test_tlsextms.t +++ b/test/recipes/70-test_tlsextms.t @@ -57,9 +57,7 @@ setrmextms(0, 0); $proxy->clientflags("-no_tls1_3"); $proxy->start() or plan skip_all => "Unable to start up Proxy for tests"; my $numtests = 9; -if (!disabled("tls1_3")) { - $numtests++; -} +$numtests++ if (!disabled("tls1_3")); plan tests => $numtests; checkmessages(1, "Default extended master secret test", 1, 1, 1); -- 2.25.1