diff mbox series

[Openvpn-devel,v3] networking_sitnl: validate netlink replies against the request

Message ID 20260901151017.16221-1-gert@greenie.muc.de
State New
Headers
Series [Openvpn-devel,v3] networking_sitnl: validate netlink replies against the request |

Commit Message

Gert Doering Sept. 1, 2026, 3:10 p.m. UTC 
  From: Antonio Quartulli <antonio@mandelbit.com>

sitnl_send() passed every well-formed reply to the callback without
checking it matched the outstanding request: the seq/pid check was
commented out and the sequence number came from time(NULL).

Seed a monotonically increasing sequence number and drop any message
that is not from the kernel (nl_pid 0) or does not echo our port id and
sequence number.

Change-Id: Ie7352dd136cccda2bd6b6e1a36db1a5f27afd8f7
GitHub: fixes OpenVPN/openvpn-private-issues#9
Signed-off-by: Antonio Quartulli <antonio@mandelbit.com>
Acked-by: Ralf Lici <ralf@mandelbit.com>
Gerrit URL: https://gerrit.openvpn.net/c/openvpn/+/1782
---

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

Acked-by according to Gerrit (reflected above):
Ralf Lici <ralf@mandelbit.com>

Comments

Gert Doering Sept. 1, 2026, 5:01 p.m. UTC | #1 
This is more a theoretical attack, with netlink replies received that
do not match the request, and our state machine getting confused as
a consequence... but that extra check won't do harm, and might catch
possible errors elsewhere.  Antonio and Ralf are the ones who understand
Linux kernel, who am I to argue :-)

I have only lightly stared at the code (complained, got a v3 in return),
and tested on an oldish Ubuntu with and without DCO (works).

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

commit 90ab7be00e73d06f7a5bf72e6a454fcfd5f2818c (master).
commit fbcf3c449787ba200731ff65b3ffa8fb4118a8e5 (release/2.7)
Author: Antonio Quartulli
Date:   Tue Sep 1 17:10:10 2026 +0200

     networking_sitnl: validate netlink replies against the request

     Signed-off-by: Antonio Quartulli <antonio@mandelbit.com>
     Acked-by: Ralf Lici <ralf@mandelbit.com>
     Gerrit URL: https://gerrit.openvpn.net/c/openvpn/+/1782
     Message-Id: <20260901151017.16221-1-gert@greenie.muc.de>
     URL: https://www.mail-archive.com/openvpn-devel@lists.sourceforge.net/msg38879.html
     Signed-off-by: Gert Doering <gert@greenie.muc.de>


--
kind regards,

Gert Doering
diff mbox series

Patch

diff --git a/src/openvpn/networking_sitnl.c b/src/openvpn/networking_sitnl.c
index e6e72d0..a396255 100644
--- a/src/openvpn/networking_sitnl.c
+++ b/src/openvpn/networking_sitnl.c
@@ -197,7 +197,7 @@ 
  * Bind socket to Netlink subsystem
  */
 static int
-sitnl_bind(int fd, uint32_t groups)
+sitnl_bind(int fd, uint32_t groups, uint32_t *local_pid)
 {
     socklen_t addr_len;
     struct sockaddr_nl local;
@@ -232,6 +232,14 @@ 
         return -EINVAL;
     }
 
+    /* We bound with nl_pid=0, so the kernel assigned this socket a unique port
+     * id (it is not the process pid - a process may own several netlink
+     * sockets). getsockname() above is the only way to learn it: hand it back
+     * to the caller, which uses it to check that replies are addressed to this
+     * socket.
+     */
+    *local_pid = local.nl_pid;
+
     return 0;
 }
 
@@ -243,6 +251,7 @@ 
            void *arg_cb)
 {
     int fd, ret;
+    uint32_t local_pid = 0;
     struct sockaddr_nl nladdr;
     struct nlmsgerr *err;
     struct nlmsghdr *h;
@@ -264,11 +273,17 @@ 
     nladdr.nl_pid = peer;
     nladdr.nl_groups = groups;
 
-    /* NB: We currently do not verify seq and pid on answers.
-     * If we ever want to start with that we probably need to come up
-     * with something better than "seconds since epoch"...
+    /* Match replies to requests with a monotonically increasing sequence
+     * number, seeded once from wall-clock time so it differs between runs.
+     * The kernel echoes this seq (and our port id) in its replies, letting the
+     * receive loop below discard any unrelated or spoofed message.
      */
-    payload->nlmsg_seq = (uint32_t)time(NULL);
+    static uint32_t sitnl_seq;
+    if (!sitnl_seq)
+    {
+        sitnl_seq = (uint32_t)time(NULL);
+    }
+    payload->nlmsg_seq = ++sitnl_seq;
 
     /* no need to send reply */
     if (!cb)
@@ -283,7 +298,7 @@ 
         return -errno;
     }
 
-    if (sitnl_bind(fd, 0) < 0)
+    if (sitnl_bind(fd, 0, &local_pid) < 0)
     {
         msg(M_WARN | M_ERRNO, "%s: can't bind rtnl socket", __func__);
         ret = -errno;
@@ -357,18 +372,23 @@ 
                 goto out;
             }
 
-            /*            if (((int)nladdr.nl_pid != peer) || (h->nlmsg_pid != nladdr.nl_pid)
-             *               || (h->nlmsg_seq != seq))
-             *           {
-             *               rcv_len -= NLMSG_ALIGN(len);
-             *               h = (struct nlmsghdr *)((char *)h + NLMSG_ALIGN(len));
-             *               msg(M_DEBUG, "%s: skipping unrelated message. nl_pid:%d (peer:%d)
-             * nl_msg_pid:%d nl_seq:%d seq:%d",
-             *                   __func__, (int)nladdr.nl_pid, peer, h->nlmsg_pid,
-             *                   h->nlmsg_seq, seq);
-             *               continue;
-             *           }
+            /* Discard any message that did not come from the kernel or does
+             * not match the request we sent: only the kernel (nl_pid 0) can
+             * legitimately reply, and a valid reply echoes our port id and
+             * sequence number. This prevents a local process from injecting a
+             * spoofed reply that the callback would otherwise act on.
              */
+            if ((nladdr.nl_pid != 0) || (h->nlmsg_pid != local_pid)
+                || (h->nlmsg_seq != payload->nlmsg_seq))
+            {
+                msg(D_RTNL,
+                    "%s: skipping unrelated message. nl_pid:%u nlmsg_pid:%u (local:%u) nlmsg_seq:%u (seq:%u)",
+                    __func__, nladdr.nl_pid, h->nlmsg_pid, local_pid, h->nlmsg_seq,
+                    payload->nlmsg_seq);
+                rcv_len -= NLMSG_ALIGN(len);
+                h = (struct nlmsghdr *)((char *)h + NLMSG_ALIGN(len));
+                continue;
+            }
 
             if (h->nlmsg_type == NLMSG_DONE)
             {