From e8408681b3fff91b794a1a5c65fd190019d6e9ee Mon Sep 17 00:00:00 2001 From: Todd Short Date: Mon, 11 Apr 2016 16:03:42 -0400 Subject: [PATCH] Secure memory fixes Fix some of the variables to be (s)size_t, so that more than 1GB of secure memory can be allocated. The arena has to be a power of 2, and 2GB fails because it ends up being a negative 32-bit signed number. The |too_late| flag is not strictly necessary; it is easy to figure out if something is secure memory by looking at the arena. As before, secure memory allocations will not fail, but now they can be freed correctly. Once initialized, secure memory can still be used, even if allocations occured before initialization. Reviewed-by: Richard Levitte Reviewed-by: Rich Salz --- crypto/mem_sec.c | 70 +++++++++++++------------- doc/crypto/OPENSSL_secure_malloc.pod | 30 +++++------- include/openssl/crypto.h | 2 +- test/secmemtest.c | 73 +++++++++++++++++++++++++--- 4 files changed, 116 insertions(+), 59 deletions(-) diff --git a/crypto/mem_sec.c b/crypto/mem_sec.c index 31c052512d..d61d945d63 100644 --- a/crypto/mem_sec.c +++ b/crypto/mem_sec.c @@ -37,7 +37,6 @@ static size_t secure_mem_used; static int secure_mem_initialized; -static int too_late; static CRYPTO_RWLOCK *sec_malloc_lock = NULL; @@ -48,7 +47,7 @@ static int sh_init(size_t size, int minsize); static char *sh_malloc(size_t size); static void sh_free(char *ptr); static void sh_done(void); -static int sh_actual_size(char *ptr); +static size_t sh_actual_size(char *ptr); static int sh_allocated(const char *ptr); #endif @@ -57,10 +56,6 @@ int CRYPTO_secure_malloc_init(size_t size, int minsize) #ifdef IMPLEMENTED int ret = 0; - if (too_late) - return ret; - - OPENSSL_assert(!secure_mem_initialized); if (!secure_mem_initialized) { sec_malloc_lock = CRYPTO_THREAD_lock_new(); if (sec_malloc_lock == NULL) @@ -75,13 +70,17 @@ int CRYPTO_secure_malloc_init(size_t size, int minsize) #endif /* IMPLEMENTED */ } -void CRYPTO_secure_malloc_done() +int CRYPTO_secure_malloc_done() { #ifdef IMPLEMENTED - sh_done(); - secure_mem_initialized = 0; - CRYPTO_THREAD_lock_free(sec_malloc_lock); + if (secure_mem_used == 0) { + sh_done(); + secure_mem_initialized = 0; + CRYPTO_THREAD_lock_free(sec_malloc_lock); + return 1; + } #endif /* IMPLEMENTED */ + return 0; } int CRYPTO_secure_malloc_initialized() @@ -100,7 +99,6 @@ void *CRYPTO_secure_malloc(size_t num, const char *file, int line) size_t actual_size; if (!secure_mem_initialized) { - too_late = 1; return CRYPTO_malloc(num, file, line); } CRYPTO_THREAD_write_lock(sec_malloc_lock); @@ -130,7 +128,7 @@ void CRYPTO_secure_free(void *ptr, const char *file, int line) if (ptr == NULL) return; - if (!secure_mem_initialized) { + if (!CRYPTO_secure_allocated(ptr)) { CRYPTO_free(ptr, file, line); return; } @@ -208,9 +206,11 @@ size_t CRYPTO_secure_actual_size(void *ptr) * place. */ -# define TESTBIT(t, b) (t[(b) >> 3] & (1 << ((b) & 7))) -# define SETBIT(t, b) (t[(b) >> 3] |= (1 << ((b) & 7))) -# define CLEARBIT(t, b) (t[(b) >> 3] &= (0xFF & ~(1 << ((b) & 7)))) +#define ONE ((size_t)1) + +# define TESTBIT(t, b) (t[(b) >> 3] & (ONE << ((b) & 7))) +# define SETBIT(t, b) (t[(b) >> 3] |= (ONE << ((b) & 7))) +# define CLEARBIT(t, b) (t[(b) >> 3] &= (0xFF & ~(ONE << ((b) & 7)))) #define WITHIN_ARENA(p) \ ((char*)(p) >= sh.arena && (char*)(p) < &sh.arena[sh.arena_size]) @@ -229,21 +229,21 @@ typedef struct sh_st char* map_result; size_t map_size; char *arena; - int arena_size; + size_t arena_size; char **freelist; - int freelist_size; - int minsize; + ossl_ssize_t freelist_size; + size_t minsize; unsigned char *bittable; unsigned char *bitmalloc; - int bittable_size; /* size in bits */ + size_t bittable_size; /* size in bits */ } SH; static SH sh; -static int sh_getlist(char *ptr) +static size_t sh_getlist(char *ptr) { - int list = sh.freelist_size - 1; - int bit = (sh.arena_size + ptr - sh.arena) / sh.minsize; + ossl_ssize_t list = sh.freelist_size - 1; + size_t bit = (sh.arena_size + ptr - sh.arena) / sh.minsize; for (; bit; bit >>= 1, list--) { if (TESTBIT(sh.bittable, bit)) @@ -257,22 +257,22 @@ static int sh_getlist(char *ptr) static int sh_testbit(char *ptr, int list, unsigned char *table) { - int bit; + size_t bit; OPENSSL_assert(list >= 0 && list < sh.freelist_size); OPENSSL_assert(((ptr - sh.arena) & ((sh.arena_size >> list) - 1)) == 0); - bit = (1 << list) + ((ptr - sh.arena) / (sh.arena_size >> list)); + bit = (ONE << list) + ((ptr - sh.arena) / (sh.arena_size >> list)); OPENSSL_assert(bit > 0 && bit < sh.bittable_size); return TESTBIT(table, bit); } static void sh_clearbit(char *ptr, int list, unsigned char *table) { - int bit; + size_t bit; OPENSSL_assert(list >= 0 && list < sh.freelist_size); OPENSSL_assert(((ptr - sh.arena) & ((sh.arena_size >> list) - 1)) == 0); - bit = (1 << list) + ((ptr - sh.arena) / (sh.arena_size >> list)); + bit = (ONE << list) + ((ptr - sh.arena) / (sh.arena_size >> list)); OPENSSL_assert(bit > 0 && bit < sh.bittable_size); OPENSSL_assert(TESTBIT(table, bit)); CLEARBIT(table, bit); @@ -280,11 +280,11 @@ static void sh_clearbit(char *ptr, int list, unsigned char *table) static void sh_setbit(char *ptr, int list, unsigned char *table) { - int bit; + size_t bit; OPENSSL_assert(list >= 0 && list < sh.freelist_size); OPENSSL_assert(((ptr - sh.arena) & ((sh.arena_size >> list) - 1)) == 0); - bit = (1 << list) + ((ptr - sh.arena) / (sh.arena_size >> list)); + bit = (ONE << list) + ((ptr - sh.arena) / (sh.arena_size >> list)); OPENSSL_assert(bit > 0 && bit < sh.bittable_size); OPENSSL_assert(!TESTBIT(table, bit)); SETBIT(table, bit); @@ -449,21 +449,21 @@ static int sh_allocated(const char *ptr) static char *sh_find_my_buddy(char *ptr, int list) { - int bit; + size_t bit; char *chunk = NULL; - bit = (1 << list) + (ptr - sh.arena) / (sh.arena_size >> list); + bit = (ONE << list) + (ptr - sh.arena) / (sh.arena_size >> list); bit ^= 1; if (TESTBIT(sh.bittable, bit) && !TESTBIT(sh.bitmalloc, bit)) - chunk = sh.arena + ((bit & ((1 << list) - 1)) * (sh.arena_size >> list)); + chunk = sh.arena + ((bit & ((ONE << list) - 1)) * (sh.arena_size >> list)); return chunk; } static char *sh_malloc(size_t size) { - int list, slist; + ossl_ssize_t list, slist; size_t i; char *chunk; @@ -522,7 +522,7 @@ static char *sh_malloc(size_t size) static void sh_free(char *ptr) { - int list; + size_t list; char *buddy; if (ptr == NULL) @@ -559,7 +559,7 @@ static void sh_free(char *ptr) } } -static int sh_actual_size(char *ptr) +static size_t sh_actual_size(char *ptr) { int list; @@ -568,6 +568,6 @@ static int sh_actual_size(char *ptr) return 0; list = sh_getlist(ptr); OPENSSL_assert(sh_testbit(ptr, list, sh.bittable)); - return sh.arena_size / (1 << list); + return sh.arena_size / (ONE << list); } #endif /* IMPLEMENTED */ diff --git a/doc/crypto/OPENSSL_secure_malloc.pod b/doc/crypto/OPENSSL_secure_malloc.pod index 2a04931cec..8e62658150 100644 --- a/doc/crypto/OPENSSL_secure_malloc.pod +++ b/doc/crypto/OPENSSL_secure_malloc.pod @@ -16,13 +16,13 @@ CYRPTO_secure_malloc_used - secure heap storage int CRYPTO_secure_malloc_initialized(); - void CRYPTO_secure_malloc_done(); + int CRYPTO_secure_malloc_done(); - void *OPENSSL_secure_malloc(int num); - void *CRYPTO_secure_malloc(int num, const char *file, int line); + void *OPENSSL_secure_malloc(size_t num); + void *CRYPTO_secure_malloc(size_t num, const char *file, int line); - void *OPENSSL_secure_zalloc(int num); - void *CRYPTO_secure_zalloc(int num, const char *file, int line); + void *OPENSSL_secure_zalloc(size_t num); + void *CRYPTO_secure_zalloc(size_t num, const char *file, int line); void OPENSSL_secure_free(void* ptr); void CRYPTO_secure_free(void *ptr, const char *, int); @@ -30,7 +30,7 @@ CYRPTO_secure_malloc_used - secure heap storage size_t OPENSSL_secure_actual_size(const void *ptr); int OPENSSL_secure_allocated(const void *ptr); - size_t CYRPTO_secure_malloc_used(); + size_t CYRPTO_secure_used(); =head1 DESCRIPTION @@ -49,14 +49,14 @@ put all intermediate values and computations there. CRYPTO_secure_malloc_init() creates the secure heap, with the specified C in bytes. The C parameter is the minimum size to allocate from the heap. Both C and C must be a power -of two. It is an error to call this after any OPENSSL_secure_malloc() -calls have been made. +of two. CRYPTO_secure_malloc_initialized() indicates whether or not the secure heap as been initialized and is available. CRYPTO_secure_malloc_done() releases the heap and makes the memory unavailable -to the process. It can take noticeably long to complete. +to the process if all secure memory has been freed. +It can take noticeably long to complete. OPENSSL_secure_malloc() allocates C bytes from the heap. If CRYPTO_secure_malloc_init() is not called, this is equivalent to @@ -83,7 +83,7 @@ OPENSSL_secure_actual_size() tells the actual size allocated to the pointer; implementations may allocate more space than initially requested, in order to "round up" and reduce secure heap fragmentation. -CRYPTO_secure_malloc_used() returns the number of bytes allocated in the +CRYPTO_secure_used() returns the number of bytes allocated in the secure heap. =head1 RETURN VALUES @@ -94,7 +94,7 @@ mapping. CRYPTO_secure_malloc_initialized() returns 1 if the secure heap is available (that is, if CRYPTO_secure_malloc_init() has been called, -but CRYPTO_secure_malloc_done() has not) or 0 if not. +but CRYPTO_secure_malloc_done() has not been called or failed) or 0 if not. OPENSSL_secure_malloc() and OPENSSL_secure_zalloc() return a pointer into the secure heap of the requested size, or C if memory could not be @@ -102,13 +102,9 @@ allocated. CRYPTO_secure_allocated() returns 1 if the pointer is in the secure heap, or 0 if not. -CRYPTO_secure_malloc_done() and OPENSSL_secure_free() -return no values. +CRYPTO_secure_malloc_done() returns 1 if the secure memory area is released, or 0 if not. -=head1 BUGS - -The size parameters should be B not B and will be changed -in a future release. +OPENSSL_secure_free() returns no values. =head1 SEE ALSO diff --git a/include/openssl/crypto.h b/include/openssl/crypto.h index 0eaf6b103e..5e16318d7b 100644 --- a/include/openssl/crypto.h +++ b/include/openssl/crypto.h @@ -371,7 +371,7 @@ void *CRYPTO_clear_realloc(void *addr, size_t old_num, size_t num, const char *file, int line); int CRYPTO_secure_malloc_init(size_t sz, int minsize); -void CRYPTO_secure_malloc_done(void); +int CRYPTO_secure_malloc_done(void); void *CRYPTO_secure_malloc(size_t num, const char *file, int line); void *CRYPTO_secure_zalloc(size_t num, const char *file, int line); void CRYPTO_secure_free(void *ptr, const char *file, int line); diff --git a/test/secmemtest.c b/test/secmemtest.c index 7a772917f9..523dffa5ee 100644 --- a/test/secmemtest.c +++ b/test/secmemtest.c @@ -1,32 +1,93 @@ #include +#define perror_line() perror_line1(__LINE__) +#define perror_line1(l) perror_line2(l) +#define perror_line2(l) perror("failed " #l) + int main(int argc, char **argv) { #if defined(OPENSSL_SYS_LINUX) || defined(OPENSSL_SYS_UNIX) - char *p = NULL, *q = NULL; + char *p = NULL, *q = NULL, *r = NULL, *s = NULL; + r = OPENSSL_secure_malloc(20); + /* r = non-secure 20 */ + if (r == NULL) { + perror_line(); + return 1; + } if (!CRYPTO_secure_malloc_init(4096, 32)) { - perror("failed"); + perror_line(); + return 1; + } + if (CRYPTO_secure_allocated(r)) { + perror_line(); return 1; } p = OPENSSL_secure_malloc(20); + /* r = non-secure 20, p = secure 20 */ if (!CRYPTO_secure_allocated(p)) { - perror("failed 1"); + perror_line(); + return 1; + } + /* 20 secure -> 32-byte minimum allocaton unit */ + if (CRYPTO_secure_used() != 32) { + perror_line(); return 1; } q = OPENSSL_malloc(20); + /* r = non-secure 20, p = secure 20, q = non-secure 20 */ if (CRYPTO_secure_allocated(q)) { - perror("failed 1"); + perror_line(); + return 1; + } + s = OPENSSL_secure_malloc(20); + /* r = non-secure 20, p = secure 20, q = non-secure 20, s = secure 20 */ + if (!CRYPTO_secure_allocated(s)) { + perror_line(); + return 1; + } + /* 2 * 20 secure -> 64 bytes allocated */ + if (CRYPTO_secure_used() != 64) { + perror_line(); return 1; } OPENSSL_secure_free(p); + /* 20 secure -> 32 bytes allocated */ + if (CRYPTO_secure_used() != 32) { + perror_line(); + return 1; + } OPENSSL_free(q); - CRYPTO_secure_malloc_done(); + /* should not complete, as secure memory is still allocated */ + if (CRYPTO_secure_malloc_done()) { + perror_line(); + return 1; + } + if (!CRYPTO_secure_malloc_initialized()) { + perror_line(); + return 1; + } + OPENSSL_secure_free(s); + /* secure memory should now be 0, so done should complete */ + if (CRYPTO_secure_used() != 0) { + perror_line(); + return 1; + } + if (!CRYPTO_secure_malloc_done()) { + perror_line(); + return 1; + } + if (CRYPTO_secure_malloc_initialized()) { + perror_line(); + return 1; + } + /* this can complete - it was not really secure */ + OPENSSL_secure_free(r); #else /* Should fail. */ if (CRYPTO_secure_malloc_init(4096, 32)) { - perror("failed"); + perror_line(); return 1; } #endif -- 2.34.1