Re: SMBFS mount's file cannot be made executable

[Takashi Yano <takashi.yano@nifty.ne.jp>]

[-- Attachment #1: Type: text/plain, Size: 2382 bytes --]

On Tue, 19 Nov 2024 21:54:44 +0100
Corinna Vinschen wrote:
> On Nov 19 17:58, Takashi Yano via Cygwin wrote:
> > On Mon, 18 Nov 2024 17:26:12 +0100
> > Corinna Vinschen wrote:
> > > We can safely assume that the current user is already authorized on the
> > > SMB server.  So... shouldn't AuthzInitializeResourceManager be
> > > sufficient and the code from class authz_ctx already does what we want?
> > > We may just have to use in in place of calling NtCheckAccess(),
> > > maybe with a tweak or two...
> > 
> > I already tried AuthzInitializeResourceManager(), but the result
> > was the same with current implementation...
> 
> So you tried to call authz_get_user_attribute()?

Yes. But resulted in the same.

> > BTW, I come up with another implementation. This make the things
> > much simpler. What do you think of the patch attached?
> 
> > [...]
> >  int
> >  check_file_access (path_conv &pc, int flags, bool effective)
> >  {
> > @@ -711,10 +618,14 @@ check_file_access (path_conv &pc, int flags, bool effective)
> >      desired |= FILE_EXECUTE;
> >    if (!get_file_sd (pc.handle (), pc, sd, false))
> >      {
> > -      /* Tweak Samba security descriptor as necessary. */
> > -      if (pc.fs_is_samba ())
> > -	convert_samba_sd (sd);
> > -      ret = check_access (sd, file_mapping, desired, flags, effective);
> > +      HANDLE h = CreateFileW (pc.get_nt_native_path ()->Buffer, desired,
> > +			      0, NULL, OPEN_EXISTING,
> > +			      FILE_FLAG_BACKUP_SEMANTICS, NULL);
> > +      if (h != INVALID_HANDLE_VALUE)
> > +	{
> > +	  CloseHandle (h);
> > +	  ret = 0;
> > +	}
> >      }
> >    debug_printf ("flags %y, ret %d", flags, ret);
> >    return ret;
> 
> No, we can't do that, it's too simple.
> 
> Just kidding.
> 
> This is so simple, I'm puzzled we never tried that before.  Or, if we
> did, it's a loooong time ago...
> 
> If we really do this, we don't even need to call get_file_sd().  And it
> should use NtOpenFile and reopen semantics i.e.  pc.init_reopen_attr().
> Also, the sharing flags should allow all access.  And the `effective'
> argument needs to be taken into account.

I have a question. What pc.init_reopen_attr() is for? I tested with
pc.get_object_attr() instead, it works. What handle should I pass
to pc.init_reopen_attr()?

Anyway, I revised the patch as attached. What do you think?

-- 
Takashi Yano <takashi.yano@nifty.ne.jp>

[-- Attachment #2: 0001-Cygwin-access-Correction-for-samba-SMB-share.patch --]
[-- Type: text/plain, Size: 6366 bytes --]

From 777bdf75527f353ac83317a82e38794206bb6dd5 Mon Sep 17 00:00:00 2001
From: Takashi Yano <takashi.yano@nifty.ne.jp>
Date: Sun, 8 Dec 2024 07:34:48 +0900
Subject: [PATCH] Cygwin: access: Correction for samba/SMB share

Previously, access() and eaccess() does not determine the permissions
for files on samba/SMB share. Even if the user logs-in as the owner
of the file, access() and eaccess() referes to others' permissions.
With this patch, to determine the permissions correctly, NtOpenFile()
with desired access mask is used.

Fixes: cf762b08cfb0 ("* security.cc (check_file_access): Create.")
Reviewed-by: Corinna Vinschen <corinna@vinschen.de>
Signed-off-by: Takashi Yano <takashi.yano@nifty.ne.jp>
---
 winsup/cygwin/sec/base.cc | 136 +++++++++++---------------------------
 1 file changed, 37 insertions(+), 99 deletions(-)

diff --git a/winsup/cygwin/sec/base.cc b/winsup/cygwin/sec/base.cc
index d5e39d281..fcc5e1ff7 100644
--- a/winsup/cygwin/sec/base.cc
+++ b/winsup/cygwin/sec/base.cc
@@ -28,10 +28,6 @@ details. */
 				  | GROUP_SECURITY_INFORMATION \
 				  | OWNER_SECURITY_INFORMATION)
 
-static GENERIC_MAPPING NO_COPY_RO file_mapping = { FILE_GENERIC_READ,
-						   FILE_GENERIC_WRITE,
-						   FILE_GENERIC_EXECUTE,
-						   FILE_ALL_ACCESS };
 LONG
 get_file_sd (HANDLE fh, path_conv &pc, security_descriptor &sd,
 	     bool justcreated)
@@ -608,99 +604,9 @@ check_access (security_descriptor &sd, GENERIC_MAPPING &mapping,
   return ret;
 }
 
-/* Samba override.  Check security descriptor for Samba UNIX user and group
-   accounts and check if we have an RFC 2307 mapping to a Windows account.
-   Create a new security descriptor with all of the UNIX accounts with
-   valid mapping replaced with their Windows counterpart. */
-static void
-convert_samba_sd (security_descriptor &sd_ret)
-{
-  NTSTATUS status;
-  BOOLEAN dummy;
-  PSID sid;
-  cygsid owner;
-  cygsid group;
-  SECURITY_DESCRIPTOR sd;
-  cyg_ldap cldap;
-  tmp_pathbuf tp;
-  PACL acl, oacl;
-  size_t acl_len;
-  PACCESS_ALLOWED_ACE ace;
-
-  if (!NT_SUCCESS (RtlGetOwnerSecurityDescriptor (sd_ret, &sid, &dummy)))
-    return;
-  owner = sid;
-  if (!NT_SUCCESS (RtlGetGroupSecurityDescriptor (sd_ret, &sid, &dummy)))
-    return;
-  group = sid;
-
-  if (sid_id_auth (owner) == 22)
-    {
-      struct passwd *pwd;
-      uid_t uid = owner.get_uid (&cldap);
-      if (uid < UNIX_POSIX_OFFSET && (pwd = internal_getpwuid (uid)))
-	owner.getfrompw (pwd);
-    }
-  if (sid_id_auth (group) == 22)
-    {
-      struct group *grp;
-      gid_t gid = group.get_gid (&cldap);
-      if (gid < UNIX_POSIX_OFFSET && (grp = internal_getgrgid (gid)))
-	group.getfromgr (grp);
-    }
-
-  if (!NT_SUCCESS (RtlGetDaclSecurityDescriptor (sd_ret, &dummy,
-						 &oacl, &dummy)))
-    return;
-  acl = (PACL) tp.w_get ();
-  RtlCreateAcl (acl, ACL_MAXIMUM_SIZE, ACL_REVISION);
-  acl_len = sizeof (ACL);
-
-  for (DWORD i = 0; i < oacl->AceCount; ++i)
-    if (NT_SUCCESS (RtlGetAce (oacl, i, (PVOID *) &ace)))
-      {
-	cygsid ace_sid ((PSID) &ace->SidStart);
-	if (sid_id_auth (ace_sid) == 22)
-	  {
-	    if (sid_sub_auth (ace_sid, 0) == 1) /* user */
-	      {
-		struct passwd *pwd;
-		uid_t uid = ace_sid.get_uid (&cldap);
-		if (uid < UNIX_POSIX_OFFSET && (pwd = internal_getpwuid (uid)))
-		  ace_sid.getfrompw (pwd);
-	      }
-	    else if (sid_sub_auth (ace_sid, 0) == 2) /* group */
-	      {
-		struct group *grp;
-		gid_t gid = ace_sid.get_gid (&cldap);
-		if (gid < UNIX_POSIX_OFFSET && (grp = internal_getgrgid (gid)))
-		  ace_sid.getfromgr (grp);
-	      }
-	  }
-	if (!add_access_allowed_ace (acl, ace->Mask, ace_sid, acl_len,
-				     ace->Header.AceFlags))
-	  return;
-      }
-  acl->AclSize = acl_len;
-
-  RtlCreateSecurityDescriptor (&sd, SECURITY_DESCRIPTOR_REVISION);
-  RtlSetControlSecurityDescriptor (&sd, SE_DACL_PROTECTED, SE_DACL_PROTECTED);
-  RtlSetOwnerSecurityDescriptor (&sd, owner, FALSE);
-  RtlSetGroupSecurityDescriptor (&sd, group, FALSE);
-
-  status = RtlSetDaclSecurityDescriptor (&sd, TRUE, acl, FALSE);
-  if (!NT_SUCCESS (status))
-    return;
-  DWORD sd_size = 0;
-  status = RtlAbsoluteToSelfRelativeSD (&sd, sd_ret, &sd_size);
-  if (sd_size > 0 && sd_ret.malloc (sd_size))
-    RtlAbsoluteToSelfRelativeSD (&sd, sd_ret, &sd_size);
-}
-
 int
 check_file_access (path_conv &pc, int flags, bool effective)
 {
-  security_descriptor sd;
   int ret = -1;
   ACCESS_MASK desired = 0;
   if (flags & R_OK)
@@ -709,12 +615,44 @@ check_file_access (path_conv &pc, int flags, bool effective)
     desired |= FILE_WRITE_DATA;
   if (flags & X_OK)
     desired |= FILE_EXECUTE;
-  if (!get_file_sd (pc.handle (), pc, sd, false))
+
+  NTSTATUS status;
+  if (!effective && cygheap->user.issetuid ())
+    {
+      /* Strip impersonation token temporarily */
+      HANDLE tok = NO_IMPERSONATION;
+      status = NtSetInformationThread (GetCurrentThread (),
+				       ThreadImpersonationToken,
+				       &tok, sizeof (tok));
+      if (!NT_SUCCESS (status))
+	{
+	  debug_printf("NtSetInformationThread() for stripping "
+		       "impersonation token failed: %y", status);
+	  __seterrno_from_nt_status (status);
+	  return ret;
+	}
+    }
+  OBJECT_ATTRIBUTES attr;
+  pc.get_object_attr (attr, sec_none_nih);
+  IO_STATUS_BLOCK io;
+  HANDLE h;
+  status = NtOpenFile (&h, desired, &attr, &io, FILE_SHARE_VALID_FLAGS,
+		       FILE_OPEN_FOR_BACKUP_INTENT);
+  if (NT_SUCCESS (status))
     {
-      /* Tweak Samba security descriptor as necessary. */
-      if (pc.fs_is_samba ())
-	convert_samba_sd (sd);
-      ret = check_access (sd, file_mapping, desired, flags, effective);
+      NtClose (h);
+      ret = 0;
+    }
+  if (!effective && cygheap->user.issetuid ())
+    {
+      /* Recover impersonation token */
+      HANDLE tok = cygheap->user.imp_token () ?: hProcImpToken;
+      status = NtSetInformationThread (GetCurrentThread (),
+				       ThreadImpersonationToken,
+				       &tok, sizeof (tok));
+      if (!NT_SUCCESS (status))
+	debug_printf("NtSetInformationThread() for recovering "
+		     "impersonation token failed: %y", status);
     }
   debug_printf ("flags %y, ret %d", flags, ret);
   return ret;
-- 
2.45.1