-----BEGIN PGP SIGNED MESSAGE----- Hash: SHA1
This proposed patch (which I believe will contribute toward solving bugs 17672, 19588 and 20643, and any others where the permissions are set too restrictive) exposes the token_sid_present call in token.c, and uses it to check the SIDs in the security descriptor against those in the process token.
Are there any changes anyone can think of before I submit it to wine-patches?
Is there a better (already exposed) way of checking a SID against the process token's group list?
- ---- server/file.c | 6 ++++-- server/security.h | 1 + server/token.c | 2 +- 3 files changed, 6 insertions(+), 3 deletions(-)
server/file.c | 6 ++++-- server/security.h | 1 + server/token.c | 2 +- 3 files changed, 6 insertions(+), 3 deletions(-)
diff --git a/server/file.c b/server/file.c index a74de14..793c24a 100644 --- a/server/file.c +++ b/server/file.c @@ -485,7 +485,8 @@ mode_t sd_to_mode( const struct security_descriptor *sd, const SID *owner ) if (access & FILE_EXECUTE) denied_mode |= S_IXUSR|S_IXGRP|S_IXOTH; } - else if (security_equal_sid( sid, owner )) + else if (security_equal_sid( sid, owner ) || + token_sid_present( current->process->token, sid, 1 )) { unsigned int access = generic_file_map_access( ad_ace->Mask ); if (access & FILE_READ_DATA) @@ -509,7 +510,8 @@ mode_t sd_to_mode( const struct security_descriptor *sd, const SID *owner ) if (access & FILE_EXECUTE) new_mode |= S_IXUSR|S_IXGRP|S_IXOTH; } - else if (security_equal_sid( sid, owner )) + else if (security_equal_sid( sid, owner ) || + token_sid_present( current->process->token, sid, 0 )) { unsigned int access = generic_file_map_access( aa_ace->Mask ); if (access & FILE_READ_DATA) diff --git a/server/security.h b/server/security.h index 39b1d2f..33cf5da 100644 --- a/server/security.h +++ b/server/security.h @@ -55,6 +55,7 @@ extern int token_check_privileges( struct token *token, int all_required, extern const ACL *token_get_default_dacl( struct token *token ); extern const SID *token_get_user( struct token *token ); extern const SID *token_get_primary_group( struct token *token ); +extern int token_sid_present( struct token *token, const SID *sid, int deny);
static inline const ACE_HEADER *ace_next( const ACE_HEADER *ace ) { diff --git a/server/token.c b/server/token.c index ce896ac..461e79d 100644 --- a/server/token.c +++ b/server/token.c @@ -776,7 +776,7 @@ int token_check_privileges( struct token *token, int all_required, return (enabled_count > 0); }
-static int token_sid_present( struct token *token, const SID *sid, int deny ) +int token_sid_present( struct token *token, const SID *sid, int deny ) { struct group *group;
2009/11/16 Ben Peddell klightspeed@netspace.net.au:
This proposed patch (which I believe will contribute toward solving bugs 17672, 19588 and 20643, and any others where the permissions are set too restrictive) exposes the token_sid_present call in token.c, and uses it to check the SIDs in the security descriptor against those in the process token.
Are there any changes anyone can think of before I submit it to wine-patches?
Is there a better (already exposed) way of checking a SID against the process token's group list?
Hi Ben,
While I agree that there is a problem that needs to be fixed, I'm not sure this is the right approach. I think you need to take a step back and consider the meanings of the different SIDs in a token by default and how they map wine running inside the Unix permissions model. For example, maybe these mappings make sense:
security_local_sid -> user + group + others security_interactive_sid -> user + group + others alias_users_sid -> user + group + others?
Now it's likely that the bugs you are trying to fix are trying to set the SD for a file to alias_admins_sid or alias_users_sid. The mapping for alias_admins_sid is less clear - one could argue that all Wine users on a given system would present themselves as admins to apps, but then again the apps may be restricting permissions on a file because it contains sensitive data and should only be shared with other admins (which would be trusted as such, unlike other users on a system).
-----BEGIN PGP SIGNED MESSAGE----- Hash: SHA1
Rob Shearman wrote:
While I agree that there is a problem that needs to be fixed, I'm not sure this is the right approach. I think you need to take a step back and consider the meanings of the different SIDs in a token by default and how they map wine running inside the Unix permissions model. For example, maybe these mappings make sense:
security_local_sid -> user + group + others security_interactive_sid -> user + group + others alias_users_sid -> user + group + others?
Now it's likely that the bugs you are trying to fix are trying to set the SD for a file to alias_admins_sid or alias_users_sid. The mapping for alias_admins_sid is less clear - one could argue that all Wine users on a given system would present themselves as admins to apps, but then again the apps may be restricting permissions on a file because it contains sensitive data and should only be shared with other admins (which would be trusted as such, unlike other users on a system).
Currently, the wine NT DACL -> unix permission code works as follows: * The permissions start as deny all. * If the world SID is granted a permission, then that permission is added to both owner and others. * If the owner SID is explicitly granted a permission, then that permission is added to owner. * If the world SID is denied a permission, then that permission is removed from both owner and others. * If the owner SID is explicitly denied a permission, then that permission is removed from owner.
This means that if neither owner nor world are listed in the NT DACL, the owner of the file is denied all permissions on that file, even if they would have been granted access by one of their groups.
Windows Access Control Lists act as a series of grant/deny Access Control Entries, and the entire Access Control List must be consulted to determine the rights of the user. If none of the user, the user's groups or the world SID are mentioned in the DACL, then that user is denied all permissions. No SIDs are treated specially (not even SYSTEM).
Unix permissions and Access Control Lists act as a set of definitive Access Control Entries, and the first Access Control Entry that applies to the user or one of their groups is used to determine the rights of the user. The superuser is treated specially, and is always granted permission (unless squashed by the filesystem or a security module).
As the owner permission is consulted before any entries in the ACL, the owner permission cannot be overridden by an entry in the ACL.
I have modified my patch to only check the groups in the process token if the owner of the process is the owner of the file.
This patch: * in server/token.c: * exposes token_sid_present(), which tests whether an SID is present in the token. * in server/file.c::sd_to_mode(): * Sets the local boolean variable user_is_owner if the owner of the process and the owner of the file are the same. * Denies permissions from the owner of the file if user_is_owner is true and the SID denying permissions is present in the token. * Grants permissions to the owner of the file if user_is_owner is true and the SID granting permissions is present in the token, and is not a deny-only SID.
The patch seeks to ensure that the owner of the file is granted and/or denied permissions based on the permissions of their groups. At the moment, it only does this if the owner of the file is also the owner of the process, as I don't know how to look up the groups of other users.
The only other way I can see of doing this is to pre-process the SID and add a grant and/or deny ACE for the owner based on the access given by their groups.
server/file.c | 7 +++++-- server/security.h | 1 + server/token.c | 2 +- 3 files changed, 7 insertions(+), 3 deletions(-)
diff --git a/server/file.c b/server/file.c index a74de14..db9dd5f 100644 --- a/server/file.c +++ b/server/file.c @@ -461,6 +461,7 @@ mode_t sd_to_mode( const struct security_descriptor *sd, const SID *owner ) if (present && dacl) { const ACE_HEADER *ace = (const ACE_HEADER *)(dacl + 1); + int user_is_owner = security_equal_sid( owner, token_get_user( current->process->token) ); ULONG i; for (i = 0; i < dacl->AceCount; i++, ace = ace_next( ace )) { @@ -485,7 +486,8 @@ mode_t sd_to_mode( const struct security_descriptor *sd, const SID *owner ) if (access & FILE_EXECUTE) denied_mode |= S_IXUSR|S_IXGRP|S_IXOTH; } - else if (security_equal_sid( sid, owner )) + else if (security_equal_sid( sid, owner ) || + (user_is_owner && token_sid_present( current->process->token, sid, 1 ))) { unsigned int access = generic_file_map_access( ad_ace->Mask ); if (access & FILE_READ_DATA) @@ -509,7 +511,8 @@ mode_t sd_to_mode( const struct security_descriptor *sd, const SID *owner ) if (access & FILE_EXECUTE) new_mode |= S_IXUSR|S_IXGRP|S_IXOTH; } - else if (security_equal_sid( sid, owner )) + else if (security_equal_sid( sid, owner ) || + (user_is_owner && token_sid_present( current->process->token, sid, 0 ))) { unsigned int access = generic_file_map_access( aa_ace->Mask ); if (access & FILE_READ_DATA) diff --git a/server/security.h b/server/security.h index 39b1d2f..33cf5da 100644 --- a/server/security.h +++ b/server/security.h @@ -55,6 +55,7 @@ extern int token_check_privileges( struct token *token, int all_required, extern const ACL *token_get_default_dacl( struct token *token ); extern const SID *token_get_user( struct token *token ); extern const SID *token_get_primary_group( struct token *token ); +extern int token_sid_present( struct token *token, const SID *sid, int deny);
static inline const ACE_HEADER *ace_next( const ACE_HEADER *ace ) { diff --git a/server/token.c b/server/token.c index ce896ac..461e79d 100644 --- a/server/token.c +++ b/server/token.c @@ -776,7 +776,7 @@ int token_check_privileges( struct token *token, int all_required, return (enabled_count > 0); }
-static int token_sid_present( struct token *token, const SID *sid, int deny ) +int token_sid_present( struct token *token, const SID *sid, int deny ) { struct group *group;
Hi!
How should server/file.c sd_to_mode() deal with filesystems that don't support full POSIX ownership and access permissions? It is quite popular to mount FAT filesystems - either from a removable media or a partition shared with a Windows installation.
Furthermore, there could be symlinks to such files so filesystem capability should be detected somehow from the file itself.
Since recently I am unable to patch WoW since it started to call SetFileSecurity on the program directory which is located on a FAT32 partition. Everything appears owned by root but with rwxrwxrwx access. Wine tries to set ---rwx--- which is, to say the least, not commonly seen.
http://bugs.winehq.org/show_bug.cgi?id=20643
Paul
-----BEGIN PGP SIGNED MESSAGE----- Hash: SHA1
Paul Chitescu wrote:
Hi!
How should server/file.c sd_to_mode() deal with filesystems that don't support full POSIX ownership and access permissions? It is quite popular to mount FAT filesystems - either from a removable media or a partition shared with a Windows installation.
Furthermore, there could be symlinks to such files so filesystem capability should be detected somehow from the file itself.
The only portable way I can think of is having certain "drives" marked as e.g. FAT32 (instead of NTFS), and having dlls/ntdll/sec.c::NtSetSecurityObject() either fake success, or (with support from other areas) do whatever it does on FAT32 (and I don't know what that is).
The fstatvfs portability function in libport only reports filesystem space, and not type or id, so that cannot be used to portably detect what type of filesystem the file sits on.
Since recently I am unable to patch WoW since it started to call SetFileSecurity on the program directory which is located on a FAT32 partition. Everything appears owned by root but with rwxrwxrwx access. Wine tries to set ---rwx--- which is, to say the least, not commonly seen.
The World of Warcraft Launcher tries to set the directory to Full Access only to the Users group, which wine currently does put the user in. This is a World of Warcraft bug in that it also affects Windows users. However, it exposes a shortcoming in the ACL handling in wine.
The setting to ---???--- is one of the bugs I am hoping to fix with this patch. Currently, server/file.c::sd_to_mode() only honours owner and world permissions, and does not look at what groups the owner is a member of.
After this patch, the Launcher (through wine) should set the permissions to rwx???--- if the user owns the directory.
I can see multiple bug reports in bugs.winehq.org involving either programs removing the owner permissions or FAT32 mounts not working due to failed permission setting.
http://bugs.winehq.org/show_bug.cgi?id=17672 Wine denies access to Oracle Client install folder http://bugs.winehq.org/show_bug.cgi?id=20357 Problem with wine creating folders on installs http://bugs.winehq.org/show_bug.cgi?id=19588 Wine is setting incorrect permissions in some instances http://bugs.winehq.org/show_bug.cgi?id=10067 Steam Failed to set file attrbutes http://bugs.winehq.org/show_bug.cgi?id=17776 Installing on Fat32 partitions seems to be impossible nowadays http://bugs.winehq.org/show_bug.cgi?id=18359 InstallShield fails with "Access Denied" dialog when trying to install to FAT32 volume http://bugs.winehq.org/show_bug.cgi?id=20643 World of Warcraft launcher tries to change folder permissions (Not a Wine bug)
-----BEGIN PGP SIGNED MESSAGE----- Hash: SHA1
I have just tested this patch with the World of Warcraft launcher (couldn't before due to a mismatch between 32-bit and 64-bit jpeg versions, and Gentoo masking out jpeg).
After SetFileSecurityW (L"H:\Games\World of Warcraft", 0x4, 0x1697c8) with the ACL "(A;OICI;GA;;;BU)", the permissions on the World of Warcraft directory are rwxr-x---, and the Launcher successfully starts the Downloader.
-----BEGIN PGP SIGNED MESSAGE----- Hash: SHA1
Paul Chitescu wrote:
How should server/file.c sd_to_mode() deal with filesystems that don't support full POSIX ownership and access permissions? It is quite popular to mount FAT filesystems - either from a removable media or a partition shared with a Windows installation.
FAT32 support
To support this, I think the following would need to be done:
* Possibly split out and extended the filesystem type autodetection code in dlls/ntdll/file.c::get_device_info() to autodetect the filesystem's ability to hold ACLs
* Implement NtQueryVolumeInformationFile(FileFsAttributeInformation) - this has flags such as: * FILE_PERSISTENT_ACLS: Persistent ACLs (NTFS vs FAT32), * FILE_CASE_SENSITIVE_SEARCH: case-sensitive search (POSIX) - this also has the name of the filesystem (e.g. NTFS, FAT32).
* Implement the honouring of FILE_PERSISTENT_ACLS in NtQuerySecurityObject, NtSetSecurityObject
* Possibly add a filesystem type configuration option to winecfg (separate to media type).
* Possibly honour the media and filesystem type settings as configured by winecfg.
Furthermore, there could be symlinks to such files so filesystem capability should be detected somehow from the file itself.
Volume Mount Point support
* Possibly extend dosdevices to include devices for Volume Mount Points.
* Possibly implement detection of symbolic links leading to mount points as volume mount points - i.e. create \Device entries for mount points, and redirect Volume Mount Point targets to those devices.
-----BEGIN PGP SIGNED MESSAGE----- Hash: SHA1
Paul Chitescu wrote:
How should server/file.c sd_to_mode() deal with filesystems that don't support full POSIX ownership and access permissions? It is quite popular to mount FAT filesystems - either from a removable media or a partition shared with a Windows installation.
A workaround for the moment on linux would be to use the uid=<your_uid> option when mounting the a FAT filesystem, as the linux vfat filesystem driver basically ignores chmod (except for the u+w bit) when the user executing chmod is the owner of the filesystem. I don't know if Mac OS X, *BSD or Solaris do the same.
(tracing using procmon and setacl) NtQuerySecurityObject and NtSetSecurityObject return STATUS_INVALID_DEVICE_REQUEST on Windows XP when attempting to get or set a security descriptor on a FAT filesystem.
(musing) I wonder if it'd be possible to have a special SD pointer (such as -1 or a special SD_INVALID_DEVICE_REQUEST descriptor) in a wineserver object to say that neither the object nor any of its children can hold security descriptors? After all, you cannot put volume mount points on a FAT filesystem, so nothing under a FAT filesystem would be able to hold a security descriptor. obj->get_sd() and obj->set_sd() would then be made to return STATUS_INVALID_DEVICE_REQUEST on such an object.
-
Ben Peddell -
Paul Chitescu -
Rob Shearman