From e8f8ad5fdee3e21d53f77aa2d693e14813cf470d Mon Sep 17 00:00:00 2001 From: Ralph Boehme Date: Sun, 8 Oct 2017 11:12:48 +0200 Subject: [PATCH 01/12] 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 Reviewed-by: Jeremy Allison (cherry picked from commit 0f8de2dee5451c9791f96050f85e4f007bec2819) --- 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 9b8808df78a..0d00bf0934f 100755 --- a/source3/script/tests/test_acl_xattr.sh +++ b/source3/script/tests/test_acl_xattr.sh @@ -72,7 +72,8 @@ nt_affects_chown() { b4_actual=$($SMBCLIENT //$SERVER/$share -U $USERNAME%$PASSWORD -c "getfacl $fname" 2>/dev/null) || 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 af_actual=$(echo "$af_actual" | sed -rn 's/^# owner: (.*)/\1/p') echo "before: $b4_actual" -- 2.13.5 From 67b23f68a11cd1e273321bb5379387b02166ea5f Mon Sep 17 00:00:00 2001 From: Ralph Boehme Date: Sun, 8 Oct 2017 11:13:46 +0200 Subject: [PATCH 02/12] 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 Reviewed-by: Jeremy Allison (cherry picked from commit 71a2d06a1e41a1c412c82e58b3966e14c29c6159) --- 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 0d00bf0934f..1b423f21876 100755 --- a/source3/script/tests/test_acl_xattr.sh +++ b/source3/script/tests/test_acl_xattr.sh @@ -94,8 +94,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 5aead9bdba58d672d9f73790c6350f6923cdb235 Mon Sep 17 00:00:00 2001 From: Ralph Boehme Date: Sun, 8 Oct 2017 08:51:05 +0200 Subject: [PATCH 03/12] 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 have 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 Reviewed-by: Jeremy Allison (cherry picked from commit ea0ea829f5af63ab9638e758631c3002cbb6b4ce) --- 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 1b423f21876..82dd741776e 100755 --- a/source3/script/tests/test_acl_xattr.sh +++ b/source3/script/tests/test_acl_xattr.sh @@ -109,7 +109,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 b61e74c2bc48c7f86e190b7583e4a9baea7012ce Mon Sep 17 00:00:00 2001 From: Ralph Boehme Date: Sun, 8 Oct 2017 11:17:12 +0200 Subject: [PATCH 04/12] 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 Reviewed-by: Jeremy Allison (cherry picked from commit 3aff6315097cc9db0216bc18aa793a996930b0f4) --- 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 82dd741776e..e4c556d08d4 100755 --- a/source3/script/tests/test_acl_xattr.sh +++ b/source3/script/tests/test_acl_xattr.sh @@ -122,5 +122,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 999c933d6a8890cf1cf29b1c55a6c7362d05d802 Mon Sep 17 00:00:00 2001 From: Ralph Boehme Date: Sat, 7 Oct 2017 09:11:56 +0200 Subject: [PATCH 05/12] 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 Reviewed-by: Jeremy Allison (backported from commit ff199d8e3ea7bd1ed12de8c39340ba640a2b83ca) --- source3/script/tests/test_inherit_owner.sh | 17 +++++++++++------ source3/selftest/tests.py | 6 +++--- 2 files changed, 14 insertions(+), 9 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 < +Usage: $0 SERVER USERNAME PASSWORD PREFIX SMBCLIENT SMBCACLS NET SHARE INH_WIN INH_UNIX 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 746f3ea0bf6..f9280d6c6d0 100755 --- a/source3/selftest/tests.py +++ b/source3/selftest/tests.py @@ -221,9 +221,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, net, '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, net, '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, net, 'inherit_owner_u', '0', '1', '-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.13.5 From 6fdc05b785096c72ae873ad22d19222a4cb409e4 Mon Sep 17 00:00:00 2001 From: Ralph Boehme Date: Fri, 6 Oct 2017 15:31:20 +0200 Subject: [PATCH 06/12] 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 Reviewed-by: Jeremy Allison (backported from commit 4b2e171e6b90f9c3594ebb705a28c66b981c2bf5) --- 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 f9280d6c6d0..b7540d5cf59 100755 --- a/source3/selftest/tests.py +++ b/source3/selftest/tests.py @@ -225,6 +225,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, net, '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, net, 'inherit_owner_u', '0', '1', '-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]) + plantestsuite("samba3.blackbox.give_owner (%s)" % env, 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 93a98dae6df6eb95cac07050e97dcbaa0b2593f6 Mon Sep 17 00:00:00 2001 From: Ralph Boehme Date: Wed, 4 Oct 2017 15:45:54 +0200 Subject: [PATCH 07/12] 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 Reviewed-by: Jeremy Allison (cherry picked from commit cc555be4d01c4140445bd30e16be3fe8343d3872) --- 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 6999c830c96..26ef113874e 100644 --- a/source3/smbd/posix_acls.c +++ b/source3/smbd/posix_acls.c @@ -3665,7 +3665,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 01b2fd12df9ec7461a01109f49e763af035e6081 Mon Sep 17 00:00:00 2001 From: Ralph Boehme Date: Wed, 4 Oct 2017 12:51:29 +0200 Subject: [PATCH 08/12] 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 Reviewed-by: Jeremy Allison (cherry picked from commit e62f90a6d15626a2e20ba92f5cd552101ec4afe0) --- 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 fa65fd19cfd..b887e0ac9b1 100644 --- a/source3/modules/vfs_acl_common.c +++ b/source3/modules/vfs_acl_common.c @@ -1056,8 +1056,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 8b6a98973d285343ed19b3a339f92275018905f5 Mon Sep 17 00:00:00 2001 From: Ralph Boehme Date: Wed, 4 Oct 2017 22:27:24 +0200 Subject: [PATCH 09/12] vfs_acl_common: fix take ownership vs give ownership Bug: https://bugzilla.samba.org/show_bug.cgi?id=7933 Signed-off-by: Ralph Boehme Reviewed-by: Jeremy Allison (cherry picked from commit 7e7afef819b4a858e6de48389c6f4fa7510cf5c6) --- 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 b887e0ac9b1..42eed028ed9 100644 --- a/source3/modules/vfs_acl_common.c +++ b/source3/modules/vfs_acl_common.c @@ -1057,6 +1057,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)) { @@ -1071,6 +1072,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 93f9f9c9d0e0649421bf646ee03c1bdd51b475fd Mon Sep 17 00:00:00 2001 From: Ralph Boehme Date: Fri, 6 Oct 2017 15:25:54 +0200 Subject: [PATCH 10/12] 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 Reviewed-by: Jeremy Allison (cherry picked from commit 0666093cb0d820cc27a265c1f0a87bc76cd3c167) --- 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 55ff7db37df..b1584ad5274 100644 --- a/source3/modules/vfs_fake_acls.c +++ b/source3/modules/vfs_fake_acls.c @@ -401,6 +401,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->base_name, @@ -435,6 +441,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 @@ -474,6 +486,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 From 4057c2ba0782e4204b50a6f443a2238fabad24d6 Mon Sep 17 00:00:00 2001 From: Ralph Boehme Date: Thu, 12 Oct 2017 17:07:15 +0200 Subject: [PATCH 11/12] selftest: add some debugging to test_give_owner.sh Bug: https://bugzilla.samba.org/show_bug.cgi?id=7933 Signed-off-by: Ralph Boehme Reviewed-by: Jeremy Allison Autobuild-User(master): Jeremy Allison Autobuild-Date(master): Fri Oct 13 01:22:05 CEST 2017 on sn-devel-144 (cherry picked from commit 156015aed0b5a72b2f7150beb5cdaffa32b554e5) --- source3/script/tests/test_give_owner.sh | 24 ++++++++++++++++++++++-- 1 file changed, 22 insertions(+), 2 deletions(-) diff --git a/source3/script/tests/test_give_owner.sh b/source3/script/tests/test_give_owner.sh index 64e09f3c2b4..c8f437ebd8b 100755 --- a/source3/script/tests/test_give_owner.sh +++ b/source3/script/tests/test_give_owner.sh @@ -76,14 +76,34 @@ add_ace() { # avoid duplicate out=$($SMBCACLS //$SERVER/$share $fname -U $USERNAME%$PASSWORD) + if [ $? -ne 0 ] ; then + echo "get acl failed" + echo "$out" + return 1 + fi + echo "Original ACL" + echo $out echo "$out" | grep "$local_ace" && return 0 # add it - $SMBCACLS //$SERVER/$share $fname -U $USERNAME%$PASSWORD -a "$ace" || return 1 + $SMBCACLS //$SERVER/$share $fname -U $USERNAME%$PASSWORD -a "$ace" + if [ $? -ne 0 ] ; then + echo "add acl failed" + return 1 + fi # check it's there - out=$($SMBCACLS //$SERVER/$share $fname -U $USERNAME%$PASSWORD) || return 1 + out=$($SMBCACLS //$SERVER/$share $fname -U $USERNAME%$PASSWORD) + if [ $? -ne 0 ] ; then + echo "get new acl failed" + echo "$out" + return 1 + fi + echo "New ACL" + echo $out + echo "Checking if new ACL has \"$local_ace\"" echo "$out" | grep "$local_ace" || return 1 + echo "ok" } chown_give_fails() { -- 2.13.5 From 0366de5b64b2b8cb198add9faffacff7508e98b3 Mon Sep 17 00:00:00 2001 From: Ralph Boehme Date: Fri, 13 Oct 2017 14:32:58 +0200 Subject: [PATCH 12/12] selftest: prevent interpretation of escape sequences in test_give_owner.sh MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Bug: https://bugzilla.samba.org/show_bug.cgi?id=7933 Signed-off-by: Ralph Boehme Reviewed-by: Volker Lendecke Reviewed-by: Andreas Schneider Autobuild-User(master): Ralph Böhme Autobuild-Date(master): Sat Oct 14 06:02:50 CEST 2017 on sn-devel-144 (cherry picked from commit 7abf0acef48cb585fa8e5666fd4c27692b9c8ae3) --- source3/script/tests/test_give_owner.sh | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/source3/script/tests/test_give_owner.sh b/source3/script/tests/test_give_owner.sh index c8f437ebd8b..7ee37341b42 100755 --- a/source3/script/tests/test_give_owner.sh +++ b/source3/script/tests/test_give_owner.sh @@ -72,7 +72,7 @@ add_ace() { local fname=$2 local ace=$3 - local_ace=$(echo $ace | sed 's|\\|/|') + local_ace=$(printf '%s' "$ace" | sed 's|\\|/|') # avoid duplicate out=$($SMBCACLS //$SERVER/$share $fname -U $USERNAME%$PASSWORD) -- 2.13.5