linux-kernel - Re: [PATCH v1 2/9] fs/fuse: add FUSE_OWNER_UID_GID

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: <CAEivzxf1TLUeR_j8h5LfkmLOAKzrenK55bw9Qj4OV0=7Dkx9=w@mail.gmail.com>
Date: Thu, 29 Aug 2024 14:17:55 +0200
From: Aleksandr Mikhalitsyn <aleksandr.mikhalitsyn@...onical.com>
To: Christian Brauner <brauner@...nel.org>
Cc: Miklos Szeredi <miklos@...redi.hu>, mszeredi@...hat.com, stgraber@...raber.org, 
	linux-fsdevel@...r.kernel.org, Seth Forshee <sforshee@...nel.org>, 
	Amir Goldstein <amir73il@...il.com>, Bernd Schubert <bschubert@....com>, linux-kernel@...r.kernel.org
Subject: Re: [PATCH v1 2/9] fs/fuse: add FUSE_OWNER_UID_GID_EXT extension

On Thu, Aug 29, 2024 at 2:08 PM Christian Brauner <brauner@...nel.org> wrote:
>
> On Thu, Aug 29, 2024 at 10:24:42AM GMT, Miklos Szeredi wrote:
> > On Thu, 18 Jul 2024 at 21:12, Aleksandr Mikhalitsyn
> > <aleksandr.mikhalitsyn@...onical.com> wrote:
> >
> > > This was a first Christian's idea when he originally proposed a
> > > patchset for cephfs [2]. The problem with this
> > > approach is that we don't have an idmapping provided in all
> > > inode_operations, we only have it where it is supposed to be.
> > > To workaround that, Christian suggested applying a mapping only when
> > > we have mnt_idmap, but if not just leave uid/gid as it is.
> > > This, of course, leads to inconsistencies between different
> > > inode_operations, for example ->lookup (idmapping is not applied) and
> > > ->symlink (idmapping is applied).
> > > This inconsistency, really, is not a big deal usually, but... what if
> > > a server does UID/GID-based permission checks? Then it is a problem,
> > > obviously.
> >
> > Is it even sensible to do UID/GID-based permission checks in the
> > server if idmapping is enabled?

Dear friends,

>
> It really makes no sense.

+

>
> >
> > If not, then we should just somehow disable that configuration (i.e.
> > by the server having to opt into idmapping), and then we can just use
> > the in_h.[ugi]d for creates, no?
>
> Fwiw, that's what the patchset is doing. It's only supported if the
> server sets "default_permissions".

Yeah. Thanks, Christian!

That's what we have:

+++ b/fs/fuse/inode.c
@@ -1345,6 +1345,12 @@ static void process_init_reply(struct
fuse_mount *fm, struct fuse_args *args,
                 fm->sb->s_export_op = &fuse_export_fid_operations;
             if (flags & FUSE_OWNER_UID_GID_EXT)
                 fc->owner_uid_gid_ext = 1;
+            if (flags & FUSE_ALLOW_IDMAP) {
+                if (fc->owner_uid_gid_ext && fc->default_permissions)
+                    fm->sb->s_iflags &= ~SB_I_NOIDMAP;
+                else
+                    ok = false;
+            }
         } else {
             ra_pages = fc->max_read / PAGE_SIZE;

So idmapped mounts can be enabled ONLY if "default_permissions" mode
is set. At the same time,
some fuse servers (glusterfs), even when "default_permissions" is set,
still have some UID/GID-based checks.
So, they effectively duplicate permission checking logic in the
userspace. We can not do anything with that, but only
fix fuse servers to stop doing so. See also my PoC for glusterfs-fuse
idmapped mounts support:
https://github.com/mihalicyn/glusterfs/commit/ab3ec2c7cbe22618cba9cc94a52a492b1904d0b2

Kind regards,
Alex