Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

FreeBSD: handle UF_NOUNLINK chflags fflag #16820

Draft
wants to merge 1 commit into
base: master
Choose a base branch
from
Draft

FreeBSD: handle UF_NOUNLINK chflags fflag #16820

wants to merge 1 commit into from
+5 −1

Conversation

ngie-eign
Copy link

@ngie-eign ngie-eign commented Nov 29, 2024 •
edited
Loading

The UF_NOUNLINK chflags fflag is analogous to SF_NOUNLINK, but instead of requiring superuser privileges when setting SF_NOUNLINK, unprivileged users (in addition to root) can set UF_NOUNLINK on compatible paths.

This makes commands like chflags uunlink <foo> work on FreeBSD.

This closes #16809.

How Has This Been Tested?

An equivalent patch was tested on FreeBSD 15.0-CURRENT using a clone of a FreeBSD source tree fork.

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:

@ngie-eign
FreeBSD: handle UF_NOUNLINK chflags fflag
d7d19bb 
The `UF_NOUNLINK` chflags fflag is analogous to `SF_NOUNLINK`, but
instead of requiring superuser privileges when setting `SF_NOUNLINK`,
unprivileged users (in addition to root) can set `UF_NOUNLINK` on
compatible paths.

This makes commands like `chflags uunlink <foo>` work on FreeBSD.

This closes openzfs#16809.

Signed-off-by: Enji Cooper <[email protected]>
@ngie-eign
Copy link
Author

ngie-eign commented Nov 29, 2024 •
edited
Loading

This unfortunately doesn't work with UID != 0 due to other permissions checks done in zfs_freebsd_setattr(..).
The block that handles this logic is highlighted below (for future reference).

zfs/module/os/freebsd/zfs/zfs_vnops_os.c

Lines 4836 to 4853 in d7d19bb

} else {
/*
* Callers may only modify the file flags on
* objects they have VADMIN rights for.
*/
if ((error = VOP_ACCESS(vp, VADMIN, cred,
curthread)) != 0)
return (error);
if (zflags &
(ZFS_IMMUTABLE | ZFS_APPENDONLY |
ZFS_NOUNLINK)) {
return (EPERM);
}
if (fflags &
(SF_IMMUTABLE | SF_APPEND | SF_NOUNLINK)) {
return (EPERM);
}
}

The preceding block is true for root (by default on FreeBSD/Linux).
It's kind of an interesting assumption for the code to make. It's true most of the time, but not all of the time, based on my review of secpolicy_fs_owner.

@ngie-eign
Copy link
Author

UF_APPEND and UF_SPARSE are not handled properly either.
I was trying to avoid a large refactor to separate out the attributes into their system-level and user-level attributes, but I think that's the only thing that can be done here.

@amotin
Copy link
Member

amotin commented Nov 30, 2024

Correct me if I am wrong but, but it seems you are mapping SF_NOUNLINK and UF_NOUNLINK flags into the same ZFS_NOUNLINK and XAT_NOUNLINK. I am not sure even the patch proposed will work, but if somebody fix permissions issue you mentioned, unprivileged user may be able to remove SF_NOUNLINK flag.

@ngie-eign ngie-eign marked this pull request as draft December 1, 2024 00:59
@github-actions github-actions bot added the Status: Work in Progress Not yet ready for general review label Dec 1, 2024
@ngie-eign
Copy link
Author

ngie-eign commented Dec 1, 2024 •
edited
Loading

@amotin: Correct me if I am wrong but, but it seems you are mapping SF_NOUNLINK and UF_NOUNLINK flags into the same ZFS_NOUNLINK and XAT_NOUNLINK. I am not sure even the patch proposed will work, but if somebody fix permissions issue you mentioned, unprivileged user may be able to remove SF_NOUNLINK flag.

Yes -- mapping both of the constants to XAT_NOUNLINK will result in the scenario you described.

I noticed that yesterday when I was trying to figure out why unprivileged users couldn't unset UF_NOUNLINK, but root could.

I have a draft (not pushed yet), which will look at splitting up the UF and SF constants for APPENDONLY, NOUNLINK, and SPARSE, since the constants are buggy in different ways.

I moved the PR back to Draft mode for clarity; the change is not ready for merge.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
Status: Work in Progress Not yet ready for general review
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

"chflags uunlink foo" on FreeBSD fails with EOPNOSUPP ("chflags sunlink foo" works)
2 participants
@ngie-eign @amotin