This is the mail archive of the
libc-alpha@sourceware.org
mailing list for the glibc project.
Re: [PATCH] login: Remove the pt_chown program
On 1/7/19 10:10 AM, Florian Weimer wrote:
> * Florian Weimer:
>
>> The program is not safe to install in a container environment.
>> It was last installed by default in glibc 2.17.
>>
>> (This is probably something for 2.30 at this point.)
OK for 2.30 if you fixup the retval issue.
I agree that it's time we removed the ability to use pt_chown, this has
to always be handled by the kernel.
Reviewed-by: Carlos O'Donell <carlos@redhat.com>
> Hmph. I forgot to update config.make.in and config.h.in.
>
> 2019-01-07 Florian Weimer <fweimer@redhat.com>
>
> login: Remove the pt_chown program.
> * configure.ac (--enable-pt_chown): Remove option.
> * config.h.in: Remove HAVE_PT_CHOWN.
> * config.make.in (build-pt-chown): Remove variable.
> * login/Makefile (others, others-pie, install-others-programs): Do
> not add pt_chown.
> (pt_chown-cflags, libcap, CFLAGS-pt_chown.c, LDLIBS-pt_chown)
> (LDFLAGS-pt_chown): Remove variables.
> (pt_chown): Remove target.
> * sysdeps/unix/grantpt.c (grantpt): Remove code to call the helper
> program.
> * manual/install.texi (Configuring and compiling): Remove
> description of --enable-pt_chown.
> (Running make install): Remove description of pt_chown.
> * manual/terminal.texi (Allocation): Update comment.
> * sysdeps/generic/pty-private.h (_PATH_PT_CHOWN): Remove.
> * login/programs/pt_chown.c: Remove file.
> * sysdeps/unix/sysv/linux/grantpt.c: Likewise.
> * configure, INSTALL: Regenerate.
>
> diff --git a/NEWS b/NEWS
> index cc20102fda..58f2c12cb8 100644
> --- a/NEWS
> +++ b/NEWS
> @@ -85,6 +85,10 @@ Deprecated and removed features, and other changes affecting compatibility:
> as all functions that call vscanf, vfscanf, or vsscanf are annotated with
> __attribute__ ((format (scanf, ...))).
>
> +* The pt_chown program has been removed. (It has not been installed by
> + default since glibc 2.18.) Changing ownership of PTYs cannot be made safe
> + in userspace in a container environment.
> +
OK.
> Changes to build and runtime requirements:
>
> * Python 3.4 or later is required to build the GNU C Library.
> diff --git a/config.h.in b/config.h.in
> index f059ec0435..43fc7a9041 100644
> --- a/config.h.in
> +++ b/config.h.in
> @@ -229,9 +229,6 @@
> /* The ARM movw/movt instructions using PC-relative relocs work right. */
> #define ARM_PCREL_MOVW_OK 0
>
> -/* The pt_chown binary is being built and used by grantpt. */
> -#define HAVE_PT_CHOWN 0
> -
OK.
> /* Define if the compiler supports __builtin_trap without
> any external dependencies such as making a function call. */
> #define HAVE_BUILTIN_TRAP 0
> diff --git a/config.make.in b/config.make.in
> index f46bfc29bb..2506e317d1 100644
> --- a/config.make.in
> +++ b/config.make.in
> @@ -101,7 +101,6 @@ build-crypt = @build_crypt@
> build-nscd = @build_nscd@
> use-nscd = @use_nscd@
> build-hardcoded-path-in-tests= @hardcoded_path_in_tests@
> -build-pt-chown = @build_pt_chown@
OK.
> have-tunables = @have_tunables@
>
> # Build tools.
> diff --git a/login/Makefile b/login/Makefile
> index 92535f0aec..5c78b491f3 100644
> --- a/login/Makefile
> +++ b/login/Makefile
> @@ -34,12 +34,6 @@ CFLAGS-grantpt.c += -DLIBEXECDIR='"$(libexecdir)"'
>
> others = utmpdump
>
> -ifeq (yes,$(build-pt-chown))
> -others += pt_chown
> -others-pie = pt_chown
> -install-others-programs = $(inst_libexecdir)/pt_chown
> -endif
> -
OK.
> subdir-dirs = programs
> vpath %.c programs
>
> @@ -54,20 +48,3 @@ libutil-routines:= login login_tty logout logwtmp openpty forkpty
> include ../Rules
>
> CFLAGS-getpt.c += -fexceptions
> -
> -ifeq (yesyes,$(have-fpie)$(build-shared))
> -pt_chown-cflags += $(pie-ccflag)
> -endif
> -ifeq (yes,$(have-libcap))
> -libcap = -lcap
> -endif
> -CFLAGS-pt_chown.c += $(pt_chown-cflags)
> -LDLIBS-pt_chown = $(libcap)
Is this the only user of libcap in glibc?
I'm curious because if it is we should notify downstream distributions
to remove it from their libc dependencies when building packages.
I see we still have libcap-devel in Fedora Rawhide.
> -ifeq (yesyes,$(have-fpie)$(build-shared))
> -LDFLAGS-pt_chown = -Wl,-z,now
> -endif
> -
> -# pt_chown needs to be setuid root.
> -$(inst_libexecdir)/pt_chown: $(objpfx)pt_chown $(+force)
> - $(make-target-directory)
> - -$(INSTALL_PROGRAM) -m 4755 -o root $< $@
OK.
> diff --git a/login/programs/pt_chown.c b/login/programs/pt_chown.c
> deleted file mode 100644
> index d44e4bf6ed..0000000000
> --- a/login/programs/pt_chown.c
> +++ /dev/null
> @@ -1,215 +0,0 @@
> -/* pt_chmod - helper program for `grantpt'.
> - Copyright (C) 1998-2019 Free Software Foundation, Inc.
> - This file is part of the GNU C Library.
> - Contributed by C. Scott Ananian <cananian@alumni.princeton.edu>, 1998.
> -
> - The GNU C Library is free software; you can redistribute it and/or
> - modify it under the terms of the GNU Lesser General Public
> - License as published by the Free Software Foundation; either
> - version 2.1 of the License, or (at your option) any later version.
> -
> - The GNU C Library 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
> - Lesser General Public License for more details.
> -
> - You should have received a copy of the GNU Lesser General Public
> - License along with the GNU C Library; if not, see
> - <http://www.gnu.org/licenses/>. */
> -
> -#include <argp.h>
> -#include <errno.h>
> -#include <error.h>
> -#include <grp.h>
> -#include <libintl.h>
> -#include <locale.h>
> -#include <signal.h>
> -#include <stdio.h>
> -#include <stdlib.h>
> -#include <string.h>
> -#include <sys/stat.h>
> -#include <unistd.h>
> -#ifdef HAVE_LIBCAP
> -# include <sys/capability.h>
> -# include <sys/prctl.h>
> -#endif
> -
> -#include "pty-private.h"
> -
> -/* Get libc version number. */
> -#include "../version.h"
> -
> -#define PACKAGE _libc_intl_domainname
> -
> -/* Name and version of program. */
> -static void print_version (FILE *stream, struct argp_state *state);
> -void (*argp_program_version_hook) (FILE *, struct argp_state *) = print_version;
> -
> -/* Function to print some extra text in the help message. */
> -static char *more_help (int key, const char *text, void *input);
> -
> -/* Data structure to communicate with argp functions. */
> -static struct argp argp =
> -{
> - NULL, NULL, NULL, NULL, NULL, more_help
> -};
> -
> -
> -/* Print the version information. */
> -static void
> -print_version (FILE *stream, struct argp_state *state)
> -{
> - fprintf (stream, "pt_chown %s%s\n", PKGVERSION, VERSION);
> - fprintf (stream, gettext ("\
> -Copyright (C) %s Free Software Foundation, Inc.\n\
> -This is free software; see the source for copying conditions. There is NO\n\
> -warranty; not even for MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.\n\
> -"), "2019");
> -}
> -
> -static char *
> -more_help (int key, const char *text, void *input)
> -{
> - char *cp;
> - char *tp;
> -
> - switch (key)
> - {
> - case ARGP_KEY_HELP_PRE_DOC:
> - asprintf (&cp, gettext ("\
> -Set the owner, group and access permission of the slave pseudo\
> - terminal corresponding to the master pseudo terminal passed on\
> - file descriptor `%d'. This is the helper program for the\
> - `grantpt' function. It is not intended to be run directly from\
> - the command line.\n"),
> - PTY_FILENO);
> - return cp;
> - case ARGP_KEY_HELP_EXTRA:
> - /* We print some extra information. */
> - if (asprintf (&tp, gettext ("\
> -For bug reporting instructions, please see:\n\
> -%s.\n"), REPORT_BUGS_TO) < 0)
> - return NULL;
> - if (asprintf (&cp, gettext ("\
> -The owner is set to the current user, the group is set to `%s',\
> - and the access permission is set to `%o'.\n\n\
> -%s"),
> - TTY_GROUP, S_IRUSR|S_IWUSR|S_IWGRP, tp) < 0)
> - {
> - free (tp);
> - return NULL;
> - }
> - return cp;
> - default:
> - break;
> - }
> - return (char *) text;
> -}
> -
> -static int
> -do_pt_chown (void)
> -{
> - char *pty;
> - struct stat64 st;
> - struct group *p;
> - gid_t gid;
> -
> - /* Check that PTY_FILENO is a valid master pseudo terminal. */
> - pty = ptsname (PTY_FILENO);
> - if (pty == NULL)
> - return errno == EBADF ? FAIL_EBADF : FAIL_EINVAL;
> -
> - /* Check that the returned slave pseudo terminal is a
> - character device. */
> - if (stat64 (pty, &st) < 0 || !S_ISCHR (st.st_mode))
> - return FAIL_EINVAL;
> -
> - /* Get the group ID of the special `tty' group. */
> - p = getgrnam (TTY_GROUP);
> - gid = p ? p->gr_gid : getgid ();
> -
> - /* Set the owner to the real user ID, and the group to that special
> - group ID. */
> - if (chown (pty, getuid (), gid) < 0)
> - return FAIL_EACCES;
> -
> - /* Set the permission mode to readable and writable by the owner,
> - and writable by the group. */
> - if ((st.st_mode & ACCESSPERMS) != (S_IRUSR|S_IWUSR|S_IWGRP)
> - && chmod (pty, S_IRUSR|S_IWUSR|S_IWGRP) < 0)
> - return FAIL_EACCES;
> -
> - return 0;
> -}
> -
> -
> -int
> -main (int argc, char *argv[])
> -{
> - uid_t euid = geteuid ();
> - uid_t uid = getuid ();
> - int remaining;
> - sigset_t signalset;
> -
> - /* Clear any signal mask from the parent process. */
> - sigemptyset (&signalset);
> - sigprocmask (SIG_SETMASK, &signalset, NULL);
> -
> - if (argc == 1 && euid == 0)
> - {
> -#ifdef HAVE_LIBCAP
> - /* Drop privileges. */
> - if (uid != euid)
> - {
> - static const cap_value_t cap_list[] =
> - { CAP_CHOWN, CAP_FOWNER };
> -# define ncap_list (sizeof (cap_list) / sizeof (cap_list[0]))
> - cap_t caps = cap_init ();
> - if (caps == NULL)
> - return FAIL_ENOMEM;
> -
> - /* There is no reason why these should not work. */
> - cap_set_flag (caps, CAP_PERMITTED, ncap_list, cap_list, CAP_SET);
> - cap_set_flag (caps, CAP_EFFECTIVE, ncap_list, cap_list, CAP_SET);
> -
> - int res = cap_set_proc (caps);
> -
> - cap_free (caps);
> -
> - if (__glibc_unlikely (res != 0))
> - return FAIL_EXEC;
> - }
> -#endif
> -
> - /* Normal invocation of this program is with no arguments and
> - with privileges. */
> - return do_pt_chown ();
> - }
> -
> - /* We aren't going to be using privileges, so drop them right now. */
> - setuid (uid);
> -
> - /* Set locale via LC_ALL. */
> - setlocale (LC_ALL, "");
> -
> - /* Set the text message domain. */
> - textdomain (PACKAGE);
> -
> - /* parse and process arguments. */
> - argp_parse (&argp, argc, argv, 0, &remaining, NULL);
> -
> - if (remaining < argc)
> - {
> - /* We should not be called with any non-option parameters. */
> - error (0, 0, gettext ("too many arguments"));
> - argp_help (&argp, stdout, ARGP_HELP_SEE | ARGP_HELP_EXIT_ERR,
> - program_invocation_short_name);
> - return EXIT_FAILURE;
> - }
> -
> - /* Check if we are properly installed. */
> - if (euid != 0)
> - error (FAIL_EXEC, 0, gettext ("needs to be installed setuid `root'"));
> -
> - return EXIT_SUCCESS;
> -}
OK.
> diff --git a/manual/install.texi b/manual/install.texi
> index 2a87a2725a..49fcc7ac95 100644
> --- a/manual/install.texi
> +++ b/manual/install.texi
> @@ -209,20 +209,6 @@ additional security hardening because it enables full RELRO and a
> read-only global offset table (GOT), at the cost of slightly increased
> program load times.
>
> -@pindex pt_chown
> -@findex grantpt
> -@item --enable-pt_chown
> -The file @file{pt_chown} is a helper binary for @code{grantpt}
> -(@pxref{Allocation, Pseudo-Terminals}) that is installed setuid root to
> -fix up pseudo-terminal ownership. It is not built by default because
> -systems using the Linux kernel are commonly built with the @code{devpts}
> -filesystem enabled and mounted at @file{/dev/pts}, which manages
> -pseudo-terminal ownership automatically. By using
> -@samp{--enable-pt_chown}, you may build @file{pt_chown} and install it
> -setuid and owned by @code{root}. The use of @file{pt_chown} introduces
> -additional security risks to the system and you should enable it only if
> -you understand and accept those risks.
> -
OK.
> @item --disable-werror
> By default, @theglibc{} is built with @option{-Werror}. If you wish
> to build without this option (for example, if building with a newer
> @@ -442,13 +428,6 @@ may or may not want to run. @code{nscd} caches name service lookups; it
> can dramatically improve performance with NIS+, and may help with DNS as
> well.
>
> -One auxiliary program, @file{/usr/libexec/pt_chown}, is installed setuid
> -@code{root} if the @samp{--enable-pt_chown} configuration option is used.
> -This program is invoked by the @code{grantpt} function; it sets the
> -permissions on a pseudoterminal so it can be used by the calling process.
> -If you are using a Linux kernel with the @code{devpts} filesystem enabled
> -and mounted at @file{/dev/pts}, you don't need this program.
> -
OK.
> Afer installation you should configure the timezone and install locales
> for your system. The time zone configuration ensures that your system
> time matches the time for your current timezone. The locales ensure that
> diff --git a/manual/terminal.texi b/manual/terminal.texi
> index d830baacd7..7bb974e018 100644
> --- a/manual/terminal.texi
> +++ b/manual/terminal.texi
> @@ -1989,16 +1989,6 @@ This function is a GNU extension.
> @c getgrnam_r @mtslocale @ascudlopen @ascuplugin @ascuheap @asulock @acucorrupt @aculock @acsfd @acsmem
> @c getgid dup ok
> @c chmod dup ok
> -@c fork dup @aculock
> -@c [child]
> -@c setrlimit
> -@c dup2
> -@c CLOSE_ALL_FDS
> -@c execle
> -@c _exit
> -@c waitpid dup ok
> -@c WIFEXITED dup ok
> -@c WEXITSTATUS dup ok
OK. Thanks.
> @c free dup @ascuheap @acsmem
> The @code{grantpt} function changes the ownership and access permission
> of the slave pseudo-terminal device corresponding to the master
> diff --git a/sysdeps/generic/pty-private.h b/sysdeps/generic/pty-private.h
> index 1315c01af9..7470ca0c6a 100644
> --- a/sysdeps/generic/pty-private.h
> +++ b/sysdeps/generic/pty-private.h
> @@ -26,9 +26,6 @@
> /* The file descriptor connected to the master pseudo terminal. */
> #define PTY_FILENO 3
>
> -/* Path to the helper program that implements `grantpt' in user space. */
> -#define _PATH_PT_CHOWN LIBEXECDIR "/pt_chown"
> -
OK.
> /* Test whether given TTY is really a Unix98 pseudo terminal. */
> /* #define unix98_pseudo_p(Dev) ... */
>
> diff --git a/sysdeps/unix/grantpt.c b/sysdeps/unix/grantpt.c
> index e2e6df14c9..7143f54605 100644
> --- a/sysdeps/unix/grantpt.c
> +++ b/sysdeps/unix/grantpt.c
> @@ -99,7 +99,6 @@ pts_name (int fd, char **pts, size_t buf_len, struct stat64 *stp)
> int
> grantpt (int fd)
> {
> - int retval = -1;
OK.
> #ifdef PATH_MAX
> char _buf[PATH_MAX];
> #else
> @@ -132,7 +131,7 @@ grantpt (int fd)
> if (st.st_uid != uid)
> {
> if (__chown (buf, uid, st.st_gid) < 0)
> - goto helper;
> + goto cleanup;
OK.
> }
>
> static int tty_gid = -1;
> @@ -155,103 +154,21 @@ grantpt (int fd)
> }
> gid_t gid = tty_gid == -1 ? __getgid () : tty_gid;
>
> -#if HAVE_PT_CHOWN
> - /* Make sure the group of the device is that special group. */
> - if (st.st_gid != gid)
> - {
> - if (__chown (buf, uid, gid) < 0)
> - goto helper;
> - }
> -
> - /* Make sure the permission mode is set to readable and writable by
> - the owner, and writable by the group. */
> - mode_t mode = S_IRUSR|S_IWUSR|S_IWGRP;
> -#else
> - /* When built without pt_chown, we have delegated the creation of the
> - pty node with the right group and permission mode to the kernel, and
> - non-root users are unlikely to be able to change it. Therefore let's
> - consider that POSIX enforcement is the responsibility of the whole
> - system and not only the GNU libc. Thus accept different group or
> - permission mode. */
> -
OK.
> /* Make sure the permission is set to readable and writable by the
> owner. For security reasons, make it writable by the group only
> when originally writable and when the group of the device is that
> special group. */
> mode_t mode = S_IRUSR|S_IWUSR|
> ((st.st_gid == gid) ? (st.st_mode & S_IWGRP) : 0);
> -#endif
>
> + int retval = 0;
^^^^^^^^^^^^^^^^^^^^ Remove.
> if ((st.st_mode & ACCESSPERMS) != mode)
> {
> if (__chmod (buf, mode) < 0)
> - goto helper;
> + goto cleanup;
> }
>
> retval = 0;
^^^^^^^^^^^^^^^^^ Remove.
> - goto cleanup;
> -
> - /* We have to use the helper program if it is available. */
> - helper:;
> -
> -#if HAVE_PT_CHOWN
> - pid_t pid = __fork ();
> - if (pid == -1)
> - goto cleanup;
> - else if (pid == 0)
> - {
> - /* Disable core dumps. */
> - struct rlimit rl = { 0, 0 };
> - __setrlimit (RLIMIT_CORE, &rl);
> -
> - /* We pass the master pseudo terminal as file descriptor PTY_FILENO. */
> - if (fd != PTY_FILENO)
> - if (__dup2 (fd, PTY_FILENO) < 0)
> - _exit (FAIL_EBADF);
> -
> -# ifdef CLOSE_ALL_FDS
> - CLOSE_ALL_FDS ();
> -# endif
> -
> - execle (_PATH_PT_CHOWN, __basename (_PATH_PT_CHOWN), NULL, NULL);
> - _exit (FAIL_EXEC);
> - }
> - else
> - {
> - int w;
> -
> - if (__waitpid (pid, &w, 0) == -1)
> - goto cleanup;
> - if (!WIFEXITED (w))
> - __set_errno (ENOEXEC);
> - else
> - switch (WEXITSTATUS (w))
> - {
> - case 0:
> - retval = 0;
> - break;
> - case FAIL_EBADF:
> - __set_errno (EBADF);
> - break;
> - case FAIL_EINVAL:
> - __set_errno (EINVAL);
> - break;
> - case FAIL_EACCES:
> - __set_errno (EACCES);
> - break;
> - case FAIL_EXEC:
> - __set_errno (ENOEXEC);
> - break;
> - case FAIL_ENOMEM:
> - __set_errno (ENOMEM);
> - break;
> -
> - default:
> - assert(! "grantpt: internal error: invalid exit code from pt_chown");
> - }
> - }
> -#endif
> -
> cleanup:
> if (buf != _buf)
> free (buf);
This ends with 'return retval;'
Should we not just make that:
/* This can never fail because the OS kernel is responsible. */
return 0;
> diff --git a/sysdeps/unix/sysv/linux/grantpt.c b/sysdeps/unix/sysv/linux/grantpt.c
> deleted file mode 100644
> index 2030e07fa6..0000000000
> --- a/sysdeps/unix/sysv/linux/grantpt.c
> +++ /dev/null
> @@ -1,44 +0,0 @@
> -#include <assert.h>
> -#include <ctype.h>
> -#include <dirent.h>
> -#include <errno.h>
> -#include <fcntl.h>
> -#include <paths.h>
> -#include <stdlib.h>
> -#include <unistd.h>
> -
> -#include <not-cancel.h>
> -
> -#include "pty-private.h"
> -
> -#if HAVE_PT_CHOWN
> -/* Close all file descriptors except the one specified. */
> -static void
> -close_all_fds (void)
> -{
> - DIR *dir = __opendir ("/proc/self/fd");
> - if (dir != NULL)
> - {
> - struct dirent64 *d;
> - while ((d = __readdir64 (dir)) != NULL)
> - if (isdigit (d->d_name[0]))
> - {
> - char *endp;
> - long int fd = strtol (d->d_name, &endp, 10);
> - if (*endp == '\0' && fd != PTY_FILENO && fd != dirfd (dir))
> - __close_nocancel_nostatus (fd);
> - }
> -
> - __closedir (dir);
> -
> - int nullfd = __open_nocancel (_PATH_DEVNULL, O_RDONLY);
> - assert (nullfd == STDIN_FILENO);
> - nullfd = __open_nocancel (_PATH_DEVNULL, O_WRONLY);
> - assert (nullfd == STDOUT_FILENO);
> - __dup2 (STDOUT_FILENO, STDERR_FILENO);
> - }
> -}
> -# define CLOSE_ALL_FDS() close_all_fds()
> -#endif
> -
> -#include <sysdeps/unix/grantpt.c>
>
OK.
--
Cheers,
Carlos.