Re: [PATCH 3/4] advapi32: Add DACL inheritance support in SetSecurityInfo
On Thu, Mar 26, 2015 at 2:59 PM, Erich E. Hoover <erich.e.hoover(a)wine-staging.com> wrote:
On Thu, Mar 26, 2015 at 12:10 PM, Piotr Caban <piotr.caban(a)gmail.com> wrote:
... The inheritance doesn't occur when function from ntdll are used for setting DACL. This makes your code inside server incorrect. Also your code is not checking handle permissions correctly. If you try to test it keep in mind that if correct inheritance flags are set in security descriptor control field the permissions will be filled when you right-click on file and display security properties on windows.
I don’t have the time to write a test at the moment, but I find it highly unlikely that MS would implement security descriptors in a high-level DLL. If that were the case then an application could easily hijack the API in order to permit access to files that would otherwise be inaccessible. I would guess that UNPROTECTED_DACL_SECURITY_INFORMATION and SE_DACL_AUTO_INHERITED probably enter into this somehow.
Besides that I think we should start implementing stored ACLs first, before even thinking about implementing suitable inheritance mechanisms. I submitted a bunch of patches for this some time ago, but Alexandre wasn’t happy with using extended user attributes to store this information. I’m just not sure that it’s useful to implement inheritance without storing the ACLs, it’ll certainly fix some apps but it will likely break a bunch of others.
Whoops, forgot reply-all - sorry for the duplicate Piotr. Best, Erich
On Thu, Mar 26, 2015 at 2:59 PM, Erich E. Hoover <erich.e.hoover(a)wine-staging.com> wrote:
On Thu, Mar 26, 2015 at 12:10 PM, Piotr Caban <piotr.caban(a)gmail.com> wrote:
... The inheritance doesn't occur when function from ntdll are used for setting DACL. This makes your code inside server incorrect. Also your code is not checking handle permissions correctly. If you try to test it keep in mind that if correct inheritance flags are set in security descriptor control field the permissions will be filled when you right-click on file and display security properties on windows.
I don’t have the time to write a test at the moment, but I find it highly unlikely that MS would implement security descriptors in a high-level DLL. If that were the case then an application could easily hijack the API in order to permit access to files that would otherwise be inaccessible. I would guess that UNPROTECTED_DACL_SECURITY_INFORMATION and SE_DACL_AUTO_INHERITED probably enter into this somehow. I'll send a test that shows the lack of inheritance in NtSetSecurityObject. Of course it's not a problem in the API, if we have rights to change the permissions we can do whatever we want anyway.
Besides that I think we should start implementing stored ACLs first, before even thinking about implementing suitable inheritance mechanisms. I submitted a bunch of patches for this some time ago, but Alexandre wasn’t happy with using extended user attributes to store this information. I’m just not sure that it’s useful to implement inheritance without storing the ACLs, it’ll certainly fix some apps but it will likely break a bunch of others. I don't think complete support for storing security descriptors needs to be done first. This is the step in correct direction.
Thanks, Piotr
participants (2)
-
Erich E. Hoover -
Piotr Caban