RAND_add(): fix heap corruption in error path

[mspncp]

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

[mspncp]

Since #7382 went to master and 1.1.1, the fix needs to go there, too.

[mspncp]

Merged to master and 1.1.1. Thanks @gvanem and @paulidale!

• 5b4cb38 master
• ece482f 1.1.1

[bernd-edlinger]

Aehm, sorry but there must be a better solution than this.

[mspncp]

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

[bernd-edlinger]

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.

[mspncp]

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.

[bernd-edlinger]

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.

[mspncp]

Ok. I'll prepare a pull request (but not right away) and request your review.

[bernd-edlinger]

OK, thanks.

[kaduk]

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!)

[bernd-edlinger]

Yes, good point.