Commit 7031ddac authored by Todd Short's avatar Todd Short Committed by Richard Levitte
Browse files

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: default avatarRich Salz <rsalz@openssl.org>
Reviewed-by: default avatarRichard Levitte <levitte@openssl.org>
(Merged from https://github.com/openssl/openssl/pull/3449)
parent b57f0c59

crypto/mem_sec.c

+12 −3
Original line number Diff line number Diff line
@@ -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);

        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--;

test/secmemtest.c

+21 −0
Original line number Diff line number Diff line
@@ -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: