screen-devel
[Top][All Lists]
Advanced

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: [screen-devel] [PATCH v3 (screen master)] screen: handle pts devices


From: Amadeusz Sławiński
Subject: Re: [screen-devel] [PATCH v3 (screen master)] screen: handle pts devices in different namespaces
Date: Thu, 6 Apr 2017 10:29:37 +0200

Hey,

Thanks, pushed to master and screen-v4.

Amadeusz

On Wed,  5 Apr 2017 14:24:21 +0200
Christian Brauner <address@hidden> wrote:

> Various programs that deal with namespaces will use pty devices that exist in
> another namespace. One obvious candidate are containers. So far ttyname() was
> 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>.
> 
> Signed-off-by: Christian Brauner <address@hidden>
> ---
> Changelog: 2017-04-05
>       - remove attach_tty_is_in_new_ns from socket.c
>         This variable is unneeded and it does harm since it may end up falsely
>         initialized when not sent over the socket itself. The most promiment
>         instance of failure is when starting screen in detached mode
>         (screen -dm).
> ---
>  src/attacher.c |  6 ++--
>  src/screen.c   | 93 
> +++++++++++++++++++++++++++++++++++++---------------------
>  src/screen.h   |  4 +++
>  src/socket.c   | 43 ++++++++++++++++++++++++---
>  src/tty.c      | 24 +++++++++++++++
>  src/tty.h      |  1 +
>  src/utmp.c     |  4 +--
>  7 files changed, 133 insertions(+), 42 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..c741eb4 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>
> @@ -76,6 +78,7 @@ static void logflush_fn(Event *, void *);
>  static int IsSymbol(char *, char *);
>  static char *ParseChar(char *, char *);
>  static int ParseEscape(char *);
> +static void SetTtyname(bool fatal, struct stat *st);
>  
>  int nversion;                        /* numerical version, used for 
> secondary DA */
>  
> @@ -86,6 +89,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,34 +651,6 @@ 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)
> -
>       if (home == 0 || *home == '\0')
>               home = ppp->pw_dir;
>       if (strlen(LoginName) > MAXLOGINLEN)
> @@ -687,7 +666,7 @@ int main(int argc, char **argv)
>               int fl;
>  
>               /* ttyname implies isatty */
> -             SET_TTYNAME(1);
> +             SetTtyname(true, &st);
>               tty_mode = (int)st.st_mode & 0777;
>  
>               fl = fcntl(0, F_GETFL, 0);
> @@ -697,7 +676,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)
> @@ -824,7 +812,7 @@ int main(int argc, char **argv)
>       xsignal(SIG_BYE, AttacherFinit);        /* prevent races */
>       if (cmdflag) {
>               /* attach_tty is not mandatory */
> -             SET_TTYNAME(0);
> +             SetTtyname(false, &st);
>               if (!*argv)
>                       Panic(0, "Please specify a command.");
>               SET_GUID();
> @@ -838,7 +826,7 @@ int main(int argc, char **argv)
>               if (multiattach)
>                       Panic(0, "Can't create sessions of other users.");
>       } else if (dflag && !mflag) {
> -             SET_TTYNAME(0);
> +             SetTtyname(false, &st);
>               Attach(MSG_DETACH);
>               Msg(0, "[%s %sdetached.]\n", SocketName, (dflag > 1 ? "power " 
> : ""));
>               eexit(0);
> @@ -846,7 +834,7 @@ int main(int argc, char **argv)
>       }
>       if (!SocketMatch && !mflag && sty) {
>               /* attach_tty is not mandatory */
> -             SET_TTYNAME(0);
> +             SetTtyname(false, &st);
>               SET_GUID();
>               nwin_options.args = argv;
>               SendCreateMsg(sty, &nwin);
> @@ -1805,3 +1793,42 @@ static int ParseEscape(char *p)
>       return 0;
>  }
>  
> +void SetTtyname(bool fatal, struct stat *st)
> +{
> +     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 (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);
> +
> +             if (strlen(attach_tty) >= MAXPATHLEN)
> +                     Panic(0, "TtyName too long - sorry.");
> +
> +             /* 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);
> +     }
> +}
> 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..af47077 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,45 @@ static int CreateTempDisplay(Message *m, int recvfd, 
> Window *win)
>               return -1;
>       }
>       if (recvfd != -1) {
> +             int ret;
> +             char ttyname_in_ns[MAXPATHLEN] = {0};
>               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");
> +             errno = 0;
> +             myttyname = GetPtsPathOrSymlink(i);
> +             if (myttyname && errno == ENODEV) {
> +                     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..67702b6 100644
> --- a/src/tty.c
> +++ b/src/tty.c
> @@ -1193,3 +1193,27 @@ 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];
> +
> +     errno = 0;
> +     tty_name = ttyname(fd);
> +     if (!tty_name && errno == ENODEV) {
> +             ret = snprintf(tty_symlink, MAX_PTS_SYMLINK, 
> "/proc/self/fd/%d", fd);
> +             if (ret < 0 || ret >= MAX_PTS_SYMLINK)
> +                     return NULL;
> +             /* We are setting errno to ENODEV to allow callers to check
> +              * whether the pts device exists in another namespace.
> +              */
> +             errno = ENODEV;
> +             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);
>  }
>  




reply via email to

[Prev in Thread] Current Thread [Next in Thread]