Hello all, It took a while, but i corrected the problems I hope.
On Wed, 2005-01-19 at 07:51 +0100, Peter Stuge wrote:
> On Tue, Jan 18, 2005 at 10:57:32PM +0200, Jos Houtman wrote:
> 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;
> 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]);
Done.
> 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.
Done, but execve required the env parameter to be terminated by a null
pointer, did I implement this in the right way.
char *env[3];
//cut, here env[0 ... 2] are setup with the USER/SHELL/HOME
//environment variables.
//execve requires the env array to be terminated by a null
//pointer
env[3] = '\0';
Something else I encountered:
I use getpwuid() to retrieve the username. But if there is no user
in /etc/passwd for that UID then the function generates a segmentation
fault. Is there a way to catch this?
And should the SHELL environment variable be set to the active shell or
the one listed in /etc/passwd in my case /bin/false. (the original
checkpassword sets it to the one in /etc/passwd).
> 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 found out that if I not call setgroups() it will keep the groups from
the previous user, in this case root. I now call setgroups() as
follows.
setgroups(0, &groupId);
which effectively unsets all supplementary groups.
> I would also suggest using setregid() and setreuid() instead of plain
> setgid() and setuid() in order to set the real gid and uid too.
Done.
somewhere in the near future I want to change it so I can check the
passwd without rebinding, but that looked like it needed some more time
then I have now.
With regards,
--
Jos Houtman <[EMAIL PROTECTED]>
chkpassword-ldap.tar
Description: Unix tar archive
