Received-SPF: pass (google.com: domain of linux-kernel+bounces-153956-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:4601:e00::3 as permitted sender) client-ip=2604:1380:4601:e00::3;
Message-ID: <81ab6c6a-0a9e-4f2f-b455-7585283acf53@samba.org>
Date: Mon, 22 Apr 2024 21:53:14 +0200
Precedence: bulk
MIME-Version: 1.0
User-Agent: Mozilla Thunderbird
Subject: Re: [PATCH 2/2] openat2: add OA2_INHERIT_CRED flag
To: Stas Sergeev <stsp2@yandex.ru>, linux-kernel@vger.kernel.org
Cc: Eric Biederman <ebiederm@xmission.com>,
 Alexander Viro <viro@zeniv.linux.org.uk>, Andy Lutomirski <luto@kernel.org>,
 Christian Brauner <brauner@kernel.org>, Jan Kara <jack@suse.cz>,
 Jeff Layton <jlayton@kernel.org>, Chuck Lever <chuck.lever@oracle.com>,
 Alexander Aring <alex.aring@gmail.com>, linux-fsdevel@vger.kernel.org,
 Paolo Bonzini <pbonzini@redhat.com>,
 =?UTF-8?Q?Christian_G=C3=B6ttsche?= <cgzones@googlemail.com>,
 Jens Axboe <axboe@kernel.dk>
References: <20240422084505.3465238-1-stsp2@yandex.ru>
 <20240422084505.3465238-2-stsp2@yandex.ru>
Content-Language: en-US, de-DE
From: Stefan Metzmacher <metze@samba.org>
In-Reply-To: <20240422084505.3465238-2-stsp2@yandex.ru>
Content-Type: text/plain; charset=UTF-8; format=flowed
Content-Transfer-Encoding: 8bit

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@yandex.ru>
> 
> CC: Eric Biederman <ebiederm@xmission.com>
> CC: Alexander Viro <viro@zeniv.linux.org.uk>
> CC: Andy Lutomirski <luto@kernel.org>
> CC: Christian Brauner <brauner@kernel.org>
> CC: Jan Kara <jack@suse.cz>
> CC: Jeff Layton <jlayton@kernel.org>
> CC: Chuck Lever <chuck.lever@oracle.com>
> CC: Alexander Aring <alex.aring@gmail.com>
> CC: linux-fsdevel@vger.kernel.org
> CC: linux-kernel@vger.kernel.org
> CC: Paolo Bonzini <pbonzini@redhat.com>
> CC: Christian Göttsche <cgzones@googlemail.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