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

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

Commit Message

Steffan Karger Dec. 3, 2020, 4:49 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>
---
 src/openvpn/init.c | 1 +
 1 file changed, 1 insertion(+)

Comments

Antonio Quartulli Dec. 3, 2020, 6:18 a.m. UTC | #1
Hi Steffan,

On 03/12/2020 16:49, Steffan Karger wrote:
> diff --git a/src/openvpn/init.c b/src/openvpn/init.c
> index 27a4170d..5cde8a4b 100644
> --- a/src/openvpn/init.c
> +++ b/src/openvpn/init.c
> @@ -3619,6 +3619,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);

A few lines below we call key_schedule_free() (under certain conditions)
which also performs:

free_key_ctx(&ks->tls_crypt_v2_server_key);


I believe it is safe to call free_key_ctx() twice on the same object,
but wouldn't it be better to have it called once only along the same
code path?

Regards,

Patch

diff --git a/src/openvpn/init.c b/src/openvpn/init.c
index 27a4170d..5cde8a4b 100644
--- a/src/openvpn/init.c
+++ b/src/openvpn/init.c
@@ -3619,6 +3619,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);