[address@hidden: Re: bugs in idvec-verify.c, almost...]

[Alfred M. Szmidt]

Ping pong...  
------- Start of forwarded message -------
Date: Tue, 16 Sep 2003 15:06:18 +0200 (MEST)
From: "Alfred M. Szmidt" <ams@kemisten.nu>
To: "Alfred M. Szmidt" <ams@kemisten.nu>
CC: bug-hurd@gnu.org
Subject: Re: bugs in idvec-verify.c, almost...

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  <ams@kemisten.nu>

        * 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:
------- End of forwarded message -------