Fix memory leaks in HMAC initial packet generation

The HMAC leaks are just forgotten frees/deinitialisations.

tls_wrap_control() will sometimes return the original buffer (non
tls-crypt) and sometimes tls_wrap.work, so handling this buffer lifetime
is a bit more complicated.  Instead of further complicating that code
just give our work buffer the same lifetime as the other one inside
tls_wrap.work (put it into per-session gc_arena) as that is also more
consistent.

Second, packet_id_init() allocates a buffer with malloc and not using a
gc_arena, so we need to also manually free it.

Patch v2: add missing deallocations in unit tests of the new workbuf
Patch v3: remove useless allocation of 0 size buffer in
          tls_auth_standalone_init

Found-By: clang with asan
Change-Id: I0cff44f79ee7e3bcf7b5981fc94f469c15f21af3
Signed-off-by: Arne Schwabe <arne@rfc2549.org>
Acked-by: Gert Doering <gert@greenie.muc.de>
Message-Id: <20230315195512.323070-1-arne@rfc2549.org>
URL: https://www.mail-archive.com/openvpn-devel@lists.sourceforge.net/
Signed-off-by: Gert Doering <gert@greenie.muc.de>

diff --git a/src/openvpn/init.c b/src/openvpn/init.c
index 124ac76bd9f126ce6fb3d25cfdc5fa1a227c8969..fa2681dc717e819adeb95aebb0baf6b23679bc3e 100644 (file)
--- a/src/openvpn/init.c
+++ b/src/openvpn/init.c
@@ -3483,6 +3483,7 @@ do_init_frame_tls(struct context *c)
         frame_print(&c->c2.tls_auth_standalone->frame, D_MTU_INFO,
                     "TLS-Auth MTU parms");
         c->c2.tls_auth_standalone->tls_wrap.work = alloc_buf_gc(BUF_SIZE(&c->c2.frame), &c->c2.gc);
+        c->c2.tls_auth_standalone->workbuf = alloc_buf_gc(BUF_SIZE(&c->c2.frame), &c->c2.gc);
     }
 }
 
@@ -3881,6 +3882,8 @@ do_close_tls(struct context *c)
         md_ctx_cleanup(c->c2.pulled_options_state);
         md_ctx_free(c->c2.pulled_options_state);
     }
+
+    tls_auth_standalone_free(c->c2.tls_auth_standalone);
 }
 
 /*

diff --git a/src/openvpn/ssl.c b/src/openvpn/ssl.c
index 78cec90a1c5afbf1f5db04e282d1c20a0c1e4a02..fe6390fada540b88b9329d72d2bd1b6ee59015e5 100644 (file)
--- a/src/openvpn/ssl.c
+++ b/src/openvpn/ssl.c
@@ -1361,6 +1361,17 @@ tls_auth_standalone_init(struct tls_options *tls_options,
     return tas;
 }
 
+void
+tls_auth_standalone_free(struct tls_auth_standalone *tas)
+{
+    if (!tas)
+    {
+        return;
+    }
+
+    packet_id_free(&tas->tls_wrap.opt.packet_id);
+}
+
 /*
  * Set local and remote option compatibility strings.
  * Used to verify compatibility of local and remote option

diff --git a/src/openvpn/ssl.h b/src/openvpn/ssl.h
index 58ff4b9b4e77c361edf0296e016f2eb4e76e4a7f..a050cd5c98144c805fec8e8ec955d1ed48846c21 100644 (file)
--- a/src/openvpn/ssl.h
+++ b/src/openvpn/ssl.h
@@ -180,6 +180,12 @@ void tls_multi_init_finalize(struct tls_multi *multi, int tls_mtu);
 struct tls_auth_standalone *tls_auth_standalone_init(struct tls_options *tls_options,
                                                      struct gc_arena *gc);
 
+/**
+ * Frees a standalone tls-auth verification object.
+ * @param tas   the object to free. May be NULL.
+ */
+void tls_auth_standalone_free(struct tls_auth_standalone *tas);
+
 /*
  * Setups the control channel frame size parameters from the data channel
  * parameters

diff --git a/src/openvpn/ssl_pkt.c b/src/openvpn/ssl_pkt.c
index 17a7f891725775e1de858bb48e20d52a36796c31..8b3391e7618ca0b5bc0bdb4beab1e990cd2851f3 100644 (file)
--- a/src/openvpn/ssl_pkt.c
+++ b/src/openvpn/ssl_pkt.c
@@ -434,7 +434,10 @@ tls_reset_standalone(struct tls_wrap_ctx *ctx,
                      uint8_t header,
                      bool request_resend_wkc)
 {
-    struct buffer buf = alloc_buf(tas->frame.buf.payload_size);
+    /* Copy buffer here to point at the same data but allow tls_wrap_control
+     * to potentially change buf to point to another buffer without
+     * modifying the buffer in tas */
+    struct buffer buf = tas->workbuf;
     ASSERT(buf_init(&buf, tas->frame.buf.headroom));
 
     /* Reliable ACK structure */
