Re: [PATCH 3/4] advapi32: Add DACL inheritance support in SetSecurityInfo
I do not believe this is the correct place to make this change. Inheritance still needs to occur even when SetSecurityInfo is not used, which pretty much means it has to be within the wineserver itself so that the same code can be used for CreateFile. The code I put together for this (as written it is somewhat dependent on my other ACL code) can be found here: https://github.com/wine-compholio/wine-staging/tree/master/patches/server-In... The first patch is for CreateFile and the second one is for SetSecurityInfo. I'm happy to try and change this to help you solve your problem without my other ACL code, it's been on my TODO list anyway - I've just been too busy with work lately :/ Best, Erich
On 03/26/15 17:59, Erich E. Hoover wrote:
I do not believe this is the correct place to make this change. Inheritance still needs to occur even when SetSecurityInfo is not used, which pretty much means it has to be within the wineserver itself so that the same code can be used for CreateFile. The code I put together for this (as written it is somewhat dependent on my other ACL code) can be found here: https://github.com/wine-compholio/wine-staging/tree/master/patches/server-In...
The first patch is for CreateFile and the second one is for SetSecurityInfo. I'm happy to try and change this to help you solve your problem without my other ACL code, it's been on my TODO list anyway - I've just been too busy with work lately :/
On windows the inheritance code is inside ntmarta.dll (according to some Microsoft docs). Probably Microsoft uses ntmarta.dll inside advapi. But I don't think we want to be compatible with native dlls in this case. 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 see 3 ways of implementing the DACLs inheritance: 1) add a server call that does the inheritance in this case there's no code duplication between CreateFile and SetFileInformation, also it's possible to skip the inheritance when NtSetSecurityObject is used directly 2) duplicate inheritance code inside advapi and ntdll/server I think this is the best solution. The code that merges 2 DACLs has only ~36 lines in my implementation. The code for getting parent directory DACL is also not long (~30 lines). This code may get longer in future (when we have complete support for security descriptors). 3) implement something like ntmarta.dll This will need much more testing. I'm also not sure if it can be used from ntdll at all. What do you think about it? Thanks, Piotr P.S. this patches fixes some real installers (TEMSInvestigation 16.1.1 (fails), Uru - Ages Beyond Myst (fails to remove some temporary files))
On 26.03.2015 19:10, Piotr Caban 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.
Hello Piotr, does that mean you reviewed our code? Would you please give some more details to us about the error you found? Feel free to open a bug report on bugs.wine-staging.com with your results. BTW: The installers are also fixed with our patchsets, but I am sure you will agree, that this should not influence the decision if the patches should be applied or not ;) Thanks in advance, Sebastian
Hi, On 26/03/15 19:21, Sebastian Lackner wrote:
On 26.03.2015 19:10, Piotr Caban 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. does that mean you reviewed our code? Would you please give some more details to us about the error you found? Feel free to open a bug report on bugs.wine-staging.com with your results. I've not really reviewed it, I've only skimmed the diff. According to my testing NtSetSecurityObject is only setting supplied DACL, it's not adding inherited entries.
I also haven't seen the implementation from wine-staging tree earlier (I think these patches were not mentioned anywhere on wine bugzilla/were never sent to wine but maybe I've missed something). Thanks, Piotr
On Thu, Mar 26, 2015 at 3:45 PM, Piotr Caban <piotr.caban(a)gmail.com> wrote:
Hi,
On 26/03/15 19:21, Sebastian Lackner wrote:
On 26.03.2015 19:10, Piotr Caban 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.
does that mean you reviewed our code? Would you please give some more details to us about the error you found? Feel free to open a bug report on bugs.wine-staging.com with your results.
I've not really reviewed it, I've only skimmed the diff. According to my testing NtSetSecurityObject is only setting supplied DACL, it's not adding inherited entries.
I also haven't seen the implementation from wine-staging tree earlier (I think these patches were not mentioned anywhere on wine bugzilla/were never sent to wine but maybe I've missed something).
Thanks, Piotr
https://bugs.winehq.org/show_bug.cgi?id=33576#c10 -- -Austin
participants (4)
-
Austin English -
Erich E. Hoover -
Piotr Caban -
Sebastian Lackner