[PATCH v7 0/3] MR1895: Fixes for NtOpenFile of read-only files with FILE_WRITE_ATTRIBUTES

[Joel Holdsworth (＠jhol)]

Previously, the Wine implementation of `NtOpenFile` would fail to open read-only files for
`FILE_WRITE_ATTRIBUTES` access in wine-server, because it would attempt to open the file with `O_RDWR`
or `O_WRONLY` access.

This patch-set adds test cases to highlight the issue, and solves it by ~~only opening the file with write access where necessary~~ temporarily modifying file access permisions to allow the file to be opened.

* Wine Bug: [#50771](https://bugs.winehq.org/show_bug.cgi?id=50771)
  * Related Wine-Staging Patches:
      * [0002-server-Allow-to-open-files-without-any-permission-bi.patch](https://gitlab.winehq.org/wine/wine-staging/-/blob/master/patches/server-Fil...)
      * [0006-ntdll-tests-Added-tests-for-open-behaviour-on-readon.patch](https://gitlab.winehq.org/wine/wine-staging/-/blob/master/patches/server-Fil...)
      * [0007-server-FILE_WRITE_ATTRIBUTES-should-succeed-for-read.patch](https://gitlab.winehq.org/wine/wine-staging/-/blob/master/patches/server-Fil...)

--
  v7: server: Allow to open files without permission bits.

https://gitlab.winehq.org/wine/wine/-/merge_requests/1895

[PATCH v7 1/3] ntdll/tests: Added tests for attributes access on read-only files.

[Joel Holdsworth]

From: Joel Holdsworth

This patch was inspired by the wine-staging patch:

server-File_Permissions/0006-ntdll-tests-Added-tests-for-open-behaviour-on-readon.patch

Co-authored-by: Qian Hong 
Signed-off-by: Joel Holdsworth 
---
 dlls/ntdll/tests/file.c | 44 +++++++++++++++++++++++++++++++++++++++++
 1 file changed, 44 insertions(+)

diff --git a/dlls/ntdll/tests/file.c b/dlls/ntdll/tests/file.c
index 6186afdfb63..57dee54076a 100644
--- a/dlls/ntdll/tests/file.c
+++ b/dlls/ntdll/tests/file.c
@@ -5279,6 +5279,7 @@ static void test_file_readonly_access(void)
     static const WCHAR fooW[] = {'f', 'o', 'o', 0};
     WCHAR path[MAX_PATH];
     OBJECT_ATTRIBUTES attr;
+    FILE_BASIC_INFORMATION fbi;
     UNICODE_STRING nameW;
     IO_STATUS_BLOCK io;
     HANDLE handle;
@@ -5303,6 +5304,49 @@ static void test_file_readonly_access(void)
     ok(status == STATUS_SUCCESS, "expected STATUS_SUCCESS, got %#lx.\n", status);
     CloseHandle(handle);

+    /* NtOpenFile FILE_READ_ATTRIBUTES */
+    status = pNtOpenFile(&handle, FILE_READ_ATTRIBUTES,  &attr, &io, default_sharing, FILE_NON_DIRECTORY_FILE);
+    ok(status == STATUS_SUCCESS, "got %#lx\n", status);
+
+    memset(&fbi, 0, sizeof(fbi));
+    status = pNtQueryInformationFile(handle, &io, &fbi, sizeof fbi, FileBasicInformation);
+    ok(status == STATUS_SUCCESS, "can't get attributes, status %#lx\n", status);
+    ok((fbi.FileAttributes & FILE_ATTRIBUTE_READONLY) == FILE_ATTRIBUTE_READONLY,
+       "attribute %lx not expected\n", fbi.FileAttributes );
+
+    CloseHandle(handle);
+
+    /* NtOpenFile FILE_WRITE_ATTRIBUTES */
+    status = pNtOpenFile(&handle, FILE_WRITE_ATTRIBUTES,  &attr, &io, default_sharing, FILE_NON_DIRECTORY_FILE);
+    todo_wine ok(status == STATUS_SUCCESS, "got %#lx\n", status);
+
+    if (status == STATUS_SUCCESS) {
+        memset(&fbi, 0, sizeof(fbi));
+        fbi.FileAttributes = FILE_ATTRIBUTE_NORMAL;
+        U(io).Status = 0xdeadbeef;
+        status = pNtSetInformationFile(handle, &io, &fbi, sizeof fbi, FileBasicInformation);
+        todo_wine ok(status == STATUS_SUCCESS, "can't set system attribute, NtSetInformationFile returned %#lx\n",
+                     status);
+        todo_wine ok(U(io).Status == STATUS_SUCCESS, "can't set system attribute, io.Status is %lx\n",
+                     U(io).Status);
+
+        fbi.FileAttributes = FILE_ATTRIBUTE_READONLY;
+        U(io).Status = 0xdeadbeef;
+        status = pNtSetInformationFile(handle, &io, &fbi, sizeof fbi, FileBasicInformation);
+        todo_wine ok(status == STATUS_SUCCESS, "can't set system attribute, NtSetInformationFile returned %#lx\n",
+                     status);
+        todo_wine ok(U(io).Status == STATUS_SUCCESS, "can't set system attribute, io.Status is %lx\n",
+                     U(io).Status);
+    }
+
+    CloseHandle(handle);
+
+    /* NtOpenFile FILE_READ_ATTRIBUTES and FILE_WRITE_ATTRIBUTES */
+    status = pNtOpenFile(&handle, FILE_READ_ATTRIBUTES|FILE_WRITE_ATTRIBUTES,  &attr, &io,
+                         default_sharing, FILE_NON_DIRECTORY_FILE);
+    todo_wine ok(status == STATUS_SUCCESS, "got %#lx\n", status);
+    CloseHandle(handle);
+
     /* NtCreateFile FILE_GENERIC_WRITE */
     status = pNtCreateFile(&handle, FILE_GENERIC_WRITE, &attr, &io, NULL, FILE_ATTRIBUTE_NORMAL, default_sharing,
                            FILE_OPEN, FILE_NON_DIRECTORY_FILE, NULL, 0);

[PATCH v7 2/3] server: Open code FILE_UNIX_{READ,WRITE}_ACCESS

[Joel Holdsworth]

From: Joel Holdsworth

These defines are counter-productive for code clarity.

Signed-off-by: Joel Holdsworth 
---
 server/fd.c   | 8 +++++---
 server/file.h | 7 -------
 2 files changed, 5 insertions(+), 10 deletions(-)

diff --git a/server/fd.c b/server/fd.c
index eaebe044f37..839a1dec914 100644
--- a/server/fd.c
+++ b/server/fd.c
@@ -1941,9 +1941,10 @@ struct fd *open_fd( struct fd *root, const char *name, struct unicode_str nt_nam
         flags &= ~(O_CREAT | O_EXCL | O_TRUNC);
     }

-    if ((access & FILE_UNIX_WRITE_ACCESS) && !(options & FILE_DIRECTORY_FILE))
+    if ((access & (FILE_WRITE_DATA|FILE_APPEND_DATA|FILE_WRITE_ATTRIBUTES|FILE_WRITE_EA)) &&
+        !(options & FILE_DIRECTORY_FILE))
     {
-        if (access & FILE_UNIX_READ_ACCESS) rw_mode = O_RDWR;
+        if (access & (FILE_READ_DATA|FILE_READ_ATTRIBUTES|FILE_READ_EA)) rw_mode = O_RDWR;
         else rw_mode = O_WRONLY;
     }
     else rw_mode = O_RDONLY;
@@ -1953,7 +1954,8 @@ struct fd *open_fd( struct fd *root, const char *name, struct unicode_str nt_nam
         /* if we tried to open a directory for write access, retry read-only */
         if (errno == EISDIR)
         {
-            if ((access & FILE_UNIX_WRITE_ACCESS) || (flags & O_CREAT))
+            if ((access & (FILE_WRITE_DATA|FILE_APPEND_DATA|FILE_WRITE_ATTRIBUTES|FILE_WRITE_EA)) ||
+                (flags & O_CREAT))
                 fd->unix_fd = open( name, O_RDONLY | (flags & ~(O_TRUNC | O_CREAT | O_EXCL)), *mode );
         }

diff --git a/server/file.h b/server/file.h
index 210fcec5e78..e97c9cc13fd 100644
--- a/server/file.h
+++ b/server/file.h
@@ -257,13 +257,6 @@ static inline int async_queued( struct async_queue *queue )
     return !list_empty( &queue->queue );
 }

-
-/* access rights that require Unix read permission */
-#define FILE_UNIX_READ_ACCESS (FILE_READ_DATA|FILE_READ_ATTRIBUTES|FILE_READ_EA)
-
-/* access rights that require Unix write permission */
-#define FILE_UNIX_WRITE_ACCESS (FILE_WRITE_DATA|FILE_APPEND_DATA|FILE_WRITE_ATTRIBUTES|FILE_WRITE_EA)
-
 /* magic file access rights for mappings */
 #define FILE_MAPPING_IMAGE  0x80000000  /* set for SEC_IMAGE mappings */
 #define FILE_MAPPING_WRITE  0x40000000  /* set for writable shared mappings */

[PATCH v7 3/3] server: Allow to open files without permission bits.

[Joel Holdsworth]

From: Joel Holdsworth

The Msys2 port of the pacman package manager creates a read-only lock file during package
installation. When it is time to delete this file, pacman calls the Msys2 implementation of
unlink(2). This function is implemented in msys2-runtime:

https://github.com/msys2/msys2-runtime/blob/msys2-3.4.3/winsup/cygwin/syscal...

A similar implementation exists in Cygwin:

https://www.cygwin.com/git/?p=newlib-cygwin.git;a=blob;f=winsup/cygwin/sysca...

The implementation works by first opening the file using NtOpenFile with FILE_WRITE_ATTRIBUTES
permissions only, it then clears the FILE_ATTRIBUTE_READONLY flag using NtSetAttributesFile, then
repoens the file with DELETE permissions.

Wine uses POSIX file permissions as one its possible methods of storing the READONLY DOS attribute
in addition to storing attributes in Samba-formatted user extentded attributes.

Files that are opened with FILE_WRITE_ATTRIBUTES must be opened with O_WRONLY or O_RDWR, because
write access is required to modify the extended attribute. However, this will fail if the POSIX file
permissions are set to read-only.

This patch solves the problem by temporarily modifying the file access permissions to enable it to
be opened before restoring the permissions to their previous values.

There are risks involved in doing this. Modifying the file permissions is a 4-step process:

1. stat - read the existing access flags
  2. chmod - modify to allow write permission.
  3. open - attempt to open the file.
  4. fchmod/chmod - restore the access flags using the fd if possible.

The process involves resolving the file path at least 3 times, which is somewhat vulnerable to
TOCTOU race conditions. However, the timing will be very brief, and this process occurs in server
meaning that other Wine threads will be excluded. Therefore, the impact is judged to be low.

This patch was derived by the following wine-staging patches:

* server-File_Permissions/0007-server-FILE_WRITE_ATTRIBUTES-should-succeed-for-read.patch
  * server-File_Permissions/0002-server-Allow-to-open-files-without-any-permission-bi.patch

Wine-Bug: https://bugs.winehq.org/show_bug.cgi?id=50771
Co-authored-by: Sebastian Lackner 
Signed-off-by: Joel Holdsworth 
---
 dlls/advapi32/tests/security.c | 31 ++++++++++++-----------------
 dlls/ntdll/tests/file.c        | 36 ++++++++++++++--------------------
 server/fd.c                    | 32 ++++++++++++++++++++++++++++--
 3 files changed, 57 insertions(+), 42 deletions(-)

diff --git a/dlls/advapi32/tests/security.c b/dlls/advapi32/tests/security.c
index 8b4a868ee11..c994781d0e2 100644
--- a/dlls/advapi32/tests/security.c
+++ b/dlls/advapi32/tests/security.c
@@ -3784,15 +3784,12 @@ static void test_CreateDirectoryA(void)
                                    (PSID *)&owner, NULL, &pDacl, NULL, &pSD);
     todo_wine
     ok(error == ERROR_SUCCESS, "GetNamedSecurityInfo failed with error %ld\n", error);
-    if (error == ERROR_SUCCESS)
-    {
-        bret = GetAclInformation(pDacl, &acl_size, sizeof(acl_size), AclSizeInformation);
-        ok(bret, "GetAclInformation failed\n");
-        todo_wine
-        ok(acl_size.AceCount == 0, "GetAclInformation returned unexpected entry count (%ld != 0).\n",
-                                   acl_size.AceCount);
-        LocalFree(pSD);
-    }
+    bret = GetAclInformation(pDacl, &acl_size, sizeof(acl_size), AclSizeInformation);
+    ok(bret, "GetAclInformation failed\n");
+    todo_wine
+    ok(acl_size.AceCount == 0, "GetAclInformation returned unexpected entry count (%ld != 0).\n",
+                               acl_size.AceCount);
+    LocalFree(pSD);
     CloseHandle(hTemp);

/* Test inheritance of ACLs in NtCreateFile without security descriptor */
@@ -3861,17 +3858,13 @@ static void test_CreateDirectoryA(void)
     error = pGetNamedSecurityInfoA(tmpfile, SE_FILE_OBJECT,
                                    OWNER_SECURITY_INFORMATION | DACL_SECURITY_INFORMATION,
                                    (PSID *)&owner, NULL, &pDacl, NULL, &pSD);
-    todo_wine
     ok(error == ERROR_SUCCESS, "GetNamedSecurityInfo failed with error %ld\n", error);
-    if (error == ERROR_SUCCESS)
-    {
-        bret = GetAclInformation(pDacl, &acl_size, sizeof(acl_size), AclSizeInformation);
-        ok(bret, "GetAclInformation failed\n");
-        todo_wine
-        ok(acl_size.AceCount == 0, "GetAclInformation returned unexpected entry count (%ld != 0).\n",
-                                   acl_size.AceCount);
-        LocalFree(pSD);
-    }
+    bret = GetAclInformation(pDacl, &acl_size, sizeof(acl_size), AclSizeInformation);
+    ok(bret, "GetAclInformation failed\n");
+    todo_wine
+    ok(acl_size.AceCount == 0, "GetAclInformation returned unexpected entry count (%ld != 0).\n",
+                               acl_size.AceCount);
+    LocalFree(pSD);
     CloseHandle(hTemp);

done:
diff --git a/dlls/ntdll/tests/file.c b/dlls/ntdll/tests/file.c
index 57dee54076a..fb02fd4a527 100644
--- a/dlls/ntdll/tests/file.c
+++ b/dlls/ntdll/tests/file.c
@@ -5318,33 +5318,27 @@ static void test_file_readonly_access(void)

/* NtOpenFile FILE_WRITE_ATTRIBUTES */
     status = pNtOpenFile(&handle, FILE_WRITE_ATTRIBUTES,  &attr, &io, default_sharing, FILE_NON_DIRECTORY_FILE);
-    todo_wine ok(status == STATUS_SUCCESS, "got %#lx\n", status);
-
-    if (status == STATUS_SUCCESS) {
-        memset(&fbi, 0, sizeof(fbi));
-        fbi.FileAttributes = FILE_ATTRIBUTE_NORMAL;
-        U(io).Status = 0xdeadbeef;
-        status = pNtSetInformationFile(handle, &io, &fbi, sizeof fbi, FileBasicInformation);
-        todo_wine ok(status == STATUS_SUCCESS, "can't set system attribute, NtSetInformationFile returned %#lx\n",
-                     status);
-        todo_wine ok(U(io).Status == STATUS_SUCCESS, "can't set system attribute, io.Status is %lx\n",
-                     U(io).Status);
-
-        fbi.FileAttributes = FILE_ATTRIBUTE_READONLY;
-        U(io).Status = 0xdeadbeef;
-        status = pNtSetInformationFile(handle, &io, &fbi, sizeof fbi, FileBasicInformation);
-        todo_wine ok(status == STATUS_SUCCESS, "can't set system attribute, NtSetInformationFile returned %#lx\n",
-                     status);
-        todo_wine ok(U(io).Status == STATUS_SUCCESS, "can't set system attribute, io.Status is %lx\n",
-                     U(io).Status);
-    }
+    ok(status == STATUS_SUCCESS, "got %#lx\n", status);
+
+    memset(&fbi, 0, sizeof(fbi));
+    fbi.FileAttributes = FILE_ATTRIBUTE_NORMAL;
+    U(io).Status = 0xdeadbeef;
+    status = pNtSetInformationFile(handle, &io, &fbi, sizeof fbi, FileBasicInformation);
+    ok(status == STATUS_SUCCESS, "can't set system attribute, NtSetInformationFile returned %#lx\n", status);
+    ok(U(io).Status == STATUS_SUCCESS, "can't set system attribute, io.Status is %lx\n", U(io).Status);
+
+    fbi.FileAttributes = FILE_ATTRIBUTE_READONLY;
+    U(io).Status = 0xdeadbeef;
+    status = pNtSetInformationFile(handle, &io, &fbi, sizeof fbi, FileBasicInformation);
+    ok(status == STATUS_SUCCESS, "can't set system attribute, NtSetInformationFile returned %#lx\n", status);
+    ok(U(io).Status == STATUS_SUCCESS, "can't set system attribute, io.Status is %lx\n", U(io).Status);

CloseHandle(handle);

/* NtOpenFile FILE_READ_ATTRIBUTES and FILE_WRITE_ATTRIBUTES */
     status = pNtOpenFile(&handle, FILE_READ_ATTRIBUTES|FILE_WRITE_ATTRIBUTES,  &attr, &io,
                          default_sharing, FILE_NON_DIRECTORY_FILE);
-    todo_wine ok(status == STATUS_SUCCESS, "got %#lx\n", status);
+    ok(status == STATUS_SUCCESS, "got %#lx\n", status);
     CloseHandle(handle);

/* NtCreateFile FILE_GENERIC_WRITE */
diff --git a/server/fd.c b/server/fd.c
index 839a1dec914..9a4f2297329 100644
--- a/server/fd.c
+++ b/server/fd.c
@@ -1899,6 +1899,7 @@ struct fd *open_fd( struct fd *root, const char *name, struct unicode_str nt_nam
     int root_fd = -1;
     int rw_mode;
     char *path;
+    int mode_assigned = 0;

if (((options & FILE_DELETE_ON_CLOSE) && !(access & DELETE)) ||
         ((options & FILE_DIRECTORY_FILE) && (flags & O_TRUNC)))
@@ -1958,6 +1959,29 @@ struct fd *open_fd( struct fd *root, const char *name, struct unicode_str nt_nam
                 (flags & O_CREAT))
                 fd->unix_fd = open( name, O_RDONLY | (flags & ~(O_TRUNC | O_CREAT | O_EXCL)), *mode );
         }
+        else if (errno == EACCES)
+        {
+            /* try to change permissions temporarily to open a file descriptor */
+            if ((access & FILE_WRITE_ATTRIBUTES) &&
+                !(access & (FILE_WRITE_DATA | FILE_APPEND_DATA | FILE_WRITE_EA |
+                    FILE_READ_DATA | FILE_READ_EA | DELETE)) &&
+                !stat( name, &st ) && st.st_uid == getuid() &&
+                !chmod( name, st.st_mode | S_IRUSR ))
+            {
+                fd->unix_fd = open( name, O_RDONLY | (flags & ~(O_TRUNC | O_CREAT | O_EXCL)), *mode );
+                if (fd->unix_fd == -1)
+                    chmod( name, st.st_mode );
+                else
+                    fchmod( fd->unix_fd, st.st_mode );
+                *mode = st.st_mode;
+                mode_assigned = 1;
+            }
+            else
+            {
+                set_error( STATUS_ACCESS_DENIED );
+                goto error;
+            }
+        }

if (fd->unix_fd == -1)
         {
@@ -1981,8 +2005,12 @@ struct fd *open_fd( struct fd *root, const char *name, struct unicode_str nt_nam
     closed_fd->unix_fd = fd->unix_fd;
     closed_fd->unlink = 0;
     closed_fd->unix_name = fd->unix_name;
-    fstat( fd->unix_fd, &st );
-    *mode = st.st_mode;
+
+    if (!mode_assigned)
+    {
+        fstat( fd->unix_fd, &st );
+        *mode = st.st_mode;
+    }

/* only bother with an inode for normal files and directories */
     if (S_ISREG(st.st_mode) || S_ISDIR(st.st_mode))

[PATCH v7 3/3] server: Allow to open files without permission bits.

[Marvin]

Hi,

It looks like your patch introduced the new failures shown below.
Please investigate and fix them before resubmitting your patch.
If they are not new, fixing them anyway would help a lot. Otherwise
please ask for the known failures list to be updated.

The tests also ran into some preexisting test failures. If you know how
to fix them that would be helpful. See the TestBot job for the details:

The full results can be found at:
https://testbot.winehq.org/JobDetails.pl?Key=131136

Your paranoid android.

=== debian11 (32 bit report) ===

advapi32:
Unhandled exception: page fault on read access to 0x0015597c in 32-bit code (0x7bc58290).

kernel32:
directory.c:221: Test failed: CreateDirectoryA(C:\users\winetest\Temp\Please Remove Me) failed err=183

=== debian11 (32 bit ar:MA report) ===

advapi32:
Unhandled exception: page fault on read access to 0x00156046 in 32-bit code (0x7bc58290).

=== debian11 (32 bit de report) ===

advapi32:
security.c:3861: Test failed: GetNamedSecurityInfo failed with error 5

=== debian11 (32 bit fr report) ===

advapi32:
Unhandled exception: page fault on read access to 0x00156046 in 32-bit code (0x7bc58290).

=== debian11 (32 bit he:IL report) ===

advapi32:
Unhandled exception: page fault on read access to 0x00156046 in 32-bit code (0x7bc58290).

=== debian11 (32 bit hi:IN report) ===

advapi32:
Unhandled exception: page fault on read access to 0x00156046 in 32-bit code (0x7bc58290).

=== debian11 (32 bit ja:JP report) ===

advapi32:
Unhandled exception: page fault on read access to 0x00152093 in 32-bit code (0x7bc58290).

=== debian11 (32 bit zh:CN report) ===

advapi32:
Unhandled exception: page fault on read access to 0x00152093 in 32-bit code (0x7bc58290).

=== debian11b (32 bit WoW report) ===

advapi32:
security.c:3861: Test failed: GetNamedSecurityInfo failed with error 5

=== debian11b (64 bit WoW report) ===

advapi32:
security.c:3861: Test failed: GetNamedSecurityInfo failed with error 5

[Joel Holdsworth (＠jhol)]

Ok - I've updated the patch set to adopt the design from (2a) in my above comment. Any thoughts?