cgroup-v1: Require capabilities to set release_agent
PATCH 1/1 cgroup-v1: Require capabilities to set release_agent
From: "Eric W. Biederman" <[email protected]>
commit 24f6008564183aa120d07c03d9289519c2fe02af upstream.
The cgroup release_agent is called with call_usermodehelper. The function call_usermodehelper starts the release_agent with a full set fo capabilities. Therefore require capabilities when setting the release_agaent.
[masami: Backport patch from 4.9. Adjust to use current_user_ns() to get current user_ns. Fix conflict in cgroup_release_agent_write().]
Reference: CVE-2022-0492
--- kernel/cgroup.c | 25 +++++++++++++++++++++++++ 1 file changed, 25 insertions(+)
kernel/cgroup.c +25 -0
@@ -1786,6 +1786,13 @@ static int cgroup_remount(struct kernfs_root *kf_root, int *flags, char *data)pr_warn ("option changes via remount are deprecated (pid=%d comm=%s)\n" ,task_tgid_nr (current), current->comm); +/* See cgroup_mount release_agent handling */ +if (opts.release_agent && + ((current_user_ns () != &init_user_ns) || !capable (CAP_SYS_ADMIN ))) { + ret = -EINVAL ; + goto out_unlock; + } + added_mask = opts.subsys_mask & ~root->subsys_mask; removed_mask = root->subsys_mask & ~opts.subsys_mask;
@@ -2135,6 +2142,16 @@ static struct dentry *cgroup_mount(struct file_system_type *fs_type, goto out_unlock; } +/* +* Release agent gets called with all capabilities, +* require capabilities to set release agent. +*/ +if (opts.release_agent && + ((current_user_ns () != &init_user_ns) || !capable (CAP_SYS_ADMIN ))) { + ret = -EINVAL ; + goto out_unlock; + } + root =kzalloc (sizeof (*root),GFP_KERNEL );if (!root) { ret = -ENOMEM ;
@@ -2839,6 +2856,14 @@ static ssize_t cgroup_release_agent_write(struct kernfs_open_file *of,BUILD_BUG_ON (sizeof (cgrp->root->release_agent_path) <PATH_MAX ); +/* +* Release agent gets called with all capabilities, +* require capabilities to set release agent. +*/ +if ((of->file->f_cred->user_ns != &init_user_ns) || + !capable (CAP_SYS_ADMIN )) +return -EPERM ; + cgrp =cgroup_kn_lock_live (of->kn);if (!cgrp)return -ENODEV ;
Signature · 1 line
2.35.1
Thanks for sharing this Masami, I've been looking into pre-cgroup_ns backport too.
Quoted 2 lines · Masami Ichikawa
[masami: Backport patch from 4.9. Adjust to use
current_user_ns()to get currentuser_ns. Fix conflict incgroup_release_agent_write().]
The condition to allow modifying release_agent is two-fold:
a) caller is capabable(CAP_SYS_ADMIN),
b) cgroup_ns is owned by init_user_ns.
In pre-cgroup_ns kernels, it is IMO safer to consider all (=the only) cgroup_ns owned by init_user_ns.
So the (positive) condition translates into capable(CAP_SYS_ADMIN) only.
[
Additionally, there's invariant/implication
capable(CAP_XXX) -> (current_user_ns() == &init_user_ns) ,
so the expression
(current_user_ns() != &init_user_ns) || !capable(CAP_SYS_ADMIN)
simplifies to
!capable(CAP_SYS_ADMIN) .
]
kernel/cgroup.c static ssize_t cgroup_release_agent_write(struct kernfs_open_file *of, · line 2866
Quoted 12 lines
kernel/cgroup.c +8
-0@@ -2839,6 +2856,14 @@ static ssize_t cgroup_release_agent_write(struct kernfs_open_file *of,BUILD_BUG_ON (sizeof (cgrp->root->release_agent_path) <PATH_MAX ); +/* +* Release agent gets called with all capabilities, +* require capabilities to set release agent. +*/ +if ((of->file->f_cred->user_ns != &init_user_ns) || + !capable (CAP_SYS_ADMIN )) +return -EPERM ; +
Following the reasoning above, the check simplifies too but it should be be against the opener, not the writer:
file_ns_capable(of->file, &init_user_ns, CAP_SYS_ADMIN)
(It seems to be to be incorrect even in the original commit. So I'll send a patch there to rectify that.)
Michal
Quoted 4 lines · Michal Koutný
Thanks for sharing this Masami, I've been looking into pre-cgroup_ns backport too.
Thank you for the review.
Quoted 13 lines · Masami Ichikawa
Quoted 2 lines
[masami: Backport patch from 4.9. Adjust to use
current_user_ns()to get currentuser_ns. Fix conflict incgroup_release_agent_write().]The condition to allow modifying
release_agentis two-fold:
a) caller iscapabable(CAP_SYS_ADMIN),
b)cgroup_nsis owned byinit_user_ns.In pre-cgroup_ns kernels, it is IMO safer to consider all (=the only)
cgroup_nsowned byinit_user_ns.So the (positive) condition translates into
capable(CAP_SYS_ADMIN)only.
I see. Thank you for the details.
If we treat cgroup_ns as only cgroup_ns that are owned by init_user_ns, we don't have to have an extra check.
Quoted 14 lines
[
Additionally, there's invariant/implication
capable(CAP_XXX)-> (current_user_ns()==&init_user_ns) ,
so the expression
(current_user_ns()!=&init_user_ns) || !capable(CAP_SYS_ADMIN)
simplifies to
!capable(CAP_SYS_ADMIN) .
]Quoted 4 lines
kernel/cgroup.c +0
-0@@ -2839,6 +2856,14 @@ static ssize_t cgroup_release_agent_write(struct kernfs_open_file *of,BUILD_BUG_ON (sizeof (cgrp->root->release_agent_path) <PATH_MAX );
Quoted 13 lines
kernel/cgroup.c static ssize_t cgroup_release_agent_write(struct kernfs_open_file *of, · line 2866
Quoted 8 lines
kernel/cgroup.c +8
-0+/* +* Release agent gets called with all capabilities, +* require capabilities to set release agent. +*/ +if ((of->file->f_cred->user_ns != &init_user_ns) || + !capable (CAP_SYS_ADMIN )) +return -EPERM ; +Following the reasoning above, the check simplifies too but it should be be against the opener, not the writer:
file_ns_capable(of->file, &init_user_ns, CAP_SYS_ADMIN)
Thanks. I got it.
Quoted 7 lines
(It seems to be to be incorrect even in the original commit. So I'll send a patch there to rectify that.)
Michal
Regards,
Signature · 5 lines
/** * Masami Ichikawa * personal: [email protected] * fedora project: [email protected] */