[S] Change in openvpn[master]: networking_sitnl: validate netlink replies against the request

Attention is currently required from: plaisthos.

Hello plaisthos,

I'd like you to do a code review.
Please visit

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

to review the following change.


Change subject: networking_sitnl: validate netlink replies against the request
......................................................................

networking_sitnl: validate netlink replies against the request

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]>
---
M src/openvpn/networking_sitnl.c
1 file changed, 30 insertions(+), 17 deletions(-)



  git pull ssh://gerrit.openvpn.net:29418/openvpn refs/changes/82/1782/1

diff --git a/src/openvpn/networking_sitnl.c b/src/openvpn/networking_sitnl.c
index b3f8e2b..3c03315 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,8 @@
         return -EINVAL;
     }
 
+    *local_pid = local.nl_pid;
+
     return 0;
 }
 
@@ -243,6 +245,7 @@
            void *arg_cb)
 {
     int fd, ret;
+    uint32_t local_pid = 0;
     struct sockaddr_nl nladdr;
     struct nlmsgerr *err;
     struct nlmsghdr *h;
@@ -264,11 +267,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 +292,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 +366,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)
             {

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

Gerrit-MessageType: newchange
Gerrit-Project: openvpn
Gerrit-Branch: master
Gerrit-Change-Id: Ie7352dd136cccda2bd6b6e1a36db1a5f27afd8f7
Gerrit-Change-Number: 1782
Gerrit-PatchSet: 1
Gerrit-Owner: ordex <[email protected]>
Gerrit-Reviewer: plaisthos <[email protected]>
Gerrit-CC: openvpn-devel <[email protected]>
Gerrit-Attention: plaisthos <[email protected]>

_______________________________________________
Openvpn-devel mailing list
[email protected]
https://lists.sourceforge.net/lists/listinfo/openvpn-devel