From caf3beb02dd931caef96975d21cb985884dfabb9 Mon Sep 17 00:00:00 2001 From: Christian Brauner 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 --- src/lxc/af_unix.c | 30 ++++++++++++++++-------------- src/lxc/af_unix.h | 2 ++ 2 files changed, 18 insertions(+), 14 deletions(-) diff --git a/src/lxc/af_unix.c b/src/lxc/af_unix.c index 46d8e50fc..ac839943e 100644 --- a/src/lxc/af_unix.c +++ b/src/lxc/af_unix.c @@ -55,8 +55,9 @@ int lxc_abstract_unix_open(const char *path, int type, int flags) addr.sun_family = AF_UNIX; - len = strlen(&path[1]) + 1; - if (len >= sizeof(addr.sun_path) - 1) { + len = strlen(&path[1]); + /* do not enforce \0-termination */ + if (len >= sizeof(addr.sun_path)) { close(fd); errno = ENAMETOOLONG; return -1; @@ -64,7 +65,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,8 +110,9 @@ int lxc_abstract_unix_connect(const char *path) addr.sun_family = AF_UNIX; - len = strlen(&path[1]) + 1; - if (len >= sizeof(addr.sun_path) - 1) { + len = strlen(&path[1]); + /* do not enforce \0-termination */ + if (len >= sizeof(addr.sun_path)) { close(fd); errno = ENAMETOOLONG; return -1; @@ -118,7 +120,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 +138,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 +168,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 +212,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 +241,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; diff --git a/src/lxc/af_unix.h b/src/lxc/af_unix.h index 3f5d01fe1..d25a2118a 100644 --- a/src/lxc/af_unix.h +++ b/src/lxc/af_unix.h @@ -24,8 +24,10 @@ #ifndef __LXC_AF_UNIX_H #define __LXC_AF_UNIX_H +/* does not enforce \0-termination */ extern int lxc_abstract_unix_open(const char *path, int type, int flags); extern int lxc_abstract_unix_close(int fd); +/* does not enforce \0-termination */ extern int lxc_abstract_unix_connect(const char *path); extern int lxc_abstract_unix_send_fd(int fd, int sendfd, void *data, size_t size); extern int lxc_abstract_unix_recv_fd(int fd, int *recvfd, void *data, size_t size); From c54a4aeeb58c078ed64a12d0131a6c9ee3d793f6 Mon Sep 17 00:00:00 2001 From: Christian Brauner 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 --- 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 b17879b54..66c8aaeae 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 860e7c43114fdd320bec7039c9d567b7285638d7 Mon Sep 17 00:00:00 2001 From: Christian Brauner Date: Sat, 6 May 2017 23:37:53 +0200 Subject: [PATCH 3/5] commands: non-functional changes Signed-off-by: Christian Brauner --- 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 66c8aaeae..27c8c084f 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 899a9f554500ad56e120302f94a0134f2e2c2354 Mon Sep 17 00:00:00 2001 From: Christian Brauner 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 --- 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 3cee18ca7..ebbcfe86c 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 fcaef9c7dd2962ac31405b508b9962a290547a03 Mon Sep 17 00:00:00 2001 From: Christian Brauner 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@laoqinren.net Signed-off-by: Christian Brauner --- src/lxc/monitor.c | 51 +++++++++++++++++++++++++++++++++-------------- 1 file changed, 36 insertions(+), 15 deletions(-) diff --git a/src/lxc/monitor.c b/src/lxc/monitor.c index 410a0f45c..1758402a9 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; } @@ -193,7 +209,8 @@ int lxc_monitor_open(const char *lxcpath) int fd; size_t retry; size_t len; - int ret = 0, backoff_ms[] = {10, 50, 100}; + int ret = -1; + int backoff_ms[] = {10, 50, 100}; if (lxc_monitor_sock_name(lxcpath, &addr) < 0) return -1; @@ -201,28 +218,32 @@ int lxc_monitor_open(const char *lxcpath) fd = socket(PF_UNIX, SOCK_STREAM, 0); if (fd < 0) { ERROR("Failed to create socket: %s.", strerror(errno)); - return -1; + return -errno; } - 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; + ret = -errno; + 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) { + ret = -errno; ERROR("Failed to connect to monitor socket: %s.", strerror(errno)); goto on_error; } + ret = 0; return fd;