Merge pull request #17233 from poettering/nspawn-reopen-fix

nspawn: reopen stdin/stdout + chmod fixes for stdin/stdout

src/nspawn/nspawn-setuid.c

@@ -63,16 +63,19 @@ int change_uid_gid_raw(
                 uid_t uid,
                 gid_t gid,
                 const gid_t *supplementary_gids,
-                size_t n_supplementary_gids) {
+                size_t n_supplementary_gids,
+                bool chown_stdio) {
 
         if (!uid_is_valid(uid))
                 uid = 0;
         if (!gid_is_valid(gid))
                 gid = 0;
 
-        (void) fchown(STDIN_FILENO, uid, gid);
-        (void) fchown(STDOUT_FILENO, uid, gid);
-        (void) fchown(STDERR_FILENO, uid, gid);
+        if (chown_stdio) {
+                (void) fchown(STDIN_FILENO, uid, gid);
+                (void) fchown(STDOUT_FILENO, uid, gid);
+                (void) fchown(STDERR_FILENO, uid, gid);
+        }
 
         if (setgroups(n_supplementary_gids, supplementary_gids) < 0)
                 return log_error_errno(errno, "Failed to set auxiliary groups: %m");
@@ -86,7 +89,7 @@ int change_uid_gid_raw(
         return 0;
 }
 
-int change_uid_gid(const char *user, char **_home) {
+int change_uid_gid(const char *user, bool chown_stdio, char **ret_home) {
         char *x, *u, *g, *h;
         _cleanup_free_ gid_t *gids = NULL;
         _cleanup_free_ char *home = NULL, *line = NULL;
@@ -99,7 +102,7 @@ int change_uid_gid(const char *user, char **_home) {
         pid_t pid;
         int r;
 
-        assert(_home);
+        assert(ret_home);
 
         if (!user || STR_IN_SET(user, "root", "0")) {
                 /* Reset everything fully to 0, just in case */
@@ -108,7 +111,7 @@ int change_uid_gid(const char *user, char **_home) {
                 if (r < 0)
                         return log_error_errno(r, "Failed to become root: %m");
 
-                *_home = NULL;
+                *ret_home = NULL;
                 return 0;
         }
 
@@ -232,12 +235,12 @@ int change_uid_gid(const char *user, char **_home) {
         if (r < 0 && !IN_SET(r, -EEXIST, -ENOTDIR))
                 return log_error_errno(r, "Failed to make home directory: %m");
 
-        r = change_uid_gid_raw(uid, gid, gids, n_gids);
+        r = change_uid_gid_raw(uid, gid, gids, n_gids, chown_stdio);
         if (r < 0)
                 return r;
 
-        if (_home)
-                *_home = TAKE_PTR(home);
+        if (ret_home)
+                *ret_home = TAKE_PTR(home);
 
         return 0;
 }

src/nspawn/nspawn-setuid.h

@@ -1,5 +1,5 @@
 /* SPDX-License-Identifier: LGPL-2.1+ */
 #pragma once
 
-int change_uid_gid_raw(uid_t uid, gid_t gid, const gid_t *supplementary_gids, size_t n_supplementary_gids);
-int change_uid_gid(const char *user, char **ret_home);
+int change_uid_gid_raw(uid_t uid, gid_t gid, const gid_t *supplementary_gids, size_t n_supplementary_gids, bool chown_stdio);
+int change_uid_gid(const char *user, bool chown_stdio, char **ret_home);

src/nspawn/nspawn.c

@@ -3321,9 +3321,9 @@ static int inner_child(
                 return log_error_errno(errno, "Failed to set PR_SET_KEEPCAPS: %m");
 
         if (uid_is_valid(arg_uid) || gid_is_valid(arg_gid))
-                r = change_uid_gid_raw(arg_uid, arg_gid, arg_supplementary_gids, arg_n_supplementary_gids);
+                r = change_uid_gid_raw(arg_uid, arg_gid, arg_supplementary_gids, arg_n_supplementary_gids, arg_console_mode != CONSOLE_PIPE);
         else
-                r = change_uid_gid(arg_user, &home);
+                r = change_uid_gid(arg_user, arg_console_mode != CONSOLE_PIPE, &home);
         if (r < 0)
                 return r;
 

src/shared/ptyfwd.c

@@ -1,6 +1,7 @@
 /* SPDX-License-Identifier: LGPL-2.1+ */
 
 #include <errno.h>
+#include <fcntl.h>
 #include <limits.h>
 #include <signal.h>
 #include <stddef.h>
@@ -27,6 +28,8 @@
 struct PTYForward {
         sd_event *event;
 
+        int input_fd;
+        int output_fd;
         int master;
 
         PTYForwardFlags flags;
@@ -40,6 +43,9 @@ struct PTYForward {
         struct termios saved_stdin_attr;
         struct termios saved_stdout_attr;
 
+        bool close_input_fd:1;
+        bool close_output_fd:1;
+
         bool saved_stdin:1;
         bool saved_stdout:1;
 
@@ -73,25 +79,36 @@ struct PTYForward {
 
 static void pty_forward_disconnect(PTYForward *f) {
 
-        if (f) {
-                f->stdin_event_source = sd_event_source_unref(f->stdin_event_source);
-                f->stdout_event_source = sd_event_source_unref(f->stdout_event_source);
+        if (!f)
+                return;
 
-                f->master_event_source = sd_event_source_unref(f->master_event_source);
-                f->sigwinch_event_source = sd_event_source_unref(f->sigwinch_event_source);
-                f->event = sd_event_unref(f->event);
+        f->stdin_event_source = sd_event_source_unref(f->stdin_event_source);
+        f->stdout_event_source = sd_event_source_unref(f->stdout_event_source);
 
+        f->master_event_source = sd_event_source_unref(f->master_event_source);
+        f->sigwinch_event_source = sd_event_source_unref(f->sigwinch_event_source);
+        f->event = sd_event_unref(f->event);
+
+        if (f->output_fd >= 0) {
                 if (f->saved_stdout)
-                        tcsetattr(STDOUT_FILENO, TCSANOW, &f->saved_stdout_attr);
-                if (f->saved_stdin)
-                        tcsetattr(STDIN_FILENO, TCSANOW, &f->saved_stdin_attr);
+                        (void) tcsetattr(f->output_fd, TCSANOW, &f->saved_stdout_attr);
 
-                f->saved_stdout = f->saved_stdin = false;
+                /* STDIN/STDOUT should not be non-blocking normally, so let's reset it */
+                (void) fd_nonblock(f->output_fd, false);
+                if (f->close_output_fd)
+                        f->output_fd = safe_close(f->output_fd);
         }
 
-        /* STDIN/STDOUT should not be nonblocking normally, so let's unconditionally reset it */
-        (void) fd_nonblock(STDIN_FILENO, false);
-        (void) fd_nonblock(STDOUT_FILENO, false);
+        if (f->input_fd >= 0) {
+                if (f->saved_stdin)
+                        (void) tcsetattr(f->input_fd, TCSANOW, &f->saved_stdin_attr);
+
+                (void) fd_nonblock(f->input_fd, false);
+                if (f->close_input_fd)
+                        f->input_fd = safe_close(f->input_fd);
+        }
+
+        f->saved_stdout = f->saved_stdin = false;
 }
 
 static int pty_forward_done(PTYForward *f, int rcode) {
@@ -191,7 +208,7 @@ static int shovel(PTYForward *f) {
 
                 if (f->stdin_readable && f->in_buffer_full < LINE_MAX) {
 
-                        k = read(STDIN_FILENO, f->in_buffer + f->in_buffer_full, LINE_MAX - f->in_buffer_full);
+                        k = read(f->input_fd, f->in_buffer + f->in_buffer_full, LINE_MAX - f->in_buffer_full);
                         if (k < 0) {
 
                                 if (errno == EAGAIN)
@@ -275,7 +292,7 @@ static int shovel(PTYForward *f) {
 
                 if (f->stdout_writable && f->out_buffer_full > 0) {
 
-                        k = write(STDOUT_FILENO, f->out_buffer, f->out_buffer_full);
+                        k = write(f->output_fd, f->out_buffer, f->out_buffer_full);
                         if (k < 0) {
 
                                 if (errno == EAGAIN)
@@ -345,7 +362,7 @@ static int on_stdin_event(sd_event_source *e, int fd, uint32_t revents, void *us
         assert(e);
         assert(e == f->stdin_event_source);
         assert(fd >= 0);
-        assert(fd == STDIN_FILENO);
+        assert(fd == f->input_fd);
 
         if (revents & (EPOLLIN|EPOLLHUP))
                 f->stdin_readable = true;
@@ -360,7 +377,7 @@ static int on_stdout_event(sd_event_source *e, int fd, uint32_t revents, void *u
         assert(e);
         assert(e == f->stdout_event_source);
         assert(fd >= 0);
-        assert(fd == STDOUT_FILENO);
+        assert(fd == f->output_fd);
 
         if (revents & (EPOLLOUT|EPOLLHUP))
                 f->stdout_writable = true;
@@ -377,7 +394,7 @@ static int on_sigwinch_event(sd_event_source *e, const struct signalfd_siginfo *
         assert(e == f->sigwinch_event_source);
 
         /* The window size changed, let's forward that. */
-        if (ioctl(STDOUT_FILENO, TIOCGWINSZ, &ws) >= 0)
+        if (ioctl(f->output_fd, TIOCGWINSZ, &ws) >= 0)
                 (void) ioctl(f->master, TIOCSWINSZ, &ws);
 
         return 0;
@@ -400,6 +417,8 @@ int pty_forward_new(
         *f = (struct PTYForward) {
                 .flags = flags,
                 .master = -1,
+                .input_fd = -1,
+                .output_fd = -1,
         };
 
         if (event)
@@ -410,14 +429,42 @@ int pty_forward_new(
                         return r;
         }
 
-        if (!(flags & PTY_FORWARD_READ_ONLY)) {
-                r = fd_nonblock(STDIN_FILENO, true);
-                if (r < 0)
-                        return r;
+        if (FLAGS_SET(flags, PTY_FORWARD_READ_ONLY))
+                f->output_fd = STDOUT_FILENO;
+        else {
+                /* If we shall be invoked in interactive mode, let's switch on non-blocking mode, so that we
+                 * never end up staving one direction while we block on the other. However, let's be careful
+                 * here and not turn on O_NONBLOCK for stdin/stdout directly, but of re-opened copies of
+                 * them. This has two advantages: when we are killed abruptly the stdin/stdout fds won't be
+                 * left in O_NONBLOCK state for the next process using them. In addition, if some process
+                 * running in the background wants to continue writing to our stdout it can do so without
+                 * being confused by O_NONBLOCK. */
 
-                r = fd_nonblock(STDOUT_FILENO, true);
-                if (r < 0)
-                        return r;
+                f->input_fd = fd_reopen(STDIN_FILENO, O_RDONLY|O_CLOEXEC|O_NOCTTY|O_NONBLOCK);
+                if (f->input_fd < 0) {
+                        /* Handle failures gracefully, after all certain fd types cannot be reopened
+                         * (sockets, …) */
+                        log_debug_errno(f->input_fd, "Failed to reopen stdin, using original fd: %m");
+
+                        r = fd_nonblock(STDIN_FILENO, true);
+                        if (r < 0)
+                                return r;
+
+                        f->input_fd = STDIN_FILENO;
+                } else
+                        f->close_input_fd = true;
+
+                f->output_fd = fd_reopen(STDOUT_FILENO, O_WRONLY|O_CLOEXEC|O_NOCTTY|O_NONBLOCK);
+                if (f->output_fd < 0) {
+                        log_debug_errno(f->output_fd, "Failed to reopen stdout, using original fd: %m");
+
+                        r = fd_nonblock(STDOUT_FILENO, true);
+                        if (r < 0)
+                                return r;
+
+                        f->output_fd = STDOUT_FILENO;
+                } else
+                        f->close_output_fd = true;
         }
 
         r = fd_nonblock(master, true);
@@ -426,7 +473,7 @@ int pty_forward_new(
 
         f->master = master;
 
-        if (ioctl(STDOUT_FILENO, TIOCGWINSZ, &ws) < 0) {
+        if (ioctl(f->output_fd, TIOCGWINSZ, &ws) < 0) {
                 /* If we can't get the resolution from the output fd, then use our internal, regular width/height,
                  * i.e. something derived from $COLUMNS and $LINES if set. */
 
@@ -439,7 +486,9 @@ int pty_forward_new(
         (void) ioctl(master, TIOCSWINSZ, &ws);
 
         if (!(flags & PTY_FORWARD_READ_ONLY)) {
-                if (tcgetattr(STDIN_FILENO, &f->saved_stdin_attr) >= 0) {
+                assert(f->input_fd >= 0);
+
+                if (tcgetattr(f->input_fd, &f->saved_stdin_attr) >= 0) {
                         struct termios raw_stdin_attr;
 
                         f->saved_stdin = true;
@@ -447,10 +496,10 @@ int pty_forward_new(
                         raw_stdin_attr = f->saved_stdin_attr;
                         cfmakeraw(&raw_stdin_attr);
                         raw_stdin_attr.c_oflag = f->saved_stdin_attr.c_oflag;
-                        tcsetattr(STDIN_FILENO, TCSANOW, &raw_stdin_attr);
+                        tcsetattr(f->input_fd, TCSANOW, &raw_stdin_attr);
                 }
 
-                if (tcgetattr(STDOUT_FILENO, &f->saved_stdout_attr) >= 0) {
+                if (tcgetattr(f->output_fd, &f->saved_stdout_attr) >= 0) {
                         struct termios raw_stdout_attr;
 
                         f->saved_stdout = true;
@@ -459,10 +508,10 @@ int pty_forward_new(
                         cfmakeraw(&raw_stdout_attr);
                         raw_stdout_attr.c_iflag = f->saved_stdout_attr.c_iflag;
                         raw_stdout_attr.c_lflag = f->saved_stdout_attr.c_lflag;
-                        tcsetattr(STDOUT_FILENO, TCSANOW, &raw_stdout_attr);
+                        tcsetattr(f->output_fd, TCSANOW, &raw_stdout_attr);
                 }
 
-                r = sd_event_add_io(f->event, &f->stdin_event_source, STDIN_FILENO, EPOLLIN|EPOLLET, on_stdin_event, f);
+                r = sd_event_add_io(f->event, &f->stdin_event_source, f->input_fd, EPOLLIN|EPOLLET, on_stdin_event, f);
                 if (r < 0 && r != -EPERM)
                         return r;
 
@@ -470,7 +519,7 @@ int pty_forward_new(
                         (void) sd_event_source_set_description(f->stdin_event_source, "ptyfwd-stdin");
         }
 
-        r = sd_event_add_io(f->event, &f->stdout_event_source, STDOUT_FILENO, EPOLLOUT|EPOLLET, on_stdout_event, f);
+        r = sd_event_add_io(f->event, &f->stdout_event_source, f->output_fd, EPOLLOUT|EPOLLET, on_stdout_event, f);
         if (r == -EPERM)
                 /* stdout without epoll support. Likely redirected to regular file. */
                 f->stdout_writable = true;