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 <[email protected]>
open-quantum-safe · Jul 29, 2024 · 841e903 · 841e903
1 parent 45972ea
commit 841e903
Show file tree

Hide file tree

Showing 6 changed files with 34 additions and 41 deletions.
diff --git 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
@@ -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
@@ -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
@@ -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
@@ -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
@@ -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);
 }