cvs commit: src/libexec/rexecd rexecd.c

[Jacques Vidrine]

On Apr 4, 2005, at 8:12 AM, Dag-Erling Smørgrav wrote:
> Jacques Vidrine <nectar at FreeBSD.org> writes:
>>   A separate bug was introduced at the same time.  The PAM library
>>   functions are called between the invocation of getpwnam(3) and the 
>> use
>>   of the returned static object.  Since many PAM library functions
>>   result in additional getpwnam(3) calls, the contents of the returned
>>   static object could be changed from under rexecd.  With this commit,
>>   getpwnam_r(3) is used instead.
>
> This is incorrect, because PAM may change the login name, so the
> struct passwd you got before calling PAM might not be the one you
> actually need.  The simplest fix is to revert this patch and instead
> add
>
>   pam_get_item(pamh, PAM_USER, &user);
>   pwd = getpwnam(user);
>
> after the PAM transaction.

Thanks, I understand what you are saying.  This bug existed prior to my 
commit, but it due to the bug that that commit fixes, it may have been 
masked in many situations.

The fix you suggest also will not work, since other interfaces are 
invoked between getpwnam() and the final usage of the result structure. 
  To be clear, we started with:

   pwd = getpwnam()
   if (pwd->pw_uid = 0)   <--- oops
   pam_authenticate etc   <--- underlying static struct passwd changes
   setlogin(pwd->pw_name) and other uses of static struct passwd
   pam_setcred etc        <--- possibly more changes to static data
   setuid(pwd->pw_uid) and other uses of static struct passwd

In -CURRENT we now have

   pwd = getpwnam_r()
   if (pwd->pw_uid == 0)
   pam_authenticate etc   <--- Now OK, we have our own struct passwd
   setlogin(pwd->pw_name) and other uses of local struct passwd
   pam_setcred etc        <--- Now OK, we have our own struct passwd
   setuid(pwd->pw_uid) and other uses of local struct passwd

You bring up that we need to lookup the user again after 
pam_authenticate.  So we will finally need:

   pwd = getpwnam_r()
   if (pwd->pw_uid == 0)
   pam_authenticate etc
   pwd = getpwnam_r(PAM_USER)  <--- user may have changed
   setlogin(pwd->pw_name)
   pam_setcred etc
   setuid(pwd->pw_uid)

Thanks for the pointer!  By the way, have you checked other PAM-using 
applications to make sure they do not make the same kinds of mistakes?

Cheers,
-- 
Jacques A Vidrine / NTT/Verio
nectar at celabo.org / jvidrine at verio.net / nectar at freebsd.org