Fix regression in POSIX mode behavior #11760

anodos325 · 2021-03-16T19:21:06Z

Commit 235a856 introduced a regression in evaluation of POSIX modes
that require group DENY entries in the internal ZFS ACL (for example 007).
When write_implies_delete_child is set, then ACE_WRITE_DATA is added
to wanted_dirperms in 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.

Simplest path forward is to actually check group membership
in zfs_groupmember() since this will ultimately be a requirement for proper
evaluation of NFSv4 ACLs.

Motivation and Context

The following should succeed.

truenas# mkdir testdir
truenas# chmod 007 testdir
truenas# su smbuser
smbuser@truenas:/mnt/tank$ touch testdir/file
smbuser@truenas:/mnt/tank$ rm testdir/file 
rm: cannot remove 'testdir/file': Operation not permitted

Description

How Has This Been Tested?

Types of changes

Bug fix (non-breaking change which fixes an issue)
New feature (non-breaking change which adds functionality)
Performance enhancement (non-breaking change which improves efficiency)
Code cleanup (non-breaking change which makes code smaller or more readable)
Breaking change (fix or feature that would cause existing functionality to change)
Library ABI change (libzfs, libzfs_core, libnvpair, libuutil and libzfsbootenv)
Documentation (a change to man pages or other documentation)

Checklist:

My code follows the OpenZFS code style requirements.
I have updated the documentation accordingly.
I have read the contributing document.
I have added tests to cover my changes.
I have run the ZFS Test Suite with this change applied.
All commit messages are properly formatted and contain Signed-off-by.

behlendorf · 2021-03-16T20:38:38Z

@pbhenson would you mind taking a look at this.

anodos325 · 2021-03-16T21:06:10Z

It might help to clarify a bit more concretely what a 007 mode looks like when expressed as NFSv4 ACL:

root@FBSD-API-TESTER:~ # getfacl foo
# file: foo
# owner: root
# group: wheel
            owner@:rwxp--aARWcCos:-------:allow
            group@:r-x---a-R-c--s:-------:allow
         everyone@:r-x---a-R-c--s:-------:allow
root@FBSD-API-TESTER:~ # chmod 007 foo
root@FBSD-API-TESTER:~ # getfacl foo
# file: foo
# owner: root
# group: wheel
            owner@:rwxp----------:-------:deny
            group@:rwxp----------:-------:deny
            owner@:------aARWcCos:-------:allow
            group@:------a-R-c--s:-------:allow
         everyone@:rwxp--a-R-c--s:-------:allow

This is on FreeBSD. Because everyone@ defines rights to everyone (including "user" and "group"), explicit DENY entries must be added to properly represent the POSIX mode. This trips us up as we iterate through aces in zfs_zaccess_aces_check. Second entry above is for ACE_IDENTIFIER_GROUP. checkit is always true, deny_mask gets set, and we return EACCES.

anodos325 · 2021-03-17T11:58:47Z

I updated commit message to reflect results of continued testing on root cause of EPERM. Issue results from changes to zfs_zaccess_delete() in 235a856.

We fail here:

	/*
	 * Case 2:
	 * If the containing directory grants ACE_DELETE_CHILD,
	 * or we're in backward compatibility mode and the
	 * containing directory has ACE_WRITE_DATA, allow.
	 * Case 2b is handled with wanted_dirperms.
	 */
	wanted_dirperms = ACE_DELETE_CHILD;
	if (zfs_write_implies_delete_child)
		wanted_dirperms |= ACE_WRITE_DATA;
	dzp_error = zfs_zaccess_common(dzp, wanted_dirperms,
	    &dzp_working_mode, &dzpcheck_privs, B_FALSE, cr);
	if (dzp_error == EACCES) {
		/* We hit a DENY ACE. */
		if (!dzpcheck_privs)			
			return (SET_ERROR(dzp_error));
		return (secpolicy_vnode_remove(cr));
	}

anodos325 · 2021-03-17T14:24:56Z

zfs_groupmember() changes fix this edge case with mode 007, but it appears there are unaddressed edge cases with POSIX ACLs:

truenas# getfacl foo
# file: foo
# owner: root
# group: smbuser
user::---
group::---
group:smbuser:rwx
mask::rwx
other::rwx
truenas# su smbuser
smbuser@truenas:/mnt/dozer$ touch foo/bar
smbuser@truenas:/mnt/dozer$ rm foo/bar 
rm: cannot remove 'foo/bar': Operation not permitted

In this case, the group@ DENY entry prevents delete even though it is permitted by the POSIX1e ACL. In this case though, the DENY does come from zfs_acl_chmod() changes in a1af567, and is written to disk. See acl_trivial_access_masks() where ACE_DELETE_CHILD is added to the write mask. This is perhaps a secondary issue to address in separate issue / merge request.

