[lxc-devel] [lxc/master] conf: terminal and /dev hardening
brauner on Github
lxc-bot at linuxcontainers.org
Mon Aug 10 10:33:27 UTC 2020
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/20200810/dfa94d77/attachment.bin>
-------------- next part --------------
From 953db219dab62555dd9e515561a918db8f47ee4a Mon Sep 17 00:00:00 2001
From: Christian Brauner <christian.brauner at ubuntu.com>
Date: Mon, 10 Aug 2020 11:01:42 +0200
Subject: [PATCH 1/2] conf: move /dev setup to be file descriptor based
Signed-off-by: Christian Brauner <christian.brauner at ubuntu.com>
---
src/lxc/conf.c | 39 ++++++++++++++++++++++++++++-----------
src/lxc/conf.h | 18 ++++++++++--------
src/lxc/file_utils.c | 7 +++++++
src/lxc/file_utils.h | 1 +
4 files changed, 46 insertions(+), 19 deletions(-)
diff --git a/src/lxc/conf.c b/src/lxc/conf.c
index c6a240d6e3..0a1bb6d75a 100644
--- a/src/lxc/conf.c
+++ b/src/lxc/conf.c
@@ -1610,15 +1610,15 @@ static int lxc_setup_dev_console(const struct lxc_rootfs *rootfs,
if (!wants_console(console))
return 0;
- ret = snprintf(path, sizeof(path), "%s/dev/console", rootfs_path);
- if (ret < 0 || (size_t)ret >= sizeof(path))
- return -1;
-
/*
* When we are asked to setup a console we remove any previous
* /dev/console bind-mounts.
*/
- if (file_exists(path)) {
+ if (exists_file_at(rootfs->dev_mntpt_fd, "console")) {
+ ret = snprintf(path, sizeof(path), "%s/dev/console", rootfs_path);
+ if (ret < 0 || (size_t)ret >= sizeof(path))
+ return -1;
+
ret = lxc_unstack_mountpoint(path, false);
if (ret < 0)
return log_error_errno(-ret, errno, "Failed to unmount \"%s\"", path);
@@ -1630,7 +1630,7 @@ static int lxc_setup_dev_console(const struct lxc_rootfs *rootfs,
* For unprivileged containers autodev or automounts will already have
* taken care of creating /dev/console.
*/
- ret = mknod(path, S_IFREG | 0000, 0);
+ ret = mknodat(rootfs->dev_mntpt_fd, "console", S_IFREG | 0000, 0);
if (ret < 0 && errno != EEXIST)
return log_error_errno(-errno, errno, "Failed to create console");
@@ -1639,7 +1639,7 @@ static int lxc_setup_dev_console(const struct lxc_rootfs *rootfs,
return log_error_errno(-errno, errno, "Failed to set mode \"0%o\" to \"%s\"", S_IXUSR | S_IXGRP, console->name);
if (pty_mnt_fd >= 0) {
- ret = move_mount(pty_mnt_fd, "", -EBADF, path, MOVE_MOUNT_F_EMPTY_PATH);
+ ret = move_mount(pty_mnt_fd, "", rootfs->dev_mntpt_fd, "console", MOVE_MOUNT_F_EMPTY_PATH);
if (!ret) {
DEBUG("Moved mount \"%s\" onto \"%s\"", console->name, path);
goto finish;
@@ -1651,9 +1651,18 @@ static int lxc_setup_dev_console(const struct lxc_rootfs *rootfs,
pty_mnt_fd, console->name, path);
}
- ret = safe_mount(console->name, path, "none", MS_BIND, 0, rootfs_path);
- if (ret < 0)
- return log_error_errno(-1, errno, "Failed to mount %d(%s) on \"%s\"", pty_mnt_fd, console->name, path);
+ ret = safe_mount_beneath_at(rootfs->dev_mntpt_fd, console->name, "console", NULL, MS_BIND, NULL);
+ if (ret < 0) {
+ if (errno == ENOSYS) {
+ ret = snprintf(path, sizeof(path), "%s/dev/console", rootfs_path);
+ if (ret < 0 || (size_t)ret >= sizeof(path))
+ return -1;
+
+ ret = safe_mount(console->name, path, "none", MS_BIND, NULL, rootfs_path);
+ if (ret < 0)
+ return log_error_errno(-1, errno, "Failed to mount %d(%s) on \"%s\"", pty_mnt_fd, console->name, path);
+ }
+ }
finish:
DEBUG("Mounted pty device %d(%s) onto \"%s\"", pty_mnt_fd, console->name, path);
@@ -2581,6 +2590,7 @@ struct lxc_conf *lxc_conf_init(void)
}
new->rootfs.managed = true;
new->rootfs.mntpt_fd = -EBADF;
+ new->rootfs.dev_mntpt_fd = -EBADF;
new->logfd = -1;
lxc_list_init(&new->cgroup);
lxc_list_init(&new->cgroup2);
@@ -3259,6 +3269,11 @@ int lxc_setup(struct lxc_handler *handler)
return log_error(-1, "Failed to mount \"/dev\"");
}
+ lxc_conf->rootfs.dev_mntpt_fd = openat(lxc_conf->rootfs.mntpt_fd, "dev",
+ O_RDONLY | O_CLOEXEC | O_DIRECTORY | O_NOFOLLOW);
+ if (lxc_conf->rootfs.dev_mntpt_fd < 0 && errno != ENOENT)
+ return log_error_errno(-errno, errno, "Failed to open \"/dev\"");
+
/* Do automatic mounts (mainly /proc and /sys), but exclude those that
* need to wait until other stuff has finished.
*/
@@ -3378,7 +3393,8 @@ int lxc_setup(struct lxc_handler *handler)
return log_error(-1, "Failed to drop capabilities");
}
- close_prot_errno_disarm(lxc_conf->rootfs.mntpt_fd);
+ close_prot_errno_disarm(lxc_conf->rootfs.mntpt_fd)
+ close_prot_errno_disarm(lxc_conf->rootfs.dev_mntpt_fd)
NOTICE("The container \"%s\" is set up", name);
return 0;
@@ -3743,6 +3759,7 @@ void lxc_conf_free(struct lxc_conf *conf)
free(conf->rootfs.path);
free(conf->rootfs.data);
close_prot_errno_disarm(conf->rootfs.mntpt_fd);
+ close_prot_errno_disarm(conf->rootfs.dev_mntpt_fd);
free(conf->logfile);
if (conf->logfd != -1)
close(conf->logfd);
diff --git a/src/lxc/conf.h b/src/lxc/conf.h
index bfdf3be314..cd54b3fe0f 100644
--- a/src/lxc/conf.h
+++ b/src/lxc/conf.h
@@ -139,17 +139,19 @@ struct lxc_tty_info {
/* Defines a structure to store the rootfs location, the
* optionals pivot_root, rootfs mount paths
- * @path : the rootfs source (directory or device)
- * @mount : where it is mounted
- * @bev_type : optional backing store type
- * @options : mount options
- * @mountflags : the portion of @options that are flags
- * @data : the portion of @options that are not flags
- * @managed : whether it is managed by LXC
- * @mntpt_fd : fd for @mount
+ * @path : the rootfs source (directory or device)
+ * @mount : where it is mounted
+ * @bev_type : optional backing store type
+ * @options : mount options
+ * @mountflags : the portion of @options that are flags
+ * @data : the portion of @options that are not flags
+ * @managed : whether it is managed by LXC
+ * @mntpt_fd : fd for @mount
+ * @dev_mntpt_fd : fd for /dev of the container
*/
struct lxc_rootfs {
int mntpt_fd;
+ int dev_mntpt_fd;
char *path;
char *mount;
char *bdev_type;
diff --git a/src/lxc/file_utils.c b/src/lxc/file_utils.c
index 46912642f7..75bbc2eb25 100644
--- a/src/lxc/file_utils.c
+++ b/src/lxc/file_utils.c
@@ -551,3 +551,10 @@ bool exists_dir_at(int dir_fd, const char *path)
return S_ISDIR(sb.st_mode);
}
+
+bool exists_file_at(int dir_fd, const char *path)
+{
+ struct stat sb;
+
+ return fstatat(dir_fd, path, &sb, 0) == 0;
+}
diff --git a/src/lxc/file_utils.h b/src/lxc/file_utils.h
index 48571185d2..2455feb7fc 100644
--- a/src/lxc/file_utils.h
+++ b/src/lxc/file_utils.h
@@ -74,5 +74,6 @@ __hidden extern FILE *fdopen_cached(int fd, const char *mode, void **caller_free
__hidden extern FILE *fopen_cached(const char *path, const char *mode, void **caller_freed_buffer);
__hidden extern int timens_offset_write(clockid_t clk_id, int64_t s_offset, int64_t ns_offset);
__hidden extern bool exists_dir_at(int dir_fd, const char *path);
+__hidden extern bool exists_file_at(int dir_fd, const char *path);
#endif /* __LXC_FILE_UTILS_H */
From 4f7c81d0868b4a5beb0084db0166c53e84fb30ce Mon Sep 17 00:00:00 2001
From: Christian Brauner <christian.brauner at ubuntu.com>
Date: Mon, 10 Aug 2020 11:13:53 +0200
Subject: [PATCH 2/2] terminal: harden terminal allocation
Signed-off-by: Christian Brauner <christian.brauner at ubuntu.com>
---
src/lxc/attach.c | 16 ++---------
src/lxc/file_utils.c | 19 +++++++++++++
src/lxc/file_utils.h | 1 +
src/lxc/terminal.c | 66 +++++++++++++++++++++++++++-----------------
src/lxc/terminal.h | 1 +
5 files changed, 64 insertions(+), 39 deletions(-)
diff --git a/src/lxc/attach.c b/src/lxc/attach.c
index 654a7d8fd0..336f54e787 100644
--- a/src/lxc/attach.c
+++ b/src/lxc/attach.c
@@ -876,7 +876,7 @@ static int attach_child_main(struct attach_clone_payload *payload)
/* Make sure that the processes STDIO is correctly owned by the user that we are switching to */
ret = fix_stdio_permissions(new_uid);
if (ret)
- WARN("Failed to ajust stdio permissions");
+ WARN("Failed to adjust stdio permissions");
if (!lxc_switch_uid_gid(new_uid, new_gid))
goto on_error;
@@ -896,23 +896,11 @@ static int lxc_attach_terminal(const char *name, const char *lxcpath, struct lxc
lxc_terminal_init(terminal);
- ret = lxc_terminal_create(name, lxcpath, terminal);
+ ret = lxc_terminal_create(name, lxcpath, conf, terminal);
if (ret < 0)
return log_error(-1, "Failed to create terminal");
- /* Shift ttys to container. */
- ret = lxc_terminal_map_ids(conf, terminal);
- if (ret < 0) {
- ERROR("Failed to chown terminal");
- goto on_error;
- }
-
return 0;
-
-on_error:
- lxc_terminal_delete(terminal);
- lxc_terminal_conf_free(terminal);
- return -1;
}
static int lxc_attach_terminal_mainloop_init(struct lxc_terminal *terminal,
diff --git a/src/lxc/file_utils.c b/src/lxc/file_utils.c
index 75bbc2eb25..4a8c7a8d99 100644
--- a/src/lxc/file_utils.c
+++ b/src/lxc/file_utils.c
@@ -18,6 +18,7 @@
#include "macro.h"
#include "memory_utils.h"
#include "string_utils.h"
+#include "syscall_wrappers.h"
#include "utils.h"
int lxc_open_dirfd(const char *dir)
@@ -558,3 +559,21 @@ bool exists_file_at(int dir_fd, const char *path)
return fstatat(dir_fd, path, &sb, 0) == 0;
}
+
+int open_beneath(int dir_fd, const char *path, unsigned int flags)
+{
+ __do_close int fd = -EBADF;
+ struct lxc_open_how how = {
+ .flags = flags,
+ .resolve = RESOLVE_NO_XDEV | RESOLVE_NO_SYMLINKS | RESOLVE_NO_MAGICLINKS | RESOLVE_BENEATH,
+ };
+
+ fd = openat2(dir_fd, path, &how, sizeof(how));
+ if (fd >= 0)
+ return move_fd(fd);
+
+ if (errno != ENOSYS)
+ return -errno;
+
+ return openat(dir_fd, path, O_NOFOLLOW | flags);
+}
diff --git a/src/lxc/file_utils.h b/src/lxc/file_utils.h
index 2455feb7fc..df3a00d4dc 100644
--- a/src/lxc/file_utils.h
+++ b/src/lxc/file_utils.h
@@ -75,5 +75,6 @@ __hidden extern FILE *fopen_cached(const char *path, const char *mode, void **ca
__hidden extern int timens_offset_write(clockid_t clk_id, int64_t s_offset, int64_t ns_offset);
__hidden extern bool exists_dir_at(int dir_fd, const char *path);
__hidden extern bool exists_file_at(int dir_fd, const char *path);
+__hidden extern int open_beneath(int dir_fd, const char *path, unsigned int flags);
#endif /* __LXC_FILE_UTILS_H */
diff --git a/src/lxc/terminal.c b/src/lxc/terminal.c
index aeb2432795..e268d604d3 100644
--- a/src/lxc/terminal.c
+++ b/src/lxc/terminal.c
@@ -828,7 +828,7 @@ int lxc_terminal_create_log_file(struct lxc_terminal *terminal)
return 0;
}
-static int lxc_terminal_create_foreign(struct lxc_terminal *terminal)
+static int lxc_terminal_create_foreign(struct lxc_conf *conf, struct lxc_terminal *terminal)
{
int ret;
@@ -838,6 +838,12 @@ static int lxc_terminal_create_foreign(struct lxc_terminal *terminal)
return -1;
}
+ ret = lxc_terminal_map_ids(conf, terminal);
+ if (ret < 0) {
+ SYSERROR("Failed to change ownership of terminal multiplexer device");
+ goto err;
+ }
+
ret = ttyname_r(terminal->pty, terminal->name, sizeof(terminal->name));
if (ret < 0) {
SYSERROR("Failed to retrieve name of terminal pty");
@@ -869,38 +875,47 @@ static int lxc_terminal_create_foreign(struct lxc_terminal *terminal)
return -ENODEV;
}
-static int lxc_terminal_create_native(const char *name, const char *lxcpath,
+static int lxc_terminal_create_native(const char *name, const char *lxcpath, struct lxc_conf *conf,
struct lxc_terminal *terminal)
{
- __do_close int devpts_fd = -EBADF, ptx_fd = -EBADF, pty_fd = -EBADF;
+ __do_close int devpts_fd = -EBADF;
int ret;
devpts_fd = lxc_cmd_get_devpts_fd(name, lxcpath);
if (devpts_fd < 0)
return log_error_errno(-1, errno, "Failed to receive devpts fd");
- ptx_fd = openat(devpts_fd, "ptmx", O_RDWR | O_NOCTTY | O_CLOEXEC);
- if (ptx_fd < 0)
+ terminal->ptx = open_beneath(devpts_fd, "ptmx", O_RDWR | O_NOCTTY | O_CLOEXEC);
+ if (terminal->ptx < 0)
return log_error_errno(-1, errno, "Failed to open terminal multiplexer device");
- ret = grantpt(ptx_fd);
- if (ret < 0)
- return log_error_errno(-1, errno, "Failed to grant access to multiplexer device");
+ if (!lxc_list_empty(&conf->id_map))
+ ret = userns_exec_mapped_root(NULL, terminal->ptx, conf);
+ else
+ ret = grantpt(terminal->ptx);
+ if (ret < 0) {
+ SYSERROR("Failed to change ownership of multiplexer device");
+ goto err;
+ }
- ret = unlockpt(ptx_fd);
- if (ret < 0)
- return log_error_errno(-1, errno, "Failed to unlock multiplexer device device");
+ ret = unlockpt(terminal->ptx);
+ if (ret < 0) {
+ SYSERROR("Failed to unlock multiplexer device device");
+ goto err;
+ }
- pty_fd = ioctl(ptx_fd, TIOCGPTPEER, O_RDWR | O_NOCTTY | O_CLOEXEC);
- if (pty_fd < 0)
- return log_error_errno(-1, errno, "Failed to allocate new pty device");
+ terminal->pty = ioctl(terminal->ptx, TIOCGPTPEER, O_RDWR | O_NOCTTY | O_CLOEXEC);
+ if (terminal->pty < 0) {
+ SYSERROR("Failed to allocate new pty device");
+ goto err;
+ }
ret = ttyname_r(terminal->pty, terminal->name, sizeof(terminal->name));
- if (ret < 0)
- return log_error_errno(-1, errno, "Failed to retrieve name of terminal pty");
+ if (ret < 0) {
+ SYSERROR("Failed to retrieve name of terminal pty");
+ goto err;
+ }
- terminal->ptx = move_fd(ptx_fd);
- terminal->pty = move_fd(pty_fd);
ret = lxc_terminal_peer_default(terminal);
if (ret < 0) {
ERROR("Failed to allocate proxy terminal");
@@ -914,12 +929,13 @@ static int lxc_terminal_create_native(const char *name, const char *lxcpath,
return -ENODEV;
}
-int lxc_terminal_create(const char *name, const char *lxcpath, struct lxc_terminal *terminal)
+int lxc_terminal_create(const char *name, const char *lxcpath, struct lxc_conf *conf,
+ struct lxc_terminal *terminal)
{
- if (!lxc_terminal_create_native(name, lxcpath, terminal))
+ if (!lxc_terminal_create_native(name, lxcpath, conf, terminal))
return 0;
- return lxc_terminal_create_foreign(terminal);
+ return lxc_terminal_create_foreign(conf, terminal);
}
int lxc_terminal_setup(struct lxc_conf *conf)
@@ -932,7 +948,7 @@ int lxc_terminal_setup(struct lxc_conf *conf)
return 0;
}
- ret = lxc_terminal_create_foreign(terminal);
+ ret = lxc_terminal_create_foreign(conf, terminal);
if (ret < 0)
return -1;
@@ -1217,16 +1233,16 @@ int lxc_terminal_map_ids(struct lxc_conf *c, struct lxc_terminal *terminal)
if (lxc_list_empty(&c->id_map))
return 0;
- if (strcmp(terminal->name, "") == 0)
+ if (is_empty_string(terminal->name) && terminal->pty < 0)
return 0;
ret = userns_exec_mapped_root(terminal->name, terminal->pty, c);
if (ret < 0) {
return log_error(-1, "Failed to chown terminal %d(%s)",
- terminal->pty, terminal->name);
+ terminal->pty, terminal->name ? terminal->name : "(null)");
}
- TRACE("Chowned terminal %d(%s)", terminal->pty, terminal->name);
+ TRACE("Chowned terminal %d(%s)", terminal->pty, terminal->name ? terminal->name : "(null)");
return 0;
}
diff --git a/src/lxc/terminal.h b/src/lxc/terminal.h
index 838993d37f..4fbab7c3e9 100644
--- a/src/lxc/terminal.h
+++ b/src/lxc/terminal.h
@@ -111,6 +111,7 @@ __hidden extern int lxc_terminal_allocate(struct lxc_conf *conf, int sockfd, in
* (Handlers for SIGWINCH and I/O are not registered in a mainloop.)
*/
__hidden extern int lxc_terminal_create(const char *name, const char *lxcpath,
+ struct lxc_conf *conf,
struct lxc_terminal *console);
/**
More information about the lxc-devel
mailing list