Get rid of CERT_PKEY slots in SESS_CERT. This doesn't even change behavior. Unlike local configuration, the peer can never have multiple certificates anyway. (Even with a renego, the SESS_CERT is created anew.) This does lose the implicit certificate type check, but the certificate type is already checked in ssl3_get_server_certificate and later checked post-facto in ssl3_check_cert_and_algorithm (except that one seems to have some bugs like it accepts ECDSA certificates for RSA cipher suites, to be cleaned up in a follow-up). Either way, we have the certificate mismatch tests for this. BUG=486295 Change-Id: I437bb723bb310ad54ee4150eda67c1cfe43377b3 Reviewed-on: https://boringssl-review.googlesource.com/5044 Reviewed-by: Adam Langley <agl@google.com>
diff --git a/ssl/internal.h b/ssl/internal.h index 7d9a5ad..0f40122 100644 --- a/ssl/internal.h +++ b/ssl/internal.h
@@ -581,15 +581,13 @@ } CERT; typedef struct sess_cert_st { - STACK_OF(X509) *cert_chain; /* as received from peer (not for SSL2) */ + /* cert_chain is the certificate chain sent by the peer. NOTE: for a client, + * this does includes the server's leaf certificate, but, for a server, this + * does NOT include the client's leaf. */ + STACK_OF(X509) *cert_chain; - /* The 'peer_...' members are used only by clients. */ - int peer_cert_type; - - CERT_PKEY *peer_key; /* points to an element of peer_pkeys (never NULL!) */ - CERT_PKEY peer_pkeys[SSL_PKEY_NUM]; - /* Obviously we don't have the private keys of these, - * so maybe we shouldn't even use the CERT_PKEY type here. */ + /* peer_cert, on a client, is the leaf certificate of the peer. */ + X509 *peer_cert; DH *peer_dh_tmp; EC_KEY *peer_ecdh_tmp; @@ -800,7 +798,6 @@ void ssl_cert_free(CERT *c); SESS_CERT *ssl_sess_cert_new(void); void ssl_sess_cert_free(SESS_CERT *sc); -int ssl_set_peer_cert_type(SESS_CERT *c, int type); int ssl_get_new_session(SSL *s, int session); int ssl_get_prev_session(SSL *s, const struct ssl_early_callback_ctx *ctx); STACK_OF(SSL_CIPHER) *ssl_bytes_to_cipher_list(SSL *s, const CBS *cbs);
diff --git a/ssl/s3_clnt.c b/ssl/s3_clnt.c index 159e2d7..d7ed14f 100644 --- a/ssl/s3_clnt.c +++ b/ssl/s3_clnt.c
@@ -1028,10 +1028,8 @@ SSL_R_WRONG_CERTIFICATE_TYPE); goto f_err; } - sc->peer_cert_type = i; - X509_free(sc->peer_pkeys[i].x509); - sc->peer_pkeys[i].x509 = X509_up_ref(x); - sc->peer_key = &(sc->peer_pkeys[i]); + X509_free(sc->peer_cert); + sc->peer_cert = X509_up_ref(x); X509_free(s->session->peer); s->session->peer = X509_up_ref(x); @@ -1195,8 +1193,7 @@ } if (alg_a & SSL_aRSA) { - pkey = X509_get_pubkey( - s->session->sess_cert->peer_pkeys[SSL_PKEY_RSA_ENC].x509); + pkey = X509_get_pubkey(s->session->sess_cert->peer_cert); } /* else anonymous DH, so no certificate or pkey. */ @@ -1258,14 +1255,12 @@ /* The ECC/TLS specification does not mention the use of DSA to sign * ECParameters in the server key exchange message. We do support RSA and * ECDSA. */ - if (alg_a & SSL_aRSA) { - pkey = X509_get_pubkey( - s->session->sess_cert->peer_pkeys[SSL_PKEY_RSA_ENC].x509); - } else if (alg_a & SSL_aECDSA) { - pkey = - X509_get_pubkey(s->session->sess_cert->peer_pkeys[SSL_PKEY_ECC].x509); + if (alg_a & (SSL_aRSA|SSL_aECDSA)) { + pkey = X509_get_pubkey(s->session->sess_cert->peer_cert); + } else { + /* Otherwise this is ECDHE_PSK, so no public key. */ + assert(alg_a == SSL_aPSK); } - /* else anonymous ECDH, so no certificate or pkey. */ EC_KEY_set_public_key(ecdh, srvr_ecpoint); s->session->sess_cert->peer_ecdh_tmp = ecdh; ecdh = NULL; @@ -1702,8 +1697,7 @@ goto err; } - pkey = X509_get_pubkey( - s->session->sess_cert->peer_pkeys[SSL_PKEY_RSA_ENC].x509); + pkey = X509_get_pubkey(s->session->sess_cert->peer_cert); if (pkey == NULL || pkey->type != EVP_PKEY_RSA || pkey->pkey.rsa == NULL) { @@ -2159,9 +2153,7 @@ #define has_bits(i, m) (((i) & (m)) == (m)) int ssl3_check_cert_and_algorithm(SSL *s) { - int i, idx; long alg_k, alg_a; - EVP_PKEY *pkey = NULL; SESS_CERT *sc; DH *dh; @@ -2181,11 +2173,9 @@ dh = s->session->sess_cert->peer_dh_tmp; - /* This is the passed certificate */ - - idx = sc->peer_cert_type; - if (idx == SSL_PKEY_ECC) { - if (ssl_check_srvr_ecc_cert_and_alg(sc->peer_pkeys[idx].x509, s) == 0) { + int cert_type = X509_certificate_type(sc->peer_cert, NULL); + if (cert_type & EVP_PK_EC) { + if (ssl_check_srvr_ecc_cert_and_alg(sc->peer_cert, s) == 0) { /* check failed */ OPENSSL_PUT_ERROR(SSL, ssl3_check_cert_and_algorithm, SSL_R_BAD_ECC_CERT); goto f_err; @@ -2197,25 +2187,21 @@ SSL_R_MISSING_ECDSA_SIGNING_CERT); goto f_err; } - pkey = X509_get_pubkey(sc->peer_pkeys[idx].x509); - i = X509_certificate_type(sc->peer_pkeys[idx].x509, pkey); - EVP_PKEY_free(pkey); /* Check that we have a certificate if we require one */ - if ((alg_a & SSL_aRSA) && !has_bits(i, EVP_PK_RSA | EVP_PKT_SIGN)) { + if ((alg_a & SSL_aRSA) && !has_bits(cert_type, EVP_PK_RSA | EVP_PKT_SIGN)) { OPENSSL_PUT_ERROR(SSL, ssl3_check_cert_and_algorithm, SSL_R_MISSING_RSA_SIGNING_CERT); goto f_err; } - if ((alg_k & SSL_kRSA) && !has_bits(i, EVP_PK_RSA | EVP_PKT_ENC)) { + if ((alg_k & SSL_kRSA) && !has_bits(cert_type, EVP_PK_RSA | EVP_PKT_ENC)) { OPENSSL_PUT_ERROR(SSL, ssl3_check_cert_and_algorithm, SSL_R_MISSING_RSA_ENCRYPTING_CERT); goto f_err; } - if ((alg_k & SSL_kDHE) && - !(has_bits(i, EVP_PK_DH | EVP_PKT_EXCH) || dh != NULL)) { + if ((alg_k & SSL_kDHE) && dh == NULL) { OPENSSL_PUT_ERROR(SSL, ssl3_check_cert_and_algorithm, SSL_R_MISSING_DH_KEY); goto f_err; }
diff --git a/ssl/ssl_cert.c b/ssl/ssl_cert.c index f1fd675..f27685d 100644 --- a/ssl/ssl_cert.c +++ b/ssl/ssl_cert.c
@@ -409,35 +409,23 @@ } memset(ret, 0, sizeof *ret); - ret->peer_key = &(ret->peer_pkeys[SSL_PKEY_RSA_ENC]); return ret; } void ssl_sess_cert_free(SESS_CERT *sc) { - int i; - if (sc == NULL) { return; } sk_X509_pop_free(sc->cert_chain, X509_free); - - for (i = 0; i < SSL_PKEY_NUM; i++) { - X509_free(sc->peer_pkeys[i].x509); - } - + X509_free(sc->peer_cert); DH_free(sc->peer_dh_tmp); EC_KEY_free(sc->peer_ecdh_tmp); OPENSSL_free(sc); } -int ssl_set_peer_cert_type(SESS_CERT *sc, int type) { - sc->peer_cert_type = type; - return 1; -} - int ssl_verify_cert_chain(SSL *s, STACK_OF(X509) *sk) { X509 *x; int i;