Bug 12460 - rename_internals_fsp missing ACL permission-check on destination folder
rename_internals_fsp missing ACL permission-check on destination folder
Status: RESOLVED FIXED
Product: Samba 4.1 and newer
Classification: Unclassified
Component: File services
4.2.13
All All
: P5 normal
: ---
Assigned To: Karolin Seeger
Samba QA Contact
:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2016-12-05 17:32 UTC by Michael Zeis
Modified: 2016-12-20 08:54 UTC (History)
2 users (show)

See Also:


Attachments
git-am fix for master. (11.85 KB, patch)
2016-12-06 17:52 UTC, Jeremy Allison
no flags Details
git-am fix for 4.5.next. (12.49 KB, patch)
2016-12-07 22:12 UTC, Jeremy Allison
slow: review+
Details
git-am fix for 4.4.next (4.25 KB, patch)
2016-12-07 22:14 UTC, Jeremy Allison
slow: review+
Details

Note You need to log in before you can comment on or make changes to this bug.
Description Michael Zeis 2016-12-05 17:32:20 UTC
It seems that the rename_internals_fsp() function is allowing a 'rename' to succeed even though the user is explicitly denied write-access to the destination folder by its ACL.

This occurs when the destination folder in the file-system has Unix permissions that allow write-access to the user, example where the permission-bits are 0777.

The result is the the user can rename a file/folder to a destination folder the user is denied access by the ACL, and then the user now loses the ability to access it later.

I was able to verify this by adding a ACL to the destination folder denying me access but I was still able to drag and drop a file into it. 

Looking at the rename_internals_fsp() code, it seems that a call to check_parent_access() should be added for the smb_fname_dst argument before lock and the SMB_VFS_RENAME call is made.

Example fix to rename_internals_fsp (below):

    if (rename_path_prefix_equal(fsp->fsp_name, smb_fname_dst)) {
        status = NT_STATUS_ACCESS_DENIED;
+       goto out;  /* SEEMS TO BE MISSING THE GOTO HERE ??? */
    }

+    uint32_t access_mask = SEC_DIR_ADD_FILE;
+    if (S_ISDIR(smb_fname_dst.st.st_ex_mode)) {
+       access_mask = SEC_DIR_ADD_SUBDIR;
+    }
+    status = check_parent_access(conn,
+                                 smb_fname_dst,
+                                 access_mask);
+    if (!NT_STATUS_IS_OK(status)) {
+        DEBUG(10, ("rename_internals_fsp: "
+               "check_parent_access on "
+               "dst %s returned %s\n",
+               smb_fname_str_dbg(smb_fname_dst),
+               nt_errstr(status) ));
+        goto out;
+    }


    lck = get_existing_share_mode_lock(talloc_tos(), fsp->file_id);

    /*
     * We have the file open ourselves, so not being able to get the
     * corresponding share mode lock is a fatal error.
     */

    SMB_ASSERT(lck != NULL);


    if(SMB_VFS_RENAME(conn, fsp->fsp_name, smb_fname_dst) == 0) {
        uint32 create_options = fsp->fh->private_options;

        DEBUG(3, ("rename_internals_fsp: succeeded doing rename on "
              "%s -> %s\n", smb_fname_str_dbg(fsp->fsp_name),
              smb_fname_str_dbg(smb_fname_dst)));
Comment 1 Jeremy Allison 2016-12-05 19:07:28 UTC
Yes this looks correct. I'll code up a regression test to make sure we can test this.
Comment 2 Michael Zeis 2016-12-05 20:04:01 UTC
Had a typo in my proposed fix.  This line should be:

 if (S_ISDIR(smb_fname_dst->st.st_ex_mode)) {
Comment 3 Jeremy Allison 2016-12-06 17:52:01 UTC
Created attachment 12731 [details]
git-am fix for master.
Comment 4 Jeremy Allison 2016-12-07 22:12:23 UTC
Created attachment 12748 [details]
git-am fix for 4.5.next.

Contains cherry-pick info.
Comment 5 Jeremy Allison 2016-12-07 22:14:04 UTC
Created attachment 12749 [details]
git-am fix for 4.4.next

Optional regression test doesn't apply, not needed for 4.4.next.
Comment 6 Ralph Böhme 2016-12-08 06:05:05 UTC
Reassigning to Karolin for inclusion in 4.4 and 4.5.
Comment 7 Karolin Seeger 2016-12-14 11:10:22 UTC
(In reply to Ralph Böhme from comment #6)
Pushed to autobuild-v4-{4,5}-test.
Comment 8 Karolin Seeger 2016-12-20 08:54:59 UTC
(In reply to Karolin Seeger from comment #7)
Pushed to both branches.
Closing out bug report.

Thanks!