OT Re: pam_chroot-0.8 released

[John Newbigin]

Solar Designer wrote:

> 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.
Thanks for the feedback.  I have modified the procedure based on your 
suggestions.  Obviously it can be modified as required.

John.

int verify_socket_path(char *name)
{
     int i = 0;
     char x;
     int result = 0;
     struct stat buf;

     name = strdup(name);
     if(!name)
     {
         return -1;
     }

     while(name[i])
     {
         if(name[i] == '/')
         {
             x = name[i + 1];
             name[i + 1] = 0;
             //printf("path = %s\n", name);
             stat(name, &buf);
             //printf("uid = %d gid = %d\n", buf.st_uid, buf.st_gid);
             if(buf.st_uid != 0)
             {
                 result = -1;
                 fprintf(stderr, "directory %s not owned by root\n", name);
             }
             if(buf.st_mode & S_IWGRP)
             {
                 result = -1;
                 fprintf(stderr, "group write access to %s\n", name);
             }
             if(buf.st_mode & S_IWOTH)
             {
                 result = -1;
                 fprintf(stderr, "other write access to %s\n", name);
             }
             name[i + 1] = x;
         }
         i++;
     }

     free(name);
     return result;
}

> 
> 
>>                        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.
> 


-- 
John Newbigin - Computer Systems Officer
School of Information Technology
Swinburne University of Technology
Melbourne, Australia
http://www.it.swin.edu.au/staff/jnewbigin