[PATCH] vfs_acl_xattr: avoid setting POSIX acls if "ignore system acls" is set
Jeremy Allison
jra at samba.org
Tue Mar 22 23:34:38 UTC 2016
On Tue, Mar 22, 2016 at 10:19:42AM +0200, Uri Simchoni wrote:
> Hi,
>
> Attached patch avoids setting the POSIX ACLs to match the NT ACLs,
> if "ignore system acls" is set.
>
> I believe this new behavior is more consistent with the
> vfs_acl_xattr man page. It also has the following benefits:
> - Avoid unnecessary sid->xid translations
> - Better compatibility with Windows, since by default Windows does
> not enforce existence of "traverse folder" right on parent folders,
> and modifying POSIX ACLs on parent folders could cause the kernel to
> deny access (dunno how RichACLs would handle this one :( ).
>
> Passes local make test.
>
> Please review,
Nice work Uri ! Pushed to autobuild.
Cheers,
Jeremy.
> From d86cae8944e92dcfa5d856893e48f94bedf7e5b4 Mon Sep 17 00:00:00 2001
> From: Uri Simchoni <uri at samba.org>
> Date: Mon, 21 Mar 2016 23:04:24 +0200
> Subject: [PATCH 1/2] vfs_acl_common: avoid setting POSIX ACLs if "ignore
> system acls" is set
>
> When "ignore system acls" is set, do not mess at all with POSIX ACLS,
> do not even calculate the would-be POSIX-ACL-based security descriptor
> (for performance reasons).
> Instead, just store a V3 blob with zero hash. This means that if we
> later read the ACL without ignoring system ACLs, the NT ACL shall be
> reset to the info derivable from the POSIX ACL.
>
> File ownership is still modified as it has bearing on disk quotas.
>
> BUG: https://bugzilla.samba.org/show_bug.cgi?id=11806
>
> Signed-off-by: Uri Simchoni <uri at samba.org>
> ---
> source3/modules/vfs_acl_common.c | 147 ++++++++++++++++++++++++++-------------
> 1 file changed, 99 insertions(+), 48 deletions(-)
>
> diff --git a/source3/modules/vfs_acl_common.c b/source3/modules/vfs_acl_common.c
> index c8c0650..d21c167 100644
> --- a/source3/modules/vfs_acl_common.c
> +++ b/source3/modules/vfs_acl_common.c
> @@ -745,6 +745,81 @@ static NTSTATUS get_nt_acl_common(vfs_handle_struct *handle,
> }
>
> /*********************************************************************
> + Set the underlying ACL (e.g. POSIX ACLS, POSIX owner, etc)
> +*********************************************************************/
> +static NTSTATUS set_underlying_acl(vfs_handle_struct *handle, files_struct *fsp,
> + struct security_descriptor *psd,
> + uint32_t security_info_sent,
> + bool chown_needed)
> +{
> + NTSTATUS status =
> + SMB_VFS_NEXT_FSET_NT_ACL(handle, fsp, security_info_sent, psd);
> + if (!NT_STATUS_EQUAL(status, NT_STATUS_ACCESS_DENIED)) {
> + return status;
> + }
> +
> + /* We got access denied here. If we're already root,
> + or we didn't need to do a chown, or the fsp isn't
> + open with WRITE_OWNER access, just return. */
> + if (get_current_uid(handle->conn) == 0 || chown_needed == false ||
> + !(fsp->access_mask & SEC_STD_WRITE_OWNER)) {
> + return NT_STATUS_ACCESS_DENIED;
> + }
> +
> + DEBUG(10, ("fset_nt_acl_common: overriding chown on file %s "
> + "for sid %s\n",
> + fsp_str_dbg(fsp), sid_string_tos(psd->owner_sid)));
> +
> + /* Ok, we failed to chown and we have
> + SEC_STD_WRITE_OWNER access - override. */
> + become_root();
> + status = SMB_VFS_NEXT_FSET_NT_ACL(handle, fsp, security_info_sent, psd);
> + unbecome_root();
> +
> + return status;
> +}
> +
> +/*********************************************************************
> + Store a v3 security descriptor
> +*********************************************************************/
> +static NTSTATUS store_v3_blob(vfs_handle_struct *handle, files_struct *fsp,
> + struct security_descriptor *psd,
> + struct security_descriptor *pdesc_next,
> + uint8_t hash[XATTR_SD_HASH_SIZE])
> +{
> + NTSTATUS status;
> + DATA_BLOB blob;
> +
> + if (DEBUGLEVEL >= 10) {
> + DEBUG(10, ("fset_nt_acl_xattr: storing xattr sd for file %s\n",
> + fsp_str_dbg(fsp)));
> + NDR_PRINT_DEBUG(
> + security_descriptor,
> + discard_const_p(struct security_descriptor, psd));
> +
> + if (pdesc_next != NULL) {
> + DEBUG(10, ("fset_nt_acl_xattr: storing has in xattr sd "
> + "based on \n"));
> + NDR_PRINT_DEBUG(
> + security_descriptor,
> + discard_const_p(struct security_descriptor,
> + pdesc_next));
> + } else {
> + DEBUG(10,
> + ("fset_nt_acl_xattr: ignoring underlying sd\n"));
> + }
> + }
> + status = create_acl_blob(psd, &blob, XATTR_SD_HASH_TYPE_SHA256, hash);
> + if (!NT_STATUS_IS_OK(status)) {
> + DEBUG(10, ("fset_nt_acl_xattr: create_acl_blob failed\n"));
> + return status;
> + }
> +
> + status = store_acl_blob_fsp(handle, fsp, &blob);
> + return status;
> +}
> +
> +/*********************************************************************
> Store a security descriptor given an fsp.
> *********************************************************************/
>
> @@ -761,6 +836,8 @@ static NTSTATUS fset_nt_acl_common(vfs_handle_struct *handle, files_struct *fsp,
> bool chown_needed = false;
> char *sys_acl_description;
> TALLOC_CTX *frame = talloc_stackframe();
> + bool ignore_file_system_acl = lp_parm_bool(
> + SNUM(handle->conn), ACL_MODULE_NAME, "ignore system acls", false);
>
> if (DEBUGLEVEL >= 10) {
> DEBUG(10,("fset_nt_acl_xattr: incoming sd for file %s\n",
> @@ -816,38 +893,29 @@ static NTSTATUS fset_nt_acl_common(vfs_handle_struct *handle, files_struct *fsp,
> psd->type |= SEC_DESC_SACL_PRESENT;
> }
>
> - status = SMB_VFS_NEXT_FSET_NT_ACL(handle, fsp, security_info_sent, psd);
> - if (!NT_STATUS_IS_OK(status)) {
> - if (!NT_STATUS_EQUAL(status, NT_STATUS_ACCESS_DENIED)) {
> - TALLOC_FREE(frame);
> - return status;
> - }
> - /* We got access denied here. If we're already root,
> - or we didn't need to do a chown, or the fsp isn't
> - open with WRITE_OWNER access, just return. */
> - if (get_current_uid(handle->conn) == 0 ||
> - chown_needed == false ||
> - !(fsp->access_mask & SEC_STD_WRITE_OWNER)) {
> - TALLOC_FREE(frame);
> - return NT_STATUS_ACCESS_DENIED;
> + if (ignore_file_system_acl) {
> + if (chown_needed) {
> + /* send only ownership stuff to lower layer */
> + security_info_sent &= (SECINFO_OWNER | SECINFO_GROUP);
> + status = set_underlying_acl(handle, fsp, psd,
> + security_info_sent, true);
> + if (!NT_STATUS_IS_OK(status)) {
> + TALLOC_FREE(frame);
> + return status;
> + }
> }
> + ZERO_ARRAY(hash);
> + status = store_v3_blob(handle, fsp, psd, NULL, hash);
>
> - DEBUG(10,("fset_nt_acl_common: overriding chown on file %s "
> - "for sid %s\n",
> - fsp_str_dbg(fsp),
> - sid_string_tos(psd->owner_sid)
> - ));
> -
> - /* Ok, we failed to chown and we have
> - SEC_STD_WRITE_OWNER access - override. */
> - become_root();
> - status = SMB_VFS_NEXT_FSET_NT_ACL(handle, fsp,
> - security_info_sent, psd);
> - unbecome_root();
> - if (!NT_STATUS_IS_OK(status)) {
> - TALLOC_FREE(frame);
> - return status;
> - }
> + TALLOC_FREE(frame);
> + return status;
> + }
> +
> + status = set_underlying_acl(handle, fsp, psd, security_info_sent,
> + chown_needed);
> + if (!NT_STATUS_IS_OK(status)) {
> + TALLOC_FREE(frame);
> + return status;
> }
>
> /* Get the full underlying sd, then hash. */
> @@ -878,24 +946,7 @@ static NTSTATUS fset_nt_acl_common(vfs_handle_struct *handle, files_struct *fsp,
> /* If we fail to get the ACL blob (for some reason) then this
> * is not fatal, we just work based on the NT ACL only */
> if (ret != 0) {
> - if (DEBUGLEVEL >= 10) {
> - DEBUG(10,("fset_nt_acl_xattr: storing xattr sd for file %s\n",
> - fsp_str_dbg(fsp)));
> - NDR_PRINT_DEBUG(security_descriptor,
> - discard_const_p(struct security_descriptor, psd));
> -
> - DEBUG(10,("fset_nt_acl_xattr: storing has in xattr sd based on \n"));
> - NDR_PRINT_DEBUG(security_descriptor,
> - discard_const_p(struct security_descriptor, pdesc_next));
> - }
> - status = create_acl_blob(psd, &blob, XATTR_SD_HASH_TYPE_SHA256, hash);
> - if (!NT_STATUS_IS_OK(status)) {
> - DEBUG(10, ("fset_nt_acl_xattr: create_acl_blob failed\n"));
> - TALLOC_FREE(frame);
> - return status;
> - }
> -
> - status = store_acl_blob_fsp(handle, fsp, &blob);
> + status = store_v3_blob(handle, fsp, psd, pdesc_next, hash);
>
> TALLOC_FREE(frame);
> return status;
> --
> 2.5.0
>
>
> From 38f59e2da148579ac054d599044471fd7a72182d Mon Sep 17 00:00:00 2001
> From: Uri Simchoni <uri at samba.org>
> Date: Mon, 21 Mar 2016 23:13:25 +0200
> Subject: [PATCH 2/2] seltest: add test for "ignore system acls" in
> vfs_acl_xattr.
>
> BUG: https://bugzilla.samba.org/show_bug.cgi?id=11806
>
> Signed-off-by: Uri Simchoni <uri at samba.org>
> ---
> selftest/selftesthelpers.py | 1 +
> selftest/target/Samba3.pm | 4 ++
> source3/script/tests/test_acl_xattr.sh | 125 +++++++++++++++++++++++++++++++++
> source3/selftest/tests.py | 1 +
> 4 files changed, 131 insertions(+)
> create mode 100755 source3/script/tests/test_acl_xattr.sh
>
> diff --git a/selftest/selftesthelpers.py b/selftest/selftesthelpers.py
> index f26484b..495a0b7 100644
> --- a/selftest/selftesthelpers.py
> +++ b/selftest/selftesthelpers.py
> @@ -186,3 +186,4 @@ vfstest = binpath('vfstest')
> smbcquotas = binpath('smbcquotas')
> smbget = binpath('smbget')
> rpcclient = binpath('rpcclient')
> +smbcacls = binpath('smbcacls')
> diff --git a/selftest/target/Samba3.pm b/selftest/target/Samba3.pm
> index 5dc4b17..0b16e03 100755
> --- a/selftest/target/Samba3.pm
> +++ b/selftest/target/Samba3.pm
> @@ -649,6 +649,10 @@ sub setup_fileserver($$)
> path = $smbget_sharedir
> comment = smb username is [%U]
> guest ok = yes
> +[ign_sysacls]
> + path = $share_dir
> + comment = ignore system acls
> + acl_xattr:ignore system acls = yes
> ";
>
> my $vars = $self->provision($path,
> diff --git a/source3/script/tests/test_acl_xattr.sh b/source3/script/tests/test_acl_xattr.sh
> new file mode 100755
> index 0000000..9b8808d
> --- /dev/null
> +++ b/source3/script/tests/test_acl_xattr.sh
> @@ -0,0 +1,125 @@
> +#!/bin/sh
> +
> +# this tests acl_xattr config parameter "ignore system acl"
> +
> +if [ $# -lt 6 ]; then
> +cat <<EOF
> +Usage: $0 SERVER USERNAME PASSWORD PREFIX SMBCLIENT SMBCACLS
> +EOF
> +exit 1;
> +fi
> +
> +SERVER="$1"
> +USERNAME="$2"
> +PASSWORD="$3"
> +PREFIX="$4"
> +SMBCLIENT="$5"
> +SMBCACLS="$6"
> +SMBCLIENT="$VALGRIND ${SMBCLIENT}"
> +SMBCACLS="$VALGRIND ${SMBCACLS}"
> +
> +incdir=`dirname $0`/../../../testprogs/blackbox
> +. $incdir/subunit.sh
> +
> +setup_remote_file() {
> + local share=$1
> + local fname="$share.$$"
> + local local_fname=$PREFIX/$fname
> + touch $local_fname
> + $SMBCLIENT //$SERVER/$share -U $USERNAME%$PASSWORD -c "rm $fname"
> + $SMBCLIENT //$SERVER/$share -U $USERNAME%$PASSWORD -c "ls" | grep "$fname" && exit 1
> + $SMBCLIENT //$SERVER/$share -U $USERNAME%$PASSWORD -c "lcd $PREFIX; put $fname" || exit 1
> +}
> +
> +nt_affects_posix() {
> + local share=$1
> + local expected=$2
> + local b4
> + local af
> + local fname="$share.$$"
> + b4=$($SMBCLIENT //$SERVER/$share -U $USERNAME%$PASSWORD -c "getfacl $fname" 2>/dev/null) || exit 1
> + $SMBCACLS //$SERVER/$share $fname -U $USERNAME%$PASSWORD -a "ACL:$SERVER\force_user:ALLOWED/0x0/READ" 2>/dev/null || exit 1
> + af=$($SMBCLIENT //$SERVER/$share -U $USERNAME%$PASSWORD -c "getfacl $fname" 2>/dev/null) || exit 1
> + echo "before: $b4"
> + echo "after: $af"
> + if test "$expected" = "true" ; then
> + test "$b4" != "$af"
> + else
> + test "$b4" = "$af"
> + fi
> +}
> +
> +nt_affects_chown() {
> + local share=$1
> + local b4_expected
> + local af_expected
> + local b4_actual
> + local af_actual
> + local fname="$share.$$"
> +
> + echo -n "determining uid of $USERNAME..."
> + b4_expected=$(getent passwd $USERNAME) || exit 1
> + b4_expected=$(echo "$b4_expected" | awk -F: '{print $3}')
> + echo "$b4_expected"
> +
> + echo -n "determining uid of force_user..."
> + af_expected=$(getent passwd force_user) || exit 1
> + af_expected=$(echo "$af_expected" | awk -F: '{print $3}')
> + echo "$af_expected"
> +
> + #basic sanity...
> + 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/^# owner: (.*)/\1/p')
> + $SMBCACLS //$SERVER/$share $fname -U $USERNAME%$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"
> + echo "after: $af_actual"
> + test "$b4_expected" = "$b4_actual" && test "$af_expected" = "$af_actual"
> +}
> +
> +nt_affects_chgrp() {
> + local share=$1
> + local b4_expected
> + local af_expected
> + local b4_actual
> + local af_actual
> + local fname="$share.$$"
> +
> + echo -n "determining gid of domusers..."
> + b4_expected=$(getent group domusers) || exit 1
> + 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
> + af_expected=$(echo "$af_expected" | awk -F: '{print $3}')
> + echo "$af_expected"
> +
> + #basic sanity...
> + 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')
> + $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
> + 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"
> +}
> +
> +testit "setup remote file tmp" setup_remote_file tmp
> +testit "setup remote file ign_sysacls" setup_remote_file ign_sysacls
> +testit "nt_affects_posix tmp" nt_affects_posix tmp "true"
> +testit "nt_affects_posix ign_sysacls" nt_affects_posix ign_sysacls "false"
> +testit "setup remote file tmp" setup_remote_file tmp
> +testit "setup remote file ign_sysacls" setup_remote_file ign_sysacls
> +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
> diff --git a/source3/selftest/tests.py b/source3/selftest/tests.py
> index c4bed49..54b5136 100755
> --- a/source3/selftest/tests.py
> +++ b/source3/selftest/tests.py
> @@ -187,6 +187,7 @@ for env in ["fileserver"]:
> plantestsuite("samba3.blackbox.smbclient.forceuser_validusers (%s)" % env, env, [os.path.join(samba3srcdir, "script/tests/test_forceuser_validusers.sh"), '$SERVER', '$DOMAIN', '$USERNAME', '$PASSWORD', '$LOCAL_PATH', smbclient3])
> plantestsuite("samba3.blackbox.smbget (%s)" % env, env, [os.path.join(samba3srcdir, "script/tests/test_smbget.sh"), '$SERVER', '$SERVER_IP', '$DOMAIN', 'smbget_user', '$PASSWORD', '$LOCAL_PATH/smbget', smbget])
> 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])
>
> #
> # tar command tests
> --
> 2.5.0
>
More information about the samba-technical
mailing list