[PATCH] Fix change-ownership vs give-ownership behaviour in vfs_acl_xattr|tdb and vfs_fake_acl
Jeremy Allison
jra at samba.org
Mon Oct 9 20:58:57 UTC 2017
On Sun, Oct 08, 2017 at 06:27:38PM +0200, Ralph Böhme wrote:
> 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!
That is a *REALLY* nice patchset, thanks Ralph and well done
on adding the tests and tracking down the issue.
RB+ and pushed.
Cheers,
Jeremy.
> 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