idvec-verify...

[Alfred M. Szmidt]

Ping on the following really old patch...  Marcus commented about it
back in 2003, and that it should be changed to check the error code
too.  I don't see a particular need for that, but it is easy to fix in
either case.

--- Old patch ---

Since there are quite a few bugs of the same nature here, it makes for
quite a long read.  So to ease for the reader, here is the list of
bugs that I have found so far (the gdb output is at the end
somewhere).  All of them segfault in either ugids-argp.c or
idvec-verify.c, and they do it for the same reason, i.e. the struct
they polute isn't checked if it is NULL.

A patch is applied at the end, with ChangeLog (search for the string
"*** Patch ***"), and it has been tested.  If someone could be nice
enough to comment about the ChangeLog entry then that would be nice,
since I don't really like it.

Oh, should we be nice enough and report some kind of a useful error
message in idvec-verify:verify_passwd()? Because right now it would do
this:
 $ addauth 123
 addauth: Authentication failure: Invalid argument

Note that this is the only the case for UID's, user/group names report
the sane message "Unknown user/group", and if the user has a valid
entry in /etc/passwd (and /etc/shadow depending on its password,
i.e. it is "x").

* `addauth' with a UID that doesn't exist (idvec-verify).
* `addauth' with a GID that doesn't exist (idvec-verify).
* `addauth' with a user name that doesn't exist in /etc/passwd and
  /etc/shadow (ugids-argp.c).
* `addauth' with a group name that doesn't exist in /etc/group
  (ugids-argp.c)
* `addauth' with a user name that doesn't exist in /etc/shadow, but
  exists in /etc/passwd (ugids-argp.c).

Running `addauth' with a UID that doesn't exist:
  Starting program: /bin/addauth 123

  Program received signal SIGSEGV, Segmentation fault.
  0x01025323 in verify_id (id=123, is_group=0, multiple=0, getpass_fn=0,
      getpass_hook=0x0, verify_fn=0x1026e40 <server_verify_make_auth>,
      verify_hook=0x1017aa0)
      at /src/hurd-2003-09-06/libshouldbeinlibc/idvec-verify.c:282

Running `addauth' with a GID that doesn't exist:
  Starting program: /bin/addauth -g 123

  Program received signal SIGSEGV, Segmentation fault.
  0x01025162 in verify_id (id=123, is_group=1, multiple=0, getpass_fn=0,
      getpass_hook=0x0, verify_fn=0x1026e40 <server_verify_make_auth>,
      verify_hook=0x1017aa0)
      at /src/hurd-2003-09-06/libshouldbeinlibc/idvec-verify.c:270

Running `addauth' with a user name that doesn't exist in /etc/passwd
and /etc/shadow:

  Starting program: /bin/addauth toor

  Program received signal SIGSEGV, Segmentation fault.
  0x01025ab4 in parse_opt (key=0, arg=0x101800d "toor", state=0x1017a6c)
      at /src/hurd-2003-09-06/libshouldbeinlibc/ugids-argp.c:88

Running `addauth' with a group name that doesn't exist in /etc/group:

  Starting program: /bin/addauth -g toor

  Program received signal SIGSEGV, Segmentation fault.
  0x01025a0a in parse_opt (key=103, arg=0x1018010 "toor", state=0x1017a6c)
      at /src/hurd-2003-09-06/libshouldbeinlibc/ugids-argp.c:126

Running `addauth' with a user name that doesn't exist in /etc/shadow,
but exists in /etc/passwd:

  Starting program: /bin/addauth fool

  Program received signal SIGSEGV, Segmentation fault.
  0x0102537f in verify_id (id=666, is_group=0, multiple=0, getpass_fn=0,
      getpass_hook=0x0, verify_fn=0x1026e40 <server_verify_make_auth>,
      verify_hook=0x1017aa0)
      at /src/hurd-2003-09-06/libshouldbeinlibc/idvec-verify.c:294

*** Patch ***
2003-09-15  Alfred M. Szmidt  <address@bogus.example.com>

        * idvec-verify.c (verify_passwd,verify_id): Check if the spwd and
        passwd structures are NULL, and return an error if so.
        (sys_encrypt): Check if NULL, return an error if so.

        * ugids-argp.c (parse_opt): Check if the group and passwd
        structures are NULL, and return an error if so.

Index: libshouldbeinlibc/idvec-verify.c
--- libshouldbeinlibc/idvec-verify.c
+++ libshouldbeinlibc/idvec-verify.c
@@ -99,17 +99,22 @@ verify_passwd (const char *password,
            {
              /* When encrypted password is "x", try shadow passwords. */
              struct spwd _sp, *sp;
-             if (getspnam_r (pw->pw_name, &_sp, sp_lookup_buf,
-                             sizeof sp_lookup_buf, &sp) == 0)
-               return sp->sp_pwdp;
+             getspnam_r (pw->pw_name, &_sp, sp_lookup_buf,
+                         sizeof sp_lookup_buf, &sp);
+             if (sp == NULL)
+               return NULL;
+             return sp->sp_pwdp;
            }
          return pw->pw_passwd;
        }

