From d34d4f97a81f6895de3da67ffbad6f986b2cdae6 Mon Sep 17 00:00:00 2001 From: Brian Behlendorf Date: Wed, 2 Oct 2024 09:12:02 -0700 Subject: [PATCH] snapdir: add 'disabled' value to make .zfs inaccessible MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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. Add zfs_snapshot_no_setuid parameter to control whether 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. Reviewed-by: Alexander Motin Reviewed-by: Brian Behlendorf Reviewed-by: Tino Reichardt Signed-off-by: Fabian Grünbichler Co-authored-by: Fabian Grünbichler Closes #3963 Closes #16587 --- include/os/freebsd/zfs/sys/zfs_ctldir.h | 2 +- include/os/freebsd/zfs/sys/zfs_vfsops_os.h | 2 +- include/os/linux/zfs/sys/zfs_ctldir.h | 2 +- include/os/linux/zfs/sys/zfs_vfsops_os.h | 2 +- include/sys/zfs_ioctl.h | 1 + man/man4/zfs.4 | 9 +++++++++ man/man7/zfsconcepts.7 | 2 +- man/man7/zfsprops.7 | 6 +++--- module/os/freebsd/zfs/zfs_vnops_os.c | 2 ++ module/os/linux/zfs/zfs_ctldir.c | 22 +++++++++++++++++----- module/os/linux/zfs/zfs_dir.c | 3 +++ module/os/linux/zfs/zfs_vfsops.c | 5 +++++ module/os/linux/zfs/zpl_ctldir.c | 4 ++++ module/zcommon/zfs_prop.c | 3 ++- module/zfs/dsl_prop.c | 4 ++++ tests/zfs-tests/include/properties.shlib | 2 +- 16 files changed, 56 insertions(+), 15 deletions(-) diff --git a/include/os/freebsd/zfs/sys/zfs_ctldir.h b/include/os/freebsd/zfs/sys/zfs_ctldir.h index 14d75df33df..f0a98a7def8 100644 --- a/include/os/freebsd/zfs/sys/zfs_ctldir.h +++ b/include/os/freebsd/zfs/sys/zfs_ctldir.h @@ -40,7 +40,7 @@ extern "C" { ((zdp)->z_zfsvfs->z_ctldir != NULL)) #define zfs_show_ctldir(zdp) \ (zfs_has_ctldir(zdp) && \ - ((zdp)->z_zfsvfs->z_show_ctldir)) + ((zdp)->z_zfsvfs->z_show_ctldir == ZFS_SNAPDIR_VISIBLE)) void zfsctl_create(zfsvfs_t *); void zfsctl_destroy(zfsvfs_t *); diff --git a/include/os/freebsd/zfs/sys/zfs_vfsops_os.h b/include/os/freebsd/zfs/sys/zfs_vfsops_os.h index 9fbca35cde8..b7cbdc736d2 100644 --- a/include/os/freebsd/zfs/sys/zfs_vfsops_os.h +++ b/include/os/freebsd/zfs/sys/zfs_vfsops_os.h @@ -76,7 +76,7 @@ struct zfsvfs { list_t z_all_znodes; /* all vnodes in the fs */ kmutex_t z_znodes_lock; /* lock for z_all_znodes */ struct zfsctl_root *z_ctldir; /* .zfs directory pointer */ - boolean_t z_show_ctldir; /* expose .zfs in the root dir */ + uint_t z_show_ctldir; /* how to expose .zfs in the root dir */ boolean_t z_issnap; /* true if this is a snapshot */ boolean_t z_use_fuids; /* version allows fuids */ boolean_t z_replay; /* set during ZIL replay */ diff --git a/include/os/linux/zfs/sys/zfs_ctldir.h b/include/os/linux/zfs/sys/zfs_ctldir.h index ad16ab5e444..8f18cda2952 100644 --- a/include/os/linux/zfs/sys/zfs_ctldir.h +++ b/include/os/linux/zfs/sys/zfs_ctldir.h @@ -45,7 +45,7 @@ (ZTOZSB(zdp)->z_ctldir != NULL)) #define zfs_show_ctldir(zdp) \ (zfs_has_ctldir(zdp) && \ - (ZTOZSB(zdp)->z_show_ctldir)) + (ZTOZSB(zdp)->z_show_ctldir == ZFS_SNAPDIR_VISIBLE)) extern int zfs_expire_snapshot; diff --git a/include/os/linux/zfs/sys/zfs_vfsops_os.h b/include/os/linux/zfs/sys/zfs_vfsops_os.h index e742e8dc392..7067eb17900 100644 --- a/include/os/linux/zfs/sys/zfs_vfsops_os.h +++ b/include/os/linux/zfs/sys/zfs_vfsops_os.h @@ -110,7 +110,7 @@ struct zfsvfs { kmutex_t z_znodes_lock; /* lock for z_all_znodes */ arc_prune_t *z_arc_prune; /* called by ARC to prune caches */ struct inode *z_ctldir; /* .zfs directory inode */ - boolean_t z_show_ctldir; /* expose .zfs in the root dir */ + uint_t z_show_ctldir; /* how to expose .zfs in the root dir */ boolean_t z_issnap; /* true if this is a snapshot */ boolean_t z_use_fuids; /* version allows fuids */ boolean_t z_replay; /* set during ZIL replay */ diff --git a/include/sys/zfs_ioctl.h b/include/sys/zfs_ioctl.h index 9e3d8150f50..470b2ed5f7c 100644 --- a/include/sys/zfs_ioctl.h +++ b/include/sys/zfs_ioctl.h @@ -57,6 +57,7 @@ extern "C" { */ #define ZFS_SNAPDIR_HIDDEN 0 #define ZFS_SNAPDIR_VISIBLE 1 +#define ZFS_SNAPDIR_DISABLED 2 /* * Property values for snapdev diff --git a/man/man4/zfs.4 b/man/man4/zfs.4 index 6840dc3171b..5a47cbbe22c 100644 --- a/man/man4/zfs.4 +++ b/man/man4/zfs.4 @@ -1570,6 +1570,15 @@ which have the .Em no_root_squash option set. . +.It Sy zfs_snapshot_no_setuid Ns = Ns Sy 0 Ns | Ns 1 Pq int +Whether to disable +.Em setuid/setgid +support for snapshot mounts triggered by access to the +.Sy .zfs/snapshot +directory by setting the +.Em nosuid +mount option. +. .It Sy zfs_flags Ns = Ns Sy 0 Pq int Set additional debugging flags. The following flags may be bitwise-ored together: diff --git a/man/man7/zfsconcepts.7 b/man/man7/zfsconcepts.7 index 1be3d961c3d..1d2dff7e486 100644 --- a/man/man7/zfsconcepts.7 +++ b/man/man7/zfsconcepts.7 @@ -71,7 +71,7 @@ File system snapshots can be accessed under the directory in the root of the file system. Snapshots are automatically mounted on demand and may be unmounted at regular intervals. -The visibility of the +The availability and visibility of the .Pa .zfs directory can be controlled by the .Sy snapdir diff --git a/man/man7/zfsprops.7 b/man/man7/zfsprops.7 index 4ea91bb9008..06e2797968e 100644 --- a/man/man7/zfsprops.7 +++ b/man/man7/zfsprops.7 @@ -1848,11 +1848,11 @@ Controls whether the volume snapshot devices under are hidden or visible. The default value is .Sy hidden . -.It Sy snapdir Ns = Ns Sy hidden Ns | Ns Sy visible +.It Sy snapdir Ns = Ns Sy disabled Ns | Ns Sy hidden Ns | Ns Sy visible Controls whether the .Pa .zfs -directory is hidden or visible in the root of the file system as discussed in -the +directory is disabled, hidden or visible in the root of the file system as +discussed in the .Sx Snapshots section of .Xr zfsconcepts 7 . diff --git a/module/os/freebsd/zfs/zfs_vnops_os.c b/module/os/freebsd/zfs/zfs_vnops_os.c index 60deab1f5ce..a2222a89938 100644 --- a/module/os/freebsd/zfs/zfs_vnops_os.c +++ b/module/os/freebsd/zfs/zfs_vnops_os.c @@ -774,6 +774,8 @@ zfs_lookup(vnode_t *dvp, const char *nm, vnode_t **vpp, } if (zfs_has_ctldir(zdp) && strcmp(nm, ZFS_CTLDIR_NAME) == 0) { zfs_exit(zfsvfs, FTAG); + if (zfsvfs->z_show_ctldir == ZFS_SNAPDIR_DISABLED) + return (SET_ERROR(ENOENT)); if ((cnp->cn_flags & ISLASTCN) != 0 && nameiop != LOOKUP) return (SET_ERROR(ENOTSUP)); error = zfsctl_root(zfsvfs, cnp->cn_lkflags, vpp); diff --git a/module/os/linux/zfs/zfs_ctldir.c b/module/os/linux/zfs/zfs_ctldir.c index a8b25b2bd8a..8a42a075cd2 100644 --- a/module/os/linux/zfs/zfs_ctldir.c +++ b/module/os/linux/zfs/zfs_ctldir.c @@ -111,6 +111,7 @@ static krwlock_t zfs_snapshot_lock; */ int zfs_expire_snapshot = ZFSCTL_EXPIRE_SNAPSHOT; static int zfs_admin_snapshot = 0; +static int zfs_snapshot_no_setuid = 0; typedef struct { char *se_name; /* full snapshot name */ @@ -807,7 +808,9 @@ zfsctl_root_lookup(struct inode *dip, const char *name, struct inode **ipp, if ((error = zfs_enter(zfsvfs, FTAG)) != 0) return (error); - if (strcmp(name, "..") == 0) { + if (zfsvfs->z_show_ctldir == ZFS_SNAPDIR_DISABLED) { + *ipp = NULL; + } else if (strcmp(name, "..") == 0) { *ipp = dip->i_sb->s_root->d_inode; } else if (strcmp(name, ZFS_SNAPDIR_NAME) == 0) { *ipp = zfsctl_inode_lookup(zfsvfs, ZFSCTL_INO_SNAPDIR, @@ -1097,9 +1100,9 @@ zfsctl_snapshot_mount(struct path *path, int flags) zfsvfs_t *zfsvfs; zfsvfs_t *snap_zfsvfs; zfs_snapentry_t *se; - char *full_name, *full_path; + char *full_name, *full_path, *options; char *argv[] = { "/usr/bin/env", "mount", "-i", "-t", "zfs", "-n", - NULL, NULL, NULL }; + "-o", NULL, NULL, NULL, NULL }; char *envp[] = { NULL }; int error; struct path spath; @@ -1113,6 +1116,7 @@ zfsctl_snapshot_mount(struct path *path, int flags) full_name = kmem_zalloc(ZFS_MAX_DATASET_NAME_LEN, KM_SLEEP); full_path = kmem_zalloc(MAXPATHLEN, KM_SLEEP); + options = kmem_zalloc(7, KM_SLEEP); error = zfsctl_snapshot_name(zfsvfs, dname(dentry), ZFS_MAX_DATASET_NAME_LEN, full_name); @@ -1128,6 +1132,9 @@ zfsctl_snapshot_mount(struct path *path, int flags) zfsvfs->z_vfs->vfs_mntpoint ? zfsvfs->z_vfs->vfs_mntpoint : "", dname(dentry)); + snprintf(options, 7, "%s", + zfs_snapshot_no_setuid ? "nosuid" : "suid"); + /* * Multiple concurrent automounts of a snapshot are never allowed. * The snapshot may be manually mounted as many times as desired. @@ -1150,8 +1157,9 @@ zfsctl_snapshot_mount(struct path *path, int flags) * value from call_usermodehelper() will be (exitcode << 8 + signal). */ dprintf("mount; name=%s path=%s\n", full_name, full_path); - argv[6] = full_name; - argv[7] = full_path; + argv[7] = options; + argv[8] = full_name; + argv[9] = full_path; error = call_usermodehelper(argv[0], argv, envp, UMH_WAIT_PROC); if (error) { if (!(error & MOUNT_BUSY << 8)) { @@ -1312,3 +1320,7 @@ MODULE_PARM_DESC(zfs_admin_snapshot, "Enable mkdir/rmdir/mv in .zfs/snapshot"); module_param(zfs_expire_snapshot, int, 0644); MODULE_PARM_DESC(zfs_expire_snapshot, "Seconds to expire .zfs/snapshot"); + +module_param(zfs_snapshot_no_setuid, int, 0644); +MODULE_PARM_DESC(zfs_snapshot_no_setuid, + "Disable setuid/setgid for automounts in .zfs/snapshot"); diff --git a/module/os/linux/zfs/zfs_dir.c b/module/os/linux/zfs/zfs_dir.c index f59281f06ca..564e89b37d1 100644 --- a/module/os/linux/zfs/zfs_dir.c +++ b/module/os/linux/zfs/zfs_dir.c @@ -415,6 +415,9 @@ zfs_dirlook(znode_t *dzp, char *name, znode_t **zpp, int flags, *zpp = zp; rw_exit(&dzp->z_parent_lock); } else if (zfs_has_ctldir(dzp) && strcmp(name, ZFS_CTLDIR_NAME) == 0) { + if (ZTOZSB(dzp)->z_show_ctldir == ZFS_SNAPDIR_DISABLED) { + return (SET_ERROR(ENOENT)); + } ip = zfsctl_root(dzp); *zpp = ITOZ(ip); } else { diff --git a/module/os/linux/zfs/zfs_vfsops.c b/module/os/linux/zfs/zfs_vfsops.c index a24f504129d..de3e8c89cfd 100644 --- a/module/os/linux/zfs/zfs_vfsops.c +++ b/module/os/linux/zfs/zfs_vfsops.c @@ -1702,6 +1702,11 @@ zfs_vget(struct super_block *sb, struct inode **ipp, fid_t *fidp) (object == ZFSCTL_INO_ROOT || object == ZFSCTL_INO_SNAPDIR)) { *ipp = zfsvfs->z_ctldir; ASSERT(*ipp != NULL); + + if (zfsvfs->z_show_ctldir == ZFS_SNAPDIR_DISABLED) { + return (SET_ERROR(ENOENT)); + } + if (object == ZFSCTL_INO_SNAPDIR) { VERIFY(zfsctl_root_lookup(*ipp, "snapshot", ipp, 0, kcred, NULL, NULL) == 0); diff --git a/module/os/linux/zfs/zpl_ctldir.c b/module/os/linux/zfs/zpl_ctldir.c index 56a30be5110..fe64bc71038 100644 --- a/module/os/linux/zfs/zpl_ctldir.c +++ b/module/os/linux/zfs/zpl_ctldir.c @@ -57,6 +57,10 @@ zpl_root_iterate(struct file *filp, struct dir_context *ctx) zfsvfs_t *zfsvfs = ITOZSB(file_inode(filp)); int error = 0; + if (zfsvfs->z_show_ctldir == ZFS_SNAPDIR_DISABLED) { + return (SET_ERROR(ENOENT)); + } + if ((error = zpl_enter(zfsvfs, FTAG)) != 0) return (error); diff --git a/module/zcommon/zfs_prop.c b/module/zcommon/zfs_prop.c index f7e6b41bf01..40254c8d956 100644 --- a/module/zcommon/zfs_prop.c +++ b/module/zcommon/zfs_prop.c @@ -238,6 +238,7 @@ zfs_prop_init(void) static const zprop_index_t snapdir_table[] = { { "hidden", ZFS_SNAPDIR_HIDDEN }, { "visible", ZFS_SNAPDIR_VISIBLE }, + { "disabled", ZFS_SNAPDIR_DISABLED }, { NULL } }; @@ -436,7 +437,7 @@ zfs_prop_init(void) "COMPRESS", compress_table, sfeatures); zprop_register_index(ZFS_PROP_SNAPDIR, "snapdir", ZFS_SNAPDIR_HIDDEN, PROP_INHERIT, ZFS_TYPE_FILESYSTEM, - "hidden | visible", "SNAPDIR", snapdir_table, sfeatures); + "disabled | hidden | visible", "SNAPDIR", snapdir_table, sfeatures); zprop_register_index(ZFS_PROP_SNAPDEV, "snapdev", ZFS_SNAPDEV_HIDDEN, PROP_INHERIT, ZFS_TYPE_FILESYSTEM | ZFS_TYPE_VOLUME, "hidden | visible", "SNAPDEV", snapdev_table, sfeatures); diff --git a/module/zfs/dsl_prop.c b/module/zfs/dsl_prop.c index 3b8683593ff..1a0e83419e7 100644 --- a/module/zfs/dsl_prop.c +++ b/module/zfs/dsl_prop.c @@ -698,6 +698,10 @@ dsl_prop_set_iuv(objset_t *mos, uint64_t zapobj, const char *propname, *(uint64_t *)value == ZFS_REDUNDANT_METADATA_NONE) iuv = B_TRUE; break; + case ZFS_PROP_SNAPDIR: + if (*(uint64_t *)value == ZFS_SNAPDIR_DISABLED) + iuv = B_TRUE; + break; default: break; } diff --git a/tests/zfs-tests/include/properties.shlib b/tests/zfs-tests/include/properties.shlib index 3dfb295a40d..5a39eb3f36f 100644 --- a/tests/zfs-tests/include/properties.shlib +++ b/tests/zfs-tests/include/properties.shlib @@ -30,7 +30,7 @@ typeset -a logbias_prop_vals=('latency' 'throughput') typeset -a primarycache_prop_vals=('all' 'none' 'metadata') typeset -a redundant_metadata_prop_vals=('all' 'most' 'some' 'none') typeset -a secondarycache_prop_vals=('all' 'none' 'metadata') -typeset -a snapdir_prop_vals=('hidden' 'visible') +typeset -a snapdir_prop_vals=('disabled' 'hidden' 'visible') typeset -a sync_prop_vals=('standard' 'always' 'disabled') typeset -a fs_props=('compress' 'checksum' 'recsize'