From db98becc488393f735790ada8b1214cb4b8c58a5 Mon Sep 17 00:00:00 2001 From: David Benjamin Date: Mon, 6 Feb 2023 14:24:28 -0500 Subject: [PATCH 01/11] Const-correct the various EVP_PKEY PEM writers Change-Id: I6fa17e204cb2003a6803e01604c0187420b4e39b Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/56945 Auto-Submit: David Benjamin Reviewed-by: Bob Beck Commit-Queue: Bob Beck --- crypto/pem/pem_pk8.c | 38 ++++++++++++++++++++------------------ include/openssl/pem.h | 22 +++++++++++----------- 2 files changed, 31 insertions(+), 29 deletions(-) diff --git a/crypto/pem/pem_pk8.c b/crypto/pem/pem_pk8.c index 85196fa9b..610f36ca7 100644 --- a/crypto/pem/pem_pk8.c +++ b/crypto/pem/pem_pk8.c @@ -64,10 +64,10 @@ #include #include -static int do_pk8pkey(BIO *bp, EVP_PKEY *x, int isder, int nid, +static int do_pk8pkey(BIO *bp, const EVP_PKEY *x, int isder, int nid, const EVP_CIPHER *enc, char *kstr, int klen, pem_password_cb *cb, void *u); -static int do_pk8pkey_fp(FILE *bp, EVP_PKEY *x, int isder, int nid, +static int do_pk8pkey_fp(FILE *bp, const EVP_PKEY *x, int isder, int nid, const EVP_CIPHER *enc, char *kstr, int klen, pem_password_cb *cb, void *u); @@ -76,29 +76,30 @@ static int do_pk8pkey_fp(FILE *bp, EVP_PKEY *x, int isder, int nid, // is NULL then it uses the unencrypted private key form. The 'nid' versions // uses PKCS#5 v1.5 PBE algorithms whereas the others use PKCS#5 v2.0. -int PEM_write_bio_PKCS8PrivateKey_nid(BIO *bp, EVP_PKEY *x, int nid, char *kstr, - int klen, pem_password_cb *cb, void *u) { +int PEM_write_bio_PKCS8PrivateKey_nid(BIO *bp, const EVP_PKEY *x, int nid, + char *kstr, int klen, pem_password_cb *cb, + void *u) { return do_pk8pkey(bp, x, 0, nid, NULL, kstr, klen, cb, u); } -int PEM_write_bio_PKCS8PrivateKey(BIO *bp, EVP_PKEY *x, const EVP_CIPHER *enc, - char *kstr, int klen, pem_password_cb *cb, - void *u) { +int PEM_write_bio_PKCS8PrivateKey(BIO *bp, const EVP_PKEY *x, + const EVP_CIPHER *enc, char *kstr, int klen, + pem_password_cb *cb, void *u) { return do_pk8pkey(bp, x, 0, -1, enc, kstr, klen, cb, u); } -int i2d_PKCS8PrivateKey_bio(BIO *bp, EVP_PKEY *x, const EVP_CIPHER *enc, +int i2d_PKCS8PrivateKey_bio(BIO *bp, const EVP_PKEY *x, const EVP_CIPHER *enc, char *kstr, int klen, pem_password_cb *cb, void *u) { return do_pk8pkey(bp, x, 1, -1, enc, kstr, klen, cb, u); } -int i2d_PKCS8PrivateKey_nid_bio(BIO *bp, EVP_PKEY *x, int nid, char *kstr, +int i2d_PKCS8PrivateKey_nid_bio(BIO *bp, const EVP_PKEY *x, int nid, char *kstr, int klen, pem_password_cb *cb, void *u) { return do_pk8pkey(bp, x, 1, nid, NULL, kstr, klen, cb, u); } -static int do_pk8pkey(BIO *bp, EVP_PKEY *x, int isder, int nid, +static int do_pk8pkey(BIO *bp, const EVP_PKEY *x, int isder, int nid, const EVP_CIPHER *enc, char *kstr, int klen, pem_password_cb *cb, void *u) { X509_SIG *p8; @@ -190,28 +191,29 @@ EVP_PKEY *d2i_PKCS8PrivateKey_bio(BIO *bp, EVP_PKEY **x, pem_password_cb *cb, } -int i2d_PKCS8PrivateKey_fp(FILE *fp, EVP_PKEY *x, const EVP_CIPHER *enc, +int i2d_PKCS8PrivateKey_fp(FILE *fp, const EVP_PKEY *x, const EVP_CIPHER *enc, char *kstr, int klen, pem_password_cb *cb, void *u) { return do_pk8pkey_fp(fp, x, 1, -1, enc, kstr, klen, cb, u); } -int i2d_PKCS8PrivateKey_nid_fp(FILE *fp, EVP_PKEY *x, int nid, char *kstr, +int i2d_PKCS8PrivateKey_nid_fp(FILE *fp, const EVP_PKEY *x, int nid, char *kstr, int klen, pem_password_cb *cb, void *u) { return do_pk8pkey_fp(fp, x, 1, nid, NULL, kstr, klen, cb, u); } -int PEM_write_PKCS8PrivateKey_nid(FILE *fp, EVP_PKEY *x, int nid, char *kstr, - int klen, pem_password_cb *cb, void *u) { +int PEM_write_PKCS8PrivateKey_nid(FILE *fp, const EVP_PKEY *x, int nid, + char *kstr, int klen, pem_password_cb *cb, + void *u) { return do_pk8pkey_fp(fp, x, 0, nid, NULL, kstr, klen, cb, u); } -int PEM_write_PKCS8PrivateKey(FILE *fp, EVP_PKEY *x, const EVP_CIPHER *enc, - char *kstr, int klen, pem_password_cb *cb, - void *u) { +int PEM_write_PKCS8PrivateKey(FILE *fp, const EVP_PKEY *x, + const EVP_CIPHER *enc, char *kstr, int klen, + pem_password_cb *cb, void *u) { return do_pk8pkey_fp(fp, x, 0, -1, enc, kstr, klen, cb, u); } -static int do_pk8pkey_fp(FILE *fp, EVP_PKEY *x, int isder, int nid, +static int do_pk8pkey_fp(FILE *fp, const EVP_PKEY *x, int isder, int nid, const EVP_CIPHER *enc, char *kstr, int klen, pem_password_cb *cb, void *u) { BIO *bp; diff --git a/include/openssl/pem.h b/include/openssl/pem.h index 56075ae8a..9319ac80b 100644 --- a/include/openssl/pem.h +++ b/include/openssl/pem.h @@ -417,40 +417,40 @@ DECLARE_PEM_rw_cb(PrivateKey, EVP_PKEY) DECLARE_PEM_rw(PUBKEY, EVP_PKEY) -OPENSSL_EXPORT int PEM_write_bio_PKCS8PrivateKey_nid(BIO *bp, EVP_PKEY *x, +OPENSSL_EXPORT int PEM_write_bio_PKCS8PrivateKey_nid(BIO *bp, const EVP_PKEY *x, int nid, char *kstr, int klen, pem_password_cb *cb, void *u); -OPENSSL_EXPORT int PEM_write_bio_PKCS8PrivateKey(BIO *, EVP_PKEY *, +OPENSSL_EXPORT int PEM_write_bio_PKCS8PrivateKey(BIO *, const EVP_PKEY *, const EVP_CIPHER *, char *, int, pem_password_cb *, void *); -OPENSSL_EXPORT int i2d_PKCS8PrivateKey_bio(BIO *bp, EVP_PKEY *x, +OPENSSL_EXPORT int i2d_PKCS8PrivateKey_bio(BIO *bp, const EVP_PKEY *x, const EVP_CIPHER *enc, char *kstr, int klen, pem_password_cb *cb, void *u); -OPENSSL_EXPORT int i2d_PKCS8PrivateKey_nid_bio(BIO *bp, EVP_PKEY *x, int nid, - char *kstr, int klen, +OPENSSL_EXPORT int i2d_PKCS8PrivateKey_nid_bio(BIO *bp, const EVP_PKEY *x, + int nid, char *kstr, int klen, pem_password_cb *cb, void *u); OPENSSL_EXPORT EVP_PKEY *d2i_PKCS8PrivateKey_bio(BIO *bp, EVP_PKEY **x, pem_password_cb *cb, void *u); -OPENSSL_EXPORT int i2d_PKCS8PrivateKey_fp(FILE *fp, EVP_PKEY *x, +OPENSSL_EXPORT int i2d_PKCS8PrivateKey_fp(FILE *fp, const EVP_PKEY *x, const EVP_CIPHER *enc, char *kstr, int klen, pem_password_cb *cb, void *u); -OPENSSL_EXPORT int i2d_PKCS8PrivateKey_nid_fp(FILE *fp, EVP_PKEY *x, int nid, - char *kstr, int klen, +OPENSSL_EXPORT int i2d_PKCS8PrivateKey_nid_fp(FILE *fp, const EVP_PKEY *x, + int nid, char *kstr, int klen, pem_password_cb *cb, void *u); -OPENSSL_EXPORT int PEM_write_PKCS8PrivateKey_nid(FILE *fp, EVP_PKEY *x, int nid, - char *kstr, int klen, +OPENSSL_EXPORT int PEM_write_PKCS8PrivateKey_nid(FILE *fp, const EVP_PKEY *x, + int nid, char *kstr, int klen, pem_password_cb *cb, void *u); OPENSSL_EXPORT EVP_PKEY *d2i_PKCS8PrivateKey_fp(FILE *fp, EVP_PKEY **x, pem_password_cb *cb, void *u); -OPENSSL_EXPORT int PEM_write_PKCS8PrivateKey(FILE *fp, EVP_PKEY *x, +OPENSSL_EXPORT int PEM_write_PKCS8PrivateKey(FILE *fp, const EVP_PKEY *x, const EVP_CIPHER *enc, char *kstr, int klen, pem_password_cb *cd, void *u); From 582904fdde86be25dfc5ee1a4f5385444c214678 Mon Sep 17 00:00:00 2001 From: David Benjamin Date: Sat, 4 Feb 2023 18:30:36 -0500 Subject: [PATCH 02/11] Move malloc failure testing into OPENSSL_malloc Rather than trying to override the actual malloc symbol, just intercept OPENSSL_malloc and gate it on a build flag. (When we first wrote these, OPENSSL_malloc was just an alias for malloc.) This has several benefits: - This is cross platform. We don't interfere with sanitizers or the libc, or have to mess with global symbols. - This removes the reason bssl_shim and handshaker linked test_support_lib, so we can fix the tes_support_lib / gtest dependency. - If we ever reduce the scope of fallible mallocs, we'll want to constrain the tests to only the ones that are fallible. An interception strategy like this can do it. Hopefully that will also take less time to run in the future. Also fix the ssl malloc failure tests, as they haven't been working for a while. (Malloc failure tests still take far too long to run to the end though. My immediate motivation is less malloc failure and more to tidy up the build.) Bug: 563 Change-Id: I32165b8ecbebfdcfde26964e06a404762edd28e3 Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/56925 Commit-Queue: Bob Beck Reviewed-by: Bob Beck Auto-Submit: David Benjamin --- CMakeLists.txt | 4 ++ crypto/mem.c | 72 +++++++++++++++++++ crypto/test/CMakeLists.txt | 1 - crypto/test/malloc.cc | 143 ------------------------------------- ssl/test/CMakeLists.txt | 4 +- ssl/test/test_config.cc | 30 ++++---- ssl/test/test_state.cc | 23 +++--- 7 files changed, 108 insertions(+), 169 deletions(-) delete mode 100644 crypto/test/malloc.cc diff --git a/CMakeLists.txt b/CMakeLists.txt index 536d83e6c..e0808d703 100644 --- a/CMakeLists.txt +++ b/CMakeLists.txt @@ -364,6 +364,10 @@ if(CONSTANT_TIME_VALIDATION) add_definitions(-DNDEBUG) endif() +if(MALLOC_FAILURE_TESTING) + add_definitions(-DBORINGSSL_MALLOC_FAILURE_TESTING) +endif() + function(go_executable dest package) set(godeps "${CMAKE_SOURCE_DIR}/util/godeps.go") if(NOT CMAKE_GENERATOR STREQUAL "Ninja") diff --git a/crypto/mem.c b/crypto/mem.c index 6ee5b0b74..abba4a448 100644 --- a/crypto/mem.c +++ b/crypto/mem.c @@ -58,6 +58,7 @@ #include #include +#include #include #include @@ -68,6 +69,12 @@ OPENSSL_MSVC_PRAGMA(warning(push, 3)) OPENSSL_MSVC_PRAGMA(warning(pop)) #endif +#if defined(BORINGSSL_MALLOC_FAILURE_TESTING) +#include +#include +#include +#endif + #include "internal.h" @@ -134,7 +141,68 @@ static const uint8_t kBoringSSLBinaryTag[18] = { 3, 0, }; +#if defined(BORINGSSL_MALLOC_FAILURE_TESTING) +static struct CRYPTO_STATIC_MUTEX malloc_failure_lock = + CRYPTO_STATIC_MUTEX_INIT; +static uint64_t current_malloc_count = 0; +static uint64_t malloc_number_to_fail = 0; +static int malloc_failure_enabled = 0, break_on_malloc_fail = 0; + +static void malloc_exit_handler(void) { + CRYPTO_STATIC_MUTEX_lock_read(&malloc_failure_lock); + if (malloc_failure_enabled && current_malloc_count > malloc_number_to_fail) { + _exit(88); + } + CRYPTO_STATIC_MUTEX_unlock_read(&malloc_failure_lock); +} + +static void init_malloc_failure(void) { + const char *env = getenv("MALLOC_NUMBER_TO_FAIL"); + if (env != NULL && env[0] != 0) { + char *endptr; + malloc_number_to_fail = strtoull(env, &endptr, 10); + if (*endptr == 0) { + malloc_failure_enabled = 1; + atexit(malloc_exit_handler); + } + } + break_on_malloc_fail = getenv("MALLOC_BREAK_ON_FAIL") != NULL; +} + +// should_fail_allocation returns one if the current allocation should fail and +// zero otherwise. +static int should_fail_allocation() { + static CRYPTO_once_t once = CRYPTO_ONCE_INIT; + CRYPTO_once(&once, init_malloc_failure); + if (!malloc_failure_enabled) { + return 0; + } + + // We lock just so multi-threaded tests are still correct, but we won't test + // every malloc exhaustively. + CRYPTO_STATIC_MUTEX_lock_write(&malloc_failure_lock); + int should_fail = current_malloc_count == malloc_number_to_fail; + current_malloc_count++; + CRYPTO_STATIC_MUTEX_unlock_write(&malloc_failure_lock); + + if (should_fail && break_on_malloc_fail) { + raise(SIGTRAP); + } + if (should_fail) { + errno = ENOMEM; + } + return should_fail; +} + +#else +static int should_fail_allocation(void) { return 0; } +#endif + void *OPENSSL_malloc(size_t size) { + if (should_fail_allocation()) { + return NULL; + } + if (OPENSSL_memory_alloc != NULL) { assert(OPENSSL_memory_free != NULL); assert(OPENSSL_memory_get_size != NULL); @@ -194,6 +262,10 @@ void OPENSSL_free(void *orig_ptr) { } void *OPENSSL_realloc(void *orig_ptr, size_t new_size) { + if (should_fail_allocation()) { + return NULL; + } + if (orig_ptr == NULL) { return OPENSSL_malloc(new_size); } diff --git a/crypto/test/CMakeLists.txt b/crypto/test/CMakeLists.txt index b968fd78c..bf2086376 100644 --- a/crypto/test/CMakeLists.txt +++ b/crypto/test/CMakeLists.txt @@ -5,7 +5,6 @@ add_library( abi_test.cc file_test.cc - malloc.cc test_util.cc wycheproof_util.cc ) diff --git a/crypto/test/malloc.cc b/crypto/test/malloc.cc deleted file mode 100644 index 17189398f..000000000 --- a/crypto/test/malloc.cc +++ /dev/null @@ -1,143 +0,0 @@ -/* Copyright (c) 2014, Google Inc. - * - * Permission to use, copy, modify, and/or distribute this software for any - * purpose with or without fee is hereby granted, provided that the above - * copyright notice and this permission notice appear in all copies. - * - * THE SOFTWARE IS PROVIDED "AS IS" AND THE AUTHOR DISCLAIMS ALL WARRANTIES - * WITH REGARD TO THIS SOFTWARE INCLUDING ALL IMPLIED WARRANTIES OF - * MERCHANTABILITY AND FITNESS. IN NO EVENT SHALL THE AUTHOR BE LIABLE FOR ANY - * SPECIAL, DIRECT, INDIRECT, OR CONSEQUENTIAL DAMAGES OR ANY DAMAGES - * WHATSOEVER RESULTING FROM LOSS OF USE, DATA OR PROFITS, WHETHER IN AN ACTION - * OF CONTRACT, NEGLIGENCE OR OTHER TORTIOUS ACTION, ARISING OUT OF OR IN - * CONNECTION WITH THE USE OR PERFORMANCE OF THIS SOFTWARE. */ - -#include - -#if defined(__GLIBC__) && !defined(__UCLIBC__) -#define OPENSSL_GLIBC -#endif - -// This file isn't built on ARM or Aarch64 because we link statically in those -// builds and trying to override malloc in a static link doesn't work. It also -// requires glibc. It's also disabled on ASan builds as this interferes with -// ASan's malloc interceptor. -// -// TODO(davidben): See if this and ASan's and MSan's interceptors can be made to -// coexist. -#if defined(__linux__) && defined(OPENSSL_GLIBC) && !defined(OPENSSL_ARM) && \ - !defined(OPENSSL_AARCH64) && !defined(OPENSSL_ASAN) && \ - !defined(OPENSSL_MSAN) && !defined(OPENSSL_TSAN) - -#include -#include -#include -#include -#include -#include - -#include - - -// This file defines overrides for the standard allocation functions that allow -// a given allocation to be made to fail for testing. If the program is run -// with MALLOC_NUMBER_TO_FAIL set to a base-10 number then that allocation will -// return NULL. If MALLOC_BREAK_ON_FAIL is also defined then the allocation -// will signal SIGTRAP rather than return NULL. -// -// This code is not thread safe. - -static uint64_t current_malloc_count = 0; -static uint64_t malloc_number_to_fail = 0; -static bool failure_enabled = false, break_on_fail = false, in_call = false; - -extern "C" { -// These are other names for the standard allocation functions. -extern void *__libc_malloc(size_t size); -extern void *__libc_calloc(size_t num_elems, size_t size); -extern void *__libc_realloc(void *ptr, size_t size); -} - -static void exit_handler(void) { - if (failure_enabled && current_malloc_count > malloc_number_to_fail) { - _exit(88); - } -} - -static void cpp_new_handler() { - // Return to try again. It won't fail a second time. - return; -} - -// should_fail_allocation returns true if the current allocation should fail. -static bool should_fail_allocation() { - static bool init = false; - - if (in_call) { - return false; - } - - in_call = true; - - if (!init) { - const char *env = getenv("MALLOC_NUMBER_TO_FAIL"); - if (env != NULL && env[0] != 0) { - char *endptr; - malloc_number_to_fail = strtoull(env, &endptr, 10); - if (*endptr == 0) { - failure_enabled = true; - atexit(exit_handler); - std::set_new_handler(cpp_new_handler); - } - } - break_on_fail = (NULL != getenv("MALLOC_BREAK_ON_FAIL")); - init = true; - } - - in_call = false; - - if (!failure_enabled) { - return false; - } - - bool should_fail = (current_malloc_count == malloc_number_to_fail); - current_malloc_count++; - - if (should_fail && break_on_fail) { - raise(SIGTRAP); - } - return should_fail; -} - -extern "C" { - -void *malloc(size_t size) { - if (should_fail_allocation()) { - errno = ENOMEM; - return NULL; - } - - return __libc_malloc(size); -} - -void *calloc(size_t num_elems, size_t size) { - if (should_fail_allocation()) { - errno = ENOMEM; - return NULL; - } - - return __libc_calloc(num_elems, size); -} - -void *realloc(void *ptr, size_t size) { - if (should_fail_allocation()) { - errno = ENOMEM; - return NULL; - } - - return __libc_realloc(ptr, size); -} - -} // extern "C" - -#endif // defined(linux) && GLIBC && !ARM && !AARCH64 && !ASAN && !TSAN diff --git a/ssl/test/CMakeLists.txt b/ssl/test/CMakeLists.txt index f02d6e24f..70bb29e8c 100644 --- a/ssl/test/CMakeLists.txt +++ b/ssl/test/CMakeLists.txt @@ -15,7 +15,7 @@ add_executable( add_dependencies(bssl_shim global_target) -target_link_libraries(bssl_shim test_support_lib ssl crypto) +target_link_libraries(bssl_shim ssl crypto) if(CMAKE_SYSTEM_NAME STREQUAL "Linux") add_executable( @@ -33,7 +33,7 @@ if(CMAKE_SYSTEM_NAME STREQUAL "Linux") add_dependencies(handshaker global_target) - target_link_libraries(handshaker test_support_lib ssl crypto) + target_link_libraries(handshaker ssl crypto) else() # Declare a dummy target for run_tests to depend on. add_custom_target(handshaker) diff --git a/ssl/test/test_config.cc b/ssl/test/test_config.cc index d51c601b4..94828c6f6 100644 --- a/ssl/test/test_config.cc +++ b/ssl/test/test_config.cc @@ -496,25 +496,26 @@ static CRYPTO_once_t once = CRYPTO_ONCE_INIT; static int g_config_index = 0; static CRYPTO_BUFFER_POOL *g_pool = nullptr; -static void init_once() { - g_config_index = SSL_get_ex_new_index(0, NULL, NULL, NULL, NULL); - if (g_config_index < 0) { - abort(); - } - g_pool = CRYPTO_BUFFER_POOL_new(); - if (!g_pool) { - abort(); - } +static bool InitGlobals() { + CRYPTO_once(&once, [] { + g_config_index = SSL_get_ex_new_index(0, NULL, NULL, NULL, NULL); + g_pool = CRYPTO_BUFFER_POOL_new(); + }); + return g_config_index >= 0 && g_pool != nullptr; } bool SetTestConfig(SSL *ssl, const TestConfig *config) { - CRYPTO_once(&once, init_once); + if (!InitGlobals()) { + return false; + } return SSL_set_ex_data(ssl, g_config_index, (void *)config) == 1; } const TestConfig *GetTestConfig(const SSL *ssl) { - CRYPTO_once(&once, init_once); - return (const TestConfig *)SSL_get_ex_data(ssl, g_config_index); + if (!InitGlobals()) { + return nullptr; + } + return static_cast(SSL_get_ex_data(ssl, g_config_index)); } static int LegacyOCSPCallback(SSL *ssl, void *arg) { @@ -1371,13 +1372,16 @@ static bool MaybeInstallCertCompressionAlg( } bssl::UniquePtr TestConfig::SetupCtx(SSL_CTX *old_ctx) const { + if (!InitGlobals()) { + return nullptr; + } + bssl::UniquePtr ssl_ctx( SSL_CTX_new(is_dtls ? DTLS_method() : TLS_method())); if (!ssl_ctx) { return nullptr; } - CRYPTO_once(&once, init_once); SSL_CTX_set0_buffer_pool(ssl_ctx.get(), g_pool); std::string cipher_list = "ALL"; diff --git a/ssl/test/test_state.cc b/ssl/test/test_state.cc index 86deb5553..7c22a620b 100644 --- a/ssl/test/test_state.cc +++ b/ssl/test/test_state.cc @@ -32,25 +32,26 @@ static void TestStateExFree(void *parent, void *ptr, CRYPTO_EX_DATA *ad, delete ((TestState *)ptr); } -static void init_once() { - g_state_index = SSL_get_ex_new_index(0, NULL, NULL, NULL, TestStateExFree); - if (g_state_index < 0) { - abort(); - } +static bool InitGlobals() { + CRYPTO_once(&g_once, [] { + g_state_index = + SSL_get_ex_new_index(0, nullptr, nullptr, nullptr, TestStateExFree); + }); + return g_state_index >= 0; } struct timeval *GetClock() { - CRYPTO_once(&g_once, init_once); return &g_clock; } void AdvanceClock(unsigned seconds) { - CRYPTO_once(&g_once, init_once); g_clock.tv_sec += seconds; } bool SetTestState(SSL *ssl, std::unique_ptr state) { - CRYPTO_once(&g_once, init_once); + if (!InitGlobals()) { + return false; + } // |SSL_set_ex_data| takes ownership of |state| only on success. if (SSL_set_ex_data(ssl, g_state_index, state.get()) == 1) { state.release(); @@ -60,8 +61,10 @@ bool SetTestState(SSL *ssl, std::unique_ptr state) { } TestState *GetTestState(const SSL *ssl) { - CRYPTO_once(&g_once, init_once); - return (TestState *)SSL_get_ex_data(ssl, g_state_index); + if (!InitGlobals()) { + return nullptr; + } + return static_cast(SSL_get_ex_data(ssl, g_state_index)); } static void ssl_ctx_add_session(SSL_SESSION *session, void *void_param) { From 29564f2b633b1275e3e97703d86b41296211fb79 Mon Sep 17 00:00:00 2001 From: David Benjamin Date: Fri, 3 Feb 2023 18:15:09 -0500 Subject: [PATCH 03/11] Reject even moduli in RSA_check_key. RSA state management is generally a mess right now, which causes thread contention issues in highly threaded servers. We need to do a lot of work within the library to fix it, but in the end state, RSA_check_key (called by the parser), BN_MONT_CTX_set_locked, and freeze_private_key should all be unified. This means that anything which can causes the latter two steps to fail will be lifted up into the parser, currently RSA_check_key. We've broadly done that, but odd moduli (n, p, and q) are currently not covered by RSA_check_key. Fix that. We only need to check for odd n, because odd p and q are then implied by p * q == n. Update-Note: RSA keys with even moduli already do not work. (In addition to being nonsensical, all operations will fail with them because we cannot do Montgomery reduction on even moduli.) This CL shifts the error from when you use the key, to when you parse the key, like our other validation steps. Also after this lands, the check for odd modulus in cl/447099278 can be removed. Bug: 316 Change-Id: Ifa4af610316a8f717a026128078a5d38d046bff9 Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/56885 Reviewed-by: Bob Beck Commit-Queue: Bob Beck Auto-Submit: David Benjamin --- crypto/evp/evp_tests.txt | 5 +++++ crypto/fipsmodule/rsa/rsa.c | 3 ++- crypto/fipsmodule/rsa/rsa_impl.c | 7 +++++++ 3 files changed, 14 insertions(+), 1 deletion(-) diff --git a/crypto/evp/evp_tests.txt b/crypto/evp/evp_tests.txt index 0c890fd63..7fe2bced7 100644 --- a/crypto/evp/evp_tests.txt +++ b/crypto/evp/evp_tests.txt @@ -21,6 +21,11 @@ PublicKey = RSA-2048-SPKI-Negative Input = 30820121300d06092a864886f70d01010105000382010e003082010902820100cd0081ea7b2ae1ea06d59f7c73d9ffb94a09615c2e4ba7c636cef08dd3533ec3185525b015c769b99a77d6725bf9c3532a9b6e5f6627d5fb85160768d3dda9cbd35974511717dc3d309d2fc47ee41f97e32adb7f9dd864a1c4767a666ecd71bc1aacf5e7517f4b38594fea9b05e42d5ada9912008013e45316a4d9bb8ed086b88d28758bacaf922d46a868b485d239c9baeb0e2b64592710f42b2d1ea0a4b4802c0becab328f8a68b0073bdb546feea9809d2849912b390c1532bc7e29c7658f8175fae46f34332ff87bcab3e40649b98577869da0ea718353f0722754886913648760d122be676e0fc483dd20ffc31bda96a31966c9aa2e75ad03de47e1c44f0203010001 Error = NEGATIVE_NUMBER +# An RSA key with an even modulus +PublicKey = RSA-2048-Even-Modulus +Input = 30820122300d06092a864886f70d01010105000382010f003082010a0282010100cd0081ea7b2ae1ea06d59f7c73d9ffb94a09615c2e4ba7c636cef08dd3533ec3185525b015c769b99a77d6725bf9c3532a9b6e5f6627d5fb85160768d3dda9cbd35974511717dc3d309d2fc47ee41f97e32adb7f9dd864a1c4767a666ecd71bc1aacf5e7517f4b38594fea9b05e42d5ada9912008013e45316a4d9bb8ed086b88d28758bacaf922d46a868b485d239c9baeb0e2b64592710f42b2d1ea0a4b4802c0becab328f8a68b0073bdb546feea9809d2849912b390c1532bc7e29c7658f8175fae46f34332ff87bcab3e40649b98577869da0ea718353f0722754886913648760d122be676e0fc483dd20ffc31bda96a31966c9aa2e75ad03de47e1c44e0203010001 +Error = BAD_RSA_PARAMETERS + # The same key but with missing parameters rather than a NULL. PublicKey = RSA-2048-SPKI-Invalid Input = 30820120300b06092a864886f70d0101010382010f003082010a0282010100cd0081ea7b2ae1ea06d59f7c73d9ffb94a09615c2e4ba7c636cef08dd3533ec3185525b015c769b99a77d6725bf9c3532a9b6e5f6627d5fb85160768d3dda9cbd35974511717dc3d309d2fc47ee41f97e32adb7f9dd864a1c4767a666ecd71bc1aacf5e7517f4b38594fea9b05e42d5ada9912008013e45316a4d9bb8ed086b88d28758bacaf922d46a868b485d239c9baeb0e2b64592710f42b2d1ea0a4b4802c0becab328f8a68b0073bdb546feea9809d2849912b390c1532bc7e29c7658f8175fae46f34332ff87bcab3e40649b98577869da0ea718353f0722754886913648760d122be676e0fc483dd20ffc31bda96a31966c9aa2e75ad03de47e1c44f0203010001 diff --git a/crypto/fipsmodule/rsa/rsa.c b/crypto/fipsmodule/rsa/rsa.c index 6b3e22814..bbac05f51 100644 --- a/crypto/fipsmodule/rsa/rsa.c +++ b/crypto/fipsmodule/rsa/rsa.c @@ -787,7 +787,8 @@ int RSA_check_key(const RSA *key) { // Check that p * q == n. Before we multiply, we check that p and q are in // bounds, to avoid a DoS vector in |bn_mul_consttime| below. Note that - // n was bound by |rsa_check_public_key|. + // n was bound by |rsa_check_public_key|. This also implicitly checks p and q + // are odd, which is a necessary condition for Montgomery reduction. if (BN_is_negative(key->p) || BN_cmp(key->p, key->n) >= 0 || BN_is_negative(key->q) || BN_cmp(key->q, key->n) >= 0) { OPENSSL_PUT_ERROR(RSA, RSA_R_N_NOT_EQUAL_P_Q); diff --git a/crypto/fipsmodule/rsa/rsa_impl.c b/crypto/fipsmodule/rsa/rsa_impl.c index b9c47cd0c..df465f242 100644 --- a/crypto/fipsmodule/rsa/rsa_impl.c +++ b/crypto/fipsmodule/rsa/rsa_impl.c @@ -85,6 +85,13 @@ int rsa_check_public_key(const RSA *rsa) { return 0; } + // RSA moduli must be odd. In addition to being necessary for RSA in general, + // we cannot setup Montgomery reduction with even moduli. + if (!BN_is_odd(rsa->n)) { + OPENSSL_PUT_ERROR(RSA, RSA_R_BAD_RSA_PARAMETERS); + return 0; + } + // Mitigate DoS attacks by limiting the exponent size. 33 bits was chosen as // the limit based on the recommendations in [1] and [2]. Windows CryptoAPI // doesn't support values larger than 32 bits [3], so it is unlikely that From 3a16df9aa055b8e330bc1fa2e09e0be8ee404a94 Mon Sep 17 00:00:00 2001 From: David Benjamin Date: Thu, 2 Feb 2023 13:57:09 -0500 Subject: [PATCH 04/11] Rearrange bn/generic.c In preparation for adding aarch64 bn_add_words and bn_sub_words implementations, rearrange this so we first define BN_ADD_ASM and BN_MUL_ASM defines, and then gate fallbacks on that. This also required moving some functions around to group the add/mul functions together. Change-Id: I59281706db35ad3fb1186a4afd345a820f5542d2 Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/56965 Reviewed-by: Bob Beck Commit-Queue: Bob Beck Commit-Queue: David Benjamin Auto-Submit: David Benjamin --- crypto/fipsmodule/bn/generic.c | 327 +++++++++++++++++---------------- 1 file changed, 170 insertions(+), 157 deletions(-) diff --git a/crypto/fipsmodule/bn/generic.c b/crypto/fipsmodule/bn/generic.c index ee80a3ce7..628cc53a6 100644 --- a/crypto/fipsmodule/bn/generic.c +++ b/crypto/fipsmodule/bn/generic.c @@ -61,11 +61,20 @@ #include "internal.h" -// This file has two other implementations: x86 assembly language in -// asm/bn-586.pl and x86_64 inline assembly in asm/x86_64-gcc.c. -#if defined(OPENSSL_NO_ASM) || \ - !(defined(OPENSSL_X86) || \ - (defined(OPENSSL_X86_64) && (defined(__GNUC__) || defined(__clang__)))) +#if !defined(OPENSSL_NO_ASM) && defined(OPENSSL_X86) +// See asm/bn-586.pl. +#define BN_ADD_ASM +#define BN_MUL_ASM +#endif + +#if !defined(OPENSSL_NO_ASM) && defined(OPENSSL_X86_64) && \ + (defined(__GNUC__) || defined(__clang__)) +// See asm/x86_64-gcc.c +#define BN_ADD_ASM +#define BN_MUL_ASM +#endif + +#if !defined(BN_MUL_ASM) #ifdef BN_ULLONG #define mul_add(r, a, w, c) \ @@ -201,157 +210,6 @@ void bn_sqr_words(BN_ULONG *r, const BN_ULONG *a, size_t n) { } } -#ifdef BN_ULLONG -BN_ULONG bn_add_words(BN_ULONG *r, const BN_ULONG *a, const BN_ULONG *b, - size_t n) { - BN_ULLONG ll = 0; - - if (n == 0) { - return 0; - } - - while (n & ~3) { - ll += (BN_ULLONG)a[0] + b[0]; - r[0] = (BN_ULONG)ll; - ll >>= BN_BITS2; - ll += (BN_ULLONG)a[1] + b[1]; - r[1] = (BN_ULONG)ll; - ll >>= BN_BITS2; - ll += (BN_ULLONG)a[2] + b[2]; - r[2] = (BN_ULONG)ll; - ll >>= BN_BITS2; - ll += (BN_ULLONG)a[3] + b[3]; - r[3] = (BN_ULONG)ll; - ll >>= BN_BITS2; - a += 4; - b += 4; - r += 4; - n -= 4; - } - while (n) { - ll += (BN_ULLONG)a[0] + b[0]; - r[0] = (BN_ULONG)ll; - ll >>= BN_BITS2; - a++; - b++; - r++; - n--; - } - return (BN_ULONG)ll; -} - -#else // !BN_ULLONG - -BN_ULONG bn_add_words(BN_ULONG *r, const BN_ULONG *a, const BN_ULONG *b, - size_t n) { - BN_ULONG c, l, t; - - if (n == 0) { - return (BN_ULONG)0; - } - - c = 0; - while (n & ~3) { - t = a[0]; - t += c; - c = (t < c); - l = t + b[0]; - c += (l < t); - r[0] = l; - t = a[1]; - t += c; - c = (t < c); - l = t + b[1]; - c += (l < t); - r[1] = l; - t = a[2]; - t += c; - c = (t < c); - l = t + b[2]; - c += (l < t); - r[2] = l; - t = a[3]; - t += c; - c = (t < c); - l = t + b[3]; - c += (l < t); - r[3] = l; - a += 4; - b += 4; - r += 4; - n -= 4; - } - while (n) { - t = a[0]; - t += c; - c = (t < c); - l = t + b[0]; - c += (l < t); - r[0] = l; - a++; - b++; - r++; - n--; - } - return (BN_ULONG)c; -} - -#endif // !BN_ULLONG - -BN_ULONG bn_sub_words(BN_ULONG *r, const BN_ULONG *a, const BN_ULONG *b, - size_t n) { - BN_ULONG t1, t2; - int c = 0; - - if (n == 0) { - return (BN_ULONG)0; - } - - while (n & ~3) { - t1 = a[0]; - t2 = b[0]; - r[0] = t1 - t2 - c; - if (t1 != t2) { - c = (t1 < t2); - } - t1 = a[1]; - t2 = b[1]; - r[1] = t1 - t2 - c; - if (t1 != t2) { - c = (t1 < t2); - } - t1 = a[2]; - t2 = b[2]; - r[2] = t1 - t2 - c; - if (t1 != t2) { - c = (t1 < t2); - } - t1 = a[3]; - t2 = b[3]; - r[3] = t1 - t2 - c; - if (t1 != t2) { - c = (t1 < t2); - } - a += 4; - b += 4; - r += 4; - n -= 4; - } - while (n) { - t1 = a[0]; - t2 = b[0]; - r[0] = t1 - t2 - c; - if (t1 != t2) { - c = (t1 < t2); - } - a++; - b++; - r++; - n--; - } - return c; -} - // mul_add_c(a,b,c0,c1,c2) -- c+=a*b for three word number c=(c2,c1,c0) // mul_add_c2(a,b,c0,c1,c2) -- c+=2*a*b for three word number c=(c2,c1,c0) // sqr_add_c(a,i,c0,c1,c2) -- c+=a[i]^2 for three word number c=(c2,c1,c0) @@ -708,4 +566,159 @@ void bn_sqr_comba4(BN_ULONG r[8], const BN_ULONG a[4]) { #undef sqr_add_c #undef sqr_add_c2 -#endif +#endif // !BN_MUL_ASM + +#if !defined(BN_ADD_ASM) + +#ifdef BN_ULLONG +BN_ULONG bn_add_words(BN_ULONG *r, const BN_ULONG *a, const BN_ULONG *b, + size_t n) { + BN_ULLONG ll = 0; + + if (n == 0) { + return 0; + } + + while (n & ~3) { + ll += (BN_ULLONG)a[0] + b[0]; + r[0] = (BN_ULONG)ll; + ll >>= BN_BITS2; + ll += (BN_ULLONG)a[1] + b[1]; + r[1] = (BN_ULONG)ll; + ll >>= BN_BITS2; + ll += (BN_ULLONG)a[2] + b[2]; + r[2] = (BN_ULONG)ll; + ll >>= BN_BITS2; + ll += (BN_ULLONG)a[3] + b[3]; + r[3] = (BN_ULONG)ll; + ll >>= BN_BITS2; + a += 4; + b += 4; + r += 4; + n -= 4; + } + while (n) { + ll += (BN_ULLONG)a[0] + b[0]; + r[0] = (BN_ULONG)ll; + ll >>= BN_BITS2; + a++; + b++; + r++; + n--; + } + return (BN_ULONG)ll; +} + +#else // !BN_ULLONG + +BN_ULONG bn_add_words(BN_ULONG *r, const BN_ULONG *a, const BN_ULONG *b, + size_t n) { + BN_ULONG c, l, t; + + if (n == 0) { + return (BN_ULONG)0; + } + + c = 0; + while (n & ~3) { + t = a[0]; + t += c; + c = (t < c); + l = t + b[0]; + c += (l < t); + r[0] = l; + t = a[1]; + t += c; + c = (t < c); + l = t + b[1]; + c += (l < t); + r[1] = l; + t = a[2]; + t += c; + c = (t < c); + l = t + b[2]; + c += (l < t); + r[2] = l; + t = a[3]; + t += c; + c = (t < c); + l = t + b[3]; + c += (l < t); + r[3] = l; + a += 4; + b += 4; + r += 4; + n -= 4; + } + while (n) { + t = a[0]; + t += c; + c = (t < c); + l = t + b[0]; + c += (l < t); + r[0] = l; + a++; + b++; + r++; + n--; + } + return (BN_ULONG)c; +} + +#endif // !BN_ULLONG + +BN_ULONG bn_sub_words(BN_ULONG *r, const BN_ULONG *a, const BN_ULONG *b, + size_t n) { + BN_ULONG t1, t2; + int c = 0; + + if (n == 0) { + return (BN_ULONG)0; + } + + while (n & ~3) { + t1 = a[0]; + t2 = b[0]; + r[0] = t1 - t2 - c; + if (t1 != t2) { + c = (t1 < t2); + } + t1 = a[1]; + t2 = b[1]; + r[1] = t1 - t2 - c; + if (t1 != t2) { + c = (t1 < t2); + } + t1 = a[2]; + t2 = b[2]; + r[2] = t1 - t2 - c; + if (t1 != t2) { + c = (t1 < t2); + } + t1 = a[3]; + t2 = b[3]; + r[3] = t1 - t2 - c; + if (t1 != t2) { + c = (t1 < t2); + } + a += 4; + b += 4; + r += 4; + n -= 4; + } + while (n) { + t1 = a[0]; + t2 = b[0]; + r[0] = t1 - t2 - c; + if (t1 != t2) { + c = (t1 < t2); + } + a++; + b++; + r++; + n--; + } + return c; +} + +#endif // !BN_ADD_ASM From d1b451676eada2f2dcad9a20debf8b76fa17f403 Mon Sep 17 00:00:00 2001 From: David Benjamin Date: Thu, 2 Feb 2023 14:50:36 -0500 Subject: [PATCH 05/11] Add bn_add_words and bn_sub_words assembly for aarch64. It is 2023 and compilers *still* cannot use carry flags effectively, particularly GCC. There are some Clang-specific built-ins which help x86_64 (where we have asm anyway) but, on aarch64, the built-ins actually *regress performance* over the current formulation! I suspect Clang is getting confused by Arm and Intel having opposite borrow flags. https://clang.llvm.org/docs/LanguageExtensions.html#multiprecision-arithmetic-builtins Just include aarch64 assembly to avoid this. This provides a noticeable perf boost in code that uses these functions (Where bn_mul_mont is available, they're not used much in RSA, but the generic EC implementation does modular additions, and RSA private key checking spends a lot of time in our add/sub-based bn_div_consttime.) The new code is also smaller than the generic one (18 instructions each), probably because it avoids all the flag spills and only tries to unroll by two iterations. Before: Did 7137 RSA 2048 signing operations in 4022094us (1774.4 ops/sec) Did 326000 RSA 2048 verify (same key) operations in 4001828us (81462.8 ops/sec) Did 278000 RSA 2048 verify (fresh key) operations in 4001392us (69475.8 ops/sec) Did 34830 RSA 2048 private key parse operations in 4038893us (8623.7 ops/sec) Did 1196 RSA 4096 signing operations in 4015759us (297.8 ops/sec) Did 90000 RSA 4096 verify (same key) operations in 4041959us (22266.4 ops/sec) Did 79000 RSA 4096 verify (fresh key) operations in 4034561us (19580.8 ops/sec) Did 12222 RSA 4096 private key parse operations in 4004831us (3051.8 ops/sec) Did 10626 ECDSA P-384 signing operations in 4030764us (2636.2 ops/sec) Did 10800 ECDSA P-384 verify operations in 4052718us (2664.9 ops/sec) Did 4182 ECDSA P-521 signing operations in 4076198us (1026.0 ops/sec) Did 4059 ECDSA P-521 verify operations in 4063819us (998.8 ops/sec) After: Did 7189 RSA 2048 signing operations in 4021331us (1787.7 ops/sec) [+0.7%] Did 326000 RSA 2048 verify (same key) operations in 4010811us (81280.3 ops/sec) [-0.2%] Did 278000 RSA 2048 verify (fresh key) operations in 4004206us (69427.0 ops/sec) [-0.1%] Did 53040 RSA 2048 private key parse operations in 4050953us (13093.2 ops/sec) [+51.8%] Did 1200 RSA 4096 signing operations in 4035548us (297.4 ops/sec) [-0.2%] Did 90000 RSA 4096 verify (same key) operations in 4035686us (22301.0 ops/sec) [+0.2%] Did 80000 RSA 4096 verify (fresh key) operations in 4020989us (19895.6 ops/sec) [+1.6%] Did 20468 RSA 4096 private key parse operations in 4037474us (5069.5 ops/sec) [+66.1%] Did 11070 ECDSA P-384 signing operations in 4023595us (2751.3 ops/sec) [+4.4%] Did 11232 ECDSA P-384 verify operations in 4063116us (2764.4 ops/sec) [+3.7%] Did 4387 ECDSA P-521 signing operations in 4052728us (1082.5 ops/sec) [+5.5%] Did 4305 ECDSA P-521 verify operations in 4064660us (1059.1 ops/sec) [+6.0%] Change-Id: If2f739373cdd10fa1d4925d5e2725e87d2255fc0 Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/56966 Reviewed-by: Bob Beck Commit-Queue: David Benjamin --- crypto/fipsmodule/CMakeLists.txt | 1 + crypto/fipsmodule/bn/asm/bn-armv8.pl | 118 +++++++++++++++++++++++++++ crypto/fipsmodule/bn/generic.c | 5 ++ 3 files changed, 124 insertions(+) create mode 100755 crypto/fipsmodule/bn/asm/bn-armv8.pl diff --git a/crypto/fipsmodule/CMakeLists.txt b/crypto/fipsmodule/CMakeLists.txt index 2bfadab44..66fd44838 100644 --- a/crypto/fipsmodule/CMakeLists.txt +++ b/crypto/fipsmodule/CMakeLists.txt @@ -3,6 +3,7 @@ include_directories(../../include) perlasm(BCM_SOURCES aarch64 aesv8-armv8 aes/asm/aesv8-armx.pl) perlasm(BCM_SOURCES aarch64 aesv8-gcm-armv8 modes/asm/aesv8-gcm-armv8.pl) perlasm(BCM_SOURCES aarch64 armv8-mont bn/asm/armv8-mont.pl) +perlasm(BCM_SOURCES aarch64 bn-armv8 bn/asm/bn-armv8.pl) perlasm(BCM_SOURCES aarch64 ghash-neon-armv8 modes/asm/ghash-neon-armv8.pl) perlasm(BCM_SOURCES aarch64 ghashv8-armv8 modes/asm/ghashv8-armx.pl) perlasm(BCM_SOURCES aarch64 p256_beeu-armv8-asm ec/asm/p256_beeu-armv8-asm.pl) diff --git a/crypto/fipsmodule/bn/asm/bn-armv8.pl b/crypto/fipsmodule/bn/asm/bn-armv8.pl new file mode 100755 index 000000000..5aed8df15 --- /dev/null +++ b/crypto/fipsmodule/bn/asm/bn-armv8.pl @@ -0,0 +1,118 @@ +#!/usr/bin/env perl +# Copyright (c) 2023, Google Inc. +# +# Permission to use, copy, modify, and/or distribute this software for any +# purpose with or without fee is hereby granted, provided that the above +# copyright notice and this permission notice appear in all copies. +# +# THE SOFTWARE IS PROVIDED "AS IS" AND THE AUTHOR DISCLAIMS ALL WARRANTIES +# WITH REGARD TO THIS SOFTWARE INCLUDING ALL IMPLIED WARRANTIES OF +# MERCHANTABILITY AND FITNESS. IN NO EVENT SHALL THE AUTHOR BE LIABLE FOR ANY +# SPECIAL, DIRECT, INDIRECT, OR CONSEQUENTIAL DAMAGES OR ANY DAMAGES +# WHATSOEVER RESULTING FROM LOSS OF USE, DATA OR PROFITS, WHETHER IN AN ACTION +# OF CONTRACT, NEGLIGENCE OR OTHER TORTIOUS ACTION, ARISING OUT OF OR IN +# CONNECTION WITH THE USE OR PERFORMANCE OF THIS SOFTWARE. + +use strict; + +my $flavour = shift; +my $output = shift; +if ($flavour =~ /\./) { $output = $flavour; undef $flavour; } + +$0 =~ m/(.*[\/\\])[^\/\\]+$/; +my $dir = $1; +my $xlate; +( $xlate="${dir}arm-xlate.pl" and -f $xlate ) or +( $xlate="${dir}../../../perlasm/arm-xlate.pl" and -f $xlate) or +die "can't locate arm-xlate.pl"; + +open OUT, "| \"$^X\" \"$xlate\" $flavour \"$output\""; +*STDOUT = *OUT; + +my ($rp, $ap, $bp, $num) = ("x0", "x1", "x2", "x3"); +my ($a0, $a1, $b0, $b1, $num_pairs) = ("x4", "x5", "x6", "x7", "x8"); +my $code = <<____; +#include + +.text + +// BN_ULONG bn_add_words(BN_ULONG *rp, const BN_ULONG *ap, const BN_ULONG *bp, +// size_t num); +.type bn_add_words, %function +.globl bn_add_words +.align 4 +bn_add_words: + AARCH64_VALID_CALL_TARGET + # Clear the carry flag. + cmn xzr, xzr + + # aarch64 can load two registers at a time, so we do two loop iterations at + # at a time. Split $num = 2 * $num_pairs + $num. This allows loop + # operations to use CBNZ without clobbering the carry flag. + lsr $num_pairs, $num, #1 + and $num, $num, #1 + + cbz $num_pairs, .Ladd_tail +.Ladd_loop: + ldp $a0, $a1, [$ap], #16 + ldp $b0, $b1, [$bp], #16 + sub $num_pairs, $num_pairs, #1 + adcs $a0, $a0, $b0 + adcs $a1, $a1, $b1 + stp $a0, $a1, [$rp], #16 + cbnz $num_pairs, .Ladd_loop + +.Ladd_tail: + cbz $num, .Ladd_exit + ldr $a0, [$ap], #8 + ldr $b0, [$bp], #8 + adcs $a0, $a0, $b0 + str $a0, [$rp], #8 + +.Ladd_exit: + cset x0, cs + ret +.size bn_add_words,.-bn_add_words + +// BN_ULONG bn_sub_words(BN_ULONG *rp, const BN_ULONG *ap, const BN_ULONG *bp, +// size_t num); +.type bn_sub_words, %function +.globl bn_sub_words +.align 4 +bn_sub_words: + AARCH64_VALID_CALL_TARGET + # Set the carry flag. Arm's borrow bit is flipped from the carry flag, + # so we want C = 1 here. + cmp xzr, xzr + + # aarch64 can load two registers at a time, so we do two loop iterations at + # at a time. Split $num = 2 * $num_pairs + $num. This allows loop + # operations to use CBNZ without clobbering the carry flag. + lsr $num_pairs, $num, #1 + and $num, $num, #1 + + cbz $num_pairs, .Lsub_tail +.Lsub_loop: + ldp $a0, $a1, [$ap], #16 + ldp $b0, $b1, [$bp], #16 + sub $num_pairs, $num_pairs, #1 + sbcs $a0, $a0, $b0 + sbcs $a1, $a1, $b1 + stp $a0, $a1, [$rp], #16 + cbnz $num_pairs, .Lsub_loop + +.Lsub_tail: + cbz $num, .Lsub_exit + ldr $a0, [$ap], #8 + ldr $b0, [$bp], #8 + sbcs $a0, $a0, $b0 + str $a0, [$rp], #8 + +.Lsub_exit: + cset x0, cc + ret +size bn_sub_words,.-bn_sub_words +____ + +print $code; +close STDOUT or die "error closing STDOUT: $!"; diff --git a/crypto/fipsmodule/bn/generic.c b/crypto/fipsmodule/bn/generic.c index 628cc53a6..df4a834af 100644 --- a/crypto/fipsmodule/bn/generic.c +++ b/crypto/fipsmodule/bn/generic.c @@ -74,6 +74,11 @@ #define BN_MUL_ASM #endif +#if !defined(OPENSSL_NO_ASM) && defined(OPENSSL_AARCH64) +// See asm/bn-armv8.pl. +#define BN_ADD_ASM +#endif + #if !defined(BN_MUL_ASM) #ifdef BN_ULLONG From d4396e387c820198f509a6927facea84592cddd8 Mon Sep 17 00:00:00 2001 From: David Benjamin Date: Wed, 1 Feb 2023 15:35:56 -0500 Subject: [PATCH 06/11] Avoid branches in GCC in bn/generic.c. bn/generic.c is used for functions like bn_add_words, when there is no assembly implementation available. They're meant to be constant-time, but are particularly dependent on the compiler in this. I ran our valgrind-based tooling and found a couple issues in GCC: First, the various mul_add and sqr_add macros end up branching in GCC. Replacing the conditionals with expressions like c += (a < b) seems to mitigate this. Second, bn_sub_words produces branches in GCC. Replacing the expressions with bit expressions involving the boolean comparisons seems to work for now. https://gcc.gnu.org/bugzilla/show_bug.cgi?id=79173 discusses problems with GCC here, which seem to be as yet unresolved. Clang already reliably avoided branches in all of these. (Though it still spills the carry flag far more than would be ideal.) I also checked in godbolt that the new versions didn't generate branches in MSVC, but we don't have tooling to validate this as rigorously. Change-Id: I739758a396fb5ee27fb88bee71bd13ae9cb92bd0 Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/56967 Commit-Queue: David Benjamin Reviewed-by: Bob Beck --- crypto/fipsmodule/bn/generic.c | 38 +++++++++------------------------- 1 file changed, 10 insertions(+), 28 deletions(-) diff --git a/crypto/fipsmodule/bn/generic.c b/crypto/fipsmodule/bn/generic.c index df4a834af..bf4971f16 100644 --- a/crypto/fipsmodule/bn/generic.c +++ b/crypto/fipsmodule/bn/generic.c @@ -232,9 +232,7 @@ void bn_sqr_words(BN_ULONG *r, const BN_ULONG *a, size_t n) { (c0) = (BN_ULONG)Lw(t); \ hi = (BN_ULONG)Hw(t); \ (c1) += (hi); \ - if ((c1) < hi) { \ - (c2)++; \ - } \ + (c2) += (c1) < hi; \ } while (0) #define mul_add_c2(a, b, c0, c1, c2) \ @@ -245,16 +243,12 @@ void bn_sqr_words(BN_ULONG *r, const BN_ULONG *a, size_t n) { (c0) = (BN_ULONG)Lw(tt); \ hi = (BN_ULONG)Hw(tt); \ (c1) += hi; \ - if ((c1) < hi) { \ - (c2)++; \ - } \ + (c2) += (c1) < hi; \ t += (c0); /* no carry */ \ (c0) = (BN_ULONG)Lw(t); \ hi = (BN_ULONG)Hw(t); \ (c1) += hi; \ - if ((c1) < hi) { \ - (c2)++; \ - } \ + (c2) += (c1) < hi; \ } while (0) #define sqr_add_c(a, i, c0, c1, c2) \ @@ -265,9 +259,7 @@ void bn_sqr_words(BN_ULONG *r, const BN_ULONG *a, size_t n) { (c0) = (BN_ULONG)Lw(t); \ hi = (BN_ULONG)Hw(t); \ (c1) += hi; \ - if ((c1) < hi) { \ - (c2)++; \ - } \ + (c2) += (c1) < hi; \ } while (0) #define sqr_add_c2(a, i, j, c0, c1, c2) mul_add_c2((a)[i], (a)[j], c0, c1, c2) @@ -675,7 +667,7 @@ BN_ULONG bn_add_words(BN_ULONG *r, const BN_ULONG *a, const BN_ULONG *b, BN_ULONG bn_sub_words(BN_ULONG *r, const BN_ULONG *a, const BN_ULONG *b, size_t n) { BN_ULONG t1, t2; - int c = 0; + BN_ULONG c = 0; if (n == 0) { return (BN_ULONG)0; @@ -685,27 +677,19 @@ BN_ULONG bn_sub_words(BN_ULONG *r, const BN_ULONG *a, const BN_ULONG *b, t1 = a[0]; t2 = b[0]; r[0] = t1 - t2 - c; - if (t1 != t2) { - c = (t1 < t2); - } + c = (t1 < t2) | ((t1 == t2) & c); t1 = a[1]; t2 = b[1]; r[1] = t1 - t2 - c; - if (t1 != t2) { - c = (t1 < t2); - } + c = (t1 < t2) | ((t1 == t2) & c); t1 = a[2]; t2 = b[2]; r[2] = t1 - t2 - c; - if (t1 != t2) { - c = (t1 < t2); - } + c = (t1 < t2) | ((t1 == t2) & c); t1 = a[3]; t2 = b[3]; r[3] = t1 - t2 - c; - if (t1 != t2) { - c = (t1 < t2); - } + c = (t1 < t2) | ((t1 == t2) & c); a += 4; b += 4; r += 4; @@ -715,9 +699,7 @@ BN_ULONG bn_sub_words(BN_ULONG *r, const BN_ULONG *a, const BN_ULONG *b, t1 = a[0]; t2 = b[0]; r[0] = t1 - t2 - c; - if (t1 != t2) { - c = (t1 < t2); - } + c = (t1 < t2) | ((t1 == t2) & c); a++; b++; r++; From a9ce915318ff01fbb4ddbb3c276a03acd2d7cc56 Mon Sep 17 00:00:00 2001 From: David Benjamin Date: Mon, 6 Feb 2023 15:45:56 -0500 Subject: [PATCH 07/11] Add ABI tests for bn_add_words, etc. They're written in assembly, yet we never actually had ABI tests for them. Change-Id: Ib1c6c1d55c91d718f77f7dce9b6d691fb86c89b4 Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/56930 Reviewed-by: Bob Beck Commit-Queue: Bob Beck --- crypto/fipsmodule/bn/bn_test.cc | 35 +++++++++++++++++++++++++++++++++ 1 file changed, 35 insertions(+) diff --git a/crypto/fipsmodule/bn/bn_test.cc b/crypto/fipsmodule/bn/bn_test.cc index 9d9e1d368..01dcfbb16 100644 --- a/crypto/fipsmodule/bn/bn_test.cc +++ b/crypto/fipsmodule/bn/bn_test.cc @@ -2796,6 +2796,41 @@ TEST_F(BNTest, MontgomeryLarge) { ctx(), nullptr)); } +#if defined(SUPPORTS_ABI_TEST) +// These functions are not always implemented in assembly, but they sometimes +// are, so include ABI tests for each. +TEST_F(BNTest, ArithmeticABI) { + EXPECT_EQ(0u, CHECK_ABI(bn_add_words, nullptr, nullptr, nullptr, 0)); + EXPECT_EQ(0u, CHECK_ABI(bn_sub_words, nullptr, nullptr, nullptr, 0)); + + for (size_t num : + {1, 2, 3, 4, 5, 6, 7, 8, 9, 15, 16, 17, 31, 32, 33, 63, 64, 65}) { + SCOPED_TRACE(num); + std::vector a(num, 123456789); + std::vector b(num, static_cast(-1)); + std::vector r(num); + + CHECK_ABI(bn_add_words, r.data(), a.data(), b.data(), num); + CHECK_ABI(bn_sub_words, r.data(), a.data(), b.data(), num); + + CHECK_ABI(bn_mul_words, r.data(), a.data(), num, 42); + CHECK_ABI(bn_mul_add_words, r.data(), a.data(), num, 42); + + r.resize(2 * num); + CHECK_ABI(bn_sqr_words, r.data(), a.data(), num); + + if (num == 4) { + CHECK_ABI(bn_mul_comba4, r.data(), a.data(), b.data()); + CHECK_ABI(bn_sqr_comba4, r.data(), a.data()); + } + if (num == 8) { + CHECK_ABI(bn_mul_comba8, r.data(), a.data(), b.data()); + CHECK_ABI(bn_sqr_comba8, r.data(), a.data()); + } + } +} +#endif + #if defined(OPENSSL_BN_ASM_MONT) && defined(SUPPORTS_ABI_TEST) TEST_F(BNTest, BNMulMontABI) { for (size_t words : {4, 5, 6, 7, 8, 16, 32}) { From 5e356a8a9a28bd6541ba360b47f69628e13bb534 Mon Sep 17 00:00:00 2001 From: David Benjamin Date: Sat, 4 Feb 2023 19:44:34 -0500 Subject: [PATCH 08/11] Partially mitigate quadratic-time malloc tests in unit tests Malloc failure testing is quadratic in the number of allocations. To test a failure at allocation N, we must first run the previous N-1 allocations. Now that we have combined GTest binaries, this does not work very well. Use the test listener to reset the counter across independent tests. We assume failures in a previous test won't interfere in the next one and run each test's counter in parallel. The assumption isn't *quite* true because we have a lot of internal init-once machinery that is reused across otherwise "independent" tests, but it's close enough that I was able to find some bugs, fixed in the next commit. That said, the tests still take too long to run to completion. Bug: 127 Change-Id: I6836793448fbdc740a8cc424361e6b3dd66fb8a6 Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/56926 Reviewed-by: Bob Beck Commit-Queue: David Benjamin --- crypto/internal.h | 10 ++++++++++ crypto/mem.c | 14 ++++++++++++-- crypto/test/gtest_main.h | 18 +++++++++++++----- 3 files changed, 35 insertions(+), 7 deletions(-) diff --git a/crypto/internal.h b/crypto/internal.h index 46a4e70ee..576ad85c9 100644 --- a/crypto/internal.h +++ b/crypto/internal.h @@ -225,6 +225,16 @@ typedef __uint128_t uint128_t; #define OPENSSL_SSE2 #endif +#if defined(BORINGSSL_MALLOC_FAILURE_TESTING) +// OPENSSL_reset_malloc_counter_for_testing, when malloc testing is enabled, +// resets the internal malloc counter, to simulate further malloc failures. This +// should be called in between independent tests, at a point where failure from +// a previous test will not impact subsequent ones. +OPENSSL_EXPORT void OPENSSL_reset_malloc_counter_for_testing(void); +#else +OPENSSL_INLINE void OPENSSL_reset_malloc_counter_for_testing(void) {} +#endif + // Pointer utility functions. diff --git a/crypto/mem.c b/crypto/mem.c index abba4a448..97a85e995 100644 --- a/crypto/mem.c +++ b/crypto/mem.c @@ -146,11 +146,14 @@ static struct CRYPTO_STATIC_MUTEX malloc_failure_lock = CRYPTO_STATIC_MUTEX_INIT; static uint64_t current_malloc_count = 0; static uint64_t malloc_number_to_fail = 0; -static int malloc_failure_enabled = 0, break_on_malloc_fail = 0; +static int malloc_failure_enabled = 0, break_on_malloc_fail = 0, + any_malloc_failed = 0; static void malloc_exit_handler(void) { CRYPTO_STATIC_MUTEX_lock_read(&malloc_failure_lock); - if (malloc_failure_enabled && current_malloc_count > malloc_number_to_fail) { + if (any_malloc_failed) { + // Signal to the test driver that some allocation failed, so it knows to + // increment the counter and continue. _exit(88); } CRYPTO_STATIC_MUTEX_unlock_read(&malloc_failure_lock); @@ -183,6 +186,7 @@ static int should_fail_allocation() { CRYPTO_STATIC_MUTEX_lock_write(&malloc_failure_lock); int should_fail = current_malloc_count == malloc_number_to_fail; current_malloc_count++; + any_malloc_failed = any_malloc_failed || should_fail; CRYPTO_STATIC_MUTEX_unlock_write(&malloc_failure_lock); if (should_fail && break_on_malloc_fail) { @@ -194,6 +198,12 @@ static int should_fail_allocation() { return should_fail; } +void OPENSSL_reset_malloc_counter_for_testing(void) { + CRYPTO_STATIC_MUTEX_lock_write(&malloc_failure_lock); + current_malloc_count = 0; + CRYPTO_STATIC_MUTEX_unlock_write(&malloc_failure_lock); +} + #else static int should_fail_allocation(void) { return 0; } #endif diff --git a/crypto/test/gtest_main.h b/crypto/test/gtest_main.h index 20ccf2143..05d468ecf 100644 --- a/crypto/test/gtest_main.h +++ b/crypto/test/gtest_main.h @@ -31,13 +31,15 @@ OPENSSL_MSVC_PRAGMA(warning(pop)) #include #endif +#include "../internal.h" + BSSL_NAMESPACE_BEGIN -class ErrorTestEventListener : public testing::EmptyTestEventListener { +class TestEventListener : public testing::EmptyTestEventListener { public: - ErrorTestEventListener() {} - ~ErrorTestEventListener() override {} + TestEventListener() {} + ~TestEventListener() override {} void OnTestEnd(const testing::TestInfo &test_info) override { if (test_info.result()->Failed()) { @@ -48,6 +50,13 @@ class ErrorTestEventListener : public testing::EmptyTestEventListener { // error queue without printing. ERR_clear_error(); } + + // Malloc failure testing is quadratic in the number of mallocs. Running + // multiple tests sequentially thus scales badly. Reset the malloc counter + // between tests. This way we will test, each test with the first allocation + // failing, then the second, and so on, until the test with the most + // allocations runs out. + OPENSSL_reset_malloc_counter_for_testing(); } }; @@ -75,8 +84,7 @@ inline void SetupGoogleTest() { signal(SIGPIPE, SIG_IGN); #endif - testing::UnitTest::GetInstance()->listeners().Append( - new ErrorTestEventListener); + testing::UnitTest::GetInstance()->listeners().Append(new TestEventListener); } BSSL_NAMESPACE_END From f7d37fba96e5640186b31ccb834bde98102d6ac7 Mon Sep 17 00:00:00 2001 From: David Benjamin Date: Sat, 4 Feb 2023 19:45:04 -0500 Subject: [PATCH 09/11] Fix various malloc failure paths. Caught by running malloc failure tests on unit tests. Bug: 563 Change-Id: Ic0167ef346a282dc8b5a26a1cedafced7fef9ed0 Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/56927 Commit-Queue: David Benjamin Reviewed-by: Bob Beck --- crypto/asn1/a_mbstr.c | 33 +++++------- crypto/asn1/asn1_test.cc | 2 + crypto/ecdh_extra/ecdh_test.cc | 1 + crypto/evp/p_hkdf.c | 4 +- crypto/fipsmodule/bn/exponentiation.c | 2 +- crypto/fipsmodule/ec/ec_test.cc | 9 ++-- crypto/obj/obj.c | 25 ++++++--- crypto/pkcs7/pkcs7_test.cc | 1 + crypto/rsa_extra/rsa_test.cc | 1 + crypto/stack/stack_test.cc | 2 + crypto/x509/x509_cmp.c | 10 ++-- crypto/x509/x509_test.cc | 4 ++ ssl/handoff.cc | 12 ++++- ssl/ssl_session.cc | 1 + ssl/ssl_test.cc | 75 +++++++++++++++++---------- ssl/tls13_enc.cc | 1 + 16 files changed, 121 insertions(+), 62 deletions(-) diff --git a/crypto/asn1/a_mbstr.c b/crypto/asn1/a_mbstr.c index ef74d0d36..81916c22e 100644 --- a/crypto/asn1/a_mbstr.c +++ b/crypto/asn1/a_mbstr.c @@ -85,11 +85,6 @@ OPENSSL_DECLARE_ERROR_REASON(ASN1, INVALID_UTF8STRING) int ASN1_mbstring_ncopy(ASN1_STRING **out, const unsigned char *in, int len, int inform, unsigned long mask, long minsize, long maxsize) { - int str_type; - char free_out; - ASN1_STRING *dest; - size_t nchar = 0; - char strbuf[32]; if (len == -1) { len = strlen((const char *)in); } @@ -128,7 +123,7 @@ int ASN1_mbstring_ncopy(ASN1_STRING **out, const unsigned char *in, int len, // Check |minsize| and |maxsize| and work out the minimal type, if any. CBS cbs; CBS_init(&cbs, in, len); - size_t utf8_len = 0; + size_t utf8_len = 0, nchar = 0; while (CBS_len(&cbs) != 0) { uint32_t c; if (!decode_func(&cbs, &c)) { @@ -169,6 +164,7 @@ int ASN1_mbstring_ncopy(ASN1_STRING **out, const unsigned char *in, int len, utf8_len += cbb_get_utf8_len(c); } + char strbuf[32]; if (minsize > 0 && nchar < (size_t)minsize) { OPENSSL_PUT_ERROR(ASN1, ASN1_R_STRING_TOO_SHORT); BIO_snprintf(strbuf, sizeof strbuf, "%ld", minsize); @@ -184,6 +180,7 @@ int ASN1_mbstring_ncopy(ASN1_STRING **out, const unsigned char *in, int len, } // Now work out output format and string type + int str_type; int (*encode_func)(CBB *, uint32_t) = cbb_add_latin1; size_t size_estimate = nchar; int outform = MBSTRING_ASC; @@ -216,31 +213,28 @@ int ASN1_mbstring_ncopy(ASN1_STRING **out, const unsigned char *in, int len, if (!out) { return str_type; } + + int free_dest = 0; + ASN1_STRING *dest; if (*out) { - free_out = 0; dest = *out; - if (dest->data) { - dest->length = 0; - OPENSSL_free(dest->data); - dest->data = NULL; - } - dest->type = str_type; } else { - free_out = 1; + free_dest = 1; dest = ASN1_STRING_type_new(str_type); if (!dest) { OPENSSL_PUT_ERROR(ASN1, ERR_R_MALLOC_FAILURE); return -1; } - *out = dest; } // If both the same type just copy across if (inform == outform) { if (!ASN1_STRING_set(dest, in, len)) { OPENSSL_PUT_ERROR(ASN1, ERR_R_MALLOC_FAILURE); - return -1; + goto err; } + dest->type = str_type; + *out = dest; return str_type; } @@ -267,12 +261,13 @@ int ASN1_mbstring_ncopy(ASN1_STRING **out, const unsigned char *in, int len, OPENSSL_free(data); goto err; } - dest->length = (int)(data_len - 1); - dest->data = data; + dest->type = str_type; + ASN1_STRING_set0(dest, data, (int)data_len - 1); + *out = dest; return str_type; err: - if (free_out) { + if (free_dest) { ASN1_STRING_free(dest); } CBB_cleanup(&cbb); diff --git a/crypto/asn1/asn1_test.cc b/crypto/asn1/asn1_test.cc index 59e80d2cb..3bb7b3484 100644 --- a/crypto/asn1/asn1_test.cc +++ b/crypto/asn1/asn1_test.cc @@ -1339,6 +1339,7 @@ TEST(ASN1Test, StringPrintEx) { SCOPED_TRACE(t.flags); bssl::UniquePtr str(ASN1_STRING_type_new(t.type)); + ASSERT_TRUE(str); ASSERT_TRUE(ASN1_STRING_set(str.get(), t.data.data(), t.data.size())); str->flags = t.str_flags; @@ -1393,6 +1394,7 @@ TEST(ASN1Test, StringPrintEx) { SCOPED_TRACE(t.flags); bssl::UniquePtr str(ASN1_STRING_type_new(t.type)); + ASSERT_TRUE(str); ASSERT_TRUE(ASN1_STRING_set(str.get(), t.data.data(), t.data.size())); str->flags = t.str_flags; diff --git a/crypto/ecdh_extra/ecdh_test.cc b/crypto/ecdh_extra/ecdh_test.cc index 4b88754fd..39485259d 100644 --- a/crypto/ecdh_extra/ecdh_test.cc +++ b/crypto/ecdh_extra/ecdh_test.cc @@ -274,6 +274,7 @@ TEST(ECDHTest, GroupMismatch) { } bssl::UniquePtr key(EC_KEY_new()); + ASSERT_TRUE(key); ASSERT_TRUE(EC_KEY_set_group(key.get(), a.get())); ASSERT_TRUE(EC_KEY_generate_key(key.get())); diff --git a/crypto/evp/p_hkdf.c b/crypto/evp/p_hkdf.c index 932372dfd..05158e299 100644 --- a/crypto/evp/p_hkdf.c +++ b/crypto/evp/p_hkdf.c @@ -64,7 +64,7 @@ static int pkey_hkdf_copy(EVP_PKEY_CTX *dst, EVP_PKEY_CTX *src) { if (hctx_src->key_len != 0) { hctx_dst->key = OPENSSL_memdup(hctx_src->key, hctx_src->key_len); - if (hctx_src->key == NULL) { + if (hctx_dst->key == NULL) { OPENSSL_PUT_ERROR(EVP, ERR_R_MALLOC_FAILURE); return 0; } @@ -73,7 +73,7 @@ static int pkey_hkdf_copy(EVP_PKEY_CTX *dst, EVP_PKEY_CTX *src) { if (hctx_src->salt_len != 0) { hctx_dst->salt = OPENSSL_memdup(hctx_src->salt, hctx_src->salt_len); - if (hctx_src->salt == NULL) { + if (hctx_dst->salt == NULL) { OPENSSL_PUT_ERROR(EVP, ERR_R_MALLOC_FAILURE); return 0; } diff --git a/crypto/fipsmodule/bn/exponentiation.c b/crypto/fipsmodule/bn/exponentiation.c index 4ec9171e2..41c723354 100644 --- a/crypto/fipsmodule/bn/exponentiation.c +++ b/crypto/fipsmodule/bn/exponentiation.c @@ -444,6 +444,7 @@ static int mod_exp_recp(BIGNUM *r, const BIGNUM *a, const BIGNUM *p, return BN_one(r); } + BN_RECP_CTX_init(&recp); BN_CTX_start(ctx); aa = BN_CTX_get(ctx); val[0] = BN_CTX_get(ctx); @@ -451,7 +452,6 @@ static int mod_exp_recp(BIGNUM *r, const BIGNUM *a, const BIGNUM *p, goto err; } - BN_RECP_CTX_init(&recp); if (m->neg) { // ignore sign of 'm' if (!BN_copy(aa, m)) { diff --git a/crypto/fipsmodule/ec/ec_test.cc b/crypto/fipsmodule/ec/ec_test.cc index 8e144ec0c..88665b2e4 100644 --- a/crypto/fipsmodule/ec/ec_test.cc +++ b/crypto/fipsmodule/ec/ec_test.cc @@ -656,6 +656,7 @@ TEST_P(ECCurveTest, Compare) { bssl::UniquePtr inf1(EC_POINT_new(group())), inf2(EC_POINT_new(group())); ASSERT_TRUE(inf1); + ASSERT_TRUE(inf2); ASSERT_TRUE(EC_POINT_set_to_infinity(group(), inf1.get())); // |q| is currently -|pub2|. ASSERT_TRUE(EC_POINT_add(group(), inf2.get(), pub2, q.get(), nullptr)); @@ -843,8 +844,8 @@ TEST_P(ECCurveTest, SetInvalidPrivateKey) { bssl::UniquePtr key(EC_KEY_new_by_curve_name(GetParam())); ASSERT_TRUE(key); - bssl::UniquePtr bn(BN_new()); - ASSERT_TRUE(BN_one(bn.get())); + bssl::UniquePtr bn(BN_dup(BN_value_one())); + ASSERT_TRUE(bn); BN_set_negative(bn.get(), 1); EXPECT_FALSE(EC_KEY_set_private_key(key.get(), bn.get())) << "Unexpectedly set a key of -1"; @@ -937,11 +938,13 @@ TEST_P(ECCurveTest, P224Bug) { TEST_P(ECCurveTest, GPlusMinusG) { const EC_POINT *g = EC_GROUP_get0_generator(group()); + bssl::UniquePtr p(EC_POINT_dup(g, group())); ASSERT_TRUE(p); ASSERT_TRUE(EC_POINT_invert(group(), p.get(), nullptr)); - bssl::UniquePtr sum(EC_POINT_new(group())); + bssl::UniquePtr sum(EC_POINT_new(group())); + ASSERT_TRUE(sum); ASSERT_TRUE(EC_POINT_add(group(), sum.get(), g, p.get(), nullptr)); EXPECT_TRUE(EC_POINT_is_at_infinity(group(), sum.get())); } diff --git a/crypto/obj/obj.c b/crypto/obj/obj.c index 958625d02..c4e1aee6f 100644 --- a/crypto/obj/obj.c +++ b/crypto/obj/obj.c @@ -506,25 +506,37 @@ static int cmp_long_name(const ASN1_OBJECT *a, const ASN1_OBJECT *b) { // obj_add_object inserts |obj| into the various global hashes for run-time // added objects. It returns one on success or zero otherwise. static int obj_add_object(ASN1_OBJECT *obj) { - int ok; - ASN1_OBJECT *old_object; - obj->flags &= ~(ASN1_OBJECT_FLAG_DYNAMIC | ASN1_OBJECT_FLAG_DYNAMIC_STRINGS | ASN1_OBJECT_FLAG_DYNAMIC_DATA); CRYPTO_STATIC_MUTEX_lock_write(&global_added_lock); if (global_added_by_nid == NULL) { global_added_by_nid = lh_ASN1_OBJECT_new(hash_nid, cmp_nid); + } + if (global_added_by_data == NULL) { global_added_by_data = lh_ASN1_OBJECT_new(hash_data, cmp_data); - global_added_by_short_name = lh_ASN1_OBJECT_new(hash_short_name, cmp_short_name); + } + if (global_added_by_short_name == NULL) { + global_added_by_short_name = + lh_ASN1_OBJECT_new(hash_short_name, cmp_short_name); + } + if (global_added_by_long_name == NULL) { global_added_by_long_name = lh_ASN1_OBJECT_new(hash_long_name, cmp_long_name); } + int ok = 0; + if (global_added_by_nid == NULL || + global_added_by_data == NULL || + global_added_by_short_name == NULL || + global_added_by_long_name == NULL) { + goto err; + } + // We don't pay attention to |old_object| (which contains any previous object // that was evicted from the hashes) because we don't have a reference count // on ASN1_OBJECT values. Also, we should never have duplicates nids and so // should always have objects in |global_added_by_nid|. - + ASN1_OBJECT *old_object; ok = lh_ASN1_OBJECT_insert(global_added_by_nid, &old_object, obj); if (obj->length != 0 && obj->data != NULL) { ok &= lh_ASN1_OBJECT_insert(global_added_by_data, &old_object, obj); @@ -535,8 +547,9 @@ static int obj_add_object(ASN1_OBJECT *obj) { if (obj->ln != NULL) { ok &= lh_ASN1_OBJECT_insert(global_added_by_long_name, &old_object, obj); } - CRYPTO_STATIC_MUTEX_unlock_write(&global_added_lock); +err: + CRYPTO_STATIC_MUTEX_unlock_write(&global_added_lock); return ok; } diff --git a/crypto/pkcs7/pkcs7_test.cc b/crypto/pkcs7/pkcs7_test.cc index bf8537964..3c042ec5f 100644 --- a/crypto/pkcs7/pkcs7_test.cc +++ b/crypto/pkcs7/pkcs7_test.cc @@ -639,6 +639,7 @@ static void TestPEMCRLs(const char *pem) { bssl::UniquePtr bio(BIO_new_mem_buf(pem, strlen(pem))); ASSERT_TRUE(bio); bssl::UniquePtr crls(sk_X509_CRL_new_null()); + ASSERT_TRUE(crls); ASSERT_TRUE(PKCS7_get_PEM_CRLs(crls.get(), bio.get())); ASSERT_EQ(1u, sk_X509_CRL_num(crls.get())); diff --git a/crypto/rsa_extra/rsa_test.cc b/crypto/rsa_extra/rsa_test.cc index 883eb97f2..d3ee69b31 100644 --- a/crypto/rsa_extra/rsa_test.cc +++ b/crypto/rsa_extra/rsa_test.cc @@ -513,6 +513,7 @@ TEST(RSATest, GenerateFIPS) { SCOPED_TRACE(bits); rsa.reset(RSA_new()); + ASSERT_TRUE(rsa); ASSERT_TRUE(RSA_generate_key_fips(rsa.get(), bits, nullptr)); EXPECT_EQ(bits, BN_num_bits(rsa->n)); } diff --git a/crypto/stack/stack_test.cc b/crypto/stack/stack_test.cc index 98e54489a..1ff44b9a4 100644 --- a/crypto/stack/stack_test.cc +++ b/crypto/stack/stack_test.cc @@ -317,6 +317,7 @@ TEST(StackTest, Sorted) { // sk_*_find should return the first matching element in all cases. TEST(StackTest, FindFirst) { bssl::UniquePtr sk(sk_TEST_INT_new(compare)); + ASSERT_TRUE(sk); auto value = TEST_INT_new(1); ASSERT_TRUE(value); ASSERT_TRUE(bssl::PushToStack(sk.get(), std::move(value))); @@ -397,6 +398,7 @@ TEST(StackTest, BinarySearch) { TEST(StackTest, DeleteIf) { bssl::UniquePtr sk(sk_TEST_INT_new(compare)); + ASSERT_TRUE(sk); for (int v : {1, 9, 2, 8, 3, 7, 4, 6, 5}) { auto obj = TEST_INT_new(v); ASSERT_TRUE(obj); diff --git a/crypto/x509/x509_cmp.c b/crypto/x509/x509_cmp.c index 7e3c39552..b640413cd 100644 --- a/crypto/x509/x509_cmp.c +++ b/crypto/x509/x509_cmp.c @@ -284,10 +284,12 @@ int X509_check_private_key(X509 *x, const EVP_PKEY *k) { // count but it has the same effect by duping the STACK and upping the ref of // each X509 structure. STACK_OF(X509) *X509_chain_up_ref(STACK_OF(X509) *chain) { - STACK_OF(X509) *ret; - size_t i; - ret = sk_X509_dup(chain); - for (i = 0; i < sk_X509_num(ret); i++) { + STACK_OF(X509) *ret = sk_X509_dup(chain); + if (ret == NULL) { + OPENSSL_PUT_ERROR(X509, ERR_R_MALLOC_FAILURE); + return NULL; + } + for (size_t i = 0; i < sk_X509_num(ret); i++) { X509_up_ref(sk_X509_value(ret, i)); } return ret; diff --git a/crypto/x509/x509_test.cc b/crypto/x509/x509_test.cc index e152f3d1a..4f931b3e7 100644 --- a/crypto/x509/x509_test.cc +++ b/crypto/x509/x509_test.cc @@ -1391,6 +1391,7 @@ TEST(X509Test, ZeroLengthsWithX509PARAM) { TEST(X509Test, ZeroLengthsWithCheckFunctions) { bssl::UniquePtr leaf(CertFromPEM(kSANTypesLeaf)); + ASSERT_TRUE(leaf); EXPECT_EQ( 1, X509_check_host(leaf.get(), kHostname, strlen(kHostname), 0, nullptr)); @@ -2467,7 +2468,9 @@ TEST(X509Test, TestPrintUTCTIME) { for (auto t : asn1_utctime_tests) { SCOPED_TRACE(t.val); bssl::UniquePtr tm(ASN1_UTCTIME_new()); + ASSERT_TRUE(tm); bssl::UniquePtr bio(BIO_new(BIO_s_mem())); + ASSERT_TRUE(bio); // Use this instead of ASN1_UTCTIME_set() because some callers get // type-confused and pass ASN1_GENERALIZEDTIME to ASN1_UTCTIME_print(). @@ -2525,6 +2528,7 @@ TEST(X509Test, PrettyPrintIntegers) { TEST(X509Test, X509NameSet) { bssl::UniquePtr name(X509_NAME_new()); + ASSERT_TRUE(name); EXPECT_TRUE(X509_NAME_add_entry_by_txt( name.get(), "C", MBSTRING_ASC, reinterpret_cast("US"), -1, -1, 0)); diff --git a/ssl/handoff.cc b/ssl/handoff.cc index b885c4c02..39f0bacf7 100644 --- a/ssl/handoff.cc +++ b/ssl/handoff.cc @@ -124,6 +124,9 @@ static bool apply_remote_features(SSL *ssl, CBS *in) { return false; } bssl::UniquePtr supported(sk_SSL_CIPHER_new_null()); + if (!supported) { + return false; + } while (CBS_len(&ciphers)) { uint16_t id; if (!CBS_get_u16(&ciphers, &id)) { @@ -141,6 +144,9 @@ static bool apply_remote_features(SSL *ssl, CBS *in) { ssl->config->cipher_list ? ssl->config->cipher_list->ciphers.get() : ssl->ctx->cipher_list->ciphers.get(); bssl::UniquePtr unsupported(sk_SSL_CIPHER_new_null()); + if (!unsupported) { + return false; + } for (const SSL_CIPHER *configured_cipher : configured) { if (sk_SSL_CIPHER_find(supported.get(), nullptr, configured_cipher)) { continue; @@ -151,7 +157,8 @@ static bool apply_remote_features(SSL *ssl, CBS *in) { } if (sk_SSL_CIPHER_num(unsupported.get()) && !ssl->config->cipher_list) { ssl->config->cipher_list = bssl::MakeUnique(); - if (!ssl->config->cipher_list->Init(*ssl->ctx->cipher_list)) { + if (!ssl->config->cipher_list || + !ssl->config->cipher_list->Init(*ssl->ctx->cipher_list)) { return false; } } @@ -488,6 +495,9 @@ bool SSL_apply_handback(SSL *ssl, Span handback) { } s3->hs = ssl_handshake_new(ssl); + if (!s3->hs) { + return false; + } SSL_HANDSHAKE *const hs = s3->hs.get(); if (!session_reused || type == handback_tls13) { hs->new_session = diff --git a/ssl/ssl_session.cc b/ssl/ssl_session.cc index 5b61ebad4..885e27d36 100644 --- a/ssl/ssl_session.cc +++ b/ssl/ssl_session.cc @@ -221,6 +221,7 @@ UniquePtr SSL_SESSION_dup(SSL_SESSION *session, int dup_flags) { new_session->certs.reset(sk_CRYPTO_BUFFER_deep_copy( session->certs.get(), buf_up_ref, CRYPTO_BUFFER_free)); if (new_session->certs == nullptr) { + OPENSSL_PUT_ERROR(SSL, ERR_R_MALLOC_FAILURE); return nullptr; } } diff --git a/ssl/ssl_test.cc b/ssl/ssl_test.cc index 17209298f..f51c11efc 100644 --- a/ssl/ssl_test.cc +++ b/ssl/ssl_test.cc @@ -1100,6 +1100,12 @@ static bool GetClientHello(SSL *ssl, std::vector *out) { if (!BIO_mem_contents(bio.get(), &client_hello, &client_hello_len)) { return false; } + + // We did not get far enough to write a ClientHello. + if (client_hello_len == 0) { + return false; + } + *out = std::vector(client_hello, client_hello + client_hello_len); return true; } @@ -1974,6 +1980,7 @@ TEST(SSLTest, UnsupportedECHConfig) { TEST(SSLTest, ECHClientRandomsMatch) { bssl::UniquePtr server_ctx = CreateContextWithTestCertificate(TLS_method()); + ASSERT_TRUE(server_ctx); bssl::UniquePtr keys = MakeTestECHKeys(); ASSERT_TRUE(keys); ASSERT_TRUE(SSL_CTX_set1_ech_keys(server_ctx.get(), keys.get())); @@ -2342,6 +2349,7 @@ TEST(SSLTest, ECHThreads) { bssl::UniquePtr server_ctx = CreateContextWithTestCertificate(TLS_method()); + ASSERT_TRUE(server_ctx); ASSERT_TRUE(SSL_CTX_set1_ech_keys(server_ctx.get(), keys1.get())); bssl::UniquePtr client_ctx(SSL_CTX_new(TLS_method())); @@ -3249,7 +3257,7 @@ static void ExpectSessionReused(SSL_CTX *client_ctx, SSL_CTX *server_ctx, bssl::UniquePtr client, server; ClientConfig config; config.session = session; - EXPECT_TRUE( + ASSERT_TRUE( ConnectClientAndServer(&client, &server, client_ctx, server_ctx, config)); EXPECT_EQ(SSL_session_reused(client.get()), SSL_session_reused(server.get())); @@ -3454,7 +3462,7 @@ TEST_P(SSLVersionTest, SessionTimeout) { for (bool server_test : {false, true}) { SCOPED_TRACE(server_test); - ResetContexts(); + ASSERT_NO_FATAL_FAILURE(ResetContexts()); SSL_CTX_set_session_cache_mode(client_ctx_.get(), SSL_SESS_CACHE_BOTH); SSL_CTX_set_session_cache_mode(server_ctx_.get(), SSL_SESS_CACHE_BOTH); @@ -4170,7 +4178,7 @@ TEST_P(SSLVersionTest, SSLWriteRetry) { TEST_P(SSLVersionTest, RecordCallback) { for (bool test_server : {true, false}) { SCOPED_TRACE(test_server); - ResetContexts(); + ASSERT_NO_FATAL_FAILURE(ResetContexts()); bool read_seen = false; bool write_seen = false; @@ -4735,6 +4743,7 @@ static void ConnectClientAndServerWithTicketMethod( state->retry_count = retry_count; state->failure_mode = failure_mode; + ASSERT_GE(ssl_test_ticket_aead_get_ex_index(), 0); ASSERT_TRUE(SSL_set_ex_data(server.get(), ssl_test_ticket_aead_get_ex_index(), state)); @@ -4788,9 +4797,9 @@ TEST_P(TicketAEADMethodTest, Resume) { SSL_CTX_set_ticket_aead_method(server_ctx.get(), &kSSLTestTicketMethod); bssl::UniquePtr client, server; - ConnectClientAndServerWithTicketMethod(&client, &server, client_ctx.get(), - server_ctx.get(), retry_count, - failure_mode, nullptr); + ASSERT_NO_FATAL_FAILURE(ConnectClientAndServerWithTicketMethod( + &client, &server, client_ctx.get(), server_ctx.get(), retry_count, + failure_mode, nullptr)); switch (failure_mode) { case ssl_test_ticket_aead_ok: case ssl_test_ticket_aead_open_hard_fail: @@ -4806,9 +4815,9 @@ TEST_P(TicketAEADMethodTest, Resume) { ASSERT_TRUE(FlushNewSessionTickets(client.get(), server.get())); bssl::UniquePtr session = std::move(g_last_session); - ConnectClientAndServerWithTicketMethod(&client, &server, client_ctx.get(), - server_ctx.get(), retry_count, - failure_mode, session.get()); + ASSERT_NO_FATAL_FAILURE(ConnectClientAndServerWithTicketMethod( + &client, &server, client_ctx.get(), server_ctx.get(), retry_count, + failure_mode, session.get())); switch (failure_mode) { case ssl_test_ticket_aead_ok: ASSERT_TRUE(client); @@ -5190,6 +5199,7 @@ TEST(SSLTest, Handoff) { ASSERT_TRUE(CBBFinishArray(cbb.get(), &handoff)); bssl::UniquePtr handshaker(SSL_new(handshaker_ctx.get())); + ASSERT_TRUE(handshaker); // Note split handshakes determines 0-RTT support, for both the current // handshake and newly-issued tickets, entirely by |handshaker|. There is // no need to call |SSL_set_early_data_enabled| on |server|. @@ -5215,6 +5225,7 @@ TEST(SSLTest, Handoff) { ASSERT_TRUE(CBBFinishArray(cbb_handback.get(), &handback)); bssl::UniquePtr server2(SSL_new(server_ctx.get())); + ASSERT_TRUE(server2); ASSERT_TRUE(SSL_apply_handback(server2.get(), handback)); MoveBIOs(server2.get(), handshaker.get()); @@ -5342,6 +5353,7 @@ TEST(SSLTest, SigAlgs) { }; UniquePtr ctx(SSL_CTX_new(TLS_method())); + ASSERT_TRUE(ctx); unsigned n = 1; for (const auto &test : kTests) { @@ -5397,6 +5409,7 @@ TEST(SSLTest, SigAlgsList) { }; UniquePtr ctx(SSL_CTX_new(TLS_method())); + ASSERT_TRUE(ctx); unsigned n = 1; for (const auto &test : kTests) { @@ -5422,7 +5435,9 @@ TEST(SSLTest, SigAlgsList) { TEST(SSLTest, ApplyHandoffRemovesUnsupportedCiphers) { bssl::UniquePtr server_ctx(SSL_CTX_new(TLS_method())); + ASSERT_TRUE(server_ctx); bssl::UniquePtr server(SSL_new(server_ctx.get())); + ASSERT_TRUE(server); // handoff is a handoff message that has been artificially modified to pretend // that only cipher 0x0A is supported. When it is applied to |server|, all @@ -5460,7 +5475,9 @@ TEST(SSLTest, ApplyHandoffRemovesUnsupportedCiphers) { TEST(SSLTest, ApplyHandoffRemovesUnsupportedCurves) { bssl::UniquePtr server_ctx(SSL_CTX_new(TLS_method())); + ASSERT_TRUE(server_ctx); bssl::UniquePtr server(SSL_new(server_ctx.get())); + ASSERT_TRUE(server); // handoff is a handoff message that has been artificially modified to pretend // that only one curve is supported. When it is applied to |server|, all @@ -5500,10 +5517,12 @@ TEST(SSLTest, ZeroSizedWiteFlushesHandshakeMessages) { // flush them. bssl::UniquePtr server_ctx( CreateContextWithTestCertificate(TLS_method())); + ASSERT_TRUE(server_ctx); EXPECT_TRUE(SSL_CTX_set_max_proto_version(server_ctx.get(), TLS1_3_VERSION)); EXPECT_TRUE(SSL_CTX_set_min_proto_version(server_ctx.get(), TLS1_3_VERSION)); bssl::UniquePtr client_ctx(SSL_CTX_new(TLS_method())); + ASSERT_TRUE(client_ctx); EXPECT_TRUE(SSL_CTX_set_max_proto_version(client_ctx.get(), TLS1_3_VERSION)); EXPECT_TRUE(SSL_CTX_set_min_proto_version(client_ctx.get(), TLS1_3_VERSION)); @@ -5584,7 +5603,7 @@ TEST_P(SSLVersionTest, SessionCacheThreads) { ClientConfig config; config.session = session; UniquePtr client, server; - EXPECT_TRUE(ConnectClientAndServer(&client, &server, client_ctx_.get(), + ASSERT_TRUE(ConnectClientAndServer(&client, &server, client_ctx_.get(), server_ctx_.get(), config)); }; @@ -5677,7 +5696,7 @@ TEST_P(SSLVersionTest, SessionCacheThreads) { TEST_P(SSLVersionTest, SessionTicketThreads) { for (bool renew_ticket : {false, true}) { SCOPED_TRACE(renew_ticket); - ResetContexts(); + ASSERT_NO_FATAL_FAILURE(ResetContexts()); SSL_CTX_set_session_cache_mode(client_ctx_.get(), SSL_SESS_CACHE_BOTH); SSL_CTX_set_session_cache_mode(server_ctx_.get(), SSL_SESS_CACHE_BOTH); if (renew_ticket) { @@ -5696,7 +5715,7 @@ TEST_P(SSLVersionTest, SessionTicketThreads) { ClientConfig config; config.session = session; UniquePtr client, server; - EXPECT_TRUE(ConnectClientAndServer(&client, &server, client_ctx_.get(), + ASSERT_TRUE(ConnectClientAndServer(&client, &server, client_ctx_.get(), server_ctx_.get(), config)); }; @@ -5733,6 +5752,8 @@ TEST(SSLTest, GetCertificateThreads) { X509 *cert2 = SSL_CTX_get0_certificate(ctx.get()); thread.join(); + ASSERT_TRUE(cert2); + ASSERT_TRUE(cert2_thread); EXPECT_EQ(cert2, cert2_thread); EXPECT_EQ(0, X509_cmp(cert.get(), cert2)); } @@ -6105,8 +6126,10 @@ class QUICMethodTest : public testing::Test { SSL_set_accept_state(server_.get()); transport_.reset(new MockQUICTransportPair); - ex_data_.Set(client_.get(), transport_->client()); - ex_data_.Set(server_.get(), transport_->server()); + if (!ex_data_.Set(client_.get(), transport_->client()) || + !ex_data_.Set(server_.get(), transport_->server())) { + return false; + } if (allow_out_of_order_writes_) { transport_->client()->AllowOutOfOrderWrites(); transport_->server()->AllowOutOfOrderWrites(); @@ -6494,7 +6517,7 @@ TEST_F(QUICMethodTest, ZeroRTTRejectMismatchedParameters) { // The server will consume the ClientHello, but it will not accept 0-RTT. ASSERT_TRUE(ProvideHandshakeData(server_.get())); ASSERT_EQ(SSL_do_handshake(server_.get()), -1); - EXPECT_EQ(SSL_ERROR_WANT_READ, SSL_get_error(server_.get(), -1)); + ASSERT_EQ(SSL_ERROR_WANT_READ, SSL_get_error(server_.get(), -1)); EXPECT_FALSE(SSL_in_early_data(server_.get())); EXPECT_FALSE(transport_->server()->HasReadSecret(ssl_encryption_early_data)); @@ -6579,7 +6602,7 @@ TEST_F(QUICMethodTest, ZeroRTTReject) { // The server will consume the ClientHello, but it will not accept 0-RTT. ASSERT_TRUE(ProvideHandshakeData(server_.get())); ASSERT_EQ(SSL_do_handshake(server_.get()), -1); - EXPECT_EQ(SSL_ERROR_WANT_READ, SSL_get_error(server_.get(), -1)); + ASSERT_EQ(SSL_ERROR_WANT_READ, SSL_get_error(server_.get(), -1)); EXPECT_FALSE(SSL_in_early_data(server_.get())); EXPECT_FALSE( transport_->server()->HasReadSecret(ssl_encryption_early_data)); @@ -6747,8 +6770,8 @@ TEST_F(QUICMethodTest, Buffered) { ASSERT_TRUE(CreateClientAndServer()); BufferedFlight client_flight, server_flight; - buffered_flights.Set(client_.get(), &client_flight); - buffered_flights.Set(server_.get(), &server_flight); + ASSERT_TRUE(buffered_flights.Set(client_.get(), &client_flight)); + ASSERT_TRUE(buffered_flights.Set(server_.get(), &server_flight)); ASSERT_TRUE(CompleteHandshakesForQUIC()); @@ -6969,10 +6992,10 @@ TEST_F(QUICMethodTest, ForbidCrossProtocolResumptionClient) { EXPECT_FALSE(g_last_session); ASSERT_TRUE(ProvideHandshakeData(client_.get())); EXPECT_EQ(SSL_process_quic_post_handshake(client_.get()), 1); - EXPECT_TRUE(g_last_session); + ASSERT_TRUE(g_last_session); // Pretend that g_last_session came from a TLS-over-TCP connection. - g_last_session.get()->is_quic = false; + g_last_session->is_quic = false; // Create a second connection and verify that resumption does not occur with // a session from a non-QUIC connection. This tests that the client does not @@ -7010,7 +7033,7 @@ TEST_F(QUICMethodTest, ForbidCrossProtocolResumptionServer) { EXPECT_FALSE(g_last_session); ASSERT_TRUE(ProvideHandshakeData(client_.get())); EXPECT_EQ(SSL_process_quic_post_handshake(client_.get()), 1); - EXPECT_TRUE(g_last_session); + ASSERT_TRUE(g_last_session); // Attempt a resumption with g_last_session using TLS_method. bssl::UniquePtr client_ctx(SSL_CTX_new(TLS_method())); @@ -7027,7 +7050,7 @@ TEST_F(QUICMethodTest, ForbidCrossProtocolResumptionServer) { // The TLS-over-TCP client will refuse to resume with a quic session, so // mark is_quic = false to bypass the client check to test the server check. - g_last_session.get()->is_quic = false; + g_last_session->is_quic = false; SSL_set_session(client.get(), g_last_session.get()); BIO *bio1, *bio2; @@ -7386,7 +7409,7 @@ TEST_P(SSLVersionTest, TicketSessionIDsMatch) { bssl::UniquePtr client, server; ClientConfig config; config.session = session.get(); - EXPECT_TRUE(ConnectClientAndServer(&client, &server, client_ctx_.get(), + ASSERT_TRUE(ConnectClientAndServer(&client, &server, client_ctx_.get(), server_ctx_.get(), config)); EXPECT_TRUE(SSL_session_reused(client.get())); EXPECT_TRUE(SSL_session_reused(server.get())); @@ -7770,7 +7793,7 @@ TEST(SSLTest, ALPNConfig) { auto check_alpn_proto = [&](Span expected) { observed_alpn.clear(); bssl::UniquePtr client, server; - EXPECT_TRUE(ConnectClientAndServer(&client, &server, ctx.get(), ctx.get())); + ASSERT_TRUE(ConnectClientAndServer(&client, &server, ctx.get(), ctx.get())); EXPECT_EQ(Bytes(expected), Bytes(observed_alpn)); }; @@ -8411,7 +8434,7 @@ TEST(SSLTest, ErrorSyscallAfterCloseNotify) { ASSERT_TRUE(client_ctx); ASSERT_TRUE(server_ctx); bssl::UniquePtr client, server; - EXPECT_TRUE(ConnectClientAndServer(&client, &server, client_ctx.get(), + ASSERT_TRUE(ConnectClientAndServer(&client, &server, client_ctx.get(), server_ctx.get())); // Replace the write |BIO| with |wbio_silent_error|. @@ -8477,7 +8500,7 @@ TEST(SSLTest, QuietShutdown) { ASSERT_TRUE(server_ctx); SSL_CTX_set_quiet_shutdown(server_ctx.get(), 1); bssl::UniquePtr client, server; - EXPECT_TRUE(ConnectClientAndServer(&client, &server, client_ctx.get(), + ASSERT_TRUE(ConnectClientAndServer(&client, &server, client_ctx.get(), server_ctx.get())); // Quiet shutdown is enabled, so |SSL_shutdown| on the server should diff --git a/ssl/tls13_enc.cc b/ssl/tls13_enc.cc index ad023ef8e..23889bd1c 100644 --- a/ssl/tls13_enc.cc +++ b/ssl/tls13_enc.cc @@ -111,6 +111,7 @@ static bool hkdf_expand_label(Span out, const EVP_MD *digest, !CBB_add_u8_length_prefixed(cbb.get(), &child) || !CBB_add_bytes(&child, hash.data(), hash.size()) || !CBBFinishArray(cbb.get(), &hkdf_label)) { + OPENSSL_PUT_ERROR(SSL, ERR_R_MALLOC_FAILURE); return false; } From 8bc06cf491243eb95afebc80020c958a16e269a7 Mon Sep 17 00:00:00 2001 From: David Benjamin Date: Sat, 4 Feb 2023 20:05:04 -0500 Subject: [PATCH 10/11] Clean up test_support_lib and GTest dependencies slightly. test_support_lib depends on GTest and should be marked as such. Historically it was a bit fuzzy, but now it's unambiguous. With that cleaned up, we can remove one of the global include_directories calls and rely on CMake's INTERFACE_INCLUDE_DIRECTORIES machinery. (CMake's documentation and "modern CMake" prefers setting include directories on the target and letting them flow up the dependency tree, rather than configuring it globally across the project.) Change-Id: I364df834d62328b69f146fbe35c10af97618a713 Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/56567 Reviewed-by: Bob Beck Commit-Queue: David Benjamin --- CMakeLists.txt | 8 +++++--- crypto/test/CMakeLists.txt | 2 ++ 2 files changed, 7 insertions(+), 3 deletions(-) diff --git a/CMakeLists.txt b/CMakeLists.txt index e0808d703..1a1a0080b 100644 --- a/CMakeLists.txt +++ b/CMakeLists.txt @@ -512,9 +512,11 @@ endif() # Add minimal googletest targets. The provided one has many side-effects, and # googletest has a very straightforward build. add_library(boringssl_gtest third_party/googletest/src/gtest-all.cc) -target_include_directories(boringssl_gtest PRIVATE third_party/googletest) - -include_directories(third_party/googletest/include) +target_include_directories( + boringssl_gtest + PUBLIC third_party/googletest/include + PRIVATE third_party/googletest +) # Declare a dummy target to build all unit tests. Test targets should inject # themselves as dependencies next to the target definition. diff --git a/crypto/test/CMakeLists.txt b/crypto/test/CMakeLists.txt index bf2086376..939cbb693 100644 --- a/crypto/test/CMakeLists.txt +++ b/crypto/test/CMakeLists.txt @@ -17,6 +17,7 @@ endif() if(WIN32) target_link_libraries(test_support_lib dbghelp) endif() +target_link_libraries(test_support_lib boringssl_gtest crypto) add_dependencies(test_support_lib global_target) add_library( @@ -28,3 +29,4 @@ add_library( ) add_dependencies(boringssl_gtest_main global_target) +target_link_libraries(boringssl_gtest_main boringssl_gtest crypto test_support_lib) From 61266e464b9b509a8a0943b9cc826c97c31e04e7 Mon Sep 17 00:00:00 2001 From: David Benjamin Date: Sun, 29 Jan 2023 15:53:30 -0500 Subject: [PATCH 11/11] Limit the CMake -isysroot assembly workaround to older CMake It was fixed in CMake 3.19 with https://gitlab.kitware.com/cmake/cmake/-/issues/20771 Change-Id: Ia76ab6690e233bc650e11a79db381c00f21c83a1 Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/56568 Commit-Queue: David Benjamin Reviewed-by: Bob Beck --- CMakeLists.txt | 5 +++-- util/generate_build_files.py | 5 +++-- 2 files changed, 6 insertions(+), 4 deletions(-) diff --git a/CMakeLists.txt b/CMakeLists.txt index 1a1a0080b..1cb72269f 100644 --- a/CMakeLists.txt +++ b/CMakeLists.txt @@ -420,8 +420,9 @@ if(FIPS_DELOCATE OR NOT OPENSSL_NO_ASM) else() enable_language(ASM) set(OPENSSL_ASM TRUE) - # CMake does not add -isysroot and -arch flags to assembly. - if(APPLE) + # Work around https://gitlab.kitware.com/cmake/cmake/-/issues/20771 in older + # CMake versions. + if(APPLE AND CMAKE_VERSION VERSION_LESS 3.19) if(CMAKE_OSX_SYSROOT) set(CMAKE_ASM_FLAGS "${CMAKE_ASM_FLAGS} -isysroot \"${CMAKE_OSX_SYSROOT}\"") endif() diff --git a/util/generate_build_files.py b/util/generate_build_files.py index ca173c8ba..c221f0478 100644 --- a/util/generate_build_files.py +++ b/util/generate_build_files.py @@ -457,8 +457,9 @@ else() else() enable_language(ASM) set(OPENSSL_ASM TRUE) - # CMake does not add -isysroot and -arch flags to assembly. - if(APPLE) + # Work around https://gitlab.kitware.com/cmake/cmake/-/issues/20771 in older + # CMake versions. + if(APPLE AND CMAKE_VERSION VERSION_LESS 3.19) if(CMAKE_OSX_SYSROOT) set(CMAKE_ASM_FLAGS "${CMAKE_ASM_FLAGS} -isysroot \"${CMAKE_OSX_SYSROOT}\"") endif()