openvpn-devel — OpenVPN developers list

[Openvpn-devel] [S] Change in openvpn[master]: Fix logic when pushed cipher triggers tun reopen and ignore more options

[cron2 (C. Review) <ge...@op...>]

cron2 has submitted this change. ( http://gerrit.openvpn.net/c/openvpn/+/1321?usp=email )

Change subject: Fix logic when pushed cipher triggers tun reopen and ignore more options
......................................................................

Fix logic when pushed cipher triggers tun reopen and ignore more options

The logic was inverted. Only when link-mtu is used, pushing a cipher can
change the MTU and not the other way round. (found by zeropath)

Also ignore a few more options that should not trigger a reopen of tun
in push message.

Reported-by: Joshua Rogers <co...@jo...>
Found-by: ZeroPath (https://zeropath.com/)

Change-Id: I76eb584024610a6054a069340adbac988abf686c
Signed-off-by: Arne Schwabe <arn...@rf...>
Acked-by: Gert Doering <ge...@gr...>
Gerrit URL: https://gerrit.openvpn.net/c/openvpn/+/1321
Message-Id: <202...@gr...>
URL: https://www.mail-archive.com/ope...@li.../msg33989.html
Signed-off-by: Gert Doering <ge...@gr...>
---
M src/openvpn/push.c
1 file changed, 14 insertions(+), 4 deletions(-)




diff --git a/src/openvpn/push.c b/src/openvpn/push.c
index 6f146fc..7852d36 100644
--- a/src/openvpn/push.c
+++ b/src/openvpn/push.c
@@ -1029,15 +1029,25 @@
     char line[OPTION_PARM_SIZE];
     while (buf_parse(buf, ',', line, sizeof(line)))
     {
-        /* peer-id and auth-token might change on restart and this should not trigger reopening tun
+        /* peer-id and auth-token might change on restart and this should not
+         * trigger reopening tun
+         * Also other options that only affect the control channel should
+         * not trigger a reopen of the tun device
          */
-        if (strprefix(line, "peer-id ") || strprefix(line, "auth-token ")
-            || strprefix(line, "auth-token-user "))
+        if (strprefix(line, "peer-id ")
+            || strprefix(line, "auth-token ")
+            || strprefix(line, "auth-token-user")
+            || strprefix(line, "protocol-flags ")
+            || strprefix(line, "key-derivation ")
+            || strprefix(line, "explicit-exit-notify ")
+            || strprefix(line, "ping ")
+            || strprefix(line, "ping-restart ")
+            || strprefix(line, "ping-timer "))
         {
             continue;
         }
         /* tun reopen only needed if cipher change can change tun MTU */
-        if (strprefix(line, "cipher ") && !opt->ce.tun_mtu_defined)
+        if (strprefix(line, "cipher ") && opt->ce.tun_mtu_defined)
         {
             continue;
         }

-- 
To view, visit http://gerrit.openvpn.net/c/openvpn/+/1321?usp=email
To unsubscribe, or for help writing mail filters, visit http://gerrit.openvpn.net/settings?usp=email

Gerrit-MessageType: merged
Gerrit-Project: openvpn
Gerrit-Branch: master
Gerrit-Change-Id: I76eb584024610a6054a069340adbac988abf686c
Gerrit-Change-Number: 1321
Gerrit-PatchSet: 2
Gerrit-Owner: plaisthos <arn...@rf...>
Gerrit-Reviewer: cron2 <ge...@gr...>
Gerrit-Reviewer: flichtenheld <fr...@li...>
Gerrit-CC: openvpn-devel <ope...@li...>

[Openvpn-devel] [S] Change in openvpn[master]: Fix logic when pushed cipher triggers tun reopen and ignore more options

[cron2 (C. Review) <ge...@op...>]

cron2 has uploaded a new patch set (#2) to the change originally created by plaisthos. ( http://gerrit.openvpn.net/c/openvpn/+/1321?usp=email )

The following approvals got outdated and were removed:
Code-Review+2 by cron2


Change subject: Fix logic when pushed cipher triggers tun reopen and ignore more options
......................................................................

Fix logic when pushed cipher triggers tun reopen and ignore more options

The logic was inverted. Only when link-mtu is used, pushing a cipher can
change the MTU and not the other way round. (found by zeropath)

Also ignore a few more options that should not trigger a reopen of tun
in push message.

Reported-by: Joshua Rogers <co...@jo...>
Found-by: ZeroPath (https://zeropath.com/)

Change-Id: I76eb584024610a6054a069340adbac988abf686c
Signed-off-by: Arne Schwabe <arn...@rf...>
Acked-by: Gert Doering <ge...@gr...>
Gerrit URL: https://gerrit.openvpn.net/c/openvpn/+/1321
Message-Id: <202...@gr...>
URL: https://www.mail-archive.com/ope...@li.../msg33989.html
Signed-off-by: Gert Doering <ge...@gr...>
---
M src/openvpn/push.c
1 file changed, 14 insertions(+), 4 deletions(-)


  git pull ssh://gerrit.openvpn.net:29418/openvpn refs/changes/21/1321/2

diff --git a/src/openvpn/push.c b/src/openvpn/push.c
index 6f146fc..7852d36 100644
--- a/src/openvpn/push.c
+++ b/src/openvpn/push.c
@@ -1029,15 +1029,25 @@
     char line[OPTION_PARM_SIZE];
     while (buf_parse(buf, ',', line, sizeof(line)))
     {
-        /* peer-id and auth-token might change on restart and this should not trigger reopening tun
+        /* peer-id and auth-token might change on restart and this should not
+         * trigger reopening tun
+         * Also other options that only affect the control channel should
+         * not trigger a reopen of the tun device
          */
-        if (strprefix(line, "peer-id ") || strprefix(line, "auth-token ")
-            || strprefix(line, "auth-token-user "))
+        if (strprefix(line, "peer-id ")
+            || strprefix(line, "auth-token ")
+            || strprefix(line, "auth-token-user")
+            || strprefix(line, "protocol-flags ")
+            || strprefix(line, "key-derivation ")
+            || strprefix(line, "explicit-exit-notify ")
+            || strprefix(line, "ping ")
+            || strprefix(line, "ping-restart ")
+            || strprefix(line, "ping-timer "))
         {
             continue;
         }
         /* tun reopen only needed if cipher change can change tun MTU */
-        if (strprefix(line, "cipher ") && !opt->ce.tun_mtu_defined)
+        if (strprefix(line, "cipher ") && opt->ce.tun_mtu_defined)
         {
             continue;
         }

-- 
To view, visit http://gerrit.openvpn.net/c/openvpn/+/1321?usp=email
To unsubscribe, or for help writing mail filters, visit http://gerrit.openvpn.net/settings?usp=email

Gerrit-MessageType: newpatchset
Gerrit-Project: openvpn
Gerrit-Branch: master
Gerrit-Change-Id: I76eb584024610a6054a069340adbac988abf686c
Gerrit-Change-Number: 1321
Gerrit-PatchSet: 2
Gerrit-Owner: plaisthos <arn...@rf...>
Gerrit-Reviewer: cron2 <ge...@gr...>
Gerrit-Reviewer: flichtenheld <fr...@li...>
Gerrit-CC: openvpn-devel <ope...@li...>

[Openvpn-devel] [PATCH applied] Re: Fix logic when pushed cipher triggers tun reopen and ignore more options

[Gert D. <ge...@gr...>]

So there was a bit of discussion about this in gerrit, and the logic is
confusing and twisted.  The tun device needs to reopened on a cipher
change *if* the tun-mtu ("ifconfig tun0 mtu 1400") could change - and
if the config has tun_mtu_defined, the tun MTU is fixed, so the cipher
could be ignored.  I think there are still funny edge cases (like
"no tun-mtu or link-mtu defined", what then?) and the check maybe should
be double-checked for "if (!opt->ce.link_mtu_defined)" - because
"link-mtu <set>" is the trigger for "tun mtu <calculated>", so in that
case we can't skip this.

OTOH, this is a small edge of an edge case, namely "cipher *change* upon
reconnect", which is rare enough - so for this to make a real difference
you need --user nobody or windows-dco (so "reopen tun" would fail) *and*
a cipher change.

I have changed the Reported-by:/Found-by: tags to look "like on the last
few commits", so it's consistent (basically adding URL and full name).

Your patch has been applied to the master branch.

commit 911a69dc1af20bc54557a208b6fd4e76261b2bb2
Author: Arne Schwabe
Date:   Wed Oct 29 07:53:10 2025 +0100

     Fix logic when pushed cipher triggers tun reopen and ignore more options

     Signed-off-by: Arne Schwabe <arn...@rf...>
     Acked-by: Gert Doering <ge...@gr...>
     Gerrit URL: https://gerrit.openvpn.net/c/openvpn/+/1321
     Message-Id: <202...@gr...>
     URL: https://www.mail-archive.com/ope...@li.../msg33989.html
     Signed-off-by: Gert Doering <ge...@gr...>


--
kind regards,

Gert Doering

[Openvpn-devel] [XS] Change in openvpn[master]: zeroize struct image in packet_id_persist_save() before writing to disk

[cron2 (C. Review) <ge...@op...>]

Hello flichtenheld, plaisthos, 

I'd like you to reexamine a change. Please visit

    http://gerrit.openvpn.net/c/openvpn/+/1323?usp=email

to look at the new patch set (#2).

The following approvals got outdated and were removed:
Code-Review+2 by flichtenheld


Change subject: zeroize struct image in packet_id_persist_save() before writing to disk
......................................................................

zeroize struct image in packet_id_persist_save() before writing to disk

while this really is only a debug function, ensuring that no uninitialized
heap content ends up in padding in the structure and thus to disk is good
practice.

Reported-by: Joshua Rogers <co...@jo...>
Found-by: ZeroPath (https://zeropath.com/)

Change-Id: I7f4c7b0ca748975defca1e5104e7077a761cd49c
Signed-off-by: Gert Doering <ge...@gr...>
Acked-by: Frank Lichtenheld <fr...@li...>
Gerrit URL: https://gerrit.openvpn.net/c/openvpn/+/1323
Message-Id: <202...@gr...>
URL: https://www.mail-archive.com/ope...@li.../msg33983.html
Signed-off-by: Gert Doering <ge...@gr...>
---
M src/openvpn/packet_id.c
1 file changed, 1 insertion(+), 0 deletions(-)


  git pull ssh://gerrit.openvpn.net:29418/openvpn refs/changes/23/1323/2

diff --git a/src/openvpn/packet_id.c b/src/openvpn/packet_id.c
index 880eee1..08d9d9b 100644
--- a/src/openvpn/packet_id.c
+++ b/src/openvpn/packet_id.c
@@ -511,6 +511,7 @@
         && (p->time != p->time_last_written || p->id != p->id_last_written))
     {
         struct packet_id_persist_file_image image;
+        CLEAR(image);
         ssize_t n;
         off_t seek_ret;
         struct gc_arena gc = gc_new();

-- 
To view, visit http://gerrit.openvpn.net/c/openvpn/+/1323?usp=email
To unsubscribe, or for help writing mail filters, visit http://gerrit.openvpn.net/settings?usp=email

Gerrit-MessageType: newpatchset
Gerrit-Project: openvpn
Gerrit-Branch: master
Gerrit-Change-Id: I7f4c7b0ca748975defca1e5104e7077a761cd49c
Gerrit-Change-Number: 1323
Gerrit-PatchSet: 2
Gerrit-Owner: cron2 <ge...@gr...>
Gerrit-Reviewer: flichtenheld <fr...@li...>
Gerrit-Reviewer: plaisthos <arn...@rf...>
Gerrit-CC: openvpn-devel <ope...@li...>

[Openvpn-devel] [XS] Change in openvpn[master]: zeroize struct image in packet_id_persist_save() before writing to disk

[cron2 (C. Review) <ge...@op...>]

cron2 has submitted this change. ( http://gerrit.openvpn.net/c/openvpn/+/1323?usp=email )

Change subject: zeroize struct image in packet_id_persist_save() before writing to disk
......................................................................

zeroize struct image in packet_id_persist_save() before writing to disk

while this really is only a debug function, ensuring that no uninitialized
heap content ends up in padding in the structure and thus to disk is good
practice.

Reported-by: Joshua Rogers <co...@jo...>
Found-by: ZeroPath (https://zeropath.com/)

Change-Id: I7f4c7b0ca748975defca1e5104e7077a761cd49c
Signed-off-by: Gert Doering <ge...@gr...>
Acked-by: Frank Lichtenheld <fr...@li...>
Gerrit URL: https://gerrit.openvpn.net/c/openvpn/+/1323
Message-Id: <202...@gr...>
URL: https://www.mail-archive.com/ope...@li.../msg33983.html
Signed-off-by: Gert Doering <ge...@gr...>
---
M src/openvpn/packet_id.c
1 file changed, 1 insertion(+), 0 deletions(-)




diff --git a/src/openvpn/packet_id.c b/src/openvpn/packet_id.c
index 880eee1..08d9d9b 100644
--- a/src/openvpn/packet_id.c
+++ b/src/openvpn/packet_id.c
@@ -511,6 +511,7 @@
         && (p->time != p->time_last_written || p->id != p->id_last_written))
     {
         struct packet_id_persist_file_image image;
+        CLEAR(image);
         ssize_t n;
         off_t seek_ret;
         struct gc_arena gc = gc_new();

-- 
To view, visit http://gerrit.openvpn.net/c/openvpn/+/1323?usp=email
To unsubscribe, or for help writing mail filters, visit http://gerrit.openvpn.net/settings?usp=email

Gerrit-MessageType: merged
Gerrit-Project: openvpn
Gerrit-Branch: master
Gerrit-Change-Id: I7f4c7b0ca748975defca1e5104e7077a761cd49c
Gerrit-Change-Number: 1323
Gerrit-PatchSet: 2
Gerrit-Owner: cron2 <ge...@gr...>
Gerrit-Reviewer: flichtenheld <fr...@li...>
Gerrit-Reviewer: plaisthos <arn...@rf...>
Gerrit-CC: openvpn-devel <ope...@li...>

[Openvpn-devel] [PATCH applied] Re: zeroize struct image in packet_id_persist_save() before writing to disk

[Gert D. <ge...@gr...>]

Not the most critical path, but "initialize data structures before using"
is considered good practice :-)  - BB is also happy with it.

Patch has been applied to the master branch.

commit c58b6e73c3508b40b3d2f26eebfc3aa4df53e524
Author: Gert Doering
Date:   Tue Oct 28 21:31:50 2025 +0100

     zeroize struct image in packet_id_persist_save() before writing to disk

     Signed-off-by: Gert Doering <ge...@gr...>
     Acked-by: Frank Lichtenheld <fr...@li...>
     Gerrit URL: https://gerrit.openvpn.net/c/openvpn/+/1323
     Message-Id: <202...@gr...>
     URL: https://www.mail-archive.com/ope...@li.../msg33983.html
     Signed-off-by: Gert Doering <ge...@gr...>


--
kind regards,

Gert Doering

[Openvpn-devel] [XS] Change in openvpn[master]: remove redundant PULL_DEFINED() macro definition

[cron2 (C. Review) <ge...@op...>]

cron2 has submitted this change. ( http://gerrit.openvpn.net/c/openvpn/+/1322?usp=email )

Change subject: remove redundant PULL_DEFINED() macro definition
......................................................................

remove redundant PULL_DEFINED() macro definition

this seems to be a leftover of the time when we had conditional
compilation for "--disable-server" or thus.  Commit d6a0cf599
removed PUSH_DEFINED() nearby but overlooked this one.

Change-Id: I9118333bb65cd5db0836abefa5d45a729f0142cc
Signed-off-by: Gert Doering <ge...@gr...>
Acked-by: Frank Lichtenheld <fr...@li...>
Gerrit URL: https://gerrit.openvpn.net/c/openvpn/+/1322
Message-Id: <202...@gr...>
URL: https://www.mail-archive.com/ope...@li.../msg33984.html
Signed-off-by: Gert Doering <ge...@gr...>
---
M src/openvpn/options.h
1 file changed, 0 insertions(+), 4 deletions(-)




diff --git a/src/openvpn/options.h b/src/openvpn/options.h
index 24253af..125e524 100644
--- a/src/openvpn/options.h
+++ b/src/openvpn/options.h
@@ -769,10 +769,6 @@
 
 #define PULL_DEFINED(opt) ((opt)->pull)
 
-#ifndef PULL_DEFINED
-#define PULL_DEFINED(opt) (false)
-#endif
-
 #ifdef _WIN32
 #define ROUTE_OPTION_FLAGS(o) ((o)->route_method & ROUTE_METHOD_MASK)
 #else

-- 
To view, visit http://gerrit.openvpn.net/c/openvpn/+/1322?usp=email
To unsubscribe, or for help writing mail filters, visit http://gerrit.openvpn.net/settings?usp=email

Gerrit-MessageType: merged
Gerrit-Project: openvpn
Gerrit-Branch: master
Gerrit-Change-Id: I9118333bb65cd5db0836abefa5d45a729f0142cc
Gerrit-Change-Number: 1322
Gerrit-PatchSet: 2
Gerrit-Owner: cron2 <ge...@gr...>
Gerrit-Reviewer: flichtenheld <fr...@li...>
Gerrit-Reviewer: plaisthos <arn...@rf...>
Gerrit-CC: openvpn-devel <ope...@li...>

[Openvpn-devel] [XS] Change in openvpn[master]: remove redundant PULL_DEFINED() macro definition

[cron2 (C. Review) <ge...@op...>]

Hello flichtenheld, plaisthos, 

I'd like you to reexamine a change. Please visit

    http://gerrit.openvpn.net/c/openvpn/+/1322?usp=email

to look at the new patch set (#2).

The following approvals got outdated and were removed:
Code-Review+2 by flichtenheld


Change subject: remove redundant PULL_DEFINED() macro definition
......................................................................

remove redundant PULL_DEFINED() macro definition

this seems to be a leftover of the time when we had conditional
compilation for "--disable-server" or thus.  Commit d6a0cf599
removed PUSH_DEFINED() nearby but overlooked this one.

Change-Id: I9118333bb65cd5db0836abefa5d45a729f0142cc
Signed-off-by: Gert Doering <ge...@gr...>
Acked-by: Frank Lichtenheld <fr...@li...>
Gerrit URL: https://gerrit.openvpn.net/c/openvpn/+/1322
Message-Id: <202...@gr...>
URL: https://www.mail-archive.com/ope...@li.../msg33984.html
Signed-off-by: Gert Doering <ge...@gr...>
---
M src/openvpn/options.h
1 file changed, 0 insertions(+), 4 deletions(-)


  git pull ssh://gerrit.openvpn.net:29418/openvpn refs/changes/22/1322/2

diff --git a/src/openvpn/options.h b/src/openvpn/options.h
index 24253af..125e524 100644
--- a/src/openvpn/options.h
+++ b/src/openvpn/options.h
@@ -769,10 +769,6 @@
 
 #define PULL_DEFINED(opt) ((opt)->pull)
 
-#ifndef PULL_DEFINED
-#define PULL_DEFINED(opt) (false)
-#endif
-
 #ifdef _WIN32
 #define ROUTE_OPTION_FLAGS(o) ((o)->route_method & ROUTE_METHOD_MASK)
 #else

-- 
To view, visit http://gerrit.openvpn.net/c/openvpn/+/1322?usp=email
To unsubscribe, or for help writing mail filters, visit http://gerrit.openvpn.net/settings?usp=email

Gerrit-MessageType: newpatchset
Gerrit-Project: openvpn
Gerrit-Branch: master
Gerrit-Change-Id: I9118333bb65cd5db0836abefa5d45a729f0142cc
Gerrit-Change-Number: 1322
Gerrit-PatchSet: 2
Gerrit-Owner: cron2 <ge...@gr...>
Gerrit-Reviewer: flichtenheld <fr...@li...>
Gerrit-Reviewer: plaisthos <arn...@rf...>
Gerrit-CC: openvpn-devel <ope...@li...>

[Openvpn-devel] [PATCH applied] Re: remove redundant PULL_DEFINED() macro definition

[Gert D. <ge...@gr...>]

Trivial enough .-) and BB confirms that I have not overlooked some random
combination of platform and compile time directive.

Patch has been applied to the master branch.

commit e6ae2bc64239dd99a2d2a94e75fdaff45eb78deb
Author: Gert Doering
Date:   Tue Oct 28 21:32:10 2025 +0100

     remove redundant PULL_DEFINED() macro definition

     Signed-off-by: Gert Doering <ge...@gr...>
     Acked-by: Frank Lichtenheld <fr...@li...>
     Gerrit URL: https://gerrit.openvpn.net/c/openvpn/+/1322
     Message-Id: <202...@gr...>
     URL: https://www.mail-archive.com/ope...@li.../msg33984.html
     Signed-off-by: Gert Doering <ge...@gr...>


--
kind regards,

Gert Doering

[Openvpn-devel] [PATCH v14] Install host routes for ifconfig-push routes when DCO is enabled

[Gert D. <ge...@gr...>]

From: Arne Schwabe <ar...@rf...>

ifconfig-push and ifconfig-ipv6-push can configure the IP address of a
client. If this IP address lies inside the network that is configured
on the ovpn/tun device this works as expected as the routing table point to
the ovpn/tun interface. However, if the IP address
is outside that range, the IP packets are not forwarded to the ovpn/tun
interface.

This patch adds logic to add host routes for these
ifconfig-push/ifconfig-ipv6-push addresses to ensure that traffic for
these IP addresses is also directed to the VPN.

For Linux it is important that these extra routes are routes using scope link
rather than static since otherwise routes via these IP addresses, like
iroute, will not work. On FreeBSD we also use interface routes as works and
routes that target interfaces instead of IP addresses are less brittle.

Tested using a server with ccd:

   openvpn --server 10.33.0.0 255.255.192.0 --server-ipv6 fd00:f00f::1/64  --client-config-dir ~/ccd [...]

and a client with lwipvonpn and the following ccd file:

   iroute-ipv6 FD00:F00F:CAFE::1001/64
   ifconfig-ipv6-push FD00:F00F:D00D::77/64
   push "setenv-safe ifconfig_ipv6_local_2 FD00:F00F:CAFE::1001"
   push "setenv-safe ifconfig_ipv6_netbits_2 64"

   iroute 10.234.234.0 255.255.255.0
   ifconfig-push 10.11.12.13 255.255.255.0
   push "setenv-safe ifconfig_local_2 10.234.234.12"
   push "setenv-safe ifconfig_netmask_2 255.255.255.0"

This setups an ifconfig-push addresses outside the --server/--server-ipv6
network and additionally configures a iroute behind that client. The
setenv-safe configure lwipovpn to use that additional IP addresses to allow
testing via ping.

Windows behaves like the user space implementation. It does require these
special routes but instead (like user space) needs static routes to redirect
IP traffic for these IP addresses to the tunnel interface. E.g. in the example
above the server config needs to have:

   route 10.234.234.0 255.255.255.0
   route 10.11.12.0 255.255.255.0

   route-ipv6 FD00:F00F:CAFE::1001/64
   route-ipv6 FD00:F00F:D00D::77/64

Change-Id: I83295e00d1a756dfa44050b0a4493095fb050fff
Signed-off-by: Arne Schwabe <ar...@rf...>
Acked-by: Gert Doering <ge...@gr...>
Gerrit URL: https://gerrit.openvpn.net/c/openvpn/+/1192
---

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/+/1192
This mail reflects revision 14 of this Change.

Acked-by according to Gerrit (reflected above):
Gert Doering <ge...@gr...>

        
diff --git a/doc/man-sections/server-options.rst b/doc/man-sections/server-options.rst
index ccc1374..9c7fa46 100644
--- a/doc/man-sections/server-options.rst
+++ b/doc/man-sections/server-options.rst
@@ -314,6 +314,10 @@
   3.  Use ``--ifconfig-pool`` allocation for dynamic IP (last
       choice).
 
+  When DCO is enabled and the IP is not in contained in the network specified
+  by ``--ifconfig``, OpenVPN will install a /32 host route for the ``local``
+  IP address.
+
 --ifconfig-ipv6-push args
   for ``--client-config-dir`` per-client static IPv6 interface
   configuration, see ``--client-config-dir`` and ``--ifconfig-push`` for
@@ -324,6 +328,10 @@
 
      ifconfig-ipv6-push ipv6addr/bits ipv6remote
 
+  When DCO is enabled and the IP is not in contained in the network specified
+  by ``--ifconfig-ipv6``, OpenVPN will install a /128 host route for the
+  ``ipv6addr`` IP address.
+
 --multihome
   Configure a multi-homed UDP server. This option needs to be used when a
   server has more than one IP address (e.g. multiple interfaces, or
diff --git a/src/openvpn/dco.c b/src/openvpn/dco.c
index 8fb4662..7abdad3 100644
--- a/src/openvpn/dco.c
+++ b/src/openvpn/dco.c
@@ -664,6 +664,14 @@
         return;
     }
 
+#if defined(_WIN32)
+    if (addr->type & MR_ONLINK_DCO_ADDR)
+    {
+        /* Windows does not need these extra routes, so we ignore/skip them */
+        return;
+    }
+#endif
+
     struct context *c = &mi->context;
     if (addrtype == MR_ADDR_IPV6)
     {
@@ -671,8 +679,14 @@
         dco_win_add_iroute_ipv6(&c->c1.tuntap->dco, addr->v6.addr, addr->netbits,
                                 c->c2.tls_multi->peer_id);
 #else
+        const struct in6_addr *gateway = &mi->context.c2.push_ifconfig_ipv6_local;
+        if (addr->type & MR_ONLINK_DCO_ADDR)
+        {
+            gateway = NULL;
+        }
+
         net_route_v6_add(&m->top.net_ctx, &addr->v6.addr, addr->netbits,
-                         &mi->context.c2.push_ifconfig_ipv6_local, c->c1.tuntap->actual_name, 0,
+                         gateway, c->c1.tuntap->actual_name, 0,
                          DCO_IROUTE_METRIC);
 #endif
     }
@@ -683,7 +697,13 @@
                                 c->c2.tls_multi->peer_id);
 #else
         in_addr_t dest = htonl(addr->v4.addr);
-        net_route_v4_add(&m->top.net_ctx, &dest, addr->netbits, &mi->context.c2.push_ifconfig_local,
+        const in_addr_t *gateway = &mi->context.c2.push_ifconfig_local;
+        if (addr->type & MR_ONLINK_DCO_ADDR)
+        {
+            gateway = NULL;
+        }
+
+        net_route_v4_add(&m->top.net_ctx, &dest, addr->netbits, gateway,
                          c->c1.tuntap->actual_name, 0, DCO_IROUTE_METRIC);
 #endif
     }
@@ -714,6 +734,20 @@
                              DCO_IROUTE_METRIC);
 #endif
         }
+
+#if !defined(_WIN32)
+        /* Check if we added a host route as the assigned client IP address was
+         * not in the on link scope defined by --ifconfig */
+        in_addr_t ifconfig_local = mi->context.c2.push_ifconfig_local;
+
+        if (multi_check_push_ifconfig_extra_route(mi, htonl(ifconfig_local)))
+        {
+            /* On windows we do not install these routes, so we also do not need to delete them */
+            net_route_v4_del(&m->top.net_ctx, &ifconfig_local,
+                             32, NULL, c->c1.tuntap->actual_name, 0,
+                             DCO_IROUTE_METRIC);
+        }
+#endif
     }
 
     if (mi->context.c2.push_ifconfig_ipv6_defined)
@@ -728,6 +762,18 @@
                              DCO_IROUTE_METRIC);
 #endif
         }
+
+        /* Checked if we added a host route as the assigned client IP address was
+         * outside the --ifconfig-ipv6 tun interface config */
+#if !defined(_WIN32)
+        struct in6_addr *dest = &mi->context.c2.push_ifconfig_ipv6_local;
+        if (multi_check_push_ifconfig_ipv6_extra_route(mi, dest))
+        {
+            /* On windows we do not install these routes, so we also do not need to delete them */
+            net_route_v6_del(&m->top.net_ctx, dest, 128, NULL,
+                             c->c1.tuntap->actual_name, 0, DCO_IROUTE_METRIC);
+        }
+#endif
     }
 #endif /* if defined(TARGET_LINUX) || defined(TARGET_FREEBSD) || defined(_WIN32) */
 }
diff --git a/src/openvpn/mroute.h b/src/openvpn/mroute.h
index 5b0c694..afd2e6c 100644
--- a/src/openvpn/mroute.h
+++ b/src/openvpn/mroute.h
@@ -20,6 +20,7 @@
  *  with this program; if not, see <https://www.gnu.org/licenses/>.
  */
 
+
 #ifndef MROUTE_H
 #define MROUTE_H
 
@@ -74,6 +75,9 @@
 /* Address type mask indicating that proto # is part of address */
 #define MR_WITH_PROTO 32
 
+/* MRoute is an on link/scope address needed for DCO on Unix platforms */
+#define MR_ONLINK_DCO_ADDR 64
+
 struct mroute_addr
 {
     uint8_t len;     /* length of address */
diff --git a/src/openvpn/multi.c b/src/openvpn/multi.c
index f60944d..ba35556 100644
--- a/src/openvpn/multi.c
+++ b/src/openvpn/multi.c
@@ -42,6 +42,7 @@
 #include "ssl_ncp.h"
 #include "vlan.h"
 #include "auth_token.h"
+#include "route.h"
 #include <inttypes.h>
 #include <string.h>
 
@@ -1231,11 +1232,18 @@
         management_learn_addr(management, &mi->context.c2.mda_context, &addr, primary);
     }
 #endif
-    if (!primary)
+    if (primary && multi_check_push_ifconfig_extra_route(mi, addr.v4.addr))
     {
-        /* "primary" is the VPN ifconfig address of the peer and already
-         * known to DCO, so only install "extra" iroutes (primary = false)
-         */
+        /* "primary" is the VPN ifconfig address of the peer */
+        /* if it does not fall into the network defined by ifconfig_local
+         * we install this as extra onscope address on the interface  */
+        addr.netbits = 32;
+        addr.type |= MR_ONLINK_DCO_ADDR;
+
+        dco_install_iroute(m, mi, &addr);
+    }
+    else if (!primary)
+    {
         ASSERT(netbits >= 0); /* DCO requires populated netbits */
         dco_install_iroute(m, mi, &addr);
     }
@@ -1269,7 +1277,17 @@
         management_learn_addr(management, &mi->context.c2.mda_context, &addr, primary);
     }
 #endif
-    if (!primary)
+    if (primary && multi_check_push_ifconfig_ipv6_extra_route(mi, &addr.v6.addr))
+    {
+        /* "primary" is the VPN ifconfig address of the peer */
+        /* if it does not fall into the network defined by ifconfig_local
+         * we install this as extra onscope address on the interface  */
+        addr.netbits = 128;
+        addr.type |= MR_ONLINK_DCO_ADDR;
+
+        dco_install_iroute(m, mi, &addr);
+    }
+    else if (!primary)
     {
         /* "primary" is the VPN ifconfig address of the peer and already
          * known to DCO, so only install "extra" iroutes (primary = false)
@@ -4391,3 +4409,49 @@
         }
     }
 }
+
+bool
+multi_check_push_ifconfig_extra_route(struct multi_instance *mi, in_addr_t dest)
+{
+    struct options *o = &mi->context.options;
+    in_addr_t local_addr, local_netmask;
+
+    if (!o->ifconfig_local || !o->ifconfig_remote_netmask)
+    {
+        /* If we do not have a local address, we just return false as
+         * this check doesn't make sense. */
+        return false;
+    }
+
+    /* if it falls into the network defined by ifconfig_local we assume
+     * it is already known to DCO and only install "extra" iroutes  */
+    inet_pton(AF_INET, o->ifconfig_local, &local_addr);
+    inet_pton(AF_INET, o->ifconfig_remote_netmask, &local_netmask);
+
+    return (local_addr & local_netmask) != (dest & local_netmask);
+}
+
+bool
+multi_check_push_ifconfig_ipv6_extra_route(struct multi_instance *mi,
+                                           struct in6_addr *dest)
+{
+    struct options *o = &mi->context.options;
+
+    if (!o->ifconfig_ipv6_local || !o->ifconfig_ipv6_netbits)
+    {
+        /* If we do not have a local address, we just return false as
+         * this check doesn't make sense. */
+        return false;
+    }
+
+    /* if it falls into the network defined by ifconfig_local we assume
+     * it is already known to DCO and only install "extra" iroutes  */
+    struct in6_addr ifconfig_local;
+    if (inet_pton(AF_INET6, o->ifconfig_ipv6_local, &ifconfig_local) != 1)
+    {
+        return false;
+    }
+
+    return (!ipv6_net_contains_host(&ifconfig_local, o->ifconfig_ipv6_netbits,
+                                    dest));
+}
\ No newline at end of file
diff --git a/src/openvpn/multi.h b/src/openvpn/multi.h
index 594ea3a..2fbb433 100644
--- a/src/openvpn/multi.h
+++ b/src/openvpn/multi.h
@@ -665,6 +665,33 @@
     return ret;
 }
 
