If only the dynamic part of procfs is mounted (subset=pid), then there is no
need to check if procfs is fully visible to the user in the new user namespace.
Alexey Gladkov (2):
proc: Relax check of mount visibility
Show /proc/self/net only for CAP_NET_ADMIN
fs/namespace.c | 27 ++++++++++++++++-----------
fs/proc/proc_net.c | 6 ++++++
fs/proc/root.c | 16 +++++++++-------
include/linux/fs.h | 1 +
4 files changed, 32 insertions(+), 18 deletions(-)
--
2.25.4
Show /proc/self/net only for CAP_NET_ADMIN if procfs is mounted with
subset=pid option in user namespace. This is done to avoid possible
information leakage.
Signed-off-by: Alexey Gladkov <[email protected]>
---
fs/proc/proc_net.c | 6 ++++++
1 file changed, 6 insertions(+)
diff --git a/fs/proc/proc_net.c b/fs/proc/proc_net.c
index dba63b2429f0..11fa2c4b3529 100644
--- a/fs/proc/proc_net.c
+++ b/fs/proc/proc_net.c
@@ -275,6 +275,12 @@ static struct net *get_proc_task_net(struct inode *dir)
struct task_struct *task;
struct nsproxy *ns;
struct net *net = NULL;
+ struct proc_fs_info *fs_info = proc_sb_info(dir->i_sb);
+
+ if ((fs_info->pidonly == PROC_PIDONLY_ON) &&
+ (current_user_ns() != &init_user_ns) &&
+ !capable(CAP_NET_ADMIN))
+ return net;
rcu_read_lock();
task = pid_task(proc_pid(dir), PIDTYPE_PID);
--
2.25.4
Alexey Gladkov <[email protected]> writes:
> Show /proc/self/net only for CAP_NET_ADMIN if procfs is mounted with
> subset=pid option in user namespace. This is done to avoid possible
> information leakage.
>
> Signed-off-by: Alexey Gladkov <[email protected]>
> ---
> fs/proc/proc_net.c | 6 ++++++
> 1 file changed, 6 insertions(+)
>
> diff --git a/fs/proc/proc_net.c b/fs/proc/proc_net.c
> index dba63b2429f0..11fa2c4b3529 100644
> --- a/fs/proc/proc_net.c
> +++ b/fs/proc/proc_net.c
> @@ -275,6 +275,12 @@ static struct net *get_proc_task_net(struct inode *dir)
> struct task_struct *task;
> struct nsproxy *ns;
> struct net *net = NULL;
> + struct proc_fs_info *fs_info = proc_sb_info(dir->i_sb);
> +
> + if ((fs_info->pidonly == PROC_PIDONLY_ON) &&
> + (current_user_ns() != &init_user_ns) &&
> + !capable(CAP_NET_ADMIN))
> + return net;
>
> rcu_read_lock();
> task = pid_task(proc_pid(dir), PIDTYPE_PID);
Hmm.
I see 3 options going forward.
1) We just make PROC_PIDONLY_ON mean the net directory does not exist.
No permission checks just always fail.
2) Move the permission checks into opendir/readdir and whichever
is the appropriate method there and always allow the dentries
to be cached.
3) Simply cache the mounters credentials and make access to the
net directories contingent of the permisions of the mounter of
proc. Something like the code below.
static struct net *get_proc_task_net(struct inode *dir)
{
struct task_struct *task;
struct nsproxy *ns;
struct net *net = NULL;
rcu_read_lock();
task = pid_task(proc_pid(dir), PIDTYPE_PID);
if (task != NULL) {
task_lock(task);
ns = task->nsproxy;
if (ns != NULL)
net = get_net(ns->net_ns);
task_unlock(task);
}
rcu_read_unlock();
if ((fs_info->pidonly == PROC_PIDONLY_ON) &&
!security_capable(fs_info->mounter_cred,
net->user_ns, CAP_SYS_ADMIN,
CAP_OPT_NONE)) {
put_net(net);
net = NULL;
}
return net;
}
Eric
On Mon, Jul 27, 2020 at 11:29:36AM -0500, Eric W. Biederman wrote:
> Alexey Gladkov <[email protected]> writes:
>
> > Show /proc/self/net only for CAP_NET_ADMIN if procfs is mounted with
> > subset=pid option in user namespace. This is done to avoid possible
> > information leakage.
> >
> > Signed-off-by: Alexey Gladkov <[email protected]>
> > ---
> > fs/proc/proc_net.c | 6 ++++++
> > 1 file changed, 6 insertions(+)
> >
> > diff --git a/fs/proc/proc_net.c b/fs/proc/proc_net.c
> > index dba63b2429f0..11fa2c4b3529 100644
> > --- a/fs/proc/proc_net.c
> > +++ b/fs/proc/proc_net.c
> > @@ -275,6 +275,12 @@ static struct net *get_proc_task_net(struct inode *dir)
> > struct task_struct *task;
> > struct nsproxy *ns;
> > struct net *net = NULL;
> > + struct proc_fs_info *fs_info = proc_sb_info(dir->i_sb);
> > +
> > + if ((fs_info->pidonly == PROC_PIDONLY_ON) &&
> > + (current_user_ns() != &init_user_ns) &&
> > + !capable(CAP_NET_ADMIN))
> > + return net;
> >
> > rcu_read_lock();
> > task = pid_task(proc_pid(dir), PIDTYPE_PID);
>
> Hmm.
>
> I see 3 options going forward.
>
> 1) We just make PROC_PIDONLY_ON mean the net directory does not exist.
> No permission checks just always fail.
I think it's wrong. Now if someone mounts a fully visible procfs then they
can see this directory. Hiding this directory completely will change the
current behavior.
> 2) Move the permission checks into opendir/readdir and whichever
> is the appropriate method there and always allow the dentries
> to be cached.
At first I did so, but then I transferred this check to get_proc_task_net
because if this function does not return anything, then 'net' directory
will exist but will simply be empty.
This allowed us to get rid of unnecessary wrappers for opendir/lookup.
> 3) Simply cache the mounters credentials and make access to the
> net directories contingent of the permisions of the mounter of
> proc. Something like the code below.
Interesting idea. I like that :)
> static struct net *get_proc_task_net(struct inode *dir)
> {
> struct task_struct *task;
> struct nsproxy *ns;
> struct net *net = NULL;
>
> rcu_read_lock();
> task = pid_task(proc_pid(dir), PIDTYPE_PID);
> if (task != NULL) {
> task_lock(task);
> ns = task->nsproxy;
> if (ns != NULL)
> net = get_net(ns->net_ns);
> task_unlock(task);
> }
> rcu_read_unlock();
> if ((fs_info->pidonly == PROC_PIDONLY_ON) &&
Is this check necessary? I mean, isn't it worth extending this check to
other cases?
> !security_capable(fs_info->mounter_cred,
> net->user_ns, CAP_SYS_ADMIN,
> CAP_OPT_NONE)) {
> put_net(net);
> net = NULL;
> }
> return net;
> }
>
> Eric
>
--
Rgrds, legion
Cache the mounters credentials and make access to the net directories
contingent of the permissions of the mounter of proc.
Show /proc/self/net only if mounter has CAP_NET_ADMIN and if proc is
mounted with subset=pid option.
Signed-off-by: Alexey Gladkov <[email protected]>
---
fs/proc/proc_net.c | 8 ++++++++
fs/proc/root.c | 7 +++++++
include/linux/proc_fs.h | 1 +
3 files changed, 16 insertions(+)
diff --git a/fs/proc/proc_net.c b/fs/proc/proc_net.c
index dba63b2429f0..c43fc5c907db 100644
--- a/fs/proc/proc_net.c
+++ b/fs/proc/proc_net.c
@@ -26,6 +26,7 @@
#include <linux/uidgid.h>
#include <net/net_namespace.h>
#include <linux/seq_file.h>
+#include <linux/security.h>
#include "internal.h"
@@ -275,6 +276,7 @@ static struct net *get_proc_task_net(struct inode *dir)
struct task_struct *task;
struct nsproxy *ns;
struct net *net = NULL;
+ struct proc_fs_info *fs_info = proc_sb_info(dir->i_sb);
rcu_read_lock();
task = pid_task(proc_pid(dir), PIDTYPE_PID);
@@ -287,6 +289,12 @@ static struct net *get_proc_task_net(struct inode *dir)
}
rcu_read_unlock();
+ if (net && (fs_info->pidonly == PROC_PIDONLY_ON) &&
+ security_capable(fs_info->mounter_cred, net->user_ns, CAP_NET_ADMIN, CAP_OPT_NONE) < 0) {
+ put_net(net);
+ net = NULL;
+ }
+
return net;
}
diff --git a/fs/proc/root.c b/fs/proc/root.c
index c6bf74de1906..eeeda375cf85 100644
--- a/fs/proc/root.c
+++ b/fs/proc/root.c
@@ -184,6 +184,8 @@ static int proc_fill_super(struct super_block *s, struct fs_context *fc)
s->s_fs_info = fs_info;
fs_info->pid_ns = get_pid_ns(ctx->pid_ns);
+ fs_info->mounter_cred = get_cred(fc->cred);
+
proc_apply_options(s, fc, current_user_ns());
/*
@@ -219,9 +221,13 @@ static int proc_fill_super(struct super_block *s, struct fs_context *fc)
static int proc_reconfigure(struct fs_context *fc)
{
struct super_block *sb = fc->root->d_sb;
+ struct proc_fs_info *fs_info = proc_sb_info(sb);
sync_filesystem(sb);
+ put_cred(fs_info->mounter_cred);
+ fs_info->mounter_cred = get_cred(fc->cred);
+
proc_apply_options(sb, fc, current_user_ns());
return 0;
}
@@ -276,6 +282,7 @@ static void proc_kill_sb(struct super_block *sb)
kill_anon_super(sb);
put_pid_ns(fs_info->pid_ns);
+ put_cred(fs_info->mounter_cred);
kfree(fs_info);
}
diff --git a/include/linux/proc_fs.h b/include/linux/proc_fs.h
index d1eed1b43651..671c6dafc4ee 100644
--- a/include/linux/proc_fs.h
+++ b/include/linux/proc_fs.h
@@ -63,6 +63,7 @@ struct proc_fs_info {
kgid_t pid_gid;
enum proc_hidepid hide_pid;
enum proc_pidonly pidonly;
+ struct cred *mounter_cred;
};
static inline struct proc_fs_info *proc_sb_info(struct super_block *sb)
--
2.25.4