From 234ee3c4bc01d32c92eede6e093465e5992fd724 Mon Sep 17 00:00:00 2001 From: Spencer Wilson Date: Mon, 29 Jul 2024 11:01:03 -0400 Subject: [PATCH] Quick fixes from Trail of Bits audit Week 1 (#1869) * Remove unused variables from CI workflows * Add missing OpenSSL guards * Fix broken link and misplaced comment in common.c --------- Signed-off-by: Spencer Wilson Signed-off-by: rtjk <47841774+rtjk@users.noreply.github.com> --- .github/workflows/unix.yml | 2 +- .github/workflows/weekly.yml | 2 +- src/common/common.c | 4 ++-- src/common/common.h | 33 +++++++++++++++++++++++---------- src/common/rand/rand_nist.c | 28 ++++------------------------ src/common/sha2/sha2_ossl.c | 6 +++--- 6 files changed, 34 insertions(+), 41 deletions(-) diff --git a/.github/workflows/unix.yml b/.github/workflows/unix.yml index 0cfa13bd27..9bac98d3b4 100644 --- a/.github/workflows/unix.yml +++ b/.github/workflows/unix.yml @@ -285,7 +285,7 @@ jobs: run: ninja working-directory: build - name: Run tests - run: mkdir -p tmp && python3 -m pytest --verbose --ignore=tests/test_code_conventions.py --ignore=tests/test_kat_all.py ${{ matrix.PYTEST_ARGS }} + run: mkdir -p tmp && python3 -m pytest --verbose --ignore=tests/test_code_conventions.py --ignore=tests/test_kat_all.py timeout-minutes: 60 linux_openssl330-dev: diff --git a/.github/workflows/weekly.yml b/.github/workflows/weekly.yml index 1eb8ec1bff..d79fda6f15 100644 --- a/.github/workflows/weekly.yml +++ b/.github/workflows/weekly.yml @@ -64,4 +64,4 @@ jobs: working-directory: build - name: Run tests timeout-minutes: 360 - run: mkdir -p tmp && SKIP_ALGS='${{ matrix.SKIP_ALGS }}' python3 -m pytest --verbose ${{ matrix.PYTEST_ARGS }} + run: mkdir -p tmp && python3 -m pytest --verbose ${{ matrix.PYTEST_ARGS }} diff --git a/src/common/common.c b/src/common/common.c index 6688b9b75a..f0044fe9b7 100644 --- a/src/common/common.c +++ b/src/common/common.c @@ -39,7 +39,7 @@ static pthread_once_t once_control = PTHREAD_ONCE_INIT; #if defined(OQS_DIST_X86_64_BUILD) /* set_available_cpu_extensions_x86_64() has been written using: - * https://github.com/google/cpu_features/blob/master/src/cpuinfo_x86.c + * https://github.com/google/cpu_features/blob/339bfd32be1285877ff517cba8b82ce72e946afd/src/cpuinfo_x86.c */ #include "x86_64_helpers.h" static void set_available_cpu_extensions(void) { @@ -105,11 +105,11 @@ static unsigned int macos_feature_detection(const char *feature_name) { } } static void set_available_cpu_extensions(void) { - /* mark that this function has been called */ cpu_ext_data[OQS_CPU_EXT_ARM_AES] = 1; cpu_ext_data[OQS_CPU_EXT_ARM_SHA2] = 1; cpu_ext_data[OQS_CPU_EXT_ARM_SHA3] = macos_feature_detection("hw.optional.armv8_2_sha3"); cpu_ext_data[OQS_CPU_EXT_ARM_NEON] = macos_feature_detection("hw.optional.neon"); + /* mark that this function has been called */ cpu_ext_data[OQS_CPU_EXT_INIT] = 1; } #elif defined(__FreeBSD__) || defined(__FreeBSD) diff --git a/src/common/common.h b/src/common/common.h index b092baa036..18993d0a5f 100644 --- a/src/common/common.h +++ b/src/common/common.h @@ -24,12 +24,12 @@ extern "C" { * Macro for terminating the program if x is * a null pointer. */ -#define OQS_EXIT_IF_NULLPTR(x, loc) \ - do { \ - if ( (x) == (void*)0 ) { \ +#define OQS_EXIT_IF_NULLPTR(x, loc) \ + do { \ + if ( (x) == (void*)0 ) { \ fprintf(stderr, "Unexpected NULL returned from %s API. Exiting.\n", loc); \ - exit(EXIT_FAILURE); \ - } \ + exit(EXIT_FAILURE); \ + } \ } while (0) /** @@ -43,13 +43,26 @@ extern "C" { * This is a temporary workaround until a better error * handling strategy is developed. */ -#define OQS_OPENSSL_GUARD(x) \ - do { \ - if( 1 != (x) ) { \ +#ifdef OQS_USE_OPENSSL +#ifdef OPENSSL_NO_STDIO +#define OQS_OPENSSL_GUARD(x) \ + do { \ + if( 1 != (x) ) { \ fprintf(stderr, "Error return value from OpenSSL API: %d. Exiting.\n", x); \ - exit(EXIT_FAILURE); \ - } \ + exit(EXIT_FAILURE); \ + } \ } while (0) +#else // OPENSSL_NO_STDIO +#define OQS_OPENSSL_GUARD(x) \ + do { \ + if( 1 != (x) ) { \ + fprintf(stderr, "Error return value from OpenSSL API: %d. Exiting.\n", x); \ + OSSL_FUNC(ERR_print_errors_fp)(stderr); \ + exit(EXIT_FAILURE); \ + } \ + } while (0) +#endif // OPENSSL_NO_STDIO +#endif // OQS_USE_OPENSSL /** * Certain functions (such as OQS_randombytes_openssl in diff --git a/src/common/rand/rand_nist.c b/src/common/rand/rand_nist.c index 07db63b3bf..12407a08d6 100644 --- a/src/common/rand/rand_nist.c +++ b/src/common/rand/rand_nist.c @@ -32,20 +32,6 @@ void OQS_randombytes_nist_kat(unsigned char *x, size_t xlen); static OQS_NIST_DRBG_struct DRBG_ctx; static void AES256_CTR_DRBG_Update(unsigned char *provided_data, unsigned char *Key, unsigned char *V); -#ifdef OQS_USE_OPENSSL -# if defined(_MSC_VER) -__declspec(noreturn) -# else -__attribute__((noreturn)) -# endif -static void handleErrors(void) { -#ifndef OPENSSL_NO_STDIO - OSSL_FUNC(ERR_print_errors_fp)(stderr); -#endif - abort(); -} -#endif - // Use whatever AES implementation you have. This uses AES from openSSL library // key - 256-bit AES key // ctr - a 128-bit plaintext value @@ -57,17 +43,11 @@ static void AES256_ECB(unsigned char *key, unsigned char *ctr, unsigned char *bu int len; /* Create and initialise the context */ - if (!(ctx = OSSL_FUNC(EVP_CIPHER_CTX_new)())) { - handleErrors(); - } - - if (1 != OSSL_FUNC(EVP_EncryptInit_ex)(ctx, oqs_aes_256_ecb(), NULL, key, NULL)) { - handleErrors(); - } + ctx = OSSL_FUNC(EVP_CIPHER_CTX_new)(); + OQS_EXIT_IF_NULLPTR(ctx, "OpenSSL"); - if (1 != OSSL_FUNC(EVP_EncryptUpdate)(ctx, buffer, &len, ctr, 16)) { - handleErrors(); - } + OQS_OPENSSL_GUARD(OSSL_FUNC(EVP_EncryptInit_ex)(ctx, oqs_aes_256_ecb(), NULL, key, NULL)); + OQS_OPENSSL_GUARD(OSSL_FUNC(EVP_EncryptUpdate)(ctx, buffer, &len, ctr, 16)); /* Clean up */ OSSL_FUNC(EVP_CIPHER_CTX_free)(ctx); diff --git a/src/common/sha2/sha2_ossl.c b/src/common/sha2/sha2_ossl.c index 3aff58fab6..234d4b1e3b 100644 --- a/src/common/sha2/sha2_ossl.c +++ b/src/common/sha2/sha2_ossl.c @@ -18,9 +18,9 @@ static void do_hash(uint8_t *output, const uint8_t *input, size_t inplen, const unsigned int outlen; mdctx = OSSL_FUNC(EVP_MD_CTX_new)(); OQS_EXIT_IF_NULLPTR(mdctx, "OpenSSL"); - OSSL_FUNC(EVP_DigestInit_ex)(mdctx, md, NULL); - OSSL_FUNC(EVP_DigestUpdate)(mdctx, input, inplen); - OSSL_FUNC(EVP_DigestFinal_ex)(mdctx, output, &outlen); + OQS_OPENSSL_GUARD(OSSL_FUNC(EVP_DigestInit_ex)(mdctx, md, NULL)); + OQS_OPENSSL_GUARD(OSSL_FUNC(EVP_DigestUpdate)(mdctx, input, inplen)); + OQS_OPENSSL_GUARD(OSSL_FUNC(EVP_DigestFinal_ex)(mdctx, output, &outlen)); OSSL_FUNC(EVP_MD_CTX_free)(mdctx); }