This is an automated email from the git hooks/post-receive script. x2go pushed a commit to branch 3.6.x in repository nx-libs. commit 6198e0376f9ce8130af3294fa284659f0055610d Author: Ulrich Sibiller <uli42@gmx.de> Date: Wed Dec 27 14:53:58 2017 +0100 nxcomp: implement correct length handling for unix socket structs (partially) fixes ArcticaProject/nx-libs#612 --- nxcomp/src/Loop.cpp | 67 +++++++++++++++++++++++++++++++++++++++------------- nxcomp/src/Proxy.cpp | 18 +++++--------- 2 files changed, 57 insertions(+), 28 deletions(-) diff --git a/nxcomp/src/Loop.cpp b/nxcomp/src/Loop.cpp index d094bb7..3ee094e 100644 --- a/nxcomp/src/Loop.cpp +++ b/nxcomp/src/Loop.cpp @@ -55,6 +55,8 @@ #include "Misc.h" +#include <cstddef> + #ifdef __sun #include <strings.h> #endif @@ -3590,19 +3592,14 @@ int SetupAuthInstance() launchdAddrUnix.sun_family = AF_UNIX; - #ifdef __linux__ - const int launchdAddrNameLength = 108; - #else - /* POSIX/SUS does not specify a length. - * BSD derivatives generally support 104 bytes, other systems may be more constrained. - * If you happen to run into such systems, extend this section with the appropriate limit. - */ - const int launchdAddrNameLength = 104; - #endif + // determine the maximum number of characters that fit into struct + // sockaddr_un's sun_path member + std::size_t launchdAddrNameLength = + sizeof(struct sockaddr_un) - offsetof(struct sockaddr_un, sun_path); int success = -1; - strncpy(launchdAddrUnix.sun_path, displayHost, launchdAddrNameLength); + snprintf(launchdAddrUnix.sun_path, launchdAddrNameLength, "%s", displayHost); *(launchdAddrUnix.sun_path + launchdAddrNameLength - 1) = '\0'; @@ -3909,6 +3906,11 @@ void SetupDisplaySocket(int &addr_family, sockaddr *&addr, // UNIX domain port. // + // determine the maximum number of characters that fit into struct + // sockaddr_un's sun_path member + std::size_t maxlen_un = + sizeof(struct sockaddr_un) - offsetof(struct sockaddr_un, sun_path); + nxinfo << "Loop: Using real X server on UNIX domain socket.\n" << std::flush; @@ -3943,10 +3945,28 @@ void SetupDisplaySocket(int &addr_family, sockaddr *&addr, sockaddr_un *xServerAddrABSTRACT = new sockaddr_un; memset(xServerAddrABSTRACT, 0, sizeof(struct sockaddr_un)); xServerAddrABSTRACT -> sun_family = AF_UNIX; - // FIXME: ensure sun_path can hold len+1 bytes! - memcpy(xServerAddrABSTRACT -> sun_path, unixSocketName, len+1); - // FIXME: comment why + 3? - addr_length = len + 3; + + if (maxlen_un < (unsigned int)len + 1) + { + nxfatal << "Loop: PANIC! Abstract socket name '" << unixSocketName + 1 + << "' is too long!" << std::flush; + + delete [] display; + delete xServerAddrABSTRACT; + + HandleCleanup(); + } + + // copy including the leading '\0' + memcpy(xServerAddrABSTRACT -> sun_path, unixSocketName, len + 1); + + // man 7 unix: + // "an abstract socket address is distinguished (from a + // pathname socket) by the fact that sun_path[0] is a null byte + // ('\0'). The socket's address in this namespace is given by the + // additional bytes in sun_path that are covered by the specified + // length of the address structure." + addr_length = offsetof(struct sockaddr_un, sun_path) + len + 1; int ret = connect(testSocketFD, (struct sockaddr *) xServerAddrABSTRACT, @@ -4032,8 +4052,17 @@ void SetupDisplaySocket(int &addr_family, sockaddr *&addr, nxinfo << "Loop: Assuming X socket name '" << unixSocketName << "'.\n" << std::flush; - sockaddr_un *xServerAddrUNIX = new sockaddr_un; + if (maxlen_un < strlen(unixSocketName) + 1) + { + nxfatal << "Loop: PANIC! Socket name '" << unixSocketName + << "' is too long!" << std::flush; + delete [] display; + + HandleCleanup(); + } + + sockaddr_un *xServerAddrUNIX = new sockaddr_un; xServerAddrUNIX -> sun_family = AF_UNIX; strcpy(xServerAddrUNIX -> sun_path, unixSocketName); @@ -6539,12 +6568,18 @@ int PrepareProxyConnectionUnix(char** path, int* timeout, int* proxyFileDescript /* FIXME: Add socket file existence and permission checks */ + *proxyFileDescriptor = -1; *reason = -1; + // determine the maximum number of characters that fit into struct + // sockaddr_un's sun_path member + const std::size_t sockpathlen = + sizeof(struct sockaddr_un) - offsetof(struct sockaddr_un, sun_path); + sockaddr_un addr; addr.sun_family = AF_UNIX; - strncpy(addr.sun_path, *path, 108 - 1); + snprintf(addr.sun_path, sockpathlen, "%s", *path); *proxyFileDescriptor = socket(AF_UNIX, SOCK_STREAM, PF_UNSPEC); *reason = EGET(); diff --git a/nxcomp/src/Proxy.cpp b/nxcomp/src/Proxy.cpp index 437296f..963ae3d 100644 --- a/nxcomp/src/Proxy.cpp +++ b/nxcomp/src/Proxy.cpp @@ -30,6 +30,7 @@ #include <cstdio> #include <unistd.h> #include <cstdlib> +#include <cstddef> #include <string.h> #include <sys/types.h> #include <sys/stat.h> @@ -6294,19 +6295,12 @@ int Proxy::handleNewGenericConnectionFromProxyUnix(int channelId, T_channel_type serverAddrUnix.sun_family = AF_UNIX; - #ifdef __linux__ - const int serverAddrNameLength = 108; - #else - /* POSIX/SUS does not specify a length. - * BSD derivatives generally support 104 bytes, other systems may be more constrained. - * If you happen to run into such systems, extend this section with the appropriate limit. - */ - const int serverAddrNameLength = 104; - #endif - - strncpy(serverAddrUnix.sun_path, path, serverAddrNameLength); + // determine the maximum number of characters that fit into struct + // sockaddr_un's sun_path member + std::size_t serverAddrNameLength = + sizeof(struct sockaddr_un) - offsetof(struct sockaddr_un, sun_path); - *(serverAddrUnix.sun_path + serverAddrNameLength - 1) = '\0'; + snprintf(serverAddrUnix.sun_path, serverAddrNameLength, "%s", path); #ifdef TEST *logofs << "Proxy: Connecting to " << label << " server " -- Alioth's /home/x2go-admin/maintenancescripts/git/hooks/post-receive-email on /srv/git/code.x2go.org/nx-libs.git