[PATCHES] fix owner of files created when running as root
Uri Simchoni
uri at samba.org
Thu Apr 27 20:31:45 UTC 2017
Hi,
The attached patch set fixes the ownership of files and directories
created while running as root. Smbd runs as root if the user is an admin
user.
The bug didn't always happen - if ACL inheritance was enabled, the ACL
inheritance made sure the POSIX owner is correct.
BTW, oddly enough, it looks like backup intent is supported only for
file listing and not file opening / creation. That is to say, smbd
records the backup intent in a flag in the files_struct, but I didn't
find any use of that flag. If the code is modified to run as root for
backup intent, then this will fix it too (hopefully).
This patch handles files and directories. As for symlinks, I have a
patch set on top with unit tests that show they don't have the correct
owner in a number of cases, but I couldn't figure out a way to modify
the owner of a symlink, so I don't know if it's valuable to have more
tests for such a corner case. Might come in handy for SMB3 POSIX extensions.
Review appreciated.
Thanks,
Uri.
-------------- next part --------------
From c084fb1c1940b5dfb53fe4b09a8cb9ca1cfa0130 Mon Sep 17 00:00:00 2001
From: Uri Simchoni <uri at samba.org>
Date: Tue, 25 Apr 2017 10:18:46 +0300
Subject: [PATCH 1/8] selftest: add a setup user to test_inherit_owner
Use a separate user for setting up the root folder,
this is required because the parent folder has no write
permissions for arbitrary users by default.
BUG: https://bugzilla.samba.org/show_bug.cgi?id=12759
Signed-off-by: Uri Simchoni <uri at samba.org>
---
source3/script/tests/test_inherit_owner.sh | 26 ++++++++++++++++++--------
source3/selftest/tests.py | 6 +++---
2 files changed, 21 insertions(+), 11 deletions(-)
diff --git a/source3/script/tests/test_inherit_owner.sh b/source3/script/tests/test_inherit_owner.sh
index 5146d40..3b55d43 100755
--- a/source3/script/tests/test_inherit_owner.sh
+++ b/source3/script/tests/test_inherit_owner.sh
@@ -4,9 +4,9 @@
# currently needs to run in SMB1 mode, because it uses UNIX
# extensions to fetch the UNIX owner of a file.
-if [ $# -lt 9 ]; then
+if [ $# -lt 10 ]; then
cat <<EOF
-Usage: $0 SERVER USERNAME PASSWORD PREFIX SMBCLIENT SMBCACLS SHARE INH_WIN INH_UNIX <additional args>
+Usage: $0 SERVER USERNAME PASSWORD PREFIX SMBCLIENT SMBCACLS SHARE INH_WIN INH_UNIX SETUP_USERNAME <additional args>
EOF
exit 1;
fi
@@ -20,10 +20,14 @@ SMBCACLS="$6"
SHARE="$7"
INH_WIN="$8"
INH_UNIX="$9"
-shift 9
+SETUP_USERNAME="${10}"
+shift 10
ADDARGS="$*"
SMBCLIENT="$VALGRIND ${SMBCLIENT} ${ADDARGS}"
SMBCACLS="$VALGRIND ${SMBCACLS} ${ADDARGS}"
+if [ -z "$SETUP_USERNAME" ] ; then
+SETUP_USERNAME=$USERNAME
+fi
incdir=`dirname $0`/../../../testprogs/blackbox
. $incdir/subunit.sh
@@ -42,11 +46,17 @@ create_file() {
create_dir() {
local share=$1
local dname=$2
+ local user
local rem_dirname=$(dirname $dname)
local bname=$(basename $dname)
- $SMBCLIENT //$SERVER/$share -U $USERNAME%$PASSWORD -c "cd $rem_dirname; rmdir $bname" 2>/dev/null
- $SMBCLIENT //$SERVER/$share -U $USERNAME%$PASSWORD -c "cd $rem_dirname; ls" 2>/dev/null | grep "$dname" && exit 1
- $SMBCLIENT //$SERVER/$share -U $USERNAME%$PASSWORD -c "cd $rem_dirname; mkdir $bname" 2>/dev/null || exit 1
+ if [ -n "$3" ] ; then
+ user=$3
+ else
+ user=$USERNAME
+ fi
+ $SMBCLIENT //$SERVER/$share -U $user%$PASSWORD -c "cd $rem_dirname; rmdir $bname" 2>/dev/null
+ $SMBCLIENT //$SERVER/$share -U $user%$PASSWORD -c "cd $rem_dirname; ls" 2>/dev/null | grep "$dname" && exit 1
+ $SMBCLIENT //$SERVER/$share -U $user%$PASSWORD -c "cd $rem_dirname; mkdir $bname" 2>/dev/null || exit 1
}
cleanup_file() {
@@ -136,8 +146,8 @@ else
fi
# SETUP
-testit "$TEST_LABEL - setup root dir" create_dir tmp tmp.$$
-testit "$TEST_LABEL - assign default ACL" $SMBCACLS //$SERVER/tmp tmp.$$ -U $USERNAME%$PASSWORD -S "REVISION:1,OWNER:$SERVER\force_user,GROUP:$SERVER\domusers,ACL:Everyone:ALLOWED/0x3/FULL" 2>/dev/null
+testit "$TEST_LABEL - setup root dir" create_dir tmp tmp.$$ $SETUP_USERNAME
+testit "$TEST_LABEL - assign default ACL" $SMBCACLS //$SERVER/tmp tmp.$$ -U $SETUP_USERNAME%$PASSWORD -S "REVISION:1,OWNER:$SERVER\force_user,GROUP:$SERVER\domusers,ACL:Everyone:ALLOWED/0x3/FULL" 2>/dev/null
# END SETUP
testit "$TEST_LABEL - create subdir under root" create_dir $SHARE tmp.$$/subdir
diff --git a/source3/selftest/tests.py b/source3/selftest/tests.py
index 9bb7903..d02909f33 100755
--- a/source3/selftest/tests.py
+++ b/source3/selftest/tests.py
@@ -225,9 +225,9 @@ for env in ["fileserver"]:
plantestsuite("samba3.blackbox.netshareenum (%s)" % env, env, [os.path.join(samba3srcdir, "script/tests/test_shareenum.sh"), '$SERVER', '$USERNAME', '$PASSWORD', rpcclient])
plantestsuite("samba3.blackbox.acl_xattr (%s)" % env, env, [os.path.join(samba3srcdir, "script/tests/test_acl_xattr.sh"), '$SERVER', '$USERNAME', '$PASSWORD', '$PREFIX', smbclient3, smbcacls])
plantestsuite("samba3.blackbox.smb2.not_casesensitive (%s)" % env, env, [os.path.join(samba3srcdir, "script/tests/test_smb2_not_casesensitive.sh"), '//$SERVER/tmp', '$SERVER_IP', '$USERNAME', '$PASSWORD', '$LOCAL_PATH', smbclient3])
- plantestsuite("samba3.blackbox.inherit_owner.default(%s)" % env, env, [os.path.join(samba3srcdir, "script/tests/test_inherit_owner.sh"), '$SERVER', '$USERNAME', '$PASSWORD', '$PREFIX', smbclient3, smbcacls, 'tmp', '0', '0', '-m', 'NT1'])
- plantestsuite("samba3.blackbox.inherit_owner.full (%s)" % env, env, [os.path.join(samba3srcdir, "script/tests/test_inherit_owner.sh"), '$SERVER', '$USERNAME', '$PASSWORD', '$PREFIX', smbclient3, smbcacls, 'inherit_owner', '1', '1', '-m', 'NT1'])
- plantestsuite("samba3.blackbox.inherit_owner.unix (%s)" % env, env, [os.path.join(samba3srcdir, "script/tests/test_inherit_owner.sh"), '$SERVER', '$USERNAME', '$PASSWORD', '$PREFIX', smbclient3, smbcacls, 'inherit_owner_u', '0', '1', '-m', 'NT1'])
+ plantestsuite("samba3.blackbox.inherit_owner.default(%s)" % env, env, [os.path.join(samba3srcdir, "script/tests/test_inherit_owner.sh"), '$SERVER', '$USERNAME', '$PASSWORD', '$PREFIX', smbclient3, smbcacls, 'tmp', '0', '0', '$USERNAME', '-m', 'NT1'])
+ plantestsuite("samba3.blackbox.inherit_owner.full (%s)" % env, env, [os.path.join(samba3srcdir, "script/tests/test_inherit_owner.sh"), '$SERVER', '$USERNAME', '$PASSWORD', '$PREFIX', smbclient3, smbcacls, 'inherit_owner', '1', '1', '$USERNAME', '-m', 'NT1'])
+ plantestsuite("samba3.blackbox.inherit_owner.unix (%s)" % env, env, [os.path.join(samba3srcdir, "script/tests/test_inherit_owner.sh"), '$SERVER', '$USERNAME', '$PASSWORD', '$PREFIX', smbclient3, smbcacls, 'inherit_owner_u', '0', '1', '$USERNAME', '-m', 'NT1'])
plantestsuite("samba3.blackbox.large_acl (%s)" % env, env, [os.path.join(samba3srcdir, "script/tests/test_large_acl.sh"), '$SERVER', '$USERNAME', '$PASSWORD', smbclient3, smbcacls])
#
--
2.9.3
From 432ff1ffa0b58816d4f268956a8cdc6933bdbc60 Mon Sep 17 00:00:00 2001
From: Uri Simchoni <uri at samba.org>
Date: Tue, 25 Apr 2017 10:21:22 +0300
Subject: [PATCH 2/8] selftest: add an ownership test with an admin user
Add a test for ownership of files created by an admin
user. As the admin user runs as root, it's important
to verify that files and directories it creates have
the uid of the user (rather than 0) as file owner uid.
BUG: https://bugzilla.samba.org/show_bug.cgi?id=12759
Signed-off-by: Uri Simchoni <uri at samba.org>
---
selftest/target/Samba3.pm | 3 +++
source3/selftest/tests.py | 1 +
2 files changed, 4 insertions(+)
diff --git a/selftest/target/Samba3.pm b/selftest/target/Samba3.pm
index c241bd1..4226bde 100755
--- a/selftest/target/Samba3.pm
+++ b/selftest/target/Samba3.pm
@@ -1682,6 +1682,9 @@ sub provision($$$$$$$$)
path = $shrdir
force group = nogroup
guest ok = yes
+[admin_user]
+ copy = tmp
+ admin users = user1
[ro-tmp]
path = $ro_shrdir
guest ok = yes
diff --git a/source3/selftest/tests.py b/source3/selftest/tests.py
index d02909f33..b6310bb 100755
--- a/source3/selftest/tests.py
+++ b/source3/selftest/tests.py
@@ -228,6 +228,7 @@ for env in ["fileserver"]:
plantestsuite("samba3.blackbox.inherit_owner.default(%s)" % env, env, [os.path.join(samba3srcdir, "script/tests/test_inherit_owner.sh"), '$SERVER', '$USERNAME', '$PASSWORD', '$PREFIX', smbclient3, smbcacls, 'tmp', '0', '0', '$USERNAME', '-m', 'NT1'])
plantestsuite("samba3.blackbox.inherit_owner.full (%s)" % env, env, [os.path.join(samba3srcdir, "script/tests/test_inherit_owner.sh"), '$SERVER', '$USERNAME', '$PASSWORD', '$PREFIX', smbclient3, smbcacls, 'inherit_owner', '1', '1', '$USERNAME', '-m', 'NT1'])
plantestsuite("samba3.blackbox.inherit_owner.unix (%s)" % env, env, [os.path.join(samba3srcdir, "script/tests/test_inherit_owner.sh"), '$SERVER', '$USERNAME', '$PASSWORD', '$PREFIX', smbclient3, smbcacls, 'inherit_owner_u', '0', '1', '$USERNAME', '-m', 'NT1'])
+ plantestsuite("samba3.blackbox.inherit_owner.admin(%s)" % env, env, [os.path.join(samba3srcdir, "script/tests/test_inherit_owner.sh"), '$SERVER', 'user1', '$PASSWORD', '$PREFIX', smbclient3, smbcacls, 'admin_user', '0', '0', '$USERNAME', '-m', 'NT1'])
plantestsuite("samba3.blackbox.large_acl (%s)" % env, env, [os.path.join(samba3srcdir, "script/tests/test_large_acl.sh"), '$SERVER', '$USERNAME', '$PASSWORD', smbclient3, smbcacls])
#
--
2.9.3
From 3eff29b17a166f9f8ddeba26b23711713a3d450a Mon Sep 17 00:00:00 2001
From: Uri Simchoni <uri at samba.org>
Date: Thu, 27 Apr 2017 08:49:15 +0300
Subject: [PATCH 3/8] selftest: add a test for admin user without ACL
inheritance
Add a test that shows that an admin user creates files
with wrong ownership if "inherit acls" is not set. This
bug is usually covered by the ACL inheritance mechanism.
BUG: https://bugzilla.samba.org/show_bug.cgi?id=12759
Signed-off-by: Uri Simchoni <uri at samba.org>
---
selftest/knownfail | 3 +++
selftest/target/Samba3.pm | 5 +++++
source3/selftest/tests.py | 1 +
3 files changed, 9 insertions(+)
diff --git a/selftest/knownfail b/selftest/knownfail
index 2cc9c70..7c5713f 100644
--- a/selftest/knownfail
+++ b/selftest/knownfail
@@ -329,3 +329,6 @@
# We currently don't send referrals for LDAP modify of non-replicated attrs
^samba4.ldap.rodc.python\(rodc\).__main__.RodcTests.test_modify_nonreplicated.*
^samba4.ldap.rodc_rwdc.python.*.__main__.RodcRwdcTests.test_change_password_reveal_on_demand_kerberos
+#unix owner not set correctly for admin users
+^samba3.blackbox.inherit_owner.admin-no-inherit\(fileserver\).default - verify
+^samba3.blackbox.inherit_owner.admin-no-inherit\(fileserver\).default - change
diff --git a/selftest/target/Samba3.pm b/selftest/target/Samba3.pm
index 4226bde..6b21bd6 100755
--- a/selftest/target/Samba3.pm
+++ b/selftest/target/Samba3.pm
@@ -1685,6 +1685,11 @@ sub provision($$$$$$$$)
[admin_user]
copy = tmp
admin users = user1
+[admin_user_no_inherit]
+ comment = share with no ACL inheritance and an admin user
+ copy = tmp
+ admin users = user1
+ vfs objects = fake_acls xattr_tdb streams_depot
[ro-tmp]
path = $ro_shrdir
guest ok = yes
diff --git a/source3/selftest/tests.py b/source3/selftest/tests.py
index b6310bb..6c7e57c 100755
--- a/source3/selftest/tests.py
+++ b/source3/selftest/tests.py
@@ -229,6 +229,7 @@ for env in ["fileserver"]:
plantestsuite("samba3.blackbox.inherit_owner.full (%s)" % env, env, [os.path.join(samba3srcdir, "script/tests/test_inherit_owner.sh"), '$SERVER', '$USERNAME', '$PASSWORD', '$PREFIX', smbclient3, smbcacls, 'inherit_owner', '1', '1', '$USERNAME', '-m', 'NT1'])
plantestsuite("samba3.blackbox.inherit_owner.unix (%s)" % env, env, [os.path.join(samba3srcdir, "script/tests/test_inherit_owner.sh"), '$SERVER', '$USERNAME', '$PASSWORD', '$PREFIX', smbclient3, smbcacls, 'inherit_owner_u', '0', '1', '$USERNAME', '-m', 'NT1'])
plantestsuite("samba3.blackbox.inherit_owner.admin(%s)" % env, env, [os.path.join(samba3srcdir, "script/tests/test_inherit_owner.sh"), '$SERVER', 'user1', '$PASSWORD', '$PREFIX', smbclient3, smbcacls, 'admin_user', '0', '0', '$USERNAME', '-m', 'NT1'])
+ plantestsuite("samba3.blackbox.inherit_owner.admin-no-inherit(%s)" % env, env, [os.path.join(samba3srcdir, "script/tests/test_inherit_owner.sh"), '$SERVER', 'user1', '$PASSWORD', '$PREFIX', smbclient3, smbcacls, 'admin_user_no_inherit', '0', '0', '$USERNAME', '-m', 'NT1'])
plantestsuite("samba3.blackbox.large_acl (%s)" % env, env, [os.path.join(samba3srcdir, "script/tests/test_large_acl.sh"), '$SERVER', '$USERNAME', '$PASSWORD', smbclient3, smbcacls])
#
--
2.9.3
From b90e7a6ad8680abbe32916ce61b93a94a22cdb01 Mon Sep 17 00:00:00 2001
From: Uri Simchoni <uri at samba.org>
Date: Thu, 27 Apr 2017 11:06:15 +0300
Subject: [PATCH 4/8] smbd: add take_ownership()
Add a function that modifies the owner of
an open file system object to be the uid of the
user running the session.
That will be used in a later commit.
BUG: https://bugzilla.samba.org/show_bug.cgi?id=12759
Signed-off-by: Uri Simchoni <uri at samba.org>
---
source3/smbd/open.c | 45 +++++++++++++++++++++++++++++++++++++++++++++
source3/smbd/proto.h | 1 +
2 files changed, 46 insertions(+)
diff --git a/source3/smbd/open.c b/source3/smbd/open.c
index 50f8a5e..ff3bcea 100644
--- a/source3/smbd/open.c
+++ b/source3/smbd/open.c
@@ -780,6 +780,51 @@ void change_file_owner_to_parent(connection_struct *conn,
TALLOC_FREE(smb_fname_parent);
}
+/****************************************************************************
+ Change the ownership of a file system object to that of the session user
+****************************************************************************/
+
+void take_ownership(connection_struct *conn, files_struct *fsp)
+{
+ struct user_struct *vuser =
+ get_valid_user_struct(conn->sconn, conn->vuid);
+ uid_t my_uid = (uid_t)-1;
+ int ret = -1;
+
+ if (vuser == NULL) {
+ DBG_WARNING("No user found for vuid %llu\n",
+ (unsigned long long)conn->vuid);
+ return;
+ }
+
+ my_uid = vuser->session_info->unix_token->uid;
+
+ if (fsp->fsp_name->st.st_ex_uid == my_uid) {
+ /* Already this uid - no need to change. */
+ DBG_DEBUG("file %s "
+ "is already owned by uid %d\n",
+ fsp_str_dbg(fsp), (int)my_uid);
+ return;
+ }
+
+ become_root();
+ ret = SMB_VFS_FCHOWN(fsp, my_uid, (gid_t)-1);
+ unbecome_root();
+ if (ret == -1) {
+ DBG_ERR("failed to fchown "
+ "file %s to uid %u. Error "
+ "was %s\n",
+ fsp_str_dbg(fsp), (unsigned int)my_uid,
+ strerror(errno));
+ } else {
+ DBG_DEBUG("changed new file %s to "
+ "uid %u.\n",
+ fsp_str_dbg(fsp), (unsigned int)my_uid);
+ /* Ensure the uid entry is updated. */
+ fsp->fsp_name->st.st_ex_uid = my_uid;
+ }
+}
+
NTSTATUS change_dir_owner_to_parent(connection_struct *conn,
const char *inherit_from_dir,
const char *fname,
diff --git a/source3/smbd/proto.h b/source3/smbd/proto.h
index 841a095..74c16e1 100644
--- a/source3/smbd/proto.h
+++ b/source3/smbd/proto.h
@@ -650,6 +650,7 @@ NTSTATUS fd_close(files_struct *fsp);
void change_file_owner_to_parent(connection_struct *conn,
const char *inherit_from_dir,
files_struct *fsp);
+void take_ownership(connection_struct *conn, files_struct *fsp);
NTSTATUS change_dir_owner_to_parent(connection_struct *conn,
const char *inherit_from_dir,
const char *fname,
--
2.9.3
From 3d1c03d77d62c1734796d79e23f735b790d6c4c2 Mon Sep 17 00:00:00 2001
From: Uri Simchoni <uri at samba.org>
Date: Thu, 27 Apr 2017 11:12:49 +0300
Subject: [PATCH 5/8] smbd: fix ownership of files created by root
If smbd creates files as root (due to an admin user
opening the file), fix the owner of the file.
BUG: https://bugzilla.samba.org/show_bug.cgi?id=12759
Signed-off-by: Uri Simchoni <uri at samba.org>
---
selftest/knownfail | 4 ++--
source3/smbd/open.c | 9 +++++++++
2 files changed, 11 insertions(+), 2 deletions(-)
diff --git a/selftest/knownfail b/selftest/knownfail
index 7c5713f..9910b90 100644
--- a/selftest/knownfail
+++ b/selftest/knownfail
@@ -330,5 +330,5 @@
^samba4.ldap.rodc.python\(rodc\).__main__.RodcTests.test_modify_nonreplicated.*
^samba4.ldap.rodc_rwdc.python.*.__main__.RodcRwdcTests.test_change_password_reveal_on_demand_kerberos
#unix owner not set correctly for admin users
-^samba3.blackbox.inherit_owner.admin-no-inherit\(fileserver\).default - verify
-^samba3.blackbox.inherit_owner.admin-no-inherit\(fileserver\).default - change
+^samba3.blackbox.inherit_owner.admin-no-inherit\(fileserver\).default - verify subdir
+^samba3.blackbox.inherit_owner.admin-no-inherit\(fileserver\).default - change dir
diff --git a/source3/smbd/open.c b/source3/smbd/open.c
index ff3bcea..44460f9 100644
--- a/source3/smbd/open.c
+++ b/source3/smbd/open.c
@@ -1279,6 +1279,15 @@ static NTSTATUS open_file(files_struct *fsp,
change_file_owner_to_parent(conn, parent_dir,
fsp);
need_re_stat = true;
+ } else if (conn->session_info->unix_token->uid ==
+ sec_initial_uid()) {
+ /* we're running as root for some reason
+ * (admin user, backup intent),
+ * let's ensure we have the right
+ * owner
+ */
+ take_ownership(conn, fsp);
+ need_re_stat = true;
}
if (need_re_stat) {
--
2.9.3
From 936c1c25d5302c3b258d43ecaf0207c4c792c678 Mon Sep 17 00:00:00 2001
From: Uri Simchoni <uri at samba.org>
Date: Thu, 27 Apr 2017 11:03:41 +0300
Subject: [PATCH 6/8] smbd: split change_dir_owner_to_parent()
Pure refactoring - Split change_dir_owner_to_parent()
into a function that determines the parent uid, and
a new function that changes the dir owner.
BUG: https://bugzilla.samba.org/show_bug.cgi?id=12759
Signed-off-by: Uri Simchoni <uri at samba.org>
---
source3/smbd/open.c | 48 ++++++++++++++++++++++++++++++------------------
source3/smbd/proto.h | 4 ++++
2 files changed, 34 insertions(+), 18 deletions(-)
diff --git a/source3/smbd/open.c b/source3/smbd/open.c
index 44460f9..177405c 100644
--- a/source3/smbd/open.c
+++ b/source3/smbd/open.c
@@ -830,9 +830,7 @@ NTSTATUS change_dir_owner_to_parent(connection_struct *conn,
const char *fname,
SMB_STRUCT_STAT *psbuf)
{
- struct smb_filename *smb_fname_parent;
- struct smb_filename *smb_fname_cwd = NULL;
- char *saved_dir = NULL;
+ struct smb_filename *smb_fname_parent = NULL;
TALLOC_CTX *ctx = talloc_tos();
NTSTATUS status = NT_STATUS_OK;
int ret;
@@ -856,6 +854,24 @@ NTSTATUS change_dir_owner_to_parent(connection_struct *conn,
goto out;
}
+ status = change_dir_owner(conn, fname, psbuf,
+ smb_fname_parent->st.st_ex_uid);
+out:
+ TALLOC_FREE(smb_fname_parent);
+ return status;
+}
+
+NTSTATUS change_dir_owner(connection_struct *conn,
+ const char *fname,
+ SMB_STRUCT_STAT *psbuf,
+ uid_t uid)
+{
+ struct smb_filename *smb_fname_cwd = NULL;
+ char *saved_dir = NULL;
+ TALLOC_CTX *ctx = talloc_tos();
+ NTSTATUS status = NT_STATUS_OK;
+ int ret;
+
/* We've already done an lstat into psbuf, and we know it's a
directory. If we can cd into the directory and the dev/ino
are the same then we can safely chown without races as
@@ -906,7 +922,7 @@ NTSTATUS change_dir_owner_to_parent(connection_struct *conn,
goto chdir;
}
- if (smb_fname_parent->st.st_ex_uid == smb_fname_cwd->st.st_ex_uid) {
+ if (uid == smb_fname_cwd->st.st_ex_uid) {
/* Already this uid - no need to change. */
DEBUG(10,("change_dir_owner_to_parent: directory %s "
"is already owned by uid %d\n",
@@ -917,30 +933,26 @@ NTSTATUS change_dir_owner_to_parent(connection_struct *conn,
}
become_root();
- ret = SMB_VFS_LCHOWN(conn,
- smb_fname_cwd,
- smb_fname_parent->st.st_ex_uid,
- (gid_t)-1);
+ ret = SMB_VFS_LCHOWN(conn, smb_fname_cwd, uid, (gid_t)-1);
unbecome_root();
if (ret == -1) {
status = map_nt_error_from_unix(errno);
- DEBUG(10,("change_dir_owner_to_parent: failed to chown "
- "directory %s to parent directory uid %u. "
- "Error was %s\n", fname,
- (unsigned int)smb_fname_parent->st.st_ex_uid,
- strerror(errno) ));
+ DEBUG(10, ("change_dir_owner_to_parent: failed to chown "
+ "directory %s to parent directory uid %u. "
+ "Error was %s\n",
+ fname, (unsigned int)uid, strerror(errno)));
} else {
- DEBUG(10,("change_dir_owner_to_parent: changed ownership of new "
- "directory %s to parent directory uid %u.\n",
- fname, (unsigned int)smb_fname_parent->st.st_ex_uid ));
+ DEBUG(10,
+ ("change_dir_owner_to_parent: changed ownership of new "
+ "directory %s to parent directory uid %u.\n",
+ fname, (unsigned int)uid));
/* Ensure the uid entry is updated. */
- psbuf->st_ex_uid = smb_fname_parent->st.st_ex_uid;
+ psbuf->st_ex_uid = uid;
}
chdir:
vfs_ChDir(conn,saved_dir);
out:
- TALLOC_FREE(smb_fname_parent);
TALLOC_FREE(smb_fname_cwd);
return status;
}
diff --git a/source3/smbd/proto.h b/source3/smbd/proto.h
index 74c16e1..4698982 100644
--- a/source3/smbd/proto.h
+++ b/source3/smbd/proto.h
@@ -651,6 +651,10 @@ void change_file_owner_to_parent(connection_struct *conn,
const char *inherit_from_dir,
files_struct *fsp);
void take_ownership(connection_struct *conn, files_struct *fsp);
+NTSTATUS change_dir_owner(connection_struct *conn,
+ const char *fname,
+ SMB_STRUCT_STAT *psbuf,
+ uid_t uid);
NTSTATUS change_dir_owner_to_parent(connection_struct *conn,
const char *inherit_from_dir,
const char *fname,
--
2.9.3
From aaa7246df71e8703906b421fb583b655fc5025dc Mon Sep 17 00:00:00 2001
From: Uri Simchoni <uri at samba.org>
Date: Thu, 27 Apr 2017 11:35:12 +0300
Subject: [PATCH 7/8] smbd: tidy up trace messages in change_dir_owner()
Because change_dir_owner() has been split from
change_dir_owner_to_parent(), remove
references to change_dir_owner_to_parent() from
trace messages. While at it, transform tracing to
new style, and change some level-0 into level-1
traces (level-0 should be reserved for events somewhat
more catastrophic)
BUG: https://bugzilla.samba.org/show_bug.cgi?id=12759
Signed-off-by: Uri Simchoni <uri at samba.org>
---
source3/smbd/open.c | 47 +++++++++++++++++++++++------------------------
1 file changed, 23 insertions(+), 24 deletions(-)
diff --git a/source3/smbd/open.c b/source3/smbd/open.c
index 177405c..4bff82a 100644
--- a/source3/smbd/open.c
+++ b/source3/smbd/open.c
@@ -882,18 +882,19 @@ NTSTATUS change_dir_owner(connection_struct *conn,
saved_dir = vfs_GetWd(ctx,conn);
if (!saved_dir) {
status = map_nt_error_from_unix(errno);
- DEBUG(0,("change_dir_owner_to_parent: failed to get "
- "current working directory. Error was %s\n",
- strerror(errno)));
+ DBG_WARNING("failed to get "
+ "current working directory. Error was %s\n",
+ strerror(errno));
goto out;
}
/* Chdir into the new path. */
if (vfs_ChDir(conn, fname) == -1) {
status = map_nt_error_from_unix(errno);
- DEBUG(0,("change_dir_owner_to_parent: failed to change "
- "current working directory to %s. Error "
- "was %s\n", fname, strerror(errno) ));
+ DBG_WARNING("failed to change "
+ "current working directory to %s. Error "
+ "was %s\n",
+ fname, strerror(errno));
goto chdir;
}
@@ -906,28 +907,27 @@ NTSTATUS change_dir_owner(connection_struct *conn,
ret = SMB_VFS_STAT(conn, smb_fname_cwd);
if (ret == -1) {
status = map_nt_error_from_unix(errno);
- DEBUG(0,("change_dir_owner_to_parent: failed to stat "
- "directory '.' (%s) Error was %s\n",
- fname, strerror(errno)));
+ DBG_WARNING("failed to stat "
+ "directory '.' (%s) Error was %s\n",
+ fname, strerror(errno));
goto chdir;
}
/* Ensure we're pointing at the same place. */
if (smb_fname_cwd->st.st_ex_dev != psbuf->st_ex_dev ||
smb_fname_cwd->st.st_ex_ino != psbuf->st_ex_ino) {
- DEBUG(0,("change_dir_owner_to_parent: "
- "device/inode on directory %s changed. "
- "Refusing to chown !\n", fname ));
+ DBG_WARNING("device/inode on directory %s changed. "
+ "Refusing to chown !\n",
+ fname);
status = NT_STATUS_ACCESS_DENIED;
goto chdir;
}
if (uid == smb_fname_cwd->st.st_ex_uid) {
/* Already this uid - no need to change. */
- DEBUG(10,("change_dir_owner_to_parent: directory %s "
- "is already owned by uid %d\n",
- fname,
- (int)smb_fname_cwd->st.st_ex_uid ));
+ DBG_DEBUG("directory %s "
+ "is already owned by uid %d\n",
+ fname, (int)smb_fname_cwd->st.st_ex_uid);
status = NT_STATUS_OK;
goto chdir;
}
@@ -937,15 +937,14 @@ NTSTATUS change_dir_owner(connection_struct *conn,
unbecome_root();
if (ret == -1) {
status = map_nt_error_from_unix(errno);
- DEBUG(10, ("change_dir_owner_to_parent: failed to chown "
- "directory %s to parent directory uid %u. "
- "Error was %s\n",
- fname, (unsigned int)uid, strerror(errno)));
+ DBG_DEBUG("failed to chown "
+ "directory %s to parent directory uid %u. "
+ "Error was %s\n",
+ fname, (unsigned int)uid, strerror(errno));
} else {
- DEBUG(10,
- ("change_dir_owner_to_parent: changed ownership of new "
- "directory %s to parent directory uid %u.\n",
- fname, (unsigned int)uid));
+ DBG_DEBUG("changed ownership of new "
+ "directory %s to parent directory uid %u.\n",
+ fname, (unsigned int)uid);
/* Ensure the uid entry is updated. */
psbuf->st_ex_uid = uid;
}
--
2.9.3
From 84d1eba1b68088e3974ecc95e8312cc5c6541bae Mon Sep 17 00:00:00 2001
From: Uri Simchoni <uri at samba.org>
Date: Thu, 27 Apr 2017 21:30:10 +0300
Subject: [PATCH 8/8] smbd: set correct created directory owner when running as
root
When running as root and creating a directory, set the (POSIX)
owner of the directory to be the uid of the session's user.
Smbd could be running as root on behalf of a user in some cases
(admin user), but we wouldn't want directories to have root
as their unix owner as that might confuse the user or have
other side effects.
BUG: https://bugzilla.samba.org/show_bug.cgi?id=12759
Signed-off-by: Uri Simchoni <uri at samba.org>
---
selftest/knownfail | 3 ---
source3/smbd/open.c | 27 +++++++++++++++++++++++++++
2 files changed, 27 insertions(+), 3 deletions(-)
diff --git a/selftest/knownfail b/selftest/knownfail
index 9910b90..2cc9c70 100644
--- a/selftest/knownfail
+++ b/selftest/knownfail
@@ -329,6 +329,3 @@
# We currently don't send referrals for LDAP modify of non-replicated attrs
^samba4.ldap.rodc.python\(rodc\).__main__.RodcTests.test_modify_nonreplicated.*
^samba4.ldap.rodc_rwdc.python.*.__main__.RodcRwdcTests.test_change_password_reveal_on_demand_kerberos
-#unix owner not set correctly for admin users
-^samba3.blackbox.inherit_owner.admin-no-inherit\(fileserver\).default - verify subdir
-^samba3.blackbox.inherit_owner.admin-no-inherit\(fileserver\).default - change dir
diff --git a/source3/smbd/open.c b/source3/smbd/open.c
index 4bff82a..9160b31 100644
--- a/source3/smbd/open.c
+++ b/source3/smbd/open.c
@@ -861,6 +861,25 @@ out:
return status;
}
+static void take_dir_ownership(connection_struct *conn,
+ const char *fname,
+ SMB_STRUCT_STAT *psbuf)
+{
+ struct user_struct *vuser =
+ get_valid_user_struct(conn->sconn, conn->vuid);
+ uid_t my_uid = (uid_t)-1;
+
+ if (vuser == NULL) {
+ DBG_WARNING("No user found for vuid %llu\n",
+ (unsigned long long)conn->vuid);
+ return;
+ }
+
+ my_uid = vuser->session_info->unix_token->uid;
+
+ change_dir_owner(conn, fname, psbuf, my_uid);
+}
+
NTSTATUS change_dir_owner(connection_struct *conn,
const char *fname,
SMB_STRUCT_STAT *psbuf,
@@ -3923,6 +3942,14 @@ static NTSTATUS mkdir_internal(connection_struct *conn,
smb_dname->base_name,
&smb_dname->st);
need_re_stat = true;
+ } else if (conn->session_info->unix_token->uid == sec_initial_uid()) {
+ /* we're running as root for some reason
+ * (admin user, backup intent),
+ * let's ensure we have the right
+ * owner
+ */
+ take_dir_ownership(conn, smb_dname->base_name, &smb_dname->st);
+ need_re_stat = true;
}
if (need_re_stat) {
--
2.9.3
More information about the samba-technical
mailing list