diff mbox series

[Openvpn-devel,v2] Fix memory leaks in HMAC initial packet id and dco open tun

Message ID 20230313134233.60335-1-arne@rfc2549.org
State Rejected
Headers
Series [Openvpn-devel,v2] Fix memory leaks in HMAC initial packet id and dco open tun |

Commit Message

Arne Schwabe March 13, 2023, 1:42 p.m. UTC 
  The open_tun_dco_generic already allocates the actual_name string, this
shadows the allocation in the FreeBSD/Linux specific methods.

The HMAC leaks are just forgotten frees/deinitialisations.

Found-By: clang with asan

Patch v2: rebase. Include linux bits accidentially forgotten.

Change-Id: I3c344af047abe94c0178bde1781eb450f10d157d
Signed-off-by: Arne Schwabe <arne@rfc2549.org>
---
 src/openvpn/dco_freebsd.c |  1 -
 src/openvpn/dco_linux.c   |  1 -
 src/openvpn/init.c        |  2 ++
 src/openvpn/mudp.c        |  1 +
 src/openvpn/ssl.c         | 11 +++++++++++
 src/openvpn/ssl.h         |  6 ++++++
 6 files changed, 20 insertions(+), 2 deletions(-)

Comments

Gert Doering March 13, 2023, 6:14 p.m. UTC | #1 
Hi,

On Mon, Mar 13, 2023 at 02:42:33PM +0100, Arne Schwabe wrote:
> The open_tun_dco_generic already allocates the actual_name string, this
> shadows the allocation in the FreeBSD/Linux specific methods.
> 
> The HMAC leaks are just forgotten frees/deinitialisations.
> 
> Found-By: clang with asan
> 
> Patch v2: rebase. Include linux bits accidentially forgotten.
> 
> Change-Id: I3c344af047abe94c0178bde1781eb450f10d157d
> Signed-off-by: Arne Schwabe <arne@rfc2549.org>

NAK, though I'm not sure I really understand why.

The free_buf() call fails on a server instance with --tls-crypt + 
--tls-crypt-v2, because "buf" is modified by tls_wrap_control() in
this case.

Sprinkled-in msg() calls show that "buf.data" points elsewhere 
after the call, and then free_buf() fails

2023-03-13 19:13:18 us=537725 Initialization Sequence Completed
2023-03-13 19:13:20 us=782049 GERT: in tls_reset_standalone, &buf=0x7ffca7010ef0, buf.data=0x562bd5669370
2023-03-13 19:13:20 us=782103 GERT: tls_reset_standalone before tls_wrap_control(), &buf=0x7ffca7010ef0, buf.data=0x562bd5669370
2023-03-13 19:13:20 us=782123 GERT: at end of tls_reset_standalone, &buf=0x7ffca7010ef0, buf.data=0x562bd565dcc8
2023-03-13 19:13:20 us=782140 GERT: in send_hmac_reset_packet, &buf=0x7ffca7010f60, buf.data=0x562bd565dcc8
free(): invalid pointer
Aborted


The tt->actual changes are fine, and the tls_auth_standalone change 
also looks good (if complicated to grok).

This code here is fine for "naked" and "tls-auth".

gert
diff mbox series

Patch

diff --git a/src/openvpn/dco_freebsd.c b/src/openvpn/dco_freebsd.c
index 92de5f04b..e605f2a9d 100644
--- a/src/openvpn/dco_freebsd.c
+++ b/src/openvpn/dco_freebsd.c
@@ -230,7 +230,6 @@  create_interface(struct tuntap *tt, const char *dev)
     }
 
     snprintf(tt->dco.ifname, IFNAMSIZ, "%s", ifr.ifr_data);
-    tt->actual_name = string_alloc(tt->dco.ifname, NULL);
 
     /* see "Interface Flags" in ifnet(9) */
     int i = IFF_POINTOPOINT | IFF_MULTICAST;
diff --git a/src/openvpn/dco_linux.c b/src/openvpn/dco_linux.c
index 2b349529f..0f5fc48d9 100644
--- a/src/openvpn/dco_linux.c
+++ b/src/openvpn/dco_linux.c
@@ -457,7 +457,6 @@  open_tun_dco(struct tuntap *tt, openvpn_net_ctx_t *ctx, const char *dev)
         msg(M_FATAL, "DCO: cannot retrieve ifindex for interface %s", dev);
     }
 
-    tt->actual_name = string_alloc(dev, NULL);
     tt->dco.dco_message_peer_id = -1;
 
     ovpn_dco_register(&tt->dco);
diff --git a/src/openvpn/init.c b/src/openvpn/init.c
index 124ac76bd..e59edd742 100644
--- a/src/openvpn/init.c
+++ b/src/openvpn/init.c
@@ -3881,6 +3881,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/mudp.c b/src/openvpn/mudp.c
index 8698aefc8..813160639 100644
--- a/src/openvpn/mudp.c
+++ b/src/openvpn/mudp.c
@@ -61,6 +61,7 @@  send_hmac_reset_packet(struct multi_context *m,
     m->hmac_reply = c->c2.buffers->aux_buf;
     m->hmac_reply_dest = &m->top.c2.from;
     msg(D_MULTI_DEBUG, "Reset packet from client, sending HMAC based reset challenge");
+    free_buf(&buf);
 }
 
 
diff --git a/src/openvpn/ssl.c b/src/openvpn/ssl.c
index 78cec90a1..fe6390fad 100644
--- 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 58ff4b9b4..a050cd5c9 100644
--- 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