[lxc-devel] [lxc/master] idmapping bugfixes
brauner on Github
lxc-bot at linuxcontainers.org
Sat Jun 3 20:06:46 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/20170603/12de51f4/attachment.bin>
-------------- next part --------------
From f07fa8df6e2670e04e5f3d9f3a32a5d32f9fc70e Mon Sep 17 00:00:00 2001
From: Christian Brauner <christian.brauner at ubuntu.com>
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 <christian.brauner at ubuntu.com>
---
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 <christian.brauner at ubuntu.com>
Date: Sat, 3 Jun 2017 19:14:45 +0200
Subject: [PATCH 2/3] conf: non-functional changes
Signed-off-by: Christian Brauner <christian.brauner at ubuntu.com>
---
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 <christian.brauner at ubuntu.com>
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 <christian.brauner at ubuntu.com>
---
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]);
More information about the lxc-devel
mailing list