diff options
author | Benjamin Kaduk <bkaduk@akamai.com> | 2017-10-11 19:25:26 +0200 |
---|---|---|
committer | Ben Kaduk <kaduk@mit.edu> | 2017-10-18 08:39:20 -0500 |
commit | 2139145b72d084a3f974a94accd7d9812de286e4 (patch) | |
tree | 4c81571b7f35acd01c44e159fb75b756d856b818 /ssl/ssl_lib.c | |
parent | e0b625f9db00509af9004b7907d44b78f332754a (diff) |
Add missing RAND_DRBG locking
The drbg's lock must be held across calls to RAND_DRBG_generate()
to prevent simultaneous modification of internal state.
This was observed in practice with simultaneous SSL_new() calls attempting
to seed the (separate) per-SSL RAND_DRBG instances from the global
rand_drbg instance; this eventually led to simultaneous calls to
ctr_BCC_update() attempting to increment drbg->bltmp_pos for their
respective partial final block, violating the invariant that bltmp_pos < 16.
The AES operations performed in ctr_BCC_blocks() makes the race window
quite easy to trigger. A value of bltmp_pos greater than 16 induces
catastrophic failure in ctr_BCC_final(), with subtraction overflowing
and leading to an attempt to memset() to zero a very large range,
which eventually reaches an unmapped page and segfaults.
Provide the needed locking in get_entropy_from_parent(), as well as
fixing a similar issue in RAND_priv_bytes(). There is also an
unlocked call to RAND_DRBG_generate() in ssl_randbytes(), but the
requisite serialization is already guaranteed by the requirements on
the application's usage of SSL objects, and no further locking is
needed for correct behavior. In that case, leave a comment noting
the apparent discrepancy and the reason for its safety (at present).
Reviewed-by: Paul Dale <paul.dale@oracle.com>
Reviewed-by: Kurt Roeckx <kurt@roeckx.be>
Reviewed-by: Rich Salz <rsalz@openssl.org>
(Merged from https://github.com/openssl/openssl/pull/4328)
Diffstat (limited to 'ssl/ssl_lib.c')
-rw-r--r-- | ssl/ssl_lib.c | 17 |
1 files changed, 15 insertions, 2 deletions
diff --git a/ssl/ssl_lib.c b/ssl/ssl_lib.c index da74e96ecb..ce45a91613 100644 --- a/ssl/ssl_lib.c +++ b/ssl/ssl_lib.c @@ -5127,7 +5127,20 @@ uint32_t SSL_get_max_early_data(const SSL *s) int ssl_randbytes(SSL *s, unsigned char *rnd, size_t size) { - if (s->drbg != NULL) - return RAND_DRBG_generate(s->drbg, rnd, size, 0, NULL, 0); + if (s->drbg != NULL) { + /* + * Currently, it's the duty of the caller to serialize the generate + * requests to the DRBG. So formally we have to check whether + * s->drbg->lock != NULL and take the lock if this is the case. + * However, this DRBG is unique to a given SSL object, and we already + * require that SSL objects are only accessed by a single thread at + * a given time. Also, SSL DRBGs have no child DRBG, so there is + * no risk that this DRBG is accessed by a child DRBG in parallel + * for reseeding. As such, we can rely on the application's + * serialization of SSL accesses for the needed concurrency protection + * here. + */ + return RAND_DRBG_generate(s->drbg, rnd, size, 0, NULL, 0); + } return RAND_bytes(rnd, (int)size); } |