Fix infinite loops in secure memory allocation. #3453
Closed
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
Backport of PR #3449 |
richsalz
approved these changes
May 12, 2017
|
This now includes the changes in PR #3455 |
|
Pulls in PR #3512 |
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 openssl#3512 commit fee423b PR openssl#3510 commit a486561 PR openssl#3455 commit c8e89d5 PR openssl#3449 commit 7031dda 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".
|
Harmonized, squashed, rebased. |
dot-asm
approved these changes
May 22, 2017
|
Since there were significant changes @richsalz has to re-confirm (and he can do so by simply merging :-) |
levitte
pushed a commit
that referenced
this pull request
May 22, 2017
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 fee423b PR #3510 commit a486561 PR #3455 commit c8e89d5 PR #3449 commit 7031dda 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 #3453)
|
thankns, and for the detailed commit comment. merged. |
pracj3am
pushed a commit
to cdn77/openssl
that referenced
this pull request
Aug 22, 2017
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 openssl#3512 commit fee423b PR openssl#3510 commit a486561 PR openssl#3455 commit c8e89d5 PR openssl#3449 commit 7031dda 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 openssl#3453)
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Backport this commit from master.
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: Rich Salz rsalz@openssl.org
Reviewed-by: Richard Levitte levitte@openssl.org
(Merged from #3449)
(cherry picked from commit 7031dda)
Conflicts:
test/secmemtest.c
Checklist