+/**
+ * Determines if the ifconfig_push_local address falls into the range of the local
+ * IP addresses of the VPN interface (ifconfig_local with ifconfig_remote_netmask)
+ *
+ * @param mi           The multi-instance to check this condition for
+ * @param dest         The destination IP address to check
+ *
+ * @return Returns true if ifconfig_push is outside that range and requires an extra
+ * route to be installed.
+ */
+bool
+multi_check_push_ifconfig_extra_route(struct multi_instance *mi, in_addr_t dest);
+
+/**
+ * Determines if the ifconfig_ipv6_local address falls into the range of the local
+ * IP addresses of the VPN interface (ifconfig_local with ifconfig_remote_netmask)
+ *
+ * @param mi           The multi-instance to check this condition for
+ * @param dest         The destination IPv6 address to check
+ *
+ * @return Returns true if ifconfig_push is outside that range and requires an extra
+ * route to be installed.
+ */
+bool
+multi_check_push_ifconfig_ipv6_extra_route(struct multi_instance *mi,
+                                           struct in6_addr *dest);
+
 /*
  * Check for signals.
  */

[Openvpn-devel] [M] Change in openvpn[master]: Install host routes for ifconfig-push routes when DCO is enabled

[cron2 (C. Review) <ge...@op...>]

Attention is currently required from: d12fk, flichtenheld, ordex, plaisthos.

