[openssl-commits] [openssl] master update

[Richard Levitte]

The branch master has been updated
       via  7031ddac94d0ae616d1b0670263a9265ce672cd2 (commit)
      from  b57f0c598bde43e147a886c9ffb0d6fdb3141d72 (commit)


- Log -----------------------------------------------------------------
commit 7031ddac94d0ae616d1b0670263a9265ce672cd2
Author: Todd Short <tshort at akamai.com>
Date:   Thu May 11 15:48:10 2017 -0400

    Fix infinite loops in secure memory allocation.
    
    Issue 1:
    
    sh.bittable_size is a size_t but i is and int, which can result in
    freelist == -1 if sh.bittable_size exceeds an int.
    
    This seems to result in an OPENSSL_assert due to invalid allocation
    size, so maybe that is "ok."
    
    Worse, if sh.bittable_size is exactly 1<<31, then this becomes an
    infinite loop (because 1<<31 is a negative int, so it can be shifted
    right forever and sticks at -1).
    
    Issue 2:
    
    CRYPTO_secure_malloc_init() sets secure_mem_initialized=1 even when
    sh_init() returns 0.
    
    If sh_init() fails, we end up with secure_mem_initialized=1 but
    sh.minsize=0. If you then call secure_malloc(), which then calls,
    sh_malloc(), this then enters an infite loop since 0 << anything will
    never be larger than size.
    
    Issue 3:
    
    That same sh_malloc loop will loop forever for a size greater
    than size_t/2 because i will proceed (assuming sh.minsize=16):
    i=16, 32, 64, ..., size_t/8, size_t/4, size_t/2, 0, 0, 0, 0, ....
    This sequence will never be larger than "size".
    
    Reviewed-by: Rich Salz <rsalz at openssl.org>
    Reviewed-by: Richard Levitte <levitte at openssl.org>
    (Merged from https://github.com/openssl/openssl/pull/3449)

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

Summary of changes:
 crypto/mem_sec.c  | 15 ++++++++++++---
 test/secmemtest.c | 21 +++++++++++++++++++++
 2 files changed, 33 insertions(+), 3 deletions(-)

diff --git a/crypto/mem_sec.c b/crypto/mem_sec.c
index 351dec4..774b696 100644
--- a/crypto/mem_sec.c
+++ b/crypto/mem_sec.c
@@ -73,8 +73,12 @@ int CRYPTO_secure_malloc_init(size_t size, int minsize)
         sec_malloc_lock = CRYPTO_THREAD_lock_new();
         if (sec_malloc_lock == NULL)
             return 0;
-        ret = sh_init(size, minsize);
-        secure_mem_initialized = 1;
+        if ((ret = sh_init(size, minsize)) != 0) {
+            secure_mem_initialized = 1;
+        } else {
+            CRYPTO_THREAD_lock_free(sec_malloc_lock);
+            sec_malloc_lock = NULL;
+        }
     }
 
     return ret;
@@ -90,6 +94,7 @@ int CRYPTO_secure_malloc_done()
         sh_done();
         secure_mem_initialized = 0;
         CRYPTO_THREAD_lock_free(sec_malloc_lock);
+        sec_malloc_lock = NULL;
         return 1;
     }
 #endif /* IMPLEMENTED */
@@ -341,7 +346,8 @@ static void sh_remove_from_list(char *ptr)
 
 static int sh_init(size_t size, int minsize)
 {
-    int i, ret;
+    int ret;
+    size_t i;
     size_t pgsize;
     size_t aligned;
 
@@ -498,6 +504,9 @@ static void *sh_malloc(size_t size)
     size_t i;
     char *chunk;
 
+    if (size > sh.arena_size)
+        return NULL;
+
     list = sh.freelist_size - 1;
     for (i = sh.minsize; i < size; i <<= 1)
         list--;
diff --git a/test/secmemtest.c b/test/secmemtest.c
index 3244d06..c92db50 100644
--- a/test/secmemtest.c
+++ b/test/secmemtest.c
@@ -61,6 +61,27 @@ static int test_sec_mem(void)
         || !TEST_true(CRYPTO_secure_malloc_done())
         || !TEST_false(CRYPTO_secure_malloc_initialized()))
         goto end;
+
+    TEST_info("Possible infinite loop: allocate more than available");
+    if (!TEST_true(CRYPTO_secure_malloc_init(32768, 16)))
+        goto end;
+    TEST_ptr_null(OPENSSL_secure_malloc((size_t)-1));
+    TEST_true(CRYPTO_secure_malloc_done());
+
+    TEST_info("Possible infinite loop: small arena");
+    if (!TEST_false(CRYPTO_secure_malloc_init(16, 16)))
+        goto end;
+    TEST_false(CRYPTO_secure_malloc_initialized());
+    TEST_ptr_null(OPENSSL_secure_malloc((size_t)-1));
+    TEST_true(CRYPTO_secure_malloc_done());
+
+    if (sizeof(size_t) > 4) {
+        TEST_info("Possible infinite loop: 1<<31 limit");
+        if (!TEST_true(CRYPTO_secure_malloc_init((size_t)1<<34, (size_t)1<<4) != 0))
+            goto end;
+        TEST_true(CRYPTO_secure_malloc_done());
+    }
+    
     /* this can complete - it was not really secure */
     testresult = 1;
  end: