chmod via unix extensions cut by "create mask"/"directory mask" - intended?

[Jeremy Allison]

On Wed, Aug 21, 2013 at 05:27:57PM +0200, Michael Adam wrote:
> Hi Jeremy,
> 
> in unix_perms_from_wire() as called from
> smb_set_file_unix_basic(), we apply the
> create mask (or directory mask) to the permissions,
> even if the file already existed, as is the
> case for the smb_set_file_unix_basic() call
> which is used, e.g. when a cifs-mount with
> unix extensions is connected to the Samba
> server.
> 
> This effectively restricts chmod by the
> create mask and directory mask.
> This is different from the behaviour when
> setting NT ACLs, where the masks do not
> restrict.
> 
> Shouldn't we change this to allow chmod over
> cifs-mount with unix extensions?
> Or is there a subtle reason for this restriction?
> 
> The obvious patch would be to change the
> switch statement in unix_perms_from_wire(),
> but I am not 100% certain about possible side
> effects, when e.g. called from smb_posix_open().

We already correctly pass in the create-type
for these cases from all callers (the calling
code checks for existence first).

In addition there are some missing FCHMOD/FCHOWN/LCHOWN
calls in that code path I've taken the liberty of
tidying up in 2 extra patches if the client passed in
a valid file handle rather than a path (they're not security
issues as there is no elevation of privilege).

Complete patchset follows. Push if you're
happy and I'll create a bug and backport
for 4.1.0 and 4.0.next.

Cheers,

	Jeremy.
-------------- next part --------------
From a4fb086223dbeb5960ddb0c2c2ec8d8d003bd0db Mon Sep 17 00:00:00 2001
From: Jeremy Allison <jra at samba.org>
Date: Wed, 21 Aug 2013 12:03:25 -0700
Subject: [PATCH 1/3] Fix the erroneous masking of chmod requests via the UNIX
 extensions.

Signed-off-by: Jeremy Allison <jra at samba.org>
---
 source3/smbd/trans2.c | 9 ++-------
 1 file changed, 2 insertions(+), 7 deletions(-)