-      if (getpwuid_r (wheel_uid, &_pw, lookup_buf, sizeof lookup_buf, &pw))
-       return errno ?: EINVAL;
+      getpwuid_r (wheel_uid, &_pw, lookup_buf, sizeof lookup_buf, &pw);
+      if (pw == NULL)
+       return EINVAL;

       sys_encrypted = check_shadow (pw);
+      if (sys_encrypted == NULL)
+       return EINVAL;

       encrypted = crypt (password, sys_encrypted);
       if (! encrypted)
@@ -264,41 +269,41 @@ verify_id (uid_t id, int is_group, int m
        if (is_group)
          {
            struct group _gr, *gr;
-           if (getgrgid_r (id, &_gr, id_lookup_buf, sizeof id_lookup_buf, &gr)
-               == 0)
-             {
-               if (!gr->gr_passwd || !*gr->gr_passwd)
-                 return (*verify_fn) ("", id, 1, gr, verify_hook);
-               name = gr->gr_name;
-               pwd_or_grp = gr;
-             }
+           getgrgid_r (id, &_gr, id_lookup_buf, sizeof id_lookup_buf, &gr);
+           if (gr == NULL)
+             return EINVAL;
+           if (!gr->gr_passwd || !*gr->gr_passwd)
+             return (*verify_fn) ("", id, 1, gr, verify_hook);
+           name = gr->gr_name;
+           pwd_or_grp = gr;
          }
        else
          {
            struct passwd _pw, *pw;
-           if (getpwuid_r (id, &_pw, id_lookup_buf, sizeof id_lookup_buf, &pw)
-               == 0)
+           getpwuid_r (id, &_pw, id_lookup_buf, sizeof id_lookup_buf, &pw);
+           if (pw == NULL)
+             return EINVAL;
+           if (strcmp (pw->pw_passwd, SHADOW_PASSWORD_STRING) == 0)
              {
-               if (strcmp (pw->pw_passwd, SHADOW_PASSWORD_STRING) == 0)
-                 {
-                   /* When encrypted password is "x", check shadow
-                      passwords to see if there is an empty password. */
-                   struct spwd _sp, *sp;
-                   if (getspnam_r (pw->pw_name, &_sp, sp_lookup_buf,
-                                   sizeof sp_lookup_buf, &sp) == 0)
-                     /* The storage for the password string is in
-                        SP_LOOKUP_BUF, a local variable in this function.
-                        We Know that the only use of PW->pw_passwd will be
-                        in the VERIFY_FN call in this function, and that
-                        the pointer will not be stored past the call.  */
-                     pw->pw_passwd = sp->sp_pwdp;
-                 }
-
-               if (pw->pw_passwd[0] == '\0')
-                 return (*verify_fn) ("", id, 0, pw, verify_hook);
-               name = pw->pw_name;
-               pwd_or_grp = pw;
+               /* When encrypted password is "x", check shadow
+                  passwords to see if there is an empty password. */
+               struct spwd _sp, *sp;
+               getspnam_r (pw->pw_name, &_sp, sp_lookup_buf,
+                           sizeof sp_lookup_buf, &sp);
+               if (sp == NULL)
+                 return EINVAL;
+               /* The storage for the password string is in
+                  SP_LOOKUP_BUF, a local variable in this function.
+                  We Know that the only use of PW->pw_passwd will be
+                  in the VERIFY_FN call in this function, and that
+                  the pointer will not be stored past the call.  */
+               pw->pw_passwd = sp->sp_pwdp;
              }
+
+           if (pw->pw_passwd[0] == '\0')
+             return (*verify_fn) ("", id, 0, pw, verify_hook);
+           name = pw->pw_name;
+           pwd_or_grp = pw;
          }
        if (! name)
          {
Index: libshouldbeinlibc/ugids-argp.c
--- libshouldbeinlibc/ugids-argp.c
+++ libshouldbeinlibc/ugids-argp.c
@@ -83,14 +83,14 @@ 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;
-         else
+         getpwnam_r (arg, &_pw, id_lookup_buf, sizeof id_lookup_buf, &pw);
+         if (pw == NULL)
            {
              argp_failure (state, 10, 0, "%s: Unknown user", arg);
              return EINVAL;
            }
+         else
+           uid = pw->pw_uid;
        }

       if (key == ARGP_KEY_ARG || key == ARGP_KEY_END)
@@ -121,14 +121,14 @@ 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');
-         else
+         getgrnam_r (arg, &_gr, id_lookup_buf, sizeof id_lookup_buf, &gr);
+         if (gr == NULL)
            {
              argp_failure (state, 11, 0, "%s: Unknown group", arg);
              return EINVAL;
            }
+         else
+           return ugids_add_gid (ugids, gr->gr_gid, key == 'G');
        }

     default: