RAND_add(): fix heap corruption in error path#7455
RAND_add(): fix heap corruption in error path#7455mspncp wants to merge 1 commit intoopenssl:masterfrom
Conversation
This bug was introduced by openssl#7382 which enhanced RAND_add() to accept large buffer sizes. As a consequence, RAND_add() now fails for buffer sizes less than 32 bytes (i.e. less than 256 bits). In addition, rand_drbg_get_entropy() forgets to reset the attached drbg->pool in the case of an error, which leads to the heap corruption. The problem occurred with RAND_load_file(), which reads the file in chunks of 1024 bytes each. If the size of the final chunk is less than 32 bytes, then RAND_add() fails, whence RAND_load_file() fails silently for buffer sizes n = k * 1024 + r with r = 1,...,31. This commit fixes the heap corruption only. The other issues will be addressed in a separate pull request. Thanks to Gisle Vanem for reporting this issue. Fixes openssl#7449
|
Since #7382 went to master and 1.1.1, the fix needs to go there, too. |
mspncp
added
branch: master
This bug was introduced by #7382 which enhanced RAND_add() to accept large buffer sizes. As a consequence, RAND_add() now fails for buffer sizes less than 32 bytes (i.e. less than 256 bits). In addition, rand_drbg_get_entropy() forgets to reset the attached drbg->pool in the case of an error, which leads to the heap corruption. The problem occurred with RAND_load_file(), which reads the file in chunks of 1024 bytes each. If the size of the final chunk is less than 32 bytes, then RAND_add() fails, whence RAND_load_file() fails silently for buffer sizes n = k * 1024 + r with r = 1,...,31. This commit fixes the heap corruption only. The other issues will be addressed in a separate pull request. Thanks to Gisle Vanem for reporting this issue. Fixes #7449 Reviewed-by: Paul Dale <[email protected]> (Merged from #7455)
This bug was introduced by #7382 which enhanced RAND_add() to accept large buffer sizes. As a consequence, RAND_add() now fails for buffer sizes less than 32 bytes (i.e. less than 256 bits). In addition, rand_drbg_get_entropy() forgets to reset the attached drbg->pool in the case of an error, which leads to the heap corruption. The problem occurred with RAND_load_file(), which reads the file in chunks of 1024 bytes each. If the size of the final chunk is less than 32 bytes, then RAND_add() fails, whence RAND_load_file() fails silently for buffer sizes n = k * 1024 + r with r = 1,...,31. This commit fixes the heap corruption only. The other issues will be addressed in a separate pull request. Thanks to Gisle Vanem for reporting this issue. Fixes #7449 Reviewed-by: Paul Dale <[email protected]> (Merged from #7455) (cherry picked from commit 5b4cb38)
|
Merged to master and 1.1.1. Thanks @gvanem and @paulidale! |
| err: | ||
| /* we need to reset drbg->pool in the error case */ | ||
| if (ret == 0 && drbg->pool != NULL) | ||
| drbg->pool = NULL; |
There was a problem hiding this comment.
Aehm, sorry but there must be a better solution than this.
There was a problem hiding this comment.
What is it that disturbs you here?
Is it the check for drbg->pool != NULL before assigning drbg->pool = NULL?
if (ret == 0 && drbg->pool != NULL)
drbg->pool = NULL;
would you prefer an unconditional assignment instead?
if (ret == 0)
drbg->pool = NULL;
My rationale behind the extra check was that it ensures that the if-condition is practically always false unless an internal error occurred. So a write will never happen under normal circumstances.
Please also note the master drbg has locking enabled, which means that the calls to rand_drbg_get_entropy() are protected by the drbg->lock write lock. which is taken and released in drbg_add(). These are the corresponding callstacks during the execution of RAND_add():
#0 rand_drbg_lock (drbg=0x55555589c380) at crypto/rand/drbg_lib.c:852
#1 0x00007ffff77d41f7 in drbg_add (buf=0x7fffffffbe90, num=32, randomness=32) at crypto/rand/drbg_lib.c:1058
#2 0x00007ffff77d5461 in RAND_add (buf=0x7fffffffbe90, num=32, randomness=32) at crypto/rand/rand_lib.c:787
#3 0x00007ffff77d5fe3 in RAND_load_file (file=0x7fffffffdd0b "RANDFILE32", bytes=-1) at crypto/rand/randfile.c:141
#4 0x00005555555db29d in loadfiles (name=0x7fffffffdd0b "RANDFILE32") at apps/app_rand.c:46
#5 0x00005555555db3c4 in opt_rand (opt=1501) at apps/app_rand.c:85
#6 0x00005555555ab9dd in rand_main (argc=5, argv=0x7fffffffd970) at apps/rand.c:68
#7 0x00005555555a242d in do_cmd (prog=0x555555896820, argc=5, argv=0x7fffffffd970) at apps/openssl.c:620
#8 0x00005555555a15dc in main (argc=5, argv=0x7fffffffd970) at apps/openssl.c:183
#0 rand_drbg_get_entropy (drbg=0x55555589c380, pout=0x7fffffffbcd8, entropy=256, min_len=32, max_len=2147483647, prediction_resistance=0) at crypto/rand/rand_lib.c:136
#1 0x00007ffff77d3421 in RAND_DRBG_reseed (drbg=0x55555589c380, adin=0x0, adinlen=0, prediction_resistance=0) at crypto/rand/drbg_lib.c:502
#2 0x00007ffff77d37f3 in rand_drbg_restart (drbg=0x55555589c380, buffer=0x7fffffffbe90 "", len=32, entropy=256) at crypto/rand/drbg_lib.c:624
#3 0x00007ffff77d424c in drbg_add (buf=0x7fffffffbe90, num=32, randomness=32) at crypto/rand/drbg_lib.c:1059
#4 0x00007ffff77d5461 in RAND_add (buf=0x7fffffffbe90, num=32, randomness=32) at crypto/rand/rand_lib.c:787
#5 0x00007ffff77d5fe3 in RAND_load_file (file=0x7fffffffdd0b "RANDFILE32", bytes=-1) at crypto/rand/randfile.c:141
#6 0x00005555555db29d in loadfiles (name=0x7fffffffdd0b "RANDFILE32") at apps/app_rand.c:46
#7 0x00005555555db3c4 in opt_rand (opt=1501) at apps/app_rand.c:85
#8 0x00005555555ab9dd in rand_main (argc=5, argv=0x7fffffffd970) at apps/rand.c:68
#9 0x00005555555a242d in do_cmd (prog=0x555555896820, argc=5, argv=0x7fffffffd970) at apps/openssl.c:620
#10 0x00005555555a15dc in main (argc=5, argv=0x7fffffffd970) at apps/openssl.c:183
#0 rand_drbg_unlock (drbg=0x55555589c380) at crypto/rand/drbg_lib.c:866
#1 0x00007ffff77d425b in drbg_add (buf=0x7fffffffbe90, num=32, randomness=32) at crypto/rand/drbg_lib.c:1062
#2 0x00007ffff77d5461 in RAND_add (buf=0x7fffffffbe90, num=32, randomness=32) at crypto/rand/rand_lib.c:787
#3 0x00007ffff77d5fe3 in RAND_load_file (file=0x7fffffffdd0b "RANDFILE32", bytes=-1) at crypto/rand/randfile.c:141
#4 0x00005555555db29d in loadfiles (name=0x7fffffffdd0b "RANDFILE32") at apps/app_rand.c:46
#5 0x00005555555db3c4 in opt_rand (opt=1501) at apps/app_rand.c:85
#6 0x00005555555ab9dd in rand_main (argc=5, argv=0x7fffffffd970) at apps/rand.c:68
#7 0x00005555555a242d in do_cmd (prog=0x555555896820, argc=5, argv=0x7fffffffd970) at apps/openssl.c:620
#8 0x00005555555a15dc in main (argc=5, argv=0x7fffffffd970) at apps/openssl.c:183
There was a problem hiding this comment.
Is it the check for drbg->pool != NULL before assigning drbg->pool = NULL?
No. And I am also not concerned about locking.
It is the resource flow that happens in drbg_add.
The problem is the drbg->pool is not cleared, when res != 0, but the object is freed.
Therefore the pointer value becomes indeterminate by the C standard.
But later it is used in an if condition, here:
void rand_drbg_cleanup_entropy(RAND_DRBG *drbg,
unsigned char *out, size_t outlen)
{
if (drbg->pool == NULL)
OPENSSL_secure_clear_free(out, outlen);
else
drbg->pool = NULL;
}
Using stale pointers in this way is very dangerous, and should be avoided.
There was a problem hiding this comment.
I am using the drbg->pool just as boolean variable here to differentiate between the case where the pool data was attached to a buffer provided by RAND_add()/RAND_seed() or if it was regularly allocated by the get_entropy callback. In the former case I am not allowed to clear it, because the buffer pointers are declared const (but the constness had to be cast away).
I can understand that a local pointer value might disappear because it was stored in a register and the register is now reused. But I wouldn't expect that the compiler explicitely erases the drbg->pool member. Or am I missing something?
If you really insist, I would have to add a separate boolean variable drbg->attached_pool.
There was a problem hiding this comment.
This is a C standard issue. It says that pointers become "indeterminate" after the object
they refer to is destroyed with free, or goes out of scope. In the past we re-wrote code
in OpenSSL that used indeterminate pointer values.
Actually I think I don't like the way how the pointer is encapsulated in a pool, and the
pool is not free'd where it is allocated. I think the get_entropy function should
not free something that it did not own.
There was a problem hiding this comment.
Ok. I'll prepare a pull request (but not right away) and request your review.
There was a problem hiding this comment.
It is perhaps helpful when thinking about the C standard to consider things like segmented addressing. If, after free(), the segment for the freed pointer becomes invalid, then even attempting to load the pointer value into a register (without dereferencing it) could cause a fault. (IIUC -- I don't think I've knowingly interacted with machines using segmented addressing!)
4 participants
This bug was introduced by #7382 which enhanced RAND_add() to
accept large buffer sizes. As a consequence, RAND_add() now fails
for buffer sizes less than 32 bytes (i.e. less than 256 bits).
In addition, rand_drbg_get_entropy() forgets to reset the attached
drbg->pool in the case of an error, which leads to the heap corruption.
The problem occurred with RAND_load_file(), which reads the file in
chunks of 1024 bytes each. If the size of the final chunk is less than
32 bytes, then RAND_add() fails, whence RAND_load_file() fails
silently for buffer sizes n = k * 1024 + r with r = 1,...,31.
This commit fixes the heap corruption only. The other issues will
be addressed in a separate pull request.
Thanks to Gisle Vanem for reporting this issue.
Fixes #7449