[openssl-dev] [openssl.org #4180] Isses with respect to malloc failures handling.

[Matt Caswell via RT]

You don't say what version of OpenSSL you were testing. It seems to be either
1.0.2 or 1.0.1 (not master). Anyway, comments inserted.

On Mon Dec 14 13:45:20 2015, skoripella at juniper.net wrote:
> Issue 1)
> We could have failed to allocate the ctx->cipher_data in
> EVP_CipherInit_ex
>
> ctx->cipher_data = OPENSSL_malloc(ctx->cipher->ctx_size);
> if (!ctx->cipher_data) {
> EVPerr(EVP_F_EVP_CIPHERINIT_EX, ERR_R_MALLOC_FAILURE);
> return 0;
> }
>
> We do subsequently return error from EVP_CipherInit_ex. However during
> shutdown because of this error we are not checking for the NULL
> cipher_data causing cores

This seems very strange to me. If we have already returned a fatal error...then
its just that - fatal. Don't try and do a graceful shutdown on an already dead
connection.

>
========================================================================================
> Issue 2
> In file pmeth_gn.c function EVP_PKEY_keygen, openssl code tries to
> allocate EVP_PKEY using EVP_PKEY_new and immediately follows with a
> dereference of the same in the below path without checking if the
> allocation was successful or not.

Fixed in 8e0a94a58.

> Issue 3:
>
> In file s3_enc.c in function ssl3_digest_cached_records,
> EVP_DigestInit_ex is called to initialize the EVP digest. Internally
> to EVP_DigestInit_ex ctx->md_data is allocated and if it fails an
> error is returned. However in ssl3_digest_cached_records the return
> value is not checked, causing a null dereference with the below

Fixed in ada5de7c and similar commit in master (this was the only one
applicable to master BTW).


> Issue 4:
> In file ssl_lib.c, in function ssl_replace_hash, an EVP_MD_CTX is
> created using EVP_MD_CTX_create. However, the return value of this
> allocation is not checked and a dereference is made just below in
> EVP_DigestInit_ex causing a core.

This was fixed in 56d9134675 which was commited a few weeks before the date of
your report.

>
=======================================================================================
> Issue 5:
> In tl_enc.c, in function tls1_enc in the case of
> /\* Explicit IV length, block ciphers and TLS version 1.1 or later \*/
> openssl tries to dereference cipher after getting the value of cipher
> from s->enc_write_ctx. However cipher can be null. This can happen
> because we returned NULL in Issue 4) above and s->enc_write_ctx-
> >cipher might not have been set. Typically
> s->enc_write_ctx->cipher would have been set in the below path but
> because of Issue 4 above we did not set s->enc_write_ctx->cipher.

Issue 4 above resulted in a core if it failed...so this confuses me! Anyway I
could not see how this could occur if Issue 4 fails more gracefully. All
callers of ssl_replace_hash propagate the error. Perhaps an issue similar to
issue 1 above? Or maybe its been fixed since. I'm assuming this is no longer an
issue. Please raise a new ticket if it is.

> Issue 6:
> Similar issue as above exists in s3_pkt.c function do_ssl3_write in
> the case
> /\* Explicit IV length, block ciphers and TLS version 1.1 or later \*/
> where again s->enc_write_ctx->cipher can be NULL.

As for issue 5.

>
=======================================================================================
> Issue 7:
> In file t1_enc.c, in function tls1_mac, openssl after calling
> EVP_DigestSignFinal has an assert on the return value to be greater
> than 0. However, EVP_DigestSignFinal internally allocates memory and
> if this memory allocation fails, an error is returned. Hence this
> assert is overaggressive for low memory cases. So Pls see if instead
> of coring, the error can be handled gracefully.

This was fixed in the same commit mentioned above that was committed a few
weeks before your report.

>
========================================================================================
> Issue 8:
> In file t1_enc.c, in function tls1_setup_key_block, memory is
> allocated twice for the keyblock through p1 and p2. If p1 succeeds but
> p2 fails, p1 is freed but the freed pointer p1 is left dangling inside
> s->s3->tmp.key_block which is later attempted to be freed while
> freeing s->s3 resulting in a double free.
> The fix would be to set the s->s3->tmp.key_block to NULL

This was fixed in ec8f246e6ed4 a few weeks ago.

Closing this ticket.

Matt

-- 
Ticket here: http://rt.openssl.org/Ticket/Display.html?id=4180
Please log in as guest with password guest if prompted