On Tue, Jan 18, 2005 at 10:57:32PM +0200, Jos Houtman wrote:
> hello all,
Hi Jos,
> This is also the first C program I write, so tips,comments, flames for
> obvious errors/faults are appreciated.
I looked the code over and did fine one major thing that may not be
so good. In the part where you get the UID and GID from the entry;
values = ldap_get_values(ld, entry, LDAP_UID_FIELD);
if (values && values[0])
{
userId = atoi(values[0]);
}
values = ldap_get_values(ld, entry, LDAP_GID_FIELD);
if (values && values[0])
{
groupId = atoi(values[0]);
}
if something fails here, either ldap_get_values() fails, or values[0]
is blank, userId and groupId will be left uninitialized, which is not
really good, since the program later assumes they are valid.
Uninitialized variables are initialized to 0 by the compiler, so if I
can manage to somehow make those LDAP calls fail, I will have root
access on your system via the network. Not good.
Make sure you initialize the variables yourself, or even better;
values = ldap_get_values(ld, entry, LDAP_UID_FIELD);
if (!values)
return 111;
if (!values[0])
return 111;
userId = atoi(values[0]);
values = ldap_get_values(ld, entry, LDAP_GID_FIELD);
if (!values)
return 111;
if (!values[0])
return 111;
groupId = atoi(values[0]);
A few other comments;
No environment variables are set up. The original checkpassword
program does so, and to be compatible with that I would suggest that
any new checkpassword program does so too. Specifically the variables
are USER, HOME and SHELL. Binc may not use any of them, but other
applications may.
The setgroups() call.. It only sets one supplementary group, which may
or may not be what's in the LDAP directory. Ideally a list of all
supplementary groups should be made by looking it up in the directory
and that list used with setgroups().
I would also suggest using setregid() and setreuid() instead of plain
setgid() and setuid() in order to set the real gid and uid too.
//Peter
