On Wed, Jan 26, 2005 at 06:32:30PM +0200, Jos Houtman wrote:
> > Make sure you initialize the variables yourself, or even better;
>
> Done.
Good. This was the most important problem.
> > 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';
Not quite.
When defining an array (char *env[3]) the number in braces isn't the
highest allowed index, but rather the number of allocated elements.
This means that for env in this case, there can only be three
elements, (env[0]..env[2]) and env[3] will be outside the allocated
space for the variable. Some time later this program might crash
because of the line env[3]='\0'; since it is writing to memory that
is right after the env variable, possibly clobbering something you
later add to the code.
Also, how exactly are the env variables "setup" ? String and memory
handling in C is difficult to get right in the beginning, if you're
not careful you can introduce security holes very easily.
If you looked at my bchkpw you noticed that I took a shortcut and let
libc handle it all, by using the setenv() call. Maximum portability
will require execve() or possibly putenv() though, and they both
require you to set up the complete NAME=value string on your own.
> 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?
Are you sure the function generates the segmentation fault?
The man page getpwnam(3) says that getpwuid() returns NULL for uids
that do not exist in /etc/passwd, check and make sure that you aren't
using the return value without first checking if the call was
successful or not. Ie.
struct passwd *pw;
errno=0;
pw=getpwuid(theLDAPuid);
if(NULL==pw) {
if(errno!=0)
perror("getpwuid");
return 111;
}
/* pw is valid here */
> 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 one from /etc/passwd, ie. contents of pw->pw_shell
> > The setgroups() call..
>
> 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.
Good start. If you don't plan on using shared folders using groups on
the unix system this will do just fine.
> > 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.
This was also an important fix.
> 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.
I'm not very familiar with LDAP, why is the rebind neccessary right
now? Hm, maybe it's using an unpriviliged bind for finding the user,
and then tries to rebind using supplied user privileges to gain
access to the encrypted password?
You're making good progress anyway! :)
//Peter
