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

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

Closed
wants to merge 3 commits into from
Closed

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

wants to merge 3 commits into from

Conversation

Fabian-Gruenbichler
Copy link
Contributor

@Fabian-Gruenbichler Fabian-Gruenbichler commented Feb 14, 2024 •
edited
Loading

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

  • 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:

@Fabian-Gruenbichler Fabian-Gruenbichler marked this pull request as draft February 14, 2024 14:08
@allanjude
Copy link
Contributor

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

@Fabian-Gruenbichler
Copy link
Contributor Author

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

@Fabian-Gruenbichler
Copy link
Contributor Author

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

@behlendorf behlendorf added the Status: Code Review Needed Ready for review and testing label Feb 15, 2024
@Fabian-Gruenbichler
snapdir: add 'disabled' value to make .zfs inaccessible
ab9c8a4 
in some environments, just making the .zfs control dir hidden from sight
might not be enough. in particular, the following scenarios might
warrant not allowing access at all:
- old snapshots with wrong permissions/ownership
- old snapshots with exploitable setuid/setgid binaries
- old snapshots with sensitive contents

introducing a new 'disabled' value that not only hides the control dir,
but prevents access to its contents by returning ENOENT solves all of
the above.

the new property value takes advantage of 'iuv' semantics ("ignore
unknown value") to automatically fall back to the old default value when
a pool is accessed by an older version of ZFS that doesn't yet know
about 'disabled' semantics.

I think that technically the zfs_dirlook change is enough to prevent
access, but preventing lookups and dir entries in an already opened .zfs
handle might also be a good idea to prevent races when modifying the
property at runtime.

Fixes: openzfs#3963

Signed-off-by: Fabian Grünbichler <f.gruenbichler@proxmox.com>

.zfs: don't return .zfs inode if disabled

Signed-off-by: Fabian Grünbichler <f.gruenbichler@proxmox.com>
@Fabian-Gruenbichler
freebsd: implement snapdir=disabled
548e418 
there might be room for improvement w.r.t. runtime changes of the snapdir value
after something obtained a handle/descriptor, but this at least implements the
basic support.

Signed-off-by: Fabian Grünbichler <f.gruenbichler@proxmox.com>
@Fabian-Gruenbichler
module: add zfs_snapshot_no_setuid parameter
b5ba501 
to control wheter automatically mounted snapshots have the setuid mount
option set or not.

this could be considered a partial fix for one of the scenarios
mentioned in desired.

Signed-off-by: Fabian Grünbichler <f.gruenbichler@proxmox.com>
@Fabian-Gruenbichler
Copy link
Contributor Author

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 Fabian-Gruenbichler marked this pull request as ready for review March 6, 2024 09:25
@Fabian-Gruenbichler Fabian-Gruenbichler changed the title WIP: fix #3963: allow snapdir=disabled to prevent automounts fix #3963: allow snapdir=disabled to prevent automounts Mar 6, 2024
@allanjude
Copy link
Contributor

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)

@Fabian-Gruenbichler
Copy link
Contributor Author

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)

@JanBeh
Copy link

JanBeh commented Jun 21, 2024

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).

behlendorf
behlendorf approved these changes Sep 25, 2024
View reviewed changes
Copy link
Contributor

@behlendorf behlendorf left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@Fabian-Gruenbichler thanks for putting this together. Looks good to me, if we can rebase this I think we can move forward with merging it.

@behlendorf behlendorf added Status: Accepted Ready to integrate (reviewed, tested) and removed Status: Code Review Needed Ready for review and testing labels Sep 25, 2024
@behlendorf behlendorf mentioned this pull request Sep 30, 2024
Merged
13 tasks
@behlendorf
Copy link
Contributor

Replaced by #16587. @Fabian-Gruenbichler I hope you don't mind but I've rebased this and squashed your commits. If you could take a look that would be great.

@behlendorf behlendorf closed this Sep 30, 2024
@Fabian-Gruenbichler
Copy link
Contributor Author

thanks and sorry for the radio silence, I've been out of action for the past ~2 weeks cause of an injury! the rebased version LGTM, FWIW.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@amotin amotin amotin approved these changes

@behlendorf behlendorf behlendorf approved these changes

Assignees
No one assigned
Labels
Status: Accepted Ready to integrate (reviewed, tested)
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@Fabian-Gruenbichler @allanjude @JanBeh @behlendorf @amotin