Skip to content

Commit

Permalink
Fix regression in POSIX mode behavior
Browse files Browse the repository at this point in the history
Commit 235a856 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 <[email protected]>
Reviewed-by: Ryan Moeller <[email protected]>
Signed-off-by: Andrew Walker <[email protected]>
Closes openzfs#11760
  • Loading branch information
anodos325 authored and RageLtMan committed May 31, 2021
1 parent 23b1950 commit c9ae64e
Show file tree
Hide file tree
Showing 5 changed files with 154 additions and 4 deletions.
4 changes: 0 additions & 4 deletions module/zfs/zfs_fuid.c
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -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
Expand Down
4 changes: 4 additions & 0 deletions tests/runfiles/common.run
Original file line number Diff line number Diff line change
Expand Up @@ -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',
Expand Down
4 changes: 4 additions & 0 deletions tests/runfiles/sanity.run
Original file line number Diff line number Diff line change
Expand Up @@ -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',
Expand Down
1 change: 1 addition & 0 deletions tests/zfs-tests/tests/functional/acl/off/Makefile.am
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@ pkgdatadir = $(datadir)/@PACKAGE@/zfs-tests/tests/functional/acl/off

dist_pkgdata_SCRIPTS = \
dosmode.ksh \
posixmode.ksh \
cleanup.ksh \
setup.ksh

Expand Down
145 changes: 145 additions & 0 deletions tests/zfs-tests/tests/functional/acl/off/posixmode.ksh
Original file line number Diff line number Diff line change
@@ -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"

0 comments on commit c9ae64e

Please sign in to comment.