[PATCH v2] networking_sitnl: validate netlink replies against the request

From: Antonio Quartulli <[email protected]>

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 <[email protected]>
Acked-by: Ralf Lici <[email protected]>
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 2 of this Change.

Acked-by according to Gerrit (reflected above):
Ralf Lici <[email protected]>

        
diff --git a/src/openvpn/networking_sitnl.c b/src/openvpn/networking_sitnl.c
index e6e72d0..7d623a4 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);
+    }
+    uint32_t seq = 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,22 @@
                 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 != 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, seq);
+                rcv_len -= NLMSG_ALIGN(len);
+                h = (struct nlmsghdr *)((char *)h + NLMSG_ALIGN(len));
+                continue;
+            }
 
             if (h->nlmsg_type == NLMSG_DONE)
             {