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@openssl.org>
Reviewed-by: Rich Salz <rsalz@openssl.org>
(Merged from https://github.com/openssl/openssl/pull/3453)

diff --git a/crypto/mem_sec.c b/crypto/mem_sec.c
index 4a3f2a84a2175eca14b9b44f3e7f8a2073e53ddc..664b4ad1b561d62bead67d919b5d25b223f6c2cc 100644 (file)
--- 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 c31f391c59eefaf55bd572bf85cdc696e0de96b1..9951f04311cc69240a0b20319264119b2cb4a31b 100644 (file)
--- 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