[PATCH] sysctl: fix permission check while owner isn't GLOBAL_ROOT_UID

[PATCH] sysctl: fix permission check while owner isn't GLOBAL_ROOT_UID

[menglong8.dong]

From: Yang Yang <yang.yang29@zte.com.cn>

With user namespace enabled, root in container can't modify
/proc/sys/net/ipv4/ip_forward. While /proc/sys/net/ipv4/ip_forward
belongs to root and mode is 644. Since root in container may
be non-root in host, but test_perm() doesn't consider about it.

Reported-by: Zeal Robot <zealci@zte.com.cn>
Signed-off-by: Yang Yang <yang.yang29@zte.com.cn>
---
 fs/proc/proc_sysctl.c | 15 ++++++++-------
 1 file changed, 8 insertions(+), 7 deletions(-)

diff --git a/fs/proc/proc_sysctl.c b/fs/proc/proc_sysctl.c
index dea0f5ee540c..71d7b2c2c8e3 100644
--- a/fs/proc/proc_sysctl.c
+++ b/fs/proc/proc_sysctl.c
@@ -400,18 +400,19 @@ static void next_entry(struct ctl_table_header **phead, struct ctl_table **pentr
  * some sysctl variables are readonly even to root.
  */
 
-static int test_perm(int mode, int op)
+static int test_perm(struct inode *inode, int mode, int op)
 {
-	if (uid_eq(current_euid(), GLOBAL_ROOT_UID))
+	if (uid_eq(current_euid(), inode->i_uid))
 		mode >>= 6;
-	else if (in_egroup_p(GLOBAL_ROOT_GID))
+	else if (in_egroup_p(inode->i_gid))
 		mode >>= 3;
 	if ((op & ~mode & (MAY_READ|MAY_WRITE|MAY_EXEC)) == 0)
 		return 0;
 	return -EACCES;
 }
 
-static int sysctl_perm(struct ctl_table_header *head, struct ctl_table *table, int op)
+static int sysctl_perm(struct inode *inode,
+	struct ctl_table_header *head, struct ctl_table *table, int op)
 {
 	struct ctl_table_root *root = head->root;
 	int mode;
@@ -421,7 +422,7 @@ static int sysctl_perm(struct ctl_table_header *head, struct ctl_table *table, i
 	else
 		mode = table->mode;
 
-	return test_perm(mode, op);
+	return test_perm(inode, mode, op);
 }
 
 static struct inode *proc_sys_make_inode(struct super_block *sb,
@@ -554,7 +555,7 @@ static ssize_t proc_sys_call_handler(struct kiocb *iocb, struct iov_iter *iter,
 	 * and won't be until we finish.
 	 */
 	error = -EPERM;
-	if (sysctl_perm(head, table, write ? MAY_WRITE : MAY_READ))
+	if (sysctl_perm(inode, head, table, write ? MAY_WRITE : MAY_READ))
 		goto out;
 
 	/* if that can happen at all, it should be -EINVAL, not -EISDIR */
@@ -803,7 +804,7 @@ static int proc_sys_permission(struct user_namespace *mnt_userns,
 	if (!table) /* global root - r-xr-xr-x */
 		error = mask & MAY_WRITE ? -EACCES : 0;
 	else /* Use the permissions on the sysctl table entry */
-		error = sysctl_perm(head, table, mask & ~MAY_NOT_BLOCK);
+		error = sysctl_perm(inode, head, table, mask & ~MAY_NOT_BLOCK);
 
 	sysctl_head_finish(head);
 	return error;
-- 
2.25.1

Re: [PATCH] sysctl: fix permission check while owner isn't GLOBAL_ROOT_UID

[Christian Brauner]

On Fri, Jun 25, 2021 at 01:33:38AM -0700, menglong8.dong@gmail.com wrote:
> From: Yang Yang <yang.yang29@zte.com.cn>
> 
> With user namespace enabled, root in container can't modify
> /proc/sys/net/ipv4/ip_forward. While /proc/sys/net/ipv4/ip_forward
> belongs to root and mode is 644. Since root in container may
> be non-root in host, but test_perm() doesn't consider about it.

I'm confused about what the actual problem is tbh:

root@h3:~# stat -c "%A %a %n" /proc/sys/net/ipv4/ip_forward
-rw-r--r-- 644 /proc/sys/net/ipv4/ip_forward

root@h3:~# echo 0 > /proc/sys/net/ipv4/ip_forward

root@h3:~# cat /proc/sys/net/ipv4/ip_forward
0

root@h3:~# cat /proc/self/uid_map 
         0     100000 1000000000

Also, this patch changes the security requirements for all sysctls which
is unfortunately unacceptable.

Re: [PATCH] sysctl: fix permission check while owner isn't GLOBAL_ROOT_UID

[Christian Brauner]

On Mon, Jun 28, 2021 at 09:14:10PM +0800, yang.yang29@zte.com.cn wrote:
> > I'm confused about what the actual problem is tbh:
> > 
> > root@h3:~# stat -c "%A %a %n" /proc/sys/net/ipv4/ip_forward
> > -rw-r--r-- 644 /proc/sys/net/ipv4/ip_forward
> > 
> > root@h3:~# echo 0 > /proc/sys/net/ipv4/ip_forward
> > 
> > root@h3:~# cat /proc/sys/net/ipv4/ip_forward
> > 0
> > 
> > root@h3:~# cat /proc/self/uid_map  
> >          0     100000 1000000000
> 
> Sorry to describe too simple. More specific:
> 1.Run dockerd with user namespace enbled
> echo dockremap:296608:65536 >  /etc/subuid;echo dockremap:296608:65536 >  /etc/subgid
> dockerd ... --userns-remap=default
> 2.Create a container
> docker run -it ... sh
> 
> Then root account in the container is 296608 account in the host:
> / # cat /proc/self/uid_map
>          0     296608      65536
> 
> In the container, /proc/sys/net/ipv4/ip_forward's owner is root, but root can't modify it:
> / # whoami
> root
> / # ls -l /proc/sys/net/ipv4/ip_forward
> -rw-r--r--    1 root     root             0 Jun 28 12:46 /proc/sys/net/ipv4/ip_forward
> / # cat /proc/sys/net/ipv4/ip_forward
> 1
> / # echo 0 > /proc/sys/net/ipv4/ip_forward
> sh: can't create /proc/sys/net/ipv4/ip_forward: Permission denied
> 
> And /proc/sys/net/ipv4/ip_forward has considerd about net namespace, 
> see net_ctl_set_ownership() in net\sysctl_net.c.
> So root in container should be able to modify /proc/sys/net/ipv4/ip_forward.
> This doesn't impact /proc/sys/net/ipv4/ip_forward in the host or other container with other net namespace.

Sorry to resort to "It works on my machine" but I just pasted you the
exact same setup and showed you that this works as expected.

So the reason here is likely that you're lacking capabilities
specifically CAP_NET_ADMIN. If I reproduce with your setup I can't even
create a dummy network device:

/ #  ip link add bla type dummy
ip: RTNETLINK answers: Operation not permitted

whereas on any unprivileged container that reatins CAP_NET_ADMIN that
just works fine.

So I would think that if you retain CAP_NET_ADMIN things will work out
just fine. Take a look at net_ctl_permissions.

There's really no reason to drop CAP_NET_ADMIN in a network namespace
that is owned by a non-initial user namespace anyway.

Christian