adamdmoss · 2021-03-17T20:53:10Z

Any chance of a new test-suite case for this?

anodos325 · 2021-03-18T00:55:27Z

Any chance of a new test-suite case for this?

I suppose it wouldn't be too hard to add one, but I don't see functional tests for POSIX mode. Is this something that should be added to POSIX ACL tests or should new tests be written explicitly to test mode?

adamdmoss · 2021-03-18T02:43:11Z

Any chance of a new test-suite case for this?

I suppose it wouldn't be too hard to add one, but I don't see functional tests for POSIX mode. Is this something that should be added to POSIX ACL tests or should new tests be written explicitly to test mode?

Either would be a huge treat compared to the lack of coverage we apparently have right now - so IMHO whatever maximizes the chance of it being done. :)

ghost · 2021-03-19T16:27:59Z

Rebased
Added test

I've started a posixmode test case that covers this. It also performs the test on tmpfs as a sanity check.

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. Signed-off-by: Andrew Walker <awalker@ixsystems.com>

ghost · 2021-03-19T16:33:07Z

Fixed an inaccurate comment in the test case

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 <behlendorf1@llnl.gov> Reviewed-by: Ryan Moeller <ryan@iXsystems.com> Signed-off-by: Andrew Walker <awalker@ixsystems.com> Closes openzfs#11760

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 <behlendorf1@llnl.gov> Reviewed-by: Ryan Moeller <ryan@iXsystems.com> Signed-off-by: Andrew Walker <awalker@ixsystems.com> Closes #11760

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 <behlendorf1@llnl.gov> Reviewed-by: Ryan Moeller <ryan@iXsystems.com> Signed-off-by: Andrew Walker <awalker@ixsystems.com> Closes openzfs#11760

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 <behlendorf1@llnl.gov> Reviewed-by: Ryan Moeller <ryan@iXsystems.com> Signed-off-by: Andrew Walker <awalker@ixsystems.com> Closes #11760

ghost added the Status: Code Review Needed Ready for review and testing label Mar 16, 2021

anodos325 force-pushed the fix-mode-007 branch 2 times, most recently from 10184fe to 7a4d594 Compare March 16, 2021 20:16

anodos325 force-pushed the fix-mode-007 branch from 7a4d594 to 1553eb7 Compare March 17, 2021 11:53

anodos325 force-pushed the fix-mode-007 branch from 1553eb7 to 93196e8 Compare March 17, 2021 14:27

ghost force-pushed the fix-mode-007 branch from 93196e8 to a89fbeb Compare March 19, 2021 16:26

ghost force-pushed the fix-mode-007 branch from a89fbeb to c9cf0bf Compare March 19, 2021 16:32

behlendorf approved these changes Mar 20, 2021

View reviewed changes

behlendorf added Status: Accepted Ready to integrate (reviewed, tested) and removed Status: Code Review Needed Ready for review and testing labels Mar 20, 2021

behlendorf merged commit 66e6d3f into openzfs:master Mar 20, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix regression in POSIX mode behavior #11760

Fix regression in POSIX mode behavior #11760

anodos325 commented Mar 16, 2021 •

edited by ghost

Loading

behlendorf commented Mar 16, 2021

anodos325 commented Mar 16, 2021

anodos325 commented Mar 17, 2021 •

edited

Loading

anodos325 commented Mar 17, 2021 •

edited

Loading

adamdmoss commented Mar 17, 2021

anodos325 commented Mar 18, 2021

adamdmoss commented Mar 18, 2021

ghost commented Mar 19, 2021 •

edited by ghost

Loading

ghost commented Mar 19, 2021

Fix regression in POSIX mode behavior #11760

Fix regression in POSIX mode behavior #11760

Conversation

anodos325 commented Mar 16, 2021 • edited by ghost Loading

Motivation and Context

Description

How Has This Been Tested?

Types of changes

Checklist:

behlendorf commented Mar 16, 2021

anodos325 commented Mar 16, 2021

anodos325 commented Mar 17, 2021 • edited Loading

anodos325 commented Mar 17, 2021 • edited Loading

adamdmoss commented Mar 17, 2021

anodos325 commented Mar 18, 2021

adamdmoss commented Mar 18, 2021

ghost commented Mar 19, 2021 • edited by ghost Loading

ghost commented Mar 19, 2021

anodos325 commented Mar 16, 2021 •

edited by ghost

Loading

anodos325 commented Mar 17, 2021 •

edited

Loading

anodos325 commented Mar 17, 2021 •

edited

Loading

ghost commented Mar 19, 2021 •

edited by ghost

Loading