From 66e6d3f128f22262e4be564c40ddc708725b6ed3 Mon Sep 17 00:00:00 2001 From: Andrew Date: Sat, 20 Mar 2021 01:50:46 -0400 Subject: [PATCH] Fix regression in POSIX mode behavior Commit 235a85657 introduced a regression in evaluation of POSIX modes that require group DENY entries in the internal ZFS ACL. An example of such a POSX mode is 007. When write_implies_delete_child is set, then ACE_WRITE_DATA is added to `wanted_dirperms` in prior to calling zfs_zaccess_common(). This occurs is zfs_zaccess_delete(). Unfortunately, when zfs_zaccess_aces_check hits this particular DENY ACE, zfs_groupmember() is checked to determine whether access should be denied, and since zfs_groupmember() always returns B_TRUE on Linux and so this check is failed, resulting ultimately in EPERM being returned. Reviewed-by: Brian Behlendorf Reviewed-by: Ryan Moeller Signed-off-by: Andrew Walker Closes #11760 --- module/zfs/zfs_fuid.c | 4 - tests/runfiles/common.run | 4 + tests/runfiles/sanity.run | 4 + .../tests/functional/acl/off/Makefile.am | 1 + .../tests/functional/acl/off/posixmode.ksh | 145 ++++++++++++++++++ 5 files changed, 154 insertions(+), 4 deletions(-) create mode 100755 tests/zfs-tests/tests/functional/acl/off/posixmode.ksh diff --git a/module/zfs/zfs_fuid.c b/module/zfs/zfs_fuid.c index 015dde4811..a90bf5feee 100644 --- a/module/zfs/zfs_fuid.c +++ b/module/zfs/zfs_fuid.c @@ -728,7 +728,6 @@ zfs_fuid_info_free(zfs_fuid_info_t *fuidp) boolean_t zfs_groupmember(zfsvfs_t *zfsvfs, uint64_t id, cred_t *cr) { -#ifdef HAVE_KSID uid_t gid; #ifdef illumos @@ -773,9 +772,6 @@ zfs_groupmember(zfsvfs_t *zfsvfs, uint64_t id, cred_t *cr) */ gid = zfs_fuid_map_id(zfsvfs, id, cr, ZFS_GROUP); return (groupmember(gid, cr)); -#else - return (B_TRUE); -#endif } void diff --git a/tests/runfiles/common.run b/tests/runfiles/common.run index 2c5ccaa990..cc24145508 100644 --- a/tests/runfiles/common.run +++ b/tests/runfiles/common.run @@ -28,6 +28,10 @@ failsafe = callbacks/zfs_failsafe outputdir = /var/tmp/test_results tags = ['functional'] +[tests/functional/acl/off] +tests = ['posixmode'] +tags = ['functional', 'acl'] + [tests/functional/alloc_class] tests = ['alloc_class_001_pos', 'alloc_class_002_neg', 'alloc_class_003_pos', 'alloc_class_004_pos', 'alloc_class_005_pos', 'alloc_class_006_pos', diff --git a/tests/runfiles/sanity.run b/tests/runfiles/sanity.run index 3200261d81..e32cf5f622 100644 --- a/tests/runfiles/sanity.run +++ b/tests/runfiles/sanity.run @@ -30,6 +30,10 @@ failsafe = callbacks/zfs_failsafe outputdir = /var/tmp/test_results tags = ['functional'] +[tests/functional/acl/off] +tests = ['posixmode'] +tags = ['functional', 'acl'] + [tests/functional/alloc_class] tests = ['alloc_class_003_pos', 'alloc_class_004_pos', 'alloc_class_005_pos', 'alloc_class_006_pos', 'alloc_class_008_pos', 'alloc_class_010_pos', diff --git a/tests/zfs-tests/tests/functional/acl/off/Makefile.am b/tests/zfs-tests/tests/functional/acl/off/Makefile.am index df8adcafef..36aa13dd03 100644 --- a/tests/zfs-tests/tests/functional/acl/off/Makefile.am +++ b/tests/zfs-tests/tests/functional/acl/off/Makefile.am @@ -4,6 +4,7 @@ pkgdatadir = $(datadir)/@PACKAGE@/zfs-tests/tests/functional/acl/off dist_pkgdata_SCRIPTS = \ dosmode.ksh \ + posixmode.ksh \ cleanup.ksh \ setup.ksh diff --git a/tests/zfs-tests/tests/functional/acl/off/posixmode.ksh b/tests/zfs-tests/tests/functional/acl/off/posixmode.ksh new file mode 100755 index 0000000000..63870caa32 --- /dev/null +++ b/tests/zfs-tests/tests/functional/acl/off/posixmode.ksh @@ -0,0 +1,145 @@ +#!/bin/ksh -p +# +# CDDL HEADER START +# +# The contents of this file are subject to the terms of the +# Common Development and Distribution License (the "License"). +# You may not use this file except in compliance with the License. +# +# You can obtain a copy of the license at usr/src/OPENSOLARIS.LICENSE +# or http://www.opensolaris.org/os/licensing. +# See the License for the specific language governing permissions +# and limitations under the License. +# +# When distributing Covered Code, include this CDDL HEADER in each +# file and include the License file at usr/src/OPENSOLARIS.LICENSE. +# If applicable, add the following below this CDDL HEADER, with the +# fields enclosed by brackets "[]" replaced with your own identifying +# information: Portions Copyright [yyyy] [name of copyright owner] +# +# CDDL HEADER END +# + +# +# Portions Copyright 2021 iXsystems, Inc. +# + +. $STF_SUITE/include/libtest.shlib +. $STF_SUITE/tests/functional/acl/acl_common.kshlib + +# +# DESCRIPTION: +# Verify that POSIX mode bits function correctly. +# +# These tests are incomplete and will be added to over time. +# +# NOTE: Creating directory entries behaves differently between platforms. +# The parent directory's group is used on FreeBSD, while the effective +# group is used on Linux. We chown to the effective group when creating +# directories and files in these tests to achieve consistency across all +# platforms. +# +# STRATEGY: +# 1. Sanity check the POSIX mode test on tmpfs +# 2. Test POSIX mode bits on ZFS +# + +verify_runnable "both" + +function cleanup +{ + umount -f $tmpdir + rm -rf $tmpdir $TESTDIR/dir +} + +log_assert "Verify POSIX mode bits function correctly" +log_onexit cleanup + +owner=$ZFS_ACL_STAFF1 +other=$ZFS_ACL_STAFF2 +group=$ZFS_ACL_STAFF_GROUP +if is_linux; then + wheel=root +else + wheel=wheel +fi + +function test_posix_mode # base +{ + typeset base=$1 + typeset dir=$base/dir + typeset file=$dir/file + + # dir owned by root + log_must mkdir $dir + log_must chown :$wheel $dir + log_must chmod 007 $dir + + # file owned by root + log_must touch $file + log_must chown :$wheel $file + log_must ls -la $dir + log_must rm $file + + log_must touch $file + log_must chown :$wheel $file + log_must user_run $other rm $file + + # file owned by user + log_must user_run $owner touch $file + log_must chown :$group $file + log_must ls -la $dir + log_must user_run $owner rm $file + + log_must user_run $owner touch $file + log_must chown :$group $file + log_must user_run $other rm $file + + log_must user_run $owner touch $file + log_must chown :$group $file + log_must rm $file + + log_must rm -rf $dir + + # dir owned by user + log_must user_run $owner mkdir $dir + log_must chown :$group $dir + log_must user_run $owner chmod 007 $dir + + # file owned by root + log_must touch $file + log_must chown :$wheel $file + log_must ls -la $dir + log_must rm $file + + log_must touch $file + log_must chown :$wheel $file + log_mustnot user_run $other rm $file + log_must rm $file + + # file owned by user + log_mustnot user_run $owner touch $file + log_must touch $file + log_must chown $owner:$group $file + log_must ls -la $dir + log_mustnot user_run $owner rm $file + log_mustnot user_run $other rm $file + log_must rm $file + + log_must rm -rf $dir +} + +# Sanity check on tmpfs first +tmpdir=$(TMPDIR=$TEST_BASE_DIR mktemp -d) +log_must mount -t tmpfs tmp $tmpdir +log_must chmod 777 $tmpdir + +test_posix_mode $tmpdir + +log_must umount $tmpdir +log_must rmdir $tmpdir + +# Verify ZFS +test_posix_mode $TESTDIR + +log_pass "POSIX mode bits function correctly"