diff --git a/source3/smbd/trans2.c b/source3/smbd/trans2.c
index 81f80c3..927ee55 100644
--- a/source3/smbd/trans2.c
+++ b/source3/smbd/trans2.c
@@ -1392,20 +1392,15 @@ static NTSTATUS unix_perms_from_wire( connection_struct *conn,
 	ret |= ((perms & UNIX_SET_UID ) ? S_ISUID : 0);
 #endif
 
-	switch (ptype) {
-	case PERM_NEW_FILE:
-	case PERM_EXISTING_FILE:
+	if (ptype == PERM_NEW_FILE) {
 		/* Apply mode mask */
 		ret &= lp_create_mask(SNUM(conn));
 		/* Add in force bits */
 		ret |= lp_force_create_mode(SNUM(conn));
-		break;
-	case PERM_NEW_DIR:
-	case PERM_EXISTING_DIR:
+	} else if (ptype == PERM_NEW_DIR) {
 		ret &= lp_dir_mask(SNUM(conn));
 		/* Add in force bits */
 		ret |= lp_force_dir_mode(SNUM(conn));
-		break;
 	}
 
 	*ret_perms = ret;
-- 
1.8.3


From 7ec9e11ad85cd94a5911806d938eb7de1880face Mon Sep 17 00:00:00 2001
From: Jeremy Allison <jra at samba.org>
Date: Wed, 21 Aug 2013 12:10:05 -0700
Subject: [PATCH 2/3] Allow UNIX extensions client to act on open fsp instead
 of pathname if available.

Eliminates possible race condition on pathname op.

Signed-off-by: Jeremy Allison <jra at samba.org>
---
 source3/smbd/trans2.c | 9 ++++++++-
 1 file changed, 8 insertions(+), 1 deletion(-)

diff --git a/source3/smbd/trans2.c b/source3/smbd/trans2.c
index 927ee55..27cc1be 100644
--- a/source3/smbd/trans2.c
+++ b/source3/smbd/trans2.c
@@ -7119,11 +7119,18 @@ static NTSTATUS smb_set_file_unix_basic(connection_struct *conn,
 	 */
 
 	if (raw_unixmode != SMB_MODE_NO_CHANGE) {
+		int ret;
+
 		DEBUG(10,("smb_set_file_unix_basic: SMB_SET_FILE_UNIX_BASIC "
 			  "setting mode 0%o for file %s\n",
 			  (unsigned int)unixmode,
 			  smb_fname_str_dbg(smb_fname)));
-		if (SMB_VFS_CHMOD(conn, smb_fname->base_name, unixmode) != 0) {
+		if (fsp && fsp->fh->fd != -1) {
+			ret = SMB_VFS_FCHMOD(fsp, unixmode);
+		} else {
+			ret = SMB_VFS_CHMOD(conn, smb_fname->base_name, unixmode);
+		}
+		if (ret != 0) {
 			return map_nt_error_from_unix(errno);
 		}
 	}
-- 
1.8.3


From cc4ce74ea1c230e86735aad045c15dce7a8db483 Mon Sep 17 00:00:00 2001
From: Jeremy Allison <jra at samba.org>
Date: Wed, 21 Aug 2013 12:20:48 -0700
Subject: [PATCH 3/3] Fix the UNIX extensions CHOWN calls to use FCHOWN if
 available, else LCHOWN.

UNIX calls never deref links.

Signed-off-by: Jeremy Allison <jra at samba.org>
---
 source3/smbd/trans2.c | 20 ++++++++++++++------
 1 file changed, 14 insertions(+), 6 deletions(-)

diff --git a/source3/smbd/trans2.c b/source3/smbd/trans2.c
index 27cc1be..6bad904 100644
--- a/source3/smbd/trans2.c
+++ b/source3/smbd/trans2.c
@@ -7148,12 +7148,12 @@ static NTSTATUS smb_set_file_unix_basic(connection_struct *conn,
 			  (unsigned int)set_owner,
 			  smb_fname_str_dbg(smb_fname)));
 
-		if (S_ISLNK(sbuf.st_ex_mode)) {
+		if (fsp && fsp->fh->fd != -1) {
+			ret = SMB_VFS_FCHOWN(fsp, set_owner, (gid_t)-1);
+		} else {
+			/* UNIX calls always operate on symlinks. */
 			ret = SMB_VFS_LCHOWN(conn, smb_fname->base_name,
 					     set_owner, (gid_t)-1);
-		} else {
-			ret = SMB_VFS_CHOWN(conn, smb_fname->base_name,
-					    set_owner, (gid_t)-1);
 		}
 
 		if (ret != 0) {
@@ -7171,12 +7171,20 @@ static NTSTATUS smb_set_file_unix_basic(connection_struct *conn,
 
 	if ((set_grp != (uid_t)SMB_GID_NO_CHANGE) &&
 	    (sbuf.st_ex_gid != set_grp)) {
+		int ret;
+
 		DEBUG(10,("smb_set_file_unix_basic: SMB_SET_FILE_UNIX_BASIC "
 			  "changing group %u for file %s\n",
 			  (unsigned int)set_owner,
 			  smb_fname_str_dbg(smb_fname)));
-		if (SMB_VFS_CHOWN(conn, smb_fname->base_name, (uid_t)-1,
-				  set_grp) != 0) {
+		if (fsp && fsp->fh->fd != -1) {
+			ret = SMB_VFS_FCHOWN(fsp, set_owner, (gid_t)-1);
+		} else {
+			/* UNIX calls always operate on symlinks. */
+			ret = SMB_VFS_LCHOWN(conn, smb_fname->base_name, (uid_t)-1,
+				  set_grp);
+		}
+		if (ret != 0) {
 			status = map_nt_error_from_unix(errno);
 			if (delete_on_fail) {
 				SMB_VFS_UNLINK(conn, smb_fname);
-- 
1.8.3