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.y...@nifty.ne.jp>
From 777bdf75527f353ac83317a82e38794206bb6dd5 Mon Sep 17 00:00:00 2001
From: Takashi Yano <takashi.y...@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 <cori...@vinschen.de>
Signed-off-by: Takashi Yano <takashi.y...@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
--
Problem reports: https://cygwin.com/problems.html
FAQ: https://cygwin.com/faq/
Documentation: https://cygwin.com/docs.html
Unsubscribe info: https://cygwin.com/ml/#unsubscribe-simple
