multiple call to mbedtls_ssl_handshake on server create a memory leak

[giso-c]

Description

• Type: Bug
• Priority: Blocker

---

Bug

when calling multiple time mbedtls_ssl_handshake, it will create a memory leak

OS
Freertos

mbed TLS build:
TLS 2.25
Following define are used as we are using multiple threads
#define MBEDTLS_THREADING_C
#define MBEDTLS_THREADING_ALT

Expected behavior
We have a TLS server, upon client connection we call mbedtls_ssl_handshake. Once the connection is terminated FW waits for a new client connection and call again mbedtls_ssl_handshake.

mbedtls_ssl_handshake calls multiple times
mbedtls_ctr_drbg_init
mbedtls_ctr_drbg_free
mbedtls_ctr_drbg_init
mbedtls_ctr_drbg_free

Issue is that at the end of mbedtls_ssl_handshake we have 1 mutex created and it will never be deleted so next call to mbedtls_ssl_handshake will add a new mutex.

There is only 1 way to delete the mbedtls_ctr_drbg_context mutex is by calling mbedtls_ctr_drbg_free, but mbedtls_ctr_drbg_free will create a new mutex.

To reproduce, call multiple times mbedtls_ssl_handshake with MBEDTLS_THREADING_C and MBEDTLS_THREADING_ALT defined

[gilles-peskine-arm]

I guess this is the same problem as #4045? Can you try to see if Mbed TLS 2.24.0 works?

[giso-c]

If I remove mutex creation from mbedtls_ctr_drbg_free like this void mbedtls_ctr_drbg_free( mbedtls_ctr_drbg_context *ctx ) { if( ctx == NULL ) return; #if defined(MBEDTLS_THREADING_C) mbedtls_mutex_free( &ctx->mutex ); #endif mbedtls_aes_free( &ctx->aes_ctx ); mbedtls_platform_zeroize( ctx, sizeof( mbedtls_ctr_drbg_context ) ); ctx->reseed_interval = MBEDTLS_CTR_DRBG_RESEED_INTERVAL; ctx->reseed_counter = -1; #if defined(MBEDTLS_THREADING_C) // mbedtls_mutex_init( &ctx->mutex ); #endif } It works, this is similar to version 2.24 so I believe2.24 should also work but not sure if this will create some other issue. From: Gilles Peskine <[email protected]> Sent: Wednesday, January 27, 2021 12:53 PM To: ARMmbed/mbedtls <[email protected]> Cc: Adalgiso Castrignano <[email protected]>; Author <[email protected]> Subject: Re: [ARMmbed/mbedtls] multiple call to mbedtls_ssl_handshake on server create a memory leak (#4071) I guess this is the same problem as #4045<#4045>? Can you try to see if Mbed TLS 2.24.0 works? — You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub<#4071 (comment)>, or unsubscribe<https://github.com/notifications/unsubscribe-auth/AOJYMCKGAHNGWU5OTPFY3ATS375BVANCNFSM4WVD5PBA>.

[giso-c]

I believe it would be hard for you to tell when this issue will be evaluated and fixed.
Do you think it would be better to use 2.24 or to use the temporary fix (not creating mutex when calling mbedtls_ctr_drbg_free)?

[chris-jones-arm]

Hi @giso-c, thanks for the bug report! After a quick look this issue does look like a duplicate of #4045 however I will keep this open for now until this can be proven.

I would advise against using a temporary fix unless you absolutely need a new feature or bug fix that was released in 2.25 as removing that mutex could have unforseen consequences which are not immediately obvious. If you can stick with 2.24 then that should be preferred until this issue is fixed.

[gilles-peskine-arm]

If you decide to stick with 2.24, please review the security issues in the ChangeLog file. Fortunately in this release all the security fixes are relatively minor in the sense that they're either only about a second line of defense or about a rarely-used feature, but do review what might apply to you.

[gilles-peskine-arm]

There's a proposed fix in #4090. Please note that it has not been reviewed yet, and we may yet revise how we resolve this issue (it's not fully clear to me how to fix this issue without breaking “unusual” usage of the API). If you want to try it out, you can apply the patches from https://github.com/gilles-peskine-arm/mbedtls/tree/fix-drbg-mutex-usage-2.25-untested https://github.com/gilles-peskine-arm/mbedtls/tree/test-mutex-usage-count-development.

[giso-c]

I got the latest code, but did not fix the memory leak, same code running on 2.14 works fine but new code does not work.
I checked the heap before and after calling
while( ( tls_handshake_ret = mbedtls_ssl_handshake( &ssl ) ) != 0 )
{
if( tls_handshake_ret != MBEDTLS_ERR_SSL_WANT_READ && tls_handshake_ret != MBEDTLS_ERR_SSL_WANT_WRITE )
{
break;
}
}
and free heap has been decreased.

[gilles-peskine-arm]

Thanks for checking. I've started to test the 2.7 patches on development and I can reproduce the leak (tests/ssl-opt.sh -f Default -m passes on my 2.7 fix branch, but not yet on my development fix branch).

[gilles-peskine-arm]

Actually, I can't reproduce the problem. The leak I saw was some missing cleanup in the test program. Can you share your exact compile-time configuration and a precise way to reproduce the problem? Either with a small, self-contained program that can build on Linux (even if you only see the leak on FreeRTOS — mutexes on Linux don't consume extra heap, which is why we didn't see notice the bug immediately), or with programs/ssl/ssl_client2 or programs/ssl/ssl_server2 (which are the programs I'm using for testing).

[gilles-peskine-arm]

Oh, and please test with https://github.com/gilles-peskine-arm/mbedtls/tree/test-mutex-usage-count-development rather than the branch I posted earlier. I don't think I made any relevant change (the differences should only be in test code), but you never know.

[gilles-peskine-arm]

I'm closing this issue as a duplicate of #4017. It's either exactly the same problem (maybe in a different branch) or a slight variation (e.g. HMAC_DRBG vs CTR_DRBG) which will be fixed together. Please reopen this issue once the proposed fix is merged if it doesn't fix your problem.

We can continue exchanging comments about the proposed fix on this thread.