Re: kernel32: Specify the access rights when creating a file object.
Dmitry Timoshkov <dmitry(a)baikal.ru> writes:
While investigating how to fix the file section access tests in kernel32 I've found that some places in Wine deliberately create objects with access rights set to 0, that leads to creation of potentially not accessible objects.
Many of these are deliberate. You'll need test cases to show that you can require more permissions. -- Alexandre Julliard julliard(a)winehq.org
Alexandre Julliard <julliard(a)winehq.org> wrote:
While investigating how to fix the file section access tests in kernel32 I've found that some places in Wine deliberately create objects with access rights set to 0, that leads to creation of potentially not accessible objects.
Many of these are deliberate. You'll need test cases to show that you can require more permissions.
In the most cases these patches just add the access rights appropriate for particular calls instead of assuming some default ones, that should be a good thing to do from a security point of view. Test cases are needed, but only to figure out what actual default permissions are provided for 0 access, and for file sections the test already exists and shows that defaults access is 0 and a not accessible object as a result. Creating objects with access rights set to 0 should not be used, and considered a bad practice in general IMO. -- Dmitry.
Alexandre Julliard <julliard(a)winehq.org> wrote:
While investigating how to fix the file section access tests in kernel32 I've found that some places in Wine deliberately create objects with access rights set to 0, that leads to creation of potentially not accessible objects.
Many of these are deliberate. You'll need test cases to show that you can require more permissions.
In the most cases these patches just add the access rights appropriate for particular calls instead of assuming some default ones, that should be a good thing to do from a security point of view. Test cases are needed, but only to figure out what actual default permissions are provided for 0 access, and for file sections the test already exists and shows that defaults access is 0 and a not accessible object as a result. Creating objects with access rights set to 0 should not be used, and considered a bad practice in general IMO.
I still would like to see your opinion on the above. I see that all these patches are marked now as 'Needs tests'. What exactly needs tests for cases 1. when a file is being opened for reading use access GENERIC_READ instead of 0? 2. symblic link object opened for deletion use access DELETE instead of 0? 3. mapping opject created with GENERIC_READ|GENERIC_WRITE (just like in another code patch a couple of lines earlier)? 4. registry key objects are opened with particular access right like KEY_QUERY_VALUE for reading or DELETE for deletion instead of 0? The cases listed above are at least a non-consistency if not the bugs IMO, requring the tests for them looks strange to me. -- Dmitry.
Dmitry Timoshkov <dmitry(a)baikal.ru> writes:
Alexandre Julliard <julliard(a)winehq.org> wrote:
While investigating how to fix the file section access tests in kernel32 I've found that some places in Wine deliberately create objects with access rights set to 0, that leads to creation of potentially not accessible objects.
Many of these are deliberate. You'll need test cases to show that you can require more permissions.
In the most cases these patches just add the access rights appropriate for particular calls instead of assuming some default ones, that should be a good thing to do from a security point of view. Test cases are needed, but only to figure out what actual default permissions are provided for 0 access, and for file sections the test already exists and shows that defaults access is 0 and a not accessible object as a result. Creating objects with access rights set to 0 should not be used, and considered a bad practice in general IMO.
I still would like to see your opinion on the above.
I see that all these patches are marked now as 'Needs tests'. What exactly needs tests for cases 1. when a file is being opened for reading use access GENERIC_READ instead of 0? 2. symblic link object opened for deletion use access DELETE instead of 0? 3. mapping opject created with GENERIC_READ|GENERIC_WRITE (just like in another code patch a couple of lines earlier)? 4. registry key objects are opened with particular access right like KEY_QUERY_VALUE for reading or DELETE for deletion instead of 0?
The cases listed above are at least a non-consistency if not the bugs IMO, requring the tests for them looks strange to me.
Requiring extra access rights can break things, especially in such low-level functions. It's not just a harmless cleanup, so you need to either show an app that requires it, or provide a test case that demonstrates that the extra accesses are required on Windows too. -- Alexandre Julliard julliard(a)winehq.org
Alexandre Julliard <julliard(a)winehq.org> wrote:
Requiring extra access rights can break things, especially in such low-level functions. It's not just a harmless cleanup, so you need to either show an app that requires it, or provide a test case that demonstrates that the extra accesses are required on Windows too.
At least one such a test case exists for the file mappings, and actually making it pass has motivated me for this kind of patches. It shows that a mapping created with access 0 is not accessible. Since all the objects on the server side share a single access check mechanism it's impossible to make one test pass without either changing that single mechanism, or creating separate access checks for every type of an object. -- Dmitry.
Dmitry Timoshkov <dmitry(a)baikal.ru> writes:
Alexandre Julliard <julliard(a)winehq.org> wrote:
Requiring extra access rights can break things, especially in such low-level functions. It's not just a harmless cleanup, so you need to either show an app that requires it, or provide a test case that demonstrates that the extra accesses are required on Windows too.
At least one such a test case exists for the file mappings, and actually making it pass has motivated me for this kind of patches. It shows that a mapping created with access 0 is not accessible. Since all the objects on the server side share a single access check mechanism it's impossible to make one test pass without either changing that single mechanism, or creating separate access checks for every type of an object.
You shouldn't need to change anything to the access check mechanism. An access of 0 of course won't satisfy places that require a specific right. If there are places that don't require a right but should, then you can fix that, with test cases. If fixing that requires the corresponding allocation to add some rights, you can do that at the same time. -- Alexandre Julliard julliard(a)winehq.org
participants (2)
-
Alexandre Julliard -
Dmitry Timoshkov