Re: pam_chroot-0.8 released

[Solar Designer <solar openwall com>]

On Wed, May 05, 2004 at 12:28:34PM +1000, John Newbigin wrote:
> Here is a more complete check procedure.

Yes, a check of all path components is preferable.

> I have used this code so I hope it is secure :)

Unfortunately, no.

>                         if(buf.st_uid != 0)
>                         {
>                                 // make sure there is no user write access
>                                 if(buf.st_mode & S_IWUSR)
>                                 {
>                                         result = -1;
>                                         fprintf(stderr, "non root user 
> has write access to %s\n", name);
>                                 }
>                         }

If a non-root user owns a directory, the user should be assumed to
have write access to it.  You must not check for S_IWUSR, that is
largely irrelevant.  This is because the user can chmod a directory
he owns after your check has run.

>                         if(buf.st_gid != 0)
>                         {
>                                 // make sure there is no group write access
>                                 if(buf.st_mode & S_IWGRP)
>                                 {
>                                         result = -1;
>                                         fprintf(stderr, "non root group 
> has write access to %s\n", name);
>                                 }
>                         }

And this check is buggy in "the opposite" way: you must not check for
GID 0 because it is not special to the kernel in any way and generally
there's no valid reason to consider it trusted.

>                         // make sure there is no group write access
>                         if(buf.st_mode & S_IWOTH)
>                         {
>                                 result = -1;
>                                 fprintf(stderr, "all users have write 
> access to %s\n", name);
>                         }

This one is OK, but I suggest that you combine it with the S_IWGRP
check above to simplify the code.  I don't see much need to have
different error messages for the three cases.

-- 
Alexander Peslyak <solar openwall com>
GPG key ID: B35D3598  fp: 6429 0D7E F130 C13E C929  6447 73C3 A290 B35D 3598
http://www.openwall.com - bringing security into open computing environments