@@ -461,7 +464,8 @@ tls_reset_standalone(struct tls_wrap_ctx *ctx,
         buf_write_u16(&buf, EARLY_NEG_FLAG_RESEND_WKC);
     }
 
-    /* Add tls-auth/tls-crypt wrapping, this might replace buf */
+    /* Add tls-auth/tls-crypt wrapping, this might replace buf with
+     * ctx->work */
     tls_wrap_control(ctx, header, &buf, own_sid);
 
     return buf;

diff --git a/src/openvpn/ssl_pkt.h b/src/openvpn/ssl_pkt.h
index ec7b48daf66dbec7e8ad47ac4d7844aeb18b8978..ef4852e5d7086da8eafda1f665b3cf1b5bd5b633 100644 (file)
--- a/src/openvpn/ssl_pkt.h
+++ b/src/openvpn/ssl_pkt.h
@@ -77,6 +77,7 @@
 struct tls_auth_standalone
 {
     struct tls_wrap_ctx tls_wrap;
+    struct buffer workbuf;
     struct frame frame;
 };
 
@@ -220,7 +221,6 @@ read_control_auth(struct buffer *buf,
  * This function creates a reset packet using the information
  * from the tls pre decrypt state.
  *
- * The returned buf needs to be free with \c free_buf
  */
 struct buffer
 tls_reset_standalone(struct tls_wrap_ctx *ctx,

diff --git a/tests/unit_tests/openvpn/test_pkt.c b/tests/unit_tests/openvpn/test_pkt.c
index f11e52a1101c7d2e87748be5356e9ce8cab083c1..736f13178595a5df13faa827080be7da042c72e7 100644 (file)
--- a/tests/unit_tests/openvpn/test_pkt.c
+++ b/tests/unit_tests/openvpn/test_pkt.c
@@ -203,6 +203,7 @@ init_tas_auth(int key_direction)
                             static_key, true, key_direction,
                             "Control Channel Authentication", "tls-auth",
                             NULL);
+    tas.workbuf = alloc_buf(1600);
 
     return tas;
 }
@@ -217,10 +218,22 @@ init_tas_crypt(bool server)
     tls_crypt_init_key(&tas.tls_wrap.opt.key_ctx_bi,
                        &tas.tls_wrap.original_wrap_keydata, static_key,
                        true, server);
+    tas.workbuf = alloc_buf(1600);
+    tas.tls_wrap.work = alloc_buf(1600);
 
     return tas;
 }
 
+void
+free_tas(struct tls_auth_standalone *tas)
+{
+    /* Not some of these might be null pointers but calling free on null
+     * pointers is a noop */
+    free_key_ctx_bi(&tas->tls_wrap.opt.key_ctx_bi);
+    free_buf(&tas->workbuf);
+    free_buf(&tas->tls_wrap.work);
+}
+
 void
 test_tls_decrypt_lite_crypt(void **ut_state)
 {
@@ -228,7 +241,6 @@ test_tls_decrypt_lite_crypt(void **ut_state)
     struct tls_pre_decrypt_state state = { 0 };
 
     struct tls_auth_standalone tas = init_tas_crypt(true);
-
     struct buffer buf = alloc_buf(1024);
 
     /* tls-auth should be invalid */
@@ -263,6 +275,7 @@ test_tls_decrypt_lite_crypt(void **ut_state)
     }
 
     free_key_ctx_bi(&tas.tls_wrap.opt.key_ctx_bi);
