| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <20220503194943.6bcmsxjvinfjrqxa@quack3.lan>
Date: Tue, 3 May 2022 21:49:43 +0200
From: Jan Kara <jack@...e.cz>
To: Guowei Du <duguoweisz@...il.com>
Cc: jack@...e.cz, amir73il@...il.com, linux-fsdevel@...r.kernel.org,
linux-kernel@...r.kernel.org, viro@...iv.linux.org.uk,
jmorris@...ei.org, serge@...lyn.com, ast@...nel.org,
daniel@...earbox.net, andrii@...nel.org, kafai@...com,
songliubraving@...com, yhs@...com, john.fastabend@...il.com,
kpsingh@...nel.org, linux-security-module@...r.kernel.org,
netdev@...r.kernel.org, bpf@...r.kernel.org, paul@...l-moore.com,
stephen.smalley.work@...il.com, eparis@...isplace.org,
keescook@...omium.org, anton@...msg.org, ccross@...roid.com,
tony.luck@...el.com, selinux@...r.kernel.org,
duguowei <duguowei@...omi.com>
Subject: Re: [PATCH] fsnotify: add generic perm check for unlink/rmdir
On Wed 04-05-22 02:37:50, Guowei Du wrote:
> From: duguowei <duguowei@...omi.com>
>
> For now, there have been open/access/open_exec perms for file operation,
> so we add new perms check with unlink/rmdir syscall. if one app deletes
> any file/dir within pubic area, fsnotify can sends fsnotify_event to
> listener to deny that, even if the app have right dac/mac permissions.
>
> Signed-off-by: duguowei <duguowei@...omi.com>
Before we go into technical details of implementation can you tell me more
details about the usecase? Why do you need to check specifically for unlink
/ delete?
Also on the design side of things: Do you realize these permission events
will not be usable together with other permission events like
FAN_OPEN_PERM? Because these require notification group returning file
descriptors while your events will return file handles... I guess we should
somehow fix that.
Honza
> ---
> fs/notify/fsnotify.c | 2 +-
> include/linux/fs.h | 2 ++
> include/linux/fsnotify.h | 16 ++++++++++++++++
> include/linux/fsnotify_backend.h | 6 +++++-
> security/security.c | 12 ++++++++++--
> security/selinux/hooks.c | 4 ++++
> 6 files changed, 38 insertions(+), 4 deletions(-)
>
> diff --git a/fs/notify/fsnotify.c b/fs/notify/fsnotify.c
> index 70a8516b78bc..9c03a5f84be0 100644
> --- a/fs/notify/fsnotify.c
> +++ b/fs/notify/fsnotify.c
> @@ -581,7 +581,7 @@ static __init int fsnotify_init(void)
> {
> int ret;
>
> - BUILD_BUG_ON(HWEIGHT32(ALL_FSNOTIFY_BITS) != 25);
> + BUILD_BUG_ON(HWEIGHT32(ALL_FSNOTIFY_BITS) != 27);
>
> ret = init_srcu_struct(&fsnotify_mark_srcu);
> if (ret)
> diff --git a/include/linux/fs.h b/include/linux/fs.h
> index bbde95387a23..9c661584db7d 100644
> --- a/include/linux/fs.h
> +++ b/include/linux/fs.h
> @@ -100,6 +100,8 @@ typedef int (dio_iodone_t)(struct kiocb *iocb, loff_t offset,
> #define MAY_CHDIR 0x00000040
> /* called from RCU mode, don't block */
> #define MAY_NOT_BLOCK 0x00000080
> +#define MAY_UNLINK 0x00000100
> +#define MAY_RMDIR 0x00000200
>
> /*
> * flags in file.f_mode. Note that FMODE_READ and FMODE_WRITE must correspond
> diff --git a/include/linux/fsnotify.h b/include/linux/fsnotify.h
> index bb8467cd11ae..68f5d4aaf1ae 100644
> --- a/include/linux/fsnotify.h
> +++ b/include/linux/fsnotify.h
> @@ -80,6 +80,22 @@ static inline int fsnotify_parent(struct dentry *dentry, __u32 mask,
> return fsnotify(mask, data, data_type, NULL, NULL, inode, 0);
> }
>
> +static inline int fsnotify_path_perm(struct path *path, struct dentry *dentry, __u32 mask)
> +{
> + __u32 fsnotify_mask = 0;
> +
> + if (!(mask & (MAY_UNLINK | MAY_RMDIR)))
> + return 0;
> +
> + if (mask & MAY_UNLINK)
> + fsnotify_mask |= FS_UNLINK_PERM;
> +
> + if (mask & MAY_RMDIR)
> + fsnotify_mask |= FS_RMDIR_PERM;
> +
> + return fsnotify_parent(dentry, fsnotify_mask, path, FSNOTIFY_EVENT_PATH);
> +}
> +
> /*
> * Simple wrappers to consolidate calls to fsnotify_parent() when an event
> * is on a file/dentry.
> diff --git a/include/linux/fsnotify_backend.h b/include/linux/fsnotify_backend.h
> index 0805b74cae44..0e2e240e8234 100644
> --- a/include/linux/fsnotify_backend.h
> +++ b/include/linux/fsnotify_backend.h
> @@ -54,6 +54,8 @@
> #define FS_OPEN_PERM 0x00010000 /* open event in an permission hook */
> #define FS_ACCESS_PERM 0x00020000 /* access event in a permissions hook */
> #define FS_OPEN_EXEC_PERM 0x00040000 /* open/exec event in a permission hook */
> +#define FS_UNLINK_PERM 0x00080000 /* unlink event in a permission hook */
> +#define FS_RMDIR_PERM 0x00100000 /* rmdir event in a permission hook */
>
> #define FS_EXCL_UNLINK 0x04000000 /* do not send events if object is unlinked */
> /*
> @@ -79,7 +81,9 @@
> #define ALL_FSNOTIFY_DIRENT_EVENTS (FS_CREATE | FS_DELETE | FS_MOVE | FS_RENAME)
>
> #define ALL_FSNOTIFY_PERM_EVENTS (FS_OPEN_PERM | FS_ACCESS_PERM | \
> - FS_OPEN_EXEC_PERM)
> + FS_OPEN_EXEC_PERM | \
> + FS_UNLINK_PERM | \
> + FS_RMDIR_PERM)
>
> /*
> * This is a list of all events that may get sent to a parent that is watching
> diff --git a/security/security.c b/security/security.c
> index b7cf5cbfdc67..8efc00ec02ed 100644
> --- a/security/security.c
> +++ b/security/security.c
> @@ -1160,16 +1160,24 @@ EXPORT_SYMBOL(security_path_mkdir);
>
> int security_path_rmdir(const struct path *dir, struct dentry *dentry)
> {
> + int ret;
> if (unlikely(IS_PRIVATE(d_backing_inode(dir->dentry))))
> return 0;
> - return call_int_hook(path_rmdir, 0, dir, dentry);
> + ret = call_int_hook(path_rmdir, 0, dir, dentry);
> + if (ret)
> + return ret;
> + return fsnotify_path_perm(dir, dentry, MAY_RMDIR);
> }
>
> int security_path_unlink(const struct path *dir, struct dentry *dentry)
> {
> + int ret;
> if (unlikely(IS_PRIVATE(d_backing_inode(dir->dentry))))
> return 0;
> - return call_int_hook(path_unlink, 0, dir, dentry);
> + ret = call_int_hook(path_unlink, 0, dir, dentry);
> + if (ret)
> + return ret;
> + return fsnotify_path_perm(dir, dentry, MAY_UNLINK);
> }
> EXPORT_SYMBOL(security_path_unlink);
>
> diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c
> index e9e959343de9..f0780f0eb903 100644
> --- a/security/selinux/hooks.c
> +++ b/security/selinux/hooks.c
> @@ -1801,8 +1801,12 @@ static int may_create(struct inode *dir,
> }
>
> #define MAY_LINK 0
> +#ifndef MAY_UNLINK
> #define MAY_UNLINK 1
> +#endif
> +#ifndef MAY_RMDIR
> #define MAY_RMDIR 2
> +#endif
>
> /* Check whether a task can link, unlink, or rmdir a file/directory. */
> static int may_link(struct inode *dir,
> --
> 2.17.1
>
--
Jan Kara <jack@...e.com>
SUSE Labs, CR
Powered by blists - more mailing lists