[PATCH] Cygwin: access: Fix X_OK behaviour for administrator

[Takashi Yano]

Hi Corinna,

On Tue, 7 Jan 2025 17:48:59 +0100
Corinna Vinschen wrote:
> Hi Takashi,
> 
> Happy New Year!
> 
> On Dec 26 21:34, Takashi Yano wrote:
> > @@ -613,6 +613,22 @@ check_file_access (path_conv &pc, int flags, bool effective)
> >    if (flags & X_OK)
> >      desired |= FILE_EXECUTE;
> >  
> > +  /* The Administrator has full access permission regardless of ACL,
> > +     however, access() should return -1 if 'x' permission is set
> > +     for neither user, group nor others, even though NtOpenFile()
> > +     succeeds. */
> 
> The explanation isn't quite right, see below.
> 
> > +  if ((flags & X_OK) && !pc.isdir ())
> > +    {
> > +      struct stat st;
> > +      if (stat (pc.get_posix (), &st))
> > +	goto out;
> > +      else if ((st.st_mode & (S_IXUSR | S_IXGRP | S_IXOTH)) == 0)
> > +	{
> > +	  set_errno (EACCES);
> > +	  goto out;
> > +	}
> > +    }
> > +
> 
> Calling stat here is not the right thing to do.  It slows down access()
> as well as exec'ing applications a lot because it adds the overhead of a
> full system call on each invocation.
> 
> When I saw your patch this morning for the first time, I was inclined to
> request that you simply revert a0933cd17d19 ("Correction for samba/SMB
> share").  The behaviour on Samba was not a regression, but this here
> is, so it would be prudent to rethink the entire approach.
> 
> However, it occured to me that there may be a simpler way to fix this:
> 
> The reason for this behaviour is the way SE_BACKUP_PRIVILEGE works.  To
> allow a user with backup privileges full access to files, you have to
> enable the SE_BACKUP_PRIVILEGE in the user's token *and* you have to
> open files with FILE_OPEN_FOR_BACKUP_INTENT.  The problem now is this:
> SE_BACKUP_PRIVILEGE + FILE_OPEN_FOR_BACKUP_INTENT allow to open the
> file, no matter what.  In particular, they allow to open the file for
> FILE_EXECUTE, even if the execute perms in the ACL deny the user
> execution of the file.
> 
> So... given how this is supposed to work, we must not use the
> FILE_OPEN_FOR_BACKUP_INTENT flag when checking for execute permissions
> and the result should be the desired one.  I tested this locally, and I
> don't see a regression compared to 3.5.4.
> 
> Patch attached.  Please review.

Thanks for reviewing and the counter patch.

With your patch, access(_, X_OK) returns -1 for a directory without 'x'
permission even with Administrator.
This seems due to lack of FILE_OPEN_FOR_BACKUP_INTENT.

How about simpler patch attached?

-- 
Takashi Yano <takashi.yano@nifty.ne.jp>
-------------- next part --------------
An embedded and charset-unspecified text was scrubbed...
Name: v2-0001-Cygwin-access-Fix-X_OK-behaviour-for-backup-opera.patch
URL: <https://cygwin.com/pipermail/cygwin-patches/attachments/20250108/68665fec/attachment.ksh>