Re: [HTCondor-devel] bug in how condor_submit checks initialdir permissions?

[David Champion <dgc@xxxxxxxxxxxx>]

Yes, good point with the close() -- I disregarded since the proc was
about to exit, but it's better to take care.  As well, since I wasn't
yet able to compile condor, you'll find I pythoned up the open().  It
needs double quotes.

Do you need me to resend with fixes?  CLA is in the mail.


* On 13 Jun 2014, Jaime Frey wrote: 
> With the addition of a âclose(back)â, this patch looks good. Are you willing to sign a Contributor License Agreement, as detail here:
>   https://htcondor-wiki.cs.wisc.edu/index.cgi/wiki?p=MakingContributions
> 
> Then, Iâll commit your patch for future releases.
> 
>  â Jaime Frey
> 
> On Jun 10, 2014, at 3:30 PM, David Champion <dgc@xxxxxxxxxxxx> wrote:
> 
> > Trying again here. *bump*
> > 
> > 
> > * On 30 May 2014, David Champion wrote: 
> >> * On 11 Mar 2013, Jaime Frey wrote: 
> >>> On Mar 5, 2013, at 9:29 AM, Jason Ferrara <jason.ferrara@xxxxxxxxxxxxx> wrote:
> >>> 
> >>>> We've been having issues where a user would submit a job, and condor_submit would respond with
> >>>> 
> >>>> ERROR: No such directory: <path to initialdir>
> >>>> 
> >>>> even though the directory pointed to by initialdir does exist and the user has full read/write/execute permission for it.
> >>>> 
> >>>> To check for access to initialdir, condor_submit calls check_iwd, which calls access_euid, which calls access_euid_dir. access_euid_dir checks if the effrective uid or gid has access by manually checking permission bits, but it doesn't check secondary groups or ACLs, so the access check can fail even if the user really does have access. Also, check_iwd always prints "No such directory", even if the failure is caused by lack of write permission to the directory.
> >>>> 
> >>>> Replacing all calls to access_euid with the system provided euidaccess seems to fix the problem. Is this the right thing to do?
> >>> 
> >>> That does sound like the right thing to do, where euidaccess() is available. I'll make an entry in our bug tracking system, though I can't say when we'll get a chance to make the change and test it.
> >> 
> >> This is catching users in OSG Connect, so I'd like to see a fix.
> >> euidaccess() (eaccess()) is a GNU extension, and pretty unportable.
> >> 
> >> access_euid_dir() amounts to this rule:
> >> 
> >> * if testing R_OK, attempt to open the directory to read
> >> * if testing W_OK, attempt to create a file in the directory (i.e. write the directory)
> >> * if testing X_OK, do a bunch of tests against euid and egid, neglecting supplementary groups and POSIX ACLs.
> >> 
> >> That is, R_OK and W_OK use empirical tests, while X_OK attempts to
> >> emulate the logic of the kernel.  Why not have the X_OK test simply
> >> attempt chdir() to the directory?  For example:
> >> 
> >> 
> >> diff --git a/src/condor_utils/access_euid.unix.cpp b/src/condor_utils/access_euid.unix.cpp
> >> --- a/src/condor_utils/access_euid.unix.cpp
> >> +++ b/src/condor_utils/access_euid.unix.cpp
> >> @@ -21,6 +21,7 @@
> >> #define _FILE_OFFSET_BITS 64
> >> #include "condor_common.h"
> >> #include "condor_debug.h"
> >> +#include <fcntl.h>
> >> #include <dirent.h>
> >> 
> >> 
> >> @@ -69,33 +70,13 @@
> >> 	}
> >> 
> >> 	if ((X_OK == 0) || (mode & X_OK)) {
> >> -		struct stat st;
> >> -		if (!statbuf) {
> >> -			/* stats are expensive, so only do it if I have to */
> >> -			if (stat(path, &st) < 0) {
> >> -				if( ! errno ) {
> >> -					dprintf( D_ALWAYS, "WARNING: stat() failed, but "
> >> -							 "errno is still 0!  Beware of misleading "
> >> -							 "error messages\n" );
> >> -				}
> >> -				return -1;
> >> -			}
> >> -			statbuf = &st;
> >> -		}
> >> -		int bit = 0;
> >> -		if( statbuf->st_uid == geteuid() ) {
> >> -			bit |= S_IXUSR;
> >> -		}
> >> -		else if( statbuf->st_gid == getegid() ) {
> >> -			bit |= S_IXGRP;
> >> -		}
> >> -		else {
> >> -			bit |= S_IXOTH;
> >> -		}
> >> -		if (!(statbuf->st_mode & bit)) {
> >> -			errno = EACCES;
> >> -			return -1;
> >> -		}
> >> +		int back;
> >> +		back = open('.', O_RDONLY);
> >> +		if (chdir(path) != 0)
> >> +			return -1;     /* errno was set by chdir() */
> >> +
> >> +		/* chdir succeeded, so this effective user has access */
> >> +		fchdir(back);
> >> 	}
> >> 
> >> 	return 0;
> 
> 
> Thanks and regards,
> Jaime Frey
> UW-Madison HTCondor Project

-- 
       David Champion â dgc@xxxxxxxxxxxx â University of Chicago
Enrico Fermi Institute â Computation Institute â USATLAS Midwest Tier 2
                      OSG Connect â CI Connect