Ping pong...
------- Start of forwarded message -------
Date: Tue, 16 Sep 2003 15:06:18 +0200 (MEST)
From: "Alfred M. Szmidt" <[EMAIL PROTECTED]>
To: "Alfred M. Szmidt" <[EMAIL PROTECTED]>
CC: [EMAIL PROTECTED]
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 <[EMAIL PROTECTED]>
* 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 -------
_______________________________________________
Bug-hurd mailing list
[EMAIL PROTECTED]
http://mail.gnu.org/mailman/listinfo/bug-hurd
