procfs: boot- and runtime configurable access mode for /proc/<pid> dirs

Hi all,

the attached patch enhances the procfs code to allow for modification of /proc/<pid> dir access modes. It does so
- by hard-wiring (.config) a boot-time default mode (555, 550 or 500)
- which can be overriden by a boot parameter piddir_mode
- and also introduces an inode /proc/piddirs (644) which enables runtime-configuration (already existing pid-dirs will be updated on next access)

I started this quite a while back [1] and it worked fine ever since on my servers. Now I finally enhanced that code by run-time configurability.

Let me know what you think and please CC me on replying.

Thanks,

Daniel

[1] http://lkml.org/lkml/2007/11/20/136

Index: linux-2.6.38/fs/proc/piddirs.c
===================================================================
@@ -0,0 +1,87 @@
+#include <linux/fs.h>
+#include <linux/init.h>
+#include <linux/module.h>
+#include <linux/moduleparam.h>
+#include <linux/pid_namespace.h>
+#include <linux/proc_fs.h>
+#include <linux/sched.h>
+#include <linux/seq_file.h>
+#include <linux/seqlock.h>
+#include <linux/time.h>
+#include "internal.h"
+
+/* prototypes */
+static int	__init param_piddir_mode(char *buf);
+static int	__init proc_piddirs_init(void);
+static int	piddirs_proc_open(struct inode *inode, struct file *file);
+static int	piddirs_proc_show(struct seq_file *m, void *v);
+static ssize_t	piddirs_proc_write(struct file *file, const char __user *buf,
+			size_t count, loff_t *ppos);
+
+/* variables */
+static const struct file_operations piddirs_proc_fops = {
+	.open		= piddirs_proc_open,
+	.read		= seq_read,
+	.write		= piddirs_proc_write,
+	.llseek		= seq_lseek,
+	.release	= single_release,
+};
+
+/* This char only stores the module parameter.
+   The actual piddir_creation_mode is stored in base.c */
+static char *piddir_mode;
+
+/* module config */
+module_param(piddir_mode, charp, 0);
+MODULE_PARM_DESC(piddir_mode, "Default creation mode");
+module_init(proc_piddirs_init);
+__setup("piddir_mode=", param_piddir_mode);
+
+
+
+
+static int __init param_piddir_mode(char *buf)
+{
+	if (strlen(buf))
+		set_piddir_creation_mode(buf[0]);
+	return 1;
+}
+
+static int __init proc_piddirs_init(void)
+{
+	proc_create(
+		"piddirs",
+		S_IRUSR|S_IWUSR|S_IRGRP|S_IROTH,
+		NULL,
+		&piddirs_proc_fops
+	);
+	return 0;
+}
+
+static int piddirs_proc_open(struct inode *inode, struct file *file)
+{
+	return single_open(file, piddirs_proc_show, NULL);
+}
+
+static int piddirs_proc_show(struct seq_file *m, void *v)
+{
+	seq_printf(m, "%c\n", get_piddir_creation_mode());
+	return 0;
+}
+
+static ssize_t piddirs_proc_write(struct file *file,
+		const char __user *buf,
+		size_t count, loff_t *ppos)
+{
+	char tmp;
+
+	if (
+		(count == 2) &&
+		!copy_from_user(&tmp, buf, 1) &&
+		(set_piddir_creation_mode(tmp) == 0)
+	) {
+		return count;
+	} else {
+		return -EFAULT;
+	}
+}

Index: linux-2.6.38/fs/proc/internal.h
===================================================================
@@ -94,6 +94,10 @@
 extern spinlock_t proc_subdir_lock;
 
 struct dentry *proc_pid_lookup(struct inode *dir, struct dentry * dentry, struct nameidata *);
