diff mbox series

[Openvpn-devel,4/9] Add missing free_key_ctx for auth_token

Message ID 20210512131511.1309914-5-arne@rfc2549.org
State Accepted
Headers
Series Miscellaneous cleanup patches/small fixes |

Commit Message

Arne Schwabe May 12, 2021, 3:15 a.m. UTC 
  This is is a small memory leak as this key is only leaked once
per server start.

Signed-off-by: Arne Schwabe <arne@rfc2549.org>
---
 src/openvpn/init.c | 1 +
 1 file changed, 1 insertion(+)

Comments

Antonio Quartulli May 13, 2021, 10:22 a.m. UTC | #1 
Hi,

On 12/05/2021 15:15, Arne Schwabe wrote:
> This is is a small memory leak as this key is only leaked once
> per server start.
> 
> Signed-off-by: Arne Schwabe <arne@rfc2549.org>

The leak can indeed be seen with valgrind when running the server with
--auth-gen-token:

==23481== HEAP SUMMARY:
==23481==     in use at exit: 536 bytes in 7 blocks
==23481==   total heap usage: 7,460 allocs, 7,453 frees, 1,364,174 bytes
allocated
==23481==
==23481== 536 (32 direct, 504 indirect) bytes in 1 blocks are definitely
lost in loss record 7 of 7
==23481==    at 0x48397EF: malloc (vg_replace_malloc.c:307)
==23481==    by 0x4AEB269: CRYPTO_zalloc (in /usr/lib64/libcrypto.so.1.1)
==23481==    by 0x4AE4D67: HMAC_CTX_new (in /usr/lib64/libcrypto.so.1.1)
==23481==    by 0x11F3A8: hmac_ctx_new (crypto_openssl.c:996)
==23481==    by 0x119B84: init_key_ctx (crypto.c:847)
==23481==    by 0x1135F6: auth_token_init_secret (auth_token.c:168)
==23481==    by 0x12B117: do_init_auth_token_key (init.c:2685)
==23481==    by 0x12B117: do_init_crypto_tls_c1 (init.c:2764)
==23481==    by 0x12B117: do_init_crypto_tls (init.c:2802)
==23481==    by 0x12FF07: do_init_crypto (init.c:3065)
==23481==    by 0x12FF07: init_instance (init.c:4179)
==23481==    by 0x130990: init_instance_handle_signals (init.c:3991)
==23481==    by 0x140109: tunnel_server_udp (mudp.c:278)
==23481==    by 0x14BD28: openvpn_main (openvpn.c:287)
==23481==    by 0x4C69E39: (below main) (in /lib64/libc-2.32.so)
==23481==
==23481== LEAK SUMMARY:
==23481==    definitely lost: 32 bytes in 1 blocks
==23481==    indirectly lost: 504 bytes in 6 blocks
==23481==      possibly lost: 0 bytes in 0 blocks
==23481==    still reachable: 0 bytes in 0 blocks
==23481==         suppressed: 0 bytes in 0 blocks

And after applying the patch the leak is gone:

==27117== HEAP SUMMARY:
==27117==     in use at exit: 0 bytes in 0 blocks
==27117==   total heap usage: 7,447 allocs, 7,447 frees, 1,295,605 bytes
allocated
==27117==
==27117== All heap blocks were freed -- no leaks are possible


Acked-by: Antonio Quartulli <antonio@oppenvpn.net>

Should this go to 2.5 too?

Regards,
Gert Doering May 14, 2021, 1:54 a.m. UTC | #2 
I won't claim to understand the lifetime of the various copies of
c1.ks.auth_token_key made by code in init.c (to "to.auth_token_key"
or "other contexts") - but it seems that these all are copying 
c1.ks.ssl_ctx as well - and if that can be safely free()'ed, the
other one should be fine, too.  I also checked that free_key_ctx()
is safe to be used should we have no auth_token_key at all.

Your patch has been applied to the master and release/2.5 branch.

release/2.4 does not have the offending code (no key-based tokens).

I have only compile-tested 2.5 and master ("it should be fine"),
but if not, the server-side test rig will find it later today...

commit fe39156a386bf0dbe79abe43717c84843830e3c0 (master)
commit 6471fd2ab1d07ad24c2c92e7fbda6bd645dd84c8 (release/2.5)
Author: Arne Schwabe
Date:   Wed May 12 15:15:06 2021 +0200

     Add missing free_key_ctx for auth_token

     Signed-off-by: Arne Schwabe <arne@rfc2549.org>
     Acked-by: Antonio Quartulli <antonio@openvpn.net>
     Message-Id: <20210512131511.1309914-5-arne@rfc2549.org>
     URL: https://www.mail-archive.com/openvpn-devel@lists.sourceforge.net/msg22345.html
     Signed-off-by: Gert Doering <gert@greenie.muc.de>


--
kind regards,

Gert Doering
diff mbox series

Patch

diff --git a/src/openvpn/init.c b/src/openvpn/init.c
index 1d77a9d42..49c742928 100644
--- a/src/openvpn/init.c
+++ b/src/openvpn/init.c
@@ -2520,6 +2520,7 @@  key_schedule_free(struct key_schedule *ks, bool free_ssl_ctx)
     if (tls_ctx_initialised(&ks->ssl_ctx) && free_ssl_ctx)
     {
         tls_ctx_free(&ks->ssl_ctx);
+        free_key_ctx(&ks->auth_token_key);
     }
     CLEAR(*ks);
 }