OT Re: pam_chroot-0.8 released

[John Newbigin <jn it swin edu au>]

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