Message ID | 20201203182230.33552-1-steffan@karger.me |
---|---|
State | Accepted |
Headers | show |
Series | [Openvpn-devel,1/2,v2] tls-crypt-v2: fix server memory leak | expand |
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>
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 --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);
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(-)