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

[Takashi Yano]

On Wed, 8 Jan 2025 06:14:24 +0900
Takashi Yano wrote:
> 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?

Revised a bit.

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