[Bug 50771] New: CreateFile(FILE_WRITE_ATTRIBUTES) on readonly file fails
https://bugs.winehq.org/show_bug.cgi?id=50771 Bug ID: 50771 Summary: CreateFile(FILE_WRITE_ATTRIBUTES) on readonly file fails Product: Wine Version: unspecified Hardware: x86-64 OS: Linux Status: UNCONFIRMED Severity: normal Priority: P2 Component: kernel32 Assignee: wine-bugs(a)winehq.org Reporter: martin(a)martin.st Distribution: --- Created attachment 69563 --> https://bugs.winehq.org/attachment.cgi?id=69563 Testcase When a file is readonly, one should be able to open it with CreateFile(FILE_WRITE_ATTRIBUTES) in order to change its permissions to writable via SetFileInformationByHandle(FileBasicInfo), however the CreateFile call fails. -- Do not reply to this email, post in Bugzilla using the above URL to reply. You are receiving this mail because: You are watching all bug changes.
https://bugs.winehq.org/show_bug.cgi?id=50771 Zebediah Figura <z.figura12(a)gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Version|unspecified |6.3 -- Do not reply to this email, post in Bugzilla using the above URL to reply. You are receiving this mail because: You are watching all bug changes.
https://bugs.winehq.org/show_bug.cgi?id=50771 Joel Holdsworth <joel(a)airwebreathe.org.uk> changed: What |Removed |Added ---------------------------------------------------------------------------- CC| |joel(a)airwebreathe.org.uk -- Do not reply to this email, post in Bugzilla using the above URL to reply. You are receiving this mail because: You are watching all bug changes.
https://bugs.winehq.org/show_bug.cgi?id=50771 --- Comment #1 from Joel Holdsworth <joel(a)airwebreathe.org.uk> --- Created attachment 72998 --> https://bugs.winehq.org/attachment.cgi?id=72998 Test case for FILE_WRITE_ATTRIBUTES on a read-only file I stumbled into the same issue with Msys2's implementation of the unlink() syscall when applied to read-only files. I suspect this also affect Cygwin. It's the same issue, except that they call into the underlying NtCreateFile API. Here is a test case for the Wine test suite that shows the issue. The issue seems to be related to the open_fd function in server/fd.c . If the access flags are in the set defined by FILE_UNIX_WRITE_ACCESS (FILE_WRITE_DATA, FILE_APPEND_DATA, FILE_WRITE_ATTRIBUTES, FILE_WRITE_EA), then the function attempts to open the file with O_RDWR or O_WRONLY - which is going to fail on a read-only file. So we need to open the file to get an fd, which later gets passed into fchmod if the application decides to modify the file permissions. Is it possible to open() a file with neither read or write access? -- Do not reply to this email, post in Bugzilla using the above URL to reply. You are receiving this mail because: You are watching all bug changes.
https://bugs.winehq.org/show_bug.cgi?id=50771 --- Comment #2 from Joel Holdsworth <joel(a)airwebreathe.org.uk> --- Created attachment 72999 --> https://bugs.winehq.org/attachment.cgi?id=72999 Test case for NtCreateFile with FILE_WRITE_ATTRIBUTES access -- Do not reply to this email, post in Bugzilla using the above URL to reply. You are receiving this mail because: You are watching all bug changes.
https://bugs.winehq.org/show_bug.cgi?id=50771 --- Comment #3 from Joel Holdsworth <joel(a)airwebreathe.org.uk> --- The man page doesn't sound very promising: The argument flags must include one of the following access modes: O_RDONLY, O_WRONLY, or O_RDWR. These request opening the file read-only, write-only, or read/write, respectively. -- Do not reply to this email, post in Bugzilla using the above URL to reply. You are receiving this mail because: You are watching all bug changes.
https://bugs.winehq.org/show_bug.cgi?id=50771 Zeb Figura <z.figura12(a)gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Summary|CreateFile(FILE_WRITE_ATTRI |msys2 unlink() fails on |BUTES) on readonly file |read-only files [needs |fails |CreateFile(FILE_WRITE_ATTRI | |BUTES)] CC| |z.figura12(a)gmail.com Component|kernel32 |ntdll --- Comment #4 from Zeb Figura <z.figura12(a)gmail.com> --- Linux has O_PATH, but that's not portable. We may need to do it without actually opening the fd. There's a wine-staging patch for this: <server-File_Permissions/0002-server-Allow-to-open-files-without-any-permission-bi.patch>. Like many old wine-staging patches it lacks documentation; probably this is what it was originally written for. -- Do not reply to this email, post in Bugzilla using the above URL to reply. You are receiving this mail because: You are watching all bug changes.
https://bugs.winehq.org/show_bug.cgi?id=50771 --- Comment #5 from Joel Holdsworth <joel(a)airwebreathe.org.uk> --- https://github.com/wine-staging/wine-staging/blob/master/patches/server-File... That patch looks extremely promising. My only comment on it is that I don't think FILE_WRITE_ATTRIBUTES (and perhaps FILE_WRITE_EA?) should be in the FILE_UNIX_WRITE_ACCESS in any case. If the application requests to write a file's attributes, that is no reason to attempt to open the file data. This patch relies on open() failing but I wonder if FILE_WRITE_ATTRIBUTES should be handled differently in the first place. Something like: 1. open() the file if the application requested FILE_READ_DATA, FILE_WRITE_DATA or FILE_APPEND_DATA 2. If the application requested FILE_WRITE_ATTRIBUTES, FILE_UNIX_READ_ACCESS or DELETE, then stat() the file. Open it with whatever permissions are avaiable. 3. If the file is non-read and non-write, then temporarily chmod() and open(). On Linux, there would be a benefit to handling #2 and #3 through a seperate code path using O_PATH, because it would remove the vulnerability to TOCTOU (time-of-check to time-of-use) issues. I might find time to attempt these changes, but what is the policy for evolving other people's patches? Is there a label I can put on for dual authorship? Would I make my changes as a seperate patch? Or just rewrite the patch, and submit it as my own work? -- Do not reply to this email, post in Bugzilla using the above URL to reply. You are receiving this mail because: You are watching all bug changes.
https://bugs.winehq.org/show_bug.cgi?id=50771 --- Comment #6 from Joel Holdsworth <joel(a)airwebreathe.org.uk> --- This patch is relevant also: https://github.com/wine-staging/wine-staging/blob/master/patches/server-File... Also there is at least one patch containing test cases: https://github.com/wine-staging/wine-staging/blob/master/patches/server-File... This looks like good stuff. What is the process for submitting these? If I want these to go upstream, should I submit a GitLab MR? Or ask the authors or wine-staging maintainers to submit? Is the patch in staging because it was once rejected? Is there any way to find out why it wasn't approved years ago? -- Do not reply to this email, post in Bugzilla using the above URL to reply. You are receiving this mail because: You are watching all bug changes.
https://bugs.winehq.org/show_bug.cgi?id=50771 --- Comment #7 from Zeb Figura <z.figura12(a)gmail.com> --- The thing that seems questionable to me about the wine-staging patch is that it changes permissions. I'm not sure we want to do that, especially given the (theoretical) prospect that the user might not be able to do that anyway. (Is it possible to omit FILE_SHARE_* flags if you don't have the corresponding access flags? Do we need to respect that? Seems like a security hole on Windows if so...) (In reply to Joel Holdsworth from comment #5)
I might find time to attempt these changes, but what is the policy for evolving other people's patches? Is there a label I can put on for dual authorship? Would I make my changes as a seperate patch? Or just rewrite the patch, and submit it as my own work?
The general practice is to either submit the patch under the name of the original author, or write a new patch with something like "Based on a patch by X" in the subject. (In reply to Joel Holdsworth from comment #6)
This looks like good stuff. What is the process for submitting these? If I want these to go upstream, should I submit a GitLab MR? Or ask the authors or wine-staging maintainers to submit? Is the patch in staging because it was once rejected? Is there any way to find out why it wasn't approved years ago?
The majority of patches in wine-staging were written by the original wine-staging maintainers, Michael Müller and Sebastian Lackner. While they did submit many patches upstream (successfully or unsuccessfully), they also wrote many patches which were never submitted upstream. They also largely disappeared several years ago, and attempts to contact them are rarely successful. This patch in particular does seem to have been sent upstream in April 2015, but judging from wine-patches and wine-devel archives it never received an on-list review. -- Do not reply to this email, post in Bugzilla using the above URL to reply. You are receiving this mail because: You are watching all bug changes.
https://bugs.winehq.org/show_bug.cgi?id=50771 --- Comment #8 from Joel Holdsworth <joel(a)airwebreathe.org.uk> --- This patch set was originally submitted as a fix for this bug: https://gitlab.winehq.org/jhol/wine/-/commits/lock-fix-3 The patch set adds support for modifying the file attributes of read-only files, by implementing a temporary chmod. However, reviewers were not happy with this design. https://gitlab.winehq.org/wine/wine/-/merge_requests/1895 The Msys2 and Cygwin unlink() implementation have an additional code path that can use NtSetInformationFile with FileDispositionInformationEx and FILE_DISPOSITION_IGNORE_READONLY_ATTRIBUTE instead. A patch set to implement support for these features has been submitted here: https://gitlab.winehq.org/wine/wine/-/merge_requests/3073 -- Do not reply to this email, post in Bugzilla using the above URL to reply. You are receiving this mail because: You are watching all bug changes.
https://bugs.winehq.org/show_bug.cgi?id=50771 Alexandre Julliard <julliard(a)winehq.org> changed: What |Removed |Added ---------------------------------------------------------------------------- Fixed by SHA1| |1ccd037e00c19c40de0de08f0f6 | |e699622fa4c4f Resolution|--- |FIXED Status|UNCONFIRMED |RESOLVED --- Comment #9 from Alexandre Julliard <julliard(a)winehq.org> --- Reported fixed. -- Do not reply to this email, post in Bugzilla using the above URL to reply. You are receiving this mail because: You are watching all bug changes.
https://bugs.winehq.org/show_bug.cgi?id=50771 Alexandre Julliard <julliard(a)winehq.org> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|RESOLVED |CLOSED --- Comment #10 from Alexandre Julliard <julliard(a)winehq.org> --- Closing bugs fixed in 8.13. -- Do not reply to this email, post in Bugzilla using the above URL to reply. You are receiving this mail because: You are watching all bug changes.
http://bugs.winehq.org/show_bug.cgi?id=50771 Keno Fischer <keno(a)juliahub.com> changed: What |Removed |Added ---------------------------------------------------------------------------- CC| |keno(a)juliahub.com --- Comment #11 from Keno Fischer <keno(a)juliahub.com> --- I ran into this bug with libuv (I/O library used by e.g. Node.JS and Julia), which also uses it for much the same purpose: https://github.com/libuv/libuv/blob/9424da3e46f485d103a40e0a9b4ba1e8bfea4c9c.... Now, as of last year, it can also use FILE_DISPOSITION_IGNORE_READONLY_ATTRIBUTE (though it still tries to request FILE_WRITE_ATTRIBUTES even in this case, which should be unnecessary). I will try to submit a PR to libuv to remove this to avoid this bug. However, it does also have another method just to toggle files between read-only and read-write https://github.com/libuv/libuv/blob/9424da3e46f485d103a40e0a9b4ba1e8bfea4c9c..., which uses the same pattern. Is there any guidance for how to avoid this issue in that situation? -- Do not reply to this email, post in Bugzilla using the above URL to reply. You are receiving this mail because: You are watching all bug changes.
participants (1)
-
WineHQ Bugzilla