procfs mount options
Subsystems (5)
A message that is not in the archive (sent off-list or not yet received): [email protected]
Solar,
Quoted 23 lines · Solar Designer
I welcome suggestions on how to achieve the desired functionality for procfs in a non-confusing and generic way. It should support the following reasonable configuration:
/proc/PIDdirectories restricted to group proc (except for owners and root, indeed). However,/proc/cpuinfoand the like unrestricted. Here's what this looks like on Linux 2.4.x-ow:dr-xr-x--- 3 root proc 0 Jun 3 22:59 1
...dr-xr-x--- 3 syslogd proc 0 Jun 3 22:59 205 dr-xr-x--- 3 klogd proc 0 Jun 3 22:59 211...
(quoted diff) +0
-3-r--r--r-- 1 root proc 0 Jun 3 23:00 cpuinfo -r-------- 1 root proc 536743936 Jun 3 23:00 kcore -r-------- 1 root proc 0 May 5 20:36 kmsgdr-xr-x--- 5 root proc 0 Jun 3 23:00 net
...(quoted diff) +0
-2-r--r--r-- 1 root proc 0 Jun 3 23:00 uptime -r--r--r-- 1 root proc 0 Jun 3 23:00 version
Quoted 4 lines
Perhaps gid=proc,umask=007 should result in the above for
/proc/PID, but how do we justify it not affecting/proc/cpuinfo, uptime, version (and many others)? How do we justify it nevertheless affecting/proc/net(or should another option do that)?
I think it should be done with separate mount options for /proc/self/net (/proc/net is a symlink to /proc/self/net since net namespaces introduction) and for /proc/PID. All other files should be e.g. chmod'ed go= and then some white list should be chmod'ed to the relaxed perms.
Quoted 2 lines
Indeed, we could set some of these perms with chmod post-mount, but as discussed this has drawbacks.
Where its drawbacks were discussed? I cannot find anything on owl-dev. Do you mean some possible diffirences between procfs files among different kernel versions? If so, white list instead of black list should partly solve it.
Quoted 3 lines
So ideally our preferred configuration
(which will be the default on Owl) should be achievable with mount options alone.
At least for sysfs it is unreachable if we go in the current direction - umask doesn't change perms of already created files, and additional "chmod -R" is needed anyway.
Thanks,
Signature · 1 line
Vasiliy
Vasiliy,
Quoted 3 lines · Vasiliy Kulikov
I think it should be done with separate mount options for
/proc/self/net(/proc/netis a symlink to/proc/self/netsince net namespaces introduction) and for/proc/PID.
OK. What do we call these? pmask (for "p"rocesses) and nmask (for "n"etwork)? Doesn't this deviation from umask reduce the chances of the patch getting accepted?.. And is there really much reason to let a user see others' processes, but not network connections, or vice versa? (Maybe there is.)
Quoted 3 lines
All other files should be e.g.
chmod'ed go= and then some white list should be chmod'ed to the relaxed perms.
Where would this logic be implemented - hard-coded in the kernel (enabled with some mount option?) or done by a script in the userspace post-mount?
Quoted 4 lines
Quoted 2 lines
Indeed, we could set some of these perms with chmod post-mount, but as discussed this has drawbacks.
Where its drawbacks were discussed?
IIRC, you, I, and some others discussed them via Jabber.
Quoted 4 lines
I cannot find anything on
owl-dev. Do you mean some possible diffirences between procfs files among different kernel versions? If so, white list instead of black list should partly solve it.
No, I meant race conditions, which you had to deal with by mounting under a parent directory with restricted perms, then mount --move'ing. Part of the intent of us patching the kernel is to avoid having to do things like that. Also, an admin might just "mount /proc", not knowing that special userspace magic was implemented by the distro.
A possible approach is to implement mount options gid=... and restricted (name suggested by spender), where the latter would enable a hard-coded set of permissions. This is not generic, but at least it's simple, not confusing, and it allows for future changes (we may change what exactly restricted means).
Quoted 7 lines
Quoted 3 lines
So ideally our preferred configuration
(which will be the default on Owl) should be achievable with mount options alone.At least for sysfs it is unreachable if we go in the current direction - umask doesn't change perms of already created files, and additional "chmod -R" is needed anyway.
Hmm. I guess I still don't understand your umask vs. mode stuff.
Ideally, I'd have umask apply to each instance of sysfs (and other special filesystems) individually, affecting all files under that instance (both those already existing in the kernel at the time of mount and those appearing after mount). But perhaps that's not how the kernel keeps track of permissions for those filesystems currently? (Sorry, I haven't played with this since Linux 2.4, which didn't even have sysfs.)
Thanks,
Alexander
Solar,
Quoted 10 lines · Solar Designer
Quoted 3 lines · Vasiliy Kulikov
I think it should be done with separate mount options for
/proc/self/net(/proc/netis a symlink to/proc/self/netsince net namespaces introduction) and for/proc/PID.OK. What do we call these? pmask (for "p"rocesses) and nmask (for "n"etwork)? Doesn't this deviation from umask reduce the chances of the patch getting accepted?.. And is there really much reason to let a user see others' processes, but not network connections, or vice versa? (Maybe there is.)
While implementing this PID permissions restricting mount opt (don't know how to call it better), I've noticed that only 2 modes make sense - full view (current behaviour) and restricted view (similar to umask=07). So, I propose to implement & call the options as follows:
(no)safepid - instead of umask/mode; if it is on, only
owner/root/special group may read /proc/PID.
(no)hidepid - hide restricted /proc/PID from *stat*(2), getdents*()
and similar. Needs safepid.
(no)hidenet - hide /proc/PID/net contents (restricting /proc/PID/net
itself is a bit more difficult to implement).
gid - group to evade safepid,hidepid,hidenet.
Also as hidepid needs safepid, maybe it makes sense to implement one option instead of safepid and hidepid: hidepid=X where:
X = 0 means nohidepid,nosafepid above
X = 1 means safepid,nohidepid above
X = 2 means safepid,hidepid above
Quoted 7 lines
Quoted 3 lines
All other files should be e.g.
chmod'ed go= and then some white list should be chmod'ed to the relaxed perms.Where would this logic be implemented - hard-coded in the kernel (enabled with some mount option?) or done by a script in the userspace post-mount?
I'm sure if we try to implement it in kernel, we'd surely get NAK because it may be implemented much simplier in userspace. Compare:
a single chmod -R vs. some in-kernel filtering of core procfs files and driver-specific files (yes, there are rare driver files in /proc, it is blamed, but anyway).
Quoted 6 lines
Quoted 4 lines
Quoted 2 lines
Indeed, we could set some of these perms with chmod post-mount, but as discussed this has drawbacks.
Where its drawbacks were discussed?
IIRC, you, I, and some others discussed them via Jabber.
Ah, OK ;)
Quoted 9 lines
Quoted 4 lines
I cannot find anything on
owl-dev. Do you mean some possible diffirences between procfs files among different kernel versions? If so, white list instead of black list should partly solve it.No, I meant race conditions, which you had to deal with by mounting under a parent directory with restricted perms, then mount --move'ing. Part of the intent of us patching the kernel is to avoid having to do things like that.
For distros with sysv init there is no race if it is implemented in a boot script before any login/cron/etc. It's a rare situation now, though :)
Quoted 2 lines
Also, an admin might just "mount /proc", not knowing
that special userspace magic was implemented by the distro.
Agreed, but I still don't think it is a good argument for upstream...
Quoted 3 lines
A possible approach is to implement mount options gid=... and restricted (name suggested by spender), where the latter would enable a hard-coded set of permissions.
To what files? All /proc/* files or recursively? Is there a white list of o=r/o=rw files or all of them would be chmod'ed "o="?
Quoted 3 lines
This is not generic, but at least it's simple, not
confusing, and it allows for future changes (we may change what exactly restricted means).
Changing what some option means after implementing it is a ABI breakage, which is terrible for upstream. However, it might be acceptable for Owl.
Quoted 8 lines
Quoted 7 lines
Quoted 3 lines
So ideally our preferred configuration
(which will be the default on Owl) should be achievable with mount options alone.At least for sysfs it is unreachable if we go in the current direction - umask doesn't change perms of already created files, and additional "chmod -R" is needed anyway.
Quoted 5 lines
Hmm. I guess I still don't understand your umask vs. mode stuff.
Ideally, I'd have umask apply to each instance of sysfs (and other special filesystems) individually, affecting all files under that instance
What instance? All files (=kobjects) are global, there is almost no division among mount points. The only division is among net namespaces, but it is mostly the exception than the rule and there will be always files global to all namespaces (e.g. modules list, devices list, etc.)
Quoted 3 lines
(both those already existing in the kernel at the time of mount and those appearing after mount). But perhaps that's not how the kernel keeps track of permissions for those filesystems currently?
I'd not change permissions of all currently existing files. What would you do with already created and chmod'ed files?
And what you suggest is strictly speaking not "umask". "umask" is for newly created files. "mode" is almost everything judging by mount(8) :)
Currently I have only one TODO in my list for procfs - restricted /proc/PID/net returns -EINVAL instead of -ENOENT as it is done in grsecurity :) Need to make it say true.
Thanks,
Vasiliy.
Quoted 14 lines · Vasiliy Kulikov
While implementing this PID permissions restricting mount opt (don't know how to call it better), I've noticed that only 2 modes make sense - full view (current behaviour) and restricted view (similar to umask=07). So, I propose to implement & call the options as follows:
(no)safepid - instead of umask/mode; if it is on, only
owner/root/special group may read/proc/PID.(no)hidepid - hide restricted
/proc/PIDfrom *stat*(2), getdents*()
and similar. Needs safepid.(no)hidenet - hide
/proc/PID/netcontents (restricting/proc/PID/net
itself is a bit more difficult to implement).
Quoted 8 lines
gid - group to evade safepid,hidepid,hidenet.
Also as hidepid needs safepid, maybe it makes sense to implement one option instead of safepid and hidepid: hidepid=X where:
X = 0 means nohidepid,nosafepid above
X = 1 means safepid,nohidepid above
X = 2 means safepid,hidepid above
This makes sense to me, and I like the tri-state option better.
Quoted 7 lines · Solar Designer
Quoted 3 lines
A possible approach is to implement mount options gid=... and restricted (name suggested by spender), where the latter would enable a hard-coded set of permissions.
To what files? All
/proc/*files or recursively? Is there a white list of o=r/o=rw files or all of them would be chmod'ed "o="?
Oh, that's not what I was referring to. The restricted option would essentially be the same as your hidepid=2 - enabling whatever grsecurity normally does. But having a tri-state option as you propose makes more sense to me.
Quoted 6 lines
Quoted 3 lines
This is not generic, but at least it's simple, not confusing, and it allows for future changes (we may change what exactly restricted means).
Changing what some option means after implementing it is a ABI breakage, which is terrible for upstream.
This depends on how the option is defined initially. It is possible to define the option as "enabling unspecified version-dependent restrictions, which are subject to change across kernel versions and builds". ;-) OK, the word "unspecified" may be dropped.
Quoted 9 lines
Quoted 3 lines
Ideally, I'd have umask apply to each instance of sysfs (and other special filesystems) individually, affecting all files under that instance
What instance? All files (=kobjects) are global, there is almost no division among mount points. The only division is among net namespaces, but it is mostly the exception than the rule and there will be always files global to all namespaces (e.g. modules list, devices list, etc.)
Quoted 6 lines
Quoted 3 lines
(both those already existing in the kernel at the time of mount and those appearing after mount). But perhaps that's not how the kernel keeps track of permissions for those filesystems currently?
I'd not change permissions of all currently existing files. What would you do with already created and chmod'ed files?
The reality (kobjects being global) doesn't quite match my desired behavior (being able to mount more than one instance of procfs, etc. and set their mount options affecting perms differently). However, this is not important enough to warrant changing the reality. ;-)
Here's a related thought: if these mount options happen to affect all instances of the filesystem (in the same container), maybe they should be sysctl's instead?
Here's another thought: if the gid is not specified but hidepid is enabled, maybe the perms should be 700 rather than 750 (don't grant any privs to group root).
Quoted 2 lines
And what you suggest is strictly speaking not "umask". "umask" is for newly created files. "mode" is almost everything judging by mount(8) :)
mount(8) mentions both "umask" and "mode" for different filesystems, with slightly different semantics.
Quoted 3 lines
Currently I have only one TODO in my list for procfs - restricted
/proc/PID/netreturns-EINVALinstead of-ENOENTas it is done in grsecurity :) Need to make it say true.
Do you mean you have already implemented the mount options as you have proposed above?
Thanks,
Alexander
RFC 1/1 procfs mount options
This patch introduces support of procfs mount options and adds mount options to restrict access to /proc/PID/ directories and /proc/PID/net/ contents. The default backward-compatible behaviour is left untouched.
The first mount option is called "hidepid" and its value defines how much info about processes we want to be available for non-owners:
hidepid=0 (default) means the current behaviour - anybody may read all /proc/PID/* files.
hidepid=1 means all files not running as current user and group are unaccessible (directories' permissions are 0550). Sensitive files like cmdline, io, sched*, status, wchan are now protected against other users.
hidepid=2 means hidepid=1 plus all /proc/PID/ will be invisible to other users. It doesn't mean that it hides a fact whether a process exists (it can be learned by other means, e.g. by sending signals), but it hides process' uid and gid. It greatly compicates intruder's task of gathering info about running processes, whether some daemon runs with elevated privileges, whether other user runs some sensitive program, whether other users run any program at all, etc.
hidenet means /proc/PID/net will be accessible to processes with CAP_NET_ADMIN capability or to members of a special group.
gid=XXX defines a group that will be able to gather all processes' info and network connections info.
TODO/thoughs:
- /proc/pid/net/ currently doesn't show ANYTHING, even "." and "..". This is confusing :)
- copy options description to Documenataion/filesystems/procfs.txt
- need to alter "(inode->i_mode == (S_IFDIR|S_IRUGO|S_IXUGO))" checks to honour non-pid directories.
- what if one keeps open /proc/PID/ while executing set*id/capable binary?
- maybe hidenet belongs to net namespace, not pid? However, it is seen through procfs, which is per-pid_namespace.
fs/proc/base.c +57 -9
@@ -1688,6 +1688,7 @@ static struct inode *proc_pid_make_inode(struct super_block * sb, struct task_ststruct inode * inode;struct proc_inode *ei;const struct cred *cred; +struct nsproxy *ns;/* We need a new inode */
@@ -1712,7 +1713,12 @@ static struct inode *proc_pid_make_inode(struct super_block * sb, struct task_strcu_read_lock (); cred =__task_cred (task); inode->i_uid = cred->euid; - inode->i_gid = cred->egid; + ns =task_nsproxy (task); +if (ns && ns->pid_ns->hide_pid) { + inode->i_gid = ns->pid_ns->pid_gid; + }else { + inode->i_gid = cred->egid; + }rcu_read_unlock (); }security_task_to_inode (task, inode);
@@ -1725,11 +1731,22 @@ out_unlock:return NULL ; } +/* TODO: ugly, need something more generic */ +static bool proc_pid_may_getattr (struct pid_namespace * pid,struct task_struct *task) +{ +if (pid->hide_pid <2 ) +return true; +if (ptrace_may_access (task,PTRACE_MODE_READ )) +return true; +return in_group_p (pid->pid_gid); +} +static int pid_getattr (struct vfsmount *mnt,struct dentry *dentry,struct kstat *stat) {struct inode *inode = dentry->d_inode;struct task_struct *task;const struct cred *cred; +struct pid_namespace *pid = dentry->d_sb->s_fs_info;generic_fillattr (inode, stat);
@@ -1738,11 +1755,17 @@ static int pid_getattr(struct vfsmount *mnt, struct dentry *dentry, struct kstat stat->gid =0 ; task =pid_task (proc_pid (inode),PIDTYPE_PID );if (task) { -if ((inode->i_mode == (S_IFDIR |S_IRUGO |S_IXUGO )) || -task_dumpable (task)) { +/* TODO: if ((inode->i_mode == (S_IFDIR|S_IRUGO|S_IXUGO)) */ +if ((((inode->i_mode &S_IFDIR ) ==S_IFDIR ) || +task_dumpable (task))) { cred =__task_cred (task); - stat->uid = cred->euid; - stat->gid = cred->egid; +if (proc_pid_may_getattr (pid, task)) { + stat->uid = cred->euid; +if (pid->hide_pid) + stat->gid = pid->pid_gid; +else + stat->gid = cred->egid; + } } }rcu_read_unlock ();
@@ -1771,6 +1794,7 @@ static int pid_revalidate(struct dentry *dentry, struct nameidata *nd)struct inode *inode;struct task_struct *task;const struct cred *cred; +struct pid_namespace *pid = dentry->d_sb->s_fs_info;if (nd && nd->flags &LOOKUP_RCU )return -ECHILD ;
@@ -1779,12 +1803,16 @@ static int pid_revalidate(struct dentry *dentry, struct nameidata *nd) task =get_proc_task (inode);if (task) { -if ((inode->i_mode == (S_IFDIR |S_IRUGO |S_IXUGO )) || +/* TODO: if ((inode->i_mode == (S_IFDIR|S_IRUGO|S_IXUGO)) */ +if (((inode->i_mode &S_IFDIR ) ==S_IFDIR ) ||task_dumpable (task)) {rcu_read_lock (); cred =__task_cred (task); inode->i_uid = cred->euid; - inode->i_gid = cred->egid; +if (pid->hide_pid) + inode->i_gid = pid->pid_gid; +else + inode->i_gid = cred->egid;rcu_read_unlock (); }else { inode->i_uid =0 ;
@@ -2986,12 +3014,21 @@ static struct dentry *proc_pid_instantiate(struct inode *dir, {struct dentry *error =ERR_PTR (-ENOENT );struct inode *inode; +struct nsproxy *ns; +unsigned int mode; inode =proc_pid_make_inode (dir->i_sb, task);if (!inode) goto out; - inode->i_mode =S_IFDIR |S_IRUGO |S_IXUGO ; +rcu_read_lock (); + ns =task_nsproxy (task); +if (ns && ns->pid_ns->hide_pid) + mode =07 ; +else + mode =0 ; +rcu_read_unlock (); + inode->i_mode =S_IFDIR | ((S_IRUGO |S_IXUGO ) & ~mode); inode->i_op = &proc_tgid_base_inode_operations; inode->i_fop = &proc_tgid_base_operations; inode->i_flags|=S_IMMUTABLE ;
@@ -3093,6 +3130,11 @@ static int proc_pid_fill_cache(struct file *filp, void *dirent, filldir_t filldi proc_pid_instantiate, iter.task, NULL); } +static int fake_filldir (void *buf,const char *name,int namelen,loff_t offset,u64 ino,unsigned d_type) +{ +return 0 ; +} +/* for the /proc/ directory itself, after non-process stuff has been done */ int proc_pid_readdir (struct file * filp,void * dirent,filldir_t filldir) {
@@ -3100,6 +3142,7 @@ int proc_pid_readdir(struct file * filp, void * dirent, filldir_t filldir)struct task_struct *reaper =get_proc_task (filp->f_path.dentry->d_inode);struct tgid_iter iter;struct pid_namespace *ns; +filldir_t __filldir;if (!reaper) goto out_no_task;
@@ -3116,8 +3159,13 @@ int proc_pid_readdir(struct file * filp, void * dirent, filldir_t filldir)for (iter =next_tgid (ns, iter); iter.task; iter.tgid +=1 , iter =next_tgid (ns, iter)) { +if (proc_pid_may_getattr (ns, iter.task)) + __filldir = filldir; +else + __filldir = fake_filldir; + filp->f_pos = iter.tgid +TGID_OFFSET ; -if (proc_pid_fill_cache (filp, dirent, filldir, iter) <0 ) { +if (proc_pid_fill_cache (filp, dirent, __filldir, iter) <0 ) {put_task_struct (iter.task); goto out; }
fs/proc/inode.c +21 -1
@@ -7,6 +7,7 @@#include <linux/time.h> #include <linux/proc_fs.h> #include <linux/kernel.h> +#include <linux/pid_namespace.h> #include <linux/mm.h> #include <linux/string.h> #include <linux/stat.h>
@@ -17,7 +18,9 @@#include <linux/init.h> #include <linux/module.h> #include <linux/sysctl.h> +#include <linux/seq_file.h> #include <linux/slab.h> +#include <linux/mount.h> #include <asm/system.h> #include <asm/uaccess.h>
@@ -93,12 +96,29 @@ void __init proc_init_inodecache(void) init_once); } +static int proc_show_options (struct seq_file *seq,struct vfsmount *vfs) +{ +struct super_block *sb = vfs->mnt_sb; +struct pid_namespace *pid = sb->s_fs_info; + +if (pid->pid_gid) +seq_printf (seq,",gid=%lu" , (unsigned long )pid->pid_gid); +if (pid->hide_pid !=0 ) +seq_printf (seq,",hidepid=%u" , pid->hide_pid); +if (pid->hide_net) +seq_printf (seq,",hidenet" ); + +return 0 ; +} +static const struct super_operations proc_sops = { .alloc_inode = proc_alloc_inode, .destroy_inode = proc_destroy_inode, .drop_inode = generic_delete_inode, .evict_inode = proc_evict_inode, .statfs = simple_statfs, + .remount_fs = proc_remount, + .show_options = proc_show_options, };static void __pde_users_dec (struct proc_dir_entry *pde)
@@ -468,7 +488,7 @@ int proc_fill_super(struct super_block *s) s->s_magic =PROC_SUPER_MAGIC ; s->s_op = &proc_sops; s->s_time_gran =1 ; - +pde_get (&proc_root); root_inode =proc_get_inode (s, &proc_root);if (!root_inode)
fs/proc/internal.h +1 -0
@@ -110,6 +110,7 @@ void pde_put(struct proc_dir_entry *pde);extern struct vfsmount *proc_mnt;int proc_fill_super (struct super_block *);struct inode *proc_get_inode (struct super_block *,struct proc_dir_entry *); +int proc_remount (struct super_block *sb,int *flags,char *data);/* * These are generic /proc routines that use the internal
fs/proc/proc_net.c +7 -1
@@ -22,6 +22,7 @@#include <linux/mount.h> #include <linux/nsproxy.h> #include <net/net_namespace.h> +#include <linux/pid_namespace.h> #include <linux/seq_file.h> #include "internal.h"
@@ -105,6 +106,11 @@ static struct net *get_proc_task_net(struct inode *dir)struct task_struct *task;struct nsproxy *ns;struct net *net =NULL ; +struct pid_namespace *pid = dir->i_sb->s_fs_info; + +if (pid->hide_net && + (!capable (CAP_NET_ADMIN ) && !in_group_p (pid->pid_gid))) +return net;rcu_read_lock (); task =pid_task (proc_pid (dir),PIDTYPE_PID );
@@ -162,7 +168,7 @@ static int proc_tgid_net_readdir(struct file *filp, void *dirent,int ret;struct net *net; - ret = -EINVAL ; + ret = -ENOENT ; net =get_proc_task_net (filp->f_path.dentry->d_inode);if (net !=NULL ) { ret =proc_readdir_de (net->proc_net, filp, dirent, filldir);
fs/proc/root.c +80 -2
114 lines not on this page · raw message
include/linux/pid_namespace.h +3 -0
@@ -30,6 +30,9 @@ struct pid_namespace {#ifdef CONFIG_BSD_PROCESS_ACCT struct bsd_acct_struct *bacct;#endif +gid_t pid_gid; +int hide_pid; +bool hide_net; };extern struct pid_namespace init_pid_ns;
--
Vasiliy, all -
To comment on this in detail (such as on specific source code changes), I'd need to dive into it myself, effectively duplicating Vasiliy's work. So I am not doing that, but I'd appreciate it if others in here review and comment (preferably, folks with more up-to-date experience of patching these areas of the kernel - such as after the introduction of namespaces; my own experience is too rusty). I think it's going to be similar for other tasks Vasiliy will be working on under this project - I am happy to provide overall guidance, but I am too rusty on many of the details involved and I am not going to have time to (re-)learn them this summer. So any help is appreciated.
Quoted 9 lines · Vasiliy Kulikov
This patch introduces support of procfs mount options and adds mount options to restrict access to
/proc/PID/directories and/proc/PID/net/contents. The default backward-compatible behaviour is left untouched.The first mount option is called "hidepid" and its value defines how much info about processes we want to be available for non-owners:
hidepid=0 (default) means the current behaviour - anybody may read all
/proc/PID/*files.
Aren't some /prod/PID/* files restricted by default, in stock kernels? I think several are (auxv, fd/, mem). So perhaps re-word this.
Quoted 1 line
hidepid=1 means all files not running as current user and group are
"... files not running ..." needs to be re-worded.
Quoted 3 lines
TODO/thoughs:
-/proc/pid/net/currently doesn't show ANYTHING, even "." and "..". This is confusing :)
Ouch. Can't you simply restrict its perms such that this directory can't be listed unless you have privs? It should act as a normal mode 550 directory on a regular filesystem.
Quoted 1 line
- copy options description to
Documenataion/filesystems/procfs.txt
Yes.
Quoted 2 lines
- need to alter "(
inode->i_mode== (S_IFDIR|S_IRUGO|S_IXUGO))" checks to honour non-pid directories.
I see this in the patch, but can't really comment without reviewing the code in full context myself.
Quoted 2 lines
- what if one keeps open
/proc/PID/while executing set*id/capable binary?
Then they deliberately grant this privilege to this process (and maybe to its heirs). I see no problem with that.
However, if a set*id/capable binary can be tricked into opening /proc/PID/ or a /proc/PID/* entry on its own, and would leave the fd open for whatever other program(s) it invokes, then we have a problem. But that should probably be regarded as an issue of that specific program, unless the underlying cause enabling the specific attack is in the kernel or in a library. I find it unlikely that an issue like that would be specific to just /proc/PID; it'd probably be usable on other restricted-perms directories and/or files as well.
Quoted 2 lines
- maybe hidenet belongs to net namespace, not pid? However, it is seen through procfs, which is per-pid_namespace.
I am not the right person to comment on that.
fs/proc/proc_net.c static struct net *get_proc_task_net(struct inode *dir) · line 112
Quoted 2 lines
fs/proc/proc_net.c +2
-0+if (pid->hide_net && + (!capable (CAP_NET_ADMIN ) && !in_group_p (pid->pid_gid)))
capable() sets a flag when it makes use of the capability (or at least it used to), visible via pacct. So, unless anything has changed in this area, it is best to check capable() last, such that it's only reached when it actually makes a difference. Thus, I'd write:
if (pid->hide_net && !in_group_p(pid->pid_gid) && !capable(CAP_NET_ADMIN))
Also, what did you mean by the extra braces? Just separating the setting check from the permissions check for readability? I don't think this helps.
Thanks,
Alexander
Solar,
Quoted 5 lines · Solar Designer
Quoted 4 lines · Vasiliy Kulikov
This patch introduces support of procfs mount options and adds mount options to restrict access to
/proc/PID/directories and/proc/PID/net/contents. The default backward-compatible behaviour is left untouched.
Quoted 8 lines
Quoted 5 lines
The first mount option is called "hidepid" and its value defines how much info about processes we want to be available for non-owners:
hidepid=0 (default) means the current behaviour - anybody may read all
/proc/PID/*files.Aren't some /prod/PID/* files restricted by default, in stock kernels? I think several are (auxv, fd/, mem). So perhaps re-word this.
Yes, I've mentioned still accessible files in hidepid=1. Will do.
Quoted 3 lines
Quoted 1 line
hidepid=1 means all files not running as current user and group are
"... files not running ..." needs to be re-worded.
Oops, made a mistake while rewording.
Quoted 6 lines
Quoted 3 lines
TODO/thoughs:
-/proc/pid/net/currently doesn't show ANYTHING, even "." and "..". This is confusing :)Ouch. Can't you simply restrict its perms such that this directory can't be listed unless you have privs?
Well, yes, but it would touch too many code - currently it is handled as an entry in a static table. Changing this would touch many high level loops of the table handling. Hiding its contents is just simplier.
Another solution - create a fake net namespace and process this namespace if not enough permissions :) It also removes weird netstat errors like "seems like networking was disabled for this kernel".
Quoted 2 lines
It should act as a normal mode
550 directory on a regular filesystem.
What 550 perms would give? /proc/pid/net/ contains all network information about _all_ network connections in current net namespace. So, /proc/1/net and /proc/2/net are logically the same directory.
However, changing the mode to 550 _and_ changing uid and gid will help.
Quoted 5 lines
Quoted 2 lines
- need to alter "(
inode->i_mode== (S_IFDIR|S_IRUGO|S_IXUGO))" checks to honour non-pid directories.I see this in the patch, but can't really comment without reviewing the code in full context myself.
As Spender commented, this is a weird thing to recognize pids from other files, which is needed due to some weird caching :)
Quoted 5 lines
Quoted 2 lines
- what if one keeps open
/proc/PID/while executing set*id/capable binary?Then they deliberately grant this privilege to this process (and maybe to its heirs). I see no problem with that.
Ehh... I mean another thing:
Process A with UID=1000 opens /proc/123/, while 123 has UID=1000.
123 exec's setuid binary, /proc/123/ becomes unaccessible to A.
However, A still keeps the directory opened and may read its contents.
Quoted 10 lines
fs/proc/proc_net.c static struct net *get_proc_task_net(struct inode *dir) · line 112
Quoted 2 lines
fs/proc/proc_net.c +2
-0+if (pid->hide_net && + (!capable (CAP_NET_ADMIN ) && !in_group_p (pid->pid_gid)))
capable()sets a flag when it makes use of the capability (or at least it used to), visible via pacct. So, unless anything has changed in this area, it is best to checkcapable()last, such that it's only reached when it actually makes a difference. Thus, I'd write:if (pid->hide_net && !in_group_p(pid->pid_gid) && !capable(CAP_NET_ADMIN))
OK.
Quoted 2 lines
Also, what did you mean by the extra braces? Just separating the setting check from the permissions check for readability?
Initially I wrote some ||, so the braces were needed. Will remove.
Thanks you for the comments, will fix and repost,
Signature · 1 line
Vasiliy
Quoted 9 lines · Vasiliy Kulikov
Quoted 7 lines · Solar Designer
Quoted 3 lines · Vasiliy Kulikov
TODO/thoughs:
-/proc/pid/net/currently doesn't show ANYTHING, even "." and "..". This is confusing :)Ouch. Can't you simply restrict its perms such that this directory can't be listed unless you have privs?
Quoted 7 lines
Well, yes, but it would touch too many code - currently it is handled as an entry in a static table. Changing this would touch many high level loops of the table handling. Hiding its contents is just simplier.
Another solution - create a fake net namespace and process this namespace if not enough permissions :) It also removes weird netstat errors like "seems like networking was disabled for this kernel".
This sounds good to me, although I am not familiar with namespaces.
Quoted 7 lines
Quoted 1 line
It should act as a normal mode 550 directory on a regular filesystem.
What 550 perms would give?
/proc/pid/net/contains all network information about _all_ network connections in current net namespace. So,/proc/1/netand/proc/2/netare logically the same directory.However, changing the mode to 550 _and_ changing uid and gid will help.
I implied that the gid would be the one set by our gid option.
Quoted 6 lines
Quoted 5 lines
Quoted 2 lines
- what if one keeps open
/proc/PID/while executing set*id/capable binary?Then they deliberately grant this privilege to this process (and maybe to its heirs). I see no problem with that.
Quoted 7 lines
Ehh... I mean another thing:
Process A with UID=1000 opens
/proc/123/, while 123 has UID=1000.123 exec's setuid binary,
/proc/123/becomes unaccessible to A.However, A still keeps the directory opened and may read its contents.
Oh, this is a valid concern. Please research this. Perhaps there should be a may-ptrace check (or maybe more than one).
As it relates to privacy, there's little need for this, because the user started those processes on their own (in fact, this is a reason why the user may want to be able to see those processes, despite of procfs perms).
However, if our concern is that the processes may leak sensitive data the user is not supposed to see, then we need to restrict this.
Overall, it makes sense either to relax things such that /proc/PID have real UID rather than effective UID as their owner (so we make such access to almost-own processes legitimate) or to have the proper may-ptrace checks in place (such that permissions are actually enforced). Maybe this should be another mount option. But this feels like too much complexity for an obscure option that few people will understand and use.
Thanks,
Alexander
Quoted 8 lines · Solar Designer
Quoted 5 lines
Process A with UID=1000 opens
/proc/123/, while 123 has UID=1000.123 exec's setuid binary,
/proc/123/becomes unaccessible to A.However, A still keeps the directory opened and may read its contents.
Oh, this is a valid concern. Please research this. Perhaps there should be a may-ptrace check (or maybe more than one).
This is similar to CVE-2011-1020:
https://lkml.org/lkml/2011/2/7/368
http://seclists.org/fulldisclosure/2011/Jan/421
The proposed solution for separate procfs files is implementing additional runtime checks (besides POSIX perms), however, it probably doesn't scale for the whole PID directory.
Will try to invent some simple way to deal with it.
Signature · 1 line
Vasiliy
Quoted 10 lines · Vasiliy Kulikov
Quoted 8 lines · Solar Designer
Quoted 5 lines
Process A with UID=1000 opens
/proc/123/, while 123 has UID=1000.123 exec's setuid binary,
/proc/123/becomes unaccessible to A.However, A still keeps the directory opened and may read its contents.
Oh, this is a valid concern. Please research this. Perhaps there should be a may-ptrace check (or maybe more than one).
Quoted 7 lines
This is similar to CVE-2011-1020:
https://lkml.org/lkml/2011/2/7/368
http://seclists.org/fulldisclosure/2011/Jan/421The proposed solution for separate procfs files is implementing additional runtime checks (besides POSIX perms),
Right. IIRC, the approach with selectively adding may-ptrace checks to sensitive proc files dates back to Linux 2.0. Apparently, some newer entries such as auxv were not covered by it (there's no /proc/PID/auxv on 2.4).
Quoted 3 lines
however, it probably doesn't scale for the whole PID directory.
Will try to invent some simple way to deal with it.
Maybe have a may-ptrace check automatically applied to all entries that are not world-readable?
...Here's a trickier attack: open a sensitive /proc/PID/something file on fd 0, invoke a SUID/SGID/fscap'ed program. Depending on that program's functionality, it might read its stdin and it might report what it read from there (e.g., quote it in an error message complaining about invalid input data). I don't recall if this has been dealt with in any way or not.
While these are good to research and fix, I am concerned about the scope creep. We might prefer to get a basic procfs mount options patch in first, then propose these enhancements.
Thanks,
Alexander
RFC v2 1/1 procfs mount options
In this version I've completely discarded grsecurity way to chech permissions. Implementing customized inode_operations->permission() is MUCH simplier. The problem with old way, changing gid, is not backward-compatible - procfs files have tricky owners depending on their permissions, and some programs depend on perms AND uid/gid. So, in this version I don't actually change gid, but match with this gid in runtime (proc_pid_permission()).
Additionally it solves the problem with suid binaries, but only for operations that use permissions checks like readdir(), open(), but not read(), write(), etc. *stat*() are denied too. The generic problem is not solved, though.
This patch is as much compatible with the current behaviour as possible. It leaves permissions and changes runtime behaviour only. Nothing should be broken.
The only problem with the patch is using net namespaces. If CONFIG_NET_NS=n then fake_net is created, but it is blank. So, in this case /proc/PID/net doesn't contain common files and netstat and other programs might spit with warnings.
I think this version of the patch is ready for LKML review.
fs/proc/base.c +58 -1
@@ -568,8 +568,38 @@ static int proc_setattr(struct dentry *dentry, struct iattr *attr)return 0 ; } +static int proc_pid_permission (struct inode *inode,int mask,unsigned int flags) +{ +struct pid_namespace *pid = inode->i_sb->s_fs_info; +struct task_struct *task =get_proc_task (inode); + +if (pid->hide_pid && + !ptrace_may_access (task,PTRACE_MODE_READ ) && + !in_group_p (pid->pid_gid)) { +if (pid->hide_pid ==2 ) +return -ENOENT ; +else +return -EPERM ; + } +return generic_permission (inode, mask, flags,NULL ); +} + +/* +* May current process learn task's euid/egid? +*/ +static bool proc_pid_may_getattr (struct pid_namespace * pid,struct task_struct *task) +{ +if (pid->hide_pid <2 ) +return true; +if (ptrace_may_access (task,PTRACE_MODE_READ )) +return true; +return in_group_p (pid->pid_gid); +} + +static const struct inode_operations proc_def_inode_operations = { .setattr = proc_setattr, + .permission = proc_pid_permission, }; static int mounts_open_common(struct inode *inode, struct file *file,
@@ -1662,6 +1692,7 @@ static const struct inode_operations proc_pid_link_inode_operations = { .readlink = proc_pid_readlink, .follow_link = proc_pid_follow_link, .setattr = proc_setattr, + .permission = proc_pid_permission, };
@@ -1730,6 +1761,7 @@ static int pid_getattr(struct vfsmount *mnt, struct dentry *dentry, struct kstatstruct inode *inode = dentry->d_inode;struct task_struct *task;const struct cred *cred; +struct pid_namespace *pid = dentry->d_sb->s_fs_info;generic_fillattr (inode, stat);
@@ -1738,6 +1770,14 @@ static int pid_getattr(struct vfsmount *mnt, struct dentry *dentry, struct kstat stat->gid =0 ; task =pid_task (proc_pid (inode),PIDTYPE_PID );if (task) { +if (!proc_pid_may_getattr (pid, task)) { +rcu_read_unlock (); +/* +* This doesn't prevent learning whether PID exists, +* it only makes getattr() consistent with readdir(). +*/ +return -ENOENT ; + }if ((inode->i_mode == (S_IFDIR |S_IRUGO |S_IXUGO )) ||task_dumpable (task)) { cred =__task_cred (task);
@@ -2184,6 +2224,7 @@ static const struct inode_operations proc_fd_inode_operations = { .lookup = proc_lookupfd, .permission = proc_fd_permission, .setattr = proc_setattr, + .permission = proc_pid_permission, }; static struct dentry *proc_fdinfo_instantiate(struct inode *dir,
@@ -2236,6 +2277,7 @@ static const struct file_operations proc_fdinfo_operations = {static const struct inode_operations proc_fdinfo_inode_operations = { .lookup = proc_lookupfdinfo, .setattr = proc_setattr, + .permission = proc_pid_permission, };
@@ -2473,6 +2515,7 @@ static const struct inode_operations proc_attr_dir_inode_operations = { .lookup = proc_attr_dir_lookup, .getattr = pid_getattr, .setattr = proc_setattr, + .permission = proc_pid_permission, };#endif
@@ -2890,6 +2933,7 @@ static const struct inode_operations proc_tgid_base_inode_operations = { .lookup = proc_tgid_base_lookup, .getattr = pid_getattr, .setattr = proc_setattr, + .permission = proc_pid_permission, };static void proc_flush_task_mnt (struct vfsmount *mnt,pid_t pid,pid_t tgid)
@@ -3093,6 +3137,11 @@ static int proc_pid_fill_cache(struct file *filp, void *dirent, filldir_t filldi proc_pid_instantiate, iter.task, NULL); } +static int fake_filldir (void *buf,const char *name,int namelen,loff_t offset,u64 ino,unsigned d_type) +{ +return 0 ; +} +/* for the /proc/ directory itself, after non-process stuff has been done */ int proc_pid_readdir (struct file * filp,void * dirent,filldir_t filldir) {
@@ -3100,6 +3149,7 @@ int proc_pid_readdir(struct file * filp, void * dirent, filldir_t filldir)struct task_struct *reaper =get_proc_task (filp->f_path.dentry->d_inode);struct tgid_iter iter;struct pid_namespace *ns; +filldir_t __filldir;if (!reaper) goto out_no_task;
@@ -3116,8 +3166,13 @@ int proc_pid_readdir(struct file * filp, void * dirent, filldir_t filldir)for (iter =next_tgid (ns, iter); iter.task; iter.tgid +=1 , iter =next_tgid (ns, iter)) { +if (proc_pid_may_getattr (ns, iter.task)) + __filldir = filldir; +else + __filldir = fake_filldir; + filp->f_pos = iter.tgid +TGID_OFFSET ; -if (proc_pid_fill_cache (filp, dirent, filldir, iter) <0 ) { +if (proc_pid_fill_cache (filp, dirent, __filldir, iter) <0 ) {put_task_struct (iter.task); goto out; }
@@ -3223,6 +3278,7 @@ static const struct inode_operations proc_tid_base_inode_operations = { .lookup = proc_tid_base_lookup, .getattr = pid_getattr, .setattr = proc_setattr, + .permission = proc_pid_permission, }; static struct dentry *proc_task_instantiate(struct inode *dir,
@@ -3448,6 +3504,7 @@ static const struct inode_operations proc_task_inode_operations = { .lookup = proc_task_lookup, .getattr = proc_task_getattr, .setattr = proc_setattr, + .permission = proc_pid_permission, };static const struct file_operations proc_task_operations = {
fs/proc/inode.c +22 -1
@@ -7,6 +7,7 @@#include <linux/time.h> #include <linux/proc_fs.h> #include <linux/kernel.h> +#include <linux/pid_namespace.h> #include <linux/mm.h> #include <linux/string.h> #include <linux/stat.h>
@@ -17,7 +18,9 @@#include <linux/init.h> #include <linux/module.h> #include <linux/sysctl.h> +#include <linux/seq_file.h> #include <linux/slab.h> +#include <linux/mount.h> #include <asm/system.h> #include <asm/uaccess.h>
@@ -93,12 +96,29 @@ void __init proc_init_inodecache(void) init_once); } +static int proc_show_options (struct seq_file *seq,struct vfsmount *vfs) +{ +struct super_block *sb = vfs->mnt_sb; +struct pid_namespace *pid = sb->s_fs_info; + +if (pid->pid_gid) +seq_printf (seq,",gid=%lu" , (unsigned long )pid->pid_gid); +if (pid->hide_pid !=0 ) +seq_printf (seq,",hidepid=%u" , pid->hide_pid); +if (pid->hide_net) +seq_printf (seq,",hidenet" ); + +return 0 ; +} +static const struct super_operations proc_sops = { .alloc_inode = proc_alloc_inode, .destroy_inode = proc_destroy_inode, .drop_inode = generic_delete_inode, .evict_inode = proc_evict_inode, .statfs = simple_statfs, + .remount_fs = proc_remount, + .show_options = proc_show_options, };static void __pde_users_dec (struct proc_dir_entry *pde)
@@ -423,6 +443,7 @@ struct inode *proc_get_inode(struct super_block *sb, struct proc_dir_entry *de) inode =iget_locked (sb, de->low_ino);if (!inode)return NULL ; +if (inode->i_state &I_NEW ) { inode->i_mtime = inode->i_atime = inode->i_ctime =CURRENT_TIME ;PROC_I (inode)->fd =0 ;
@@ -468,7 +489,7 @@ int proc_fill_super(struct super_block *s) s->s_magic =PROC_SUPER_MAGIC ; s->s_op = &proc_sops; s->s_time_gran =1 ; - +pde_get (&proc_root); root_inode =proc_get_inode (s, &proc_root);if (!root_inode)
fs/proc/internal.h +1 -0
@@ -110,6 +110,7 @@ void pde_put(struct proc_dir_entry *pde);extern struct vfsmount *proc_mnt;int proc_fill_super (struct super_block *);struct inode *proc_get_inode (struct super_block *,struct proc_dir_entry *); +int proc_remount (struct super_block *sb,int *flags,char *data);/* * These are generic /proc routines that use the internal
fs/proc/proc_net.c +23 -1
@@ -22,10 +22,13 @@#include <linux/mount.h> #include <linux/nsproxy.h> #include <net/net_namespace.h> +#include <linux/pid_namespace.h> #include <linux/seq_file.h> #include "internal.h" +static struct net *fake_net; +static struct net *get_proc_net (const struct inode *inode) {
@@ -105,6 +108,13 @@ static struct net *get_proc_task_net(struct inode *dir)struct task_struct *task;struct nsproxy *ns;struct net *net =NULL ; +struct pid_namespace *pid = dir->i_sb->s_fs_info; + +if (pid->hide_net && + (!capable (CAP_NET_ADMIN ) && !in_group_p (pid->pid_gid))) { +pr_err ("return fake_net = %p\n" , fake_net); +return get_net (fake_net); + }rcu_read_lock (); task =pid_task (proc_pid (dir),PIDTYPE_PID );
@@ -236,6 +246,18 @@ static struct pernet_operations __net_initdata proc_net_ns_ops = {int __init proc_net_init (void ) {proc_symlink ("net" ,NULL ,"self/net" ); -return register_pernet_subsys (&proc_net_ns_ops); } + +int __init proc_net_initcall (void ) +{ + fake_net =net_create (); +pr_err ("fake_net = %p\n" , fake_net); +if (fake_net ==NULL ) +return -ENOMEM ; + +get_net (fake_net); +return 0 ; +} + +late_initcall (proc_net_initcall);
fs/proc/root.c +80 -2
114 lines not on this page · raw message
include/linux/pid_namespace.h +3 -0
@@ -30,6 +30,9 @@ struct pid_namespace {#ifdef CONFIG_BSD_PROCESS_ACCT struct bsd_acct_struct *bacct;#endif +gid_t pid_gid; +int hide_pid; +bool hide_net; };extern struct pid_namespace init_pid_ns;
include/net/net_namespace.h +2 -0
@@ -113,6 +113,8 @@ static inline struct net *copy_net_ns(unsigned long flags, struct net *net_ns) }#endif /* CONFIG_NET */ +extern struct net *net_create (void ); +extern struct list_head net_namespace_list;
net/core/net_namespace.c +60 -1
79 lines not on this page · raw message
--
Quoted 1 line · Vasiliy Kulikov
I think this version of the patch is ready for LKML review.
I mean the patch is OK except net part, that's why LKML is needed now :)
Signature · 2 lines
Vasiliy Kulikov http://www.openwall.com - bringing security into open computing environments
Vasiliy,
Quoted 1 line · Vasiliy Kulikov
I think this version of the patch is ready for LKML review.
OK, post it - and CC kernel-hardening on your posting, as planned. (We'll see if we can afford the traffic, or if we need to split this list in two...)
Unfortunately, I don't have time to review this more closely (in context), and no one else in here provided any comments. ;-(
fs/proc/proc_net.c static struct net *get_proc_task_net(struct inode *dir) · line 114
Quoted 2 lines
fs/proc/proc_net.c +2
-0+if (pid->hide_net && + (!capable (CAP_NET_ADMIN ) && !in_group_p (pid->pid_gid))) {
As discussed, capable() should be the very last check.
Thanks,
Alexander
Hi,
A small follow-up of my current work.
I'm implementing the scheme I've proposed on LKML. Instead of on/off semantics a mask of allowed files is passed as a mount option. Like this:
mount -o tgid_allowed=.;cmdline;mounts;smaps /proc
All files not enumerated in tgid_allowed= will be restricted to processes being able to ptrace the task. Files enumerated in tgid_allowed= are not additionally restricted and classic POSIX permission checking rules are applied here. So, it's impossible to _relax_ procfs permissions with my scheme.
";" is used instead of "," as a comma is already used as options separator.
Besides tgid_allowed= there will be attr_allowed= and tid_allowed= to enumerated files in appropriate subdirectories.
As a "side effect" it will effectively fix all holes of poor permission checking of setuid binaries (including check and execve() race) without massive code refactoring. (All future procfs files are already protected too ;-))
Currently I have a working implementation of per-files restriction, but without mount option parsing. I'll post the code when I have a working implementation of at least parsing and handling tgid_allowed=XXX. Also there is a (minor, I hope) issue with /proc/PID/maps handling, where the ptrace check is too restrictive.
TODO: net restriction, privileged monitoring gid.
Thanks,
Signature · 2 lines
Vasiliy Kulikov http://www.openwall.com - bringing security into open computing environments
A message that is not in the archive (sent off-list or not yet received): 20110724185036.GC3510@albatros
Hi,
This is a demonstration of procfs feature I'm writing now. It is not yet fully ready (need more clean code and need to divide the patch into series), however, it's ready for some demonstration:
# mount -o remount,tgid_allowed=none,tid_allowed=none,attr_allowed=none /proc
root@albatros:~# ls -ld /proc/1
dr-x------ 8 root root 0 2011-07-26 17:57 /proc/1
# mount -o remount,tgid_allowed=stat /proc
# ls -ld /proc/1
dr-xr-xr-x 8 root root 0 2011-07-26 18:03 /proc/1
# ls -ld /proc/1/{stat,status,sched}
-rw------- 1 root root 0 2011-07-26 18:03 /proc/1/sched
-r--r--r-- 1 root root 0 2011-07-26 18:03 /proc/1/stat
-r-------- 1 root root 0 2011-07-26 18:03 /proc/1/status
# mount -o remount,tgid_allowed=all /proc
# ls -ld /proc/1/{stat,status,sched}
-rw-r--r-- 1 root root 0 2011-07-26 18:03 /proc/1/sched
-r--r--r-- 1 root root 0 2011-07-26 18:03 /proc/1/stat
-r--r--r-- 1 root root 0 2011-07-26 18:03 /proc/1/status
# ls -ld /proc/1/task/1/
dr-xr-xr-x 6 root root 0 2011-07-26 18:03 /proc/1/task/1/
# ls -ld /proc/1/task/1/{stat,status,sched}
-rw------- 1 root root 0 2011-07-26 18:04 /proc/1/task/1/sched
-r-------- 1 root root 0 2011-07-26 18:04 /proc/1/task/1/stat
-r-------- 1 root root 0 2011-07-26 18:04 /proc/1/task/1/status
# mount -o remount,tid_allowed=sched /proc
# ls -ld /proc/1/task/1/{stat,status,sched}
-rw-r--r-- 1 root root 0 2011-07-26 18:04 /proc/1/task/1/sched
-r-------- 1 root root 0 2011-07-26 18:04 /proc/1/task/1/stat
-r-------- 1 root root 0 2011-07-26 18:04 /proc/1/task/1/status
# mount -o remount,tid_allowed=all /proc
# ls -ld /proc/1/task/1/{stat,status,sched}
-rw-r--r-- 1 root root 0 2011-07-26 18:04 /proc/1/task/1/sched
-r--r--r-- 1 root root 0 2011-07-26 18:04 /proc/1/task/1/stat
-r--r--r-- 1 root root 0 2011-07-26 18:04 /proc/1/task/1/status
# ls -l /proc/1/attr/
итого 0
-rw------- 1 root root 0 2011-07-26 18:04 current
-rw------- 1 root root 0 2011-07-26 18:04 exec
-rw------- 1 root root 0 2011-07-26 18:04 fscreate
-rw------- 1 root root 0 2011-07-26 18:04 keycreate
-r-------- 1 root root 0 2011-07-26 18:04 prev
-rw------- 1 root root 0 2011-07-26 18:04 sockcreate
# mount -o remount,attr_allowed=exec\;sockcreate /proc
# ls -l /proc/1/attr/
итого 0
-rw------- 1 root root 0 2011-07-26 18:05 current
-rw-rw-rw- 1 root root 0 2011-07-26 18:05 exec
-rw------- 1 root root 0 2011-07-26 18:05 fscreate
-rw------- 1 root root 0 2011-07-26 18:05 keycreate
-r-------- 1 root root 0 2011-07-26 18:05 prev
-rw-rw-rw- 1 root root 0 2011-07-26 18:05 sockcreate
# mount -o remount,attr_allowed=all /proc
# ls -l /proc/1/attr/
итого 0
-rw-rw-rw- 1 root root 0 2011-07-26 18:05 current
-rw-rw-rw- 1 root root 0 2011-07-26 18:05 exec
-rw-rw-rw- 1 root root 0 2011-07-26 18:05 fscreate
-rw-rw-rw- 1 root root 0 2011-07-26 18:05 keycreate
-r--r--r-- 1 root root 0 2011-07-26 18:05 prev
-rw-rw-rw- 1 root root 0 2011-07-26 18:05 sockcreate
# mount -o remount,attr_allowed=none /proc
# ls -l /proc/1/attr/
итого 0
-rw------- 1 root root 0 2011-07-26 18:08 current
-rw------- 1 root root 0 2011-07-26 18:08 exec
-rw------- 1 root root 0 2011-07-26 18:08 fscreate
-rw------- 1 root root 0 2011-07-26 18:08 keycreate
-r-------- 1 root root 0 2011-07-26 18:08 prev
-rw------- 1 root root 0 2011-07-26 18:08 sockcreateAs you see, there are different sets for /proc/PID, /proc/PID/task/TID/, /proc/PID/attr. I've slightly changed the semantics and removed ".", it is implicitly enabled iff something inside is enabled. To deny "." XXX=none should be passed.
Questions/comments:
1) it seems to me "\;" is hard to pass, maybe ":" is a better separator?
2) almost all tid files are copies of tgid files. coredump_filter, mountstats, net are missing in task/. So, maybe fully remove tid_allowed= and copy the tgid files' permissions to the appropriate tid files?
3) currently the implementation is somewhat slow as every file is wrapped into additional file operations handlers. It cannot be removed for "fast" cases as all permissions of all files should be dynamically recalculated for every access. Probably I could make it faster a bit. It is the most significant drawback of the patch :-(
Thanks,
Signature · 1 line
Vasiliy Kulikov
[kernel-hardening] procfs {tid,tgid,attr}_allowed mount options
Hi,
Quoted 3 lines · Vasiliy Kulikov
This is a demonstration of procfs feature I'm writing now. It is not yet fully ready (need more clean code and need to divide the patch into series), however, it's ready for some demonstration:
Now the patch seems rather stable, more or less clean. It lacks some testing, after that I'll send it as RFC to LKML.
BTW, it fully solves the problem of procfs files kept across execve() (CVE-2011-1020).
--- fs/proc/Makefile | 2 +- fs/proc/base.c | 253 ++++++++++++++++----------------- fs/proc/base_perms.c | 314 +++++++++++++++++++++++++++++++++++++++++ fs/proc/inode.c | 18 +++ fs/proc/internal.h | 24 +++ fs/proc/root.c | 121 ++++++++++++++++- fs/proc/task_nommu.c | 2 +- include/linux/pid_namespace.h | 13 ++ include/linux/proc_fs.h | 12 ++ kernel/pid.c | 2 + kernel/pid_namespace.c | 10 ++- kernel/sysctl.c | 1 + 12 files changed, 635 insertions(+), 137 deletions(-)
--
fs/proc/Makefile +1 -1
@@ -8,7 +8,7 @@ proc-y := nommu.o task_nommu.o proc-$(CONFIG_MMU) := mmu.o task_mmu.o proc-y += inode.o root.o base.o generic.o array.o \ - proc_tty.o + proc_tty.o base_perms.o proc-y += cmdline.o proc-y += consoles.o proc-y += cpuinfo.o
fs/proc/base.c +121 -132
493 lines not on this page · raw message
fs/proc/base_perms.c new file +314 -0
314 lines not on this page · raw message
fs/proc/inode.c +18 -0
@@ -18,6 +18,10 @@#include <linux/module.h> #include <linux/sysctl.h> #include <linux/slab.h> +#include <linux/pid_namespace.h> +#include <linux/seq_file.h> +#include <linux/mount.h> +#include <asm/system.h> #include <asm/uaccess.h>
@@ -69,6 +73,7 @@ static struct inode *proc_alloc_inode(struct super_block *sb) ei->sysctl_entry =NULL ; ei->ns =NULL ; ei->ns_ops =NULL ; + ei->real_fops =NULL ; inode = &ei->vfs_inode; inode->i_mtime = inode->i_atime = inode->i_ctime =CURRENT_TIME ;return inode;
@@ -102,12 +107,25 @@ void __init proc_init_inodecache(void) init_once); } +static int proc_show_options (struct seq_file *seq,struct vfsmount *vfs) +{ +struct super_block *sb = vfs->mnt_sb; +struct pid_namespace *pid = sb->s_fs_info; + +if (pid->proc_perms.proc_pid_allowed) +seq_printf (seq,",proc_pid_allowed=." ); + +return 0 ; +} +static const struct super_operations proc_sops = { .alloc_inode = proc_alloc_inode, .destroy_inode = proc_destroy_inode, .drop_inode = generic_delete_inode, .evict_inode = proc_evict_inode, .statfs = simple_statfs, + .remount_fs = proc_remount, + .show_options = proc_show_options, };static void __pde_users_dec (struct proc_dir_entry *pde)
fs/proc/internal.h +24 -0
@@ -141,7 +141,31 @@ struct inode *proc_pid_make_inode(struct super_block * sb, struct task_struct *textern const struct dentry_operations pid_dentry_operations;int pid_getattr (struct vfsmount *mnt,struct dentry *dentry,struct kstat *stat);int proc_setattr (struct dentry *dentry,struct iattr *attr); +int proc_remount (struct super_block *sb,int *flags,char *data);extern const struct inode_operations proc_ns_dir_inode_operations;extern const struct file_operations proc_ns_dir_operations; +extern const struct inode_operations proc_tgid_base_inode_operations; +extern const struct inode_operations proc_tid_base_inode_operations; +extern const struct inode_operations proc_attr_dir_inode_operations; + +extern const struct file_operations proc_pid_perms_fops; +extern const int perms_size[]; +extern const struct pid_entry *dir_ents[]; + +extern mode_t get_ent_perms (const struct pid_entry *p, +struct proc_perms *perms, +int ndir, +int nent); + +struct pid_entry { +char *name; +int len; +mode_t mode; +const struct inode_operations *iop; +const struct file_operations *fop; +union proc_op op; +bool need_perms_check; +}; +
fs/proc/root.c +119 -2
154 lines not on this page · raw message
fs/proc/task_nommu.c +1 -1
@@ -201,7 +201,7 @@ static void *m_start(struct seq_file *m, loff_t *pos)if (!priv->task)return ERR_PTR (-ESRCH ); - mm =mm_for_maps (priv->task); + mm =get_task_mm (priv->task);if (!mm ||IS_ERR (mm)) {put_task_struct (priv->task); priv->task =NULL ;
include/linux/pid_namespace.h +13 -0
@@ -16,6 +16,18 @@ struct pidmap {struct bsd_acct_struct ; +enum { +PROC_PERMS_NTID , +PROC_PERMS_NTGID , +PROC_PERMS_NATTR , +PROC_PERMS_NMAX +}; + +struct proc_perms { +bool proc_pid_allowed; +bool *proc_ent_allowed[PROC_PERMS_NMAX ]; +}; +struct pid_namespace {struct kref kref;struct pidmap pidmap[PIDMAP_ENTRIES ];
@@ -30,6 +42,7 @@ struct pid_namespace {#ifdef CONFIG_BSD_PROCESS_ACCT struct bsd_acct_struct *bacct;#endif +struct proc_perms proc_perms; };extern struct pid_namespace init_pid_ns;
include/linux/proc_fs.h +12 -0
@@ -115,10 +115,17 @@ struct proc_dir_entry *proc_create_data(const char *name, mode_t mode,extern void remove_proc_entry (const char *name,struct proc_dir_entry *parent);struct pid_namespace ; +struct proc_perms ;extern int pid_ns_prepare_proc (struct pid_namespace *ns);extern void pid_ns_release_proc (struct pid_namespace *ns); +extern int init_proc_perms (struct proc_perms *p); +extern void free_proc_perms (struct proc_perms *p); + +extern void fill_proc_perms_ent (struct proc_perms *p,int ndir,bool val); +extern bool set_proc_perms_ent (struct proc_perms *p,int ndir,const char *fname,bool val); +/* * proc_tty.c */
@@ -267,6 +274,11 @@ struct proc_inode {struct pid *pid;int fd;union proc_op op; + +/* Used by files inside /proc/PID/ only */ +const struct file_operations *real_fops; +const void *dirent; +struct proc_dir_entry *pde;struct ctl_table_header *sysctl;struct ctl_table *sysctl_entry;
kernel/pid.c +2 -0
@@ -565,6 +565,8 @@ void __init pidmap_init(void)set_bit (0 , init_pid_ns.pidmap[0 ].page);atomic_dec (&init_pid_ns.pidmap[0 ].nr_free); +BUG_ON (init_proc_perms (&init_pid_ns.proc_perms)); + init_pid_ns.pid_cachep =KMEM_CACHE (pid,SLAB_HWCACHE_ALIGN |SLAB_PANIC ); }
kernel/pid_namespace.c +9 -1
@@ -97,12 +97,18 @@ static struct pid_namespace *create_pid_namespace(struct pid_namespace *parent_pfor (i =1 ; i <PIDMAP_ENTRIES ; i++)atomic_set (&ns->pidmap[i].nr_free,BITS_PER_PAGE ); - err =pid_ns_prepare_proc (ns); + err =init_proc_perms (&ns->proc_perms);if (err) goto out_put_parent_pid_ns; + err =pid_ns_prepare_proc (ns); +if (err) + goto out_free_proc_perms; +return ns; +out_free_proc_perms: +free_proc_perms (&ns->proc_perms); out_put_parent_pid_ns:put_pid_ns (parent_pid_ns); out_free_map:
@@ -119,6 +125,8 @@ static void destroy_pid_namespace(struct pid_namespace *ns)for (i =0 ; i <PIDMAP_ENTRIES ; i++)kfree (ns->pidmap[i].page); + +free_proc_perms (&ns->proc_perms);kmem_cache_free (pid_ns_cachep, ns); }
kernel/sysctl.c +1 -0
@@ -57,6 +57,7 @@#include <linux/pipe_fs_i.h> #include <linux/oom.h> #include <linux/kmod.h> +#include <linux/pid_namespace.h> #include <asm/uaccess.h> #include <asm/processor.h>
--
[kernel-hardening] Re: procfs {tid,tgid,attr}_allowed mount options
Hi,
New version. Cleanups/fixes here and there.
It lacks net/ restriction, but IMO it is already complicated enough (more than 500 new lines). Such (relatively) simple thing as net_allowed= is an additional care. I'd achieve at least processes restrictions in upstream, after it will come networking.
--- fs/proc/Makefile | 2 +- fs/proc/base.c | 286 +++++++++++++++++++++------------------ fs/proc/base_perms.c | 305 +++++++++++++++++++++++++++++++++++++++++ fs/proc/inode.c | 19 +++ fs/proc/internal.h | 25 ++++ fs/proc/root.c | 121 ++++++++++++++++- fs/proc/task_nommu.c | 2 +- include/linux/pid_namespace.h | 13 ++ include/linux/proc_fs.h | 12 ++ kernel/pid.c | 2 + kernel/pid_namespace.c | 10 ++- kernel/sysctl.c | 1 + 12 files changed, 660 insertions(+), 138 deletions(-)
---
fs/proc/Makefile +1 -1
@@ -8,7 +8,7 @@ proc-y := nommu.o task_nommu.o proc-$(CONFIG_MMU) := mmu.o task_mmu.o proc-y += inode.o root.o base.o generic.o array.o \ - proc_tty.o + proc_tty.o base_perms.o proc-y += cmdline.o proc-y += consoles.o proc-y += cpuinfo.o
fs/proc/base.c +153 -133
520 lines not on this page · raw message
fs/proc/base_perms.c new file +305 -0
305 lines not on this page · raw message
fs/proc/inode.c +19 -0
@@ -18,6 +18,10 @@#include <linux/module.h> #include <linux/sysctl.h> #include <linux/slab.h> +#include <linux/pid_namespace.h> +#include <linux/seq_file.h> +#include <linux/mount.h> +#include <asm/system.h> #include <asm/uaccess.h>
@@ -69,6 +73,8 @@ static struct inode *proc_alloc_inode(struct super_block *sb) ei->sysctl_entry =NULL ; ei->ns =NULL ; ei->ns_ops =NULL ; + ei->real_fops =NULL ; + ei->dirent =NULL ; inode = &ei->vfs_inode; inode->i_mtime = inode->i_atime = inode->i_ctime =CURRENT_TIME ;return inode;
@@ -102,12 +108,25 @@ void __init proc_init_inodecache(void) init_once); } +static int proc_show_options (struct seq_file *seq,struct vfsmount *vfs) +{ +struct super_block *sb = vfs->mnt_sb; +struct pid_namespace *pid = sb->s_fs_info; + +if (pid->proc_perms.proc_pid_allowed) +seq_printf (seq,",proc_pid_allowed=." ); + +return 0 ; +} +static const struct super_operations proc_sops = { .alloc_inode = proc_alloc_inode, .destroy_inode = proc_destroy_inode, .drop_inode = generic_delete_inode, .evict_inode = proc_evict_inode, .statfs = simple_statfs, + .remount_fs = proc_remount, + .show_options = proc_show_options, };static void __pde_users_dec (struct proc_dir_entry *pde)
fs/proc/internal.h +25 -0
@@ -137,11 +137,36 @@ int proc_fill_cache(struct file *filp, void *dirent, filldir_t filldir, const char *name, int len, instantiate_t instantiate, struct task_struct *task, const void *ptr);int pid_revalidate (struct dentry *dentry,struct nameidata *nd); +extern void __pid_revalidate (struct inode *inode,struct task_struct *task);struct inode *proc_pid_make_inode (struct super_block * sb,struct task_struct *task);extern const struct dentry_operations pid_dentry_operations;int pid_getattr (struct vfsmount *mnt,struct dentry *dentry,struct kstat *stat);int proc_setattr (struct dentry *dentry,struct iattr *attr); +int proc_remount (struct super_block *sb,int *flags,char *data);extern const struct inode_operations proc_ns_dir_inode_operations;extern const struct file_operations proc_ns_dir_operations; +extern const struct inode_operations proc_tgid_base_inode_operations; +extern const struct inode_operations proc_tid_base_inode_operations; +extern const struct inode_operations proc_attr_dir_inode_operations; + +extern const struct file_operations proc_pid_perms_fops; +extern const int perms_size[]; +extern const struct pid_entry *dir_ents[]; + +extern mode_t get_ent_perms (const struct pid_entry *p, +struct proc_perms *perms, +int ndir, +int nent); + +struct pid_entry { +char *name; +int len; +mode_t mode; +const struct inode_operations *iop; +const struct file_operations *fop; +union proc_op op; +bool need_perms_check; +}; +
fs/proc/root.c +119 -2
154 lines not on this page · raw message
fs/proc/task_nommu.c +1 -1
@@ -201,7 +201,7 @@ static void *m_start(struct seq_file *m, loff_t *pos)if (!priv->task)return ERR_PTR (-ESRCH ); - mm =mm_for_maps (priv->task); + mm =get_task_mm (priv->task);if (!mm ||IS_ERR (mm)) {put_task_struct (priv->task); priv->task =NULL ;
include/linux/pid_namespace.h +13 -0
@@ -16,6 +16,18 @@ struct pidmap {struct bsd_acct_struct ; +enum { +PROC_PERMS_NTID , +PROC_PERMS_NTGID , +PROC_PERMS_NATTR , +PROC_PERMS_NMAX +}; + +struct proc_perms { +bool proc_pid_allowed; +bool *proc_ent_allowed[PROC_PERMS_NMAX ]; +}; +struct pid_namespace {struct kref kref;struct pidmap pidmap[PIDMAP_ENTRIES ];
@@ -30,6 +42,7 @@ struct pid_namespace {#ifdef CONFIG_BSD_PROCESS_ACCT struct bsd_acct_struct *bacct;#endif +struct proc_perms proc_perms; };extern struct pid_namespace init_pid_ns;
include/linux/proc_fs.h +12 -0
@@ -115,10 +115,17 @@ struct proc_dir_entry *proc_create_data(const char *name, mode_t mode,extern void remove_proc_entry (const char *name,struct proc_dir_entry *parent);struct pid_namespace ; +struct proc_perms ;extern int pid_ns_prepare_proc (struct pid_namespace *ns);extern void pid_ns_release_proc (struct pid_namespace *ns); +extern int init_proc_perms (struct proc_perms *p); +extern void free_proc_perms (struct proc_perms *p); + +extern void fill_proc_perms_ent (struct proc_perms *p,int ndir,bool val); +extern bool set_proc_perms_ent (struct proc_perms *p,int ndir,const char *fname,bool val); +/* * proc_tty.c */
@@ -267,6 +274,11 @@ struct proc_inode {struct pid *pid;int fd;union proc_op op; + +/* Used by files inside /proc/PID/ only */ +const struct file_operations *real_fops; +const void *dirent; +struct proc_dir_entry *pde;struct ctl_table_header *sysctl;struct ctl_table *sysctl_entry;
kernel/pid.c +2 -0
@@ -565,6 +565,8 @@ void __init pidmap_init(void)set_bit (0 , init_pid_ns.pidmap[0 ].page);atomic_dec (&init_pid_ns.pidmap[0 ].nr_free); +BUG_ON (init_proc_perms (&init_pid_ns.proc_perms)); + init_pid_ns.pid_cachep =KMEM_CACHE (pid,SLAB_HWCACHE_ALIGN |SLAB_PANIC ); }
kernel/pid_namespace.c +9 -1
@@ -97,12 +97,18 @@ static struct pid_namespace *create_pid_namespace(struct pid_namespace *parent_pfor (i =1 ; i <PIDMAP_ENTRIES ; i++)atomic_set (&ns->pidmap[i].nr_free,BITS_PER_PAGE ); - err =pid_ns_prepare_proc (ns); + err =init_proc_perms (&ns->proc_perms);if (err) goto out_put_parent_pid_ns; + err =pid_ns_prepare_proc (ns); +if (err) + goto out_free_proc_perms; +return ns; +out_free_proc_perms: +free_proc_perms (&ns->proc_perms); out_put_parent_pid_ns:put_pid_ns (parent_pid_ns); out_free_map:
@@ -119,6 +125,8 @@ static void destroy_pid_namespace(struct pid_namespace *ns)for (i =0 ; i <PIDMAP_ENTRIES ; i++)kfree (ns->pidmap[i].page); + +free_proc_perms (&ns->proc_perms);kmem_cache_free (pid_ns_cachep, ns); }
kernel/sysctl.c +1 -0
@@ -57,6 +57,7 @@#include <linux/pipe_fs_i.h> #include <linux/oom.h> #include <linux/kmod.h> +#include <linux/pid_namespace.h> #include <asm/uaccess.h> #include <asm/processor.h>
[kernel-hardening] Re: procfs {tid,tgid,attr}_allowed mount options
Solar,
Quoted 1 line · Vasiliy Kulikov
New version. Cleanups/fixes here and there.
One question: do we really need gid= option? The only user I know is identd, but does anybody use it nowadays?
With gid= I see 2 drawbacks:
1) Code becomes worse because of additional permission checks.
2) From the upstream's point of view it is very limited and unextendable feature.
So, I'd go further without gid=, at least for the beginning.
Thanks,
Signature · 2 lines
Vasiliy Kulikov http://www.openwall.com - bringing security into open computing environments
Re: [kernel-hardening] procfs {tid,tgid,attr}_allowed mount options
Quoted 2 lines · Vasiliy Kulikov
One question: do we really need gid= option? The only user I know is identd, but does anybody use it nowadays?
identd is mostly obsolete, but I am using gid= with 2.4.x-ow kernels to let a group of sysadmins see all users' processes and network connections without having to use su. In fact, I use it on my very own computers - again, to let my main desktop user account see everything, while not letting my pseudo-user accounts (that I use for things such as a web browser) also see everything (they're not in group proc).
Quoted 6 lines
With gid= I see 2 drawbacks:
1) Code becomes worse because of additional permission checks.
2) From the upstream's point of view it is very limited and unextendable feature.
This feature is precisely what I needed and used for over a decade. I never needed more flexibility. Maybe this says something.
Quoted 1 line
So, I'd go further without gid=, at least for the beginning.
I don't know what works best for upstream acceptance in the beginning, but I definitely want this feature to get in, and it is a must for Owl.
Thanks,
Alexander
Re: [kernel-hardening] procfs {tid,tgid,attr}_allowed mount options
In reply to Vasiliy KulikovVasiliy,
Quoted 6 lines · Vasiliy Kulikov
New version. Cleanups/fixes here and there.
It lacks
net/restriction, but IMO it is already complicated enough (more than 500 new lines). Such (relatively) simple thing as net_allowed= is an additional care. I'd achieve at least processes restrictions in upstream, after it will come networking.
That's a lot of code already. I tried reviewing it, but I felt that I need an English description of its functionality first. Perhaps you're preparing one for posting to LKML anyway - can you please post it in here first?
Thanks,
Alexander
Re: [kernel-hardening] procfs {tid,tgid,attr}_allowed mount options
Solar,
Quoted 9 lines · Solar Designer
Quoted 6 lines · Vasiliy Kulikov
New version. Cleanups/fixes here and there.
It lacks
net/restriction, but IMO it is already complicated enough (more than 500 new lines). Such (relatively) simple thing as net_allowed= is an additional care. I'd achieve at least processes restrictions in upstream, after it will come networking.That's a lot of code already.
Yes, so I'd try to get comments from net-less and gid-less patch first. Probably the whole way of permissions handling would be ridicules for kernel forlk :)
Quoted 4 lines
I tried reviewing it, but I felt that I
need an English description of its functionality first. Perhaps you're preparing one for posting to LKML anyway - can you please post it in here first?
Something like this:
This patch adds support of pid_allowed=XX and attr_allowed=YY mount options for procfs. When set, all /proc/PID/ files are restricted to the owner, except filenames passed via pid_allowed= argument. E.g. with pid_allowed=sched sched file would be world readable and other files would be restricted to the task owner. The same for /proc/PID/attr/ files and attr_allowed=YY.
The new struct file_operations proc_pid_perms_fops was created to deal with permissions keeping in mind pid_allowed=. It is a wrapper for all /proc/PID/* and /proc/PID/task/TID/* file_operations. It checks current task permissions against the target task on each struct file access to avoid races against execve().
All pid_entry's gain the following permission checking algorithm by default:
1) if the filename is not passed via pid_allowed= then ptrace check is applied.
2) Otherwise, if file POSIX permissions deny access to the world, ptrace check is applied.
3) Otherwise, generic permission checking scheme is applied. For procfs files it additionally includes ptrace check if current euid and the procfs inode uid are equal.
For the simplicity meta values are allowed: "none" and "all". "all" means all files are a subject of (2) and (3) checks. "none" means all files are a subject of (1) check. "none" additionally restricts access to the /proc/PID/ directory itself.
Old ptrace checks are removed from the handlers as they are now obsoleted by proc_pid_perms_fops checks. Handlers may further restrict the permission model by introducing additional checks. For some files the ptrace check is too strong (environ, auxv, maps, numa_maps, and smaps), so no implicit permission checking is applied for them. These files should use pid_entry macros with "_PERMS" suffix.
Only /proc/PID/, /proc/PID/*, /proc/PID/task/TID/, /proc/PID/task/TID/*, /proc/PID/attr/* files' permission checking is changed. fd/ and fdinfo/ are outstanding files and check the permissions on their own.
Thanks,
---
Vasiliy Kulikov
http://www.openwall.com - bringing security into open computing environments
Re: [kernel-hardening] procfs {tid,tgid,attr}_allowed mount options
Quoted 6 lines · Vasiliy Kulikov
Quoted 1 line · Solar Designer
That's a lot of code already.
Yes, so I'd try to get comments from net-less and gid-less patch first. Probably the whole way of permissions handling would be ridicules for kernel forlk :)
I feel that you may have treated Andrew Morton's suggestion too seriously. I think he was hoping for something not only more general, but also simple.
With your invasive changes, even checking for (lack of) potential new vulnerabilities (such as lack of ptrace check where it previously existed and was needed) feels non-trivial. Or maybe I just did not look closely enough.
Perhaps run this by LKML as RFC and see what they think? And be willing to revert to your old approach, with more hard-coding, now that you have this arguably overly complicated alternative. Maybe it will convince Andrew Morton that something simpler and less flexible would be better.
Thanks,
Alexander
Re: [kernel-hardening] procfs {tid,tgid,attr}_allowed mount options
Quoted 4 lines · Solar Designer
Perhaps run this by LKML as RFC and see what they think? And be willing to revert to your old approach, with more hard-coding, now that you have this arguably overly complicated alternative. Maybe it will convince Andrew Morton that something simpler and less flexible would be better.
Just my opinion, but the gid option is simple and to the point. More complex solution will likely :
* not be used at all
* not be relevant to people with very specific needs anyway
* introduce bugs and/or vulnerabilities, either from the code or from misconfigurations
Point #2 is important. Very specific needs should not be addressed in this specific patch, it should be configured in something with a global scope, such as a LSM.
I believe having effective security systems enabled by default is more important than having generalistic and configurable systems nobody care about. For example, being able to let a process choose the set of system calls it should use is more useful to me than having SELinux loaded.
Quoted 9 lines · Solar Designer
Quoted 8 lines · Vasiliy Kulikov
Quoted 7 lines · Solar Designer
Quoted 3 lines · Vasiliy Kulikov
TODO/thoughs:
-/proc/pid/net/currently doesn't show ANYTHING, even "." and "..". This is confusing :)Ouch. Can't you simply restrict its perms such that this directory can't be listed unless you have privs?
...
Quoted 3 lines
Quoted 3 lines
Another solution - create a fake net namespace and process this namespace if not enough permissions :) It also removes weird netstat errors like "seems like networking was disabled for this kernel".
A fake net namespace works perfect:
$ LANG=C netstat -nlp4 (Not all processes could be identified, non-owned process info will not be shown, you would have to be root to see it all.) Active Internet connections (only servers) Proto Recv-Q Send-Q Local Address Foreign Address State PID/Program name
No warning from netstat. I remember brctl didn't properly handle missing sysfs files, so fake files make sense.
Will repost the patch after I'm sure that changing hidepid works well with inode caching (I see a bug in my current implementation).
Thanks,
Signature · 1 line
Vasiliy
Solar,
Quoted 11 lines · Solar Designer
Quoted 6 lines
Quoted 3 lines
This is not generic, but at least it's simple, not confusing, and it allows for future changes (we may change what exactly restricted means).
Changing what some option means after implementing it is a ABI breakage, which is terrible for upstream.
This depends on how the option is defined initially. It is possible to define the option as "enabling unspecified version-dependent restrictions, which are subject to change across kernel versions and builds". ;-) OK, the word "unspecified" may be dropped.
Everything that is implemented should behave the same way it was initially implemented, regardless of what is explicitly marked as "ABI". This is upsteam's way to define "ABI" term. Often they deliberately break such ABI, but normally this is not encouraged.
Quoted 3 lines
Here's a related thought: if these mount options happen to affect all instances of the filesystem (in the same container), maybe they should be sysctl's instead?
AFAIR, only net namespaces have their own sysctl sets. Other sysctls are global. So, implementing pid_namespace-specific sysctl would be a bit weird (according to current policies).
Thanks,
Signature · 1 line
Vasiliy
Quoted 8 lines · Vasiliy Kulikov
Quoted 3 lines · Solar Designer
Here's a related thought: if these mount options happen to affect all instances of the filesystem (in the same container), maybe they should be sysctl's instead?
AFAIR, only net namespaces have their own sysctl sets. Other sysctls are global. So, implementing pid_namespace-specific sysctl would be a bit weird (according to current policies).
Here's what we have immediate need for, in practice:
We need to be able to mount /proc with different permission settings in different OpenVZ containers (perhaps running different distros, which have their different defaults - e.g., Owl will use the restricted proc options by default, but other distros mostly won't).
Since recent versions of OpenVZ build upon the namespaces code that has been upstream'ed, I guess this will rely on upstream's namespaces code (once we move to RHEL6'ish OpenVZ kernels and beyond), correct?
Now, leaving sysctl's aside and speaking of mount options only for now, what happens when a container mounts /proc with umask=007, but then another container mounts /proc without that option or with umask=0? Does the first container retain its restricted perms, including for newly appearing entries under its /proc? If so, where is this different setting stored? Is it per mount (preferable)? Is it per pid namespace (OK)? Or per net namespace (weird)?
Thanks,
Alexander
Solar,
Quoted 3 lines · Solar Designer
Since recent versions of OpenVZ build upon the namespaces code that has been upstream'ed, I guess this will rely on upstream's namespaces code (once we move to RHEL6'ish OpenVZ kernels and beyond), correct?
Namespaces - yes. But AFAIK, only a pair of sysctls is accessible in containers, IIRC, only net sysctls.
Quoted 7 lines
Now, leaving sysctl's aside and speaking of mount options only for now, what happens when a container mounts /proc with umask=007, but then another container mounts /proc without that option or with umask=0? Does the first container retain its restricted perms, including for newly appearing entries under its /proc? If so, where is this different setting stored? Is it per mount (preferable)? Is it per pid namespace (OK)?
It is per-pid_namespace, it should be fully consistent with OpenVZ.
Thanks,
Signature · 1 line
Vasiliy
Quoted 4 lines · Solar Designer
Indeed, we could set some of these perms with chmod post-mount, but as discussed this has drawbacks. So ideally our preferred configuration (which will be the default on Owl) should be achievable with mount options alone.
What if implement mode=XXX option to alter root directory permissions only, like tmpfs? Then all non-pid files may be chmod'ed without any race due to distro-specific policy and then "chmod a+rx /proc" to allow nonroot users to see procfs files.
Thanks,
Signature · 2 lines
Vasiliy Kulikov http://www.openwall.com - bringing security into open computing environments
Quoted 10 lines · Vasiliy Kulikov
Quoted 4 lines · Solar Designer
Indeed, we could set some of these perms with chmod post-mount, but as discussed this has drawbacks. So ideally our preferred configuration (which will be the default on Owl) should be achievable with mount options alone.
What if implement mode=XXX option to alter root directory permissions only, like tmpfs? Then all non-pid files may be chmod'ed without any race due to distro-specific policy and then "chmod a+rx /proc" to allow nonroot users to see procfs files.
This makes sense to me, although other mount options that you implemented appear to be sufficient to implement the desired default policy for Owl.
Alexander