This is the mail archive of the glibc-bugs@sourceware.org mailing list for the glibc project.


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]
Other format: [Raw text]

[Bug libc/19347] grantpt: try to force a specific gid even without pt_chown


https://sourceware.org/bugzilla/show_bug.cgi?id=19347

Aurelien Jarno <aurelien at aurel32 dot net> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
             Status|NEW                         |RESOLVED
         Resolution|---                         |FIXED

--- Comment #1 from Aurelien Jarno <aurelien at aurel32 dot net> ---
Fixed in master:

commit 77356912e83601fd0240d22fe4d960348b82b5c3
Author: Aurelien Jarno <aurelien@aurel32.net>
Date:   Thu Dec 10 22:33:10 2015 +0100

    grantpt: trust the kernel about pty group and permission mode

    According to POSIX the grantpt() function does the following:

      The grantpt() function shall change the mode and ownership of the
      slave pseudo-terminal device associated with its master
      pseudo-terminal counterpart. The fildes argument is a file descriptor
      that refers to a master pseudo-terminal device. The user ID of the
      slave shall be set to the real UID of the calling process and the
      group ID shall be set to an unspecified group ID. The permission
      mode of the slave pseudo-terminal shall be set to readable and
      writable by the owner, and writable by the group.

    Historically the GNU libc has been responsible to setup the permission
    mode to 0620 and the group to 'tty' usually number 5, using the pt_chown
    helper, badly known for its security issues. With the creation of the
    devpts filesytem in the Linux kernel, this responsibility has been moved
    to the Linux kernel. The system is responsible to mount the devpts
    filesystem in /dev/pts with the options gid=5 and mode=0620. In that
    case the GNU libc has nothing to do and pt_chown is not need anymore. So
    far so good.

    The problem is that by default the devpts filesystem is shared between
    all mounts, and that contrary to other filesystem, the mount options are
    honored at the second mount, including for the default mount options.
    Given it corresponds to mode=0600 without gid parameter (that is the
    filesystem GID of the creating process), it's common to see systems
    where the devpts filesystem is mounted using these options. It is enough
    to run a "mount -t devpts devpts /mychroot/dev/pts" to come into this
    situation, and it's unfortunately wrongly used in a lot of scripts
    dealing with chroots, or for creating virtual machines images.

    When this happens the GNU libc tries to fix the group and permission
    mode of the pty nodes, and given it fails to do so for non-root users,
    grantpt() almost always fail. It means users are not able to open new
    terminals.

    This patch changes grantpt() to not enforce this anymore, while still
    enforcing minimum security measures to the permission mode. Therefore
    the responsibility to follow POSIX is now shared at the system level,
    i.e. kernel + system scripts + GNU libc. It stops trying to change the
    group, and makes the pty node readable and writable by the owner, and
    writable by the group only when originally writable and when the group
    is the tty one.

    As a result, on a system wrongly mounted with gid=0 and mode=0600, the
    pty nodes won't be accessible by the tty group, but the grantpt()
    function will succeed and users will have a working system. The system
    is not fully POSIX compliant (which might be an admin choice to default
    to "mesg n" mode), but the GNU libc is not to blame here, as without the
    pt_chown helper it can't do anything.

    With this patch there should not be any reason left to build the GNU
    libc with the --enable-pt_chown configure option on a GNU/Linux system.

diff --git a/ChangeLog b/ChangeLog
index 81e5791..80f4635 100644
--- a/ChangeLog
+++ b/ChangeLog
@@ -1,3 +1,10 @@
+2015-12-10  Aurelien Jarno  <aurelien@aurel32.net>
+           Jakub Wilk  <jwilk@debian.org>
+
+       [BZ #19347]
+       * sysdeps/unix/grantpt.c [!HAVE_PT_CHOWN] (grantpt): Do not try
+       to change the group of the device to the tty group.
+
 2015-12-10  Paul Eggert  <eggert@cs.ucla.edu>

        Split large string section; add truncation advice
diff --git a/sysdeps/unix/grantpt.c b/sysdeps/unix/grantpt.c
index f019b9d..ad5996d 100644
--- a/sysdeps/unix/grantpt.c
+++ b/sysdeps/unix/grantpt.c
@@ -155,6 +155,7 @@ 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)
     {
@@ -164,9 +165,26 @@ grantpt (int fd)

   /* Make sure the permission mode is set to readable and writable by
      the owner, and writable by the group.  */
-  if ((st.st_mode & ACCESSPERMS) != (S_IRUSR|S_IWUSR|S_IWGRP))
+  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.  */
+
+  /* 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
+
+  if ((st.st_mode & ACCESSPERMS) != mode)
     {
-      if (__chmod (buf, S_IRUSR|S_IWUSR|S_IWGRP) < 0)
+      if (__chmod (buf, mode) < 0)
        goto helper;
     }

-- 
You are receiving this mail because:
You are on the CC list for the bug.

Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]