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

[Mark Kettenis]

> From: Keith Packard <keithp at keithp.com>
> Date: Fri, 01 Nov 2013 15:08:55 -0700
> 
> > 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.
> 
> Yeah, I'm afraid that API didn't make any sense to me, so I just made it
> work in the only environment I had available. If you've got something
> that works differently and can test it, please send patches along.

The testing bit is a bit of a problem as there is no DRI3 support on
OpenBSD, at least not yet.  But I'll see if I can write a little bit
of test code that uses this code.  An I'll update the fixes to also
cover your new readv implementation.

> > 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.
> 
> The upper levels of the code are careful to not send more descriptors
> than the other end is willing to accept, so yeah, treating these cases
> as errors (to the point of just closing the client connection) would be
> sufficient.

You'll still need to close the file descriptors that did get passed in
the message.

Unfortunately you didn't respond to my general concern about adding
this file descriptor passing code:

> > 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.

I now see that you added code to xserver to cleanup unconsumed file
descriptors after each request.  However things like xfs don't contain
such code and will be vulnerable to a DOS attack like a describe
above.  And of course people building an old xserver release using a
new libxtrans will suffer from the same problem.

So I think that unconditionally defining XTRANS_SEND_FDS is a mistake.
I think it should be left to the component that embeds libxtrans to
request file descriptor passing support.  That is the only way
libxtrans can be used safely, i.e. something like the diff below.


Don't include file descriptor passing code by default

Leave it up to the consumer to request this functionality by defining
XTRANS_SEND_FDS.

Signed-off-by: Mark Kettenis <kettenis at openbsd.org>
---
 Xtransint.h | 2 --
 1 file changed, 2 deletions(-)

diff --git a/Xtransint.h b/Xtransint.h
index dd886db..5f96c8a 100644
--- a/Xtransint.h
+++ b/Xtransint.h
@@ -72,8 +72,6 @@ from The Open Group.
 #  define XTRANSDEBUG 1
 #endif
 
-#define XTRANS_SEND_FDS       1
-
 #ifdef WIN32
 # define _WILLWINSOCK_
 #endif
-- 
1.8.4.1