linux-kernel - Re: [PATCH v9 0/4] fuse: mounts from non-init user namespaces

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] [day] [month] [year] [list]

Date:   Wed, 21 Mar 2018 09:38:46 +0100
From:   Miklos Szeredi <mszeredi@...hat.com>
To:     "Eric W. Biederman" <ebiederm@...ssion.com>
Cc:     lkml <linux-kernel@...r.kernel.org>,
        Linux Containers <containers@...ts.linux-foundation.org>,
        linux-fsdevel <linux-fsdevel@...r.kernel.org>,
        Alban Crequy <alban@...volk.io>,
        Seth Forshee <seth.forshee@...onical.com>,
        Sargun Dhillon <sargun@...gun.me>,
        Dongsu Park <dongsu@...volk.io>,
        "Serge E. Hallyn" <serge@...lyn.com>
Subject: Re: [PATCH v9 0/4] fuse: mounts from non-init user namespaces

On Tue, Mar 20, 2018 at 7:27 PM, Eric W. Biederman
<ebiederm@...ssion.com> wrote:
> Miklos Szeredi <mszeredi@...hat.com> writes:

>> I did just one modification to "fuse: Fail all requests with invalid
>> uids or gids": instead of zeroing out the context for the nofail case,
>> continue to use the "_munged" variants. I don't think this hurts and
>> is better for backward compatibility (I guess the only relevant use
>> would be for debugging output, but we don't want to regress even for
>> that if not necessary)
>
> Hmm...
>
> The thing is the failure doesn't come in the difference between the
> _munged and the normal variants.  The difference between
> munged and non-munged variants is how they handled failure ((uid16_t)-2)
> aka 0xfffe for munged and -1 for the non-munged case.
>
> The failures are introduced by changing &init_user_ns to fc->user_ns.

Right.

> The operations in question are iop->flush and fuse_force_forget (on an
> error).   I don't know what value having ids on those paths will do
> they are operations that must succeed, and they should not change the
> on-disk ids.  I was thinking saying the most privileged id was asking
> for the oepration would seem to make sense.

I don't think anybody should actually *care* about the id's in flush,
but I'd still not change the current behavior for change's sake.

>
> With the munged variants we will get (uid16_t)-2 aka 0xfffe aka
> nobody asking for the operation if things don't map.  In practice
> the don't map case is new.
>
> Since the id's should not be looked at anyway I don't see it makes
> much difference which ids we use so the munged case seems at least
> plausible.
>
> It might be better to use the non-munghed variant and do:
>         if (req->in.h.uid == (uid_t)-1)
>                 req.in.h.uid = 0;
>         if (req->in.h.gid == (gid_t)-1)
>                 req.in.h.gid = 0;
>
> That might be less surprising to userspace.  As I don't think the
> unmapped case has ever occurred in practice yet.

Right, that would work too, but I don't think it actually matters, so
unless you can think of an actual security issue arising from using
the munged variants, I'd just leave it as it is.

Thanks,
Miklos