Page MenuHomePhabricator

I have upgraded libssh from 0.7.6 to 0.9.0 to use diffie-hellman-group-exchange-sha256 kex algo both in client and server, but I found that memory leak , see details
Closed, ResolvedPublic

Description

Hi
I have upgraded libssh from 0.7.6 to 0.9.0 to use diffie-hellman-group-exchange-sha256 kex algo both in client and server, but I found that memory leak

==7451==ERROR: LeakSanitizer: detected memory leaks

Direct leak of 48 byte(s) in 2 object(s) allocated from:
    #0 0x7f46654047f0 in __interceptor_malloc ../../.././libsanitizer/asan/asan_malloc_linux.cc:86
    #1 0x7f465c8784b7 in CRYPTO_malloc (/lib64/libcrypto.so.10+0x6e4b7)

I am afraid there are memory leak in the following code block in ssh_packet_client_dhgex_group

/* all checks passed, set parameters */
rc = ssh_dh_set_parameters(session->next_crypto->dh_ctx,
                           modulus, generator);
if (rc != SSH_OK) {
    bignum_safe_free(modulus);
    bignum_safe_free(generator);
    goto error;
}

Can you tell me how and where to free the memory of modulus and generator ? or with different compile flag ?

By the way, in 0.9.0 version, the function ssh_dh_cleanup and struct dh_ctx are defined in dh_crypto.c and dh_key.c,but
the former ssh_dh_cleanup was called by gdb.

void ssh_dh_cleanup(struct ssh_crypto_struct *crypto)
{
    if (crypto->dh_ctx != NULL) {
        DH_free(crypto->dh_ctx->keypair[0]);
        DH_free(crypto->dh_ctx->keypair[1]);
        free(crypto->dh_ctx);
        crypto->dh_ctx = NULL;
    }
}

but this function does not free modulus, generator.

Event Timeline

foxge triaged this task as High priority.Mon, Sep 16, 10:33 AM
foxge created this task.

Thank you for the bug report. Indeed, the handling of the modulus and generator is wrong. These bignums are copied in the ssh_dh_set_parameters() into the keypair structures (when using openssl backend), but the calling function did not free them as expected. Also the handling of errors was wrong in case of some failures checking them.

Can you verify that the following patch addresses the issue for you?

https://gitlab.com/jjelen/libssh-mirror/commit/5d265b95633e27cbd17805c7278fdb708a891901

I tested using valgrind and the memory leak went away according to my observations.

valgrind --trace-children=yes --trace-children-skip=/usr/sbin/sshd --leak-check=full ctest -R torture_algorithms

The valgrind memcheck jobs are not running for some time. @asn, can we expect them up some time soon?