The following pull request was submitted through Github. It can be accessed and reviewed at: https://github.com/lxc/lxc/pull/1612
This e-mail was sent by the LXC bot, direct replies will not reach the author unless they happen to be subscribed to this list. === Description (from pull-request) === Signed-off-by: Christian Brauner <[email protected]>
From f07fa8df6e2670e04e5f3d9f3a32a5d32f9fc70e Mon Sep 17 00:00:00 2001 From: Christian Brauner <[email protected]> Date: Thu, 1 Jun 2017 05:40:59 +0200 Subject: [PATCH 1/3] start: log sending and receiving of tty fds This is a potentially security sensitive operation and I really want to keep an eye on *when exactly* this is send. So add more logging on the TRACE() level. Signed-off-by: Christian Brauner <[email protected]> --- src/lxc/conf.c | 19 ++++++++++++++----- src/lxc/start.c | 16 ++++++++++++---- 2 files changed, 26 insertions(+), 9 deletions(-) diff --git a/src/lxc/conf.c b/src/lxc/conf.c index fb82303c8..25c0aca25 100644 --- a/src/lxc/conf.c +++ b/src/lxc/conf.c @@ -4107,21 +4107,30 @@ static int send_fd(int sock, int fd) static int send_ttys_to_parent(struct lxc_handler *handler) { + int i, ret; struct lxc_conf *conf = handler->conf; const struct lxc_tty_info *tty_info = &conf->tty_info; - int i; int sock = handler->ttysock[0]; for (i = 0; i < tty_info->nbtty; i++) { struct lxc_pty_info *pty_info = &tty_info->pty_info[i]; - if (send_fd(sock, pty_info->slave) < 0) - goto bad; + ret = send_fd(sock, pty_info->slave); + if (ret >= 0) + send_fd(sock, pty_info->master); + TRACE("sending pty \"%s\" with master fd %d and slave fd %d to " + "parent", + pty_info->name, pty_info->master, pty_info->slave); close(pty_info->slave); pty_info->slave = -1; - if (send_fd(sock, pty_info->master) < 0) - goto bad; close(pty_info->master); pty_info->master = -1; + if (ret < 0) { + ERROR("failed to send pty \"%s\" with master fd %d and " + "slave fd %d to parent : %s", + pty_info->name, pty_info->master, pty_info->slave, + strerror(errno)); + goto bad; + } } close(handler->ttysock[0]); diff --git a/src/lxc/start.c b/src/lxc/start.c index f1b3f8e11..36f8b2318 100644 --- a/src/lxc/start.c +++ b/src/lxc/start.c @@ -1021,8 +1021,9 @@ static int recv_fd(int sock, int *fd) static int recv_ttys_from_child(struct lxc_handler *handler) { + int i, ret; + int sock = handler->ttysock[1]; struct lxc_conf *conf = handler->conf; - int i, sock = handler->ttysock[1]; struct lxc_tty_info *tty_info = &conf->tty_info; if (!conf->tty) @@ -1035,11 +1036,18 @@ static int recv_ttys_from_child(struct lxc_handler *handler) for (i = 0; i < conf->tty; i++) { struct lxc_pty_info *pty_info = &tty_info->pty_info[i]; pty_info->busy = 0; - if (recv_fd(sock, &pty_info->slave) < 0 || - recv_fd(sock, &pty_info->master) < 0) { - ERROR("Error receiving tty info from child process."); + ret = recv_fd(sock, &pty_info->slave); + if (ret >= 0) + recv_fd(sock, &pty_info->master); + if (ret < 0) { + ERROR("failed to receive pty with master fd %d and " + "slave fd %d from child: %s", + pty_info->master, pty_info->slave, + strerror(errno)); return -1; } + TRACE("received pty with master fd %d and slave fd %d from child", + pty_info->master, pty_info->slave); } tty_info->nbtty = conf->tty; From b0ee5983576a6ac8d860758a6b8b0f8452612c00 Mon Sep 17 00:00:00 2001 From: Christian Brauner <[email protected]> Date: Sat, 3 Jun 2017 19:14:45 +0200 Subject: [PATCH 2/3] conf: non-functional changes Signed-off-by: Christian Brauner <[email protected]> --- src/lxc/conf.c | 49 +++++++++++++++++++++++++------------------------ 1 file changed, 25 insertions(+), 24 deletions(-) diff --git a/src/lxc/conf.c b/src/lxc/conf.c index 25c0aca25..208c5ca0d 100644 --- a/src/lxc/conf.c +++ b/src/lxc/conf.c @@ -815,16 +815,16 @@ static int lxc_mount_auto_mounts(struct lxc_conf *conf, int flags, struct lxc_ha * :mixed, because then the container can't remount it read-write. */ if (cg_flags == LXC_AUTO_CGROUP_NOSPEC || cg_flags == LXC_AUTO_CGROUP_FULL_NOSPEC) { int has_sys_admin = 0; - if (!lxc_list_empty(&conf->keepcaps)) { + + if (!lxc_list_empty(&conf->keepcaps)) has_sys_admin = in_caplist(CAP_SYS_ADMIN, &conf->keepcaps); - } else { + else has_sys_admin = !in_caplist(CAP_SYS_ADMIN, &conf->caps); - } - if (cg_flags == LXC_AUTO_CGROUP_NOSPEC) { + + if (cg_flags == LXC_AUTO_CGROUP_NOSPEC) cg_flags = has_sys_admin ? LXC_AUTO_CGROUP_RW : LXC_AUTO_CGROUP_MIXED; - } else { + else cg_flags = has_sys_admin ? LXC_AUTO_CGROUP_FULL_RW : LXC_AUTO_CGROUP_FULL_MIXED; - } } if (!cgroup_mount(conf->rootfs.path ? conf->rootfs.mount : "", handler, cg_flags)) { @@ -2764,8 +2764,8 @@ struct lxc_conf *lxc_conf_init(void) static int instantiate_veth(struct lxc_handler *handler, struct lxc_netdev *netdev) { - char veth1buf[IFNAMSIZ], *veth1; - char veth2buf[IFNAMSIZ], *veth2; + char *veth1, *veth2; + char veth1buf[IFNAMSIZ], veth2buf[IFNAMSIZ]; int bridge_index, err; unsigned int mtu = 0; @@ -2797,8 +2797,8 @@ static int instantiate_veth(struct lxc_handler *handler, struct lxc_netdev *netd err = lxc_veth_create(veth1, veth2); if (err) { - ERROR("failed to create veth pair (%s and %s): %s", veth1, veth2, - strerror(-err)); + ERROR("failed to create veth pair \"%s\" and \"%s\": %s", veth1, + veth2, strerror(-err)); goto out_delete; } @@ -2807,30 +2807,30 @@ static int instantiate_veth(struct lxc_handler *handler, struct lxc_netdev *netd * of a container */ err = setup_private_host_hw_addr(veth1); if (err) { - ERROR("failed to change mac address of host interface '%s': %s", - veth1, strerror(-err)); + ERROR("failed to change mac address of host interface \"%s\": %s", + veth1, strerror(-err)); goto out_delete; } netdev->ifindex = if_nametoindex(veth2); if (!netdev->ifindex) { - ERROR("failed to retrieve the index for %s", veth2); + ERROR("failed to retrieve the index for \"%s\"", veth2); goto out_delete; } if (netdev->mtu) { if (lxc_safe_uint(netdev->mtu, &mtu) < 0) - WARN("Failed to parse mtu from."); + WARN("failed to parse mtu from"); else - INFO("Retrieved mtu %d", mtu); + INFO("retrieved mtu %d", mtu); } else if (netdev->link) { bridge_index = if_nametoindex(netdev->link); if (bridge_index) { mtu = netdev_get_mtu(bridge_index); - INFO("Retrieved mtu %d from %s", mtu, netdev->link); + INFO("retrieved mtu %d from %s", mtu, netdev->link); } else { mtu = netdev_get_mtu(netdev->ifindex); - INFO("Retrieved mtu %d from %s", mtu, veth2); + INFO("retrieved mtu %d from %s", mtu, veth2); } } @@ -2839,7 +2839,8 @@ static int instantiate_veth(struct lxc_handler *handler, struct lxc_netdev *netd if (!err) err = lxc_netdev_set_mtu(veth2, mtu); if (err) { - ERROR("failed to set mtu '%i' for veth pair (%s and %s): %s", + ERROR("failed to set mtu \"%d\" for veth pair \"%s\" " + "and \"%s\": %s", mtu, veth1, veth2, strerror(-err)); goto out_delete; } @@ -2848,16 +2849,16 @@ static int instantiate_veth(struct lxc_handler *handler, struct lxc_netdev *netd if (netdev->link) { err = lxc_bridge_attach(handler->lxcpath, handler->name, netdev->link, veth1); if (err) { - ERROR("failed to attach '%s' to the bridge '%s': %s", - veth1, netdev->link, strerror(-err)); + ERROR("failed to attach \"%s\" to bridge \"%s\": %s", + veth1, netdev->link, strerror(-err)); goto out_delete; } - INFO("Attached '%s': to the bridge '%s': ", veth1, netdev->link); + INFO("attached \"%s\" to bridge \"%s\"", veth1, netdev->link); } err = lxc_netdev_up(veth1); if (err) { - ERROR("failed to set %s up : %s", veth1, strerror(-err)); + ERROR("failed to set \"%s\" up: %s", veth1, strerror(-err)); goto out_delete; } @@ -2868,8 +2869,8 @@ static int instantiate_veth(struct lxc_handler *handler, struct lxc_netdev *netd goto out_delete; } - DEBUG("instantiated veth '%s/%s', index is '%d'", - veth1, veth2, netdev->ifindex); + DEBUG("instantiated veth \"%s/%s\", index is \"%d\"", veth1, veth2, + netdev->ifindex); return 0; From 1d90e06436151ae9ce4c958227570ae499137b3a Mon Sep 17 00:00:00 2001 From: Christian Brauner <[email protected]> Date: Sat, 3 Jun 2017 20:28:13 +0200 Subject: [PATCH 3/3] conf: avoid double-frees in userns_exec_1() Signed-off-by: Christian Brauner <[email protected]> --- src/lxc/conf.c | 42 +++++++++++++++++++++++++----------------- 1 file changed, 25 insertions(+), 17 deletions(-) diff --git a/src/lxc/conf.c b/src/lxc/conf.c index 208c5ca0d..f5357d51b 100644 --- a/src/lxc/conf.c +++ b/src/lxc/conf.c @@ -4837,17 +4837,16 @@ int userns_exec_1(struct lxc_conf *conf, int (*fn)(void *), void *data) goto on_error; } + host_uid_map = container_root_uid; + host_gid_map = container_root_gid; + /* Check whether the {g,u}id of the user has a mapping. */ euid = geteuid(); egid = getegid(); - if (euid == container_root_uid->hostid) - host_uid_map = container_root_uid; - else + if (euid != container_root_uid->hostid) host_uid_map = idmap_add(conf, euid, ID_TYPE_UID); - if (egid == container_root_gid->hostid) - host_gid_map = container_root_gid; - else + if (egid != container_root_gid->hostid) host_gid_map = idmap_add(conf, egid, ID_TYPE_GID); if (!host_uid_map) { @@ -4873,7 +4872,7 @@ int userns_exec_1(struct lxc_conf *conf, int (*fn)(void *), void *data) lxc_list_add_elem(tmplist, container_root_uid); lxc_list_add_tail(idmap, tmplist); - if (host_uid_map != container_root_uid) { + if (host_uid_map && (host_uid_map != container_root_uid)) { /* idmap will now keep track of that memory. */ container_root_uid = NULL; @@ -4883,9 +4882,11 @@ int userns_exec_1(struct lxc_conf *conf, int (*fn)(void *), void *data) goto on_error; lxc_list_add_elem(tmplist, host_uid_map); lxc_list_add_tail(idmap, tmplist); - /* idmap will now keep track of that memory. */ - host_uid_map = NULL; } + /* idmap will now keep track of that memory. */ + container_root_uid = NULL; + /* idmap will now keep track of that memory. */ + host_uid_map = NULL; tmplist = malloc(sizeof(*tmplist)); if (!tmplist) @@ -4893,7 +4894,7 @@ int userns_exec_1(struct lxc_conf *conf, int (*fn)(void *), void *data) lxc_list_add_elem(tmplist, container_root_gid); lxc_list_add_tail(idmap, tmplist); - if (host_gid_map != container_root_gid) { + if (host_gid_map && (host_gid_map != container_root_gid)) { /* idmap will now keep track of that memory. */ container_root_gid = NULL; @@ -4902,9 +4903,11 @@ int userns_exec_1(struct lxc_conf *conf, int (*fn)(void *), void *data) goto on_error; lxc_list_add_elem(tmplist, host_gid_map); lxc_list_add_tail(idmap, tmplist); - /* idmap will now keep track of that memory. */ - host_gid_map = NULL; } + /* idmap will now keep track of that memory. */ + container_root_gid = NULL; + /* idmap will now keep track of that memory. */ + host_gid_map = NULL; if (lxc_log_get_level() == LXC_LOG_PRIORITY_TRACE || conf->loglevel == LXC_LOG_PRIORITY_TRACE) { @@ -4937,11 +4940,16 @@ int userns_exec_1(struct lxc_conf *conf, int (*fn)(void *), void *data) ret = wait_for_pid(pid); on_error: - lxc_free_idmap(idmap); - free(container_root_uid); - free(container_root_gid); - free(host_uid_map); - free(host_gid_map); + if (idmap) + lxc_free_idmap(idmap); + if (container_root_uid) + free(container_root_uid); + if (container_root_gid) + free(container_root_gid); + if (host_uid_map && (host_uid_map != container_root_uid)) + free(host_uid_map); + if (host_gid_map && (host_gid_map != container_root_gid)) + free(host_gid_map); if (p[0] != -1) close(p[0]);
_______________________________________________ lxc-devel mailing list [email protected] http://lists.linuxcontainers.org/listinfo/lxc-devel
