From 5d6cca05b0c17bbed1e24e17dfddb9a309c0516b Mon Sep 17 00:00:00 2001 From: "Dr. Stephen Henson" Date: Wed, 8 Mar 2017 18:17:17 +0000 Subject: [PATCH] Move parsing and construction of CA names to separate functions Reviewed-by: Rich Salz (Merged from https://github.com/openssl/openssl/pull/2918) --- include/openssl/ssl.h | 1 + ssl/ssl_err.c | 1 + ssl/statem/statem_clnt.c | 58 ++---------------------- ssl/statem/statem_lib.c | 95 ++++++++++++++++++++++++++++++++++++++++ ssl/statem/statem_locl.h | 3 ++ ssl/statem/statem_srvr.c | 29 +----------- 6 files changed, 105 insertions(+), 82 deletions(-) diff --git a/include/openssl/ssl.h b/include/openssl/ssl.h index 8003959f7ced4..bca7f29bab207 100644 --- a/include/openssl/ssl.h +++ b/include/openssl/ssl.h @@ -2174,6 +2174,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_PARSE_CA_NAMES 541 # define SSL_F_PROCESS_KEY_SHARE_EXT 439 # define SSL_F_READ_STATE_MACHINE 352 # define SSL_F_SET_CLIENT_CIPHERSUITE 540 diff --git a/ssl/ssl_err.c b/ssl/ssl_err.c index f7ee171602215..a7821ac4ac10e 100644 --- a/ssl/ssl_err.c +++ b/ssl/ssl_err.c @@ -74,6 +74,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_PARSE_CA_NAMES), "parse_ca_names"}, {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_SET_CLIENT_CIPHERSUITE), "set_client_ciphersuite"}, diff --git a/ssl/statem/statem_clnt.c b/ssl/statem/statem_clnt.c index d153afe78b83f..b0bd0d90eea41 100644 --- a/ssl/statem/statem_clnt.c +++ b/ssl/statem/statem_clnt.c @@ -65,7 +65,6 @@ static MSG_PROCESS_RETURN tls_process_encrypted_extensions(SSL *s, PACKET *pkt); static ossl_inline int cert_req_allowed(SSL *s); static int key_exchange_expected(SSL *s); -static int ca_dn_cmp(const X509_NAME *const *a, const X509_NAME *const *b); static int ssl_cipher_list_to_bytes(SSL *s, STACK_OF(SSL_CIPHER) *sk, WPACKET *pkt); @@ -2328,16 +2327,8 @@ MSG_PROCESS_RETURN tls_process_key_exchange(SSL *s, PACKET *pkt) MSG_PROCESS_RETURN tls_process_certificate_request(SSL *s, PACKET *pkt) { int ret = MSG_PROCESS_ERROR; - unsigned int i, name_len; - X509_NAME *xn = NULL; - const unsigned char *namestart, *namebytes; - STACK_OF(X509_NAME) *ca_sk = NULL; - PACKET cadns; - - if ((ca_sk = sk_X509_NAME_new(ca_dn_cmp)) == NULL) { - SSLerr(SSL_F_TLS_PROCESS_CERTIFICATE_REQUEST, ERR_R_MALLOC_FAILURE); - goto err; - } + int al = -1; + unsigned int i; if (SSL_IS_TLS13(s)) { PACKET reqctx; @@ -2396,42 +2387,11 @@ MSG_PROCESS_RETURN tls_process_certificate_request(SSL *s, PACKET *pkt) } /* get the CA RDNs */ - if (!PACKET_get_length_prefixed_2(pkt, &cadns)) { - ssl3_send_alert(s, SSL3_AL_FATAL, SSL_AD_DECODE_ERROR); - SSLerr(SSL_F_TLS_PROCESS_CERTIFICATE_REQUEST, SSL_R_LENGTH_MISMATCH); + if (!parse_ca_names(s, pkt, &al)) { + ssl3_send_alert(s, SSL3_AL_FATAL, al); goto err; } - while (PACKET_remaining(&cadns)) { - if (!PACKET_get_net_2(&cadns, &name_len) - || !PACKET_get_bytes(&cadns, &namebytes, name_len)) { - ssl3_send_alert(s, SSL3_AL_FATAL, SSL_AD_DECODE_ERROR); - SSLerr(SSL_F_TLS_PROCESS_CERTIFICATE_REQUEST, - SSL_R_LENGTH_MISMATCH); - goto err; - } - - namestart = namebytes; - - if ((xn = d2i_X509_NAME(NULL, (const unsigned char **)&namebytes, - name_len)) == NULL) { - ssl3_send_alert(s, SSL3_AL_FATAL, SSL_AD_DECODE_ERROR); - SSLerr(SSL_F_TLS_PROCESS_CERTIFICATE_REQUEST, ERR_R_ASN1_LIB); - goto err; - } - - if (namebytes != (namestart + name_len)) { - ssl3_send_alert(s, SSL3_AL_FATAL, SSL_AD_DECODE_ERROR); - SSLerr(SSL_F_TLS_PROCESS_CERTIFICATE_REQUEST, - SSL_R_CA_DN_LENGTH_MISMATCH); - goto err; - } - if (!sk_X509_NAME_push(ca_sk, xn)) { - SSLerr(SSL_F_TLS_PROCESS_CERTIFICATE_REQUEST, ERR_R_MALLOC_FAILURE); - goto err; - } - xn = NULL; - } /* TODO(TLS1.3) need to parse and process extensions, for now ignore */ if (SSL_IS_TLS13(s)) { PACKET reqexts; @@ -2452,25 +2412,15 @@ MSG_PROCESS_RETURN tls_process_certificate_request(SSL *s, PACKET *pkt) /* we should setup a certificate to return.... */ s->s3->tmp.cert_req = 1; - sk_X509_NAME_pop_free(s->s3->tmp.ca_names, X509_NAME_free); - s->s3->tmp.ca_names = ca_sk; - ca_sk = NULL; ret = MSG_PROCESS_CONTINUE_PROCESSING; goto done; err: ossl_statem_set_error(s); done: - X509_NAME_free(xn); - sk_X509_NAME_pop_free(ca_sk, X509_NAME_free); return ret; } -static int ca_dn_cmp(const X509_NAME *const *a, const X509_NAME *const *b) -{ - return (X509_NAME_cmp(*a, *b)); -} - MSG_PROCESS_RETURN tls_process_new_session_ticket(SSL *s, PACKET *pkt) { int al = SSL_AD_DECODE_ERROR; diff --git a/ssl/statem/statem_lib.c b/ssl/statem/statem_lib.c index 5164cc0c29aeb..849310ea75ffa 100644 --- a/ssl/statem/statem_lib.c +++ b/ssl/statem/statem_lib.c @@ -1907,3 +1907,98 @@ int create_synthetic_message_hash(SSL *s) return 1; } + +static int ca_dn_cmp(const X509_NAME *const *a, const X509_NAME *const *b) +{ + return X509_NAME_cmp(*a, *b); +} + +int parse_ca_names(SSL *s, PACKET *pkt, int *al) +{ + STACK_OF(X509_NAME) *ca_sk = sk_X509_NAME_new(ca_dn_cmp); + X509_NAME *xn = NULL; + PACKET cadns; + + if (ca_sk == NULL) { + SSLerr(SSL_F_PARSE_CA_NAMES, ERR_R_MALLOC_FAILURE); + goto decerr; + } + /* get the CA RDNs */ + if (!PACKET_get_length_prefixed_2(pkt, &cadns)) { + *al = SSL_AD_DECODE_ERROR; + SSLerr(SSL_F_PARSE_CA_NAMES, SSL_R_LENGTH_MISMATCH); + goto decerr; + } + + while (PACKET_remaining(&cadns)) { + const unsigned char *namestart, *namebytes; + unsigned int name_len; + + if (!PACKET_get_net_2(&cadns, &name_len) + || !PACKET_get_bytes(&cadns, &namebytes, name_len)) { + SSLerr(SSL_F_PARSE_CA_NAMES, SSL_R_LENGTH_MISMATCH); + goto decerr; + } + + namestart = namebytes; + if ((xn = d2i_X509_NAME(NULL, &namebytes, name_len)) == NULL) { + SSLerr(SSL_F_PARSE_CA_NAMES, ERR_R_ASN1_LIB); + goto decerr; + } + if (namebytes != (namestart + name_len)) { + SSLerr(SSL_F_PARSE_CA_NAMES, SSL_R_CA_DN_LENGTH_MISMATCH); + goto decerr; + } + + if (!sk_X509_NAME_push(ca_sk, xn)) { + SSLerr(SSL_F_PARSE_CA_NAMES, ERR_R_MALLOC_FAILURE); + *al = SSL_AD_INTERNAL_ERROR; + goto err; + } + xn = NULL; + } + + sk_X509_NAME_pop_free(s->s3->tmp.ca_names, X509_NAME_free); + s->s3->tmp.ca_names = ca_sk; + + return 1; + + decerr: + *al = SSL_AD_DECODE_ERROR; + err: + sk_X509_NAME_pop_free(ca_sk, X509_NAME_free); + X509_NAME_free(xn); + return 0; +} + +int construct_ca_names(SSL *s, WPACKET *pkt) +{ + STACK_OF(X509_NAME) *ca_sk = SSL_get_client_CA_list(s); + + /* Start sub-packet for client CA list */ + if (!WPACKET_start_sub_packet_u16(pkt)) + return 0; + + if (ca_sk != NULL) { + int i; + + for (i = 0; i < sk_X509_NAME_num(ca_sk); i++) { + unsigned char *namebytes; + X509_NAME *name = sk_X509_NAME_value(ca_sk, i); + int namelen; + + if (name == NULL + || (namelen = i2d_X509_NAME(name, NULL)) < 0 + || !WPACKET_sub_allocate_bytes_u16(pkt, namelen, + &namebytes) + || i2d_X509_NAME(name, &namebytes) != namelen) { + return 0; + } + } + } + + if (!WPACKET_close(pkt)) + return 0; + + return 1; +} diff --git a/ssl/statem/statem_locl.h b/ssl/statem/statem_locl.h index 1c78a4da70b64..f16ba11bd040d 100644 --- a/ssl/statem/statem_locl.h +++ b/ssl/statem/statem_locl.h @@ -80,6 +80,9 @@ typedef int (*confunc_f) (SSL *s, WPACKET *pkt); int check_in_list(SSL *s, unsigned int group_id, const unsigned char *groups, size_t num_groups, int checkallow); int create_synthetic_message_hash(SSL *s); +int parse_ca_names(SSL *s, PACKET *pkt, int *al); +int construct_ca_names(SSL *s, WPACKET *pkt); + /* * TLS/DTLS client state machine functions */ diff --git a/ssl/statem/statem_srvr.c b/ssl/statem/statem_srvr.c index 78f977fcc8516..d42499134643d 100644 --- a/ssl/statem/statem_srvr.c +++ b/ssl/statem/statem_srvr.c @@ -2499,9 +2499,6 @@ int tls_construct_server_key_exchange(SSL *s, WPACKET *pkt) int tls_construct_certificate_request(SSL *s, WPACKET *pkt) { - int i; - STACK_OF(X509_NAME) *sk = NULL; - if (SSL_IS_TLS13(s)) { /* TODO(TLS1.3) for now send empty request context */ if (!WPACKET_put_bytes_u8(pkt, 0)) { @@ -2533,35 +2530,11 @@ int tls_construct_certificate_request(SSL *s, WPACKET *pkt) } } - /* Start sub-packet for client CA list */ - if (!WPACKET_start_sub_packet_u16(pkt)) { + if (!construct_ca_names(s, pkt)) { SSLerr(SSL_F_TLS_CONSTRUCT_CERTIFICATE_REQUEST, ERR_R_INTERNAL_ERROR); goto err; } - sk = SSL_get_client_CA_list(s); - if (sk != NULL) { - for (i = 0; i < sk_X509_NAME_num(sk); i++) { - unsigned char *namebytes; - X509_NAME *name = sk_X509_NAME_value(sk, i); - int namelen; - - if (name == NULL - || (namelen = i2d_X509_NAME(name, NULL)) < 0 - || !WPACKET_sub_allocate_bytes_u16(pkt, namelen, - &namebytes) - || i2d_X509_NAME(name, &namebytes) != namelen) { - SSLerr(SSL_F_TLS_CONSTRUCT_CERTIFICATE_REQUEST, - ERR_R_INTERNAL_ERROR); - goto err; - } - } - } - /* else no CA names */ - if (!WPACKET_close(pkt)) { - SSLerr(SSL_F_TLS_CONSTRUCT_CERTIFICATE_REQUEST, ERR_R_INTERNAL_ERROR); - goto err; - } /* * TODO(TLS1.3) implement configurable certificate_extensions * For now just send zero length extensions.