If a user unchecks the checkbox "Allow inheritable permissions from the parent ..." Windows sends a Security Descriptor with the SE_DACL_PROTECTED bit set, along with a few others (SE_DACL_AUTO_INHERIT_REQ perhaps, and two more bits). These are silently stripped by cfs_acl_common.c, it seems, and will drive users crazy, because even though they uncheck the checkbox I mentioned above, it comes back every time they look at advanced permissions on the object. They are being stripped out only in the case that Windows is sending only the DACL. vfs_acl_common.c tries to preserve the existing owner and group owner info, which it does by retrieving the existing ACL, and then overwriting the new DACL. However, vfs_acl_common.c does not replace the type field in the retrieved SD with that from the incoming SD, and then replaces the pointer to the incoming SD with the retrieved SD. We should at least not strip out SE_DACL_PROTECTED. I will check master for this and create a patch if needed.
The code in master is different from that in Samba 3.5.x (at least 3.5.6). However, it still has the problem. Here is what it does: static NTSTATUS fset_nt_acl_common(vfs_handle_struct *handle, files_struct *fsp, uint32_t security_info_sent, const struct security_descriptor *orig_psd) { NTSTATUS status; DATA_BLOB blob; struct security_descriptor *pdesc_next = NULL; struct security_descriptor *psd = NULL; uint8_t hash[XATTR_SD_HASH_SIZE]; if (DEBUGLEVEL >= 10) { DEBUG(10,("fset_nt_acl_xattr: incoming sd for file %s\n", fsp_str_dbg(fsp))); NDR_PRINT_DEBUG(security_descriptor, discard_const_p(struct security_descriptor, orig_psd)); } status = get_nt_acl_internal(handle, fsp, NULL, SECINFO_OWNER|SECINFO_GROUP|SECINFO_DACL|SECINFO_SACL, &psd); if (!NT_STATUS_IS_OK(status)) { return status; } psd->revision = orig_psd->revision; /* All our SD's are self relative. */ psd->type = orig_psd->type | SEC_DESC_SELF_RELATIVE; That statement above wipes out the SE_DACL_PROTECTED bit if it was there and now we cannot check any longer. I propose that we change it to: psd->type = (psd->type & SEC_DESC_DACL_PROTECTED) | orig_psd->type | SEC_DESC_SELF_RELATIVE; However, that still leaves the other bits that might be present.
Here is the code in v3-5-test: psd->revision = orig_psd->revision; /* All our SD's are self relative. */ psd->type = orig_psd->type | SEC_DESC_SELF_RELATIVE; if ((security_info_sent & SECINFO_OWNER) && (orig_psd->owner_sid != NULL)) { psd->owner_sid = orig_psd->owner_sid; } if ((security_info_sent & SECINFO_GROUP) && (orig_psd->group_sid != NULL)) { psd->group_sid = orig_psd->group_sid; } if (security_info_sent & SECINFO_DACL) { psd->dacl = orig_psd->dacl; psd->type |= SEC_DESC_DACL_PRESENT; } if (security_info_sent & SECINFO_SACL) { psd->sacl = orig_psd->sacl; psd->type |= SEC_DESC_SACL_PRESENT; } status = SMB_VFS_NEXT_FSET_NT_ACL(handle, fsp, security_info_sent, psd); if (!NT_STATUS_IS_OK(status)) { return status; } These lines: /* All our SD's are self relative. */ psd->type = orig_psd->type | SEC_DESC_SELF_RELATIVE; Should become psd->type = (psd_type & SEC_DESC_DACL_PROTECTED) | orig_psd->type | SEC_DESC_SELF_RELATIVE; Again, there is also the issue of what about those other few bits, but at least we should get this fix into the code.
Hmmm, actually, I think the following is what is needed: /* * All our SD's are self relative, and preserve incoming * SE_DACL_PROTECTED. */ psd->type = (psd->type & SEC_DESC_DACL_PROTECTED) | (orig_psd->type & ~SEC_DESC_DACL_PROTECTED) | SEC_DESC_SELF_RELATIVE; We have to use the incoming value of the bit in the SE_DACL_PROTECTED bit of the type field, so we need to eliminate whatever is saved.
I have verified that the above change on Samba 3.5.6 fixes this problem. That is, the setting of the bit is sticky now. The change was the same, but in a slightly different position.
(In reply to comment #4) > I have verified that the above change on Samba 3.5.6 fixes this problem. That > is, the setting of the bit is sticky now. The change was the same, but in a > slightly different position. You can both set the bit and unset the bit and it is sticky. So, both cases have been tested.
An additional question here is: Is the open behavior correct in vfs_acl_common.c? That is, if SE_DACL_PROTECTED is set, then no ACLs should be inherited from the parent directory, however, if it is not set, then any ACEs on the parent marked as CONTAINER_INHERIT or OBJECT_INHERIT should be inherited as appropriate. I will check.
(In reply to comment #6) > An additional question here is: Is the open behavior correct in > vfs_acl_common.c? > > That is, if SE_DACL_PROTECTED is set, then no ACLs should be inherited from the > parent directory, however, if it is not set, then any ACEs on the parent marked > as CONTAINER_INHERIT or OBJECT_INHERIT should be inherited as appropriate. > > I will check. Hmmm, that probably applies only to file creation not opening of existing files.
Ok, I don't understand this. Looking at least at the code in master: source3/modules/vfs_acl_common.c we have: static NTSTATUS fset_nt_acl_common(vfs_handle_struct *handle, files_struct *fsp, uint32_t security_info_sent, const struct security_descriptor *orig_psd) { .... status = get_nt_acl_internal(handle, fsp, NULL, SECINFO_OWNER|SECINFO_GROUP|SECINFO_DACL|SECINFO_SACL, &psd); .... psd->revision = orig_psd->revision; /* All our SD's are self relative. */ psd->type = orig_psd->type | SEC_DESC_SELF_RELATIVE; How is this wrong ? We are simply adding in the self-relative bit to the incoming type field given to us by the client, then storing it. You change looks wrong to me. Why are you looking at the bits in the type field from the on-disk security descriptor ? It is being replaced with what the client is sending. Where is the SE_DACL_PROTECTED bit being stripped out ? I don't see it. Jeremy.
Are you sure you're not running into this code in modules/vfs_acl_common.c: static NTSTATUS get_nt_acl_internal(vfs_handle_struct *handle, files_struct *fsp, const char *name, uint32_t security_info, struct security_descriptor **ppdesc) { ... } /* The underlying POSIX module always sets the ~SEC_DESC_DACL_PROTECTED bit, as ACLs can't be inherited in this way under POSIX. Remove it for Windows-style ACLs. */ psd->type &= ~SEC_DESC_DACL_PROTECTED; ... } This code gets activated when reading the xattr blob has failed (due to bad hash or other problem) and we know we're returning something from the underlying file system (aka a mapped POSIX ACL).
Arrgh. You are right. I mixed up where psd and orig_psd were coming from. From memory, I think 3.5.6 also receives psd in fset_nt_acl_common but I will have to check that later. Indeed, this problem is not in master or v3-5-test, and only in 3.5.6 (and maybe one or more beyond that). Sorry for the false alarm. I will do a little more checking and close the bug as invalid later, most likely.
OK, this problem existed in 3.5.6 but was fixed by 3.5.8. I was confused by the change in parameter names between the 3.5.6 and later code. I am closing it as invalid. Sorry for the waste of time.