FIPS-2022-11-02: thread-local using system allocator (#1401)

Author: justsmth

crypto/err/err.c

@@ -106,6 +106,8 @@
  * ([email protected]).  This product includes software written by Tim
  * Hudson ([email protected]). */
 
+// Ensure we can't call OPENSSL_malloc circularly.
+#define _BORINGSSL_PROHIBIT_OPENSSL_MALLOC
 #include <openssl/err.h>
 
 #include <assert.h>
@@ -115,7 +117,8 @@
 #define __STDC_FORMAT_MACROS
 #endif
 #include <inttypes.h>
-
+#include <limits.h>
+#include <stdarg.h>
 #include <string.h>
 
 #if defined(OPENSSL_WINDOWS)
@@ -134,8 +137,8 @@ OPENSSL_MSVC_PRAGMA(warning(pop))
 struct err_error_st {
   // file contains the filename where the error occurred.
   const char *file;
-  // data contains a NUL-terminated string with optional data. It must be freed
-  // with |OPENSSL_free|.
+  // data contains a NUL-terminated string with optional data. It is allocated
+  // with system |malloc| and must be freed with |free| (not |OPENSSL_free|)
   char *data;
   // packed contains the error library and reason, as packed by ERR_PACK.
   uint32_t packed;
@@ -167,20 +170,27 @@ extern const char kOpenSSLReasonStringData[];
 
 // err_clear clears the given queued error.
 static void err_clear(struct err_error_st *error) {
-  OPENSSL_free(error->data);
+  free(error->data);
   OPENSSL_memset(error, 0, sizeof(struct err_error_st));
 }
 
 static void err_copy(struct err_error_st *dst, const struct err_error_st *src) {
   err_clear(dst);
   dst->file = src->file;
   if (src->data != NULL) {
-    dst->data = OPENSSL_strdup(src->data);
+    // Disable deprecated functions on msvc so it doesn't complain about strdup.
+    OPENSSL_MSVC_PRAGMA(warning(push))
+    OPENSSL_MSVC_PRAGMA(warning(disable : 4996))
+    // We can't use OPENSSL_strdup because we don't want to call OPENSSL_malloc,
+    // which can affect the error stack.
+    dst->data = strdup(src->data);
+    OPENSSL_MSVC_PRAGMA(warning(pop))
   }
   dst->packed = src->packed;
   dst->line = src->line;
 }
 
+
 // global_next_library contains the next custom library value to return.
 static int global_next_library = ERR_NUM_LIBS;
 
@@ -199,15 +209,15 @@ static void err_state_free(void *statep) {
   for (unsigned i = 0; i < ERR_NUM_ERRORS; i++) {
     err_clear(&state->errors[i]);
   }
-  OPENSSL_free(state->to_free);
-  OPENSSL_free(state);
+  free(state->to_free);
+  free(state);
 }
 
 // err_get_state gets the ERR_STATE object for the current thread.
 static ERR_STATE *err_get_state(void) {
   ERR_STATE *state = CRYPTO_get_thread_local(OPENSSL_THREAD_LOCAL_ERR);
   if (state == NULL) {
-    state = OPENSSL_malloc(sizeof(ERR_STATE));
+    state = malloc(sizeof(ERR_STATE));
     if (state == NULL) {
       return NULL;
     }
@@ -263,7 +273,10 @@ static uint32_t get_error_values(int inc, int top, const char **file, int *line,
     } else {
       *data = error->data;
       if (flags != NULL) {
-        *flags = ERR_FLAG_STRING;
+        // Without |ERR_FLAG_MALLOCED|, rust-openssl assumes the string has a
+        // static lifetime. In both cases, we retain ownership of the string,
+        // and the caller is not expected to free it.
+        *flags = ERR_FLAG_STRING | ERR_FLAG_MALLOCED;
       }
       // If this error is being removed, take ownership of data from
       // the error. The semantics are such that the caller doesn't
@@ -272,7 +285,7 @@ static uint32_t get_error_values(int inc, int top, const char **file, int *line,
       // error queue.
       if (inc) {
         if (error->data != NULL) {
-          OPENSSL_free(state->to_free);
+          free(state->to_free);
           state->to_free = error->data;
         }
         error->data = NULL;
@@ -340,7 +353,7 @@ void ERR_clear_error(void) {
   for (i = 0; i < ERR_NUM_ERRORS; i++) {
     err_clear(&state->errors[i]);
   }
-  OPENSSL_free(state->to_free);
+  free(state->to_free);
   state->to_free = NULL;
 
   state->top = state->bottom = 0;
@@ -634,13 +647,13 @@ static void err_set_error_data(char *data) {
   struct err_error_st *error;
 
   if (state == NULL || state->top == state->bottom) {
-    OPENSSL_free(data);
+    free(data);
     return;
   }
 
   error = &state->errors[state->top];
 
-  OPENSSL_free(error->data);
+  free(error->data);
   error->data = data;
 }
 
@@ -677,48 +690,42 @@ void ERR_put_error(int library, int unused, int reason, const char *file,
 // concatenates them and sets the result as the data on the most recent
 // error.
 static void err_add_error_vdata(unsigned num, va_list args) {
-  size_t alloced, new_len, len = 0, substr_len;
-  char *buf;
+  size_t total_size = 0;
   const char *substr;
-  unsigned i;
+  char *buf;
 
-  alloced = 80;
-  buf = OPENSSL_malloc(alloced + 1);
-  if (buf == NULL) {
+  va_list args_copy;
+  va_copy(args_copy, args);
+  for (size_t i = 0; i < num; i++) {
+    substr = va_arg(args_copy, const char *);
+    if (substr == NULL) {
+      continue;
+    }
+    size_t substr_len = strlen(substr);
+    if (SIZE_MAX - total_size < substr_len) {
+      return; // Would overflow.
+    }
+    total_size += substr_len;
+  }
+  va_end(args_copy);
+  if (total_size == SIZE_MAX) {
+      return; // Would overflow.
+  }
+  total_size += 1; // NUL terminator.
+  if ((buf = malloc(total_size)) == NULL) {
     return;
   }
-
-  for (i = 0; i < num; i++) {
+  buf[0] = '\0';
+  for (size_t i = 0; i < num; i++) {
     substr = va_arg(args, const char *);
     if (substr == NULL) {
       continue;
     }
-
-    substr_len = strlen(substr);
-    new_len = len + substr_len;
-    if (new_len > alloced) {
-      char *new_buf;
-
-      if (alloced + 20 + 1 < alloced) {
-        // overflow.
-        OPENSSL_free(buf);
-        return;
-      }
-
-      alloced = new_len + 20;
-      new_buf = OPENSSL_realloc(buf, alloced + 1);
-      if (new_buf == NULL) {
-        OPENSSL_free(buf);
-        return;
-      }
-      buf = new_buf;
+    if (OPENSSL_strlcat(buf, substr, total_size) >= total_size) {
+      assert(0); // should not be possible.
     }
-
-    OPENSSL_memcpy(buf + len, substr, substr_len);
-    len = new_len;
   }
-
-  buf[len] = 0;
+  va_end(args);
   err_set_error_data(buf);
 }
 
@@ -730,21 +737,13 @@ void ERR_add_error_data(unsigned count, ...) {
 }
 
 void ERR_add_error_dataf(const char *format, ...) {
+  char *buf = NULL;
   va_list ap;
-  char *buf;
-  static const unsigned buf_len = 256;
 
-  // A fixed-size buffer is used because va_copy (which would be needed in
-  // order to call vsnprintf twice and measure the buffer) wasn't defined until
-  // C99.
-  buf = OPENSSL_malloc(buf_len + 1);
-  if (buf == NULL) {
+  va_start(ap, format);
+  if (OPENSSL_vasprintf_internal(&buf, format, ap, /*system_malloc=*/1) == -1) {
     return;
   }
-
-  va_start(ap, format);
-  BIO_vsnprintf(buf, buf_len, format, ap);
-  buf[buf_len] = 0;
   va_end(ap);
 
   err_set_error_data(buf);
@@ -756,13 +755,20 @@ void ERR_set_error_data(char *data, int flags) {
     assert(0);
     return;
   }
+  // Disable deprecated functions on msvc so it doesn't complain about strdup.
+  OPENSSL_MSVC_PRAGMA(warning(push))
+  OPENSSL_MSVC_PRAGMA(warning(disable : 4996))
+  // We can not use OPENSSL_strdup because we don't want to call OPENSSL_malloc,
+  // which can affect the error stack.
+  char *copy = strdup(data);
+  OPENSSL_MSVC_PRAGMA(warning(pop))
+  if (copy != NULL) {
+    err_set_error_data(copy);
+  }
   if (flags & ERR_FLAG_MALLOCED) {
-    err_set_error_data(data);
-  } else {
-    char *copy = OPENSSL_strdup(data);
-    if (copy != NULL) {
-      err_set_error_data(copy);
-    }
+    // We can not take ownership of |data| directly because it is allocated with
+    // |OPENSSL_malloc| and we will free it with system |free| later.
+    OPENSSL_free(data);
   }
 }
 
@@ -824,8 +830,8 @@ void ERR_SAVE_STATE_free(ERR_SAVE_STATE *state) {
   for (size_t i = 0; i < state->num_errors; i++) {
     err_clear(&state->errors[i]);
   }
-  OPENSSL_free(state->errors);
-  OPENSSL_free(state);
+  free(state->errors);
+  free(state);
 }
 
 ERR_SAVE_STATE *ERR_save_state(void) {
@@ -834,7 +840,7 @@ ERR_SAVE_STATE *ERR_save_state(void) {
     return NULL;
   }
 
-  ERR_SAVE_STATE *ret = OPENSSL_malloc(sizeof(ERR_SAVE_STATE));
+  ERR_SAVE_STATE *ret = malloc(sizeof(ERR_SAVE_STATE));
   if (ret == NULL) {
     return NULL;
   }
@@ -844,9 +850,9 @@ ERR_SAVE_STATE *ERR_save_state(void) {
                           ? state->top - state->bottom
                           : ERR_NUM_ERRORS + state->top - state->bottom;
   assert(num_errors < ERR_NUM_ERRORS);
-  ret->errors = OPENSSL_malloc(num_errors * sizeof(struct err_error_st));
+  ret->errors = malloc(num_errors * sizeof(struct err_error_st));
   if (ret->errors == NULL) {
-    OPENSSL_free(ret);
+    free(ret);
     return NULL;
   }
   OPENSSL_memset(ret->errors, 0, num_errors * sizeof(struct err_error_st));

crypto/err/err_test.cc

@@ -71,7 +71,7 @@ TEST(ErrTest, PutError) {
 
   EXPECT_STREQ("test", file);
   EXPECT_EQ(4, line);
-  EXPECT_TRUE(flags & ERR_FLAG_STRING);
+  EXPECT_EQ(flags, ERR_FLAG_STRING | ERR_FLAG_MALLOCED);
   EXPECT_EQ(1, ERR_GET_LIB(packed_error));
   EXPECT_EQ(2, ERR_GET_REASON(packed_error));
   EXPECT_STREQ("testing", data);
@@ -167,7 +167,7 @@ TEST(ErrTest, SaveAndRestore) {
   EXPECT_STREQ("test1.c", file);
   EXPECT_EQ(line, 1);
   EXPECT_STREQ(data, "data1");
-  EXPECT_EQ(flags, ERR_FLAG_STRING);
+  EXPECT_EQ(flags, ERR_FLAG_STRING | ERR_FLAG_MALLOCED);
 
   // The state may be restored, both over an empty and non-empty state.
   for (unsigned i = 0; i < 2; i++) {
@@ -180,7 +180,7 @@ TEST(ErrTest, SaveAndRestore) {
     EXPECT_STREQ("test1.c", file);
     EXPECT_EQ(line, 1);
     EXPECT_STREQ(data, "data1");
-    EXPECT_EQ(flags, ERR_FLAG_STRING);
+    EXPECT_EQ(flags, ERR_FLAG_STRING | ERR_FLAG_MALLOCED);
 
     packed_error = ERR_get_error_line_data(&file, &line, &data, &flags);
     EXPECT_EQ(ERR_GET_LIB(packed_error), 2);
@@ -196,7 +196,7 @@ TEST(ErrTest, SaveAndRestore) {
     EXPECT_STREQ("test3.c", file);
     EXPECT_EQ(line, 3);
     EXPECT_STREQ(data, "data3");
-    EXPECT_EQ(flags, ERR_FLAG_STRING);
+    EXPECT_EQ(flags, ERR_FLAG_STRING | ERR_FLAG_MALLOCED);
 
     // The error queue is now empty for the next iteration.
     EXPECT_EQ(0u, ERR_get_error());

crypto/fipsmodule/rand/rand.c

@@ -12,6 +12,8 @@
  * OF CONTRACT, NEGLIGENCE OR OTHER TORTIOUS ACTION, ARISING OUT OF OR IN
  * CONNECTION WITH THE USE OR PERFORMANCE OF THIS SOFTWARE. */
 
+// Ensure we can't call OPENSSL_malloc.
+#define _BORINGSSL_PROHIBIT_OPENSSL_MALLOC
 #include <openssl/rand.h>
 
 #include <assert.h>
@@ -259,9 +261,11 @@ static void rand_thread_state_free(void *state_in) {
   CRYPTO_STATIC_MUTEX_unlock_write(thread_states_list_lock_bss_get());
 
   rand_state_fips_clear(state);
+#else
+  OPENSSL_cleanse(state, sizeof(struct rand_thread_state));
 #endif
 
-  OPENSSL_free(state);
+  free(state);
 }
 
 #if defined(OPENSSL_X86_64) && !defined(OPENSSL_NO_ASM) && \
@@ -402,7 +406,7 @@ void RAND_bytes_with_additional_data(uint8_t *out, size_t out_len,
       CRYPTO_get_thread_local(OPENSSL_THREAD_LOCAL_RAND);
 
   if (state == NULL) {
-    state = OPENSSL_malloc(sizeof(struct rand_thread_state));
+    state = malloc(sizeof(struct rand_thread_state));
     if (state != NULL) {
       OPENSSL_memset(state, 0, sizeof(struct rand_thread_state));
     }

crypto/fipsmodule/self_check/fips.c

@@ -12,6 +12,8 @@
  * OF CONTRACT, NEGLIGENCE OR OTHER TORTIOUS ACTION, ARISING OUT OF OR IN
  * CONNECTION WITH THE USE OR PERFORMANCE OF THIS SOFTWARE. */
 
+// Ensure we can't call OPENSSL_malloc
+#define _BORINGSSL_PROHIBIT_OPENSSL_MALLOC
 #include <openssl/crypto.h>
 
 #include "../../internal.h"
@@ -97,14 +99,14 @@ void boringssl_fips_inc_counter(enum fips_counter_t counter) {
       CRYPTO_get_thread_local(OPENSSL_THREAD_LOCAL_FIPS_COUNTERS);
   if (!array) {
     const size_t num_bytes = sizeof(size_t) * (fips_counter_max + 1);
-    array = OPENSSL_malloc(num_bytes);
+    array = malloc(num_bytes);
     if (!array) {
       return;
     }
 
     OPENSSL_memset(array, 0, num_bytes);
     if (!CRYPTO_set_thread_local(OPENSSL_THREAD_LOCAL_FIPS_COUNTERS, array,
-                                 OPENSSL_free)) {
+                                 free)) {
       // |OPENSSL_free| has already been called by |CRYPTO_set_thread_local|.
       return;
     }