[PATCH] Fix change-ownership vs give-ownership behaviour in vfs_acl_xattr|tdb and vfs_fake_acl
Ralph Böhme
slow at samba.org
Sun Oct 8 16:27:38 UTC 2017
Hi!
Attached is a patchset to make vfs_acl_xattr and tdb behave like a Windows
server with respect to not allowing giving ownership of files and directories
away -- unless you have SeRestorePrivilege.
The behaviour was introduced by a fix for bug 7933.
Currently vfs_acl_xattr freely allows giving ownership away on files and
directories if the user has SEC_STD_WRITE_OWNER permission. Windows otoh doesn't
allow this and fails such requests with NT_STATUS_INVALID_OWNER. You're only
allowed to give ownership away if you have SeRestorePrivilege. This behaviour is
mentioned in MS-FSA 2.1.5.16.
The patchset adds a test that verifies this and also fixes related existing
tests that relied on a succeeding give-ownership.
Finally, I'm modifying vfs_fake_acls to implement the MS-FSA 2.1.5.16 check so
we can properly tests this in autobuild. The corresponding commit has a larger
explanation why I placed this constraint into the VFS backend and not into the
caller.
Please review&push if ok. Thanks!
Cheerio!
-slow
-------------- next part --------------
From b7cab04f8905a474093655dd1b8d2458f5482c44 Mon Sep 17 00:00:00 2001
From: Ralph Boehme <slow at samba.org>
Date: Sun, 8 Oct 2017 11:12:48 +0200
Subject: [PATCH 01/11] selftest: fix acl_xattr test: changing owner
Don't give ownership to user "force_user" as user "$USERNAME", this
would fail with NT_STATUS_INVALID_OWNER, instead just take ownership as
user "force_user". Adding a corresponding ACE for "force_user" with FULL
rights ensures this works.
Bug: https://bugzilla.samba.org/show_bug.cgi?id=7933
Signed-off-by: Ralph Boehme <slow at samba.org>
---
source3/script/tests/test_acl_xattr.sh | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/source3/script/tests/test_acl_xattr.sh b/source3/script/tests/test_acl_xattr.sh
index ddccf4fce8a..9697f06a43b 100755
--- a/source3/script/tests/test_acl_xattr.sh
+++ b/source3/script/tests/test_acl_xattr.sh
@@ -77,7 +77,8 @@ nt_affects_chown() {
b4_actual=$($SMBCLIENT //$SERVER/$share -U $USERNAME%$PASSWORD -c "getfacl $fname" 2>/dev/null) || exit 1
echo "${b4_actual}" | grep -q "^# owner:" || exit 1
b4_actual=$(echo "$b4_actual" | sed -rn 's/^# owner: (.*)/\1/p')
- $SMBCACLS //$SERVER/$share $fname -U $USERNAME%$PASSWORD -C force_user 2>/dev/null || exit 1
+ $SMBCACLS //$SERVER/$share $fname -U $USERNAME%$PASSWORD -a "ACL:$SERVER\force_user:ALLOWED/0x0/FULL" || exit 1
+ $SMBCACLS //$SERVER/$share $fname -U force_user%$PASSWORD -C force_user 2>/dev/null || exit 1
af_actual=$($SMBCLIENT //$SERVER/$share -U $USERNAME%$PASSWORD -c "getfacl $fname" 2>/dev/null) || exit 1
echo "${af_actual}" | grep -q "^# owner:" || exit 1
af_actual=$(echo "$af_actual" | sed -rn 's/^# owner: (.*)/\1/p')
--
2.13.5
From bb535c7ceb8c3cf122c219f4b1e12356376ee339 Mon Sep 17 00:00:00 2001
From: Ralph Boehme <slow at samba.org>
Date: Sun, 8 Oct 2017 11:13:46 +0200
Subject: [PATCH 02/11] selftest: fix acl_xattr test: group, not user
In nt_affects_chgrp() check for domadmins *group*, not user. This didn't
trigger an error as nt_affects_chgrp() isn't actually called, see next
commit.
Bug: https://bugzilla.samba.org/show_bug.cgi?id=7933
Signed-off-by: Ralph Boehme <slow at samba.org>
---
source3/script/tests/test_acl_xattr.sh | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/source3/script/tests/test_acl_xattr.sh b/source3/script/tests/test_acl_xattr.sh
index 9697f06a43b..bc3326cf919 100755
--- a/source3/script/tests/test_acl_xattr.sh
+++ b/source3/script/tests/test_acl_xattr.sh
@@ -100,8 +100,8 @@ nt_affects_chgrp() {
b4_expected=$(echo "$b4_expected" | awk -F: '{print $3}')
echo "$b4_expected"
- echo -n "determining uid of domadmins..."
- af_expected=$(getent passwd domadmins) || exit 1
+ echo -n "determining gid of domadmins..."
+ af_expected=$(getent group domadmins) || exit 1
af_expected=$(echo "$af_expected" | awk -F: '{print $3}')
echo "$af_expected"
--
2.13.5
From 5329349d04425d3e1dfa13432459734b1821535f Mon Sep 17 00:00:00 2001
From: Ralph Boehme <slow at samba.org>
Date: Sun, 8 Oct 2017 11:16:27 +0200
Subject: [PATCH 03/11] selftest: fix acl_xattr test: grep ouput before munging
The check of the smbclient getfacl output for presence of a "^# group:"
line must be done before munging the saved output with a sed filter.
Bug: https://bugzilla.samba.org/show_bug.cgi?id=7933
Signed-off-by: Ralph Boehme <slow at samba.org>
---
source3/script/tests/test_acl_xattr.sh | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/source3/script/tests/test_acl_xattr.sh b/source3/script/tests/test_acl_xattr.sh
index bc3326cf919..463969363fb 100755
--- a/source3/script/tests/test_acl_xattr.sh
+++ b/source3/script/tests/test_acl_xattr.sh
@@ -109,8 +109,8 @@ nt_affects_chgrp() {
test "$b4_expected" != "$af_expected" || exit 1
b4_actual=$($SMBCLIENT //$SERVER/$share -U $USERNAME%$PASSWORD -c "getfacl $fname" 2>/dev/null) || exit 1
- b4_actual=$(echo "$b4_actual" | sed -rn 's/^# group: (.*)/\1/p')
echo "${b4_actual}" | grep -q "^# group:" || exit 1
+ b4_actual=$(echo "$b4_actual" | sed -rn 's/^# group: (.*)/\1/p')
$SMBCACLS //$SERVER/$share $fname -U $USERNAME%$PASSWORD -G domadmins 2>/dev/null || exit 1
af_actual=$($SMBCLIENT //$SERVER/$share -U $USERNAME%$PASSWORD -c "getfacl $fname" 2>/dev/null) || exit 1
echo "${af_actual}" | grep -q "^# group:" || exit 1
--
2.13.5
From 2c8333b27ab1bafd714c7095a2dbe7dd71023f07 Mon Sep 17 00:00:00 2001
From: Ralph Boehme <slow at samba.org>
Date: Sun, 8 Oct 2017 08:51:05 +0200
Subject: [PATCH 04/11] selftest: fix acl_xattr test: sn-devel unreliable gid
The "nt_affects_chgrp" kept failing in a full autobuild on sn-devel
because the actual gid of the created file as returned by smbclient -c
getfacl was reliably the unix gid of my account. It should habe been the
mapped domusers group for the primary users "Domain Users"
group. Running the test individually or even the full set of
"samba3.blackbox" tests didn't trigger the error.
Looks like an issue with vfs_fake_acls and vfs_xattr_tdb, but I wasn't
able to track it down. As the test only really want to ensure that
smbcacls -G set the gid to the requested value, just remove the check
for the actual initial gid.
Bug: https://bugzilla.samba.org/show_bug.cgi?id=7933
Signed-off-by: Ralph Boehme <slow at samba.org>
---
source3/script/tests/test_acl_xattr.sh | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/source3/script/tests/test_acl_xattr.sh b/source3/script/tests/test_acl_xattr.sh
index 463969363fb..facd1dad5af 100755
--- a/source3/script/tests/test_acl_xattr.sh
+++ b/source3/script/tests/test_acl_xattr.sh
@@ -117,7 +117,7 @@ nt_affects_chgrp() {
af_actual=$(echo "$af_actual" | sed -rn 's/^# group: (.*)/\1/p')
echo "before: $b4_actual"
echo "after: $af_actual"
- test "$b4_expected" = "$b4_actual" && test "$af_expected" = "$af_actual"
+ test "$af_expected" != "$b4_actual" && test "$af_expected" = "$af_actual"
}
testit "setup remote file tmp" setup_remote_file tmp
--
2.13.5
From e9ac9445b49b19af0c4732593a74542ccc061798 Mon Sep 17 00:00:00 2001
From: Ralph Boehme <slow at samba.org>
Date: Sun, 8 Oct 2017 11:17:12 +0200
Subject: [PATCH 05/11] selftest: fix acl_xattr test script test_acl_xattr.sh
The two "nt_affects_chgrp" tests called the wrong function so the
function nt_affects_chgrp() was never run.
Bug: https://bugzilla.samba.org/show_bug.cgi?id=7933
Signed-off-by: Ralph Boehme <slow at samba.org>
---
source3/script/tests/test_acl_xattr.sh | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/source3/script/tests/test_acl_xattr.sh b/source3/script/tests/test_acl_xattr.sh
index facd1dad5af..cba79122045 100755
--- a/source3/script/tests/test_acl_xattr.sh
+++ b/source3/script/tests/test_acl_xattr.sh
@@ -130,5 +130,5 @@ testit "nt_affects_chown tmp" nt_affects_chown tmp
testit "nt_affects_chown ign_sysacls" nt_affects_chown ign_sysacls
testit "setup remote file tmp" setup_remote_file tmp
testit "setup remote file ign_sysacls" setup_remote_file ign_sysacls
-testit "nt_affects_chgrp tmp" nt_affects_chown tmp
-testit "nt_affects_chgrp ign_sysacls" nt_affects_chown ign_sysacls
+testit "nt_affects_chgrp tmp" nt_affects_chgrp tmp
+testit "nt_affects_chgrp ign_sysacls" nt_affects_chgrp ign_sysacls
--
2.13.5
From f1e8c425b6ed166e1e952b167b4904b05fdea989 Mon Sep 17 00:00:00 2001
From: Ralph Boehme <slow at samba.org>
Date: Sat, 7 Oct 2017 09:11:56 +0200
Subject: [PATCH 06/11] selftest: fix samba3.blackbox.inherit_owner.default
test script test_inherit_owner.sh
Grant the test-user SeRestorePrivilege, this is needed for
give-ownership operations. And then granting SeRestorePrivilege requires
`net`, so add that as an additional argument to the script.
Bug: https://bugzilla.samba.org/show_bug.cgi?id=7933
Signed-off-by: Ralph Boehme <slow at samba.org>
---
source3/script/tests/test_inherit_owner.sh | 17 +++++++++++------
source3/selftest/tests.py | 12 ++++++------
2 files changed, 17 insertions(+), 12 deletions(-)
diff --git a/source3/script/tests/test_inherit_owner.sh b/source3/script/tests/test_inherit_owner.sh
index 5146d406546..9f22a2c1cba 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 NET SHARE INH_WIN INH_UNIX <additional args>
EOF
exit 1;
fi
@@ -17,13 +17,15 @@ PASSWORD="$3"
PREFIX="$4"
SMBCLIENT="$5"
SMBCACLS="$6"
-SHARE="$7"
-INH_WIN="$8"
-INH_UNIX="$9"
-shift 9
+NET="$7"
+SHARE="$8"
+INH_WIN="$9"
+INH_UNIX="${10}"
+shift 10
ADDARGS="$*"
SMBCLIENT="$VALGRIND ${SMBCLIENT} ${ADDARGS}"
SMBCACLS="$VALGRIND ${SMBCACLS} ${ADDARGS}"
+NET="$VALGRIND ${NET}"
incdir=`dirname $0`/../../../testprogs/blackbox
. $incdir/subunit.sh
@@ -137,6 +139,7 @@ fi
# SETUP
testit "$TEST_LABEL - setup root dir" create_dir tmp tmp.$$
+testit "grant SeRestorePrivilege" $NET rpc rights grant $USERNAME SeRestorePrivilege -U $USERNAME%$PASSWORD -I $SERVER || exit 1
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
# END SETUP
@@ -155,3 +158,5 @@ testit "$TEST_LABEL - verify file unix owner after change" unix_owner_id_is $SHA
testit "$TEST_LABEL - cleanup subdir" cleanup_dir $SHARE tmp.$$/subdir
testit "$TEST_LABEL - cleanup file" cleanup_file $SHARE tmp.$$/afile
testit "$TEST_LABEL - cleanup root" cleanup_dir $SHARE tmp.$$
+
+testit "revoke SeRestorePrivilege" $NET rpc rights revoke $USERNAME SeRestorePrivilege -U $USERNAME%$PASSWORD -I $SERVER || exit 1
diff --git a/source3/selftest/tests.py b/source3/selftest/tests.py
index 1c40e7e049b..4137e2cb46b 100755
--- a/source3/selftest/tests.py
+++ b/source3/selftest/tests.py
@@ -252,12 +252,12 @@ for env in ["fileserver"]:
plantestsuite("samba3.blackbox.acl_xattr.NT1", env, [os.path.join(samba3srcdir, "script/tests/test_acl_xattr.sh"), '$SERVER', '$USERNAME', '$PASSWORD', '$PREFIX', smbclient3, smbcacls, '-mNT1'])
plantestsuite("samba3.blackbox.acl_xattr.SMB3", env, [os.path.join(samba3srcdir, "script/tests/test_acl_xattr.sh"), '$SERVER', '$USERNAME', '$PASSWORD', '$PREFIX', smbclient3, smbcacls, '-mSMB3'])
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.NT1", 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.default.SMB3", env, [os.path.join(samba3srcdir, "script/tests/test_inherit_owner.sh"), '$SERVER', '$USERNAME', '$PASSWORD', '$PREFIX', smbclient3, smbcacls, 'tmp', '0', '0', '-m', 'SMB3'])
- plantestsuite("samba3.blackbox.inherit_owner.full.NT1", 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.full.SMB3", env, [os.path.join(samba3srcdir, "script/tests/test_inherit_owner.sh"), '$SERVER', '$USERNAME', '$PASSWORD', '$PREFIX', smbclient3, smbcacls, 'inherit_owner', '1', '1', '-m', 'SMB3'])
- plantestsuite("samba3.blackbox.inherit_owner.unix.NT1", 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.unix.SMB3", env, [os.path.join(samba3srcdir, "script/tests/test_inherit_owner.sh"), '$SERVER', '$USERNAME', '$PASSWORD', '$PREFIX', smbclient3, smbcacls, 'inherit_owner_u', '0', '1', '-m', 'SMB3'])
+ plantestsuite("samba3.blackbox.inherit_owner.default.NT1", env, [os.path.join(samba3srcdir, "script/tests/test_inherit_owner.sh"), '$SERVER', '$USERNAME', '$PASSWORD', '$PREFIX', smbclient3, smbcacls, net, 'tmp', '0', '0', '-m', 'NT1'])
+ plantestsuite("samba3.blackbox.inherit_owner.default.SMB3", env, [os.path.join(samba3srcdir, "script/tests/test_inherit_owner.sh"), '$SERVER', '$USERNAME', '$PASSWORD', '$PREFIX', smbclient3, smbcacls, net, 'tmp', '0', '0', '-m', 'SMB3'])
+ plantestsuite("samba3.blackbox.inherit_owner.full.NT1", env, [os.path.join(samba3srcdir, "script/tests/test_inherit_owner.sh"), '$SERVER', '$USERNAME', '$PASSWORD', '$PREFIX', smbclient3, smbcacls, net, 'inherit_owner', '1', '1', '-m', 'NT1'])
+ plantestsuite("samba3.blackbox.inherit_owner.full.SMB3", env, [os.path.join(samba3srcdir, "script/tests/test_inherit_owner.sh"), '$SERVER', '$USERNAME', '$PASSWORD', '$PREFIX', smbclient3, smbcacls, net, 'inherit_owner', '1', '1', '-m', 'SMB3'])
+ plantestsuite("samba3.blackbox.inherit_owner.unix.NT1", env, [os.path.join(samba3srcdir, "script/tests/test_inherit_owner.sh"), '$SERVER', '$USERNAME', '$PASSWORD', '$PREFIX', smbclient3, smbcacls, net, 'inherit_owner_u', '0', '1', '-m', 'NT1'])
+ plantestsuite("samba3.blackbox.inherit_owner.unix.SMB3", env, [os.path.join(samba3srcdir, "script/tests/test_inherit_owner.sh"), '$SERVER', '$USERNAME', '$PASSWORD', '$PREFIX', smbclient3, smbcacls, net, 'inherit_owner_u', '0', '1', '-m', 'SMB3'])
plantestsuite("samba3.blackbox.large_acl.NT1", env, [os.path.join(samba3srcdir, "script/tests/test_large_acl.sh"), '$SERVER', '$USERNAME', '$PASSWORD', smbclient3, smbcacls, '-m', 'NT1'])
plantestsuite("samba3.blackbox.large_acl.SMB3", env, [os.path.join(samba3srcdir, "script/tests/test_large_acl.sh"), '$SERVER', '$USERNAME', '$PASSWORD', smbclient3, smbcacls, '-m', 'SMB3'])
--
2.13.5
From c4819271960de779207d33b8e892f3c84c6f8e4c Mon Sep 17 00:00:00 2001
From: Ralph Boehme <slow at samba.org>
Date: Fri, 6 Oct 2017 15:31:20 +0200
Subject: [PATCH 07/11] selftest: tests for change ownership on a file
This test verifies that SEC_STD_WRITE_OWNER only effectively grants
take-ownership permissions but NOT give-ownership. The latter requires
SeRestorePrivilege privilege.
Bug: https://bugzilla.samba.org/show_bug.cgi?id=7933
Signed-off-by: Ralph Boehme <slow at samba.org>
---
selftest/knownfail.d/samba3.blackbox.give_owner | 1 +
source3/script/tests/test_give_owner.sh | 121 ++++++++++++++++++++++++
source3/selftest/tests.py | 1 +
3 files changed, 123 insertions(+)
create mode 100644 selftest/knownfail.d/samba3.blackbox.give_owner
create mode 100755 source3/script/tests/test_give_owner.sh
diff --git a/selftest/knownfail.d/samba3.blackbox.give_owner b/selftest/knownfail.d/samba3.blackbox.give_owner
new file mode 100644
index 00000000000..28fc0c03236
--- /dev/null
+++ b/selftest/knownfail.d/samba3.blackbox.give_owner
@@ -0,0 +1 @@
+samba3.blackbox.give_owner.give owner without SeRestorePrivilege\(fileserver\)
diff --git a/source3/script/tests/test_give_owner.sh b/source3/script/tests/test_give_owner.sh
new file mode 100755
index 00000000000..64e09f3c2b4
--- /dev/null
+++ b/source3/script/tests/test_give_owner.sh
@@ -0,0 +1,121 @@
+#!/bin/sh
+#
+# this verifies that SEC_STD_WRITE_OWNER only effectively grants take-ownership
+# permissions but NOT give-ownership.
+#
+
+if [ $# -lt 9 ]; then
+ echo "Usage: $0 SERVER SERVER_IP USERNAME PASSWORD PREFIX SMBCLIENT SMBCACLS NET SHARE"
+ exit 1
+fi
+
+SERVER="$1"
+SERVER_IP="$2"
+USERNAME="$3"
+PASSWORD="$4"
+PREFIX="$5"
+SMBCLIENT="$6"
+SMBCACLS="$7"
+NET="$8"
+SHARE="$9"
+
+SMBCLIENT="$VALGRIND ${SMBCLIENT}"
+SMBCACLS="$VALGRIND ${SMBCACLS}"
+NET="$VALGRIND ${NET}"
+failed=0
+
+incdir=`dirname $0`/../../../testprogs/blackbox
+. $incdir/subunit.sh
+
+setup_testfile() {
+ local share=$1
+ local fname=$2
+ touch $PREFIX/$fname
+ $SMBCLIENT //$SERVER/$share -U $USERNAME%$PASSWORD -c "rm $fname"
+ $SMBCLIENT //$SERVER/$share -U $USERNAME%$PASSWORD -c "ls" | grep "$fname" && return 1
+ $SMBCLIENT //$SERVER/$share -U $USERNAME%$PASSWORD -c "lcd $PREFIX; put $fname" || return 1
+}
+
+remove_testfile() {
+ local share=$1
+ local fname=$2
+ $SMBCLIENT //$SERVER/$share -U $USERNAME%$PASSWORD -c "rm $fname"
+}
+
+set_win_owner() {
+ local share=$1
+ local fname=$2
+ local owner=$3
+ echo "$SMBCACLS //$SERVER/$share $fname -U $USERNAME%$PASSWORD -C '$owner'"
+ $SMBCACLS //$SERVER/$share $fname -U $USERNAME%$PASSWORD -C "$owner" || return 1
+}
+
+win_owner_is() {
+ local share=$1
+ local fname=$2
+ local expected_owner=$3
+ local actual_owner
+
+ echo "$SMBCACLS //$SERVER/$share $fname -U $USERNAME%$PASSWORD"
+ $SMBCACLS //$SERVER/$share $fname -U $USERNAME%$PASSWORD
+ actual_owner=$($SMBCACLS //$SERVER/$share $fname -U $USERNAME%$PASSWORD | sed -rn 's/^OWNER:(.*)/\1/p')
+ echo "actual_owner = $actual_owner"
+ if ! test "x$actual_owner" = "x$expected_owner" ; then
+ echo "Actual owner of $share/$fname is [$actual_owner] expected [$expected_owner]"
+ return 1
+ fi
+ return 0
+}
+
+add_ace() {
+ local share=$1
+ local fname=$2
+ local ace=$3
+
+ local_ace=$(echo $ace | sed 's|\\|/|')
+
+ # avoid duplicate
+ out=$($SMBCACLS //$SERVER/$share $fname -U $USERNAME%$PASSWORD)
+ echo "$out" | grep "$local_ace" && return 0
+
+ # add it
+ $SMBCACLS //$SERVER/$share $fname -U $USERNAME%$PASSWORD -a "$ace" || return 1
+
+ # check it's there
+ out=$($SMBCACLS //$SERVER/$share $fname -U $USERNAME%$PASSWORD) || return 1
+ echo "$out" | grep "$local_ace" || return 1
+}
+
+chown_give_fails() {
+ local share=$1
+ local fname=$2
+ local user=$3
+ local expected_error=$4
+
+ # this must fail
+ out=$($SMBCACLS //$SERVER/$share $fname -U $USERNAME%$PASSWORD -C "$user") && return 1
+ # it failed, now check it returned the expected error code
+ echo "$out" | grep $expected_error || return 1
+}
+
+# Create a testfile
+testit "create testfile" setup_testfile $SHARE afile || failed=`expr $failed + 1`
+testit "verify owner" win_owner_is $SHARE afile "$SERVER/$USERNAME" || failed=`expr $failed + 1`
+
+# Grant SeRestorePrivilege to the user and full rights on the file
+testit "grant SeRestorePrivilege" $NET rpc rights grant $USERNAME SeRestorePrivilege -U $USERNAME%$PASSWORD -I $SERVER_IP || failed=`expr $failed + 1`
+testit "grant full rights" add_ace $SHARE afile "ACL:$SERVER\\$USERNAME:ALLOWED/0x0/FULL" || failed=`expr $failed + 1`
+
+# We have SeRestorePrivilege, so both give and take ownership must succeed
+testit "give owner with SeRestorePrivilege" set_win_owner $SHARE afile "$SERVER\user1" || failed=`expr $failed + 1`
+testit "verify owner" win_owner_is $SHARE afile "$SERVER/user1" || failed=`expr $failed + 1`
+testit "take owner" set_win_owner $SHARE afile "$SERVER\\$USERNAME" || failed=`expr $failed + 1`
+testit "verify owner" win_owner_is $SHARE afile "$SERVER/$USERNAME" || failed=`expr $failed + 1`
+
+# Revoke SeRestorePrivilege, give ownership must fail now with NT_STATUS_INVALID_OWNER
+testit "revoke SeRestorePrivilege" $NET rpc rights revoke $USERNAME SeRestorePrivilege -U $USERNAME%$PASSWORD -I $SERVER_IP || failed=`expr $failed + 1`
+testit "give owner without SeRestorePrivilege" chown_give_fails $SHARE afile "$SERVER\user1" NT_STATUS_INVALID_OWNER || failed=`expr $failed + 1`
+
+testit "delete testfile" remove_testfile $SHARE afile || failed=`expr $failed + 1`
+
+exit $failed
diff --git a/source3/selftest/tests.py b/source3/selftest/tests.py
index 4137e2cb46b..8efc42c58d0 100755
--- a/source3/selftest/tests.py
+++ b/source3/selftest/tests.py
@@ -260,6 +260,7 @@ for env in ["fileserver"]:
plantestsuite("samba3.blackbox.inherit_owner.unix.SMB3", env, [os.path.join(samba3srcdir, "script/tests/test_inherit_owner.sh"), '$SERVER', '$USERNAME', '$PASSWORD', '$PREFIX', smbclient3, smbcacls, net, 'inherit_owner_u', '0', '1', '-m', 'SMB3'])
plantestsuite("samba3.blackbox.large_acl.NT1", env, [os.path.join(samba3srcdir, "script/tests/test_large_acl.sh"), '$SERVER', '$USERNAME', '$PASSWORD', smbclient3, smbcacls, '-m', 'NT1'])
plantestsuite("samba3.blackbox.large_acl.SMB3", env, [os.path.join(samba3srcdir, "script/tests/test_large_acl.sh"), '$SERVER', '$USERNAME', '$PASSWORD', smbclient3, smbcacls, '-m', 'SMB3'])
+ plantestsuite("samba3.blackbox.give_owner", env, [os.path.join(samba3srcdir, "script/tests/test_give_owner.sh"), '$SERVER', '$SERVER_IP', '$USERNAME', '$PASSWORD', '$PREFIX', smbclient3, smbcacls, net, 'tmp'])
#
# tar command tests
--
2.13.5
From 5f1ed65fd8131f2227db75e2fafd76a55fea9847 Mon Sep 17 00:00:00 2001
From: Ralph Boehme <slow at samba.org>
Date: Wed, 4 Oct 2017 15:45:54 +0200
Subject: [PATCH 08/11] s3/smbd/posix_acls: return correct status in try_chown
Bug: https://bugzilla.samba.org/show_bug.cgi?id=7933
Signed-off-by: Ralph Boehme <slow at samba.org>
---
source3/smbd/posix_acls.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/source3/smbd/posix_acls.c b/source3/smbd/posix_acls.c
index e4403458495..7bd65390406 100644
--- a/source3/smbd/posix_acls.c
+++ b/source3/smbd/posix_acls.c
@@ -3671,7 +3671,7 @@ NTSTATUS try_chown(files_struct *fsp, uid_t uid, gid_t gid)
a local SID on the users workstation
*/
if (uid != get_current_uid(fsp->conn)) {
- return NT_STATUS_ACCESS_DENIED;
+ return NT_STATUS_INVALID_OWNER;
}
become_root();
--
2.13.5
From 30f61f2710ae428839e6c49b229774277aff1277 Mon Sep 17 00:00:00 2001
From: Ralph Boehme <slow at samba.org>
Date: Wed, 4 Oct 2017 12:51:29 +0200
Subject: [PATCH 09/11] vfs_acl_common: factor out a variable declaration
Just some refactoring, no change in behaviour.
Bug: https://bugzilla.samba.org/show_bug.cgi?id=7933
Signed-off-by: Ralph Boehme <slow at samba.org>
---
source3/modules/vfs_acl_common.c | 5 +++--
1 file changed, 3 insertions(+), 2 deletions(-)
diff --git a/source3/modules/vfs_acl_common.c b/source3/modules/vfs_acl_common.c
index c4849b6061f..55f3141dfa7 100644
--- a/source3/modules/vfs_acl_common.c
+++ b/source3/modules/vfs_acl_common.c
@@ -1018,8 +1018,9 @@ static NTSTATUS set_underlying_acl(vfs_handle_struct *handle, files_struct *fsp,
uint32_t security_info_sent,
bool chown_needed)
{
- NTSTATUS status =
- SMB_VFS_NEXT_FSET_NT_ACL(handle, fsp, security_info_sent, psd);
+ NTSTATUS status;
+
+ status = SMB_VFS_NEXT_FSET_NT_ACL(handle, fsp, security_info_sent, psd);
if (!NT_STATUS_EQUAL(status, NT_STATUS_ACCESS_DENIED)) {
return status;
}
--
2.13.5
From 22661d984e15dd9931b030354d4fb19ca09b6899 Mon Sep 17 00:00:00 2001
From: Ralph Boehme <slow at samba.org>
Date: Wed, 4 Oct 2017 22:27:24 +0200
Subject: [PATCH 10/11] vfs_acl_common: fix take ownership vs give ownership
Bug: https://bugzilla.samba.org/show_bug.cgi?id=7933
Signed-off-by: Ralph Boehme <slow at samba.org>
---
source3/modules/vfs_acl_common.c | 13 +++++++++++++
1 file changed, 13 insertions(+)
diff --git a/source3/modules/vfs_acl_common.c b/source3/modules/vfs_acl_common.c
index 55f3141dfa7..7958fd1ca72 100644
--- a/source3/modules/vfs_acl_common.c
+++ b/source3/modules/vfs_acl_common.c
@@ -1019,6 +1019,7 @@ static NTSTATUS set_underlying_acl(vfs_handle_struct *handle, files_struct *fsp,
bool chown_needed)
{
NTSTATUS status;
+ const struct security_token *token = NULL;
status = SMB_VFS_NEXT_FSET_NT_ACL(handle, fsp, security_info_sent, psd);
if (!NT_STATUS_EQUAL(status, NT_STATUS_ACCESS_DENIED)) {
@@ -1033,6 +1034,18 @@ static NTSTATUS set_underlying_acl(vfs_handle_struct *handle, files_struct *fsp,
return NT_STATUS_ACCESS_DENIED;
}
+ /*
+ * Only allow take-ownership, not give-ownership. That's the way Windows
+ * implements SEC_STD_WRITE_OWNER. MS-FSA 2.1.5.16 just states: If
+ * InputBuffer.OwnerSid is not a valid owner SID for a file in the
+ * objectstore, as determined in an implementation specific manner, the
+ * object store MUST return STATUS_INVALID_OWNER.
+ */
+ token = get_current_nttok(fsp->conn);
+ if (!security_token_is_sid(token, psd->owner_sid)) {
+ return NT_STATUS_INVALID_OWNER;
+ }
+
DBG_DEBUG("overriding chown on file %s for sid %s\n",
fsp_str_dbg(fsp), sid_string_tos(psd->owner_sid));
--
2.13.5
From 8439888eb063a18cb7bdf23ca38eac6272b26e91 Mon Sep 17 00:00:00 2001
From: Ralph Boehme <slow at samba.org>
Date: Fri, 6 Oct 2017 15:25:54 +0200
Subject: [PATCH 11/11] vfs_fake_acls: deny give-ownership
Windows doesn't allow giving ownership away unless the user has
SEC_PRIV_RESTORE privilege.
This follows from MS-FSA 2.1.5.1, so it's a property of the filesystem
layer, not the SMB layer. By implementing this restriction here, we can
now have test for this restriction.
Other filesystems may want to deliberately allow this behaviour --
although I'm not aware of any that does -- therefor I'm putting in this
restriction in the implementation of the chmod VFS function and not into
the caller.
Bug: https://bugzilla.samba.org/show_bug.cgi?id=7933
Signed-off-by: Ralph Boehme <slow at samba.org>
---
selftest/knownfail.d/samba3.blackbox.give_owner | 1 -
source3/modules/vfs_fake_acls.c | 18 ++++++++++++++++++
2 files changed, 18 insertions(+), 1 deletion(-)
delete mode 100644 selftest/knownfail.d/samba3.blackbox.give_owner
diff --git a/selftest/knownfail.d/samba3.blackbox.give_owner b/selftest/knownfail.d/samba3.blackbox.give_owner
deleted file mode 100644
index 28fc0c03236..00000000000
--- a/selftest/knownfail.d/samba3.blackbox.give_owner
+++ /dev/null
@@ -1 +0,0 @@
-samba3.blackbox.give_owner.give owner without SeRestorePrivilege\(fileserver\)
diff --git a/source3/modules/vfs_fake_acls.c b/source3/modules/vfs_fake_acls.c
index 7de5cf00bd6..0f539d1f29c 100644
--- a/source3/modules/vfs_fake_acls.c
+++ b/source3/modules/vfs_fake_acls.c
@@ -413,6 +413,12 @@ static int fake_acls_chown(vfs_handle_struct *handle,
int ret;
uint8_t id_buf[4];
if (uid != -1) {
+ uid_t current_uid = get_current_uid(handle->conn);
+
+ if (current_uid != 0 && current_uid != uid) {
+ return EACCES;
+ }
+
SIVAL(id_buf, 0, uid);
ret = SMB_VFS_NEXT_SETXATTR(handle,
smb_fname,
@@ -447,6 +453,12 @@ static int fake_acls_lchown(vfs_handle_struct *handle,
int ret;
uint8_t id_buf[4];
if (uid != -1) {
+ uid_t current_uid = get_current_uid(handle->conn);
+
+ if (current_uid != 0 && current_uid != uid) {
+ return EACCES;
+ }
+
/* This isn't quite right (calling setxattr not
* lsetxattr), but for the test purposes of this
* module (fake NT ACLs from windows clients), it is
@@ -486,6 +498,12 @@ static int fake_acls_fchown(vfs_handle_struct *handle, files_struct *fsp, uid_t
int ret;
uint8_t id_buf[4];
if (uid != -1) {
+ uid_t current_uid = get_current_uid(handle->conn);
+
+ if (current_uid != 0 && current_uid != uid) {
+ return EACCES;
+ }
+
SIVAL(id_buf, 0, uid);
ret = SMB_VFS_NEXT_FSETXATTR(handle, fsp, FAKE_UID, id_buf, sizeof(id_buf), 0);
if (ret != 0) {
--
2.13.5
More information about the samba-technical
mailing list