+#ifndef CONFIG_PROC_PIDDIRS_UNRESTRICTED
+char get_piddir_creation_mode(void);
+int set_piddir_creation_mode(char mode);
+#endif
 int proc_pid_readdir(struct file * filp, void * dirent, filldir_t filldir);
 unsigned long task_vsize(struct mm_struct *);
 unsigned long task_statm(struct mm_struct *,

Index: linux-2.6.38/fs/proc/Makefile
===================================================================
@@ -27,3 +27,6 @@
 proc-$(CONFIG_PROC_DEVICETREE)	+= proc_devtree.o
 proc-$(CONFIG_PRINTK)	+= kmsg.o
 proc-$(CONFIG_PROC_PAGE_MONITOR)	+= page.o
+proc-$(CONFIG_PROC_PIDDIRS_MODE_UGO)	+= piddirs.o
+proc-$(CONFIG_PROC_PIDDIRS_MODE_UG)	+= piddirs.o
+proc-$(CONFIG_PROC_PIDDIRS_MODE_U)	+= piddirs.o

Keeping u/g/o inside kernel is horrible.

What is the usecase? Content of /proc/* is identical.

Why exactly? Since it's only a char and not char[] I don't see the disadvantage over int or a define or whatever. Of course I could always change that if that's a de-facto standard I just didn't know about.

Use-case is to isolate process information from other users' or groups' eyes, e.g. with 550 the output of ps aux only lists processes of the groups your user is a member of.

Keep mode_t inside kernel, this will get rid of many ifdefs.

This is doable with some ps(1) switch, I'm sure.

The content of /proc/$PID directory is not a secret.

More to the point, permissions in /proc/<pid>/* don't do us much good. As the matter of fact, we ought to make them all flat - i.e. same for user/group/other, since we have to recheck access rights on every damn IO operations. Checks done at open() are useless here - have the task exec suid-root binary and they are obsolete.

Too obvious, point made.

Sure, I could just run ps ux instead of ps aux and I'm done - in case I wanna see only MY procs. That's my very point: sometimes it needs to be a secret and not by ps-invoking-user's choice at that but by an admin's enforcement. There are cases where I wouldn't want anybody ELSE to know ANYTHING about my procs, not even their existence. So even when I'm root on a box and I could restrict user-space tools...there's always another unrestricted one. A curious user just compiles his own toy and goes fishing.

Real-world example: amongst many other (administrative) isolation mechanisms to keep users apart, I've been using this approach for years to enforce privacy in several hosting environments. Just think of poorly implemented software which doesn't mask cmdline parameters like --password. Of course one could argue "Just switch to another software." Needless to say, that's often not option.

Well, my patch is about modes of the pid-dirs themselves, not their contents. And it changes procfs' behaviour about modes both on initial creation and during revalidation on access. However flattening all the piddir's entries DOES pose a security risk. Have a look at the "traditional behaviour": piddir world-readable, however e.g. <pid>/environ isn't. Often it's a workaround for broken software to specify a password within an environment variable instead of by cmdline. Since up until now all processes including their full cmdlines are visible to everyone, environ must be considered more sensitive than a cmdline.

The security implications are taken care of by my patch. The more restrictive the modes for the piddir, the less access to its contents. So from a security point of view, my patch takes care of that and IMHO there's no need to put hands on the procfs' management of modes for piddir-contents.

Bull. /proc/<pid>/foo contents is sensitive, your patch doesn't do you any good. fork(), open /proc/<child's PID>/foo in parent, then exec suid-root binary in child. At that point mode_t of any files or directories does not matter anymore.

You *must* do whatever access checks at read(2) time for these files. And if you do that, the checks at open(2) time do not matter.

In particular, /proc/*/environ does ptrace_may_access() on read(2) (with a race fixed by today's merge, BTW). It could very well be r--r--r--; having it r-------- does not increase security at all.

Not sure If I understand correctly, but:

...you would have to roll your own suid-root binary to be able to look into other /proc/PID directories, no? But making a binary suid-root requires root to begin with.

I'd love to finally have a more restrictive /proc directory. Even if it only makes things "harder" (not necessarily "impossible") to get information from user users on the same box.

Christian.

BOFH excuse #216:

What office are you in? Oh, that one.  Did you know that your building was built over the universities first nuclear research site? And wow, aren't you the lucky one, your office is right over where the core is buried!

No. You could look into /proc/PID/foo of process running a suid-root binary that you have execve()'ed. A binary that had been there legitimately. If these files have sensitive contents, you have a bad problem on hands (and we had quite a few of those); IOW, you need to do checks at read(2) time, since the identity of owner can change between open() and read(). And if you do those checks there, you don't need to care about open-time checks at all, since any attempt to do IO will be rejected anyway.