From 75c13e7830653eee4b61dd96ceea7c446c381316 Mon Sep 17 00:00:00 2001 From: "Dr. Stephen Henson" Date: Thu, 23 Feb 2017 22:12:28 +0000 Subject: [PATCH] Tidy up certificate type handling. The certificate types used to be held in a fixed length array or (if it was too long) a malloced buffer. This was done to retain binary compatibility. The code can be simplified now SSL is opaque by always using a malloced buffer. Reviewed-by: Rich Salz (Merged from https://github.com/openssl/openssl/pull/2733) --- ssl/s3_lib.c | 30 +++++++++++++----------------- ssl/ssl_cert.c | 11 +++++------ ssl/ssl_locl.h | 15 ++++++--------- ssl/statem/statem_clnt.c | 24 ++++++------------------ ssl/t1_lib.c | 19 +++++++------------ 5 files changed, 37 insertions(+), 62 deletions(-) diff --git a/ssl/s3_lib.c b/ssl/s3_lib.c index 6449f8c4f1..1df1afa02e 100644 --- a/ssl/s3_lib.c +++ b/ssl/s3_lib.c @@ -2925,6 +2925,7 @@ void ssl3_free(SSL *s) s->s3->tmp.pkey = NULL; #endif + OPENSSL_free(s->s3->tmp.ctype); sk_X509_NAME_pop_free(s->s3->tmp.ca_names, X509_NAME_free); OPENSSL_free(s->s3->tmp.ciphers_raw); OPENSSL_clear_free(s->s3->tmp.pms, s->s3->tmp.pmslen); @@ -2943,6 +2944,7 @@ void ssl3_free(SSL *s) void ssl3_clear(SSL *s) { ssl3_cleanup_key_block(s); + OPENSSL_free(s->s3->tmp.ctype); sk_X509_NAME_pop_free(s->s3->tmp.ca_names, X509_NAME_free); OPENSSL_free(s->s3->tmp.ciphers_raw); OPENSSL_clear_free(s->s3->tmp.pms, s->s3->tmp.pmslen); @@ -3234,14 +3236,9 @@ long ssl3_ctrl(SSL *s, int cmd, long larg, void *parg) const unsigned char **pctype = parg; if (s->server || !s->s3->tmp.cert_req) return 0; - if (s->cert->ctypes) { - if (pctype) - *pctype = s->cert->ctypes; - return (int)s->cert->ctype_num; - } if (pctype) - *pctype = (unsigned char *)s->s3->tmp.ctype; - return s->s3->tmp.ctype_num; + *pctype = s->s3->tmp.ctype; + return s->s3->tmp.ctype_len; } case SSL_CTRL_SET_CLIENT_CERT_TYPES: @@ -3787,9 +3784,8 @@ int ssl3_get_req_cert_type(SSL *s, WPACKET *pkt) uint32_t alg_k, alg_a = 0; /* If we have custom certificate types set, use them */ - if (s->cert->ctypes) { - return WPACKET_memcpy(pkt, s->cert->ctypes, s->cert->ctype_num); - } + if (s->cert->ctype) + return WPACKET_memcpy(pkt, s->cert->ctype, s->cert->ctype_len); /* Get mask of algorithms disabled by signature list */ ssl_set_sig_mask(&alg_a, s, SSL_SECOP_SIGALG_MASK); @@ -3837,17 +3833,17 @@ int ssl3_get_req_cert_type(SSL *s, WPACKET *pkt) static int ssl3_set_req_cert_type(CERT *c, const unsigned char *p, size_t len) { - OPENSSL_free(c->ctypes); - c->ctypes = NULL; - if (!p || !len) + OPENSSL_free(c->ctype); + c->ctype = NULL; + c->ctype_len = 0; + if (p == NULL || len == 0) return 1; if (len > 0xff) return 0; - c->ctypes = OPENSSL_malloc(len); - if (c->ctypes == NULL) + c->ctype = OPENSSL_memdup(p, len); + if (c->ctype == NULL) return 0; - memcpy(c->ctypes, p, len); - c->ctype_num = len; + c->ctype_len = len; return 1; } diff --git a/ssl/ssl_cert.c b/ssl/ssl_cert.c index a75fb6564a..70aa697564 100644 --- a/ssl/ssl_cert.c +++ b/ssl/ssl_cert.c @@ -164,12 +164,11 @@ CERT *ssl_cert_dup(CERT *cert) /* Shared sigalgs also NULL */ ret->shared_sigalgs = NULL; /* Copy any custom client certificate types */ - if (cert->ctypes) { - ret->ctypes = OPENSSL_malloc(cert->ctype_num); - if (ret->ctypes == NULL) + if (cert->ctype) { + ret->ctype = OPENSSL_memdup(cert->ctype, cert->ctype_len); + if (ret->ctype == NULL) goto err; - memcpy(ret->ctypes, cert->ctypes, cert->ctype_num); - ret->ctype_num = cert->ctype_num; + ret->ctype_len = cert->ctype_len; } ret->cert_flags = cert->cert_flags; @@ -252,7 +251,7 @@ void ssl_cert_free(CERT *c) OPENSSL_free(c->conf_sigalgs); OPENSSL_free(c->client_sigalgs); OPENSSL_free(c->shared_sigalgs); - OPENSSL_free(c->ctypes); + OPENSSL_free(c->ctype); X509_STORE_free(c->verify_store); X509_STORE_free(c->chain_store); custom_exts_free(&c->cli_ext); diff --git a/ssl/ssl_locl.h b/ssl/ssl_locl.h index 26464a584d..ebfd459c82 100644 --- a/ssl/ssl_locl.h +++ b/ssl/ssl_locl.h @@ -1316,8 +1316,9 @@ typedef struct ssl3_state_st { # endif /* used for certificate requests */ int cert_req; - int ctype_num; - char ctype[SSL3_CT_NUMBER]; + /* Certificate types in certificate request message. */ + uint8_t *ctype; + size_t ctype_len; STACK_OF(X509_NAME) *ca_names; size_t key_block_length; unsigned char *key_block; @@ -1606,13 +1607,9 @@ typedef struct cert_st { /* Flags related to certificates */ uint32_t cert_flags; CERT_PKEY pkeys[SSL_PKEY_NUM]; - /* - * Certificate types (received or sent) in certificate request message. - * On receive this is only set if number of certificate types exceeds - * SSL3_CT_NUMBER. - */ - unsigned char *ctypes; - size_t ctype_num; + /* Custom certificate types sent in certificate request message. */ + uint8_t *ctype; + size_t ctype_len; /* * supported signature algorithms. When set on a client this is sent in * the client hello as the supported signature algorithms extension. For diff --git a/ssl/statem/statem_clnt.c b/ssl/statem/statem_clnt.c index bc35a3ea25..6e162fb305 100644 --- a/ssl/statem/statem_clnt.c +++ b/ssl/statem/statem_clnt.c @@ -2196,11 +2196,11 @@ 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 list_len, ctype_num, i, name_len; + unsigned int list_len, i, name_len; X509_NAME *xn = NULL; - const unsigned char *data; const unsigned char *namestart, *namebytes; STACK_OF(X509_NAME) *ca_sk = NULL; + PACKET ctypes; if ((ca_sk = sk_X509_NAME_new(ca_dn_cmp)) == NULL) { SSLerr(SSL_F_TLS_PROCESS_CERTIFICATE_REQUEST, ERR_R_MALLOC_FAILURE); @@ -2208,27 +2208,16 @@ MSG_PROCESS_RETURN tls_process_certificate_request(SSL *s, PACKET *pkt) } /* get the certificate types */ - if (!PACKET_get_1(pkt, &ctype_num) - || !PACKET_get_bytes(pkt, &data, ctype_num)) { + if (!PACKET_get_length_prefixed_1(pkt, &ctypes)) { ssl3_send_alert(s, SSL3_AL_FATAL, SSL_AD_DECODE_ERROR); SSLerr(SSL_F_TLS_PROCESS_CERTIFICATE_REQUEST, SSL_R_LENGTH_MISMATCH); goto err; } - OPENSSL_free(s->cert->ctypes); - s->cert->ctypes = NULL; - if (ctype_num > SSL3_CT_NUMBER) { - /* If we exceed static buffer copy all to cert structure */ - s->cert->ctypes = OPENSSL_malloc(ctype_num); - if (s->cert->ctypes == NULL) { - SSLerr(SSL_F_TLS_PROCESS_CERTIFICATE_REQUEST, ERR_R_MALLOC_FAILURE); + + if (!PACKET_memdup(&ctypes, &s->s3->tmp.ctype, &s->s3->tmp.ctype_len)) { + SSLerr(SSL_F_TLS_PROCESS_CERTIFICATE_REQUEST, ERR_R_INTERNAL_ERROR); goto err; - } - memcpy(s->cert->ctypes, data, ctype_num); - s->cert->ctype_num = ctype_num; - ctype_num = SSL3_CT_NUMBER; } - for (i = 0; i < ctype_num; i++) - s->s3->tmp.ctype[i] = data[i]; if (SSL_USE_SIGALGS(s)) { PACKET sigalgs; @@ -2297,7 +2286,6 @@ MSG_PROCESS_RETURN tls_process_certificate_request(SSL *s, PACKET *pkt) /* we should setup a certificate to return.... */ s->s3->tmp.cert_req = 1; - s->s3->tmp.ctype_num = ctype_num; sk_X509_NAME_pop_free(s->s3->tmp.ca_names, X509_NAME_free); s->s3->tmp.ca_names = ca_sk; ca_sk = NULL; diff --git a/ssl/t1_lib.c b/ssl/t1_lib.c index f13c0ada93..8b31e84b63 100644 --- a/ssl/t1_lib.c +++ b/ssl/t1_lib.c @@ -2009,25 +2009,20 @@ int tls1_check_chain(SSL *s, X509 *x, EVP_PKEY *pk, STACK_OF(X509) *chain, break; } if (check_type) { - const unsigned char *ctypes; - int ctypelen; - if (c->ctypes) { - ctypes = c->ctypes; - ctypelen = (int)c->ctype_num; - } else { - ctypes = (unsigned char *)s->s3->tmp.ctype; - ctypelen = s->s3->tmp.ctype_num; - } - for (i = 0; i < ctypelen; i++) { - if (ctypes[i] == check_type) { + const uint8_t *ctypes = s->s3->tmp.ctype; + size_t j; + + for (j = 0; j < s->s3->tmp.ctype_len; j++, ctypes++) { + if (*ctypes == check_type) { rv |= CERT_PKEY_CERT_TYPE; break; } } if (!(rv & CERT_PKEY_CERT_TYPE) && !check_flags) goto end; - } else + } else { rv |= CERT_PKEY_CERT_TYPE; + } ca_dn = s->s3->tmp.ca_names;