When a client requests a Windows ACL, we always ask for the DACL and SACL from the disk representation - even if the client didn't ask for this. This means the security descriptor types we return always contain SEC_DESC_DACL_PRESENT or SEC_DESC_SACL_PRESENT in the type field, even if the DACL or SACL pointer is NULL. This can cause WindowsXP clients to set invalid security descriptors back onto a file when it is being copied by the shell. I have a log file showing this, but will not post without permission from the OEM. Patch for 3.5.x and 3.6.x to follow.
Hmmm. Didn't add the patch to this did I :-). Turns out that it wasn't needed to fix the original bug I was trying to fix (bug #8458). I need to test this against a Windows server to see if this bit is stripped out if SECINFO_DACL isn't requested. Jeremy.
Created attachment 7247 [details] git-am fix for 3.6.x. Fix that already went into master. Jeremy.
Created attachment 7248 [details] git-am fix for 3.5.x.
Comment on attachment 7247 [details] git-am fix for 3.6.x. Looks good
Comment on attachment 7247 [details] git-am fix for 3.6.x. hmm, Jeremy please explain why the 3.5 patch is different. Which one is the correct one?
They are both correct for their respective branches. They are different as the code (slightly) diverged between the two. The master back-port (which is what I used for 3.6.x) didn't apply to the 3.5.x patch. The only other change I did in the 3.5.x code was to ensure that we always request: OWNER_SECURITY_INFORMATION | GROUP_SECURITY_INFORMATION | DACL_SECURITY_INFORMATION | SACL_SECURITY_INFORMATION when fetching the ACL in this module (as we already do in the 3.6.x and master code) - the features returned are always filtered at the upper level and I wanted to make sure there's no chance we could ever hash with an incompletely read ACL. Jeremy.
Comment on attachment 7247 [details] git-am fix for 3.6.x. Requesting re-review from metze.
(In reply to comment #6) > They are both correct for their respective branches. They are different as the > code (slightly) diverged between the two. The master back-port (which is what I > used for 3.6.x) didn't apply to the 3.5.x patch. > > The only other change I did in the 3.5.x code was to ensure that we always > request: > > OWNER_SECURITY_INFORMATION | > GROUP_SECURITY_INFORMATION | > DACL_SECURITY_INFORMATION | > SACL_SECURITY_INFORMATION > > when fetching the ACL in this module (as we already do in the 3.6.x and master > code) - the features returned are always filtered at the upper level and I > wanted to make sure there's no chance we could ever hash with an incompletely > read ACL. No, in v3-6-test I see get_parent_acl_common without SECINFO_SACL. metze
(In reply to comment #8) > (In reply to comment #6) > > They are both correct for their respective branches. They are different as the > > code (slightly) diverged between the two. The master back-port (which is what I > > used for 3.6.x) didn't apply to the 3.5.x patch. > > > > The only other change I did in the 3.5.x code was to ensure that we always > > request: > > > > OWNER_SECURITY_INFORMATION | > > GROUP_SECURITY_INFORMATION | > > DACL_SECURITY_INFORMATION | > > SACL_SECURITY_INFORMATION > > > > when fetching the ACL in this module (as we already do in the 3.6.x and master > > code) - the features returned are always filtered at the upper level and I > > wanted to make sure there's no chance we could ever hash with an incompletely > > read ACL. > > No, in v3-6-test I see get_parent_acl_common without SECINFO_SACL. The same for open_acl_common(). I guess we should a similar patch as for 3.5. Where are the similar code parts in master? metze
Oh, that's a bug then (although probably a cosmetic one, not causing any issues). I'll update the fix for 3.6.x. The code paths in master are different. open_acl_common() has been removed there. Jeremy.
Created attachment 7252 [details] git-am fix for 3.6.next. Contains both changes.
Comment on attachment 7252 [details] git-am fix for 3.6.next. Looks good
Comment on attachment 7248 [details] git-am fix for 3.5.x. Looks good
Karolin, please pick for the next releases.
Pushed to v3-5-test and v3-6-test. Closing out bug report. Thanks!