Bug 9843 - source3/modules/vfs_acl_common.c:open_acl_common incorrectly orders tests for new file
source3/modules/vfs_acl_common.c:open_acl_common incorrectly orders tests for...
Status: NEW
Product: Samba 3.6
Classification: Unclassified
Component: File services
unspecified
All All
: P5 normal
: ---
Assigned To: Volker Lendecke
Samba QA Contact
:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2013-05-01 03:30 UTC by Richard Sharpe
Modified: 2013-05-02 08:15 UTC (History)
1 user (show)

See Also:


Attachments

Note You need to log in before you can comment on or make changes to this bug.
Description Richard Sharpe 2013-05-01 03:30:48 UTC
In open_acl_common we see:

        status = get_nt_acl_internal(handle,
                                NULL,
                                fname,
                                (SECINFO_OWNER |
                                 SECINFO_GROUP |
                                 SECINFO_DACL  |
                                 SECINFO_SACL),
                                &pdesc);
        if (NT_STATUS_IS_OK(status)) {
                /* See if we can access it. */
                status = smb1_file_se_access_check(handle->conn,
                                        pdesc,
                                        get_current_nttok(handle->conn),
                                        fsp->access_mask,
                                        &access_granted);
                if (!NT_STATUS_IS_OK(status)) {
                        DEBUG(10,("open_acl_xattr: %s open "
                                "for access 0x%x (0x%x) "
                                "refused with error %s\n",
                                fsp_str_dbg(fsp),
                                (unsigned int)fsp->access_mask,
                                (unsigned int)access_granted,
                                nt_errstr(status) ));
                        goto err;
                }
        } else if (NT_STATUS_EQUAL(status,NT_STATUS_OBJECT_NAME_NOT_FOUND)) {
                file_existed = false;
                /*
                 * If O_CREAT is true then we're trying to create a file.
                 * Check the parent directory ACL will allow this.
                 */
                if (flags & O_CREAT) {
                        struct security_descriptor *parent_desc = NULL;
                        struct security_descriptor **pp_psd = NULL;

                        status = check_parent_acl_common(handle, fname,
                                        SEC_DIR_ADD_FILE, &parent_desc);
                        if (!NT_STATUS_IS_OK(status)) {
                                goto err;
                        }

However, this means that in the create path we are burdened with a call to get the NT ACL, even though it does not exist. This also causes any VFS modules called by get_nt_acl_common to have problems if they are doing interesting things.

The check for O_CREATE should occur first, and only if it is not set should we try to get the NT ACL on the file.
Comment 1 Richard Sharpe 2013-05-01 03:32:38 UTC
Not sure if this is a problem in samba.master. This code has moved into Samba in master.

I am filing this mainly as a place holder and in case any OEMs check bugzilla. That seems to be a vain hope.
Comment 2 Jeremy Allison 2013-05-01 03:55:05 UTC
Hmmm. There are atomicity issues if we don't do the get. Also, remember you can set O_CREAT even if the file exists, so just having O_CREAT set isn't enough to know if the file doesn't exist.

I hope that OEM VFS modules correctly cope with doing a GET_NT_ACL call on a non-existent pathname (they're a bit broken if they don't).

Jeremy.
Comment 3 Richard Sharpe 2013-05-01 04:15:20 UTC
(In reply to comment #2)
> Hmmm. There are atomicity issues if we don't do the get. Also, remember you can
> set O_CREAT even if the file exists, so just having O_CREAT set isn't enough to
> know if the file doesn't exist.

Don't we know when we call SMB_VFS_OPEN whether or not the file/folder exist?
 
> I hope that OEM VFS modules correctly cope with doing a GET_NT_ACL call on a
> non-existent pathname (they're a bit broken if they don't).

I imagine they can, but it does add to the create path.
Comment 4 Jeremy Allison 2013-05-01 16:48:27 UTC
> Don't we know when we call SMB_VFS_OPEN whether or not the file/folder exist?

Yes we do, but it's not atomic. Someone may have removed the file after the stat, but before the open.

> I imagine they can, but it does add to the create path.

Yeah, but it has to be there due to the atomicity requirement. I think this is handled better in master.