[lxc-devel] [lxc/master] bugfixes: {segfaults, hashes, abstract unix sockets}

brauner on Github lxc-bot at linuxcontainers.org
Sat May 6 21:52:51 UTC 2017


A non-text attachment was scrubbed...
Name: not available
Type: text/x-mailbox
Size: 364 bytes
Desc: not available
URL: <http://lists.linuxcontainers.org/pipermail/lxc-devel/attachments/20170506/43b3bcca/attachment.bin>
-------------- next part --------------
From 3caaf485e7999f788ea817c802cbaf5e545995ba Mon Sep 17 00:00:00 2001
From: Christian Brauner <christian.brauner at ubuntu.com>
Date: Sat, 6 May 2017 18:33:28 +0200
Subject: [PATCH 1/5] af unix: allow for maximum socket name

Abstract unix sockets need not be \0-terminated. So you can effectively have
107 chars available. If you \0-terminate you'll have a 106. Don't enforce
\0-termination in these low-level functions. Enforce it higher up which we
already do.

Signed-off-by: Christian Brauner <christian.brauner at ubuntu.com>
---
 src/lxc/af_unix.c | 24 ++++++++++++------------
 1 file changed, 12 insertions(+), 12 deletions(-)

diff --git a/src/lxc/af_unix.c b/src/lxc/af_unix.c
index 46d8e50..b529dd1 100644
--- a/src/lxc/af_unix.c
+++ b/src/lxc/af_unix.c
@@ -55,7 +55,7 @@ int lxc_abstract_unix_open(const char *path, int type, int flags)
 
 	addr.sun_family = AF_UNIX;
 
