From 676d1e780e96b201113958e8a6f2e787438f200b Mon Sep 17 00:00:00 2001 From: David Benjamin Date: Tue, 8 Jul 2014 14:34:10 -0400 Subject: [PATCH] Separate client and server certificate_types. This is the first of reorganizing state between connection state and handshake state. The existing set are retained in cert_st for the server; they are server configuration. The client gets a copy in s->s3->tmp alongside other handshake state. With other handshake state moved there, hopefully we can reset that state in one go and possibly not even maintain it when there is no handshake in progress. Rather than currently where we sometimes confused connection state and handshake state and have to reset as appropriate on renegotiate. While I'm here, document the fields and name them something more useful than 'ctypes'. Change-Id: Ib927579f0004fc5c6854fce2127625df669b2b6d Reviewed-on: https://boringssl-review.googlesource.com/1113 Reviewed-by: Adam Langley --- ssl/s3_clnt.c | 4 +++- ssl/s3_lib.c | 31 +++++++++++++++++++------------ ssl/ssl3.h | 23 ++++++++++++++++++++++- ssl/ssl_cert.c | 16 +++++++++------- ssl/ssl_locl.h | 8 ++++---- ssl/t1_lib.c | 13 +++---------- 6 files changed, 60 insertions(+), 35 deletions(-) diff --git a/ssl/s3_clnt.c b/ssl/s3_clnt.c index 33626358a..6f6cf3d97 100644 --- a/ssl/s3_clnt.c +++ b/ssl/s3_clnt.c @@ -1871,7 +1871,9 @@ int ssl3_get_certificate_request(SSL *s) OPENSSL_PUT_ERROR(SSL, ssl3_get_certificate_request, SSL_R_DECODE_ERROR); goto err; } - if (!CBS_stow(&certificate_types, &s->cert->ctypes, &s->cert->ctype_num)) + if (!CBS_stow(&certificate_types, + &s->s3->tmp.certificate_types, + &s->s3->tmp.num_certificate_types)) { ssl3_send_alert(s, SSL3_AL_FATAL, SSL_AD_INTERNAL_ERROR); goto err; diff --git a/ssl/s3_lib.c b/ssl/s3_lib.c index 2518bd38a..978f55211 100644 --- a/ssl/s3_lib.c +++ b/ssl/s3_lib.c @@ -148,6 +148,7 @@ #include +#include #include #include #include @@ -2711,6 +2712,8 @@ void ssl3_free(SSL *s) if (s->s3->tmp.ca_names != NULL) sk_X509_NAME_pop_free(s->s3->tmp.ca_names,X509_NAME_free); + if (s->s3->tmp.certificate_types != NULL) + OPENSSL_free(s->s3->tmp.certificate_types); if (s->s3->handshake_buffer) { BIO_free(s->s3->handshake_buffer); } @@ -2732,6 +2735,9 @@ void ssl3_clear(SSL *s) ssl3_cleanup_key_block(s); if (s->s3->tmp.ca_names != NULL) sk_X509_NAME_pop_free(s->s3->tmp.ca_names,X509_NAME_free); + if (s->s3->tmp.certificate_types != NULL) + OPENSSL_free(s->s3->tmp.certificate_types); + s->s3->tmp.num_certificate_types = 0; #ifndef OPENSSL_NO_DH if (s->s3->tmp.dh != NULL) @@ -3082,8 +3088,8 @@ long ssl3_ctrl(SSL *s, int cmd, long larg, void *parg) if (s->server || !s->s3->tmp.cert_req) return 0; if (pctype) - *pctype = s->cert->ctypes; - return (int)s->cert->ctype_num; + *pctype = s->s3->tmp.certificate_types; + return (int)s->s3->tmp.num_certificate_types; } case SSL_CTRL_SET_CLIENT_CERT_TYPES: @@ -3772,10 +3778,11 @@ int ssl3_get_req_cert_type(SSL *s, unsigned char *p) unsigned long alg_k; /* If we have custom certificate types set, use them */ - if (s->cert->ctypes) + if (s->cert->client_certificate_types) { - memcpy(p, s->cert->ctypes, s->cert->ctype_num); - return (int)s->cert->ctype_num; + memcpy(p, s->cert->client_certificate_types, + s->cert->num_client_certificate_types); + return (int)s->cert->num_client_certificate_types; } /* get configured sigalgs */ siglen = tls12_get_psigalgs(s, &sig); @@ -3855,20 +3862,20 @@ int ssl3_get_req_cert_type(SSL *s, unsigned char *p) static int ssl3_set_req_cert_type(CERT *c, const unsigned char *p, size_t len) { - if (c->ctypes) + if (c->client_certificate_types) { - OPENSSL_free(c->ctypes); - c->ctypes = NULL; + OPENSSL_free(c->client_certificate_types); + c->client_certificate_types = NULL; } + c->num_client_certificate_types = 0; if (!p || !len) return 1; if (len > 0xff) return 0; - c->ctypes = OPENSSL_malloc(len); - if (!c->ctypes) + c->client_certificate_types = BUF_memdup(p, len); + if (!c->client_certificate_types) return 0; - memcpy(c->ctypes, p, len); - c->ctype_num = len; + c->num_client_certificate_types = len; return 1; } diff --git a/ssl/ssl3.h b/ssl/ssl3.h index 72db84056..fc2f2b0ae 100644 --- a/ssl/ssl3.h +++ b/ssl/ssl3.h @@ -476,6 +476,12 @@ typedef struct ssl3_state_st int in_read_app_data; + /* State pertaining to the pending handshake. + * + * TODO(davidben): State is current spread all over the place. Move + * pending handshake state here so it can be managed separately from + * established connection state in case of renegotiations. + */ struct { /* actually only needs to be 16+20 */ unsigned char cert_verify_md[EVP_MAX_MD_SIZE*2]; @@ -504,10 +510,22 @@ typedef struct ssl3_state_st int reuse_message; - /* used for certificate requests */ + /* Client-only: cert_req determines if a client certificate is + * to be sent. This is 0 if no client Certificate message is to + * be sent, 1 if there is a client certificate, and 2 to send an + * empty client Certificate message. */ int cert_req; + + /* Client-only: ca_names contains the list of CAs received in a + * CertificateRequest message. */ STACK_OF(X509_NAME) *ca_names; + /* Client-only: certificate_types contains the set of + * certificate types received in a CertificateRequest + * message. */ + unsigned char *certificate_types; + size_t num_certificate_types; + int use_rsa_tmp; int key_block_length; @@ -518,6 +536,9 @@ typedef struct ssl3_state_st const EVP_MD *new_hash; int new_mac_pkey_type; int new_mac_secret_size; + + /* Server-only: cert_request is true if a client certificate was + * requested. */ int cert_request; } tmp; diff --git a/ssl/ssl_cert.c b/ssl/ssl_cert.c index eab55962a..e4523c359 100644 --- a/ssl/ssl_cert.c +++ b/ssl/ssl_cert.c @@ -116,6 +116,7 @@ #include #include +#include #include #include #include @@ -356,13 +357,14 @@ CERT *ssl_cert_dup(CERT *cert) /* Shared sigalgs also NULL */ ret->shared_sigalgs = NULL; /* Copy any custom client certificate types */ - if (cert->ctypes) + if (cert->client_certificate_types) { - ret->ctypes = OPENSSL_malloc(cert->ctype_num); - if (!ret->ctypes) + ret->client_certificate_types = BUF_memdup( + cert->client_certificate_types, + cert->num_client_certificate_types); + if (!ret->client_certificate_types) goto err; - memcpy(ret->ctypes, cert->ctypes, cert->ctype_num); - ret->ctype_num = cert->ctype_num; + ret->num_client_certificate_types = cert->num_client_certificate_types; } ret->cert_flags = cert->cert_flags; @@ -457,8 +459,8 @@ void ssl_cert_free(CERT *c) OPENSSL_free(c->client_sigalgs); if (c->shared_sigalgs) OPENSSL_free(c->shared_sigalgs); - if (c->ctypes) - OPENSSL_free(c->ctypes); + if (c->client_certificate_types) + OPENSSL_free(c->client_certificate_types); if (c->verify_store) X509_STORE_free(c->verify_store); if (c->chain_store) diff --git a/ssl/ssl_locl.h b/ssl/ssl_locl.h index 95ba123b6..d2a8dc582 100644 --- a/ssl/ssl_locl.h +++ b/ssl/ssl_locl.h @@ -564,11 +564,11 @@ typedef struct cert_st unsigned int cert_flags; CERT_PKEY pkeys[SSL_PKEY_NUM]; - /* Certificate types (received or sent) in certificate request - * message. + /* Server-only: client_certificate_types is list of certificate types to + * include in the CertificateRequest message. */ - unsigned char *ctypes; - size_t ctype_num; + unsigned char *client_certificate_types; + size_t num_client_certificate_types; /* signature algorithms peer reports: e.g. supported signature * algorithms extension for server or as part of a certificate diff --git a/ssl/t1_lib.c b/ssl/t1_lib.c index 2fa4101d6..954dfc945 100644 --- a/ssl/t1_lib.c +++ b/ssl/t1_lib.c @@ -3805,7 +3805,7 @@ int tls1_check_chain(SSL *s, X509 *x, EVP_PKEY *pk, STACK_OF(X509) *chain, if (!s->server && strict_mode) { STACK_OF(X509_NAME) *ca_dn; - int check_type = 0; + uint8_t check_type = 0; switch (pk->type) { case EVP_PKEY_RSA: @@ -3829,17 +3829,10 @@ int tls1_check_chain(SSL *s, X509 *x, EVP_PKEY *pk, STACK_OF(X509) *chain, } if (check_type) { - const unsigned char *ctypes; - int ctypelen; - ctypes = c->ctypes; - ctypelen = (int)c->ctype_num; - for (i = 0; i < ctypelen; i++) + if (s->s3->tmp.certificate_types && + memchr(s->s3->tmp.certificate_types, check_type, s->s3->tmp.num_certificate_types)) { - if (ctypes[i] == check_type) - { rv |= CERT_PKEY_CERT_TYPE; - break; - } } if (!(rv & CERT_PKEY_CERT_TYPE) && !check_flags) goto end;