Zero memory in |OPENSSL_free|.

Allocations by |OPENSSL_malloc| are prefixed with their length.
|OPENSSL_free| zeros the allocation before calling free(), eliminating
the need for a separate call to |OPENSSL_cleanse| for sensitive data.

This change will be followed up by the cleanup in
https://boringssl-review.googlesource.com/c/boringssl/+/19824.

Change-Id: Ie272f07e9248d7d78af9aea81dacec0fdb7484c4
Reviewed-on: https://boringssl-review.googlesource.com/19544
Reviewed-by: Martin Kreichgauer <martinkr@google.com>
Commit-Queue: Martin Kreichgauer <martinkr@google.com>
CQ-Verified: CQ bot account: commit-bot@chromium.org <commit-bot@chromium.org>

crypto/cipher_extra/e_aesgcmsiv.c

@@ -64,8 +64,10 @@ static int aead_aes_gcm_siv_asm_init(EVP_AEAD_CTX *ctx, const uint8_t *key,
     return 0;
   }
 
+  // The asm implementation expects a 16-byte-aligned address here, so we use
+  // |malloc| rather than |OPENSSL_malloc|, which would add a length prefix.
   struct aead_aes_gcm_siv_asm_ctx *gcm_siv_ctx =
-      OPENSSL_malloc(sizeof(struct aead_aes_gcm_siv_asm_ctx));
+      malloc(sizeof(struct aead_aes_gcm_siv_asm_ctx));
   if (gcm_siv_ctx == NULL) {
     return 0;
   }
@@ -87,9 +89,7 @@ static int aead_aes_gcm_siv_asm_init(EVP_AEAD_CTX *ctx, const uint8_t *key,
 }
 
 static void aead_aes_gcm_siv_asm_cleanup(EVP_AEAD_CTX *ctx) {
-  struct aead_aes_gcm_siv_asm_ctx *gcm_siv_asm_ctx = ctx->aead_state;
-  OPENSSL_cleanse(gcm_siv_asm_ctx, sizeof(struct aead_aes_gcm_siv_asm_ctx));
-  OPENSSL_free(gcm_siv_asm_ctx);
+  free(ctx->aead_state);  // allocated with native |malloc|
 }
 
 // aesgcmsiv_polyval_horner updates the POLYVAL value in |in_out_poly| to

crypto/mem.c

@@ -76,32 +76,66 @@ OPENSSL_MSVC_PRAGMA(warning(pop))
 #include "internal.h"
 
 
