Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

RAND_add(): fix heap corruption in error path #7455

Closed
wants to merge 1 commit into from
Closed

RAND_add(): fix heap corruption in error path #7455

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
4 changes: 4 additions & 0 deletions crypto/rand/rand_lib.c
View file
Open in desktop
Expand Up @@ -200,6 +200,10 @@ size_t rand_drbg_get_entropy(RAND_DRBG *drbg,
}

err:
/* we need to reset drbg->pool in the error case */
if (ret == 0 && drbg->pool != NULL)
drbg->pool = NULL;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

@mspncp mspncp Oct 24, 2018 •
edited

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OK, thanks.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, good point.


rand_pool_free(pool);
return ret;
}
Expand Down