[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [screen-devel] [PATCH (screen master)] screen: handle pts devices in
|
From: |
Amadeusz Sławiński |
|
Subject: |
Re: [screen-devel] [PATCH (screen master)] screen: handle pts devices in different namespaces |
|
Date: |
Mon, 3 Apr 2017 14:26:10 +0200 |
Hey,
thanks for patch, comments inline
On Mon, 3 Apr 2017 13:04:05 +0200
Christian Brauner <address@hidden> wrote:
> Various programs that deal with namespaces will use pty devices that exist in
> another namespace. One obvious candiate are containers. So far ttyname() was
/candiate/candidate
> incorrectly handling this case because the pts device stems from the host and
> thus cannot be found amongst the current namespace's /dev/pts/<n> entries.
> Serge Hallyn and I recently upstreamed patches to glibc that allow
> ttyname{_r}() to correctly handle this case. At a minimum, ttyname{_r}() will
> set errno to ENODEV in case it finds that the /dev/pts/<n> device that the
> symlink points to exists in another namespace.
>
> (The next comment is a little longer but tries to ensure that one can still
> understand what is going on after some time has passed.)
> In case we detect that ttyname{_r}() returns NULL and sets errno to ENODEV we
> have ample reason to assume that the pts device exists in a different
> namespace. In this case, the code will set a global flag indicating this case
> to true. Furthermore, all operations (e.g. chmod(), chown(), etc.) will now
> need to operate on the symbolic link /proc/self/fd/0 directly. While this
> sounds straightforward, it becomes difficult to handle this case correctly
> when
> we reattach to an already existing screen session from a different pts device
> than the original one. Let's look at the general reattach logic a little
> closer:
>
> Assume we are running a shell that uses a pts device from a different
> namespace:
>
> address@hidden:~# ls -al /proc/self/fd/
> total 0
> dr-x------ 2 root root 0 Apr 2 20:22 .
> dr-xr-xr-x 9 root root 0 Apr 2 20:22 ..
> lrwx------ 1 root root 64 Apr 2 20:22 0 -> /dev/pts/6
> lrwx------ 1 root root 64 Apr 2 20:22 1 -> /dev/pts/6
> lrwx------ 1 root root 64 Apr 2 20:22 2 -> /dev/pts/6
> l-wx------ 1 root root 64 Apr 2 20:22 3 -> pipe:[3067913]
> lr-x------ 1 root root 64 Apr 2 20:22 4 -> /proc/27413/fd
> lrwx------ 1 root root 64 Apr 2 20:22 9 -> socket:[32944]
>
> address@hidden:~# ls -al /dev/pts/
> total 0
> drwxr-xr-x 2 root root 0 Mar 30 17:55 .
> drwxr-xr-x 8 root root 580 Mar 30 17:55 ..
> crw--w---- 1 root tty 136, 0 Mar 30 17:55 0
> crw--w---- 1 root tty 136, 1 Mar 30 17:55 1
> crw--w---- 1 root tty 136, 2 Mar 30 17:55 2
> crw--w---- 1 root tty 136, 3 Mar 30 17:55 3
> crw--w---- 1 root tty 136, 4 Mar 30 17:55 4
> crw-rw-rw- 1 root root 5, 2 Apr 2 20:22 ptmx
>
> (As one can see /dev/pts/6 does not exist in the current namespace.)
> Now, start a screen session in this shell. In this case this patch will have
> screen directly operate on /proc/self/fd/0.
> Let's look at the attach case. When we attach to an existing screen session
> where the associated pts device lives in another namespace we need a way to
> uniquely identify the pts device that is used and also need a way to get a
> valid fd when we need one. This patch solves this by ensuring that a valid
> file
> descriptor to the pts device is sent via a unix socket and SCM_RIGHTS to the
> socket and display handling part of screen. However, screen also sends around
> the name of the associated pts device or, in the case where the pts device
> exists in another namespace, the symlink /proc/self/fd/0. But after having
> sent
> the fd this part of the codebase cannot simply operate on /proc/self/fd/0
> since
> it very likely refers to a different file. So we need to operate on
> /proc/self/fd/<fd-sent-via-SCM_RIGHTS> but also need to ensure that we haven't
> been tricked into operating on a tampered with file or device. So we cannot
> simply sent /proc/self/fd/0 via the unix socket. Instead we read the contents
> of the symbolic link /proc/self/fd/0 in the main function and sent it via the
> unix socket. Then in the socket and display handling part of screen, we read
> the contents of the /proc/self/fd/<fd-sent-via-SCM_RIGHTS> as well and compare
> the pts device names. If they match we know that everything is well. However,
> now we also need to update any tty handling code to directly operate on
> /proc/self/fd/<fd-sent-via-SCM_RIGHTS>.
What happens if screen runs on /dev/pts/4 and there exists /dev/pts/4 in
namespace?
Will we also get ENODEV?
>
> Signed-off-by: Christian Brauner <address@hidden>
> ---
> src/attacher.c | 6 ++--
> src/screen.c | 89
> ++++++++++++++++++++++++++++++++++++++++------------------
> src/screen.h | 4 +++
> src/socket.c | 44 ++++++++++++++++++++++++++---
> src/tty.c | 23 +++++++++++++++
> src/tty.h | 1 +
> src/utmp.c | 4 +--
> 7 files changed, 134 insertions(+), 37 deletions(-)
>
> diff --git a/src/attacher.c b/src/attacher.c
> index 4db7243..e483c0b 100644
> --- a/src/attacher.c
> +++ b/src/attacher.c
> @@ -137,7 +137,7 @@ int Attach(int how)
> memset((char *)&m, 0, sizeof(m));
> m.type = how;
> m.protocol_revision = MSG_REVISION;
> - strncpy(m.m_tty, attach_tty, sizeof(m.m_tty) - 1);
> + strncpy(m.m_tty, attach_tty_is_in_new_ns ? attach_tty_name_in_ns :
> attach_tty, sizeof(m.m_tty) - 1);
> m.m_tty[sizeof(m.m_tty) - 1] = 0;
>
> if (how == MSG_WINCH) {
> @@ -328,7 +328,7 @@ void AttacherFinit(int sigsig)
> /* Check if signal comes from backend */
> if (stat(SocketPath, &statb) == 0 && (statb.st_mode & 0777) != 0600) {
> memset((char *)&m, 0, sizeof(m));
> - strncpy(m.m_tty, attach_tty, sizeof(m.m_tty) - 1);
> + strncpy(m.m_tty, attach_tty_is_in_new_ns ?
> attach_tty_name_in_ns : attach_tty, sizeof(m.m_tty) - 1);
> m.m_tty[sizeof(m.m_tty) - 1] = 0;
> m.m.detach.dpid = getpid();
> m.type = MSG_HANGUP;
> @@ -493,7 +493,7 @@ void SendCmdMessage(char *sty, char *match, char **av,
> int query)
> memset((char *)&m, 0, sizeof(m));
> m.type = query ? MSG_QUERY : MSG_COMMAND;
> if (attach_tty) {
> - strncpy(m.m_tty, attach_tty, sizeof(m.m_tty) - 1);
> + strncpy(m.m_tty, attach_tty_is_in_new_ns ?
> attach_tty_name_in_ns : attach_tty, sizeof(m.m_tty) - 1);
> m.m_tty[sizeof(m.m_tty) - 1] = 0;
> }
> p = m.m.command.cmd;
> diff --git a/src/screen.c b/src/screen.c
> index 6d06cf1..1fb5668 100644
> --- a/src/screen.c
> +++ b/src/screen.c
> @@ -37,6 +37,8 @@
> #include <pwd.h>
> #include <signal.h>
> #include <stdint.h>
> +#include <stdbool.h>
> +#include <unistd.h>
> #include <sys/ioctl.h>
> #include <sys/stat.h>
> #include <sys/types.h>
> @@ -86,6 +88,10 @@ int attach_fd = -1;
> char *attach_term;
> char *LoginName;
> struct mode attach_Mode;
> +/* Indicator whether the current tty exists in another namespace. */
> +bool attach_tty_is_in_new_ns = false;
> +/* Content of the tty symlink when attach_tty_is_in_new_ns == true. */
> +char attach_tty_name_in_ns[MAXPATHLEN];
>
> char SocketPath[MAXPATHLEN + 2 * MAXSTR];
> char *SocketName; /* SocketName is pointer in SocketPath
> */
> @@ -644,33 +650,51 @@ int main(int argc, char **argv)
> eff_gid = real_gid; \
> } while (0)
>
> -#define SET_TTYNAME(fatal) do \
> - { \
> - int saved_errno = 0; \
> - errno = 0; \
> - if (!(attach_tty = ttyname(0))) \
> - { \
> - /* stdin is a tty but it exists in another namespace. */ \
> - if (fatal && errno == ENODEV) \
> - attach_tty = ""; \
> - else if (fatal) \
> - Panic(0, "Must be connected to a terminal."); \
> - else \
> - attach_tty = ""; \
> - } \
> - else \
> - { \
> - saved_errno = errno; \
> - if (stat(attach_tty, &st)) \
> - Panic(errno, "Cannot access '%s'", attach_tty); \
> - /* Only call CheckTtyname() if the device does not exist in another \
> - * namespace. */ \
> - if (saved_errno != ENODEV && CheckTtyname(attach_tty)) \
> - Panic(0, "Bad tty '%s'", attach_tty); \
> - } \
> - if (strlen(attach_tty) >= MAXPATHLEN) \
> - Panic(0, "TtyName too long - sorry."); \
> - } while (0)
> +#define SET_TTYNAME(fatal)
> \
> + do { \
> + int ret; \
> + int saved_errno = 0; \
> + attach_tty_is_in_new_ns = false; \
> + memset(&attach_tty_name_in_ns, 0, \
> + sizeof(attach_tty_name_in_ns)); \
> + errno = 0; \
> + attach_tty = ttyname(0); \
> + if (!attach_tty) { \
> + if (fatal && errno == ENODEV || \
> + !fatal && errno == ENODEV) { \
Uh... I think Mr. Bool would agree with me that it can be just (errno ==
ENODEV).
> + saved_errno = errno; \
> + attach_tty = "/proc/self/fd/0"; \
> + attach_tty_is_in_new_ns = true; \
> + ret = readlink(attach_tty, \
> + attach_tty_name_in_ns, \
> + sizeof(attach_tty_name_in_ns)); \
> + if (ret < 0 || \
> + (size_t)ret >= \
> + sizeof(attach_tty_name_in_ns)) { \
> + Panic(0, "Bad tty '%s'", attach_tty); \
> + } \
> + } else if (fatal) { \
> + Panic(0, "Must be connected to a terminal."); \
> + } else { \
> + attach_tty = ""; \
> + } \
> + } \
> +
> \
> + if (attach_tty) { \
> + if (stat(attach_tty, &st)) \
> + Panic(errno, "Cannot access '%s'", \
> + attach_tty); \
> +
> \
> + /* Only call CheckTtyname() if the device does not \
> + * exist in another \
> + * namespace. */ \
> + if (saved_errno != ENODEV && CheckTtyname(attach_tty)) \
> + Panic(0, "Bad tty '%s'", attach_tty); \
> +
> \
> + if (strlen(attach_tty) >= MAXPATHLEN) \
> + Panic(0, "TtyName too long - sorry."); \
If we are checking this, wouldn't it be better to check this before stat()?
> + } \
> + } while (0)
I think it may be time to make SET_TTYNAME into separate function... it's
starting to get big.
>
> if (home == 0 || *home == '\0')
> home = ppp->pw_dir;
> @@ -697,7 +721,16 @@ int main(int argc, char **argv)
> if (attach_fd == -1) {
> if ((n = secopen(attach_tty, O_RDWR | O_NONBLOCK, 0)) <
> 0)
> Panic(0, "Cannot open your terminal '%s' -
> please check.", attach_tty);
> - close(n);
> + /* In case the pts device exists in another namespace
> we directly operate
> + * on the symbolic link itself. However, this means
> that we need to keep
> + * the fd open since we have no direct way of
> identifying the associated
> + * pts device accross namespaces. This is ok though
> since keeping fds open
> + * is done in the codebase already.
> + */
> + if (attach_tty_is_in_new_ns)
> + attach_fd = n;
> + else
> + close(n);
> }
>
> if ((attach_term = getenv("TERM")) == 0 || *attach_term == 0)
> diff --git a/src/screen.h b/src/screen.h
> index e0876b0..7486252 100644
> --- a/src/screen.h
> +++ b/src/screen.h
> @@ -233,6 +233,8 @@ void setbacktick (int, int, int, char **);
>
> /* global variables */
>
> +/* Content of the tty symlink when attach_tty_is_in_new_ns == true. */
> +extern char attach_tty_name_in_ns[];
> extern char strnomem[];
> extern char HostName[];
> extern char SocketPath[];
> @@ -274,6 +276,8 @@ extern bool lsflag;
> extern bool quietflag;
> extern bool wipeflag;
> extern bool xflag;
> +/* Indicator whether the current tty exists in another namespace. */
> +extern bool attach_tty_is_in_new_ns;
>
> extern int attach_fd;
> extern int dflag;
> diff --git a/src/socket.c b/src/socket.c
> index cfb43fe..82c69f7 100644
> --- a/src/socket.c
> +++ b/src/socket.c
> @@ -41,6 +41,8 @@
> #include <utime.h>
> #include <stdint.h>
> #include <stdbool.h>
> +#include <string.h>
> +#include <unistd.h>
> #include <signal.h>
>
> #include "screen.h"
> @@ -574,12 +576,46 @@ static int CreateTempDisplay(Message *m, int recvfd,
> Window *win)
> return -1;
> }
> if (recvfd != -1) {
> + int ret;
> + char ttyname_in_ns[MAXPATHLEN];
> char *myttyname;
> i = recvfd;
> - myttyname = ttyname(i);
> - if (myttyname == 0 || strcmp(myttyname, m->m_tty)) {
> - Msg(errno, "Attach: passed fd does not match tty: %s -
> %s!", m->m_tty,
> - myttyname ? myttyname : "NULL");
> + memset(&ttyname_in_ns, 0, sizeof(ttyname_in_ns));
You can just do char ttyname_in_ns[MAXPATHLEN] = { 0 }; on master branch.
> + errno = 0;
> + myttyname = GetPtsPathOrSymlink(i);
> + if (myttyname && errno == ENODEV && attach_tty_is_in_new_ns) {
> + ret = readlink(myttyname, ttyname_in_ns,
> + sizeof(ttyname_in_ns));
> + if (ret < 0 || (size_t)ret >= sizeof(ttyname_in_ns)) {
> + Msg(errno, "Could not perform necessary sanity "
> + "checks on pts device.");
> + close(i);
> + Kill(pid, SIG_BYE);
> + return -1;
> + }
> + if (strcmp(ttyname_in_ns, m->m_tty)) {
> + Msg(errno, "Attach: passed fd does not match "
> + "tty: %s - %s!",
> + ttyname_in_ns,
> + m->m_tty[0] != '\0' ? m->m_tty : "(null)");
> + close(i);
> + Kill(pid, SIG_BYE);
> + return -1;
> + }
> + /* m->m_tty so far contains the actual name of the pts
> + * device in its namespace (e.g. /dev/pts/0). This name
> + * however is not valid in the current namespace. So
> + * after we verified that the symlink returned by
> + * GetPtsPathOrSymlink() refers to the same pts device
> + * in this namespace we need to update m->m_tty to use
> + * that symlink for all future operations.
> + */
> + strncpy(m->m_tty, myttyname, sizeof(m->m_tty) - 1);
> + m->m_tty[sizeof(m->m_tty) - 1] = 0;
> + } else if (myttyname == 0 || strcmp(myttyname, m->m_tty)) {
> + Msg(errno,
> + "Attach: passed fd does not match tty: %s - %s!",
> + m->m_tty, myttyname ? myttyname : "NULL");
> close(i);
> Kill(pid, SIG_BYE);
> return -1;
> diff --git a/src/tty.c b/src/tty.c
> index 0f18896..1861fb4 100644
> --- a/src/tty.c
> +++ b/src/tty.c
> @@ -1193,3 +1193,26 @@ int CheckTtyname(char *tty)
>
> return rc;
> }
> +
> +/* len(/proc/self/fd/) + len(max 64 bit int) */
> +#define MAX_PTS_SYMLINK (14 + 21)
> +char *GetPtsPathOrSymlink(int fd)
> +{
> + int ret;
> + char *tty_name;
> + static char tty_symlink[MAX_PTS_SYMLINK];
> + int saved_errno = 0;
> +
> + errno = 0;
> + tty_name = ttyname(fd);
> + if (!tty_name && errno == ENODEV) {
> + saved_errno = errno;
> + ret = snprintf(tty_symlink, MAX_PTS_SYMLINK,
> "/proc/self/fd/%d", fd);
> + if (ret < 0 || ret >= MAX_PTS_SYMLINK)
> + return NULL;
> + errno = saved_errno;
only case we care about here is errno == ENODEV so we don't need to save it into
another variable (saved_errno), just set it directly, also wouldn't mind some
comment like: /* used to check outside of function to see if we are inside
namespace */
> + return tty_symlink;
> + }
> +
> + return tty_name;
> +}
> diff --git a/src/tty.h b/src/tty.h
> index ccb1e53..fd95b67 100644
> --- a/src/tty.h
> +++ b/src/tty.h
> @@ -17,6 +17,7 @@ struct baud_values *lookup_baud (int bps);
> int SetBaud (struct mode *, int, int);
> int SttyMode (struct mode *, char *);
> int CheckTtyname (char *);
> +char *GetPtsPathOrSymlink (int);
>
> /* global variables */
>
> diff --git a/src/utmp.c b/src/utmp.c
> index 75f7fc8..7657525 100644
> --- a/src/utmp.c
> +++ b/src/utmp.c
> @@ -210,7 +210,7 @@ void RemoveLoginSlot()
> struct stat stb;
> char *tty;
> D_loginttymode = 0;
> - if ((tty = ttyname(D_userfd)) && stat(tty, &stb) == 0 &&
> stb.st_uid == real_uid && !CheckTtyname(tty)
> + if ((tty = GetPtsPathOrSymlink(D_userfd)) && stat(tty, &stb) ==
> 0 && stb.st_uid == real_uid && !CheckTtyname(tty)
> && ((int)stb.st_mode & 0777) != 0666) {
> D_loginttymode = (int)stb.st_mode & 0777;
> chmod(D_usertty, stb.st_mode & 0600);
> @@ -230,7 +230,7 @@ void RestoreLoginSlot()
> Msg(errno, "Could not write %s", UtmpName);
> }
> D_loginslot = (slot_t) 0;
> - if (D_loginttymode && (tty = ttyname(D_userfd)) && !CheckTtyname(tty))
> + if (D_loginttymode && (tty = GetPtsPathOrSymlink(D_userfd)) &&
> !CheckTtyname(tty))
> fchmod(D_userfd, D_loginttymode);
> }
>
Can you also provide some instructions, so I can test this?
Cheers,
Amadeusz