[openssl-commits] [openssl] OpenSSL_1_1_0-stable update

[Paul I. Dale]

The branch OpenSSL_1_1_0-stable has been updated
       via  837017b4748d587912d9d218894644d6ca86721f (commit)
      from  8255fd0f4f86fa4202962d4b27185c0d96f21d75 (commit)


- Log -----------------------------------------------------------------
commit 837017b4748d587912d9d218894644d6ca86721f
Author: Pauli <paul.dale at oracle.com>
Date:   Wed Aug 22 10:04:27 2018 +1000

    Zero memory in CRYPTO_secure_malloc.
    
    This commit destroys the free list pointers which would otherwise be
    present in the returned memory blocks.  This in turn helps prevent
    information leakage from the secure memory area.
    
    Note: CRYPTO_secure_malloc is not guaranteed to return zeroed memory:
    before the secure memory system is initialised or if it isn't implemented.
    
    [manual merge of #7011]
    
    Reviewed-by: Matthias St. Pierre <Matthias.St.Pierre at ncp-e.com>
    (Merged from https://github.com/openssl/openssl/pull/7026)

-----------------------------------------------------------------------

Summary of changes:
 crypto/mem_sec.c  | 16 +++++++++++-----
 test/secmemtest.c | 44 ++++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 55 insertions(+), 5 deletions(-)

diff --git a/crypto/mem_sec.c b/crypto/mem_sec.c
index 25cdb47..1ccf68c 100644
--- a/crypto/mem_sec.c
+++ b/crypto/mem_sec.c
@@ -134,11 +134,12 @@ void *CRYPTO_secure_malloc(size_t num, const char *file, int line)
 
 void *CRYPTO_secure_zalloc(size_t num, const char *file, int line)
 {
-    void *ret = CRYPTO_secure_malloc(num, file, line);
-
-    if (ret != NULL)
-        memset(ret, 0, num);
-    return ret;
+#ifdef IMPLEMENTED
+    if (secure_mem_initialized)
+        /* CRYPTO_secure_malloc() zeroes allocations when it is implemented */
+        return CRYPTO_secure_malloc(num, file, line);
+#endif
+    return CRYPTO_zalloc(num, file, line);
 }
 
 void CRYPTO_secure_free(void *ptr, const char *file, int line)
@@ -574,6 +575,9 @@ static char *sh_malloc(size_t size)
 
     OPENSSL_assert(WITHIN_ARENA(chunk));
 
+    /* zero the free list header as a precaution against information leakage */
+    memset(chunk, 0, sizeof(SH_LIST));
+
     return chunk;
 }
 
@@ -606,6 +610,8 @@ static void sh_free(char *ptr)
 
         list--;
 
+        /* Zero the higher addressed block's free list pointers */
+        memset(ptr > buddy ? ptr : buddy, 0, sizeof(SH_LIST));
         if (ptr > buddy)
             ptr = buddy;
 
diff --git a/test/secmemtest.c b/test/secmemtest.c
index 9405f34..6077216 100644
--- a/test/secmemtest.c
+++ b/test/secmemtest.c
@@ -18,6 +18,8 @@ int main(int argc, char **argv)
 {
 #if defined(OPENSSL_SYS_LINUX) || defined(OPENSSL_SYS_UNIX)
     char *p = NULL, *q = NULL, *r = NULL, *s = NULL;
+    int i;
+    const int size = 64;
 
     s = OPENSSL_secure_malloc(20);
     /* s = non-secure 20 */
@@ -128,6 +130,48 @@ int main(int argc, char **argv)
         return 1;
     }
 
+    if (!CRYPTO_secure_malloc_init(32768, 16)) {
+        perror_line();
+        return 1;
+    }
+
+    /*
+     * Verify that secure memory gets zeroed properly.
+     */
+    if ((p = OPENSSL_secure_malloc(size)) == NULL) {
+        perror_line();
+        return 1;
+    }
+    for (i = 0; i < size; i++)
+        if (p[i] != 0) {
+            perror_line();
+            fprintf(stderr, "iteration %d\n", i);
+            return 1;
+        }
+
+    for (i = 0; i < size; i++)
+        p[i] = (unsigned char)(i + ' ' + 1);
+    OPENSSL_secure_free(p);
+
+    /*
+     * A deliberate use after free here to verify that the memory has been
+     * cleared properly.  Since secure free doesn't return the memory to
+     * libc's memory pool, it technically isn't freed.  However, the header
+     * bytes have to be skipped and these consist of two pointers in the
+     * current implementation.
+     */
+    for (i = sizeof(void *) * 2; i < size; i++)
+        if (p[i] != 0) {
+            perror_line();
+            fprintf(stderr, "iteration %d\n", i);
+            return 1;
+        }
+
+    if (!CRYPTO_secure_malloc_done()) {
+        perror_line();
+        return 1;
+    }
+
     /*-
      * There was also a possible infinite loop when the number of
      * elements was 1<<31, as |int i| was set to that, which is a