From 4492a615674c502f006573185edafeea9c51a25c Mon Sep 17 00:00:00 2001 From: David Benjamin Date: Wed, 2 Aug 2017 17:16:31 -0400 Subject: [PATCH] More scopers. Note the legacy client cert callback case fixes a leak. Change-Id: I2772167bd03d308676d9e00885c751207002b31e Reviewed-on: https://boringssl-review.googlesource.com/18824 Commit-Queue: Steven Valdez Reviewed-by: Steven Valdez CQ-Verified: CQ bot account: commit-bot@chromium.org --- crypto/x509/x509_vfy.c | 9 ++++-- include/openssl/base.h | 3 ++ include/openssl/x509.h | 9 ++++-- include/openssl/x509_vfy.h | 1 + ssl/ssl_privkey.cc | 57 ++++++++++++-------------------------- ssl/ssl_x509.cc | 57 +++++++++++++++++--------------------- 6 files changed, 60 insertions(+), 76 deletions(-) diff --git a/crypto/x509/x509_vfy.c b/crypto/x509/x509_vfy.c index 2413a1c16..b427df635 100644 --- a/crypto/x509/x509_vfy.c +++ b/crypto/x509/x509_vfy.c @@ -2254,10 +2254,15 @@ X509_STORE_CTX *X509_STORE_CTX_new(void) OPENSSL_PUT_ERROR(X509, ERR_R_MALLOC_FAILURE); return NULL; } - OPENSSL_memset(ctx, 0, sizeof(X509_STORE_CTX)); + X509_STORE_CTX_zero(ctx); return ctx; } +void X509_STORE_CTX_zero(X509_STORE_CTX *ctx) +{ + OPENSSL_memset(ctx, 0, sizeof(X509_STORE_CTX)); +} + void X509_STORE_CTX_free(X509_STORE_CTX *ctx) { if (ctx == NULL) { @@ -2272,7 +2277,7 @@ int X509_STORE_CTX_init(X509_STORE_CTX *ctx, X509_STORE *store, X509 *x509, { int ret = 1; - OPENSSL_memset(ctx, 0, sizeof(X509_STORE_CTX)); + X509_STORE_CTX_zero(ctx); ctx->ctx = store; ctx->cert = x509; ctx->untrusted = chain; diff --git a/include/openssl/base.h b/include/openssl/base.h index e575069b6..6b43c7674 100644 --- a/include/openssl/base.h +++ b/include/openssl/base.h @@ -413,6 +413,9 @@ class StackAllocated { T *get() { return &ctx_; } const T *get() const { return &ctx_; } + T *operator->() { return &ctx_; } + const T *operator->() const { return &ctx_; } + void Reset() { cleanup(&ctx_); init(&ctx_); diff --git a/include/openssl/x509.h b/include/openssl/x509.h index 40b2332af..138c060e0 100644 --- a/include/openssl/x509.h +++ b/include/openssl/x509.h @@ -1094,7 +1094,9 @@ DECLARE_ASN1_FUNCTIONS(RSA_PSS_PARAMS) #ifdef __cplusplus } +#endif +#if !defined(BORINGSSL_NO_CXX) extern "C++" { namespace bssl { @@ -1118,11 +1120,14 @@ BORINGSSL_MAKE_DELETER(X509_STORE, X509_STORE_free) BORINGSSL_MAKE_DELETER(X509_STORE_CTX, X509_STORE_CTX_free) BORINGSSL_MAKE_DELETER(X509_VERIFY_PARAM, X509_VERIFY_PARAM_free) +using ScopedX509_STORE_CTX = + internal::StackAllocated; + } // namespace bssl } /* extern C++ */ - -#endif +#endif /* !BORINGSSL_NO_CXX */ #define X509_R_AKID_MISMATCH 100 #define X509_R_BAD_PKCS7_VERSION 101 diff --git a/include/openssl/x509_vfy.h b/include/openssl/x509_vfy.h index ac739eaab..4abd9cdae 100644 --- a/include/openssl/x509_vfy.h +++ b/include/openssl/x509_vfy.h @@ -449,6 +449,7 @@ OPENSSL_EXPORT X509_STORE_CTX *X509_STORE_CTX_new(void); OPENSSL_EXPORT int X509_STORE_CTX_get1_issuer(X509 **issuer, X509_STORE_CTX *ctx, X509 *x); +OPENSSL_EXPORT void X509_STORE_CTX_zero(X509_STORE_CTX *ctx); OPENSSL_EXPORT void X509_STORE_CTX_free(X509_STORE_CTX *ctx); OPENSSL_EXPORT int X509_STORE_CTX_init(X509_STORE_CTX *ctx, X509_STORE *store, X509 *x509, STACK_OF(X509) *chain); diff --git a/ssl/ssl_privkey.cc b/ssl/ssl_privkey.cc index 3e3fa94ba..ecdf48f47 100644 --- a/ssl/ssl_privkey.cc +++ b/ssl/ssl_privkey.cc @@ -320,27 +320,19 @@ int ssl_private_key_supports_signature_algorithm(SSL_HANDSHAKE *hs, using namespace bssl; int SSL_use_RSAPrivateKey(SSL *ssl, RSA *rsa) { - EVP_PKEY *pkey; - int ret; - if (rsa == NULL) { OPENSSL_PUT_ERROR(SSL, ERR_R_PASSED_NULL_PARAMETER); return 0; } - pkey = EVP_PKEY_new(); - if (pkey == NULL) { + UniquePtr pkey(EVP_PKEY_new()); + if (!pkey || + !EVP_PKEY_set1_RSA(pkey.get(), rsa)) { OPENSSL_PUT_ERROR(SSL, ERR_R_EVP_LIB); return 0; } - RSA_up_ref(rsa); - EVP_PKEY_assign_RSA(pkey, rsa); - - ret = ssl_set_pkey(ssl->cert, pkey); - EVP_PKEY_free(pkey); - - return ret; + return ssl_set_pkey(ssl->cert, pkey.get()); } int SSL_use_RSAPrivateKey_ASN1(SSL *ssl, const uint8_t *der, size_t der_len) { @@ -370,52 +362,40 @@ int SSL_use_PrivateKey_ASN1(int type, SSL *ssl, const uint8_t *der, } const uint8_t *p = der; - EVP_PKEY *pkey = d2i_PrivateKey(type, NULL, &p, (long)der_len); - if (pkey == NULL || p != der + der_len) { + UniquePtr pkey(d2i_PrivateKey(type, NULL, &p, (long)der_len)); + if (!pkey || p != der + der_len) { OPENSSL_PUT_ERROR(SSL, ERR_R_ASN1_LIB); - EVP_PKEY_free(pkey); return 0; } - int ret = SSL_use_PrivateKey(ssl, pkey); - EVP_PKEY_free(pkey); - return ret; + return SSL_use_PrivateKey(ssl, pkey.get()); } int SSL_CTX_use_RSAPrivateKey(SSL_CTX *ctx, RSA *rsa) { - int ret; - EVP_PKEY *pkey; - if (rsa == NULL) { OPENSSL_PUT_ERROR(SSL, ERR_R_PASSED_NULL_PARAMETER); return 0; } - pkey = EVP_PKEY_new(); - if (pkey == NULL) { + UniquePtr pkey(EVP_PKEY_new()); + if (!pkey || + !EVP_PKEY_set1_RSA(pkey.get(), rsa)) { OPENSSL_PUT_ERROR(SSL, ERR_R_EVP_LIB); return 0; } - RSA_up_ref(rsa); - EVP_PKEY_assign_RSA(pkey, rsa); - - ret = ssl_set_pkey(ctx->cert, pkey); - EVP_PKEY_free(pkey); - return ret; + return ssl_set_pkey(ctx->cert, pkey.get()); } int SSL_CTX_use_RSAPrivateKey_ASN1(SSL_CTX *ctx, const uint8_t *der, size_t der_len) { - RSA *rsa = RSA_private_key_from_bytes(der, der_len); - if (rsa == NULL) { + UniquePtr rsa(RSA_private_key_from_bytes(der, der_len)); + if (!rsa) { OPENSSL_PUT_ERROR(SSL, ERR_R_ASN1_LIB); return 0; } - int ret = SSL_CTX_use_RSAPrivateKey(ctx, rsa); - RSA_free(rsa); - return ret; + return SSL_CTX_use_RSAPrivateKey(ctx, rsa.get()); } int SSL_CTX_use_PrivateKey(SSL_CTX *ctx, EVP_PKEY *pkey) { @@ -435,16 +415,13 @@ int SSL_CTX_use_PrivateKey_ASN1(int type, SSL_CTX *ctx, const uint8_t *der, } const uint8_t *p = der; - EVP_PKEY *pkey = d2i_PrivateKey(type, NULL, &p, (long)der_len); - if (pkey == NULL || p != der + der_len) { + UniquePtr pkey(d2i_PrivateKey(type, NULL, &p, (long)der_len)); + if (!pkey || p != der + der_len) { OPENSSL_PUT_ERROR(SSL, ERR_R_ASN1_LIB); - EVP_PKEY_free(pkey); return 0; } - int ret = SSL_CTX_use_PrivateKey(ctx, pkey); - EVP_PKEY_free(pkey); - return ret; + return SSL_CTX_use_PrivateKey(ctx, pkey.get()); } void SSL_set_private_key_method(SSL *ssl, diff --git a/ssl/ssl_x509.cc b/ssl/ssl_x509.cc index 22f4fb486..98a5b8c38 100644 --- a/ssl/ssl_x509.cc +++ b/ssl/ssl_x509.cc @@ -429,50 +429,47 @@ static int ssl_crypto_x509_session_verify_cert_chain(SSL_SESSION *session, } X509 *leaf = sk_X509_value(cert_chain, 0); - int ret = 0; - X509_STORE_CTX ctx; - if (!X509_STORE_CTX_init(&ctx, verify_store, leaf, cert_chain)) { + ScopedX509_STORE_CTX ctx; + if (!X509_STORE_CTX_init(ctx.get(), verify_store, leaf, cert_chain)) { OPENSSL_PUT_ERROR(SSL, ERR_R_X509_LIB); return 0; } - if (!X509_STORE_CTX_set_ex_data(&ctx, SSL_get_ex_data_X509_STORE_CTX_idx(), - ssl)) { - goto err; + if (!X509_STORE_CTX_set_ex_data(ctx.get(), + SSL_get_ex_data_X509_STORE_CTX_idx(), ssl)) { + return 0; } /* We need to inherit the verify parameters. These can be determined by the * context: if its a server it will verify SSL client certificates or vice * versa. */ - X509_STORE_CTX_set_default(&ctx, ssl->server ? "ssl_client" : "ssl_server"); + X509_STORE_CTX_set_default(ctx.get(), + ssl->server ? "ssl_client" : "ssl_server"); /* Anything non-default in "param" should overwrite anything in the ctx. */ - X509_VERIFY_PARAM_set1(X509_STORE_CTX_get0_param(&ctx), ssl->param); + X509_VERIFY_PARAM_set1(X509_STORE_CTX_get0_param(ctx.get()), ssl->param); if (ssl->verify_callback) { - X509_STORE_CTX_set_verify_cb(&ctx, ssl->verify_callback); + X509_STORE_CTX_set_verify_cb(ctx.get(), ssl->verify_callback); } int verify_ret; if (ssl->ctx->app_verify_callback != NULL) { - verify_ret = ssl->ctx->app_verify_callback(&ctx, ssl->ctx->app_verify_arg); + verify_ret = + ssl->ctx->app_verify_callback(ctx.get(), ssl->ctx->app_verify_arg); } else { - verify_ret = X509_verify_cert(&ctx); + verify_ret = X509_verify_cert(ctx.get()); } - session->verify_result = ctx.error; + session->verify_result = ctx->error; /* If |SSL_VERIFY_NONE|, the error is non-fatal, but we keep the result. */ if (verify_ret <= 0 && ssl->verify_mode != SSL_VERIFY_NONE) { - *out_alert = ssl_verify_alarm_type(ctx.error); - goto err; + *out_alert = ssl_verify_alarm_type(ctx->error); + return 0; } ERR_clear_error(); - ret = 1; - -err: - X509_STORE_CTX_cleanup(&ctx); - return ret; + return 1; } static void ssl_crypto_x509_hs_flush_cached_ca_names(SSL_HANDSHAKE *hs) { @@ -509,31 +506,27 @@ static int ssl_crypto_x509_ssl_auto_chain_if_needed(SSL *ssl) { return 1; } - X509 *leaf = - X509_parse_from_buffer(sk_CRYPTO_BUFFER_value(ssl->cert->chain, 0)); + UniquePtr leaf( + X509_parse_from_buffer(sk_CRYPTO_BUFFER_value(ssl->cert->chain, 0))); if (!leaf) { OPENSSL_PUT_ERROR(SSL, ERR_R_X509_LIB); return 0; } - X509_STORE_CTX ctx; - if (!X509_STORE_CTX_init(&ctx, ssl->ctx->cert_store, leaf, NULL)) { - X509_free(leaf); + ScopedX509_STORE_CTX ctx; + if (!X509_STORE_CTX_init(ctx.get(), ssl->ctx->cert_store, leaf.get(), NULL)) { OPENSSL_PUT_ERROR(SSL, ERR_R_X509_LIB); return 0; } /* Attempt to build a chain, ignoring the result. */ - X509_verify_cert(&ctx); - X509_free(leaf); + X509_verify_cert(ctx.get()); ERR_clear_error(); /* Remove the leaf from the generated chain. */ - X509_free(sk_X509_shift(ctx.chain)); + X509_free(sk_X509_shift(ctx->chain)); - const int ok = ssl_cert_set_chain(ssl->cert, ctx.chain); - X509_STORE_CTX_cleanup(&ctx); - if (!ok) { + if (!ssl_cert_set_chain(ssl->cert, ctx->chain)) { return 0; } @@ -1248,6 +1241,8 @@ static int do_client_cert_cb(SSL *ssl, void *arg) { if (ret < 0) { return -1; } + UniquePtr free_x509(x509); + UniquePtr free_pkey(pkey); if (ret != 0) { if (!SSL_use_certificate(ssl, x509) || @@ -1256,8 +1251,6 @@ static int do_client_cert_cb(SSL *ssl, void *arg) { } } - X509_free(x509); - EVP_PKEY_free(pkey); return 1; }