diff mbox series

[Openvpn-devel,v3] options: fix use-after-free of DNS options on client connect

Message ID 20260622120856.21586-1-gert@greenie.muc.de
State New
Headers
Series [Openvpn-devel,v3] options: fix use-after-free of DNS options on client connect |

Commit Message

Gert Doering June 22, 2026, 12:08 p.m. UTC 
  From: Antonio Quartulli <antonio@mandelbit.com>

struct dns_options embeds its own gc_arena. When inherit_context_child()
/inherit_context_top() copy struct options by value, the child shares the
parent's DNS arena. options_detach() detached o->gc but not
o->dns_options.gc, so pre_connect_restore()'s gc_free() (and context
teardown) freed allocations the parent still referenced.

With one or more non-pushed --dhcp-option directives that yield a DNS
entry, a connecting client triggers this and the server crashes
(use-after-free in setenv_dns_options(), reported as a double free).

Detach o->dns_options.gc as well, mirroring the existing o->gc handling.

Change-Id: I49b37b5a90554fa2d4a83c8fc5608dad2a36b835
GitHub: closes openvpn/OpenVPN#1060
Signed-off-by: Antonio Quartulli <antonio@mandelbit.com>
Acked-by: Arne Schwabe <arne-openvpn@rfc2549.org>
Gerrit URL: https://gerrit.openvpn.net/c/openvpn/+/1715
---

This change was reviewed on Gerrit and approved by at least one
developer. I request to merge it to master.

Gerrit URL: https://gerrit.openvpn.net/c/openvpn/+/1715
This mail reflects revision 3 of this Change.

Acked-by according to Gerrit (reflected above):
Arne Schwabe <arne-openvpn@rfc2549.org>

Comments

Gert Doering June 22, 2026, 2:41 p.m. UTC | #1 
Thanks for digging into this, and thanks Arne for the review - all these
GC-and-option-copy-magic is not exactly where I feel safe.  Anyway, this
said, I can confirm that it fixed the issue on OpenBSD, and passes my
t_server setup (which has multiple clients connecting...) just fine.

Your patch has been applied to the master and release/2.7 branch (bugfix).

The offending code is quite different in 2.6 (no copying around of
DNS and DHCP options) so I think the bugfix is not needed - at least I could
not reproduce the double-free()-assert on OpenBSD.

commit 97ec63372ab354ad48c89e73d1e37715679370ba (master)
commit 40a3bc951f64c6afe69c55afff9ff7a855b69097 (release/2.7)
Author: Antonio Quartulli
Date:   Mon Jun 22 14:08:51 2026 +0200

     options: fix use-after-free of DNS options on client connect

     Signed-off-by: Antonio Quartulli <antonio@mandelbit.com>
     Acked-by: Arne Schwabe <arne-openvpn@rfc2549.org>
     Gerrit URL: https://gerrit.openvpn.net/c/openvpn/+/1715
     Message-Id: <20260622120856.21586-1-gert@greenie.muc.de>
     URL: https://www.mail-archive.com/openvpn-devel@lists.sourceforge.net/msg37230.html
     Signed-off-by: Gert Doering <gert@greenie.muc.de>


--
kind regards,

Gert Doering
Gert Doering June 23, 2026, 9:36 a.m. UTC | #2 
Hi,

On Mon, Jun 22, 2026 at 04:41:31PM +0200, Gert Doering wrote:
> Thanks for digging into this, and thanks Arne for the review - all these
> GC-and-option-copy-magic is not exactly where I feel safe.  Anyway, this
> said, I can confirm that it fixed the issue on OpenBSD, and passes my
> t_server setup (which has multiple clients connecting...) just fine.
> 
> Your patch has been applied to the master and release/2.7 branch (bugfix).
> 
> The offending code is quite different in 2.6 (no copying around of
> DNS and DHCP options) so I think the bugfix is not needed - at least I could
> not reproduce the double-free()-assert on OpenBSD.

As it turns out, the bug is in 2.6 as well, and it can be reproduced
there as well, but needs different config ("dns" instead of "dhcp-option"
in the server config).  Crash without the patch, no crash with the patch.

> commit 97ec63372ab354ad48c89e73d1e37715679370ba (master)
> commit 40a3bc951f64c6afe69c55afff9ff7a855b69097 (release/2.7)

commit 14d761c8bedde3fd7c45e355556ed8a1883f9ca7 (release/2.6)

gert
diff mbox series

Patch

diff --git a/src/openvpn/options.c b/src/openvpn/options.c
index 0c2866c..75bd87c 100644
--- a/src/openvpn/options.c
+++ b/src/openvpn/options.c
@@ -1531,7 +1531,18 @@ 
 void
 options_detach(struct options *o)
 {
+    /* The options struct carries two gc_arena's (one generic and one specific
+     * to the DNS settings), which the by-value options
+     * copy in inherit_context_child()/inherit_context_top() shares with the
+     * source.
+     *
+     * Detach both (i.e. re-initialize them), otherwise child's call of
+     * gc_free() (or context teardown) would free allocations the source
+     * context still references, leading to a use-after-free (and subsequent
+     * double-free).
+     */
     gc_detach(&o->gc);
+    gc_detach(&o->dns_options.gc);
     o->routes = NULL;
     o->client_nat = NULL;
     clone_push_list(o);