cron2 has posted comments on this change by plaisthos. ( http://gerrit.openvpn.net/c/openvpn/+/1192?usp=email )

Change subject: Install host routes for ifconfig-push routes when DCO is enabled
......................................................................


Patch Set 14: Code-Review+2

(1 comment)

Patchset:

PS14: 
thanks :-)



-- 
To view, visit http://gerrit.openvpn.net/c/openvpn/+/1192?usp=email
To unsubscribe, or for help writing mail filters, visit http://gerrit.openvpn.net/settings?usp=email

Gerrit-MessageType: comment
Gerrit-Project: openvpn
Gerrit-Branch: master
Gerrit-Change-Id: I83295e00d1a756dfa44050b0a4493095fb050fff
Gerrit-Change-Number: 1192
Gerrit-PatchSet: 14
Gerrit-Owner: plaisthos <arn...@rf...>
Gerrit-Reviewer: cron2 <ge...@gr...>
Gerrit-Reviewer: d12fk <he...@op...>
Gerrit-Reviewer: flichtenheld <fr...@li...>
Gerrit-Reviewer: ordex <an...@ma...>
Gerrit-CC: openvpn-devel <ope...@li...>
Gerrit-Attention: plaisthos <arn...@rf...>
Gerrit-Attention: flichtenheld <fr...@li...>
Gerrit-Attention: ordex <an...@ma...>
Gerrit-Attention: d12fk <he...@op...>
Gerrit-Comment-Date: Wed, 29 Oct 2025 07:06:44 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: Yes

[Openvpn-devel] [PATCH v1] Fix logic when pushed cipher triggers tun reopen and ignore more options

[Gert D. <ge...@gr...>]

From: Arne Schwabe <ar...@rf...>

The logic was inverted. Only when link-mtu is used, pushing a cipher can
change the MTU and not the other way round. (found by zeropath)

Also ignore a few more options that should not trigger a reopen of tun
in push message.

Reported-By: co...@jo...
Found-By: Zeropath
Change-Id: I76eb584024610a6054a069340adbac988abf686c
Signed-off-by: Arne Schwabe <arn...@rf...>
Acked-by: Gert Doering <ge...@gr...>
Gerrit URL: https://gerrit.openvpn.net/c/openvpn/+/1321
---

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/+/1321
This mail reflects revision 1 of this Change.

Signed-off-by line for the author was added as per our policy.

