[PATCH v7 0/3] MR1895: Fixes for NtOpenFile of read-only files with FILE_WRITE_ATTRIBUTES
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
From: Joel Holdsworth <joel(a)airwebreathe.org.uk> 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 <qhong(a)codeweavers.com> Signed-off-by: Joel Holdsworth <joel(a)airwebreathe.org.uk> --- 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); -- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/1895
From: Joel Holdsworth <joel(a)airwebreathe.org.uk> These defines are counter-productive for code clarity. Signed-off-by: Joel Holdsworth <joel(a)airwebreathe.org.uk> --- 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 */ -- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/1895
From: Joel Holdsworth <joel(a)airwebreathe.org.uk> 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 <sebastian(a)fds-team.de> Signed-off-by: Joel Holdsworth <joel(a)airwebreathe.org.uk> --- 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)) -- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/1895
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
Ok - I've updated the patch set to adopt the design from (2a) in my above comment. Any thoughts? -- https://gitlab.winehq.org/wine/wine/-/merge_requests/1895#note_28206
participants (3)
-
Joel Holdsworth -
Joel Holdsworth (＠jhol)
-
Marvin