diff mbox series

[Openvpn-devel,1/2,v2] tls-crypt-v2: fix server memory leak

Message ID 20201203182230.33552-1-steffan@karger.me
State Accepted
Headers
Series [Openvpn-devel,1/2,v2] tls-crypt-v2: fix server memory leak |

Commit Message

Steffan Karger Dec. 3, 2020, 7:22 a.m. UTC 
  tls-crypt-v2 was developed in parallel with the changes that allowed to
use tls-auth/tls-crypt in connection blocks. The tls-crypt-v2 patch set
was never updated to the new reality after commit 5817b49b, causing a
memory leak of about 600 bytes for each connecting client.

It would be nicer to not reload the tls-crypt-v2 server key for each
connecting client, but that requires more refactoring (and thus more time
to get right). So for now just plug the leak by free'ing the memory when
we close a client connection.

To test this easily, compile openvpn with -fsanity=address, run a server
with tls-crypt-v2, connect a client, stop the server.

Signed-off-by: Steffan Karger <steffan@karger.me>
---
v2: remove now-redundant free_key_ctx() from key_schedule_free()

 src/openvpn/init.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

Comments

Antonio Quartulli Dec. 3, 2020, 7:56 a.m. UTC | #1 
Hi,

On 03/12/2020 19:22, Steffan Karger wrote:
> tls-crypt-v2 was developed in parallel with the changes that allowed to
> use tls-auth/tls-crypt in connection blocks. The tls-crypt-v2 patch set
> was never updated to the new reality after commit 5817b49b, causing a
> memory leak of about 600 bytes for each connecting client.
> 
> It would be nicer to not reload the tls-crypt-v2 server key for each
> connecting client, but that requires more refactoring (and thus more time
> to get right). So for now just plug the leak by free'ing the memory when
> we close a client connection.
> 
> To test this easily, compile openvpn with -fsanity=address, run a server
> with tls-crypt-v2, connect a client, stop the server.
> 
> Signed-off-by: Steffan Karger <steffan@karger.me>


Acked-by: Antonio Quartulli <a@unstable.cc>
Gert Doering Dec. 3, 2020, 11:46 p.m. UTC | #2 
I have not done much testing, and not even stared much at the
code.  But it's your code and Antonio has a keen eye :-)

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

commit fb169c3b8fdfa9792c0eee8441956f062dfd7982 (master)
commit 06e769552481729ddae28ee46b30f2dc8ca77509 (release/2.5)
Author: Steffan Karger
Date:   Thu Dec 3 19:22:30 2020 +0100

     tls-crypt-v2: fix server memory leak

     Signed-off-by: Steffan Karger <steffan@karger.me>
     Acked-by: Antonio Quartulli <antonio@openvpn.net>
     Message-Id: <20201203182230.33552-1-steffan@karger.me>
     URL: https://www.mail-archive.com/openvpn-devel@lists.sourceforge.net/msg21310.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 27a4170d..c3493c42 100644
--- a/src/openvpn/init.c
+++ b/src/openvpn/init.c
@@ -2561,7 +2561,6 @@  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->tls_crypt_v2_server_key);
     }
     CLEAR(*ks);
 }
@@ -3619,6 +3618,7 @@  do_close_free_key_schedule(struct context *c, bool free_ssl_ctx)
      * always free the tls_auth/crypt key. If persist_key is true, the key will
      * be reloaded from memory (pre-cached)
      */
+    free_key_ctx(&c->c1.ks.tls_crypt_v2_server_key);
     free_key_ctx_bi(&c->c1.ks.tls_wrap_key);
     CLEAR(c->c1.ks.tls_wrap_key);
     buf_clear(&c->c1.ks.tls_crypt_v2_wkc);