-void *OPENSSL_realloc_clean(void *ptr, size_t old_size, size_t new_size) {
+#define OPENSSL_MALLOC_PREFIX 8
+
+
+void *OPENSSL_malloc(size_t size) {
+  void *ptr = malloc(size + OPENSSL_MALLOC_PREFIX);
   if (ptr == NULL) {
+    return NULL;
+  }
+
+  *(size_t *)ptr = size;
+
+  return ((uint8_t *)ptr) + OPENSSL_MALLOC_PREFIX;
+}
+
+void OPENSSL_free(void *orig_ptr) {
+  if (orig_ptr == NULL) {
+    return;
+  }
+
+  void *ptr = ((uint8_t *)orig_ptr) - OPENSSL_MALLOC_PREFIX;
+
+  size_t size = *(size_t *)ptr;
+  OPENSSL_cleanse(ptr, size + OPENSSL_MALLOC_PREFIX);
+  free(ptr);
+}
+
+void *OPENSSL_realloc(void *orig_ptr, size_t new_size) {
+  if (orig_ptr == NULL) {
     return OPENSSL_malloc(new_size);
   }
 
-  if (new_size == 0) {
-    return NULL;
-  }
-
-  // We don't support shrinking the buffer. Note the memcpy that copies
-  // |old_size| bytes to the new buffer, below.
-  if (new_size < old_size) {
-    return NULL;
-  }
+  void *ptr = ((uint8_t *)orig_ptr) - OPENSSL_MALLOC_PREFIX;
+  size_t old_size = *(size_t *)ptr;
 
   void *ret = OPENSSL_malloc(new_size);
   if (ret == NULL) {
     return NULL;
   }
 
-  OPENSSL_memcpy(ret, ptr, old_size);
-  OPENSSL_cleanse(ptr, old_size);
-  OPENSSL_free(ptr);
+  size_t to_copy = new_size;
+  if (old_size < to_copy) {
+    to_copy = old_size;
+  }
+
+  memcpy(ret, orig_ptr, to_copy);
+  OPENSSL_free(orig_ptr);
+
   return ret;
 }
 
+void *OPENSSL_realloc_clean(void *orig_ptr, size_t old_size, size_t new_size) {
+  void *ptr = ((uint8_t *)orig_ptr) - OPENSSL_MALLOC_PREFIX;
+  size_t actual_size = *(size_t *)ptr;
+  if (actual_size != old_size) {
+    return NULL;
+  }
+
+  return OPENSSL_realloc(orig_ptr, new_size);
+}
+
 void OPENSSL_cleanse(void *ptr, size_t len) {
 #if defined(OPENSSL_WINDOWS)
   SecureZeroMemory(ptr, len);
@@ -155,15 +189,15 @@ size_t OPENSSL_strnlen(const char *s, size_t len) {
   return len;
 }
 
-#if defined(OPENSSL_WINDOWS)
-
-char *OPENSSL_strdup(const char *s) { return _strdup(s); }
-
-#else
-
-char *OPENSSL_strdup(const char *s) { return strdup(s); }
-
-#endif
+char *OPENSSL_strdup(const char *s) {
+  const size_t len = strlen(s) + 1;
+  char *ret = OPENSSL_malloc(len);
+  if (ret == NULL) {
+    return NULL;
+  }
+  OPENSSL_memcpy(ret, s, len);
+  return ret;
+}
 
 int OPENSSL_tolower(int c) {
   if (c >= 'A' && c <= 'Z') {

include/openssl/mem.h

@@ -69,20 +69,28 @@ extern "C" {
 
 // Memory and string functions, see also buf.h.
 //
-// OpenSSL has, historically, had a complex set of malloc debugging options.
-// However, that was written in a time before Valgrind and ASAN. Since we now
-// have those tools, the OpenSSL allocation functions are simply macros around
-// the standard memory functions.
+// BoringSSL has its own set of allocation functions, which keep track of
+// allocation lengths and zero them out before freeing. All memory returned by
+// BoringSSL API calls must therefore generally be freed using |OPENSSL_free|
+// unless stated otherwise.
 
 
-#define OPENSSL_malloc malloc
-#define OPENSSL_realloc realloc
-#define OPENSSL_free free
+// OPENSSL_malloc acts like a regular |malloc|.
+OPENSSL_EXPORT void *OPENSSL_malloc(size_t size);
 
-// OPENSSL_realloc_clean acts like |realloc|, but clears the previous memory
-// buffer.  Because this is implemented as a wrapper around |malloc|, it needs
-// to be given the size of the buffer pointed to by |ptr|.
-void *OPENSSL_realloc_clean(void *ptr, size_t old_size, size_t new_size);
+// OPENSSL_free does nothing if |ptr| is NULL. Otherwise it zeros out the
+// memory allocated at |ptr| and frees it.
+OPENSSL_EXPORT void OPENSSL_free(void *ptr);
+
+// OPENSSL_realloc returns a pointer to a buffer of |new_size| bytes that
+// contains the contents of |ptr|. Unlike |realloc|, a new buffer is always
+// allocated and the data at |ptr| is always wiped and freed.
+OPENSSL_EXPORT void *OPENSSL_realloc(void *ptr, size_t new_size);
+
+// OPENSSL_realloc_clean behaves exactly like |OPENSSL_realloc|.
+// TODO(martinkr): Remove.
+OPENSSL_EXPORT void *OPENSSL_realloc_clean(void *ptr, size_t old_size,
+                                           size_t new_size);
 
 // OPENSSL_cleanse zeros out |len| bytes of memory at |ptr|. This is similar to
 // |memset_s| from C11.

ssl/ssl_buffer.cc

@@ -50,7 +50,11 @@ static int ensure_buffer(SSL3_BUFFER *buf, size_t header_len, size_t cap) {
   }
 
   // Add up to |SSL3_ALIGN_PAYLOAD| - 1 bytes of slack for alignment.
-  uint8_t *new_buf = (uint8_t *)OPENSSL_malloc(cap + SSL3_ALIGN_PAYLOAD - 1);
+  //
+  // Since this buffer gets allocated quite frequently and doesn't contain any
+  // sensitive data, we allocate with malloc rather than |OPENSSL_malloc| and
+  // avoid zeroing on free.
+  uint8_t *new_buf = (uint8_t *)malloc(cap + SSL3_ALIGN_PAYLOAD - 1);
   if (new_buf == NULL) {
     OPENSSL_PUT_ERROR(SSL, ERR_R_MALLOC_FAILURE);
     return 0;
@@ -62,7 +66,7 @@ static int ensure_buffer(SSL3_BUFFER *buf, size_t header_len, size_t cap) {
 
   if (buf->buf != NULL) {
     OPENSSL_memcpy(new_buf + new_offset, buf->buf + buf->offset, buf->len);
-    OPENSSL_free(buf->buf);
+    free(buf->buf);  // Allocated with malloc().
   }
 
   buf->buf = new_buf;
@@ -81,7 +85,7 @@ static void consume_buffer(SSL3_BUFFER *buf, size_t len) {
 }
 
 static void clear_buffer(SSL3_BUFFER *buf) {
-  OPENSSL_free(buf->buf);
+  free(buf->buf);  // Allocated with malloc().
   OPENSSL_memset(buf, 0, sizeof(SSL3_BUFFER));
 }
 

ssl/ssl_cipher.cc

@@ -1337,11 +1337,15 @@ int ssl_create_cipher_list(
     goto err;
   }
   pref_list->ciphers = cipherstack;
-  pref_list->in_group_flags = (uint8_t *)OPENSSL_malloc(num_in_group_flags);
-  if (!pref_list->in_group_flags) {
-    goto err;
+  pref_list->in_group_flags = NULL;
+  if (num_in_group_flags) {
+    pref_list->in_group_flags = (uint8_t *)OPENSSL_malloc(num_in_group_flags);
+    if (!pref_list->in_group_flags) {
+      goto err;
+    }
+    OPENSSL_memcpy(pref_list->in_group_flags, in_group_flags,
+                   num_in_group_flags);
   }
-  OPENSSL_memcpy(pref_list->in_group_flags, in_group_flags, num_in_group_flags);
   OPENSSL_free(in_group_flags);
   in_group_flags = NULL;
   if (*out_cipher_list != NULL) {