On Thu, Mar 24, 2005 at 03:55:06PM +0300, Michael Tokarev wrote:
> Package: libpam-ssh
> Version: 1.91.0-5
> Severity: critical
>
> A long time ago (circa 1998 or so) I looked at pam-ssh project and
> noticied several problems with it. And since it's now in Debian,
> the same problems applies to Debian too.
>
> Here's one.
>
> in pam_sm_authenticate() routine, pam_ssh saves struct passwd as
> a pam variable, this way (error checking removed for simplicitly):
Are any other getpwnam()-type functions actually called, allowing the
buffers to be overwritten?
Documentation is not clear about how the fields of the struct are
allocated; a minimal test indicates that they are malloc()ated for the
first call only, but that implementation could change; I don't know if
there are relevant standards, and it may be that it is intentionally
"opaque".
It does seem like best-practice would be to copy the entire contents
of the structure, and not just the pointers.
> Luckly, most (depending on the other modules in the PAM stack) getpw*
> calls will be the same as this module does, and hence the problem
> will not occur.
I think you are implying that every libpam module shares memory space
with every other libpam module, which AIUI is incorrect. libpam are
ELF shared objects, and every process that links with such an object
(at runtime) will get its own private copy of the data section, but
the text section will be shared (mmaped, really, it is backed by
disk).
Does your claim still stand? Does *that* module call getpwnam()-type
functions multiple times, without memcpy the pointers, and then reuse
the top-level pointer?
> I pointed this problem out to the author the same time I looked at
> the module, but instead of an ACK he replied with something like
> "If you don't like my program write your own". Later on, he changed
> logic a bit -- previously he where saving the pwent pointer, now
> he saves the whole structure (as pwent_keep), but the same problem
> is still here.
Right; it is a static buffer, and multiple calls to getpwnam() return
the same pointer, *and* the pointer structure fields always point to
the same place.
It seems that your request can be easily satisfied by using the
reentrant versions of these functions, like getpwnam_r. I'm including
a test file I've been playing with, which indicates that a patch, if
necessary, would be unintrusive.
Justin
#include <unistd.h>
#include <pwd.h>
#include <sys/types.h>
int main()
{
struct passwd p,q;
char *buf,*buf2;
int buflen=sysconf(_SC_GETPW_R_SIZE_MAX);
buf=(char *)malloc(buflen);
buf2=(char *)malloc(buflen);
struct passwd *v=malloc(sizeof (void *));
//struct passwd *p=getpwnam("pryzbyj");
getpwnam_r("pryzbyj", &p, buf, buflen, (struct passwd **)&v);
//struct passwd *q=getpwnam("root");
getpwnam_r("root", &q, buf2, buflen, (struct passwd **)&v);
printf("%s\n", p.pw_name);
printf("%s\n", q.pw_name);
return 0;
}
