User namespace: don't allow sysctl in non-init user ns
A message that is not in the archive (sent off-list or not yet received): xs4all.20110915194812.GA24348@sergelap
Quoted 21 lines · Serge E. Hallyn
sysctl.chas its own custom uid check, which is not user namespace aware. As discovered by Richard, that allows root in a container privileged access to set all sysctls.To fix that, just refuse access if current is not in
init_user_ns. We may at some point want to relax that check so that some sysctls are allowed - for instancedmesg_restrictwhen syslog is containerized.Signed-off-by Serge Hallyn <[email protected]>Cc "Eric W. Biederman" <[email protected]>Cc Vasiliy Kulikov <[email protected]>--- kernel/sysctl.c | 2 ++ 1 files changed, 2 insertions(+), 0 deletions(-)kernel/sysctl.c +0
-0
kernel/sysctl.c void register_sysctl_root(struct ctl_table_root *root) · line 1704
Quoted 8 lines
kernel/sysctl.c +2
-0@@ -1697,6 +1697,8 @@ void register_sysctl_root(struct ctl_table_root *root)static int test_perm (int mode,int op) { +if (current_user_ns () != &init_user_ns) +return -EACCES ;if (!current_euid ()) mode >>=6 ;else if (in_egroup_p (0 ))
I haven't tested it, but it looks like this denies access to /proc/sys completely, right ? Wouldn't it be better to make access read-only ? For example glibc reads from several /proc/sys/... files for sysconf() and pathconf(). That would fail with this patch I think ?
Something like
kernel/sysctl.c renamed from kernel/sysctl.c.orig +9 -4
@@ -1892,10 +1892,15 @@static int test_perm (int mode,int op) { -if (!current_euid ()) - mode >>=6 ; -else if (in_egroup_p (0 )) - mode >>=3 ; +if (current_user_ns () != &init_user_ns) { +if (op &MAY_WRITE ) +return -EACCES ; + }else { +if (!current_euid ()) + mode >>=6 ; +else if (in_egroup_p (0 )) + mode >>=3 ; + }if ((op & ~mode & (MAY_READ |MAY_WRITE |MAY_EXEC )) ==0 )return 0 ;return -EACCES ;
Mike.
Quoted 23 lines · Miquel van Smoorenburg
Quoted 22 lines · Serge E. Hallyn
sysctl.chas its own custom uid check, which is not user namespace aware. As discovered by Richard, that allows root in a container privileged access to set all sysctls.To fix that, just refuse access if current is not in
init_user_ns. We may at some point want to relax that check so that some sysctls are allowed - for instancedmesg_restrictwhen syslog is containerized.Signed-off-by Serge Hallyn <[email protected]>Cc "Eric W. Biederman" <[email protected]>Cc Vasiliy Kulikov <[email protected]>--- kernel/sysctl.c | 2 ++ 1 files changed, 2 insertions(+), 0 deletions(-)kernel/sysctl.c +0
-0@@ -1697,6 +1697,8 @@ void register_sysctl_root(struct ctl_table_root *root)
Quoted 10 lines
kernel/sysctl.c void register_sysctl_root(struct ctl_table_root *root) · line 1704
Quoted 7 lines
kernel/sysctl.c +2
-0static int test_perm (int mode,int op) { +if (current_user_ns () != &init_user_ns) +return -EACCES ;if (!current_euid ()) mode >>=6 ;else if (in_egroup_p (0 ))I haven't tested it, but it looks like this denies access to
/proc/syscompletely, right ?
True. Good point.
Quoted 1 line
Wouldn't it be better to make access read-only ?
It'd be better, yes. For the moment we're trying to focus on making sure there are no leaks out of the user namespace, so that then we can start relaxing. But I think you're right, this is easy enough to do right from the start.
Quoted 17 lines
For example glibc reads from several
/proc/sys/... files forsysconf()andpathconf(). That would fail with this patch I think ?Something like
kernel/sysctl.c renamed from kernel/sysctl.c.orig +2
-4@@ -1892,10 +1892,15 @@static int test_perm (int mode,int op) { -if (!current_euid ()) - mode >>=6 ; -else if (in_egroup_p (0 )) - mode >>=3 ; +if (current_user_ns () != &init_user_ns) { +if (op &MAY_WRITE )
kernel/sysctl.c line 1906
Quoted 10 lines
kernel/sysctl.c renamed from kernel/sysctl.c.orig +7
-0+return -EACCES ; + }else { +if (!current_euid ()) + mode >>=6 ; +else if (in_egroup_p (0 )) + mode >>=3 ; + }if ((op & ~mode & (MAY_READ |MAY_WRITE |MAY_EXEC )) ==0 )return 0 ;return -EACCES ;
How about the following, slightly changed? It tries to more precisely follow the rule that access from another (non-ancestor) user-ns is checked as access from the overflow userid (-1).
if (current_user_ns() == &init_user_ns) {
if (!current_euid())
mode >>= 6;
else if (in_egroup_p(0))
mode >>= 3;
} if ((op & ~mode & (MAY_READ|MAY_WRITE|MAY_EXEC)) == 0)
return 0;
return -EACCES;
Does that make sense? Yes it does mean that any world-writeable files are writeable from a container, but those are the rules we'd earlier defined. (File creation (directory write) has to be a special exception only because we don't have a valid owner to assign to the new file).
Thanks for pointing this out!
-serge
PATCH 1/1 User namespace: don't allow sysctl in non-init user ns (v2)
sysctl.c has its own custom uid check, which is not user namespace aware. As discovered by Richard, that allows root in a container privileged access to set all sysctls.
To fix that, don't compare uid or groups if current is not in the initial user namespace. We may at some point want to relax that check so that some sysctls are allowed - for instance dmesg_restrict when syslog is containerized.
Changelog:
Sep 22: As Miquel van Smoorenburg pointed out, rather than always
refusing access if not in initial user_ns, we should allow world access rights to sysctl files. We just want to prevent a task in a non-init user namespace from getting the root user or group access rights.
--- kernel/sysctl.c | 10 ++++++---- 1 files changed, 6 insertions(+), 4 deletions(-)
kernel/sysctl.c +6 -4
@@ -1697,10 +1697,12 @@ void register_sysctl_root(struct ctl_table_root *root)static int test_perm (int mode,int op) { -if (!current_euid ()) - mode >>=6 ; -else if (in_egroup_p (0 )) - mode >>=3 ; +if (current_user_ns () == &init_user_ns) { +if (!current_euid ()) + mode >>=6 ; +else if (in_egroup_p (0 )) + mode >>=3 ; + }if ((op & ~mode & (MAY_READ |MAY_WRITE |MAY_EXEC )) ==0 )return 0 ;return -EACCES ;
Signature · 1 line
1.7.0.4