[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
idvec-verify...
From: |
Alfred M. Szmidt |
Subject: |
idvec-verify... |
Date: |
Fri, 22 Oct 2004 22:31:28 +0200 |
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:
- idvec-verify...,
Alfred M. Szmidt <=