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>
diff --git a/crypto/cipher_extra/e_aesgcmsiv.c b/crypto/cipher_extra/e_aesgcmsiv.c
index 8c6589d..e4c5bb3 100644
--- a/crypto/cipher_extra/e_aesgcmsiv.c
+++ b/crypto/cipher_extra/e_aesgcmsiv.c
@@ -64,8 +64,10 @@
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 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
diff --git a/crypto/mem.c b/crypto/mem.c
index 576ab7f..1c19122 100644
--- a/crypto/mem.c
+++ b/crypto/mem.c
@@ -76,32 +76,66 @@
#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 @@
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') {
diff --git a/include/openssl/mem.h b/include/openssl/mem.h
index 92cbb0d..6c21512 100644
--- a/include/openssl/mem.h
+++ b/include/openssl/mem.h
@@ -69,20 +69,28 @@
// 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.
diff --git a/ssl/ssl_buffer.cc b/ssl/ssl_buffer.cc
index d74278e..b424138 100644
--- a/ssl/ssl_buffer.cc
+++ b/ssl/ssl_buffer.cc
@@ -50,7 +50,11 @@
}
// 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 @@
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 clear_buffer(SSL3_BUFFER *buf) {
- OPENSSL_free(buf->buf);
+ free(buf->buf); // Allocated with malloc().
OPENSSL_memset(buf, 0, sizeof(SSL3_BUFFER));
}
diff --git a/ssl/ssl_cipher.cc b/ssl/ssl_cipher.cc
index e7a81a3..78cf60d 100644
--- a/ssl/ssl_cipher.cc
+++ b/ssl/ssl_cipher.cc
@@ -1337,11 +1337,15 @@
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) {
