[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [screen-devel] [PATCH (screen master)] screen: handle pts devices in
|
From: |
Christian Brauner |
|
Subject: |
Re: [screen-devel] [PATCH (screen master)] screen: handle pts devices in different namespaces |
|
Date: |
Mon, 3 Apr 2017 16:10:05 +0200 |
Hey,
thanks for the quick reply! :)
On Mon, Apr 03, 2017 at 02:26:10PM +0200, Amadeusz Sławiński wrote:
> 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
Changed.
>
> > 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?
Yes, when we wrote the glibc patch we were very careful to account for this
case by checking the st_dev and st_ino fields. So even if, by accident, the
/dev/pts/4 device exists in the namespace ttyname{_r}() will return NULL and set
errno to 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).
Right, thanks! Changed.
>
> > + 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()?
Yes. I just left the order like I found it before. Changed.
>
> > + } \
> > + } while (0)
>
> I think it may be time to make SET_TTYNAME into separate function... it's
> starting to get big.
Sure, will do. Changed.
>
> >
> > 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.
Changed.
>
> > + 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 */
Right, so just do errno = ENODEV. Changed.
>
> > + 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?
Oh sure! Note however, that you require a version of glibc that includes this
patch:
commit 15e9a4f378c8607c2ae1aa465436af4321db0e23
Author: Christian Brauner <address@hidden>
Date: Fri Jan 27 15:59:59 2017 +0100
linux ttyname and ttyname_r: do not return wrong results
If a link (say /proc/self/fd/0) pointing to a device, say /dev/pts/2,
in a
parent mount namespace is passed to ttyname, and a /dev/pts/2 exists
(in a
different devpts) in the current namespace, then it returns /dev/pts/2.
But /dev/pts/2 is NOT the current tty, it is a different file and
device.
Detect this case and return ENODEV. Userspace can choose to take this
as a hint
that the fd points to a tty device but to act on the fd rather than the
link.
Signed-off-by: Serge Hallyn <address@hidden>
Signed-off-by: Christian Brauner <address@hidden>
If you have one then you can do
sudo unshare --fork --pid --mount-proc
and then to mount a new devpts instance:
mount -t devpts devpts /dev/pts
If you call
tty
you should see "not a tty". To further test whether the glibc is patched you can
compile this test program:
/*
*
* Copyright © 2017 Christian Brauner <address@hidden>.
*
* This program is free software; you can redistribute it and/or modify
* it under the terms of the GNU General Public License version 2, as
* published by the Free Software Foundation.
*
* This program is distributed in the hope that it will be useful,
* but WITHOUT ANY WARRANTY; without even the implied warranty of
* MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
* GNU General Public License for more details.
*
* You should have received a copy of the GNU General Public License along
* with this program; if not, write to the Free Software Foundation, Inc.,
* 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301 USA.
*/
#define _GNU_SOURCE
#include <errno.h>
#include <fcntl.h>
#include <stdio.h>
#include <stdlib.h>
#include <string.h>
#include <sys/stat.h>
#include <sys/types.h>
#include <unistd.h>
/* Test whether ttyname() */
int main(int argc, char *argv[])
{
int fd;
char buf[4096];
fd = open("/proc/self/fd/0", O_RDONLY);
if (fd < 0) {
fprintf(stderr, "Could not open \"/proc/self/fd/0\": %s.\n",
strerror(errno));
exit(EXIT_FAILURE);
}
if (!ttyname(fd)) {
/* COMMENT(brauner): ENODEV will only be set by a patched
* glibc.
*/
if (errno == ENODEV) {
printf("ttyname(): The pty device might exist in a "
"different "
"namespace: %s\n",
strerror(errno));
} else {
exit(EXIT_FAILURE);
}
}
if (ttyname_r(fd, buf, sizeof(buf))) {
/* COMMENT(brauner): ENODEV will only be set by a patched
* glibc.
*/
if (errno == ENODEV) {
printf("ttyname_r(): The pty device might exist in a "
"different "
"namespace: %s\n",
strerror(errno));
} else {
exit(EXIT_FAILURE);
}
}
exit(EXIT_SUCCESS);
}
Now, in this setting you can also test a patched screen version. A non-patched
screen version should refuse to run, a patched screen version should run just
fine. An updated version of this patch will follow in a bit.