linux-kernel - Re: [PATCH 2/2] openat2: add OA2_INHERIT

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

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <81ab6c6a-0a9e-4f2f-b455-7585283acf53@samba.org>
Date: Mon, 22 Apr 2024 21:53:14 +0200
From: Stefan Metzmacher <metze@...ba.org>
To: Stas Sergeev <stsp2@...dex.ru>, linux-kernel@...r.kernel.org
Cc: Eric Biederman <ebiederm@...ssion.com>,
 Alexander Viro <viro@...iv.linux.org.uk>, Andy Lutomirski <luto@...nel.org>,
 Christian Brauner <brauner@...nel.org>, Jan Kara <jack@...e.cz>,
 Jeff Layton <jlayton@...nel.org>, Chuck Lever <chuck.lever@...cle.com>,
 Alexander Aring <alex.aring@...il.com>, linux-fsdevel@...r.kernel.org,
 Paolo Bonzini <pbonzini@...hat.com>,
 Christian Göttsche <cgzones@...glemail.com>,
 Jens Axboe <axboe@...nel.dk>
Subject: Re: [PATCH 2/2] openat2: add OA2_INHERIT_CRED flag

Am 22.04.24 um 10:45 schrieb Stas Sergeev:
> This flag performs the open operation with the fsuid/fsgid that
> were in effect when dir_fd was opened.
> This allows the process to pre-open some directories and then
> change eUID (and all other UIDs/GIDs) to a less-privileged user,
> retaining the ability to open/create files within these directories.
> 
> Design goal:
> The idea is to provide a very light-weight sandboxing, where the
> process, without the use of any heavy-weight techniques like chroot
> within namespaces, can restrict the access to the set of pre-opened
> directories.
> This patch is just a first step to such sandboxing. If things go
> well, in the future the same extension can be added to more syscalls.
> These should include at least unlinkat(), renameat2() and the
> not-yet-upstreamed setxattrat().
> 
> Security considerations:
> To avoid sandboxing escape, this patch makes sure the restricted
> lookup modes are used. Namely, RESOLVE_BENEATH or RESOLVE_IN_ROOT.
> To avoid leaking creds across exec, this patch requires O_CLOEXEC
> flag on a directory.
> 
> Use cases:
> Virtual machines that deal with untrusted code, can use that
> instead of a more heavy-weighted approaches.
> Currently the approach is being tested on a dosemu2 VM.
> 
> Signed-off-by: Stas Sergeev <stsp2@...dex.ru>
> 
> CC: Eric Biederman <ebiederm@...ssion.com>
> CC: Alexander Viro <viro@...iv.linux.org.uk>
> CC: Andy Lutomirski <luto@...nel.org>
> CC: Christian Brauner <brauner@...nel.org>
> CC: Jan Kara <jack@...e.cz>
> CC: Jeff Layton <jlayton@...nel.org>
> CC: Chuck Lever <chuck.lever@...cle.com>
> CC: Alexander Aring <alex.aring@...il.com>
> CC: linux-fsdevel@...r.kernel.org
> CC: linux-kernel@...r.kernel.org
> CC: Paolo Bonzini <pbonzini@...hat.com>
> CC: Christian Göttsche <cgzones@...glemail.com>
> ---
>   fs/file_table.c              |  2 ++
>   fs/internal.h                |  2 +-
>   fs/namei.c                   | 54 ++++++++++++++++++++++++++++++++++--
>   fs/open.c                    |  2 +-
>   include/linux/fcntl.h        |  2 ++
>   include/linux/fs.h           |  2 ++
>   include/uapi/linux/openat2.h |  3 ++
>   7 files changed, 63 insertions(+), 4 deletions(-)
> 
> diff --git a/fs/file_table.c b/fs/file_table.c
> index 4f03beed4737..9991bdd538e9 100644
> --- a/fs/file_table.c
> +++ b/fs/file_table.c
> @@ -160,6 +160,8 @@ static int init_file(struct file *f, int flags, const struct cred *cred)
>   	mutex_init(&f->f_pos_lock);
>   	f->f_flags = flags;
>   	f->f_mode = OPEN_FMODE(flags);
> +	f->f_fsuid = cred->fsuid;
> +	f->f_fsgid = cred->fsgid;
>   	/* f->f_version: 0 */
>   
>   	/*
> diff --git a/fs/internal.h b/fs/internal.h
> index 7ca738904e34..692b53b19aad 100644
> --- a/fs/internal.h
> +++ b/fs/internal.h
> @@ -169,7 +169,7 @@ static inline void sb_end_ro_state_change(struct super_block *sb)
>    * open.c
>    */
>   struct open_flags {
> -	int open_flag;
> +	u64 open_flag;
>   	umode_t mode;
>   	int acc_mode;
>   	int intent;
> diff --git a/fs/namei.c b/fs/namei.c
> index 2fde2c320ae9..d1db6ceee4bd 100644
> --- a/fs/namei.c
> +++ b/fs/namei.c
> @@ -586,6 +586,8 @@ struct nameidata {
>   	int		dfd;
>   	vfsuid_t	dir_vfsuid;
>   	umode_t		dir_mode;
> +	kuid_t		dir_open_fsuid;
> +	kgid_t		dir_open_fsgid;
>   } __randomize_layout;
>   
>   #define ND_ROOT_PRESET 1
> @@ -2414,6 +2416,8 @@ static const char *path_init(struct nameidata *nd, unsigned flags)
>   			get_fs_pwd(current->fs, &nd->path);
>   			nd->inode = nd->path.dentry->d_inode;
>   		}
> +		nd->dir_open_fsuid = current_cred()->fsuid;
> +		nd->dir_open_fsgid = current_cred()->fsgid;

I'm wondering if it would be better to capture the whole cred structure.

Similar to io_register_personality(), which uses get_current_cred().

Only using uid and gid, won't reflect any group memberships or capabilities...

metze