bugs in idvec-verify.c, almost...

[Alfred M. Szmidt]

Hi,

there are two or possibly three small bugs in idvec-verify.c that
use--or abuse--the functions getpwuid_r() and getspnam_r().  It mostly
concerns the fact that they don't return (nor does any other NSS
related function AFAIK) anything useful, that is they always return
0), nor do they set errno.  So this could apply to _any_ function that
is based on the getXXbyYY_r.c (same thing for getXXbyYY.c?) file in
libc, I haven't checked this yet--and I don't want to since that code
is quite similar to /dev/random.  I also want to double check the code
in idvec-verify.c is correct (or wrong) before moving this to
libc-alpha, since it could apply to any system that uses the GNU C
Library.


For example, in idvec-verify.c (around line 280), we have the
following,

 if (getpwuid_r (id, &_pw, id_lookup_buf, sizeof id_lookup_buf, &pw)
     == 0)
   {
     if (strcmp (pw->pw_passwd, SHADOW_PASSWORD_STRING) == 0)

Since getpwuid_r() doesn't set a meaningful return value or errno, the
above will always be true.  So lets assume that the user ID does not
exist, since getpwuid_r() doesn't set anything meaningful to return,
we will pass a NULL pointer to strcmp().  Which will in turn result in
a segfault.  Simply running gdb on "addauth 123", where 123 does not
exists should reproduce the above.


The second problem is quite similar, lets assume that the user 123
exists, but doesn't have an shadow entry (i.e. the user is listed in
/etc/passwd, but not in /etc/shadow), and her password is `x'.  What
happens is this (still the same bit of code as above, but we can
follow it a bit further),

 if (getpwuid_r (id, &_pw, id_lookup_buf, sizeof id_lookup_buf, &pw)
     == 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;
       }

Since we assumed that the user 123 exists, and its password is `x',
the strcmp() test case will succeed (obviously).  So we call
getspnam_r() with a semi-valid PW->PW_NAME (the user exists after
all).  But since it doesn't have an shadow password entry,
getspnam_r() will _not_ populate (it acts just like getpwuid_r(), not
returning anything of use, or setting errno) structure (it will be
NULL to be exact).  So we get something like, PW->PW_PASSWD =
NULL->NULL, which as we all know will cause a segfault.  A simple way
to produce this is to create a user in /etc/passwd, but not creating
it in /etc/shadow, and then running "addauth I-AM-HERE-BUT-NOT-THERE".


There might be one more bug concerning the usage of getpwuid_r() in
idvec-verify.c, around line 109.  I.e. we have the following:

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

 sys_encrypted = check_shadow (pw);

Say we have an invalid wheel_uid (the UID is listed in the group file,
but the UID doesn't exist).  So we call getpwuid_r(), it doesn't set
errno as we expect it to, so we have PW that is NULL.  We now call
check_shadow() (which is defined a few lines above), so around line
102 we could get an segfault, since we are doing something along the
lines, getspnam_r (NULL->NULL, ...), and we know what happens in those
cases right?


Now, does anyone got any corking ideas on what can be done?  We could
add checks after/before each function to check if the structure that
they populate is NULL, and act based on that.  Or we can fix
getXXbyYY.c to either set errno, and/or return something that is
useful.  I don't like the last option, since I just don't want to
touch getXXbyYY.c.

Cheer and cherish cheesy cheerleaders that cheeringly cheered the
cheap cheeryblossom.