Re: [bug-inetutils] [PATCH] rexecd possible security problem

[Giuseppe Scrivano]

"Alfred M. Szmidt" <address@hidden> writes:

> Please use error over fprintf/exit.  Though I see that rexecd
> redefines error to something perverse...  Would you like to fix that?

I used fprintf/exit just to be coherent with the rest of rexecd.

I amended my previous patch and merged the second one.

Cheers,
Giuseppe

>From dc1585b806d360ce684509ec62d8d7eb4b065024 Mon Sep 17 00:00:00 2001
From: Giuseppe Scrivano <address@hidden>
Date: Thu, 9 Jul 2009 11:35:43 +0200
Subject: [PATCH] rexecd: check for errors of setegid/setuid/setgid/initgroups

2009-07-10  Giuseppe Scrivano <address@hidden>

        * rexecd/rexecd.c (doit): Add return value check after
        use setegid/setuid/setgid/initgroups and define its
        prototype.
        * rexecd/rexecd.c (error): Use the <error.h> version
        in place of it.
---
 rexecd/rexecd.c |   84 +++++++++++++++++++-----------------------------------
 1 files changed, 30 insertions(+), 54 deletions(-)

diff --git a/rexecd/rexecd.c b/rexecd/rexecd.c
index be8511b..5832382 100644
--- a/rexecd/rexecd.c
+++ b/rexecd/rexecd.c
@@ -27,7 +27,7 @@
  * SUCH DAMAGE.
  */
 
-/* Copyright (C) 2000, 2001, 2002, 2003, 2004, 2005, 2006, 2007, 2008
+/* Copyright (C) 2000, 2001, 2002, 2003, 2004, 2005, 2006, 2007, 2008, 2009
    Free Software Foundation, Inc.
 
    This file is part of GNU Inetutils.
@@ -71,6 +71,7 @@
 
 #include <netinet/in.h>
 
+#include <error.h>
 #include <errno.h>
 #include <netdb.h>
 #include <pwd.h>
@@ -97,8 +98,8 @@
 #endif
 #include <progname.h>
 
-void error (const char *fmt, ...);
 void usage (void);
+int doit (int, struct sockaddr_in *);
 
 static const char *short_options = "hV";
 static struct option long_options[] = {
@@ -141,10 +142,8 @@ main (int argc, char **argv)
 
   fromlen = sizeof (from);
   if (getpeername (sockfd, (struct sockaddr *) &from, &fromlen) < 0)
-    {
-      fprintf (stderr, "rexecd: getpeername: %s\n", strerror (errno));
-      exit (1);
-    }
+    error (EXIT_FAILURE, errno, "cannot retrieve peer address");
+
   doit (sockfd, &from);
   exit (0);
 }
@@ -243,20 +242,14 @@ doit (int f, struct sockaddr_in *fromp)
   setpwent ();
   pwd = getpwnam (user);
   if (pwd == NULL)
-    {
-      error ("Login incorrect.\n");
-      exit (1);
-    }
+    error (EXIT_FAILURE, 0, "login incorrect");
   endpwent ();
   pw_password = get_user_password (pwd);
   if (*pw_password != '\0')
     {
       namep = crypt (pass, pw_password);
       if (strcmp (namep, pw_password))
-       {
-         error ("Password incorrect.\n");
-         exit (1);
-       }
+        error (EXIT_FAILURE, 0, "password incorrect");
     }
   write (STDERR_FILENO, "\0", 1);
   if (port)
@@ -264,10 +257,8 @@ doit (int f, struct sockaddr_in *fromp)
       pipe (pv);
       pid = fork ();
       if (pid == -1)
-       {
-         error ("Try again.\n");
-         exit (1);
-       }
+        error (EXIT_FAILURE, errno, "cannot fork the process");
+
       if (pid)
        {
          close (STDIN_FILENO);
@@ -320,17 +311,24 @@ doit (int f, struct sockaddr_in *fromp)
     pwd->pw_shell = PATH_BSHELL;
   if (f > 2)
     close (f);
-  setegid ((gid_t) pwd->pw_gid);
-  setgid ((gid_t) pwd->pw_gid);
+
+  if (setegid ((gid_t) pwd->pw_gid) < 0)
+    error (EXIT_FAILURE, errno, "failed to set additional groups");
+
+  if (setgid ((gid_t) pwd->pw_gid) < 0)
+    error (EXIT_FAILURE, errno, "failed to set group-ID");
+
 #ifdef HAVE_INITGROUPS
-  initgroups (pwd->pw_name, pwd->pw_gid);
+  if (initgroups (pwd->pw_name, pwd->pw_gid) < 0)
+    error (EXIT_FAILURE, errno, "failed to initialize the supplementary group "
+           "access list");
 #endif
-  setuid ((uid_t) pwd->pw_uid);
+  if (setuid ((uid_t) pwd->pw_uid) < 0)
+    error (EXIT_FAILURE, errno, "failed to set user-ID");
+
   if (chdir (pwd->pw_dir) < 0)
-    {
-      error ("No remote directory.\n");
-      exit (1);
-    }
+    error (EXIT_FAILURE, errno, "failed to change home directory");
+
   strcat (path, PATH_DEFPATH);
   environ = envinit;
   strncat (homedir, pwd->pw_dir, sizeof (homedir) - 6);
@@ -342,23 +340,7 @@ doit (int f, struct sockaddr_in *fromp)
   else
     cp = pwd->pw_shell;
   execl (pwd->pw_shell, cp, "-c", cmdbuf, 0);
-  perror (pwd->pw_shell);
-  exit (1);
-}
-
-void
-error (const char *fmt, ...)
-{
-  va_list ap;
-  char buf[BUFSIZ];
-#if defined(HAVE_STDARG_H) && defined(__STDC__) && __STDC__
-  va_start (ap, fmt);
-#else
-  va_start (ap);
-#endif
-  buf[0] = 1;
-  snprintf (buf + 1, sizeof buf - 1, fmt, ap);
-  write (STDERR_FILENO, buf, strlen (buf));
+  error (EXIT_FAILURE, errno, "cannot execl %s", pwd->pw_shell);
 }
 
 char *
@@ -368,10 +350,7 @@ getstr (const char *err)
   char *buf = malloc (buf_len), *end = buf;
 
   if (!buf)
-    {
-      error ("Out of space reading %s\n", err);
-      exit (1);
-    }
+    error (EXIT_FAILURE, errno, "out of space reading %s", err);
 
   do
     {
@@ -380,10 +359,9 @@ getstr (const char *err)
       if (rd <= 0)
        {
          if (rd == 0)
-           error ("EOF reading %s\n", err);
+           error (EXIT_FAILURE, errno, "EOF reading");
          else
-           perror (err);
-         exit (1);
+           error (EXIT_FAILURE, errno, "error reading");
        }
 
       end += rd;
@@ -394,10 +372,8 @@ getstr (const char *err)
          buf_len += buf_len;
          buf = realloc (buf, buf_len);
          if (!buf)
-           {
-             error ("Out of space reading %s\n", err);
-             exit (1);
-           }
+           error (EXIT_FAILURE, errno, "out of space reading %", err);
+
          end = buf + end_offs;
        }
     }
-- 
1.6.3.1