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

Rich Salz rsalz at openssl.org
Mon May 22 16:39:24 UTC 2017


The branch OpenSSL_1_1_0-stable has been updated
       via  ddcccb65185cdf51b17433ad2a63abc7fedb1e2e (commit)
      from  0870b2cdaa65a30645bd0cc70f7ad6e30db7a2cf (commit)


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

    Fix infinite loops in secure memory allocation.
    
    Remove assertion when mmap() fails.
    Only give the 1<<31 limit test as an example.
    
    Fix the small arena test to just check for the symptom of the infinite
    loop (i.e. initialized set on failure), rather than the actual infinite
    loop. This avoids some valgrind errors.
    
    Backport of:
    PR #3512 commit fee423bb68869de02fceaceefbc847e98213574b
    PR #3510 commit a486561b691d6293a901b412172ca0c6d1ffc0dc
    PR #3455 commit c8e89d58a5d44b9dd657d6d13a5a10d1d4d30733
    PR #3449 commit 7031ddac94d0ae616d1b0670263a9265ce672cd2
    
    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: Andy Polyakov <appro at openssl.org>
    Reviewed-by: Rich Salz <rsalz at openssl.org>
    (Merged from https://github.com/openssl/openssl/pull/3453)

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

Summary of changes:
 crypto/mem_sec.c  | 16 ++++++++++----
 test/secmemtest.c | 62 +++++++++++++++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 74 insertions(+), 4 deletions(-)

diff --git a/crypto/mem_sec.c b/crypto/mem_sec.c
index 4a3f2a8..664b4ad 100644
--- a/crypto/mem_sec.c
+++ b/crypto/mem_sec.c
@@ -68,8 +68,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;
@@ -85,6 +89,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 */
@@ -336,7 +341,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;
 
@@ -414,7 +420,6 @@ static int sh_init(size_t size, int minsize)
             close(fd);
         }
     }
-    OPENSSL_assert(sh.map_result != MAP_FAILED);
     if (sh.map_result == MAP_FAILED)
         goto err;
     sh.arena = (char *)(sh.map_result + pgsize);
@@ -482,6 +487,9 @@ static char *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 c31f391..9951f04 100644
--- a/test/secmemtest.c
+++ b/test/secmemtest.c
@@ -7,6 +7,7 @@
  * https://www.openssl.org/source/license.html
  */
 
+#include <stdio.h>
 #include <openssl/crypto.h>
 
 #define perror_line()    perror_line1(__LINE__)
@@ -90,6 +91,67 @@ int main(int argc, char **argv)
         perror_line();
         return 1;
     }
+
+    fprintf(stderr, "Possible infinite loop: allocate more than available\n");
+    if (!CRYPTO_secure_malloc_init(32768, 16)) {
+        perror_line();
+        return 1;
+    }
+    if (OPENSSL_secure_malloc((size_t)-1) != NULL) {
+        perror_line();
+        return 1;
+    }
+    if (!CRYPTO_secure_malloc_done()) {
+        perror_line();
+        return 1;
+    }
+
+    /*
+     * If init fails, then initialized should be false, if not, this
+     * could cause an infinite loop secure_malloc, but we don't test it
+     */
+    if (!CRYPTO_secure_malloc_init(16, 16) &&
+        CRYPTO_secure_malloc_initialized()) {
+        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
+     * negative number. However, it requires minimum input values:
+     *
+     * CRYPTO_secure_malloc_init((size_t)1<<34, (size_t)1<<4);
+     *
+     * Which really only works on 64-bit systems, since it took 16 GB
+     * secure memory arena to trigger the problem. It naturally takes
+     * corresponding amount of available virtual and physical memory
+     * for test to be feasible/representative. Since we can't assume
+     * that every system is equipped with that much memory, the test
+     * remains disabled. If the reader of this comment really wants
+     * to make sure that infinite loop is fixed, they can enable the
+     * code below.
+     */
+# if 0
+    /*-
+     * On Linux and BSD this test has a chance to complete in minimal
+     * time and with minimum side effects, because mlock is likely to
+     * fail because of RLIMIT_MEMLOCK, which is customarily [much]
+     * smaller than 16GB. In other words Linux and BSD users can be
+     * limited by virtual space alone...
+     */
+    if (sizeof(size_t) > 4) {
+        fprintf(stderr, "Possible infinite loop: 1<<31 limit\n");
+        if (CRYPTO_secure_malloc_init((size_t)1<<34, (size_t)1<<4) == 0) {
+            perror_line();
+        } else if (!CRYPTO_secure_malloc_done()) {
+            perror_line();
+            return 1;
+        }
+    }
+# endif
+
     /* this can complete - it was not really secure */
     OPENSSL_secure_free(r);
 #else


More information about the openssl-commits mailing list