Acked-by according to Gerrit (reflected above):
Gert Doering <ge...@gr...>

        
diff --git a/src/openvpn/push.c b/src/openvpn/push.c
index 2c717c7..d7063e6 100644
--- a/src/openvpn/push.c
+++ b/src/openvpn/push.c
@@ -1025,15 +1025,25 @@
     char line[OPTION_PARM_SIZE];
     while (buf_parse(buf, ',', line, sizeof(line)))
     {
-        /* peer-id and auth-token might change on restart and this should not trigger reopening tun
+        /* peer-id and auth-token might change on restart and this should not
+         * trigger reopening tun
+         * Also other options that only affect the control channel should
+         * not trigger a reopen of the tun device
          */
-        if (strprefix(line, "peer-id ") || strprefix(line, "auth-token ")
-            || strprefix(line, "auth-token-user "))
+        if (strprefix(line, "peer-id ")
+            || strprefix(line, "auth-token ")
+            || strprefix(line, "auth-token-user")
+            || strprefix(line, "protocol-flags ")
+            || strprefix(line, "key-derivation ")
+            || strprefix(line, "explicit-exit-notify ")
+            || strprefix(line, "ping ")
+            || strprefix(line, "ping-restart ")
+            || strprefix(line, "ping-timer "))
         {
             continue;
         }
         /* tun reopen only needed if cipher change can change tun MTU */
-        if (strprefix(line, "cipher ") && !opt->ce.tun_mtu_defined)
+        if (strprefix(line, "cipher ") && opt->ce.tun_mtu_defined)
         {
             continue;
         }

[Openvpn-devel] [S] Change in openvpn[master]: Fix logic when pushed cipher triggers tun reopen and ignore more options

[cron2 (C. Review) <ge...@op...>]

Attention is currently required from: flichtenheld, plaisthos.

cron2 has posted comments on this change by plaisthos. ( http://gerrit.openvpn.net/c/openvpn/+/1321?usp=email )

Change subject: Fix logic when pushed cipher triggers tun reopen and ignore more options
......................................................................


Patch Set 1: Code-Review+2

(1 comment)

Patchset:

PS1: 
> welcome to openvpn logic. link-mtu and tun-mtu are mutally exclusive. […]
Right, this is why the code should check for `link_mtu_defined` and not `tun_mtu_defined`.  Right?  `if (cipher && !link_mtu_defined) { skip this }`.

But you're right, I misread this - `tun_mtu_defined` implies `!link_mtu_defined`, so while I find the other one easier to follow ("if link_mtu defined, tun_mtu is derived from cipher etc.") the code is correct.



-- 
To view, visit http://gerrit.openvpn.net/c/openvpn/+/1321?usp=email
To unsubscribe, or for help writing mail filters, visit http://gerrit.openvpn.net/settings?usp=email

Gerrit-MessageType: comment
Gerrit-Project: openvpn
Gerrit-Branch: master
Gerrit-Change-Id: I76eb584024610a6054a069340adbac988abf686c
Gerrit-Change-Number: 1321
Gerrit-PatchSet: 1
Gerrit-Owner: plaisthos <arn...@rf...>
Gerrit-Reviewer: cron2 <ge...@gr...>
Gerrit-Reviewer: flichtenheld <fr...@li...>
Gerrit-CC: openvpn-devel <ope...@li...>
Gerrit-Attention: plaisthos <arn...@rf...>
Gerrit-Attention: flichtenheld <fr...@li...>
Gerrit-Comment-Date: Wed, 29 Oct 2025 06:52:59 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: Yes
Comment-In-Reply-To: plaisthos <arn...@rf...>
Comment-In-Reply-To: cron2 <ge...@gr...>

[Openvpn-devel] [M] Change in openvpn[master]: Install host routes for ifconfig-push routes when DCO is enabled

[plaisthos (C. Review) <ge...@op...>]

Attention is currently required from: d12fk, flichtenheld, ordex, plaisthos.

Hello cron2, d12fk, flichtenheld, ordex, 

I'd like you to reexamine a change. Please visit

    http://gerrit.openvpn.net/c/openvpn/+/1192?usp=email

to look at the new patch set (#14).


Change subject: Install host routes for ifconfig-push routes when DCO is enabled
......................................................................

Install host routes for ifconfig-push routes when DCO is enabled

ifconfig-push and ifconfig-ipv6-push can configure the IP address of a
client. If this IP address lies inside the network that is configured
on the ovpn/tun device this works as expected as the routing table point to
the ovpn/tun interface. However, if the IP address
is outside that range, the IP packets are not forwarded to the ovpn/tun
interface.

This patch adds logic to add host routes for these
ifconfig-push/ifconfig-ipv6-push addresses to ensure that traffic for
these IP addresses is also directed to the VPN.

For Linux it is important that these extra routes are routes using scope link
rather than static since otherwise routes via these IP addresses, like
iroute, will not work. On FreeBSD we also use interface routes as works and
routes that target interfaces instead of IP addresses are less brittle.

Tested using a server with ccd:

   openvpn --server 10.33.0.0 255.255.192.0 --server-ipv6 fd00:f00f::1/64  --client-config-dir ~/ccd [...]

and a client with lwipvonpn and the following ccd file:

   iroute-ipv6 FD00:F00F:CAFE::1001/64
   ifconfig-ipv6-push FD00:F00F:D00D::77/64
   push "setenv-safe ifconfig_ipv6_local_2 FD00:F00F:CAFE::1001"
   push "setenv-safe ifconfig_ipv6_netbits_2 64"

   iroute 10.234.234.0 255.255.255.0
   ifconfig-push 10.11.12.13 255.255.255.0
   push "setenv-safe ifconfig_local_2 10.234.234.12"
   push "setenv-safe ifconfig_netmask_2 255.255.255.0"

This setups an ifconfig-push addresses outside the --server/--server-ipv6
network and additionally configures a iroute behind that client. The
setenv-safe configure lwipovpn to use that additional IP addresses to allow
testing via ping.

Windows behaves like the user space implementation. It does require these
special routes but instead (like user space) needs static routes to redirect
IP traffic for these IP addresses to the tunnel interface. E.g. in the example
above the server config needs to have:

   route 10.234.234.0 255.255.255.0
   route 10.11.12.0 255.255.255.0

   route-ipv6 FD00:F00F:CAFE::1001/64
   route-ipv6 FD00:F00F:D00D::77/64

Change-Id: I83295e00d1a756dfa44050b0a4493095fb050fff
Signed-off-by: Arne Schwabe <ar...@rf...>
---
M doc/man-sections/server-options.rst
M src/openvpn/dco.c
M src/openvpn/mroute.h
M src/openvpn/multi.c
M src/openvpn/multi.h
5 files changed, 156 insertions(+), 7 deletions(-)


  git pull ssh://gerrit.openvpn.net:29418/openvpn refs/changes/92/1192/14

diff --git a/doc/man-sections/server-options.rst b/doc/man-sections/server-options.rst
index ccc1374..9c7fa46 100644
--- a/doc/man-sections/server-options.rst
+++ b/doc/man-sections/server-options.rst
@@ -314,6 +314,10 @@
   3.  Use ``--ifconfig-pool`` allocation for dynamic IP (last
       choice).
 
+  When DCO is enabled and the IP is not in contained in the network specified
+  by ``--ifconfig``, OpenVPN will install a /32 host route for the ``local``
+  IP address.
+
 --ifconfig-ipv6-push args
   for ``--client-config-dir`` per-client static IPv6 interface
   configuration, see ``--client-config-dir`` and ``--ifconfig-push`` for
@@ -324,6 +328,10 @@
 
      ifconfig-ipv6-push ipv6addr/bits ipv6remote
 
+  When DCO is enabled and the IP is not in contained in the network specified
+  by ``--ifconfig-ipv6``, OpenVPN will install a /128 host route for the
+  ``ipv6addr`` IP address.
+
 --multihome
   Configure a multi-homed UDP server. This option needs to be used when a
   server has more than one IP address (e.g. multiple interfaces, or
diff --git a/src/openvpn/dco.c b/src/openvpn/dco.c
index 8fb4662..7abdad3 100644
--- a/src/openvpn/dco.c
+++ b/src/openvpn/dco.c
@@ -664,6 +664,14 @@
         return;
     }
 
+#if defined(_WIN32)
+    if (addr->type & MR_ONLINK_DCO_ADDR)
+    {
+        /* Windows does not need these extra routes, so we ignore/skip them */
+        return;
+    }
+#endif
+
     struct context *c = &mi->context;
     if (addrtype == MR_ADDR_IPV6)
     {
@@ -671,8 +679,14 @@
         dco_win_add_iroute_ipv6(&c->c1.tuntap->dco, addr->v6.addr, addr->netbits,
                                 c->c2.tls_multi->peer_id);
 #else
+        const struct in6_addr *gateway = &mi->context.c2.push_ifconfig_ipv6_local;
+        if (addr->type & MR_ONLINK_DCO_ADDR)
+        {
+            gateway = NULL;
+        }
+
         net_route_v6_add(&m->top.net_ctx, &addr->v6.addr, addr->netbits,
-                         &mi->context.c2.push_ifconfig_ipv6_local, c->c1.tuntap->actual_name, 0,
+                         gateway, c->c1.tuntap->actual_name, 0,
                          DCO_IROUTE_METRIC);
 #endif
     }
@@ -683,7 +697,13 @@
                                 c->c2.tls_multi->peer_id);
 #else
         in_addr_t dest = htonl(addr->v4.addr);
-        net_route_v4_add(&m->top.net_ctx, &dest, addr->netbits, &mi->context.c2.push_ifconfig_local,
+        const in_addr_t *gateway = &mi->context.c2.push_ifconfig_local;
+        if (addr->type & MR_ONLINK_DCO_ADDR)
+        {
+            gateway = NULL;
+        }
+
+        net_route_v4_add(&m->top.net_ctx, &dest, addr->netbits, gateway,
                          c->c1.tuntap->actual_name, 0, DCO_IROUTE_METRIC);
 #endif
     }
@@ -714,6 +734,20 @@
                              DCO_IROUTE_METRIC);
 #endif
         }
+
+#if !defined(_WIN32)
+        /* Check if we added a host route as the assigned client IP address was
+         * not in the on link scope defined by --ifconfig */
+        in_addr_t ifconfig_local = mi->context.c2.push_ifconfig_local;
+
+        if (multi_check_push_ifconfig_extra_route(mi, htonl(ifconfig_local)))
+        {
+            /* On windows we do not install these routes, so we also do not need to delete them */
+            net_route_v4_del(&m->top.net_ctx, &ifconfig_local,
+                             32, NULL, c->c1.tuntap->actual_name, 0,
+                             DCO_IROUTE_METRIC);
+        }
+#endif
     }
 
     if (mi->context.c2.push_ifconfig_ipv6_defined)
@@ -728,6 +762,18 @@
                              DCO_IROUTE_METRIC);
 #endif
         }
+
+        /* Checked if we added a host route as the assigned client IP address was
+         * outside the --ifconfig-ipv6 tun interface config */
+#if !defined(_WIN32)
+        struct in6_addr *dest = &mi->context.c2.push_ifconfig_ipv6_local;
+        if (multi_check_push_ifconfig_ipv6_extra_route(mi, dest))
+        {
+            /* On windows we do not install these routes, so we also do not need to delete them */
+            net_route_v6_del(&m->top.net_ctx, dest, 128, NULL,
+                             c->c1.tuntap->actual_name, 0, DCO_IROUTE_METRIC);
+        }
+#endif
     }
 #endif /* if defined(TARGET_LINUX) || defined(TARGET_FREEBSD) || defined(_WIN32) */
 }
diff --git a/src/openvpn/mroute.h b/src/openvpn/mroute.h
index 5b0c694..afd2e6c 100644
--- a/src/openvpn/mroute.h
+++ b/src/openvpn/mroute.h
@@ -20,6 +20,7 @@
  *  with this program; if not, see <https://www.gnu.org/licenses/>.
  */
 
+
 #ifndef MROUTE_H
 #define MROUTE_H
 
@@ -74,6 +75,9 @@
 /* Address type mask indicating that proto # is part of address */
 #define MR_WITH_PROTO 32
 
+/* MRoute is an on link/scope address needed for DCO on Unix platforms */
+#define MR_ONLINK_DCO_ADDR 64
+
 struct mroute_addr
 {
     uint8_t len;     /* length of address */
diff --git a/src/openvpn/multi.c b/src/openvpn/multi.c
index f60944d..ba35556 100644
--- a/src/openvpn/multi.c
+++ b/src/openvpn/multi.c
@@ -42,6 +42,7 @@
 #include "ssl_ncp.h"
 #include "vlan.h"
 #include "auth_token.h"
+#include "route.h"
 #include <inttypes.h>
 #include <string.h>
 
@@ -1231,11 +1232,18 @@
         management_learn_addr(management, &mi->context.c2.mda_context, &addr, primary);
     }
 #endif
-    if (!primary)
+    if (primary && multi_check_push_ifconfig_extra_route(mi, addr.v4.addr))
     {
-        /* "primary" is the VPN ifconfig address of the peer and already
-         * known to DCO, so only install "extra" iroutes (primary = false)
-         */
+        /* "primary" is the VPN ifconfig address of the peer */
+        /* if it does not fall into the network defined by ifconfig_local
+         * we install this as extra onscope address on the interface  */
+        addr.netbits = 32;
+        addr.type |= MR_ONLINK_DCO_ADDR;
+
+        dco_install_iroute(m, mi, &addr);
+    }
+    else if (!primary)
+    {
         ASSERT(netbits >= 0); /* DCO requires populated netbits */
         dco_install_iroute(m, mi, &addr);
     }
@@ -1269,7 +1277,17 @@
         management_learn_addr(management, &mi->context.c2.mda_context, &addr, primary);
     }
 #endif
-    if (!primary)
+    if (primary && multi_check_push_ifconfig_ipv6_extra_route(mi, &addr.v6.addr))
+    {
+        /* "primary" is the VPN ifconfig address of the peer */
+        /* if it does not fall into the network defined by ifconfig_local
+         * we install this as extra onscope address on the interface  */
+        addr.netbits = 128;
+        addr.type |= MR_ONLINK_DCO_ADDR;
+
+        dco_install_iroute(m, mi, &addr);
+    }
+    else if (!primary)
     {
         /* "primary" is the VPN ifconfig address of the peer and already
          * known to DCO, so only install "extra" iroutes (primary = false)
@@ -4391,3 +4409,49 @@
         }
     }
 }
+
+bool
+multi_check_push_ifconfig_extra_route(struct multi_instance *mi, in_addr_t dest)
+{
+    struct options *o = &mi->context.options;
+    in_addr_t local_addr, local_netmask;
+
+    if (!o->ifconfig_local || !o->ifconfig_remote_netmask)
+    {
+        /* If we do not have a local address, we just return false as
+         * this check doesn't make sense. */
+        return false;
+    }
+
+    /* if it falls into the network defined by ifconfig_local we assume
+     * it is already known to DCO and only install "extra" iroutes  */
+    inet_pton(AF_INET, o->ifconfig_local, &local_addr);
+    inet_pton(AF_INET, o->ifconfig_remote_netmask, &local_netmask);
+
+    return (local_addr & local_netmask) != (dest & local_netmask);
+}
+
+bool
+multi_check_push_ifconfig_ipv6_extra_route(struct multi_instance *mi,
+                                           struct in6_addr *dest)
+{
+    struct options *o = &mi->context.options;
+
+    if (!o->ifconfig_ipv6_local || !o->ifconfig_ipv6_netbits)
+    {
+        /* If we do not have a local address, we just return false as
+         * this check doesn't make sense. */
+        return false;
+    }
+
+    /* if it falls into the network defined by ifconfig_local we assume
+     * it is already known to DCO and only install "extra" iroutes  */
+    struct in6_addr ifconfig_local;
+    if (inet_pton(AF_INET6, o->ifconfig_ipv6_local, &ifconfig_local) != 1)
+    {
+        return false;
+    }
+
+    return (!ipv6_net_contains_host(&ifconfig_local, o->ifconfig_ipv6_netbits,
+                                    dest));
+}
\ No newline at end of file
diff --git a/src/openvpn/multi.h b/src/openvpn/multi.h
index 594ea3a..2fbb433 100644
--- a/src/openvpn/multi.h
+++ b/src/openvpn/multi.h
@@ -665,6 +665,33 @@
     return ret;
 }
 
