Re: [PATCH v2 04/28] fsuidgid: add fsid mapping helpers

On Fri, Feb 14, 2020 at 7:37 PM Christian Brauner
[off-list ref] 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?

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