| Message ID | 20231214132523.929567-1-maze@google.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | [net,v2] net: sysctl: fix edge case wrt. sysctl write access | expand |
diff --git a/net/sysctl_net.c b/net/sysctl_net.c index 051ed5f6fc93..2cdda78308be 100644 --- a/net/sysctl_net.c +++ b/net/sysctl_net.c @@ -62,12 +62,10 @@ static void net_ctl_set_ownership(struct ctl_table_header *head, kgid_t ns_root_gid; ns_root_uid = make_kuid(net->user_ns, 0); - if (uid_valid(ns_root_uid)) - *uid = ns_root_uid; + *uid = uid_valid(ns_root_uid) ? ns_root_uid : net->user_ns->owner; ns_root_gid = make_kgid(net->user_ns, 0); - if (gid_valid(ns_root_gid)) - *gid = ns_root_gid; + *gid = gid_valid(ns_root_gid) ? ns_root_gid : net->user_ns->group; } static struct ctl_table_root net_sysctl_root = {
The clear intent of net_ctl_permissions() is that having CAP_NET_ADMIN grants write access to networking sysctls. However, it turns out there is an edge case where this is insufficient: inode_permission() has an additional check on HAS_UNMAPPED_ID(inode) which can return -EACCES and thus block *all* write access. Note: AFAICT this check is wrt. the uid/gid mapping that was active at the time the filesystem (ie. proc) was mounted. In order for this check to not fail, we need net_ctl_set_ownership() to set valid uid/gid. It is not immediately clear what value to use, nor what values are guaranteed to work. It does make sense that /proc/sys/net appear to be owned by root from within the netns owning userns. As such we only modify what happens if the code fails to map uid/gid 0. Currently the code just fails to do anything, which in practice results in using the zeroes of freshly allocated memory, and we thus end up with global root. With this change we instead use the uid/gid of the owning userns. While it is probably (?) theoretically possible for this to *also* be unmapped from the /proc filesystem's point of view, this seems much less likely to happen in practice. The old code is observed to fail in a relatively complex setup, within a global root created user namespace with selectively mapped uid/gids (not including global root) and /proc mounted afterwards (so this /proc mount does not have global root mapped). Within this user namespace another non privileged task creates a new user namespace, maps it's own uid/gid (but not uid/gid 0), and then creates a network namespace. It cannot write to networking sysctls even though it does have CAP_NET_ADMIN. This is because net_ctl_set_ownership fails to map uid/gid 0 (because uid/gid 0 are *not* mapped in the owning 2nd level user_ns), and falls back to global root. But global root is not mapped in the 1st level user_ns, which was inherited by the /proc mount, and thus fails... Note: the uid/gid of networking sysctls is of purely superficial importance, outside of this UNMAPPED check, it does not actually affect access, and only affects display. Access is always based on whether you are *global* root uid (or have CAP_NET_ADMIN over the netns) for user write access bits (or are in *global* root gid for group write access bits). Cc: Flavio Crisciani <fcrisciani@google.com> Cc: "Theodore Y. Ts'o" <tytso@google.com> Cc: "Eric W. Biederman" <ebiederm@xmission.com> Cc: Dmitry Torokhov <dmitry.torokhov@gmail.com> Cc: Paolo Abeni <pabeni@redhat.com> Fixes: e79c6a4fc923 ("net: make net namespace sysctls belong to container's owner") Signed-off-by: Maciej Żenczykowski <maze@google.com> --- net/sysctl_net.c | 6 ++---- 1 file changed, 2 insertions(+), 4 deletions(-)