[openssl-dev] Memory leak in application when we use ECDH

Matt Caswell matt at openssl.org
Tue Mar 21 09:46:31 UTC 2017



On 15/03/17 05:44, Mody, Darshan (Darshan) wrote:
> Hi,
> 
> We have observed memory leak when we register for ECDH
> callback(SSL_set_tmp_ecdh_callback). While performing negative testing
> with load we find that the applications starts leaking memory.  
> 
> Further checking the Openssl implementation in the s3_srvr.c file
> (openssl version 1.0.2). I find that a pointer is not deleted after
> copying EC_KEY from the application or EC_KEY_new being called.
> 
> I suspect the below code.

The code looks ok to me:

            if (s->cert->ecdh_tmp_auto)
                ecdh = ecdhp;
            else if ((ecdh = EC_KEY_dup(ecdhp)) == NULL) {
                SSLerr(SSL_F_SSL3_SEND_SERVER_KEY_EXCHANGE, ERR_R_ECDH_LIB);
                goto err;
            }

            s->s3->tmp.ecdh = ecdh;

After the EC_KEY_dup() call, the result is immediately assigned to
"s->s3->tmp.ecdh". This will get freed when you call SSL_free().
Similarly in the non-callback case.

There is a potential leak in this case:

            if (s->s3->tmp.ecdh != NULL) {
                SSLerr(SSL_F_SSL3_SEND_SERVER_KEY_EXCHANGE,
                       ERR_R_INTERNAL_ERROR);
                goto err;
            }

But this is a "should not happen" scenario - so there is another bug if
that is happening - and you would see "internal error" messages on the
error stack.

Another slight oddity in this code is the double check of ecdhp against
NULL which seems redundant (but otherwise harmless):

            if (ecdhp == NULL) {
                al = SSL_AD_HANDSHAKE_FAILURE;
                SSLerr(SSL_F_SSL3_SEND_SERVER_KEY_EXCHANGE,
                       SSL_R_MISSING_TMP_ECDH_KEY);
                goto f_err;
            }

            ...

            /* Duplicate the ECDH structure. */
            if (ecdhp == NULL) {
                SSLerr(SSL_F_SSL3_SEND_SERVER_KEY_EXCHANGE, ERR_R_ECDH_LIB);
                goto err;
            }


Also note that SSL_set_tmp_ecdh_callback() has been removed from OpenSSL
1.1.0 with this commit description:

commit 6f78b9e824c053d062188578635c575017b587c5
Author:     Kurt Roeckx <kurt at roeckx.be>
AuthorDate: Fri Dec 4 22:22:31 2015 +0100
Commit:     Kurt Roeckx <kurt at roeckx.be>
CommitDate: Fri Dec 4 22:22:31 2015 +0100

    Remove support for SSL_{CTX_}set_tmp_ecdh_callback().

    This only gets used to set a specific curve without actually
checking that the
    peer supports it or not and can therefor result in handshake
failures that can
    be avoided by selecting a different cipher.

    Reviewed-by: Dr. Stephen Henson <steve at openssl.org>


Matt



More information about the openssl-dev mailing list