From: Jann Horn <jannh@google.com>
To: Christian Brauner <christian.brauner@ubuntu.com>
Cc: "Stéphane Graber" <stgraber@ubuntu.com>,
"Eric W. Biederman" <ebiederm@xmission.com>,
"Aleksa Sarai" <cyphar@cyphar.com>,
"Stephen Barber" <smbarber@chromium.org>,
"Seth Forshee" <seth.forshee@canonical.com>,
"Alexander Viro" <viro@zeniv.linux.org.uk>,
"Alexey Dobriyan" <adobriyan@gmail.com>,
"Serge Hallyn" <serge@hallyn.com>,
"James Morris" <jmorris@namei.org>,
"Kees Cook" <keescook@chromium.org>,
"Jonathan Corbet" <corbet@lwn.net>,
"Phil Estes" <estesp@gmail.com>,
"kernel list" <linux-kernel@vger.kernel.org>,
linux-fsdevel <linux-fsdevel@vger.kernel.org>,
"Linux Containers" <containers@lists.linux-foundation.org>,
linux-security-module <linux-security-module@vger.kernel.org>,
"Linux API" <linux-api@vger.kernel.org>
Subject: Re: [PATCH v2 04/28] fsuidgid: add fsid mapping helpers
Date: Fri, 14 Feb 2020 20:11:36 +0100 [thread overview]
Message-ID: <CAG48ez2o81ZwwL9muYyheN9vY69vJR5sB9LsLh=nk6wB4iuUgw@mail.gmail.com> (raw)
In-Reply-To: <20200214183554.1133805-5-christian.brauner@ubuntu.com>
On Fri, Feb 14, 2020 at 7:37 PM Christian Brauner
<christian.brauner@ubuntu.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
>
next prev parent reply other threads:[~2020-02-14 19:12 UTC|newest]
Thread overview: 43+ messages / expand[flat|nested] mbox.gz Atom feed top
2020-02-14 18:35 [PATCH v2 00/28] user_namespace: introduce fsid mappings Christian Brauner
2020-02-14 18:35 ` [PATCH v2 01/28] user_namespace: introduce fsid mappings infrastructure Christian Brauner
2020-02-14 18:35 ` [PATCH v2 02/28] proc: add /proc/<pid>/fsuid_map Christian Brauner
2020-02-14 18:35 ` [PATCH v2 03/28] proc: add /proc/<pid>/fsgid_map Christian Brauner
2020-02-14 18:35 ` [PATCH v2 04/28] fsuidgid: add fsid mapping helpers Christian Brauner
2020-02-14 19:11 ` Jann Horn [this message]
2020-02-16 16:55 ` Christian Brauner
2020-02-14 18:35 ` [PATCH v2 05/28] proc: task_state(): use from_kfs{g,u}id_munged Christian Brauner
2020-02-14 18:35 ` [PATCH v2 06/28] cred: add kfs{g,u}id Christian Brauner
2020-02-14 18:35 ` [PATCH v2 07/28] sys: __sys_setfsuid(): handle fsid mappings Christian Brauner
2020-02-14 18:35 ` [PATCH v2 08/28] sys: __sys_setfsgid(): " Christian Brauner
2020-02-14 18:35 ` [PATCH v2 09/28] sys:__sys_setuid(): " Christian Brauner
2020-02-14 18:35 ` [PATCH v2 10/28] sys:__sys_setgid(): " Christian Brauner
2020-02-14 18:35 ` [PATCH v2 11/28] sys:__sys_setreuid(): " Christian Brauner
2020-02-14 18:35 ` [PATCH v2 12/28] sys:__sys_setregid(): " Christian Brauner
2020-02-14 18:35 ` [PATCH v2 13/28] sys:__sys_setresuid(): " Christian Brauner
2020-02-14 18:35 ` [PATCH v2 14/28] sys:__sys_setresgid(): " Christian Brauner
2020-02-14 18:35 ` [PATCH v2 15/28] fs: add is_userns_visible() helper Christian Brauner
2020-02-14 18:35 ` [PATCH v2 16/28] namei: may_{o_}create(): handle fsid mappings Christian Brauner
2020-02-14 18:35 ` [PATCH v2 17/28] inode: inode_owner_or_capable(): " Christian Brauner
2020-02-14 18:35 ` [PATCH v2 18/28] capability: privileged_wrt_inode_uidgid(): " Christian Brauner
2020-02-14 18:35 ` [PATCH v2 19/28] stat: " Christian Brauner
2020-02-14 19:03 ` Tycho Andersen
2020-02-16 14:12 ` Christian Brauner
2020-02-14 18:35 ` [PATCH v2 20/28] open: " Christian Brauner
2020-02-14 18:35 ` [PATCH v2 21/28] posix_acl: " Christian Brauner
2020-02-14 18:35 ` [PATCH v2 22/28] attr: notify_change(): " Christian Brauner
2020-02-14 18:35 ` [PATCH v2 23/28] commoncap: cap_bprm_set_creds(): " Christian Brauner
2020-02-14 18:35 ` [PATCH v2 24/28] commoncap: cap_task_fix_setuid(): " Christian Brauner
2020-02-14 18:35 ` [PATCH v2 25/28] commoncap: handle fsid mappings with vfs caps Christian Brauner
2020-02-14 18:35 ` [PATCH v2 26/28] exec: bprm_fill_uid(): handle fsid mappings Christian Brauner
2020-02-14 18:35 ` [PATCH v2 27/28] ptrace: adapt ptrace_may_access() to always uses unmapped fsids Christian Brauner
2020-02-14 18:35 ` [PATCH v2 28/28] devpts: handle fsid mappings Christian Brauner
2020-02-16 15:55 ` [PATCH v2 00/28] user_namespace: introduce " Florian Weimer
2020-02-16 16:40 ` Christian Brauner
2020-02-17 21:06 ` James Bottomley
2020-02-17 21:20 ` Christian Brauner
2020-02-17 22:35 ` James Bottomley
2020-02-17 23:05 ` Christian Brauner
2020-02-17 21:11 ` James Bottomley
[not found] ` <CA+enf=vwd-dxzve87t7Mw1Z35RZqdLzVaKq=fZ4EGOpnES0f5w@mail.gmail.com>
2020-02-17 22:02 ` Stéphane Graber
2020-02-17 23:03 ` James Bottomley
2020-02-17 23:11 ` Stéphane Graber
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to='CAG48ez2o81ZwwL9muYyheN9vY69vJR5sB9LsLh=nk6wB4iuUgw@mail.gmail.com' \
--to=jannh@google.com \
--cc=adobriyan@gmail.com \
--cc=christian.brauner@ubuntu.com \
--cc=containers@lists.linux-foundation.org \
--cc=corbet@lwn.net \
--cc=cyphar@cyphar.com \
--cc=ebiederm@xmission.com \
--cc=estesp@gmail.com \
--cc=jmorris@namei.org \
--cc=keescook@chromium.org \
--cc=linux-api@vger.kernel.org \
--cc=linux-fsdevel@vger.kernel.org \
--cc=linux-kernel@vger.kernel.org \
--cc=linux-security-module@vger.kernel.org \
--cc=serge@hallyn.com \
--cc=seth.forshee@canonical.com \
--cc=smbarber@chromium.org \
--cc=stgraber@ubuntu.com \
--cc=viro@zeniv.linux.org.uk \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).