Bug 8605 - When storing a security descriptor, vfs_acl_common.c strips SE_DACL_PROTECTED
Summary: When storing a security descriptor, vfs_acl_common.c strips SE_DACL_PROTECTED
Status: RESOLVED INVALID
Alias: None
Product: Samba 3.5
Classification: Unclassified
Component: VFS Modules (show other bugs)
Version: 3.5.6
Hardware: All All
: P5 normal
Target Milestone: ---
Assignee: Samba Bugzilla Account
QA Contact: Samba QA Contact
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2011-11-12 20:21 UTC by Richard Sharpe
Modified: 2011-11-16 02:47 UTC (History)
0 users

See Also:


Attachments

Note You need to log in before you can comment on or make changes to this bug.
Description Richard Sharpe 2011-11-12 20:21:20 UTC
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.
Comment 1 Richard Sharpe 2011-11-12 22:33:39 UTC
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.
Comment 2 Richard Sharpe 2011-11-13 17:47:52 UTC
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.
Comment 3 Richard Sharpe 2011-11-13 18:35:24 UTC
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.
Comment 4 Richard Sharpe 2011-11-13 20:00:03 UTC
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.
Comment 5 Richard Sharpe 2011-11-13 20:26:36 UTC
(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.
Comment 6 Richard Sharpe 2011-11-13 21:32:31 UTC
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.
Comment 7 Richard Sharpe 2011-11-13 23:10:15 UTC
(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.
Comment 8 Jeremy Allison 2011-11-15 23:05:48 UTC
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.
Comment 9 Jeremy Allison 2011-11-15 23:13:37 UTC
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).
Comment 10 Richard Sharpe 2011-11-15 23:55:43 UTC
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.
Comment 11 Richard Sharpe 2011-11-16 02:47:20 UTC
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.