[PATCH 2/2] Add APIs to send file descriptors through the network

[Mark Kettenis]

> From: Keith Packard <keithp at keithp.com>
> Date: Thu, 31 Oct 2013 13:10:25 -0700
> 
> Exposes new TRANS(SendFd)/TRANS(RecvFd) APIs.

Keith.  This implementation isn't quite right as it doesn't use the
proper CMSG_ macros to manipulate the ancillary data object
information.  You get way with this on Linux, because it deviates from
POSIX and declares the cmsg_len member as size_t, which means no
additional padding between the cmsghdr and the data array is
necessary.  But on other systems this code won't work.  Attached is an
(untested) diff that should fix this.  If you didn't do so yet, please
read appendix A of RFC 3542, which has a decent description of the API.

I also believe the handling of MSG_TRUNC and MSG_CTRUNC isn't correct,
and will result in leaked file descriptors.  If I read the Linux
kernel code correctly MSG_CTRUNC gets set when there is not enough
room to store all filedescriptors.  But you'll still get the
filedescriptors that fit.  So you either need to attach those file
descriptors or perhaps close them.  And I don't think you should check
for MSG_TRUNC at all as it provides no indication of whether you
received any file descriptors or not.  If you consider MSG_TRUNC to be
an error, you should at least close the file descriptors.

But even with that fixed, there are some serious security issues with
the approach taken here.  Anything that calls SocketRead() will
blindly accept file descriptors.  And if I understand things correctly
that includes xserver.  So a malicious client can easily run the
server out of file descriptors by passing file descriptors along with
requests for which the server doesn't expect any.


diff --git a/Xtranssock.c b/Xtranssock.c
index 23150b2..c979132 100644
--- a/Xtranssock.c
+++ b/Xtranssock.c
@@ -2197,25 +2197,28 @@ TRANS(SocketSendFdInvalid)(XtransConnInfo ciptr, int fd, int do_close)
 
 #define MAX_FDS		128
 
-struct fd_pass {
+union fd_pass {
 	struct cmsghdr	cmsghdr;
-	int		fd[MAX_FDS];
+	char		buf[CMSG_SPACE(MAX_FDS * sizeof(int))];
 };
 
-static inline void init_msg_recv(struct msghdr *msg, struct iovec *iov, int niov, struct fd_pass *pass, int nfd) {
+static inline void init_msg_recv(struct msghdr *msg, struct iovec *iov, int niov, union fd_pass *pass, int nfd) {
     msg->msg_name = NULL;
     msg->msg_namelen = 0;
     msg->msg_iov = iov;
     msg->msg_iovlen = niov;
     msg->msg_control = pass;
-    msg->msg_controllen = sizeof (struct cmsghdr) + nfd * sizeof (int);
+    msg->msg_controllen = CMSG_LEN(nfd * sizeof(int));
 }
 
-static inline void init_msg_send(struct msghdr *msg, struct iovec *iov, int niov, struct fd_pass *pass, int nfd) {
+static inline void init_msg_send(struct msghdr *msg, struct iovec *iov, int niov, union fd_pass *pass, int nfd) {
+    struct cmsghdr *cmsghdr;
+
     init_msg_recv(msg, iov, niov, pass, nfd);
-    pass->cmsghdr.cmsg_len = msg->msg_controllen;
-    pass->cmsghdr.cmsg_level = SOL_SOCKET;
-    pass->cmsghdr.cmsg_type = SCM_RIGHTS;
+    cmsghdr = CMSG_FIRSTHDR(msg);
+    cmsghdr->cmsg_len = msg->msg_controllen;
+    cmsghdr->cmsg_level = SOL_SOCKET;
+    cmsghdr->cmsg_type = SCM_RIGHTS;
 }
 
 #endif /* XTRANS_SEND_FDS */
@@ -2239,21 +2242,25 @@ TRANS(SocketRead) (XtransConnInfo ciptr, char *buf, int size)
     {
         struct msghdr   msg;
         struct iovec    iov;
-        struct fd_pass  pass;
+        union fd_pass   pass;
 
         iov.iov_base = buf;
         iov.iov_len = size;
 
         init_msg_recv(&msg, &iov, 1, &pass, MAX_FDS);
         size = recvmsg(ciptr->fd, &msg, 0);
-        if (size >= 0 && msg.msg_controllen > sizeof (struct cmsghdr)) {
-            if (pass.cmsghdr.cmsg_level == SOL_SOCKET &&
-                pass.cmsghdr.cmsg_type == SCM_RIGHTS &&
+
+        if (size >= 0) {
+            struct cmsghdr *cmsghdr = CMSG_FIRSTHDR(&msg);
+
+            if (cmsghdr != NULL &&
+        	cmsghdr->cmsg_level == SOL_SOCKET &&
+                cmsghdr->cmsg_type == SCM_RIGHTS &&
                 !((msg.msg_flags & MSG_TRUNC) ||
                   (msg.msg_flags & MSG_CTRUNC)))
             {
-                int nfd = (msg.msg_controllen - sizeof (struct cmsghdr)) / sizeof (int);
-                int *fd = (int *) CMSG_DATA(&pass.cmsghdr);
+                int nfd = (msg.msg_controllen - CMSG_LEN(0)) / sizeof(int);
+                int *fd = (int *) CMSG_DATA(cmsghdr);
                 int i;
                 for (i = 0; i < nfd; i++)
                     appendFd(&ciptr->recv_fds, fd[i], 0);
@@ -2287,7 +2294,9 @@ TRANS(SocketWritev) (XtransConnInfo ciptr, struct iovec *buf, int size)
     if (ciptr->send_fds)
     {
         struct msghdr           msg;
-        struct fd_pass          pass;
+        struct cmsghdr          *cmsghdr;
+        union fd_pass           pass;
+        int                     *fd;
         int                     nfd;
         struct _XtransConnFd    *cf;
         int                     i;
@@ -2295,13 +2304,16 @@ TRANS(SocketWritev) (XtransConnInfo ciptr, struct iovec *buf, int size)
         nfd = nFd(&ciptr->send_fds);
         cf = ciptr->send_fds;
 
+        init_msg_send(&msg, buf, size, &pass, nfd);
+
         /* Set up fds */
+        cmsghdr = CMSG_FIRSTHDR(&msg);
+        fd = (int *) CMSG_DATA(cmsghdr);
         for (i = 0; i < nfd; i++) {
-            pass.fd[i] = cf->fd;
+            fd[i] = cf->fd;
             cf = cf->next;
         }
 
-        init_msg_send(&msg, buf, size, &pass, nfd);
         i = sendmsg(ciptr->fd, &msg, 0);
         if (i > 0)
             discardFd(&ciptr->send_fds, cf, 0);