diff mbox series

[Openvpn-devel,v1] Ensure pushed tun-mtu is no lower than TUN_MTU_MIN

Message ID 20260605180931.3547-1-gert@greenie.muc.de
State New
Headers
Series [Openvpn-devel,v1] Ensure pushed tun-mtu is no lower than TUN_MTU_MIN |

Commit Message

Gert Doering June 5, 2026, 6:09 p.m. UTC 
  From: Arne Schwabe <arne@rfc2549.org>

The normal path ensure that the minimum tun mtu is set to at least
TUN_MTU_MIN. However, the pushed options path does not have this
restriction.

Check that the tun-mtu is within the limits of min/max mtu
in options.c. This ensure that the check is also correctly done
on the pushed variant.

Also add an extra check to keep the allowed payload for icmp6 packets
to be at least 64 bytes in the the block-ipv6 code path
(ipv6_send_icmp_unreachable) as extra layer of defence.

Pushing a low mtu like 1 and also block-ipv6 could trigger an
assertion in the ipv6_send_icmp_unreachable code path.

Reported-By: Haiyang Huang <huanghaiyang83@gmail.com>
Change-Id: Iff8b336126a5dff9871213664b1e8585fb70d21e
Signed-off-by: Arne Schwabe <arne-openvpn@rfc2549.org>
Acked-by: MaxF <max@max-fillinger.net>
Gerrit URL: https://gerrit.openvpn.net/c/openvpn/+/1707
---

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/+/1707
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):
MaxF <max@max-fillinger.net>

Comments

Gert Doering June 7, 2026, 5 p.m. UTC | #1 
The patch is fairly straightforward, and got a thorough review from Max
(thanks).

It was reported to us as a security vulnerability, but after some internal
discussion it was decided to not treat it as such, and not assign a CVE -
yes, the server can push a value that will make the client ASSERT() and
exit, but there are many other ways a server can achieve this (like,
push "ping-exit 1").  It can not be used to overflow anything, discover
secrets, execute code on the client, etc., and it can also not be used
by a non-trusted third party or by a client to crash a server.

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

I think the fix should go to release/2.6 as well (bugfix), but since the
atoi_constrained() infrastructure is not there, the options.c patch does
not apply "as is" (the forward.c likely would).

commit aca8546df6b04afee275b78f92f5564f5590f76a (master)
commit b2d92f17b5c956346b951e21f216d0dae8f1239d (release/2.7)
Author: Arne Schwabe
Date:   Fri Jun 5 20:09:25 2026 +0200

     Ensure pushed tun-mtu is no lower than TUN_MTU_MIN

     Signed-off-by: Arne Schwabe <arne-openvpn@rfc2549.org>
     Acked-by: MaxF <max@max-fillinger.net>
     Gerrit URL: https://gerrit.openvpn.net/c/openvpn/+/1707
     Message-Id: <20260605180931.3547-1-gert@greenie.muc.de>
     URL: https://www.mail-archive.com/openvpn-devel@lists.sourceforge.net/msg37069.html
     Signed-off-by: Gert Doering <gert@greenie.muc.de>


--
kind regards,

Gert Doering
diff mbox series

Patch

diff --git a/src/openvpn/forward.c b/src/openvpn/forward.c
index 27cfd36..d24f534 100644
--- a/src/openvpn/forward.c
+++ b/src/openvpn/forward.c
@@ -1598,7 +1598,9 @@ 
      * frame should be <= 1280 and have as much as possible of the original
      * packet
      */
-    const int max_payload_size = min_int(MAX_ICMPV6LEN, c->c2.frame.tun_mtu - icmpheader_len);
+    int max_payload_size = min_int(MAX_ICMPV6LEN, c->c2.frame.tun_mtu - icmpheader_len);
+    /* Ensure that minimum payload size is at least 64 bytes as extra safety layer */
+    max_payload_size = max_int(max_payload_size, 64);
     const int payload_len = min_int(max_payload_size, BLEN(&inputipbuf));
     const uint16_t icmp_len = (uint16_t)(sizeof(struct openvpn_icmp6hdr) + payload_len);
 
diff --git a/src/openvpn/mtu.h b/src/openvpn/mtu.h
index ca8109c..8f07e75 100644
--- a/src/openvpn/mtu.h
+++ b/src/openvpn/mtu.h
@@ -68,6 +68,11 @@ 
  */
 #define TUN_MTU_DEFAULT 1500
 
+/**
+ * Maximum MTU we accept for MTU related options
+ */
+#define TUN_MTU_MAX 65536
+
 /*
  * Minimum maximum MTU
  */
diff --git a/src/openvpn/options.c b/src/openvpn/options.c
index 0c2866c..128d1e5 100644
--- a/src/openvpn/options.c
+++ b/src/openvpn/options.c
@@ -6409,26 +6409,28 @@ 
     else if (streq(p[0], "tun-mtu") && p[1] && !p[3])
     {
         VERIFY_PERMISSION(OPT_P_PUSH_MTU | OPT_P_CONNECTION);
-        options->ce.tun_mtu = positive_atoi(p[1], msglevel);
-        options->ce.tun_mtu_defined = true;
-        if (p[2])
+        if (atoi_constrained(p[1], &options->ce.tun_mtu, "tun-mtu", TUN_MTU_MIN, TUN_MTU_MAX, msglevel))
         {
-            options->ce.occ_mtu = positive_atoi(p[2], msglevel);
-        }
-        else
-        {
-            options->ce.occ_mtu = 0;
+            options->ce.tun_mtu_defined = true;
+            if (p[2])
+            {
+                atoi_constrained(p[2], &options->ce.occ_mtu, "tun-mtu occ-mtu", TUN_MTU_MIN, TUN_MTU_MAX, msglevel);
+            }
+            else
+            {
+                options->ce.occ_mtu = 0;
+            }
         }
     }
     else if (streq(p[0], "tun-mtu-max") && p[1] && !p[2])
     {
         VERIFY_PERMISSION(OPT_P_MTU | OPT_P_CONNECTION);
-        atoi_constrained(p[1], &options->ce.tun_mtu_max, p[0], TUN_MTU_MAX_MIN, 65536, msglevel);
+        atoi_constrained(p[1], &options->ce.tun_mtu_max, p[0], TUN_MTU_MAX_MIN, TUN_MTU_MAX, msglevel);
     }
     else if (streq(p[0], "tun-mtu-extra") && p[1] && !p[2])
     {
         VERIFY_PERMISSION(OPT_P_MTU | OPT_P_CONNECTION);
-        if (atoi_constrained(p[1], &options->ce.tun_mtu_extra, p[0], 0, 65536, msglevel))
+        if (atoi_constrained(p[1], &options->ce.tun_mtu_extra, p[0], 0, TUN_MTU_MAX, msglevel))
         {
             options->ce.tun_mtu_extra_defined = true;
         }