Re: setauth segfaults when giving an inexistent username

[Marc-Olivier Mercier]

Marc-Olivier Mercier wrote:
> Hi all,
>
> I tried to used setauth and it segfaults when I passed a wrong user 
> name (or even a wrong group name). The problem is when the args are 
> parsed. In libshouldbeinlibc/ugids-argp.c, the function getpwnam_r is 
> used. It is assumed that getpwnam_r return non-zero if the user does 
> not exist, which is false. There's nothing in the POSIX spec that 
> tells that user name that doesn't exist should return an error. In 
> fact, this is considered as a success. The only way to know for sure 
> that a user doesn't exist is by looking if pwbufp is NULL. The same 
> thing applies to the group and the getgrnam_r function.

This affects all programs that use ugids_argp. This includes : setauth, 
addauth, rmauth and login. Here's another version of the patch. It now 
handles when getpwnam_r and getgrnam_r return non-zero.

Without the patch :
$ setauth doesnotexist
(after a few seconds..)
Segmentation fault

With this patch, if the user does not exist, there's a message printed 
on the screen :
$ setauth doesnotexist
setauth: Unknown user: doesnotexist

And if getpwnam_r does not return 0, another message is printed (I was 
able to create such condition by supplying a buffer with insufficient space)
$ setauth anyuser
setauth: Could not get uid for user: anyuser: Numerical result out of range

(the message can look strange, but getpwnam_r returns ERANGE when 
insufficient buffer space is supplied)

-- Marc. O

Index: ChangeLog
===================================================================
RCS file: /sources/hurd/hurd/libshouldbeinlibc/ChangeLog,v
retrieving revision 1.88
diff -u -p -r1.88 ChangeLog
--- ChangeLog    16 Mar 2008 17:35:03 -0000    1.88
+++ ChangeLog    20 Mar 2008 01:02:27 -0000
@@ -1,3 +1,8 @@
+2008-03-19  Marc-Olivier Mercier  <mercier.m@sympatico.ca>
+
+    * ugids-argp.c (parse_opt): Check null condition for struct passwd (or
+    group) pointer returned by getpwnam_r and getgrnam_r.
+
2008-03-16  Samuel Thibault  <samuel.thibault@ens-lyon.org>

    * idvec-verify.c (verify_id): Compare id to (uid_t) -1 instead of
Index: ugids-argp.c
===================================================================
RCS file: /sources/hurd/hurd/libshouldbeinlibc/ugids-argp.c,v
retrieving revision 1.2
diff -u -p -r1.2 ugids-argp.c
--- ugids-argp.c    11 Jul 1999 06:02:55 -0000    1.2
+++ ugids-argp.c    20 Mar 2008 01:02:27 -0000
@@ -83,20 +83,32 @@ parse_opt (int key, char *arg, struct ar
      else
    {
      struct passwd _pw, *pw;
-      if (getpwnam_r (arg, &_pw, id_lookup_buf, sizeof id_lookup_buf, &pw)
-          == 0)
-        uid = pw->pw_uid;
+      int err;
+      err = getpwnam_r (arg, &_pw, id_lookup_buf,
+                sizeof id_lookup_buf, &pw);
+      if (err == 0)
+        {
+          /* Even if the username does not exist, getpwnam_r succeded */
+          if (pw == NULL)
+        {
+          argp_failure (state, 10, 0, "Unknown user: %s", arg);
+          return EINVAL;
+        }
+
+          uid = pw->pw_uid;
+        }
      else
        {
-          argp_failure (state, 10, 0, "%s: Unknown user", arg);
-          return EINVAL;
+          argp_failure (state, 12, err,
+                "Could not get uid for user: %s", arg);
+          return err;
        }
    }

      if (key == ARGP_KEY_ARG || key == ARGP_KEY_END)
    {
-      /* A user arg, which means add the user, and any appropriate
-         groups. */
+      /* A user arg, which means add the user,
+         and any appropriate groups. */
      if (!params->user_args_are_effective
          && !params->user_args_are_available)
        return ugids_set_posix_user (ugids, uid);
@@ -121,13 +133,23 @@ parse_opt (int key, char *arg, struct ar
      else
    {
      struct group _gr, *gr;
-      if (getgrnam_r (arg, &_gr, id_lookup_buf, sizeof id_lookup_buf, &gr)
-          == 0)
-        return ugids_add_gid (ugids, gr->gr_gid, key == 'G');
+      int err = getgrnam_r (arg, &_gr, id_lookup_buf,
+                sizeof id_lookup_buf, &gr);
+      if (err == 0)
+        {
+          if (gr == NULL)
+        {
+          argp_failure (state, 11, 0, "Unknown group: %s", arg);
+          return EINVAL;
+        }
+
+          return ugids_add_gid (ugids, gr->gr_gid, key == 'G');
+        }
      else
        {
-          argp_failure (state, 11, 0, "%s: Unknown group", arg);
-          return EINVAL;
+          argp_failure (state, 13, err,
+                "Could not get gid for group: %s", arg);
+          return err;
        }
    }