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
