smbd: Clean up the logic inside vfs_chown_fsp() to prevent future security issues.

Signed-off-by: Jeremy Allison <jra@samba.org>
Reviewed-by: Ralph Boehme <slow@samba.org>
Autobuild-User(master): Ralph Böhme <slow@samba.org>
Autobuild-Date(master): Sat Mar  5 12:53:11 CET 2016 on sn-devel-144

diff --git a/source3/smbd/vfs.c b/source3/smbd/vfs.c
index 878d8b11c5d773c2c230c05aeb5898efb167bc2b..19f75d17cc07f96348d96c381089b575c5e54c2d 100644 (file)
--- a/source3/smbd/vfs.c
+++ b/source3/smbd/vfs.c
@@ -1911,10 +1911,7 @@ NTSTATUS vfs_chown_fsp(files_struct *fsp, uid_t uid, gid_t gid)
 {
        int ret;
        bool as_root = false;
-       char *saved_dir = NULL;
-       char *parent_dir = NULL;
        NTSTATUS status;
-       struct smb_filename *local_smb_fname = NULL;
 
        if (fsp->fh->fd != -1) {
                /* Try fchown. */
@@ -1929,13 +1926,6 @@ NTSTATUS vfs_chown_fsp(files_struct *fsp, uid_t uid, gid_t gid)
 
        as_root = (geteuid() == 0);
 
-       /*
-        * FIXME. The logic around as_root and FSP_POSIX_FLAGS_OPEN
-        * is way too complex and is a security issue waiting to
-        * happen. This should be simplified into separate if
-        * blocks. JRA.
-        */
-
        if (as_root) {
                /*
                 * We are being asked to chown as root. Make
@@ -1943,7 +1933,10 @@ NTSTATUS vfs_chown_fsp(files_struct *fsp, uid_t uid, gid_t gid)
                 * and always act using lchown to ensure we
                 * don't deref any symbolic links.
                 */
+               char *saved_dir = NULL;
+               char *parent_dir = NULL;
                const char *final_component = NULL;
+               struct smb_filename *local_smb_fname = NULL;
 
                saved_dir = vfs_GetWd(talloc_tos(),fsp->conn);
                if (!saved_dir) {
@@ -1989,14 +1982,31 @@ NTSTATUS vfs_chown_fsp(files_struct *fsp, uid_t uid, gid_t gid)
                         status = NT_STATUS_ACCESS_DENIED;
                        goto out;
                 }
-        } else {
-               local_smb_fname = fsp->fsp_name;
-       }
 
-       if ((fsp->posix_flags & FSP_POSIX_FLAGS_OPEN) || as_root) {
                ret = SMB_VFS_LCHOWN(fsp->conn,
                        local_smb_fname,
                        uid, gid);
+
+               if (ret == 0) {
+                       status = NT_STATUS_OK;
+               } else {
+                       status = map_nt_error_from_unix(errno);
+               }
+
+  out:
+
+               vfs_ChDir(fsp->conn,saved_dir);
+               TALLOC_FREE(local_smb_fname);
+               TALLOC_FREE(saved_dir);
+               TALLOC_FREE(parent_dir);
+
+               return status;
+       }
+
+       if (fsp->posix_flags & FSP_POSIX_FLAGS_OPEN) {
+               ret = SMB_VFS_LCHOWN(fsp->conn,
+                       fsp->fsp_name,
+                       uid, gid);
        } else {
                ret = SMB_VFS_CHOWN(fsp->conn,
                        fsp->fsp_name,
@@ -2008,15 +2018,6 @@ NTSTATUS vfs_chown_fsp(files_struct *fsp, uid_t uid, gid_t gid)
        } else {
                status = map_nt_error_from_unix(errno);
        }
-
-  out:
-
-       if (as_root) {
-               vfs_ChDir(fsp->conn,saved_dir);
-               TALLOC_FREE(local_smb_fname);
-               TALLOC_FREE(saved_dir);
-               TALLOC_FREE(parent_dir);
-       }
        return status;
 }