+/**
+ * Determines if the ifconfig_push_local address falls into the range of the local
+ * IP addresses of the VPN interface (ifconfig_local with ifconfig_remote_netmask)
+ *
+ * @param mi           The multi-instance to check this condition for
+ * @param dest         The destination IP address to check
+ *
+ * @return Returns true if ifconfig_push is outside that range and requires an extra
+ * route to be installed.
+ */
+bool
+multi_check_push_ifconfig_extra_route(struct multi_instance *mi, in_addr_t dest);
+
+/**
+ * Determines if the ifconfig_ipv6_local address falls into the range of the local
+ * IP addresses of the VPN interface (ifconfig_local with ifconfig_remote_netmask)
+ *
+ * @param mi           The multi-instance to check this condition for
+ * @param dest         The destination IPv6 address to check
+ *
+ * @return Returns true if ifconfig_push is outside that range and requires an extra
+ * route to be installed.
+ */
+bool
+multi_check_push_ifconfig_ipv6_extra_route(struct multi_instance *mi,
+                                           struct in6_addr *dest);
+
 /*
  * Check for signals.
  */

-- 
To view, visit http://gerrit.openvpn.net/c/openvpn/+/1192?usp=email
To unsubscribe, or for help writing mail filters, visit http://gerrit.openvpn.net/settings?usp=email

Gerrit-MessageType: newpatchset
Gerrit-Project: openvpn
Gerrit-Branch: master
Gerrit-Change-Id: I83295e00d1a756dfa44050b0a4493095fb050fff
Gerrit-Change-Number: 1192
Gerrit-PatchSet: 14
Gerrit-Owner: plaisthos <arn...@rf...>
Gerrit-Reviewer: cron2 <ge...@gr...>
Gerrit-Reviewer: d12fk <he...@op...>
Gerrit-Reviewer: flichtenheld <fr...@li...>
Gerrit-Reviewer: ordex <an...@ma...>
Gerrit-CC: openvpn-devel <ope...@li...>
Gerrit-Attention: plaisthos <arn...@rf...>
Gerrit-Attention: flichtenheld <fr...@li...>
Gerrit-Attention: ordex <an...@ma...>
Gerrit-Attention: d12fk <he...@op...>

[Openvpn-devel] [S] Change in openvpn[master]: Fix logic when pushed cipher triggers tun reopen and ignore more options

[plaisthos (C. Review) <ge...@op...>]

Attention is currently required from: cron2, flichtenheld.

