chmod via unix extensions cut by "create mask"/"directory mask" - intended?
Jeremy Allison
jra at samba.org
Wed Aug 21 13:35:47 MDT 2013
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
More information about the samba-technical
mailing list