From ddcccb65185cdf51b17433ad2a63abc7fedb1e2e Mon Sep 17 00:00:00 2001 From: Todd Short Date: Thu, 11 May 2017 15:48:10 -0400 Subject: [PATCH] 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 Reviewed-by: Rich Salz (Merged from https://github.com/openssl/openssl/pull/3453) --- 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 4a3f2a84a2..664b4ad1b5 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 c31f391c59..9951f04311 100644 --- a/test/secmemtest.c +++ b/test/secmemtest.c @@ -7,6 +7,7 @@ * https://www.openssl.org/source/license.html */ +#include #include #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 -- 2.34.1