[PATCH] vfs_acl_xattr: avoid setting POSIX acls if "ignore system acls" is set
Uri Simchoni
uri at samba.org
Tue Mar 22 08:19:42 UTC 2016
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,
Uri.
-------------- next part --------------
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