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
 
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [thread-next>] [day] [month] [year] [list]
Message-Id: <20231210111033.1823491-1-maze@google.com>
Date: Sun, 10 Dec 2023 03:10:32 -0800
From: "Maciej Żenczykowski" <maze@...gle.com>
To: "Maciej Żenczykowski" <zenczykowski@...il.com>
Cc: Linux Network Development Mailing List <netdev@...r.kernel.org>, "David S . Miller" <davem@...emloft.net>, 
	Eric Dumazet <edumazet@...gle.com>, Jakub Kicinski <kuba@...nel.org>, Paolo Abeni <pabeni@...hat.com>, 
	"Maciej Żenczykowski" <maze@...gle.com>, Flavio Crisciani <fcrisciani@...gle.com>, 
	"Theodore Y. Ts'o" <tytso@...gle.com>, "Eric W. Biederman" <ebiederm@...ssion.com>, 
	Dmitry Torokhov <dmitry.torokhov@...il.com>
Subject: [PATCH] net: sysctl: fix edge case wrt. sysctl write access

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@...gle.com>
Cc: "Theodore Y. Ts'o" <tytso@...gle.com>
Cc: "Eric W. Biederman" <ebiederm@...ssion.com>
Cc: Dmitry Torokhov <dmitry.torokhov@...il.com>
Fixes: e79c6a4fc923 ("net: make net namespace sysctls belong to container's owner")
Signed-off-by: Maciej Żenczykowski <maze@...gle.com>
---
 net/sysctl_net.c | 13 ++++---------
 1 file changed, 4 insertions(+), 9 deletions(-)

diff --git a/net/sysctl_net.c b/net/sysctl_net.c
index 051ed5f6fc93..ded399f380d9 100644
--- a/net/sysctl_net.c
+++ b/net/sysctl_net.c
@@ -58,16 +58,11 @@ static void net_ctl_set_ownership(struct ctl_table_header *head,
 				  kuid_t *uid, kgid_t *gid)
 {
 	struct net *net = container_of(head->set, struct net, sysctls);
-	kuid_t ns_root_uid;
-	kgid_t ns_root_gid;
+	kuid_t ns_root_uid = make_kuid(net->user_ns, 0);
+	kgid_t ns_root_gid = make_kgid(net->user_ns, 0);
 
-	ns_root_uid = make_kuid(net->user_ns, 0);
-	if (uid_valid(ns_root_uid))
-		*uid = ns_root_uid;
-
-	ns_root_gid = make_kgid(net->user_ns, 0);
-	if (gid_valid(ns_root_gid))
-		*gid = ns_root_gid;
+	*uid = uid_valid(ns_root_uid) ? ns_root_uid : net->user_ns->owner;
+	*gid = gid_valid(ns_root_gid) ? ns_root_gid : net->user_ns->group;
 }
 
 static struct ctl_table_root net_sysctl_root = {
-- 
2.43.0.472.g3155946c3a-goog


Powered by blists - more mailing lists

Powered by Openwall GNU/*/Linux Powered by OpenVZ