> From: Keith Packard <[email protected]>
> 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);
_______________________________________________
[email protected]: X.Org development
Archives: http://lists.x.org/archives/xorg-devel
Info: http://lists.x.org/mailman/listinfo/xorg-devel
