Bug 8811 - sd_has_inheritable_components segfaults on an SD that se_access_check accepts
sd_has_inheritable_components segfaults on an SD that se_access_check accepts
Status: RESOLVED FIXED
Product: Samba 3.6
Classification: Unclassified
Component: File services
3.6.3
All All
: P5 normal
: ---
Assigned To: Karolin Seeger
Samba QA Contact
:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2012-03-13 23:38 UTC by Richard Sharpe
Modified: 2012-06-13 17:58 UTC (History)
0 users

See Also:


Attachments
Proposed patch ? (489 bytes, patch)
2012-03-13 23:48 UTC, Jeremy Allison
no flags Details
Second proposed patch. (1.96 KB, patch)
2012-03-16 22:26 UTC, Jeremy Allison
no flags Details
git-am fix for 3.6.next (3.43 KB, patch)
2012-03-30 19:01 UTC, Jeremy Allison
jra: review? (rsharpe)
Details

Note You need to log in before you can comment on or make changes to this bug.
Description Richard Sharpe 2012-03-13 23:38:48 UTC
In testing a small change to a VFS at Dell I found that the routine libcli/security/secdesc.c:sd_has_inheritable_components chokes on an SD with a null DACL even though libcli/security/access_check.c:se_access_check is perfectly happy to deal with an SD where:

        if ((sd->type & SEC_DESC_DACL_PRESENT) && sd->dacl == NULL) {
                *access_granted = access_desired;
                return NT_STATUS_OK;
        }
Comment 1 Jeremy Allison 2012-03-13 23:48:31 UTC
Created attachment 7385 [details]
Proposed patch ?

Richard, does this fix the issue ?
Comment 2 Richard Sharpe 2012-03-14 01:56:41 UTC
In looking at this:

