zfs icon indicating copy to clipboard operation
zfs copied to clipboard

fix #3963: allow `snapdir=disabled` to prevent automounts

Open Fabian-Gruenbichler opened this issue 1 year ago • 7 comments

Motivation and Context

there is currently a gap as described in #3963 that allows potentially problematic access to snapshots via the .zfs control dir.

Description

this PR fixes the issue via two avenues:

  • introducing a new property value for snapdir that allows disabling access altogether, instead of just hiding
  • introducing a new module parameter that makes automounts set nosuid on Linux, to at least close the "exploit vulnerable setuid binaries" gap while still allowing snapshot access in general

How Has This Been Tested?

tested that the new property value works as expected, including fallback on old ZFS versions. in case of a version mismatch (new kernel module, old userspace) - is displayed as value of the property. tested that the module parameter works as expected. existing mounts are not modified.

Types of changes

  • [x] Bug fix (non-breaking change which fixes an issue)
  • [x] 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)
  • [x] Documentation (a change to man pages or other documentation)

Checklist:

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

Fabian-Gruenbichler avatar Feb 14 '24 13:02 Fabian-Gruenbichler

I've had this on my wishlist for years, thank you.

allanjude avatar Feb 14 '24 14:02 allanjude

fixed an error in the test property values list, should clear up the send test failure

Fabian-Gruenbichler avatar Feb 15 '24 09:02 Fabian-Gruenbichler

minimized the changes now that the inode for .zfs is no longer returned if access is disabled. freebsd still missing ;)

Fabian-Gruenbichler avatar Feb 15 '24 13:02 Fabian-Gruenbichler

basic freebsd support for snapdir=disabled now added (as standalone commit). AFAICT, freebsd already disables suid for the automounts unconditionally, so that part is not needed on freebsd at all.

I hope this is ready for review now :)

Fabian-Gruenbichler avatar Mar 06 '24 09:03 Fabian-Gruenbichler

I think we should break the nosuid part out into its own PR, and it will need an implementation for FreeBSD as well (I am someone else from FreeBSD can help with that)

allanjude avatar Jun 21 '24 00:06 allanjude

it is its own commit already, so splitting it out should be easily done.

AFAIU, the BSD code already doesn't mount snapshots with setuid enabled no matter what? see https://github.com/openzfs/zfs/blob/master/module/os/freebsd/spl/spl_vfs.c#L187 (which is called by the ctl dir code)

Fabian-Gruenbichler avatar Jun 21 '24 06:06 Fabian-Gruenbichler

Unless access becomes restricted to privileged users (root) only, I would propose to make snapdir=disabled the (safe) default, because I see the current behavior as a security vulnerability (as pointed out in #3963 in this post, for example).

JanBeh avatar Jun 21 '24 08:06 JanBeh