plaisthos has posted comments on this change by plaisthos. ( http://gerrit.openvpn.net/c/openvpn/+/1321?usp=email )

Change subject: Fix logic when pushed cipher triggers tun reopen and ignore more options
......................................................................


Patch Set 1:

(2 comments)

Patchset:

PS1: 
> I don't think the logic is right now (neither before nor afterwards). […]
welcome to openvpn logic. link-mtu and tun-mtu are mutally exclusive. And you need to have link-mtu in the config to actually have a pushed cipher to change the MTU of the tun device.


File src/openvpn/push.c:

http://gerrit.openvpn.net/c/openvpn/+/1321/comment/036771c8_d363c718?usp=email :
PS1, Line 1046:         if (strprefix(line, "cipher ") && opt->ce.tun_mtu_defined)
> I don't think this is actually correct - `--tun-mtu` is the *inner* MTU, and that never changes on ` […]
The question is more if we want to deprecate link-mtu and its weird behaviour.



-- 
To view, visit http://gerrit.openvpn.net/c/openvpn/+/1321?usp=email
To unsubscribe, or for help writing mail filters, visit http://gerrit.openvpn.net/settings?usp=email

Gerrit-MessageType: comment
Gerrit-Project: openvpn
Gerrit-Branch: master
Gerrit-Change-Id: I76eb584024610a6054a069340adbac988abf686c
Gerrit-Change-Number: 1321
Gerrit-PatchSet: 1
Gerrit-Owner: plaisthos <arn...@rf...>
Gerrit-Reviewer: cron2 <ge...@gr...>
Gerrit-Reviewer: flichtenheld <fr...@li...>
Gerrit-CC: openvpn-devel <ope...@li...>
Gerrit-Attention: cron2 <ge...@gr...>
Gerrit-Attention: flichtenheld <fr...@li...>
Gerrit-Comment-Date: Tue, 28 Oct 2025 22:47:16 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Comment-In-Reply-To: cron2 <ge...@gr...>

[Openvpn-devel] [M] Change in openvpn[master]: iservice: use interface index with netsh

[d12fk (C. Review) <ge...@op...>]

Attention is currently required from: d12fk, flichtenheld, plaisthos.

Hello cron2, flichtenheld, plaisthos, 

I'd like you to reexamine a change. Please visit

    http://gerrit.openvpn.net/c/openvpn/+/1308?usp=email

to look at the new patch set (#2).


Change subject: iservice: use interface index with netsh
......................................................................

iservice: use interface index with netsh

We use the interface index with netsh everywhere else, so convert this
invocation of netsh to index use as well.

Change-Id: I329b407693b97dd048bd3788ecad345d6e25dab2
Signed-off-by: Heiko Hund <he...@is...>
---
M src/openvpnserv/interactive.c
1 file changed, 26 insertions(+), 38 deletions(-)


  git pull ssh://gerrit.openvpn.net:29418/openvpn refs/changes/08/1308/2

diff --git a/src/openvpnserv/interactive.c b/src/openvpnserv/interactive.c
index ce0d4dd..52f9c66 100644
--- a/src/openvpnserv/interactive.c
+++ b/src/openvpnserv/interactive.c
@@ -995,16 +995,16 @@
 }
 
 /**
- * Run the command: netsh interface ip $action wins $if_name [static] $addr
+ * Run the command: netsh interface ip $action wins $if_index [static] $addr
  * @param  action      "delete", "add" or "set"
- * @param  if_name     "name_of_interface"
+ * @param  if_index    index of the interface to modify WINS for
  * @param  addr        IPv4 address as a string
  *
  * If addr is null and action = "delete" all addresses are deleted.
  * if action = "set" then "static" is added before $addr
  */
 static DWORD
-netsh_wins_cmd(const wchar_t *action, const wchar_t *if_name, const wchar_t *addr)
+netsh_wins_cmd(const wchar_t *action, int if_index, const wchar_t *addr)
 {
     DWORD err = 0;
     int timeout = 30000; /* in msec */
@@ -1030,10 +1030,10 @@
     /* cmd template:
      * netsh interface ip $action wins $if_name $static $addr
      */
-    const wchar_t *fmt = L"netsh interface ip %ls wins \"%ls\" %ls %ls";
+    const wchar_t *fmt = L"netsh interface ip %ls wins %d %ls %ls";
 
     /* max cmdline length in wchars -- include room for worst case and some */
-    size_t ncmdline = wcslen(fmt) + wcslen(if_name) + wcslen(action) + wcslen(addr)
+    size_t ncmdline = wcslen(fmt) + 11 /*if_index*/ + wcslen(action) + wcslen(addr)
                       + wcslen(addr_static) + 32 + 1;
     cmdline = malloc(ncmdline * sizeof(wchar_t));
     if (!cmdline)
@@ -1042,7 +1042,7 @@
         goto out;
     }
 
-    swprintf(cmdline, ncmdline, fmt, action, if_name, addr_static, addr);
+    swprintf(cmdline, ncmdline, fmt, action, if_index, addr_static, addr);
 
     err = ExecCommand(argv0, cmdline, timeout);
 
@@ -2882,7 +2882,7 @@
 static DWORD
 HandleWINSConfigMessage(const wins_cfg_message_t *msg, undo_lists_t *lists)
 {
-    DWORD err = 0;
+    DWORD err = NO_ERROR;
     wchar_t addr[16]; /* large enough to hold string representation of an ipv4 */
     int addr_len = msg->addr_len;
 
@@ -2892,38 +2892,25 @@
         addr_len = _countof(msg->addr);
     }
 
-    if (!msg->iface.name[0]) /* interface name is required */
+    if (!msg->iface.index) /* interface index is required */
     {
         return ERROR_MESSAGE_DATA;
     }
 
-    /* use a non-const reference with limited scope to enforce null-termination of strings from
-     * client */
-    {
-        wins_cfg_message_t *msgptr = (wins_cfg_message_t *)msg;
-        msgptr->iface.name[_countof(msg->iface.name) - 1] = '\0';
-    }
-
-    wchar_t *wide_name = utf8to16(msg->iface.name); /* utf8 to wide-char */
-    if (!wide_name)
-    {
-        return ERROR_OUTOFMEMORY;
-    }
-
     /* We delete all current addresses before adding any
      * OR if the message type is del_wins_cfg
      */
     if (addr_len > 0 || msg->header.type == msg_del_wins_cfg)
     {
-        err = netsh_wins_cmd(L"delete", wide_name, NULL);
+        err = netsh_wins_cmd(L"delete", msg->iface.index, NULL);
         if (err)
         {
             goto out;
         }
-        free(RemoveListItem(&(*lists)[undo_wins], CmpWString, wide_name));
+        free(RemoveListItem(&(*lists)[undo_wins], CmpAny, NULL));
     }
 
-    if (msg->header.type == msg_del_wins_cfg)
+    if (addr_len == 0 || msg->header.type == msg_del_wins_cfg)
     {
         goto out; /* job done */
     }
@@ -2931,7 +2918,7 @@
     for (int i = 0; i < addr_len; ++i)
     {
         RtlIpv4AddressToStringW(&msg->addr[i].ipv4, addr);
-        err = netsh_wins_cmd(i == 0 ? L"set" : L"add", wide_name, addr);
+        err = netsh_wins_cmd(i == 0 ? L"set" : L"add", msg->iface.index, addr);
         if (i == 0 && err)
         {
             goto out;
@@ -2941,22 +2928,23 @@
          */
     }
 
-    err = 0;
-
-    if (addr_len > 0)
+    int *if_index = malloc(sizeof(msg->iface.index));
+    if (if_index)
     {
-        wchar_t *tmp_name = _wcsdup(wide_name);
-        if (!tmp_name || AddListItem(&(*lists)[undo_wins], tmp_name))
-        {
-            free(tmp_name);
-            netsh_wins_cmd(L"delete", wide_name, NULL);
-            err = ERROR_OUTOFMEMORY;
-            goto out;
-        }
+        *if_index = msg->iface.index;
     }
 
+    if (!if_index || AddListItem(&(*lists)[undo_wins], if_index))
+    {
+        free(if_index);
+        netsh_wins_cmd(L"delete", msg->iface.index, NULL);
+        err = ERROR_OUTOFMEMORY;
+        goto out;
+    }
+
+    err = 0;
+
 out:
-    free(wide_name);
     return err;
 }
 
@@ -3206,7 +3194,7 @@
                     break;
 
                 case undo_wins:
-                    netsh_wins_cmd(L"delete", item->data, NULL);
+                    netsh_wins_cmd(L"delete", *(int *)item->data, NULL);
                     break;
 
                 case wfp_block:

-- 
To view, visit http://gerrit.openvpn.net/c/openvpn/+/1308?usp=email
To unsubscribe, or for help writing mail filters, visit http://gerrit.openvpn.net/settings?usp=email

Gerrit-MessageType: newpatchset
Gerrit-Project: openvpn
Gerrit-Branch: master
Gerrit-Change-Id: I329b407693b97dd048bd3788ecad345d6e25dab2
Gerrit-Change-Number: 1308
Gerrit-PatchSet: 2
Gerrit-Owner: d12fk <he...@op...>
Gerrit-Reviewer: cron2 <ge...@gr...>
Gerrit-Reviewer: flichtenheld <fr...@li...>
Gerrit-Reviewer: plaisthos <arn...@rf...>
Gerrit-CC: openvpn-devel <ope...@li...>
Gerrit-Attention: plaisthos <arn...@rf...>
Gerrit-Attention: flichtenheld <fr...@li...>
Gerrit-Attention: d12fk <he...@op...>

[Openvpn-devel] [PATCH v1] remove redundant PULL_DEFINED() macro definition

[Gert D. <ge...@gr...>]

this seems to be a leftover of the time when we had conditional
compilation for "--disable-server" or thus.  Commit d6a0cf599
removed PUSH_DEFINED() nearby but overlooked this one.

Change-Id: I9118333bb65cd5db0836abefa5d45a729f0142cc
Signed-off-by: Gert Doering <ge...@gr...>
Acked-by: Frank Lichtenheld <fr...@li...>
Gerrit URL: https://gerrit.openvpn.net/c/openvpn/+/1322
---

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/+/1322
This mail reflects revision 1 of this Change.

Acked-by according to Gerrit (reflected above):
Frank Lichtenheld <fr...@li...>

        
diff --git a/src/openvpn/options.h b/src/openvpn/options.h
index 24253af..125e524 100644
--- a/src/openvpn/options.h
+++ b/src/openvpn/options.h
@@ -769,10 +769,6 @@
 
 #define PULL_DEFINED(opt) ((opt)->pull)
 
-#ifndef PULL_DEFINED
-#define PULL_DEFINED(opt) (false)
-#endif
-
 #ifdef _WIN32
 #define ROUTE_OPTION_FLAGS(o) ((o)->route_method & ROUTE_METHOD_MASK)
 #else

[Openvpn-devel] [PATCH v1] zeroize struct image in packet_id_persist_save() before writing to disk

[Gert D. <ge...@gr...>]

while this really is only a debug function, ensuring that no uninitialized
heap content ends up in padding in the structure and thus to disk is good
practice.

Reported-by: Joshua Rogers <co...@jo...>
Found-by: ZeroPath (https://zeropath.com/)

Change-Id: I7f4c7b0ca748975defca1e5104e7077a761cd49c
Signed-off-by: Gert Doering <ge...@gr...>
Acked-by: Frank Lichtenheld <fr...@li...>
Gerrit URL: https://gerrit.openvpn.net/c/openvpn/+/1323
---

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/+/1323
This mail reflects revision 1 of this Change.

Acked-by according to Gerrit (reflected above):
Frank Lichtenheld <fr...@li...>

        
diff --git a/src/openvpn/packet_id.c b/src/openvpn/packet_id.c
index 880eee1..08d9d9b 100644
--- a/src/openvpn/packet_id.c
+++ b/src/openvpn/packet_id.c
@@ -511,6 +511,7 @@
         && (p->time != p->time_last_written || p->id != p->id_last_written))
     {
         struct packet_id_persist_file_image image;
+        CLEAR(image);
         ssize_t n;
         off_t seek_ret;
         struct gc_arena gc = gc_new();

[Openvpn-devel] [M] Change in openvpn[master]: PUSH_UPDATE server: invalid read bug-fix and unit-tests improvements

[mrbff (C. Review) <ge...@op...>]

Attention is currently required from: flichtenheld, plaisthos.

mrbff has posted comments on this change by mrbff. ( http://gerrit.openvpn.net/c/openvpn/+/1316?usp=email )

Change subject: PUSH_UPDATE server: invalid read bug-fix and unit-tests improvements
......................................................................


Patch Set 2:

(1 comment)

File tests/unit_tests/openvpn/test_push_update_msg.c:

http://gerrit.openvpn.net/c/openvpn/+/1316/comment/c55bce38_3e8b3f8e?usp=email :
PS1, Line 147:                 printf("\n\nexpected_size: %lu\n  actual_size: %lu", res_len, str_len);
> ```suggestion […]
Done



-- 
To view, visit http://gerrit.openvpn.net/c/openvpn/+/1316?usp=email
To unsubscribe, or for help writing mail filters, visit http://gerrit.openvpn.net/settings?usp=email

Gerrit-MessageType: comment
Gerrit-Project: openvpn
Gerrit-Branch: master
Gerrit-Change-Id: Idba419681fe3ccc4e6e2f6ce7592332dcff62cd9
Gerrit-Change-Number: 1316
Gerrit-PatchSet: 2
Gerrit-Owner: mrbff <ma...@ma...>
Gerrit-Reviewer: flichtenheld <fr...@li...>
Gerrit-Reviewer: plaisthos <arn...@rf...>
Gerrit-CC: openvpn-devel <ope...@li...>
Gerrit-Attention: plaisthos <arn...@rf...>
Gerrit-Attention: flichtenheld <fr...@li...>
Gerrit-Comment-Date: Tue, 28 Oct 2025 19:52:49 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Comment-In-Reply-To: flichtenheld <fr...@li...>

[Openvpn-devel] [XS] Change in openvpn[master]: zeroize struct image in packet_id_persist_save() before writing to disk

[flichtenheld (C. Review) <ge...@op...>]

Attention is currently required from: cron2, plaisthos.

flichtenheld has posted comments on this change by cron2. ( http://gerrit.openvpn.net/c/openvpn/+/1323?usp=email )

Change subject: zeroize struct image in packet_id_persist_save() before writing to disk
......................................................................


Patch Set 1: Code-Review+2


-- 
To view, visit http://gerrit.openvpn.net/c/openvpn/+/1323?usp=email
To unsubscribe, or for help writing mail filters, visit http://gerrit.openvpn.net/settings?usp=email

Gerrit-MessageType: comment
Gerrit-Project: openvpn
Gerrit-Branch: master
Gerrit-Change-Id: I7f4c7b0ca748975defca1e5104e7077a761cd49c
Gerrit-Change-Number: 1323
Gerrit-PatchSet: 1
Gerrit-Owner: cron2 <ge...@gr...>
Gerrit-Reviewer: flichtenheld <fr...@li...>
Gerrit-Reviewer: plaisthos <arn...@rf...>
Gerrit-CC: openvpn-devel <ope...@li...>
Gerrit-Attention: plaisthos <arn...@rf...>
Gerrit-Attention: cron2 <ge...@gr...>
Gerrit-Comment-Date: Tue, 28 Oct 2025 19:51:04 +0000
Gerrit-HasComments: No
Gerrit-Has-Labels: Yes

[Openvpn-devel] [XS] Change in openvpn[master]: remove redundant PULL_DEFINED() macro definition

[flichtenheld (C. Review) <ge...@op...>]

Attention is currently required from: cron2, plaisthos.

flichtenheld has posted comments on this change by cron2. ( http://gerrit.openvpn.net/c/openvpn/+/1322?usp=email )

Change subject: remove redundant PULL_DEFINED() macro definition
......................................................................


Patch Set 1: Code-Review+2


-- 
To view, visit http://gerrit.openvpn.net/c/openvpn/+/1322?usp=email
To unsubscribe, or for help writing mail filters, visit http://gerrit.openvpn.net/settings?usp=email

Gerrit-MessageType: comment
Gerrit-Project: openvpn
Gerrit-Branch: master
Gerrit-Change-Id: I9118333bb65cd5db0836abefa5d45a729f0142cc
Gerrit-Change-Number: 1322
Gerrit-PatchSet: 1
Gerrit-Owner: cron2 <ge...@gr...>
Gerrit-Reviewer: flichtenheld <fr...@li...>
Gerrit-Reviewer: plaisthos <arn...@rf...>
Gerrit-CC: openvpn-devel <ope...@li...>
Gerrit-Attention: plaisthos <arn...@rf...>
Gerrit-Attention: cron2 <ge...@gr...>
Gerrit-Comment-Date: Tue, 28 Oct 2025 19:49:45 +0000
Gerrit-HasComments: No
Gerrit-Has-Labels: Yes

[Openvpn-devel] [M] Change in openvpn[master]: iservice: use interface index with netsh

[cron2 (C. Review) <ge...@op...>]

Attention is currently required from: d12fk, flichtenheld, plaisthos.

cron2 has posted comments on this change by d12fk. ( http://gerrit.openvpn.net/c/openvpn/+/1308?usp=email )

Change subject: iservice: use interface index with netsh
......................................................................


Patch Set 1: Code-Review-2

(1 comment)

Patchset:

PS1: 
This fails the integer checks on a windows build

```
oC:\Buildbot\windows-server-2025-latent-ec2-msbuild-x86-clang\build\src\openvpnserv\interactive.c(3197,47): error : incompatible pointer to integer conversion passing 'LPVOID' (aka 'void *') to parameter of type 'int' [-Wint-conversion] [C:\Buildbot\windows-server-2025-latent-ec2-msbuild-x86-clang\build\out\build\win-x86-clang-release\src\openvpnserv\openvpnserv.vcxproj]
o  C:\Buildbot\windows-server-2025-latent-ec2-msbuild-x86-clang\build\src\openvpnserv\interactive.c(1007,43): note: passing argument to parameter 'if_index' here
```



-- 
To view, visit http://gerrit.openvpn.net/c/openvpn/+/1308?usp=email
To unsubscribe, or for help writing mail filters, visit http://gerrit.openvpn.net/settings?usp=email

Gerrit-MessageType: comment
Gerrit-Project: openvpn
Gerrit-Branch: master
Gerrit-Change-Id: I329b407693b97dd048bd3788ecad345d6e25dab2
Gerrit-Change-Number: 1308
Gerrit-PatchSet: 1
Gerrit-Owner: d12fk <he...@op...>
Gerrit-Reviewer: cron2 <ge...@gr...>
Gerrit-Reviewer: flichtenheld <fr...@li...>
Gerrit-Reviewer: plaisthos <arn...@rf...>
Gerrit-CC: openvpn-devel <ope...@li...>
Gerrit-Attention: plaisthos <arn...@rf...>
Gerrit-Attention: flichtenheld <fr...@li...>
Gerrit-Attention: d12fk <he...@op...>
Gerrit-Comment-Date: Tue, 28 Oct 2025 19:28:36 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: Yes

[Openvpn-devel] [M] Change in openvpn[master]: PUSH_UPDATE server: invalid read bug-fix and unit-tests improvements

[mrbff (C. Review) <ge...@op...>]

Attention is currently required from: mrbff, plaisthos.

Hello flichtenheld, plaisthos, 

I'd like you to reexamine a change. Please visit

    http://gerrit.openvpn.net/c/openvpn/+/1316?usp=email

to look at the new patch set (#2).


Change subject: PUSH_UPDATE server: invalid read bug-fix and unit-tests improvements
......................................................................

PUSH_UPDATE server: invalid read bug-fix and unit-tests improvements

The number of messages calculated before the call to message_splitter(), used
in the memory allocation in the buffer array, could in certain cases be less
than one than the actual number of messages, thus causing an override of the
sentinel buffer in message_splitter and therefore an invalid read in
send_single_push_update(). The case in question would be, for example, a
sequence of three options "A, B, C" with the size of B equal to safe_cap - 1
and the sum of the sizes of A and C less than safe_cap - 2.
The buffer array was therefore replaced with a list of buffers to completely
avoid calculating the number of messages before it was actually computed.
The test case in question has been added to the unit tests.
The unit tests have been improved using cmocka macros.

Change-Id: Idba419681fe3ccc4e6e2f6ce7592332dcff62cd9
Signed-off-by: Marco Baffo <ma...@ma...>
---
M src/openvpn/push_util.c
M tests/unit_tests/openvpn/test_push_update_msg.c
2 files changed, 145 insertions(+), 32 deletions(-)


  git pull ssh://gerrit.openvpn.net:29418/openvpn refs/changes/16/1316/2

diff --git a/src/openvpn/push_util.c b/src/openvpn/push_util.c
index 552679a..51c7b5f 100644
--- a/src/openvpn/push_util.c
+++ b/src/openvpn/push_util.c
@@ -91,7 +91,7 @@
  * Return `false` on failure an `true` on success.
  */
 static bool
-message_splitter(const char *s, struct buffer *msgs, struct gc_arena *gc, const size_t safe_cap)
+message_splitter(const char *s, struct buffer_list *msgs, struct gc_arena *gc, const size_t safe_cap)
 {
     if (!s || !*s)
     {
@@ -100,7 +100,6 @@
 
     char *str = gc_strdup(s, gc);
     size_t i = 0;
-    int im = 0;
 
     while (*str)
     {
@@ -115,37 +114,38 @@
             }
             str[ci] = '\0';
             /* copy from i to (ci -1) */
-            msgs[im] = forge_msg(str, ",push-continuation 2", gc);
+            struct buffer tmp = forge_msg(str, ",push-continuation 2", gc);
+            buffer_list_push(msgs, BSTR(&tmp));
             i = ci + 1;
         }
         else
         {
-            if (im)
+            if (msgs->head)
             {
-                msgs[im] = forge_msg(str, ",push-continuation 1", gc);
+                struct buffer tmp = forge_msg(str, ",push-continuation 1", gc);
+                buffer_list_push(msgs, BSTR(&tmp));
             }
             else
             {
-                msgs[im] = forge_msg(str, NULL, gc);
+                struct buffer tmp = forge_msg(str, NULL, gc);
+                buffer_list_push(msgs, BSTR(&tmp));
             }
             i = strlen(str);
         }
         str = &str[i];
-        im++;
     }
     return true;
 }
 
 /* send the message(s) prepared to one single client */
 static bool
-send_single_push_update(struct multi_context *m, struct multi_instance *mi, struct buffer *msgs)
+send_single_push_update(struct multi_context *m, struct multi_instance *mi, struct buffer_list *msgs)
 {
-    if (!msgs[0].data || !*(msgs[0].data))
+    if (!msgs->head)
     {
         return false;
     }
 
-    int i = -1;
     unsigned int option_types_found = 0;
     struct context *c = &mi->context;
     struct options o;
@@ -160,9 +160,10 @@
     o.ifconfig_local = canary;
     o.ifconfig_ipv6_local = canary;
 
-    while (msgs[++i].data && *(msgs[i].data))
+    struct buffer_entry *e = msgs->head;
+    while (e)
     {
-        if (!send_control_channel_string(c, BSTR(&msgs[i]), D_PUSH))
+        if (!send_control_channel_string(c, BSTR(&e->buf), D_PUSH))
         {
             return false;
         }
@@ -182,13 +183,14 @@
          * Also we need to make a temporary copy so we can buf_advance()
          * without modifying original buffer.
          */
-        struct buffer tmp_msg = msgs[i];
+        struct buffer tmp_msg = e->buf;
         buf_string_compare_advance(&tmp_msg, push_update_cmd);
         unsigned int permission_mask = pull_permission_mask(c);
         if (process_push_update(c, &o, permission_mask, &option_types_found, &tmp_msg, true) == PUSH_MSG_ERROR)
         {
             msg(M_WARN, "Failed to process push update message sent to client ID: %u", c->c2.tls_multi->peer_id);
         }
+        e = e->next;
     }
 
     if (option_types_found & OPT_P_UP)
@@ -270,12 +272,11 @@
      * we want to send exceeds that size we have to split it into smaller messages */
     ASSERT(push_bundle_size > extra);
     const size_t safe_cap = push_bundle_size - extra;
-    size_t msgs_num = (strlen(msg) / safe_cap) + ((strlen(msg) % safe_cap) != 0);
-    struct buffer *msgs = gc_malloc((msgs_num + 1) * sizeof(struct buffer), true, &gc);
+    struct buffer_list *msgs = buffer_list_new();
 
-    msgs[msgs_num].data = NULL;
     if (!message_splitter(msg, msgs, &gc, safe_cap))
     {
+        buffer_list_free(msgs);
         gc_free(&gc);
         return -EINVAL;
     }
@@ -286,6 +287,7 @@
 
         if (!mi)
         {
+            buffer_list_free(msgs);
             gc_free(&gc);
             return -ENOENT;
         }
@@ -293,6 +295,7 @@
         if (!support_push_update(mi))
         {
             msg(M_CLIENT, "PUSH_UPDATE: not sending message to unsupported peer with ID: %u", mi->context.c2.tls_multi->peer_id);
+            buffer_list_free(msgs);
             gc_free(&gc);
             return 0;
         }
@@ -300,11 +303,13 @@
         if (!mi->halt
             && send_single_push_update(m, mi, msgs))
         {
+            buffer_list_free(msgs);
             gc_free(&gc);
             return 1;
         }
         else
         {
+            buffer_list_free(msgs);
             gc_free(&gc);
             return 0;
         }
@@ -334,6 +339,7 @@
     }
 
     hash_iterator_free(&hi);
+    buffer_list_free(msgs);
     gc_free(&gc);
     return count;
 }
diff --git a/tests/unit_tests/openvpn/test_push_update_msg.c b/tests/unit_tests/openvpn/test_push_update_msg.c
index 516e94c..e450a31 100644
--- a/tests/unit_tests/openvpn/test_push_update_msg.c
+++ b/tests/unit_tests/openvpn/test_push_update_msg.c
@@ -136,11 +136,7 @@
 bool
 send_control_channel_string(struct context *c, const char *str, msglvl_t msglevel)
 {
-    if (res && res[i] && strcmp(res[i], str))
-    {
-        printf("\n\nexpected: %s\n\n  actual: %s\n\n", res[i], str);
-        return false;
-    }
+    check_expected(str);
     i++;
     return true;
 }
@@ -301,44 +297,75 @@
 
 #ifdef ENABLE_MANAGEMENT
 char *r0[] = {
-    "PUSH_UPDATE,redirect-gateway local,route 192.168.1.0 255.255.255.0"
+    "PUSH_UPDATE,redirect-gateway local,route 192.168.1.0 255.255.255.0",
+    NULL
 };
 char *r1[] = {
     "PUSH_UPDATE,-dhcp-option,blablalalalalalalalalalalalalf, lalalalalalalalalalalalalalaf,push-continuation 2",
     "PUSH_UPDATE, akakakakakakakakakakakaf, dhcp-option DNS 8.8.8.8,redirect-gateway local,push-continuation 2",
-    "PUSH_UPDATE,route 192.168.1.0 255.255.255.0,push-continuation 1"
+    "PUSH_UPDATE,route 192.168.1.0 255.255.255.0,push-continuation 1",
+    NULL
 };
 char *r3[] = {
-    "PUSH_UPDATE,,,"
+    "PUSH_UPDATE,,,",
+    NULL
 };
 char *r4[] = {
     "PUSH_UPDATE,-dhcp-option, blablalalalalalalalalalalalalf, lalalalalalalalalalalalalalaf,push-continuation 2",
     "PUSH_UPDATE, akakakakakakakakakakakaf,dhcp-option DNS 8.8.8.8, redirect-gateway local,push-continuation 2",
-    "PUSH_UPDATE, route 192.168.1.0 255.255.255.0,,push-continuation 1"
+    "PUSH_UPDATE, route 192.168.1.0 255.255.255.0,,push-continuation 1",
+    NULL
 };
 char *r5[] = {
     "PUSH_UPDATE,,-dhcp-option, blablalalalalalalalalalalalalf, lalalalalalalalalalalalalalaf,push-continuation 2",
     "PUSH_UPDATE, akakakakakakakakakakakaf,dhcp-option DNS 8.8.8.8, redirect-gateway local,push-continuation 2",
-    "PUSH_UPDATE, route 192.168.1.0 255.255.255.0,push-continuation 1"
+    "PUSH_UPDATE, route 192.168.1.0 255.255.255.0,push-continuation 1",
+    NULL
 };
 char *r6[] = {
     "PUSH_UPDATE,-dhcp-option,blablalalalalalalalalalalalalf, lalalalalalalalalalalalalalaf,push-continuation 2",
     "PUSH_UPDATE, akakakakakakakakakakakaf, dhcp-option DNS 8.8.8.8, redirect-gateway 10.10.10.10,,push-continuation 2",
-    "PUSH_UPDATE, route 192.168.1.0 255.255.255.0,,push-continuation 1"
+    "PUSH_UPDATE, route 192.168.1.0 255.255.255.0,,push-continuation 1",
+    NULL
 };
 char *r7[] = {
     "PUSH_UPDATE,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,push-continuation 2",
-    "PUSH_UPDATE,,,,,,,,,,,,,,,,,,,push-continuation 1"
+    "PUSH_UPDATE,,,,,,,,,,,,,,,,,,,push-continuation 1",
+    NULL
 };
 char *r8[] = {
     "PUSH_UPDATE,-dhcp-option,blablalalalalalalalalalalalalf, lalalalalalalalalalalalalalaf,push-continuation 2",
     "PUSH_UPDATE, akakakakakakakakakakakaf, dhcp-option DNS 8.8.8.8,redirect-gateway\n local,push-continuation 2",
-    "PUSH_UPDATE,route 192.168.1.0 255.255.255.0\n\n\n,push-continuation 1"
+    "PUSH_UPDATE,route 192.168.1.0 255.255.255.0\n\n\n,push-continuation 1",
+    NULL
 };
 char *r9[] = {
-    "PUSH_UPDATE,,"
+    "PUSH_UPDATE,,",
+    NULL
 };
-
+char *r11[] = {
+    "PUSH_UPDATE,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,push-continuation 2",
+    "PUSH_UPDATE,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,push-continuation 2",
+    "PUSH_UPDATE,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,push-continuation 2",
+    "PUSH_UPDATE,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,push-continuation 2",
+    "PUSH_UPDATE,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,push-continuation 1",
+    NULL
+};
+char *r12[] = {
+    "PUSH_UPDATE,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,,,,,,a,push-continuation 2",
+    "PUSH_UPDATE,abc,push-continuation 1",
+    NULL
+};
+char *r13[] = {
+    "PUSH_UPDATE,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,,,,,,a,",
+    NULL
+};
+char *r14[] = {
+    "PUSH_UPDATE,a,push-continuation 2",
+    "PUSH_UPDATE,aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa,push-continuation 2",
+    "PUSH_UPDATE,a,push-continuation 1",
+    NULL
+};
 
 const char *msg0 = "redirect-gateway local,route 192.168.1.0 255.255.255.0";
 const char *msg1 = "-dhcp-option,blablalalalalalalalalalalalalf, lalalalalalalalalalalalalalaf,"
@@ -365,8 +392,29 @@
                     "daisy damage damp dance danger daring dash daughter dawn day deal debate debris decade december decide decline"
                     "decorate decrease deer defense define defy degree delay deliver demand demise denial";
 
+const char *msg11 = "a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,"
+                    "a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,"
+                    "a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,"
+                    "a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,"
+                    "a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a";
+
+const char *msg12 = "a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,,,,,,a,abc";
+
+const char *msg13 = "a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,,,,,,a,";
+
+const char *msg14 = "a,aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa,a";
+
 #define PUSH_BUNDLE_SIZE_TEST 184
 
+#define expect_control_channel_strings()                             \
+    do                                                               \
+    {                                                                \
+        for (int j = 0; res[j] != NULL; j++)                         \
+        {                                                            \
+            expect_string(send_control_channel_string, str, res[j]); \
+        }                                                            \
+    } while (0)
+
 static void
 test_send_push_msg0(void **state)
 {
@@ -374,8 +422,10 @@
     res = r0;
     struct multi_context *m = *state;
     const unsigned long cid = 0;
+    expect_control_channel_strings();
     assert_int_equal(send_push_update(m, &cid, msg0, UPT_BY_CID, PUSH_BUNDLE_SIZE_TEST), 1);
 }
+
 static void
 test_send_push_msg1(void **state)
 {
@@ -383,6 +433,7 @@
     res = r1;
     struct multi_context *m = *state;
     const unsigned long cid = 0;
+    expect_control_channel_strings();
     assert_int_equal(send_push_update(m, &cid, msg1, UPT_BY_CID, PUSH_BUNDLE_SIZE_TEST), 1);
 }
 
@@ -403,6 +454,7 @@
     res = r3;
     struct multi_context *m = *state;
     const unsigned long cid = 0;
+    expect_control_channel_strings();
     assert_int_equal(send_push_update(m, &cid, msg3, UPT_BY_CID, PUSH_BUNDLE_SIZE_TEST), 1);
 }
 
@@ -413,6 +465,7 @@
     res = r4;
     struct multi_context *m = *state;
     const unsigned long cid = 0;
+    expect_control_channel_strings();
     assert_int_equal(send_push_update(m, &cid, msg4, UPT_BY_CID, PUSH_BUNDLE_SIZE_TEST), 1);
 }
 
@@ -423,6 +476,7 @@
     res = r5;
     struct multi_context *m = *state;
     const unsigned long cid = 0;
+    expect_control_channel_strings();
     assert_int_equal(send_push_update(m, &cid, msg5, UPT_BY_CID, PUSH_BUNDLE_SIZE_TEST), 1);
 }
 
@@ -433,6 +487,7 @@
     res = r6;
     struct multi_context *m = *state;
     const unsigned long cid = 0;
+    expect_control_channel_strings();
     assert_int_equal(send_push_update(m, &cid, msg6, UPT_BY_CID, PUSH_BUNDLE_SIZE_TEST), 1);
 }
 
@@ -443,6 +498,7 @@
     res = r7;
     struct multi_context *m = *state;
     const unsigned long cid = 0;
+    expect_control_channel_strings();
     assert_int_equal(send_push_update(m, &cid, msg7, UPT_BY_CID, PUSH_BUNDLE_SIZE_TEST), 1);
 }
 
@@ -453,6 +509,7 @@
     res = r8;
     struct multi_context *m = *state;
     const unsigned long cid = 0;
+    expect_control_channel_strings();
     assert_int_equal(send_push_update(m, &cid, msg8, UPT_BY_CID, PUSH_BUNDLE_SIZE_TEST), 1);
 }
 
@@ -463,6 +520,7 @@
     res = r9;
     struct multi_context *m = *state;
     const unsigned long cid = 0;
+    expect_control_channel_strings();
     assert_int_equal(send_push_update(m, &cid, msg9, UPT_BY_CID, PUSH_BUNDLE_SIZE_TEST), 1);
 }
 
@@ -476,6 +534,50 @@
     assert_int_equal(send_push_update(m, &cid, msg10, UPT_BY_CID, PUSH_BUNDLE_SIZE_TEST), -EINVAL);
 }
 
+static void
+test_send_push_msg11(void **state)
+{
+    i = 0;
+    res = r11;
+    struct multi_context *m = *state;
+    const unsigned long cid = 0;
+    expect_control_channel_strings();
+    assert_int_equal(send_push_update(m, &cid, msg11, UPT_BY_CID, PUSH_BUNDLE_SIZE_TEST), 1);
+}
+
+static void
+test_send_push_msg12(void **state)
+{
+    i = 0;
+    res = r12;
+    struct multi_context *m = *state;
+    const unsigned long cid = 0;
+    expect_control_channel_strings();
+    assert_int_equal(send_push_update(m, &cid, msg12, UPT_BY_CID, PUSH_BUNDLE_SIZE_TEST), 1);
+}
+
+static void
+test_send_push_msg13(void **state)
+{
+    i = 0;
+    res = r13;
+    struct multi_context *m = *state;
+    const unsigned long cid = 0;
+    expect_control_channel_strings();
+    assert_int_equal(send_push_update(m, &cid, msg13, UPT_BY_CID, PUSH_BUNDLE_SIZE_TEST), 1);
+}
+
+static void
+test_send_push_msg14(void **state)
+{
+    i = 0;
+    res = r14;
+    struct multi_context *m = *state;
+    const unsigned long cid = 0;
+    expect_control_channel_strings();
+    assert_int_equal(send_push_update(m, &cid, msg14, UPT_BY_CID, PUSH_BUNDLE_SIZE_TEST), 1);
+}
+
 #undef PUSH_BUNDLE_SIZE_TEST
 
 static int
@@ -535,6 +637,7 @@
         cmocka_unit_test_setup_teardown(test_incoming_push_message_mix, setup, teardown),
         cmocka_unit_test_setup_teardown(test_incoming_push_message_mix2, setup, teardown),
 #ifdef ENABLE_MANAGEMENT
+
         cmocka_unit_test_setup_teardown(test_send_push_msg0, setup2, teardown2),
         cmocka_unit_test_setup_teardown(test_send_push_msg1, setup2, teardown2),
         cmocka_unit_test_setup_teardown(test_send_push_msg2, setup2, teardown2),
@@ -545,7 +648,11 @@
         cmocka_unit_test_setup_teardown(test_send_push_msg7, setup2, teardown2),
         cmocka_unit_test_setup_teardown(test_send_push_msg8, setup2, teardown2),
         cmocka_unit_test_setup_teardown(test_send_push_msg9, setup2, teardown2),
-        cmocka_unit_test_setup_teardown(test_send_push_msg10, setup2, teardown2)
+        cmocka_unit_test_setup_teardown(test_send_push_msg10, setup2, teardown2),
+        cmocka_unit_test_setup_teardown(test_send_push_msg11, setup2, teardown2),
+        cmocka_unit_test_setup_teardown(test_send_push_msg12, setup2, teardown2),
+        cmocka_unit_test_setup_teardown(test_send_push_msg13, setup2, teardown2),
+        cmocka_unit_test_setup_teardown(test_send_push_msg14, setup2, teardown2)
 #endif
     };
 

-- 
To view, visit http://gerrit.openvpn.net/c/openvpn/+/1316?usp=email
To unsubscribe, or for help writing mail filters, visit http://gerrit.openvpn.net/settings?usp=email

Gerrit-MessageType: newpatchset
Gerrit-Project: openvpn
Gerrit-Branch: master
Gerrit-Change-Id: Idba419681fe3ccc4e6e2f6ce7592332dcff62cd9
Gerrit-Change-Number: 1316
Gerrit-PatchSet: 2
Gerrit-Owner: mrbff <ma...@ma...>
Gerrit-Reviewer: flichtenheld <fr...@li...>
Gerrit-Reviewer: plaisthos <arn...@rf...>
Gerrit-CC: openvpn-devel <ope...@li...>
Gerrit-Attention: plaisthos <arn...@rf...>
Gerrit-Attention: mrbff <ma...@ma...>

[Openvpn-devel] [S] Change in openvpn[master]: iservice: validate config path better

[cron2 (C. Review) <ge...@op...>]

Attention is currently required from: d12fk, flichtenheld, plaisthos.

cron2 has posted comments on this change by d12fk. ( http://gerrit.openvpn.net/c/openvpn/+/1307?usp=email )

Change subject: iservice: validate config path better
......................................................................


Patch Set 1: Code-Review-2

(1 comment)

Patchset:

PS1: 
This conflicted with Selva working on the same function, and I saw the other one first :-) - so we have commit 05a8ba808 in tree which does the `PathCchCanonicalizeEx()` bit, but not the `PathCchCombine()` part... arguably your patch is nicer, but does not apply anymore.  Can you do a new version that does the Combine thing based on master?



-- 
To view, visit http://gerrit.openvpn.net/c/openvpn/+/1307?usp=email
To unsubscribe, or for help writing mail filters, visit http://gerrit.openvpn.net/settings?usp=email

Gerrit-MessageType: comment
Gerrit-Project: openvpn
Gerrit-Branch: master
Gerrit-Change-Id: I0e94068f467f2899daf133b032a785d2d7fc05e4
Gerrit-Change-Number: 1307
Gerrit-PatchSet: 1
Gerrit-Owner: d12fk <he...@op...>
Gerrit-Reviewer: cron2 <ge...@gr...>
Gerrit-Reviewer: flichtenheld <fr...@li...>
Gerrit-Reviewer: plaisthos <arn...@rf...>
Gerrit-CC: openvpn-devel <ope...@li...>
Gerrit-Attention: plaisthos <arn...@rf...>
Gerrit-Attention: flichtenheld <fr...@li...>
Gerrit-Attention: d12fk <he...@op...>
Gerrit-Comment-Date: Tue, 28 Oct 2025 19:27:03 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: Yes

[Openvpn-devel] [S] Change in openvpn[master]: crypto_backend: Change len argument of md_ctx_update to size_t

[cron2 (C. Review) <ge...@op...>]

cron2 has submitted this change. ( http://gerrit.openvpn.net/c/openvpn/+/1287?usp=email )

Change subject: crypto_backend: Change len argument of md_ctx_update to size_t
......................................................................

crypto_backend: Change len argument of md_ctx_update to size_t

The underlying APIs already use size_t and all the
users (only httpdigest and push) already put size_t
into it. So avoid conversion warnings.

Also fix one trivial conversion warning in push.c
to able to easily remove the -Wconversion override
from the affected code paths.

Change-Id: I27f2fcd903d26ccbfbd0cdc45f99cc3cd8b0e49a
Signed-off-by: Frank Lichtenheld <fr...@li...>
Acked-by: Gert Doering <ge...@gr...>
Gerrit URL: https://gerrit.openvpn.net/c/openvpn/+/1287
Message-Id: <202...@gr...>
URL: https://www.mail-archive.com/ope...@li.../msg33973.html
Signed-off-by: Gert Doering <ge...@gr...>
---
M src/openvpn/crypto_backend.h
M src/openvpn/crypto_mbedtls.c
M src/openvpn/crypto_openssl.c
M src/openvpn/httpdigest.c
M src/openvpn/push.c
5 files changed, 18 insertions(+), 18 deletions(-)




diff --git a/src/openvpn/crypto_backend.h b/src/openvpn/crypto_backend.h
index 4d6a96c..e95752a 100644
--- a/src/openvpn/crypto_backend.h
+++ b/src/openvpn/crypto_backend.h
@@ -599,7 +599,7 @@
  * @param src           Buffer to digest. May not be NULL.
  * @param src_len       The length of the incoming buffer.
  */
-void md_ctx_update(md_ctx_t *ctx, const uint8_t *src, int src_len);
+void md_ctx_update(md_ctx_t *ctx, const uint8_t *src, size_t src_len);
 
 /*
  * Output the message digest to the given buffer.
diff --git a/src/openvpn/crypto_mbedtls.c b/src/openvpn/crypto_mbedtls.c
index 076d4ee..2e328c3 100644
--- a/src/openvpn/crypto_mbedtls.c
+++ b/src/openvpn/crypto_mbedtls.c
@@ -765,6 +765,10 @@
     return 1;
 }
 
+#if defined(__GNUC__) || defined(__clang__)
+#pragma GCC diagnostic pop
+#endif
+
 /*
  *
  * Generic message digest information functions
@@ -877,7 +881,7 @@
 }
 
 void
-md_ctx_update(mbedtls_md_context_t *ctx, const uint8_t *src, int src_len)
+md_ctx_update(mbedtls_md_context_t *ctx, const uint8_t *src, size_t src_len)
 {
     ASSERT(0 == mbedtls_md_update(ctx, src, src_len));
 }
@@ -994,6 +998,11 @@
                                        seed_len, output, output_len));
 }
 #else /* defined(HAVE_MBEDTLS_SSL_TLS_PRF) && defined(MBEDTLS_SSL_TLS_PRF_TLS1) */
+#if defined(__GNUC__) || defined(__clang__)
+#pragma GCC diagnostic push
+#pragma GCC diagnostic ignored "-Wconversion"
+#endif
+
 /*
  * Generate the hash required by for the \c tls1_PRF function.
  *
@@ -1122,10 +1131,10 @@
     gc_free(&gc);
     return true;
 }
-#endif /* HAVE_MBEDTLS_SSL_TLS_PRF && defined(MBEDTLS_SSL_TLS_PRF_TLS1) */
 
 #if defined(__GNUC__) || defined(__clang__)
 #pragma GCC diagnostic pop
 #endif
+#endif /* HAVE_MBEDTLS_SSL_TLS_PRF && defined(MBEDTLS_SSL_TLS_PRF_TLS1) */
 
 #endif /* ENABLE_CRYPTO_MBEDTLS */
diff --git a/src/openvpn/crypto_openssl.c b/src/openvpn/crypto_openssl.c
index f596b8c..ec0269c 100644
--- a/src/openvpn/crypto_openssl.c
+++ b/src/openvpn/crypto_openssl.c
@@ -1165,7 +1165,7 @@
 }
 
 void
-md_ctx_update(EVP_MD_CTX *ctx, const uint8_t *src, int src_len)
+md_ctx_update(EVP_MD_CTX *ctx, const uint8_t *src, size_t src_len)
 {
     EVP_DigestUpdate(ctx, src, src_len);
 }
diff --git a/src/openvpn/httpdigest.c b/src/openvpn/httpdigest.c
index f665b17..be20638 100644
--- a/src/openvpn/httpdigest.c
+++ b/src/openvpn/httpdigest.c
@@ -61,11 +61,6 @@
     Hex[HASHHEXLEN] = '\0';
 }
 
-#if defined(__GNUC__) || defined(__clang__)
-#pragma GCC diagnostic push
-#pragma GCC diagnostic ignored "-Wconversion"
-#endif
-
 /* calculate H(A1) as per spec */
 void
 DigestCalcHA1(IN char *pszAlg, IN char *pszUserName, IN char *pszRealm, IN char *pszPassword,
@@ -150,8 +145,4 @@
     CvtHex(RespHash, Response);
 }
 
-#if defined(__GNUC__) || defined(__clang__)
-#pragma GCC diagnostic pop
-#endif
-
 #endif /* if PROXY_DIGEST_AUTH */
diff --git a/src/openvpn/push.c b/src/openvpn/push.c
index 2c717c7..6f146fc 100644
--- a/src/openvpn/push.c
+++ b/src/openvpn/push.c
@@ -772,6 +772,10 @@
     return true;
 }
 
+#if defined(__GNUC__) || defined(__clang__)
+#pragma GCC diagnostic pop
+#endif
+
 void
 send_push_reply_auth_token(struct tls_multi *multi)
 {
@@ -1046,7 +1050,7 @@
                             unsigned int *option_types_found, struct buffer *buf)
 {
     int ret = PUSH_MSG_ERROR;
-    const uint8_t ch = buf_read_u8(buf);
+    const int ch = buf_read_u8(buf);
     if (ch == ',')
     {
         struct buffer buf_orig = (*buf);
@@ -1090,10 +1094,6 @@
     return ret;
 }
 
-#if defined(__GNUC__) || defined(__clang__)
-#pragma GCC diagnostic pop
-#endif
-
 int
 process_incoming_push_msg(struct context *c, const struct buffer *buffer,
                           bool honor_received_options, unsigned int permission_mask,

-- 
To view, visit http://gerrit.openvpn.net/c/openvpn/+/1287?usp=email
To unsubscribe, or for help writing mail filters, visit http://gerrit.openvpn.net/settings?usp=email

Gerrit-MessageType: merged
Gerrit-Project: openvpn
Gerrit-Branch: master
Gerrit-Change-Id: I27f2fcd903d26ccbfbd0cdc45f99cc3cd8b0e49a
Gerrit-Change-Number: 1287
Gerrit-PatchSet: 3
Gerrit-Owner: flichtenheld <fr...@li...>
Gerrit-Reviewer: cron2 <ge...@gr...>
Gerrit-Reviewer: plaisthos <arn...@rf...>
Gerrit-CC: openvpn-devel <ope...@li...>