Re: pt_chown

[Zack Weinberg <zack@rabi.columbia.edu>]

On Wed, 31 Mar 1999 10:34:16 +1000, Geoff Keating wrote:
>> Date: Mon, 29 Mar 1999 10:30:10 -0500
>> From: Mark Kettenis <kettenis@gnu.org>
>> 
>> PR 1046 reports a security problem in pt_chown (for which I'll send a
>> patch later today).  The author also questions the use of argument
>> parsing and localisation in the program.  I tend to disagree with him
>> since the argp and gettext code should be safe to use in a setuid
>> program (actually using it in pt_chown might help us catching bugs).
>> But since I wrote big parts of the code I may not be very impartial in
>> this matter.  Does any of us have an opinion on the issue.
>
>su(1) uses gettext and getopt_long from glibc.  Certainly, gettext
>must be safe in setuid programs.  I don't see why argp should be
>considered to be less safe than getopt_long, and I'd expect it's
>already used in privileged code.

pt_chown also uses nsswitch, via getgrnam.  I'd be a lot more worried
about that, nsswitch seems to me more likely to have bugs.

That said, here's a patch that makes pt_chown drop privileges before
calling argp or gettext; normal execution doesn't need either.  It
also fixes the real security problem reported in the PR (race due to
closing the master pty).

- Do we have user side support for the capability stuff in 2.[12] yet?

zw

1999-03-30 20:59 -0500  Zack Weinberg  <zack@rabi.phys.columbia.edu>

	* login/programs/pt_chown.c: Drop privileges if invoked with
	arguments.  Don't close the master pty.

===================================================================
Index: login/programs/pt_chown.c
--- login/programs/pt_chown.c	1999/01/12 00:18:37	1.4
+++ login/programs/pt_chown.c	1999/03/31 01:54:11
@@ -55,7 +55,7 @@
 static void
 print_version (FILE *stream, struct argp_state *state)
 {
-  fprintf (stream, "pt_chmod (GNU %s) %s\n", PACKAGE, VERSION);
+  fprintf (stream, "pt_chown (GNU %s) %s\n", PACKAGE, 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\
@@ -95,41 +95,17 @@
 }
 
 int
-main (int argc, char *argv[])
+do_pt_chown (void)
 {
   char *pty;
-  int remaining;
   struct stat st;
   struct group *p;
   gid_t gid;
 
-  /* 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);
-      exit (EXIT_FAILURE);
-    }
-
-  /* Check if we are properly installed.  */
-  if (geteuid () != 0)
-    error (FAIL_EXEC, 0, gettext ("needs to be installed setuid `root'"));
-
   /* Check that PTY_FILENO is a valid master pseudo terminal.  */
   pty = ptsname (PTY_FILENO);
   if (pty == NULL)
     return errno == EBADF ? FAIL_EBADF : FAIL_EINVAL;
-  close (PTY_FILENO);
 
   /* Check that the returned slave pseudo terminal is a
      character device.  */
@@ -149,6 +125,46 @@
      and writable by the group.  */
   if (chmod (pty, S_IRUSR|S_IWUSR|S_IWGRP) < 0)
     return FAIL_EACCES;
+
+  return 0;
+}
+
+
+int
+main (int argc, char *argv[])
+{
+  int remaining;
+
+  /* Normal invocation of this program is with no arguments and
+     with privileges.
+     FIXME: Should use capable (CAP_CHOWN|CAP_FOWNER).  */
+  if (argc == 1 && geteuid () == 0)
+    return do_pt_chown ();
+
+  /* We aren't going to be using privileges, so drop them right now. */
+  setuid (getuid ());
+  
+  /* 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 (geteuid () != 0)
+    error (FAIL_EXEC, 0, gettext ("needs to be installed setuid `root'"));
 
-  exit (EXIT_SUCCESS);
+  return EXIT_SUCCESS;
 }