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; }
Created attachment 7385 [details] Proposed patch ? Richard, does this fix the issue ?
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.
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.)
(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 ...
No, keep it here for now. Can you add more info on the exact case that causes modules/vfs_acl_common to die ? Jeremy.
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.
Hmmm. Not sure the second patch is the right thing to do (even though it should fix the crash). Richard, let's discuss. Jeremy.
OK, source3/smbd/file_access.c:directory_has_default_acl has the same issue.
(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.
(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 :-)
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.
(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.
Can you add your changes as an attachment to this bug so I can examine them and maybe merge them ? Jeremy.
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|
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.
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.)
Richard - ping. Can you review the fix here and give me a thumbs up/down so I can schedule for 3.6.next ? Jeremy.
This looks good to me. Acked.
Re-assigning to Karolin for inclusion in 3.6.next now Richard has acked. Jeremy.
Pushed to v3-6-test. Closing out bug report. Thanks!