Date:   Fri, 14 Feb 2020 20:11:36 +0100
From:   Jann Horn <jannh@...gle.com>
To:     Christian Brauner <christian.brauner@...ntu.com>
Cc:     Stéphane Graber <stgraber@...ntu.com>,
        "Eric W. Biederman" <ebiederm@...ssion.com>,
        Aleksa Sarai <cyphar@...har.com>,
        Stephen Barber <smbarber@...omium.org>,
        Seth Forshee <seth.forshee@...onical.com>,
        Alexander Viro <viro@...iv.linux.org.uk>,
        Alexey Dobriyan <adobriyan@...il.com>,
        Serge Hallyn <serge@...lyn.com>,
        James Morris <jmorris@...ei.org>,
        Kees Cook <keescook@...omium.org>,
        Jonathan Corbet <corbet@....net>,
        Phil Estes <estesp@...il.com>,
        kernel list <linux-kernel@...r.kernel.org>,
        linux-fsdevel <linux-fsdevel@...r.kernel.org>,
        Linux Containers <containers@...ts.linux-foundation.org>,
        linux-security-module <linux-security-module@...r.kernel.org>,
        Linux API <linux-api@...r.kernel.org>
Subject: Re: [PATCH v2 04/28] fsuidgid: add fsid mapping helpers

On Fri, Feb 14, 2020 at 7:37 PM Christian Brauner
<christian.brauner@...ntu.com> wrote:
> This adds a set of helpers to translate between kfsuid/kfsgid and their
> userspace fsuid/fsgid counter parts relative to a given user namespace.
>
> - kuid_t make_kfsuid(struct user_namespace *from, uid_t fsuid)
>   Maps a user-namespace fsuid pair into a kfsuid.
>   If no fsuid mappings have been written it behaves identical to calling
>   make_kuid(). This ensures backwards compatibility for workloads unaware
>   or not in need of fsid mappings.
[...]
> +#ifdef CONFIG_USER_NS_FSID
> +/**
> + *     make_kfsuid - Map a user-namespace fsuid pair into a kuid.
> + *     @ns:  User namespace that the fsuid is in
> + *     @fsuid: User identifier
> + *
> + *     Maps a user-namespace fsuid pair into a kernel internal kfsuid,
> + *     and returns that kfsuid.
> + *
> + *     When there is no mapping defined for the user-namespace kfsuid
> + *     pair INVALID_UID is returned.  Callers are expected to test
> + *     for and handle INVALID_UID being returned.  INVALID_UID
> + *     may be tested for using uid_valid().
> + */
> +kuid_t make_kfsuid(struct user_namespace *ns, uid_t fsuid)
> +{
> +       unsigned extents = ns->fsuid_map.nr_extents;
> +       smp_rmb();
> +
> +       /* Map the fsuid to a global kernel fsuid */
> +       if (extents == 0)
> +               return KUIDT_INIT(map_id_down(&ns->uid_map, fsuid));
> +
> +       return KUIDT_INIT(map_id_down(&ns->fsuid_map, fsuid));
> +}
> +EXPORT_SYMBOL(make_kfsuid);

What effect is this fallback going to have for nested namespaces?

Let's say we have an outer namespace N1 with this uid_map:

    0 100000 65535

and with this fsuid_map:

    0 300000 65535

Now from in there, a process that is not aware of the existence of
fsuid mappings creates a new user namespace N2 with the following
uid_map:

    0 1000 1

At this point, if a process in N2 does chown("foo", 0, 0), is that
going to make "foo" owned by kuid 101000, which isn't even mapped in
N1?

> @@ -1215,11 +1376,13 @@ static bool new_idmap_permitted(const struct file *file,
>             uid_eq(ns->owner, cred->euid)) {
>                 u32 id = new_map->extent[0].lower_first;
>                 if (cap_setid == CAP_SETUID) {
> -                       kuid_t uid = make_kuid(ns->parent, id);
> +                       kuid_t uid = map_fsid ? make_kfsuid(ns->parent, id) :
> +                                               make_kuid(ns->parent, id);
>                         if (uid_eq(uid, cred->euid))
>                                 return true;

Let's say we have an outer user namespace N1 with this uid_map:

    0 1000 3000

and this fsuid_map:

    0 2000 3000

and in that namespace, a process is running as UID 1000 (which means
kernel-euid=2000, kernel-fsuid=3000). Now this process unshares its
user namespace and from this nested user namespace N2, tries to write
the following fsuid_map:

    0 1000 1

This should work, since the only ID it maps is the one the process had
in N1; but the code is AFAICS going to run as follows:

        if ((new_map->nr_extents == 1) && (new_map->extent[0].count == 1) &&
            uid_eq(ns->owner, cred->euid)) { // branch taken
                u32 id = new_map->extent[0].lower_first;
                if (cap_setid == CAP_SETUID) { // branch taken
                        // uid = make_kfsuid(ns->parent, 1000) = 3000
                        kuid_t uid = map_fsid ? make_kfsuid(ns->parent, id) :
                                                make_kuid(ns->parent, id);
                        // uid_eq(3000, 2000)
                        if (uid_eq(uid, cred->euid)) // not taken
                                return true;
                } else [...]
        }

Instead, I think what would succeed is this, which shouldn't be allowed:

    0 0 1

which AFAICS will evaluate as follows:

        if ((new_map->nr_extents == 1) && (new_map->extent[0].count == 1) &&
            uid_eq(ns->owner, cred->euid)) { // branch taken
                u32 id = new_map->extent[0].lower_first;
                if (cap_setid == CAP_SETUID) { // branch taken
                        // uid = make_kfsuid(ns->parent, 0) = 2000
                        kuid_t uid = map_fsid ? make_kfsuid(ns->parent, id) :
                                                make_kuid(ns->parent, id);
                        // uid_eq(2000, 2000)
                        if (uid_eq(uid, cred->euid)) // taken
                                return true;
                } else [...]
        }

>                 } else if (cap_setid == CAP_SETGID) {
> -                       kgid_t gid = make_kgid(ns->parent, id);
> +                       kgid_t gid = map_fsid ? make_kfsgid(ns->parent, id) :
> +                                               make_kgid(ns->parent, id);
>                         if (!(ns->flags & USERNS_SETGROUPS_ALLOWED) &&
>                             gid_eq(gid, cred->egid))
>                                 return true;
> --
> 2.25.0
>

Powered by blists - more mailing lists