+    free_tas(&tas);
     free_buf(&buf);
 }
 
@@ -318,6 +331,7 @@ test_tls_decrypt_lite_auth(void **ut_state)
     free_tls_pre_decrypt_state(&state);
     /* Wrong key direction gives a wrong hmac key and should not validate */
     free_key_ctx_bi(&tas.tls_wrap.opt.key_ctx_bi);
+    free_tas(&tas);
     tas = init_tas_auth(KEY_DIRECTION_INVERSE);
 
     buf_reset_len(&buf);
@@ -326,7 +340,7 @@ test_tls_decrypt_lite_auth(void **ut_state)
     assert_int_equal(verdict, VERDICT_INVALID);
 
     free_tls_pre_decrypt_state(&state);
-    free_key_ctx_bi(&tas.tls_wrap.opt.key_ctx_bi);
+    free_tas(&tas);
     free_buf(&buf);
 }
 
@@ -371,6 +385,7 @@ test_tls_decrypt_lite_none(void **ut_state)
 
     free_tls_pre_decrypt_state(&state);
     free_buf(&buf);
+    free_tas(&tas);
 }
 
 static void
@@ -442,10 +457,9 @@ test_verify_hmac_tls_auth(void **ut_state)
     bool valid = check_session_id_hmac(&state, &from.dest, hmac, 30);
     assert_false(valid);
 
-    free_key_ctx_bi(&tas.tls_wrap.opt.key_ctx_bi);
-    free_key_ctx(&tas.tls_wrap.tls_crypt_v2_server_key);
     free_tls_pre_decrypt_state(&state);
     free_buf(&buf);
+    free_tas(&tas);
     hmac_ctx_cleanup(hmac);
     hmac_ctx_free(hmac);
 }
@@ -563,6 +577,7 @@ test_generate_reset_packet_plain(void **ut_state)
     tas.tls_wrap.mode = TLS_WRAP_NONE;
     struct frame frame = { {.headroom = 200, .payload_size = 1400}, 0};
     tas.frame = frame;
+    tas.workbuf = alloc_buf(1600);
 
     uint8_t header = 0 | (P_CONTROL_HARD_RESET_CLIENT_V2 << P_OPCODE_SHIFT);
 
@@ -577,10 +592,9 @@ test_generate_reset_packet_plain(void **ut_state)
     struct buffer buf2 = tls_reset_standalone(&tas.tls_wrap, &tas, &client_id, &server_id, header, false);
     assert_int_equal(BLEN(&buf), BLEN(&buf2));
     assert_memory_equal(BPTR(&buf), BPTR(&buf2), BLEN(&buf));
-    free_buf(&buf2);
 
     free_tls_pre_decrypt_state(&state);
-    free_buf(&buf);
+    free_buf(&tas.workbuf);
 }
 
 static void
@@ -614,15 +628,12 @@ test_generate_reset_packet_tls_auth(void **ut_state)
     assert_int_equal(BLEN(&buf), BLEN(&buf2));
     assert_memory_equal(BPTR(&buf), BPTR(&buf2), BLEN(&buf));
 
-    free_buf(&buf2);
     free_tls_pre_decrypt_state(&state);
 
     packet_id_free(&tas_client.tls_wrap.opt.packet_id);
 
-    free_buf(&buf);
-    free_key_ctx_bi(&tas_server.tls_wrap.opt.key_ctx_bi);
-    free_key_ctx_bi(&tas_client.tls_wrap.opt.key_ctx_bi);
-
+    free_tas(&tas_client);
+    free_tas(&tas_server);
 }
 
 int