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.y...@nifty.ne.jp>
From 15bed39093acd20b7114fc706f455c5c40e00c29 Mon Sep 17 00:00:00 2001
From: Corinna Vinschen <cori...@vinschen.de>
Date: Tue, 7 Jan 2025 17:44:44 +0100
Subject: [PATCH v3] Cygwin: access: Fix X_OK behaviour for backup operators
and admins
After commit a0933cd17d19, access(_, X_OK) returns 0 if the user
holds SE_BACKUP_PRIVILEGE, even if the file's ACL denies execution
to the user. This is triggered by trying to open the file with
FILE_OPEN_FOR_BACKUP_INTENT.
Fix check_file_access() so it checks for X_OK without specifying
the FILE_OPEN_FOR_BACKUP_INTENT flag if the file is not a directory.
Rearrange function slightly and add comments for easier comprehension.
Fixes: a0933cd17d19 ("Cygwin: access: Correction for samba/SMB share")
Reported-by: Bruno Haible <br...@clisp.org>
Signed-off-by: Corinna Vinschen <cori...@vinschen.de>
---
winsup/cygwin/sec/base.cc | 28 +++++++++++++++++++++-------
1 file changed, 21 insertions(+), 7 deletions(-)
diff --git a/winsup/cygwin/sec/base.cc b/winsup/cygwin/sec/base.cc
index 647c27ec6..9f1527b7b 100644
--- a/winsup/cygwin/sec/base.cc
+++ b/winsup/cygwin/sec/base.cc
@@ -604,8 +604,14 @@ check_access (security_descriptor &sd, GENERIC_MAPPING
&mapping,
int
check_file_access (path_conv &pc, int flags, bool effective)
{
- int ret = -1;
+ NTSTATUS status;
ACCESS_MASK desired = 0;
+ ULONG opts = 0;
+ OBJECT_ATTRIBUTES attr;
+ IO_STATUS_BLOCK io;
+ HANDLE h = NULL;
+ int ret = -1;
+
if (flags & R_OK)
desired |= FILE_READ_DATA;
if (flags & W_OK)
@@ -616,13 +622,21 @@ check_file_access (path_conv &pc, int flags, bool
effective)
if (!effective)
cygheap->user.deimpersonate ();
- OBJECT_ATTRIBUTES attr;
pc.init_reopen_attr (attr, pc.handle ());
- NTSTATUS status;
- IO_STATUS_BLOCK io;
- HANDLE h;
- status = NtOpenFile (&h, desired, &attr, &io, FILE_SHARE_VALID_FLAGS,
- FILE_OPEN_FOR_BACKUP_INTENT);
+
+ /* For R_OK and W_OK we check with FILE_OPEN_FOR_BACKUP_INTENT since
+ we want to enable the full power of backup/restore privileges.
+ For X_OK, drop the FILE_OPEN_FOR_BACKUP_INTENT flag. If the caller
+ holds SE_BACKUP_PRIVILEGE, FILE_OPEN_FOR_BACKUP_INTENT opens the file,
+ no matter what access is requested.
+ For directories, FILE_OPEN_FOR_BACKUP_INTENT flag is always set
+ regardless of X_OK. */
+ if (!(flags & X_OK) || pc.isdir ())
+ opts |= FILE_OPEN_FOR_BACKUP_INTENT;
+ else /* For a regular file to be executable, it must also be readable. */
+ desired |= FILE_READ_DATA;
+
+ status = NtOpenFile (&h, desired, &attr, &io, FILE_SHARE_VALID_FLAGS, opts);
if (NT_SUCCESS (status))
{
NtClose (h);
--
2.45.1