-	len = strlen(&path[1]) + 1;
+	len = strlen(&path[1]);
 	if (len >= sizeof(addr.sun_path) - 1) {
 		close(fd);
 		errno = ENAMETOOLONG;
@@ -64,7 +64,7 @@ int lxc_abstract_unix_open(const char *path, int type, int flags)
 	/* addr.sun_path[0] has already been set to 0 by memset() */
 	strncpy(&addr.sun_path[1], &path[1], strlen(&path[1]));
 
-	if (bind(fd, (struct sockaddr *)&addr, offsetof(struct sockaddr_un, sun_path) + len)) {
+	if (bind(fd, (struct sockaddr *)&addr, offsetof(struct sockaddr_un, sun_path) + len + 1)) {
 		int tmp = errno;
 		close(fd);
 		errno = tmp;
@@ -109,7 +109,7 @@ int lxc_abstract_unix_connect(const char *path)
 
 	addr.sun_family = AF_UNIX;
 
-	len = strlen(&path[1]) + 1;
+	len = strlen(&path[1]);
 	if (len >= sizeof(addr.sun_path) - 1) {
 		close(fd);
 		errno = ENAMETOOLONG;
@@ -118,7 +118,7 @@ int lxc_abstract_unix_connect(const char *path)
 	/* addr.sun_path[0] has already been set to 0 by memset() */
 	strncpy(&addr.sun_path[1], &path[1], strlen(&path[1]));
 
-	if (connect(fd, (struct sockaddr *)&addr, offsetof(struct sockaddr_un, sun_path) + len)) {
+	if (connect(fd, (struct sockaddr *)&addr, offsetof(struct sockaddr_un, sun_path) + len + 1)) {
 		int tmp = errno;
 		/* special case to connect to older containers */
 		if (connect(fd, (struct sockaddr *)&addr, sizeof(addr)) == 0)
@@ -136,8 +136,8 @@ int lxc_abstract_unix_send_fd(int fd, int sendfd, void *data, size_t size)
 	struct msghdr msg = { 0 };
 	struct iovec iov;
 	struct cmsghdr *cmsg;
-	char cmsgbuf[CMSG_SPACE(sizeof(int))];
-	char buf[1];
+	char cmsgbuf[CMSG_SPACE(sizeof(int))] = {0};
+	char buf[1] = {0};
 	int *val;
 
 	msg.msg_control = cmsgbuf;
@@ -166,9 +166,9 @@ int lxc_abstract_unix_recv_fd(int fd, int *recvfd, void *data, size_t size)
 	struct msghdr msg = { 0 };
 	struct iovec iov;
 	struct cmsghdr *cmsg;
-	char cmsgbuf[CMSG_SPACE(sizeof(int))];
-	char buf[1];
 	int ret, *val;
+	char cmsgbuf[CMSG_SPACE(sizeof(int))] = {0};
+	char buf[1] = {0};
 
 	msg.msg_name = NULL;
 	msg.msg_namelen = 0;
@@ -210,8 +210,8 @@ int lxc_abstract_unix_send_credential(int fd, void *data, size_t size)
 		.uid = getuid(),
 		.gid = getgid(),
 	};
-	char cmsgbuf[CMSG_SPACE(sizeof(cred))];
-	char buf[1];
+	char cmsgbuf[CMSG_SPACE(sizeof(cred))] = {0};
+	char buf[1] = {0};
 
 	msg.msg_control = cmsgbuf;
 	msg.msg_controllen = sizeof(cmsgbuf);
@@ -239,9 +239,9 @@ int lxc_abstract_unix_rcv_credential(int fd, void *data, size_t size)
 	struct iovec iov;
 	struct cmsghdr *cmsg;
 	struct ucred cred;
-	char cmsgbuf[CMSG_SPACE(sizeof(cred))];
-	char buf[1];
 	int ret;
+	char cmsgbuf[CMSG_SPACE(sizeof(cred))] = {0};
+	char buf[1] = {0};
 
 	msg.msg_name = NULL;
 	msg.msg_namelen = 0;

From 3c082b0cf010b4c20addb675e13a9382f27712bd Mon Sep 17 00:00:00 2001
From: Christian Brauner <christian.brauner at ubuntu.com>
Date: Sat, 6 May 2017 23:35:57 +0200
Subject: [PATCH 2/5] commands: avoid NULL pointer dereference

lxc_cmd_get_lxcpath() and lxc_cmd_get_name() both pass a nil pointer to
fill_sock_name(). Make sure that they are not dereferenced.

Signed-off-by: Christian Brauner <christian.brauner at ubuntu.com>
---
 src/lxc/commands.c | 7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)

diff --git a/src/lxc/commands.c b/src/lxc/commands.c
index b17879b..66c8aae 100644
--- a/src/lxc/commands.c
+++ b/src/lxc/commands.c
@@ -74,14 +74,19 @@
 
 lxc_log_define(lxc_commands, lxc);
 
-static int fill_sock_name(char *path, int len, const char *name,
+static int fill_sock_name(char *path, int len, const char *lxcname,
 			  const char *lxcpath, const char *hashed_sock_name)
 {
+	const char *name;
 	char *tmppath;
 	size_t tmplen;
 	uint64_t hash;
 	int ret;
 
+	name = lxcname;
+	if (!name)
+		name = "";
+
 	if (hashed_sock_name != NULL) {
 		ret = snprintf(path, len, "lxc/%s/command", hashed_sock_name);
 		if (ret < 0 || ret >= len) {

From f992439250a3d8356b0ff3cdd94ad76605c87f0b Mon Sep 17 00:00:00 2001
From: Christian Brauner <christian.brauner at ubuntu.com>
Date: Sat, 6 May 2017 23:37:53 +0200
Subject: [PATCH 3/5] commands: non-functional changes

Signed-off-by: Christian Brauner <christian.brauner at ubuntu.com>
---
 src/lxc/commands.c | 9 ++++++---
 1 file changed, 6 insertions(+), 3 deletions(-)

diff --git a/src/lxc/commands.c b/src/lxc/commands.c
index 66c8aae..27c8c08 100644
--- a/src/lxc/commands.c
+++ b/src/lxc/commands.c
@@ -198,8 +198,11 @@ static int lxc_cmd_rsp_recv(int sock, struct lxc_cmd_rr *cmd)
 		rsp->data = rspdata;
 	}
 
-	if (rsp->datalen == 0)
+	if (rsp->datalen == 0) {
+		DEBUG("command %s response data length is 0",
+		      lxc_cmd_str(cmd->req.cmd));
 		return ret;
+	}
 	if (rsp->datalen > LXC_CMD_DATA_MAX) {
 		ERROR("Command %s response data %d too long.",
 		      lxc_cmd_str(cmd->req.cmd), rsp->datalen);
@@ -279,7 +282,7 @@ static int lxc_cmd(const char *name, struct lxc_cmd_rr *cmd, int *stopped,
 	int sock, ret = -1;
 	char path[sizeof(((struct sockaddr_un *)0)->sun_path)] = { 0 };
 	char *offset = &path[1];
-	int len;
+	size_t len;
 	int stay_connected = cmd->req.cmd == LXC_CMD_CONSOLE;
 
 	*stopped = 0;
@@ -987,7 +990,7 @@ int lxc_cmd_init(const char *name, struct lxc_handler *handler,
 	 * Although null termination isn't required by the API, we do it anyway
 	 * because we print the sockname out sometimes.
 	 */
-	len = sizeof(path)-2;
+	len = sizeof(path) - 2;
 	if (fill_sock_name(offset, len, name, lxcpath, NULL))
 		return -1;
 

From c9a9b6f97ea26eed34b72d08d648dede53708324 Mon Sep 17 00:00:00 2001
From: Christian Brauner <christian.brauner at ubuntu.com>
Date: Sat, 6 May 2017 23:38:22 +0200
Subject: [PATCH 4/5] lxccontainer: avoid NULL pointer dereference

In case the lxc command socket is hashed and the socket was created for a
different path than the one we're currently querying
lxc_cmd_get_{lxcpath,name}() can return NULL. The command socket path is hashed
when len(lxcpath) > sizeof(sun_path) - 2.

Signed-off-by: Christian Brauner <christian.brauner at ubuntu.com>
---
 src/lxc/lxccontainer.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/src/lxc/lxccontainer.c b/src/lxc/lxccontainer.c
index 3cee18c..ebbcfe8 100644
--- a/src/lxc/lxccontainer.c
+++ b/src/lxc/lxccontainer.c
@@ -4370,7 +4370,10 @@ int list_active_containers(const char *lxcpath, char ***nret,
 		*p2 = '\0';
 
 		if (is_hashed) {
-			if (strncmp(lxcpath, lxc_cmd_get_lxcpath(p), lxcpath_len) != 0)
+			char *recvpath = lxc_cmd_get_lxcpath(p);
+			if (!recvpath)
+				continue;
+			if (strncmp(lxcpath, recvpath, lxcpath_len) != 0)
 				continue;
 			p = lxc_cmd_get_name(p);
 		}

From 37b1ae3d461b34c061616baa56cd7e62981ccb2a Mon Sep 17 00:00:00 2001
From: Christian Brauner <christian.brauner at ubuntu.com>
Date: Sat, 6 May 2017 23:40:04 +0200
Subject: [PATCH 5/5] monitor: simplify abstract socket logic

Older version of liblxc only allowed for 105 bytes to be used for the abstract
unix domain socket name because the code for our abstract unix socket handling
performed invalid checks. Since we \0-terminate we could now have a maximum of
106 chars. But do not break backwards compatibility we keep the limit at 105.

Reported-by: 0x0916 w at laoqinren.net
Signed-off-by: Christian Brauner <christian.brauner at ubuntu.com>
---
 src/lxc/monitor.c | 43 +++++++++++++++++++++++++++++++------------
 1 file changed, 31 insertions(+), 12 deletions(-)

diff --git a/src/lxc/monitor.c b/src/lxc/monitor.c
index 410a0f4..f66932d 100644
--- a/src/lxc/monitor.c
+++ b/src/lxc/monitor.c
@@ -153,36 +153,52 @@ int lxc_monitor_close(int fd)
 	return close(fd);
 }
 
+/* Enforces \0-termination for the abstract unix socket. This is not required
+ * but allows us to print it out.
+ *
+ * Older version of liblxc only allowed for 105 bytes to be used for the
+ * abstract unix domain socket name because the code for our abstract unix
+ * socket handling performed invalid checks. Since we \0-terminate we could now
+ * have a maximum of 106 chars. But to not break backwards compatibility we keep
+ * the limit at 105.
+ */
 int lxc_monitor_sock_name(const char *lxcpath, struct sockaddr_un *addr) {
 	size_t len;
 	int ret;
-	char *sockname;
 	char *path;
 	uint64_t hash;
 
 	/* addr.sun_path is only 108 bytes, so we hash the full name and
 	 * then append as much of the name as we can fit.
 	 */
-	sockname = &addr->sun_path[1];
 	memset(addr, 0, sizeof(*addr));
 	addr->sun_family = AF_UNIX;
 
+	/* strlen("lxc/") + strlen("/monitor-sock") + 1 = 18 */
 	len = strlen(lxcpath) + 18;
 	path = alloca(len);
 	ret = snprintf(path, len, "lxc/%s/monitor-sock", lxcpath);
 	if (ret < 0 || (size_t)ret >= len) {
-		ERROR("Failed to create path for monitor.");
+		ERROR("failed to create name for monitor socket");
 		return -1;
 	}
 
+	/* Note: snprintf() will \0-terminate addr->sun_path on the 106th byte
+	 * and so the abstract socket name has 105 "meaningful" characters. This
+	 * is absolutely intentional. For further info read the comment for this
+	 * function above!
+	 */
 	len = sizeof(addr->sun_path) - 1;
 	hash = fnv_64a_buf(path, ret, FNV1A_64_INIT);
-	ret = snprintf(sockname, len, "lxc/%016" PRIx64 "/%s", hash, lxcpath);
-	if (ret < 0)
+	ret = snprintf(addr->sun_path, len, "@lxc/%016" PRIx64 "/%s", hash, lxcpath);
+	if (ret < 0) {
+		ERROR("failed to create hashed name for monitor socket");
 		return -1;
+	}
 
-	sockname[sizeof(addr->sun_path)-3] = '\0';
-	INFO("Using monitor socket name \"%s\".", sockname);
+	/* replace @ with \0 */
+	addr->sun_path[0] = '\0';
+	INFO("using monitor socket name \"%s\" (length of socket name %zu must be <= %zu)", &addr->sun_path[1], strlen(&addr->sun_path[1]), sizeof(addr->sun_path) - 3);
 
 	return 0;
 }
@@ -204,25 +220,28 @@ int lxc_monitor_open(const char *lxcpath)
 		return -1;
 	}
 
-	len = strlen(&addr.sun_path[1]) + 1;
+	len = strlen(&addr.sun_path[1]);
+	DEBUG("opening monitor socket %s with len %zu", &addr.sun_path[1], len);
 	if (len >= sizeof(addr.sun_path) - 1) {
 		ret = -1;
 		errno = ENAMETOOLONG;
+		ERROR("name of monitor socket too long (%zu bytes): %s", len, strerror(errno));
 		goto on_error;
 	}
 
 	for (retry = 0; retry < sizeof(backoff_ms) / sizeof(backoff_ms[0]); retry++) {
-		ret = connect(fd, (struct sockaddr *)&addr, offsetof(struct sockaddr_un, sun_path) + len);
-		if (ret == 0 || errno != ECONNREFUSED)
+		fd = lxc_abstract_unix_connect(addr.sun_path);
+		if (fd < 0 || errno != ECONNREFUSED)
 			break;
-		ERROR("Failed to connect to monitor socket. Retrying in %d ms.", backoff_ms[retry]);
+		ERROR("Failed to connect to monitor socket. Retrying in %d ms: %s", backoff_ms[retry], strerror(errno));
 		usleep(backoff_ms[retry] * 1000);
 	}
 
-	if (ret < 0) {
+	if (fd < 0) {
 		ERROR("Failed to connect to monitor socket: %s.", strerror(errno));
 		goto on_error;
 	}
+	ret = 0;
 
 	return fd;
 


More information about the lxc-devel mailing list