[PATCH] Fix /proc/[pid]/ns permissions

[PATCH] Fix /proc/[pid]/ns permissions

[Debabrata Banerjee]

This seems like a bug since the original commit 6b4e306aa3dc. Having ns
directory be executable but not readable does not make sense. Further,
it breaks userspace when it needs to know which namespace it belongs
to. For example, setting a process to prctl(PR_SET_DUMPABLE, 0)
immediately hides the namespace from that user, breaking the latest
pgrep with namespace support.

The main problem here is that unlike other namespaces, pid namespaces
appear flat as you follow the parents upwards in the heirarchy. It is
important to be able to tell that a process is in your namespace, a
child namespace, or an entirely different namespace. In the latter
case, the pid is already hidden from you, thus these permission don't
matter.

CC: Eric W. Biederman <ebiederm@xmission.com>
CC: Daniel Lezcano <daniel.lezcano@free.fr>

Signed-off-by: Debabrata Banerjee <dbanerje@akamai.com>
---
 fs/proc/base.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/fs/proc/base.c b/fs/proc/base.c
index d53246863cfb..2295ac0d8e1c 100644
--- a/fs/proc/base.c
+++ b/fs/proc/base.c
@@ -2922,7 +2922,7 @@ static const struct pid_entry tgid_base_stuff[] = {
 	DIR("fd",         S_IRUSR|S_IXUSR, proc_fd_inode_operations, proc_fd_operations),
 	DIR("map_files",  S_IRUSR|S_IXUSR, proc_map_files_inode_operations, proc_map_files_operations),
 	DIR("fdinfo",     S_IRUSR|S_IXUSR, proc_fdinfo_inode_operations, proc_fdinfo_operations),
-	DIR("ns",	  S_IRUSR|S_IXUGO, proc_ns_dir_inode_operations, proc_ns_dir_operations),
+	DIR("ns",	  S_IRUGO|S_IXUGO, proc_ns_dir_inode_operations, proc_ns_dir_operations),
 #ifdef CONFIG_NET
 	DIR("net",        S_IRUGO|S_IXUGO, proc_net_inode_operations, proc_net_operations),
 #endif
-- 
2.16.2

Re: [PATCH] Fix /proc/[pid]/ns permissions

[Banerjee, Debabrata]

Actually, this patch is incomplete. proc_ns_get_link() and proc_ns_readlink() gate on ptrace_may_access(task, PTRACE_MODE_READ_FSCREDS). I'm not sure why this is here either. It seems problematic that after a user creates a pid namespace, that a user cannot tell anymore which namespace new pids belongs to, including the parent namespace after prctl(). Some mechanism is required here. If ns->inum from ns_get_name() is secret, then at least a flag if it is in your namespace.

-Deb

On 4/5/18, 1:15 PM, "Debabrata Banerjee" <dbanerje@akamai.com> wrote:

    This seems like a bug since the original commit 6b4e306aa3dc. Having ns
    directory be executable but not readable does not make sense. Further,
    it breaks userspace when it needs to know which namespace it belongs
    to. For example, setting a process to prctl(PR_SET_DUMPABLE, 0)
    immediately hides the namespace from that user, breaking the latest
    pgrep with namespace support.
    
    The main problem here is that unlike other namespaces, pid namespaces
    appear flat as you follow the parents upwards in the heirarchy. It is
    important to be able to tell that a process is in your namespace, a
    child namespace, or an entirely different namespace. In the latter
    case, the pid is already hidden from you, thus these permission don't
    matter.
    
    CC: Eric W. Biederman <ebiederm@xmission.com>
    CC: Daniel Lezcano <daniel.lezcano@free.fr>
    
    Signed-off-by: Debabrata Banerjee <dbanerje@akamai.com>
    ---
     fs/proc/base.c | 2 +-
     1 file changed, 1 insertion(+), 1 deletion(-)
    
    diff --git a/fs/proc/base.c b/fs/proc/base.c
    index d53246863cfb..2295ac0d8e1c 100644
    --- a/fs/proc/base.c
    +++ b/fs/proc/base.c
    @@ -2922,7 +2922,7 @@ static const struct pid_entry tgid_base_stuff[] = {
     	DIR("fd",         S_IRUSR|S_IXUSR, proc_fd_inode_operations, proc_fd_operations),
     	DIR("map_files",  S_IRUSR|S_IXUSR, proc_map_files_inode_operations, proc_map_files_operations),
     	DIR("fdinfo",     S_IRUSR|S_IXUSR, proc_fdinfo_inode_operations, proc_fdinfo_operations),
    -	DIR("ns",	  S_IRUSR|S_IXUGO, proc_ns_dir_inode_operations, proc_ns_dir_operations),
    +	DIR("ns",	  S_IRUGO|S_IXUGO, proc_ns_dir_inode_operations, proc_ns_dir_operations),
     #ifdef CONFIG_NET
     	DIR("net",        S_IRUGO|S_IXUGO, proc_net_inode_operations, proc_net_operations),
     #endif
    -- 
    2.16.2
    
    

Re: [PATCH] Fix /proc/[pid]/ns permissions

[Eric W. Biederman]

"Banerjee, Debabrata" <dbanerje@akamai.com> writes:

> Actually, this patch is incomplete. proc_ns_get_link() and
> proc_ns_readlink() gate on ptrace_may_access(task,
> PTRACE_MODE_READ_FSCREDS).  I'm not sure why this is here either. It
> seems problematic that after a user creates a pid namespace, that a
> user cannot tell anymore which namespace new pids belongs to,
> including the parent namespace after prctl(). Some mechanism is
> required here. If ns->inum from ns_get_name() is secret, then at least
> a flag if it is in your namespace.

> On 4/5/18, 1:15 PM, "Debabrata Banerjee" <dbanerje@akamai.com> wrote:
>
>     This seems like a bug since the original commit 6b4e306aa3dc. Having ns
>     directory be executable but not readable does not make sense. Further,
>     it breaks userspace when it needs to know which namespace it belongs
>     to. For example, setting a process to prctl(PR_SET_DUMPABLE, 0)
>     immediately hides the namespace from that user, breaking the latest
>     pgrep with namespace support.
>     
>     The main problem here is that unlike other namespaces, pid namespaces
>     appear flat as you follow the parents upwards in the heirarchy. It is
>     important to be able to tell that a process is in your namespace, a
>     child namespace, or an entirely different namespace. In the latter
>     case, the pid is already hidden from you, thus these permission don't
>     matter.

I agree there is an inconsistency on the directory permissions for
the ns directory that could reasonably be fixed.

prctl(PR_SET_DUMPABLE, 0) is an interesting.  Fundamentally it is about not
letting unprivileged tasks poke about with the process, and it is the
mechanism used by setuid/setgid exec.   If you are root in the user
namespace of the task or your uid created the user namespace or an
ancestor user namespace you should be able to inspect the process.

What happens with dumpable is also going to vary a little bit depending
on how recent your kernel is.  The dumpable status was not really
namespaced until recently.

There might also be some issues from yama or other lsms that love to
limit ptrace.  Though it sounds like your issue is the dumpability.

The check "ptrace_may_access(task, PTRACE_MODE_READ_FSCREDS)" is the
standard check for accessing anything even a little bit sensitive of
a process, and that seems appropriate here.

Eric

>     
>     CC: Eric W. Biederman <ebiederm@xmission.com>
>     CC: Daniel Lezcano <daniel.lezcano@free.fr>
>     
>     Signed-off-by: Debabrata Banerjee <dbanerje@akamai.com>
>     ---
>      fs/proc/base.c | 2 +-
>      1 file changed, 1 insertion(+), 1 deletion(-)
>     
>     diff --git a/fs/proc/base.c b/fs/proc/base.c
>     index d53246863cfb..2295ac0d8e1c 100644
>     --- a/fs/proc/base.c
>     +++ b/fs/proc/base.c
>     @@ -2922,7 +2922,7 @@ static const struct pid_entry tgid_base_stuff[] = {
>      	DIR("fd",         S_IRUSR|S_IXUSR, proc_fd_inode_operations, proc_fd_operations),
>      	DIR("map_files",  S_IRUSR|S_IXUSR, proc_map_files_inode_operations, proc_map_files_operations),
>      	DIR("fdinfo",     S_IRUSR|S_IXUSR, proc_fdinfo_inode_operations, proc_fdinfo_operations),
>     -	DIR("ns",	  S_IRUSR|S_IXUGO, proc_ns_dir_inode_operations, proc_ns_dir_operations),
>     +	DIR("ns",	  S_IRUGO|S_IXUGO, proc_ns_dir_inode_operations, proc_ns_dir_operations),
>      #ifdef CONFIG_NET
>      	DIR("net",        S_IRUGO|S_IXUGO, proc_net_inode_operations, proc_net_operations),
>      #endif
>     -- 
>     2.16.2
>     
>     

RE: [PATCH] Fix /proc/[pid]/ns permissions

[Banerjee, Debabrata]

> From: Eric W. Biederman [mailto:ebiederm@xmission.com]
>
> I agree there is an inconsistency on the directory permissions for the ns
> directory that could reasonably be fixed.

So you'd recommend taking this patch as-is?

> prctl(PR_SET_DUMPABLE, 0) is an interesting.  Fundamentally it is about not
> letting unprivileged tasks poke about with the process, and it is the
> mechanism used by setuid/setgid exec.   If you are root in the user
> namespace of the task or your uid created the user namespace or an
> ancestor user namespace you should be able to inspect the process.
> 
> What happens with dumpable is also going to vary a little bit depending on
> how recent your kernel is.  The dumpable status was not really namespaced
> until recently.
> 
> There might also be some issues from yama or other lsms that love to limit
> ptrace.  Though it sounds like your issue is the dumpability.
> 
> The check "ptrace_may_access(task, PTRACE_MODE_READ_FSCREDS)" is the
> standard check for accessing anything even a little bit sensitive of a process,
> and that seems appropriate here.

Why is the namespace inode # secret? If it is because it's an inode, perhaps there is a better way to represent the namespace that is not secret? What we are trying to do is check if link points to the same place as our namespace, the contents don't matter. On the other hand it would be very convenient if pid's not in your namespace, even from a parent namespace, did not appear. That would make user tools work properly without workarounds, i.e. killall, pgrep, ps, top.