Re: advapi32: Handle NULL DACL in SetSecurityInfo

[Sebastian Lackner]

On 02.04.2015 17:20, Piotr Caban wrote:
> ---
>  dlls/advapi32/security.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)

It doesn't match the Windows behaviour, please add at least a couple of tests.
https://newtestbot.winehq.org/JobDetails.pl?Key=12622&log_204=1#k204

advapi32: Handle NULL DACL in SetSecurityInfo

[Piotr Caban]

Hi,

On 02/04/15 19:42, Sebastian Lackner wrote:
> It doesn't match the Windows behaviour, please add at least a couple 
> of tests. 
> https://newtestbot.winehq.org/JobDetails.pl?Key=12622&log_204=1#k204 
> the patch restores old SetSecurityInfo behavior. It also works around a 
> crash in current implementation when NULL DACL is passed. It's as close 
> to native behavior as it currently can be. Setting NULL DACL means: 
> allow every permission on windows. Your test shows that we should get 
> NULL while asking about DACL but there's no way of correctly supporting 
> it in wine currently (and I'm not sure if it can be cleanly supported in 
> future, as far as I can see nor POSIX ACL, nor NFSv4 ACL supports 
> anything like this).

There's following code in server to handle NULL DACL case:
mode_t sd_to_mode( const struct security_descriptor *sd, const SID *owner )
...
if (present && dacl)
...
else
         /* no ACL means full access rights to anyone */
         new_mode = S_IRWXU | S_IRWXG | S_IRWXO;

Cheers,
Piotr

advapi32: Handle NULL DACL in SetSecurityInfo

[Sebastian Lackner]

On 03.04.2015 00:54, Piotr Caban wrote:
> the patch restores old SetSecurityInfo behavior. It also works around
> a crash in current implementation when NULL DACL is passed. It's as
> close to native behavior as it currently can be. Setting NULL DACL
> means: allow every permission on windows. Your test shows that we
> should get NULL while asking about DACL but there's no way of
> correctly supporting it in wine currently (and I'm not sure if it can
> be cleanly supported in future, as far as I can see nor POSIX ACL,
> nor NFSv4 ACL supports anything like this).

I didn't say that I have any problem with your solution, but I would really like to have all the differences to Windows documented in the form of tests. You probably know even better than me that the current ACL implementation of Wine is still terrible broken in a lot of situations, and its very easy to break things again, while trying to fix a different issue. Honestly, I would have really preferred to have more tests for everything before accepting a huge change like the DACL inheritance patch, but I think we should at least document all future changes a bit better. In this case the test is trivial and just a couple of lines long, so there is no good excuse to leave it out. ;)

Best regards,
Sebastian

advapi32: Handle NULL DACL in SetSecurityInfo

[Erich E. Hoover]

On Thu, Apr 2, 2015 at 4:54 PM, Piotr Caban <piotr(a)codeweavers.com> wrote:
> ...
> the patch restores old SetSecurityInfo behavior. It also works around a
> crash in current implementation when NULL DACL is passed. It's as close to
> native behavior as it currently can be. Setting NULL DACL means: allow every
> permission on windows. Your test shows that we should get NULL while asking
> about DACL but there's no way of correctly supporting it in wine currently
> (and I'm not sure if it can be cleanly supported in future, as far as I can
> see nor POSIX ACL, nor NFSv4 ACL supports anything like this).
> ...

This is one of the reasons why in our implementation we stored the
security descriptor in an extended filesystem attribute.  Samba
encountered the same problems with a lot of the win ACLs, they found
that for some things the only way to provide full support is to store
a perfect copy of the security descriptor.  Doing so works around a
lot of these issues where there's no equivalent Unix permission.

Best,
Erich

advapi32: Handle NULL DACL in SetSecurityInfo

[Piotr Caban]

On 04/03/15 01:56, Erich E. Hoover wrote:
> On Thu, Apr 2, 2015 at 4:54 PM, Piotr Caban <piotr(a)codeweavers.com> wrote:
> > ...
> > the patch restores old SetSecurityInfo behavior. It also works around a
> > crash in current implementation when NULL DACL is passed. It's as close to
> > native behavior as it currently can be. Setting NULL DACL means: allow every
> > permission on windows. Your test shows that we should get NULL while asking
> > about DACL but there's no way of correctly supporting it in wine currently
> > (and I'm not sure if it can be cleanly supported in future, as far as I can
> > see nor POSIX ACL, nor NFSv4 ACL supports anything like this).
> > ...
>
> This is one of the reasons why in our implementation we stored the
> security descriptor in an extended filesystem attribute.  Samba
> encountered the same problems with a lot of the win ACLs, they found
> that for some things the only way to provide full support is to store
> a perfect copy of the security descriptor.  Doing so works around a
> lot of these issues where there's no equivalent Unix permission.

The problem is that reporting permissions that we don't really support 
leads to other problems. Hopefully NFSv4 (richacls) will get popular in 
Linux soon so it will be possible to fix it properly (currently only 
available on OSX and Suse 11.3).

Thanks,
Piotr