[Openvpn-devel] Fix a potential memory leak in tls_ctx_use_management_external_key

Message ID 20220120162645.13881-1-selva.nair@gmail.com
State Accepted
Headers show
Series
  • [Openvpn-devel] Fix a potential memory leak in tls_ctx_use_management_external_key
Related show

Commit Message

Selva Nair Jan. 20, 2022, 4:26 p.m.
From: Selva Nair <selva.nair@gmail.com>

As pointed out by Gert Doering <gert@greenie.muc.de>

Signed-off-by: Selva Nair <selva.nair@gmail.com>
---
To be applied after 06/18 of xkey patchset

 src/openvpn/ssl_openssl.c | 1 +
 1 file changed, 1 insertion(+)

Comments

Antonio Quartulli Jan. 25, 2022, 4:47 p.m. | #1
Hi,

On 20/01/2022 17:26, selva.nair@gmail.com wrote:
> From: Selva Nair <selva.nair@gmail.com>
> 
> As pointed out by Gert Doering <gert@greenie.muc.de>

This can be "gitified" as:

Reported-by: Gert Doering <gert@greenie.muc.de>

> 
> Signed-off-by: Selva Nair <selva.nair@gmail.com>
> ---
> To be applied after 06/18 of xkey patchset
> 
>   src/openvpn/ssl_openssl.c | 1 +
>   1 file changed, 1 insertion(+)
> 
> diff --git a/src/openvpn/ssl_openssl.c b/src/openvpn/ssl_openssl.c
> index b48845eb..3f8c3091 100644
> --- a/src/openvpn/ssl_openssl.c
> +++ b/src/openvpn/ssl_openssl.c
> @@ -1493,6 +1493,7 @@ tls_ctx_use_management_external_key(struct tls_root_ctx *ctx)
>       if (!privkey
>           || !SSL_CTX_use_PrivateKey(ctx->ctx, privkey))
>       {
> +        EVP_PKEY_free(privkey);
>           goto cleanup;
>       }
>       EVP_PKEY_free(privkey);

This is non-crucial because we goto cleanup and then do a M_FATAL.

Nonetheless, we should strive to have cleaner error paths that take care 
of releasing allocated resources when possible.

Acked-by: Antonio Quartulli <a@unstable.cc>
Gert Doering Jan. 26, 2022, 1:58 p.m. | #2
Thanks.

Even if, as Antonio noticed, we will only leak memory for a very short
time before hitting the M_FATAL message in "cleanup"...

Your patch has been applied to the master branch.

commit 0f7cd474118deeced48168ed9cec0806e7f4cc15
Author: Selva Nair
Date:   Thu Jan 20 11:26:45 2022 -0500

     Fix a potential memory leak in tls_ctx_use_management_external_key

     Signed-off-by: Selva Nair <selva.nair@gmail.com>
     Acked-by: Antonio Quartulli <a@unstable.cc>
     Message-Id: <20220120162645.13881-1-selva.nair@gmail.com>
     URL: https://www.mail-archive.com/openvpn-devel@lists.sourceforge.net/msg23610.html
     Signed-off-by: Gert Doering <gert@greenie.muc.de>


--
kind regards,

Gert Doering

Patch

diff --git a/src/openvpn/ssl_openssl.c b/src/openvpn/ssl_openssl.c
index b48845eb..3f8c3091 100644
--- a/src/openvpn/ssl_openssl.c
+++ b/src/openvpn/ssl_openssl.c
@@ -1493,6 +1493,7 @@  tls_ctx_use_management_external_key(struct tls_root_ctx *ctx)
     if (!privkey
         || !SSL_CTX_use_PrivateKey(ctx->ctx, privkey))
     {
+        EVP_PKEY_free(privkey);
         goto cleanup;
     }
     EVP_PKEY_free(privkey);