diff mbox

grantpt: trust the kernel about pty group and permission mode

Message ID 1449414023-28926-1-git-send-email-aurelien@aurel32.net
State New
Headers show

Commit Message

Aurelien Jarno Dec. 6, 2015, 3 p.m. UTC 
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 responsability 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, including the mount options. The options used to do the last
mount are the ones used by the system, and given the default options are
gid=0 and mode=0600, it's common to see systems 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.
---
 ChangeLog              |  6 ++++++
 sysdeps/unix/grantpt.c | 22 ++++++++++++++++++++--
 2 files changed, 26 insertions(+), 2 deletions(-)

The original idea comes from Jakub Wilk. I was originally a bit
skeptical about it, but after thinking a bit more about it, I don't
see any security consequences. Review by other eyes would therefore
be greatly appreciated.

The goal of this patch is to fully get rid of pt_chown, without
breaking the user experience.

Comments

Florian Weimer Dec. 6, 2015, 8:28 p.m. UTC | #1 
On 12/06/2015 04:00 PM, Aurelien Jarno wrote:
> +2015-12-06  Aurelien Jarno  <aurelien@aurel32.net>
> +	    Jakub Wilk  <jwilk@debian.org>
> +
> +	* sysdeps/unix/grantpt.c (grantpt): Do not try to change the group
> +	of the device to the tty group when built without pt_chown support.

Thanks for posting this.  It looks good to me.  I have tried it with a
default-settings devpts, and screen works (when it would fail before).

Can you file a bug and reference it in the commit and ChangeLog, for
tracking purposes?  I don't think a separate NEWS entry is needed, but
the bug might be helpful in the future.

Florian
Jakub Wilk Dec. 6, 2015, 9:39 p.m. UTC | #2 
* Aurelien Jarno <aurelien@aurel32.net>, 2015-12-06, 16:00:
>The options used to do the last mount are the ones used by the system, 
>and given the default options are gid=0 and mode=0600,

Minor correction: gid=0 is not the default.
By default, group of a newly created pseudo-terminal is set to 
filesystem GID of the creating process.

>+     Therefore let's consider that POSIX enforcement is the responsability

Typo: responsability -> responsibility
diff mbox

Patch

diff --git a/ChangeLog b/ChangeLog
index 379ba75..1bc473c 100644
--- a/ChangeLog
+++ b/ChangeLog
@@ -1,3 +1,9 @@ 
+2015-12-06  Aurelien Jarno  <aurelien@aurel32.net>
+	    Jakub Wilk  <jwilk@debian.org>
+
+	* sysdeps/unix/grantpt.c (grantpt): Do not try to change the group
+	of the device to the tty group when built without pt_chown support.
+
 2015-12-04  Paul Eggert  <eggert@cs.ucla.edu>
 
 	Fix typo in strncat, wcsncat manual entries
diff --git a/sysdeps/unix/grantpt.c b/sysdeps/unix/grantpt.c
index c04c85d..33352e3 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
+     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 responsability
+     of the whole system and not only the GNU libc and accept different
+     group or permission mode.  */
+
+  /* Make sure the permission is set to readable and writable by the
+     owner.  For security reason, 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;
     }