diff --git a/libcli/security/secdesc.c b/libcli/security/secdesc.c
index fcd0828..84128e4 100644
--- a/libcli/security/secdesc.c
+++ b/libcli/security/secdesc.c
@@ -542,6 +542,10 @@ bool sd_has_inheritable_components(const struct security_descriptor *parent_ctr,
 	unsigned int i;
 	const struct security_acl *the_acl = parent_ctr->dacl;
 
+	if (the_acl == NULL) {
+		return false;
+	}
+
 	for (i = 0; i < the_acl->num_aces; i++) {
 		const struct security_ace *ace = &the_acl->aces[i];
 

Yes, this looks correct.
Comment 3 Richard Sharpe 2012-03-14 20:26:02 UTC
Hmmm, source3/modules/vfs_acl_common.c also seems to have a problem. 

The routine add_directory_inheritable_components will try to add some ACEs to a NULL DACL and segfaults.

Perhaps it should not try to add them to an NULL DACL, and if it wants to then it needs to create one ... but I would prefer that it didn't try to do anything to a NULL DACL because I have done that for a reason. (Upgrading to ACL support from non-ACL support and want something that will give them what they currently have. That is, open access until an Admin goes and puts the hammer down.)
Comment 4 Richard Sharpe 2012-03-15 13:32:12 UTC
(In reply to comment #3)
> Hmmm, source3/modules/vfs_acl_common.c also seems to have a problem. 
> 
> The routine add_directory_inheritable_components will try to add some ACEs to a
> NULL DACL and segfaults.
> 
> Perhaps it should not try to add them to an NULL DACL, and if it wants to then
> it needs to create one ... but I would prefer that it didn't try to do anything
> to a NULL DACL because I have done that for a reason. (Upgrading to ACL support
> from non-ACL support and want something that will give them what they currently
> have. That is, open access until an Admin goes and puts the hammer down.)

Should I create a new bug to deal with this issue?

I can see several approaches here, including a possible module parameter specifying whether or not to add these inheritable ACEs ...
Comment 5 Jeremy Allison 2012-03-15 19:44:04 UTC
No, keep it here for now. Can you add more info on the exact case that causes modules/vfs_acl_common to die ?

Jeremy.
Comment 6 Jeremy Allison 2012-03-16 22:26:05 UTC
Created attachment 7397 [details]
Second proposed patch.

Richard - I think this should fix the second coredump in source3/modules/vfs_acl_common.c.

Can you confirm ?

Jeremy.
Comment 7 Jeremy Allison 2012-03-17 16:33:58 UTC
Hmmm. Not sure the second patch is the right thing to do (even though it should fix the crash). Richard, let's discuss.

Jeremy.
Comment 8 Richard Sharpe 2012-03-17 18:42:27 UTC
OK, source3/smbd/file_access.c:directory_has_default_acl has the same issue.
Comment 9 Richard Sharpe 2012-03-17 18:46:51 UTC
(In reply to comment #6)
> Created attachment 7397 [details]
> Second proposed patch.
> 
> Richard - I think this should fix the second coredump in
> source3/modules/vfs_acl_common.c.
> 
> Can you confirm ?
> 
> Jeremy.

For the moment I fixed it in a slightly different way by only calling add_directory_inheritable_components if psd->dacl exists.

However, this might not be the correct fix, and we are exploring what the ramifications are.

W2K03 allows all access and says nothing funny when you pull up the Security tab. W2K08 warns you that it is insecure.

I still have to sort out claims by QA that ownership of new files is not being correctly set and so forth, so it will take me a few more days to work through the issues.
Comment 10 Richard Sharpe 2012-03-17 18:47:43 UTC
(In reply to comment #7)
> Hmmm. Not sure the second patch is the right thing to do (even though it should
> fix the crash). Richard, let's discuss.
> 
> Jeremy.

Sure. Just give me until about Tuesday or Wednesday to work through some of the issues I am seeing with this :-)
Comment 11 Jeremy Allison 2012-03-19 17:13:53 UTC
Yeah, we need to decide if we just ignore NULL DACLs on inheritance, or actually add the inheritance ACE entries. I'll wait until you check up on this.

Jeremy.
Comment 12 Richard Sharpe 2012-03-22 17:36:01 UTC
(In reply to comment #11)
> Yeah, we need to decide if we just ignore NULL DACLs on inheritance, or
> actually add the inheritance ACE entries. I'll wait until you check up on this.
> Jeremy.

So, for the moment my changes are working for us as desired.

However, there is a corner case where unexpected things will happen. If a user creates files in a directory from Linux or NFS, there will be no SD on these new files, and when we are asked to get the SD, the entries that would have been inherited if the file was created via CIFS would not be there.

However, since these putative Linux or NFS users could add a whole directory tree, the problem is large and I suggest we do not add any inheritable entries at all.
Comment 13 Jeremy Allison 2012-03-22 17:41:49 UTC
Can you add your changes as an attachment to this bug so I can examine them and maybe merge them ?

Jeremy.
Comment 14 Richard Sharpe 2012-03-24 00:10:23 UTC
Here is the other change I made:

--- samba-3.5.12/source3/smbd/file_access.c     2012-03-16 10:49:29.046360624 -0
700
+++ samba-3.5.12/source3/smbd/file_access.c.new 2012-03-16 10:57:11.255298209 -0
700
@@ -159,6 +159,10 @@
                return false;
        }

+       /* Don't crash on a NULL DACL */
+       if (!secdesc->dacl)
+               return false;
+
        for (i = 0; i < secdesc->dacl->num_aces; i++) {
                struct security_ace *psa = &secdesc->dacl->aces[i];
                if (psa->flags & (SEC_ACE_FLAG_OBJECT_INHERIT|
Comment 15 Jeremy Allison 2012-03-30 19:01:30 UTC
Created attachment 7412 [details]
git-am fix for 3.6.next

Ok, here is the complete patchset from master that should fix all issues of a NULL DACL in an SD. Richard please review.

Jeremy.
Comment 16 Richard Sharpe 2012-04-02 19:04:52 UTC
While I think Samba should properly handle these cases because you can indeed, with some effort, set a NULL DACL, there are a number of problems with doing so.

Windows tools, like the W2K08 ACL editor does not like them in a number of places, and xcopy can end up converting them to an EMPTY DACL (a different beast altogether, which locks users out :-().

The W2K08 problem is that if you add/edit the ACL and Click Apply, it tries to propogate the entries down the tree, and if it runs into more NULL DACLs, it throws its hands in the air. (However, if you use the Advanced tab, things work OK.)
Comment 17 Jeremy Allison 2012-06-05 16:59:35 UTC
Richard - ping. Can you review the fix here and give me a thumbs up/down so I can schedule for 3.6.next ?

Jeremy.
Comment 18 Richard Sharpe 2012-06-05 22:28:34 UTC
This looks good to me.

Acked.
Comment 19 Jeremy Allison 2012-06-05 22:37:14 UTC
Re-assigning to Karolin for inclusion in 3.6.next now Richard has acked.
Jeremy.
Comment 20 Karolin Seeger 2012-06-13 17:58:22 UTC
Pushed to v3-6-test.
Closing out bug report.

Thanks!