Bug 8636 - When returning an ACL without SECINFO_DACL requested, we still set SEC_DESC_DACL_PRESENT in the type field.
When returning an ACL without SECINFO_DACL requested, we still set SEC_DESC_D...
Status: RESOLVED FIXED
Product: Samba 3.6
Classification: Unclassified
Component: File services
3.6.1
All All
: P5 normal
: ---
Assigned To: Karolin Seeger
Samba QA Contact
:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2011-11-30 00:20 UTC by Jeremy Allison
Modified: 2012-01-23 20:40 UTC (History)
1 user (show)

See Also:


Attachments
git-am fix for 3.6.x. (1.59 KB, patch)
2012-01-21 00:22 UTC, Jeremy Allison
no flags Details
git-am fix for 3.5.x. (2.07 KB, patch)
2012-01-21 00:39 UTC, Jeremy Allison
metze: review+
Details
git-am fix for 3.6.next. (2.92 KB, patch)
2012-01-23 19:41 UTC, Jeremy Allison
metze: review+
Details

Note You need to log in before you can comment on or make changes to this bug.
Description Jeremy Allison 2011-11-30 00:20:43 UTC
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.
Comment 1 Jeremy Allison 2012-01-21 00:12:35 UTC
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.
Comment 2 Jeremy Allison 2012-01-21 00:22:17 UTC
Created attachment 7247 [details]
git-am fix for 3.6.x.

Fix that already went into master.

Jeremy.
Comment 3 Jeremy Allison 2012-01-21 00:39:56 UTC
Created attachment 7248 [details]
git-am fix for 3.5.x.
Comment 4 Stefan Metzmacher 2012-01-21 09:56:04 UTC
Comment on attachment 7247 [details]
git-am fix for 3.6.x.

Looks good
Comment 5 Stefan Metzmacher 2012-01-21 09:58:44 UTC
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?
Comment 6 Jeremy Allison 2012-01-23 04:49:31 UTC
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 7 Jeremy Allison 2012-01-23 04:50:24 UTC
Comment on attachment 7247 [details]
git-am fix for 3.6.x.

Requesting re-review from metze.
Comment 8 Stefan Metzmacher 2012-01-23 06:45:10 UTC
(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
Comment 9 Stefan Metzmacher 2012-01-23 06:50:00 UTC
(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
Comment 10 Jeremy Allison 2012-01-23 17:51:58 UTC
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.
Comment 11 Jeremy Allison 2012-01-23 19:41:02 UTC
Created attachment 7252 [details]
git-am fix for 3.6.next.

Contains both changes.
Comment 12 Stefan Metzmacher 2012-01-23 20:30:30 UTC
Comment on attachment 7252 [details]
git-am fix for 3.6.next.

Looks good
Comment 13 Stefan Metzmacher 2012-01-23 20:31:00 UTC
Comment on attachment 7248 [details]
git-am fix for 3.5.x.

Looks good
Comment 14 Stefan Metzmacher 2012-01-23 20:31:42 UTC
Karolin, please pick for the next releases.
Comment 15 Karolin Seeger 2012-01-23 20:40:26 UTC
Pushed to v3-5-test and v3-6-test.
Closing out bug report.

Thanks!