| 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
| ||
|
Message-ID: <CAG48ez2o81ZwwL9muYyheN9vY69vJR5sB9LsLh=nk6wB4iuUgw@mail.gmail.com>
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