[PATCH 0/9] procfs: smooth steps to secure some /proc/<pid>/*

[PATCH 0/9] procfs: smooth steps to secure some /proc/<pid>/*

[Djalal Harouni]

Series to apply on top of next-20140526

Some /proc/<pid>/* are sensitive files that need appropriate permission
checks.

Currently there are two issues with these files, to summarize:
1) unprivileged process:
   open("/proc/1/*", O_RDONLY)
   and passes the fd to something privileged.

   This affects already running deamons and processes.

2) unprivileged process:
   open("/proc/self/*", O_RDONLY) and forks.
   The parent execve a privileged process.

   This affects processes that are executed by that user...

   The only restriction: is to find a privileged program that reads
   from the specified input. Determined attackers may find one...

   Some distro just don't chip these suid binaries.


A months ago I've sent a series to use file->f_cred during ->read()
to protect the sensitive entries:
https://lkml.org/lkml/2013/10/1/371

That solution was rejected, Ok.

It seems that every body is busy, and in the mean time these files are
exposed, and can be used to bypass ASLR. So I propose here smooth steps
to secure at least issue (1) and it works without breaking userspace,
but it does not cover all files. If this series is Ack-ed I'll send
another series to handle the left ones.


For issue (2) from the file->f_cred discussion:
https://lkml.org/lkml/2013/10/1/371

Every one seems to want a mechanism to revoke the fd. Yes I do agree, it
should be the best solution. However it should not stop us from handling
issue (1).


So to solve issue (1) of *already* *running* processes:

I start with the entries that contain sensitive info and at the same
time are world readable:
/proc/<pid>/stat
/proc/<pid>/wchan
/proc/<pid>/maps  (will be handled in next patches).

/proc/<pid>/{stack|syscall} I add the ptrace capability check and cache
it.


The following scheme is used in order to prevent ASLR leaks:
a) Perform permission checks during ->open()
b) Cache the result of a) and return succes
c) Recheck the cached result during ->read()
d) If cached == DENY:
   then we replace the sensitive fields with zeros, userspace won't
   break and sensitive fields are protected.

   As proposed by Ingo and as it's currently done for /proc/*/stat:
   https://lkml.org/lkml/2013/10/3/203


Some of these files use sequence iterators, and have their proper
internal logic. I've made sure that we don't change this logic nor the
handlers. The only change is: make the cached permission check done
during ->open() available during ->read().

This design will also cover every one concerns about LSM, especially
the YAMA module.


As I've said the logic of the handlers of these files did not change, I
just made the cached permission available. This should work with any
future improvements.


Please, if time permits do review!

Thanks!


Patches:

1) Preparation for the whole series:
Patch 1/9: preparation patch to add flags to deny or allow access.
Patch 2/9: preparation patch to unify the permission checks.
           The new pid_entry_access() can be used by other code too.

2) Preparation to handle /proc/<pid>/{wchan|syscall}
Patch 3/9: add helpers to handle shared code between wchan and syscall.
           This patch makes sure that the internal handlers will keep
	   their semantic.

3) Improve /proc/<pid>/{wchan|syscall} protections:
Patch 4/9: improve /proc/<pid>/whcan protection.
Patch 5/9: improve /proc/<pid>/syscall protection.

4) Preparation to handle /proc/<pid>/{stat|stack}
Patch 6/9: add shared struct 'pid_seq_private' that will be used by both
           /proc/<pid>/{stat|stack} to have access to the cached
	   permission checks.
Patch 7/9: add helper to handle shared code between stat and stack.
           This patch makes sure that these entries will continue to use
	   sequence iterators and that the semantic of their internal
	   handlers wont change.

5): Improve /proc/<pid>/{stat|stack} protections:
Patch 8/9: improve /proc/<pid>/stat protection.
Patch 9/9: improve /proc/<pid>/stack protection.


Djalal Harouni (9)
 procfs: use flags to deny or allow access to /proc/<pid>/$entry
 procfs: add pid_entry_access() for proper checks on /proc/<pid>/*
 procfs: add proc_read_from_buffer() and pid_entry_read() helpers
 procfs: improve /proc/<pid>/wchan protection
 procfs: improve /proc/<pid>/syscall protection
 procfs: add pid_seq_private struct to handle /proc/<pid>/{stat|stack}
 procfs: add pid_entry_show() helper to handle /proc/<pid>/{stat|stack}
 procfs: improve /proc/<pid>/stat protection
 procfs: improve /proc/<pid>/stack protection

 fs/proc/array.c    |  90 ++++++++++++++++++++++++++++++++++++++++---
 fs/proc/base.c     | 248 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++-----------
 fs/proc/generic.c  |  22 +++++++++++
 fs/proc/internal.h |  34 ++++++++++++++--
 4 files changed, 361 insertions(+), 33 deletions(-)

[PATCH 1/9] procfs: use flags to deny or allow access to /proc/<pid>/$entry

[Djalal Harouni]

Add the deny or allow flags, so we can perform proper permission checks
and set the result accordingly. These flags are needed in case we have
to cache the result of permission checks that are done during ->open()
time. Later during ->read(), we can decide to allow or deny the read().

The pid entries that need these flags are:
/proc/<pid>/stat
/proc/<pid>/wchan
/proc/<pid>/maps  (will be handled in next patches).

These files are world readable, userspace depend on that. To prevent
ASLR leaks and to avoid breaking userspace, we follow this scheme:

a) Perform permission checks during ->open()
b) Cache the result of a) and return success
c) Recheck the cached result during ->read()
d) If cached == PID_ENTRY_DENY:
   then we replace the sensitive fields with zeros, userspace won't
   break and sensitive fields are protected.

These flags are internal to /proc/<pid>/*

Signed-off-by: Djalal Harouni <tixxdz@opendz.org>
---
 fs/proc/internal.h | 9 +++++++++
 1 file changed, 9 insertions(+)

diff --git a/fs/proc/internal.h b/fs/proc/internal.h
index 3ab6d14..e696284 100644
--- a/fs/proc/internal.h
+++ b/fs/proc/internal.h
@@ -19,6 +19,15 @@ struct ctl_table_header;
 struct mempolicy;
 
 /*
+ * Flags used to deny or allow current to access /proc/<pid>/$entry
+ * after proper permission checks.
+ */
+enum {
+	PID_ENTRY_DENY	= 0,	/* Deny access */
+	PID_ENTRY_ALLOW	= 1,	/* Allow access */
+};
+
+/*
  * This is not completely implemented yet. The idea is to
  * create an in-memory tree (like the actual /proc filesystem
  * tree) of these proc_dir_entries, so that we can dynamically
-- 
1.7.11.7

[PATCH 2/9] procfs: add pid_entry_access() for proper checks on /proc/<pid>/*

[Djalal Harouni]

Add the helper pid_entry_access() to unify the permission checks during
->open()

This is a preparation patch.

Signed-off-by: Djalal Harouni <tixxdz@opendz.org>
---
 fs/proc/generic.c  | 22 ++++++++++++++++++++++
 fs/proc/internal.h |  2 ++
 2 files changed, 24 insertions(+)

diff --git a/fs/proc/generic.c b/fs/proc/generic.c
index b7f268e..98ed927 100644
--- a/fs/proc/generic.c
+++ b/fs/proc/generic.c
@@ -23,6 +23,7 @@
 #include <linux/bitops.h>
 #include <linux/spinlock.h>
 #include <linux/completion.h>
+#include <linux/ptrace.h>
 #include <asm/uaccess.h>
 
 #include "internal.h"
@@ -596,3 +597,24 @@ void *PDE_DATA(const struct inode *inode)
 	return __PDE_DATA(inode);
 }
 EXPORT_SYMBOL(PDE_DATA);
+
+int pid_entry_access(struct file *filp, unsigned int mode)
+{
+	int err = -ESRCH;
+	struct task_struct *task = get_proc_task(file_inode(filp));
+
+	if (!task)
+		return err;
+
+	err = mutex_lock_killable(&task->signal->cred_guard_mutex);
+	if (err)
+		goto out;
+
+	if (!ptrace_may_access(task, mode))
+		err = -EACCES;
+
+	mutex_unlock(&task->signal->cred_guard_mutex);
+out:
+	put_task_struct(task);
+	return err;
+}
diff --git a/fs/proc/internal.h b/fs/proc/internal.h
index e696284..4f828fa 100644
--- a/fs/proc/internal.h
+++ b/fs/proc/internal.h
@@ -202,6 +202,8 @@ static inline struct proc_dir_entry *pde_get(struct proc_dir_entry *pde)
 }
 extern void pde_put(struct proc_dir_entry *);
 
+extern int pid_entry_access(struct file *filp, unsigned int mode);
+
 /*
  * inode.c
  */
-- 
1.7.11.7

[PATCH 3/9] procfs: add proc_read_from_buffer() and pid_entry_read() helpers

[Djalal Harouni]

This patch is preparation, it adds a couple of helpers to read data and
to get the cached permission checks during that ->read().

Currently INF entries share the same code, they do not implement
specific ->open(), only ->read() coupled with callback calls. Doing
permission checks during ->open() will not work and will only disturb
the INF entries that do not need permission checks. Yes not all the INF
entries need checks, the ones that need protections are listed below:
/proc/<pid>/wchan
/proc/<pid>/syscall
/proc/<pid>/{auxv,io}  (will be handled in next patches).

So to have proper permission checks convert this INF entries to REG
entries and use their open() and read() handlers to implement these
checks. To achieve this we add the following helpers:

* proc_read_from_buffer() a wrapper around simple_read_from_buffer(), it
  makes sure that count <= PROC_BLOCK_SIZE (3*1024)

* pid_entry_read(): it will get a free page and pass it to the specific
  /proc/<pid>/$entry handler (callback). The handler is of the same
  types of the previous INF handlers, it will only receive an extra
  "permitted" argument that contains the cached permission check that
  was performed during ->open().

  The handler is of type:
  typedef int (*proc_read_fn_t)(char *page,
                               struct task_struct *task, int permitted);

So the converted code for example: /proc/pid/wchan will be:

  Before:
  static int proc_pid_wchan(struct task_struct *task, char *buffer)

  After:
  static int proc_pid_wchan(char *buffer,
			    struct task_struct *task, int permitted)

The extra "permitted" can be used by the handler to allow/deny reads.

And the whole read() of /proc/pid/wchan will be:

static ssize_t wchan_read(struct file *file, char __user *buf,
			  size_t count, loff_t *ppos)
{
	ssize_t length;
	unsigned long page = 0UL;

	length = pid_entry_read(file, &page, proc_pid_wchan);
	if (length >= 0) {
		length = proc_read_from_buffer(buf, count, ppos,
					       (char *)page, length);
		free_page(page);
	}

	return length;
}

Signed-off-by: Djalal Harouni <tixxdz@opendz.org>
---
 fs/proc/base.c     | 53 +++++++++++++++++++++++++++++++++++++++++++++++------
 fs/proc/internal.h |  3 +++
 2 files changed, 50 insertions(+), 6 deletions(-)

diff --git a/fs/proc/base.c b/fs/proc/base.c
index e442784..efe2a11 100644
--- a/fs/proc/base.c
+++ b/fs/proc/base.c
@@ -139,6 +139,50 @@ struct pid_entry {
 		NULL, &proc_single_file_operations,	\
 		{ .proc_show = show } )
 
+/* 4K page size but our output routines use some slack for overruns */
+#define PROC_BLOCK_SIZE	(3*1024)
+
+static ssize_t proc_read_from_buffer(void __user *to, size_t count,
+				     loff_t *ppos, const void *from,
+				     size_t available)
+{
+	if (count > PROC_BLOCK_SIZE)
+		count = PROC_BLOCK_SIZE;
+
+	return simple_read_from_buffer(to, count, ppos, from, available);
+}
+
+static ssize_t pid_entry_read(struct file *file, unsigned long *page,
+			      proc_read_fn_t proc_read)
+{
+	unsigned long addr;
+	ssize_t length = -ESRCH;
+	struct task_struct *task;
+	struct inode *inode = file_inode(file);
+	int permitted = (unsigned long)(void *)file->private_data;
+
+	task = get_proc_task(inode);
+	if (!task)
+		goto out_no_task;
+
+	length = -ENOMEM;
+	addr = __get_free_page(GFP_TEMPORARY);
+	if (!addr)
+		goto out;
+
+	length = proc_read((char *)addr, task, permitted);
+	if (length < 0) {
+		free_page(addr);
+		goto out;
+	}
+
+	*page = addr;
+out:
+	put_task_struct(task);
+out_no_task:
+	return length;
+}
+
 /*
  * Count the number of hardlinks for the pid_entry table, excluding the .
  * and .. links.
@@ -598,8 +642,6 @@ static const struct inode_operations proc_def_inode_operations = {
 	.setattr	= proc_setattr,
 };
 
-#define PROC_BLOCK_SIZE	(3*1024)		/* 4K page size but our output routines use some slack for overruns */
-
 static ssize_t proc_info_read(struct file * file, char __user * buf,
 			  size_t count, loff_t *ppos)
 {
@@ -612,9 +654,6 @@ static ssize_t proc_info_read(struct file * file, char __user * buf,
 	if (!task)
 		goto out_no_task;
 
-	if (count > PROC_BLOCK_SIZE)
-		count = PROC_BLOCK_SIZE;
-
 	length = -ENOMEM;
 	if (!(page = __get_free_page(GFP_TEMPORARY)))
 		goto out;
@@ -622,7 +661,9 @@ static ssize_t proc_info_read(struct file * file, char __user * buf,
 	length = PROC_I(inode)->op.proc_read(task, (char*)page);
 
 	if (length >= 0)
-		length = simple_read_from_buffer(buf, count, ppos, (char *)page, length);
+		length = proc_read_from_buffer(buf, count, ppos,
+					       (char *)page, length);
+
 	free_page(page);
 out:
 	put_task_struct(task);
diff --git a/fs/proc/internal.h b/fs/proc/internal.h
index 4f828fa..f5c452c 100644
--- a/fs/proc/internal.h
+++ b/fs/proc/internal.h
@@ -78,6 +78,9 @@ struct proc_inode {
 	struct inode vfs_inode;
 };
 
+typedef int (*proc_read_fn_t)(char *page,
+			      struct task_struct *task, int permitted);
+
 /*
  * General functions
  */
-- 
1.7.11.7

[PATCH 4/9] procfs: improve /proc/<pid>/wchan protection

[Djalal Harouni]

Convert wchan from an INF entry to a REG one. This way we can perform
and cache the permission checks during ->open().

The checks are only cached, since /proc/<pid>/wchan is world readable,
and it needs permissions only when returning an address, returning the
symbol name is not subject to permission checks, so do not change this.

With this logic userspace should continue to work without any problem,
only cases where users are trying to play tricks or something
sophisticated is trying to use privileged programs to disclose sensitive
data will notice.

Suggested-by: Alexey Dobriyan <adobriyan@gmail.com>
Signed-off-by: Djalal Harouni <tixxdz@opendz.org>
---
 fs/proc/base.c | 55 +++++++++++++++++++++++++++++++++++++++++++++++++------
 1 file changed, 49 insertions(+), 6 deletions(-)

diff --git a/fs/proc/base.c b/fs/proc/base.c
index efe2a11..ef35544 100644
--- a/fs/proc/base.c
+++ b/fs/proc/base.c
@@ -273,21 +273,64 @@ static int proc_pid_auxv(struct task_struct *task, char *buffer)
  * Provides a wchan file via kallsyms in a proper one-value-per-file format.
  * Returns the resolved symbol.  If that fails, simply return the address.
  */
-static int proc_pid_wchan(struct task_struct *task, char *buffer)
+static int wchan_open(struct inode *inode, struct file *filp)
+{
+	/* we only cache the result for wchan entry */
+	if (pid_entry_access(filp, PTRACE_MODE_READ))
+		filp->private_data = (void *)(unsigned long)PID_ENTRY_DENY;
+	else
+		filp->private_data = (void *)(unsigned long)PID_ENTRY_ALLOW;
+
+	return 0;
+}
+
+static int proc_pid_wchan(char *buffer,
+			  struct task_struct *task, int permitted)
 {
 	unsigned long wchan;
 	char symname[KSYM_NAME_LEN];
 
 	wchan = get_wchan(task);
 
-	if (lookup_symbol_name(wchan, symname) < 0)
-		if (!ptrace_may_access(task, PTRACE_MODE_READ))
+	if (lookup_symbol_name(wchan, symname) < 0) {
+		if (!permitted)
+			return 0;
+
+		/* Update only if access was granted during ->open */
+		if (!mutex_lock_killable(&task->signal->cred_guard_mutex)) {
+			permitted = ptrace_may_access(task, PTRACE_MODE_READ);
+			mutex_unlock(&task->signal->cred_guard_mutex);
+		}
+
+		if (!permitted)
 			return 0;
 		else
 			return sprintf(buffer, "%lu", wchan);
-	else
+	} else
 		return sprintf(buffer, "%s", symname);
 }
+
+static ssize_t wchan_read(struct file *file, char __user *buf,
+			  size_t count, loff_t *ppos)
+{
+	ssize_t length;
+	unsigned long page = 0UL;
+
+	length = pid_entry_read(file, &page, proc_pid_wchan);
+	if (length >= 0) {
+		length = proc_read_from_buffer(buf, count, ppos,
+					       (char *)page, length);
+		free_page(page);
+	}
+
+	return length;
+}
+
+static const struct file_operations proc_pid_wchan_operations = {
+	.open   = wchan_open,
+	.read   = wchan_read,
+	.llseek = generic_file_llseek,
+};
 #endif /* CONFIG_KALLSYMS */
 
 static int lock_trace(struct task_struct *task)
@@ -2631,7 +2674,7 @@ static const struct pid_entry tgid_base_stuff[] = {
 	DIR("attr",       S_IRUGO|S_IXUGO, proc_attr_dir_inode_operations, proc_attr_dir_operations),
 #endif
 #ifdef CONFIG_KALLSYMS
-	INF("wchan",      S_IRUGO, proc_pid_wchan),
+	REG("wchan",      S_IRUGO, proc_pid_wchan_operations),
 #endif
 #ifdef CONFIG_STACKTRACE
 	ONE("stack",      S_IRUSR, proc_pid_stack),
@@ -2967,7 +3010,7 @@ static const struct pid_entry tid_base_stuff[] = {
 	DIR("attr",      S_IRUGO|S_IXUGO, proc_attr_dir_inode_operations, proc_attr_dir_operations),
 #endif
 #ifdef CONFIG_KALLSYMS
-	INF("wchan",     S_IRUGO, proc_pid_wchan),
+	REG("wchan",     S_IRUGO, proc_pid_wchan_operations),
 #endif
 #ifdef CONFIG_STACKTRACE
 	ONE("stack",      S_IRUSR, proc_pid_stack),
-- 
1.7.11.7

[PATCH 5/9] procfs: improve /proc/<pid>/syscall protection

[Djalal Harouni]

Convert syscall from an INF entry to a REG one. This way we can perform
and cache the permission checks during ->open().

The ptrace capability is only cached, it will be re-checked during
->read(). If the opener did not have enough privileges then fail.

Signed-off-by: Djalal Harouni <tixxdz@opendz.org>
---
 fs/proc/base.c | 56 ++++++++++++++++++++++++++++++++++++++++++++++++++++----
 1 file changed, 52 insertions(+), 4 deletions(-)

diff --git a/fs/proc/base.c b/fs/proc/base.c
index ef35544..f0ce94a 100644
--- a/fs/proc/base.c
+++ b/fs/proc/base.c
@@ -183,6 +183,18 @@ out_no_task:
 	return length;
 }
 
+static int pid_entry_attach(struct file *filp)
+{
+	int ret = pid_entry_access(filp, PTRACE_MODE_ATTACH);
+
+	if (ret)
+		filp->private_data = (void *)(unsigned long)PID_ENTRY_DENY;
+	else
+		filp->private_data = (void *)(unsigned long)PID_ENTRY_ALLOW;
+
+	return ret;
+}
+
 /*
  * Count the number of hardlinks for the pid_entry table, excluding the .
  * and .. links.
@@ -574,11 +586,25 @@ static int proc_pid_limits(struct task_struct *task, char *buffer)
 }
 
 #ifdef CONFIG_HAVE_ARCH_TRACEHOOK
-static int proc_pid_syscall(struct task_struct *task, char *buffer)
+static int syscall_open(struct inode *inode, struct file *filp)
+{
+	/* we only cache the result */
+	pid_entry_attach(filp);
+
+	return 0;
+}
+
+static int proc_pid_syscall(char *buffer,
+			    struct task_struct *task, int permitted)
 {
 	long nr;
+	int res = -EPERM;
 	unsigned long args[6], sp, pc;
-	int res = lock_trace(task);
+
+	if (!permitted)
+		return res;
+
+	res = lock_trace(task);
 	if (res)
 		return res;
 
@@ -595,6 +621,28 @@ static int proc_pid_syscall(struct task_struct *task, char *buffer)
 	unlock_trace(task);
 	return res;
 }
+
+static ssize_t syscall_read(struct file *file, char __user *buf,
+			    size_t count, loff_t *ppos)
+{
+	ssize_t length;
+	unsigned long page = 0UL;
+
+	length = pid_entry_read(file, &page, proc_pid_syscall);
+	if (length >= 0) {
+		length = proc_read_from_buffer(buf, count, ppos,
+					       (char *)page, length);
+		free_page(page);
+	}
+
+	return length;
+}
+
+static const struct file_operations proc_pid_syscall_operations = {
+	.open	= syscall_open,
+	.read	= syscall_read,
+	.llseek	= generic_file_llseek,
+};
 #endif /* CONFIG_HAVE_ARCH_TRACEHOOK */
 
 /************************************************************************/
@@ -2649,7 +2697,7 @@ static const struct pid_entry tgid_base_stuff[] = {
 #endif
 	REG("comm",      S_IRUGO|S_IWUSR, proc_pid_set_comm_operations),
 #ifdef CONFIG_HAVE_ARCH_TRACEHOOK
-	INF("syscall",    S_IRUSR, proc_pid_syscall),
+	REG("syscall",    S_IRUSR, proc_pid_syscall_operations),
 #endif
 	INF("cmdline",    S_IRUGO, proc_pid_cmdline),
 	ONE("stat",       S_IRUGO, proc_tgid_stat),
@@ -2983,7 +3031,7 @@ static const struct pid_entry tid_base_stuff[] = {
 #endif
 	REG("comm",      S_IRUGO|S_IWUSR, proc_pid_set_comm_operations),
 #ifdef CONFIG_HAVE_ARCH_TRACEHOOK
-	INF("syscall",   S_IRUSR, proc_pid_syscall),
+	REG("syscall",   S_IRUSR, proc_pid_syscall_operations),
 #endif
 	INF("cmdline",   S_IRUGO, proc_pid_cmdline),
 	ONE("stat",      S_IRUGO, proc_tid_stat),
-- 
1.7.11.7

[PATCH 6/9] procfs: add pid_seq_private struct to handle /proc/<pid>/{stat|stack}

[Djalal Harouni]

This patch is preparation to handle sensitive ONE entries:
/proc/<pid>/stat
/proc/<pid>/stack

These files use sequence iterators and we want to keep that logic, and
their internal handler semantics.

The sequence iterators stock the inode in the seq_file->private field,
so in order to keep the semantic and to make the cached permission
checks available during ->read(), we add the 'pid_seq_private' struct
that contains both the inode and the cached permission. It will be the
one referenced in the seq_file->private.

This way the internal handlers of /proc/<pid>/{stat|stack} wont change.

Signed-off-by: Djalal Harouni <tixxdz@opendz.org>
---
 fs/proc/internal.h | 11 +++++++++++
 1 file changed, 11 insertions(+)

diff --git a/fs/proc/internal.h b/fs/proc/internal.h
index f5c452c..f28e4f01 100644
--- a/fs/proc/internal.h
+++ b/fs/proc/internal.h
@@ -78,6 +78,17 @@ struct proc_inode {
 	struct inode vfs_inode;
 };
 
+/*
+ * Struct used by some /proc/<pid>/$entries that use sequence
+ * iterators.
+ * This struct will be saved in seq_file->private so seq handlers
+ * can access the inode and the cached permission checks of ->open().
+ */
+struct pid_seq_private {
+	int permitted;
+	struct inode *inode;
+};
+
 typedef int (*proc_read_fn_t)(char *page,
 			      struct task_struct *task, int permitted);
 
-- 
1.7.11.7

[PATCH 7/9] procfs: add pid_entry_show() helper to handle /proc/<pid>/{stat|stack}

[Djalal Harouni]

This patch is preparation, it adds the pid_entry_show() helper function
which will be used to handle /proc/<pid>/{stat|stack} and to call their
internal handlers. This logic allows these files to continue to use
sequence iterators.

Currently ONE entries share the same code, and they do not perform
permission checks during ->open(). So adding these checks during open
will only disturb the ONE entries that do not need permission checks.

The ONE entries that need checks are:
/proc/<pid>/stat
/proc/<pid>/stack

So to have proper permission checks convert this ONE entries to REG
entries and use their open() and read() handlers to implement these
checks. To achieve this we add the following helper:

* pid_entry_show(): which will get the necessary info and passe it to the
appropriate handlers. The handlers are of the same type of the previous
ONE entries, so nothing changes for /proc/<pid>/{stat|stack} handlers.

The handler type is:
typedef int (*proc_show_fn_t)(struct seq_file *seq, struct pid_namespace *ns,
			      struct pid *pid, struct task_struct *task);

Which is the same type of:

/proc/<pid>/stat:
int proc_tid_stat(struct seq_file *m, struct pid_namespace *ns,
		  struct pid *pid, struct task_struct *task)

/proc/<pid>/stack:
static int proc_pid_stack(struct seq_file *m, struct pid_namespace *ns,
			  struct pid *pid, struct task_struct *task)

The 'seq_file->private' will contain the struct 'pid_seq_private' which
contains the inode and the cached permission checks.

Signed-off-by: Djalal Harouni <tixxdz@opendz.org>
---
 fs/proc/base.c     | 21 +++++++++++++++++++++
 fs/proc/internal.h |  3 +++
 2 files changed, 24 insertions(+)

diff --git a/fs/proc/base.c b/fs/proc/base.c
index f0ce94a..b40345b 100644
--- a/fs/proc/base.c
+++ b/fs/proc/base.c
@@ -183,6 +183,27 @@ out_no_task:
 	return length;
 }
 
+int pid_entry_show(struct seq_file *seq, proc_show_fn_t proc_show)
+{
+	int ret = -ESRCH;
+	struct pid *pid;
+	struct task_struct *task;
+	struct pid_namespace *ns;
+	struct pid_seq_private *priv = seq->private;
+	struct inode *inode = priv->inode;
+
+	ns = inode->i_sb->s_fs_info;
+	pid = proc_pid(inode);
+	task = get_pid_task(pid, PIDTYPE_PID);
+	if (!task)
+		return ret;
+
+	ret = proc_show(seq, ns, pid, task);
+
+	put_task_struct(task);
+	return ret;
+}
+
 static int pid_entry_attach(struct file *filp)
 {
 	int ret = pid_entry_access(filp, PTRACE_MODE_ATTACH);
diff --git a/fs/proc/internal.h b/fs/proc/internal.h
index f28e4f01..3c4bd73 100644
--- a/fs/proc/internal.h
+++ b/fs/proc/internal.h
@@ -91,6 +91,8 @@ struct pid_seq_private {
 
 typedef int (*proc_read_fn_t)(char *page,
 			      struct task_struct *task, int permitted);
+typedef int (*proc_show_fn_t)(struct seq_file *seq, struct pid_namespace *ns,
+			      struct pid *pid, struct task_struct *task);
 
 /*
  * General functions
@@ -191,6 +193,7 @@ extern int pid_delete_dentry(const struct dentry *);
 extern int proc_pid_readdir(struct file *, struct dir_context *);
 extern struct dentry *proc_pid_lookup(struct inode *, struct dentry *, unsigned int);
 extern loff_t mem_lseek(struct file *, loff_t, int);
+extern int pid_entry_show(struct seq_file *seq, proc_show_fn_t proc_show);
 
 /* Lookups */
 typedef int instantiate_t(struct inode *, struct dentry *,
-- 
1.7.11.7

[PATCH 8/9] procfs: improve /proc/<pid>/stat protection

[Djalal Harouni]

Convert wchan from an INF entry to a REG one. This way we can perform
and cache the permission checks during ->open(). We make sure that the
/proc/<pid>/stat will continue to use sequence iterators, in fact this
patch do not affect the logic of /proc/<pid>/stat, it only makes the
cached permission checks available to that handler.

The checks are only cached, since /proc/<pid>/stat is world readable,
and only a small subset of its fields need protections, all the other
fields are world readable. So in order to not break userspace, we cache
the permission checks during ->open() and we re-check during ->read()
and pad sensitive fields with zeros if the reader did not have enough
privileges.

With this logic userspace should continue to work without any problem,
only cases where users are trying to play tricks or something
sophisticated is trying to use privileged programs to disclose sensitive
data will notice that fields are zeroed.

Signed-off-by: Djalal Harouni <tixxdz@opendz.org>
---
 fs/proc/array.c    | 90 ++++++++++++++++++++++++++++++++++++++++++++++++++----
 fs/proc/base.c     |  4 +--
 fs/proc/internal.h |  6 ++--
 3 files changed, 88 insertions(+), 12 deletions(-)

diff --git a/fs/proc/array.c b/fs/proc/array.c
index 64db2bc..84f588f 100644
--- a/fs/proc/array.c
+++ b/fs/proc/array.c
@@ -387,7 +387,6 @@ static int do_task_stat(struct seq_file *m, struct pid_namespace *ns,
 	char state;
 	pid_t ppid = 0, pgid = -1, sid = -1;
 	int num_threads = 0;
-	int permitted;
 	struct mm_struct *mm;
 	unsigned long long start_time;
 	unsigned long cmin_flt = 0, cmaj_flt = 0;
@@ -397,10 +396,22 @@ static int do_task_stat(struct seq_file *m, struct pid_namespace *ns,
 	unsigned long rsslim = 0;
 	char tcomm[sizeof(task->comm)];
 	unsigned long flags;
+	struct pid_seq_private *priv = m->private;
+	int permitted = priv->permitted;
 
 	state = *get_task_state(task);
 	vsize = eip = esp = 0;
-	permitted = ptrace_may_access(task, PTRACE_MODE_READ | PTRACE_MODE_NOAUDIT);
+
+	if (permitted) {
+		/* Update only if access was granted during ->open */
+		unsigned int mode = PTRACE_MODE_READ | PTRACE_MODE_NOAUDIT;
+
+		if (!mutex_lock_killable(&task->signal->cred_guard_mutex)) {
+			permitted = ptrace_may_access(task, mode);
+			mutex_unlock(&task->signal->cred_guard_mutex);
+		}
+	}
+
 	mm = get_task_mm(task);
 	if (mm) {
 		vsize = task_vsize(mm);
@@ -550,18 +561,85 @@ static int do_task_stat(struct seq_file *m, struct pid_namespace *ns,
 	return 0;
 }
 
-int proc_tid_stat(struct seq_file *m, struct pid_namespace *ns,
-			struct pid *pid, struct task_struct *task)
+static int proc_show_tid_stat(struct seq_file *m, struct pid_namespace *ns,
+			      struct pid *pid, struct task_struct *task)
 {
 	return do_task_stat(m, ns, pid, task, 0);
 }
 
-int proc_tgid_stat(struct seq_file *m, struct pid_namespace *ns,
-			struct pid *pid, struct task_struct *task)
+static int proc_tid_stat(struct seq_file *m, void *v)
+{
+	return pid_entry_show(m, proc_show_tid_stat);
+}
+
+static int do_proc_stat_open(struct inode *inode, struct file *filp,
+			     int (*proc_stat)(struct seq_file *, void *))
+{
+	int ret = -ENOMEM;
+	struct pid_seq_private *priv;
+
+	priv = kzalloc(sizeof(*priv), GFP_KERNEL);
+	if (!priv)
+		return ret;
+
+	priv->inode = inode;
+	if (pid_entry_access(filp, PTRACE_MODE_READ | PTRACE_MODE_NOAUDIT))
+		priv->permitted = PID_ENTRY_DENY;
+	else
+		priv->permitted = PID_ENTRY_ALLOW;
+
+	ret = single_open(filp, proc_stat, priv);
+	if (ret)
+		kfree(priv);
+
+	return ret;
+}
+
+static int proc_tid_stat_open(struct inode *inode, struct file *filp)
+{
+	return do_proc_stat_open(inode, filp, proc_tid_stat);
+}
+
+static int proc_stat_release(struct inode *inode, struct file *filp)
+{
+	struct seq_file *seq = filp->private_data;
+
+	kfree(seq->private);
+	seq->private = NULL;
+
+	return single_release(inode, filp);
+}
+
+const struct file_operations proc_tid_stat_operations = {
+	.open		= proc_tid_stat_open,
+	.read           = seq_read,
+	.llseek         = seq_lseek,
+	.release        = proc_stat_release,
+};
+
+static int proc_show_tgid_stat(struct seq_file *m, struct pid_namespace *ns,
+			       struct pid *pid, struct task_struct *task)
 {
 	return do_task_stat(m, ns, pid, task, 1);
 }
 
+static int proc_tgid_stat(struct seq_file *m, void *v)
+{
+	return pid_entry_show(m, proc_show_tgid_stat);
+}
+
+static int proc_tgid_stat_open(struct inode *inode, struct file *filp)
+{
+	return do_proc_stat_open(inode, filp, proc_tgid_stat);
+}
+
+const struct file_operations proc_tgid_stat_operations = {
+	.open		= proc_tgid_stat_open,
+	.read           = seq_read,
+	.llseek         = seq_lseek,
+	.release        = proc_stat_release,
+};
+
 int proc_pid_statm(struct seq_file *m, struct pid_namespace *ns,
 			struct pid *pid, struct task_struct *task)
 {
diff --git a/fs/proc/base.c b/fs/proc/base.c
index b40345b..d98ce15 100644
--- a/fs/proc/base.c
+++ b/fs/proc/base.c
@@ -2721,7 +2721,7 @@ static const struct pid_entry tgid_base_stuff[] = {
 	REG("syscall",    S_IRUSR, proc_pid_syscall_operations),
 #endif
 	INF("cmdline",    S_IRUGO, proc_pid_cmdline),
-	ONE("stat",       S_IRUGO, proc_tgid_stat),
+	REG("stat",       S_IRUGO, proc_tgid_stat_operations),
 	ONE("statm",      S_IRUGO, proc_pid_statm),
 	REG("maps",       S_IRUGO, proc_pid_maps_operations),
 #ifdef CONFIG_NUMA
@@ -3055,7 +3055,7 @@ static const struct pid_entry tid_base_stuff[] = {
 	REG("syscall",   S_IRUSR, proc_pid_syscall_operations),
 #endif
 	INF("cmdline",   S_IRUGO, proc_pid_cmdline),
-	ONE("stat",      S_IRUGO, proc_tid_stat),
+	REG("stat",      S_IRUGO, proc_tid_stat_operations),
 	ONE("statm",     S_IRUGO, proc_pid_statm),
 	REG("maps",      S_IRUGO, proc_tid_maps_operations),
 #ifdef CONFIG_CHECKPOINT_RESTORE
diff --git a/fs/proc/internal.h b/fs/proc/internal.h
index 3c4bd73..fc2f59d 100644
--- a/fs/proc/internal.h
+++ b/fs/proc/internal.h
@@ -171,11 +171,9 @@ out:
  * array.c
  */
 extern const struct file_operations proc_tid_children_operations;
+extern const struct file_operations proc_tid_stat_operations;
+extern const struct file_operations proc_tgid_stat_operations;
 
-extern int proc_tid_stat(struct seq_file *, struct pid_namespace *,
-			 struct pid *, struct task_struct *);
-extern int proc_tgid_stat(struct seq_file *, struct pid_namespace *,
-			  struct pid *, struct task_struct *);
 extern int proc_pid_status(struct seq_file *, struct pid_namespace *,
 			   struct pid *, struct task_struct *);
 extern int proc_pid_statm(struct seq_file *, struct pid_namespace *,
-- 
1.7.11.7

[PATCH 9/9] procfs: improve /proc/<pid>/stack protection

[Djalal Harouni]

Convert stack from an INF entry to a REG one. This way we can perform
and cache the permission checks during ->open(). We make sure that
/proc/<pid>/stack will continue to use sequence iterators.

The ptrace capability is only cached, it will be re-checked during
->read(). If the opener did not have enough privileges then fail.

Signed-off-by: Djalal Harouni <tixxdz@opendz.org>
---
 fs/proc/base.c | 59 +++++++++++++++++++++++++++++++++++++++++++++++++++++-----
 1 file changed, 54 insertions(+), 5 deletions(-)

diff --git a/fs/proc/base.c b/fs/proc/base.c
index d98ce15..6786878 100644
--- a/fs/proc/base.c
+++ b/fs/proc/base.c
@@ -387,14 +387,19 @@ static void unlock_trace(struct task_struct *task)
 
 #define MAX_STACK_TRACE_DEPTH	64
 
-static int proc_pid_stack(struct seq_file *m, struct pid_namespace *ns,
-			  struct pid *pid, struct task_struct *task)
+static int proc_show_pid_stack(struct seq_file *m, struct pid_namespace *ns,
+			       struct pid *pid, struct task_struct *task)
 {
 	struct stack_trace trace;
 	unsigned long *entries;
-	int err;
+	struct pid_seq_private *priv = m->private;
+	int permitted = priv->permitted;
+	int err = -EPERM;
 	int i;
 
+	if (!permitted)
+		return err;
+
 	entries = kmalloc(MAX_STACK_TRACE_DEPTH * sizeof(*entries), GFP_KERNEL);
 	if (!entries)
 		return -ENOMEM;
@@ -418,6 +423,50 @@ static int proc_pid_stack(struct seq_file *m, struct pid_namespace *ns,
 
 	return err;
 }
+
+static int proc_pid_stack(struct seq_file *m, void *v)
+{
+	return pid_entry_show(m, proc_show_pid_stack);
+}
+
+static int stack_open(struct inode *inode, struct file *filp)
+{
+	int ret = -ENOMEM;
+	struct pid_seq_private *priv;
+
+	priv = kzalloc(sizeof(*priv), GFP_KERNEL);
+	if (!priv)
+		return ret;
+
+	priv->inode = inode;
+	if (pid_entry_access(filp, PTRACE_MODE_ATTACH))
+		priv->permitted = PID_ENTRY_DENY;
+	else
+		priv->permitted = PID_ENTRY_ALLOW;
+
+	ret = single_open(filp, proc_pid_stack, priv);
+	if (ret)
+		kfree(priv);
+
+	return ret;
+}
+
+static int stack_release(struct inode *inode, struct file *filp)
+{
+	struct seq_file *seq = filp->private_data;
+
+	kfree(seq->private);
+	seq->private = NULL;
+
+	return single_release(inode, filp);
+}
+
+static const struct file_operations proc_pid_stack_operations = {
+	.open		= stack_open,
+	.read           = seq_read,
+	.llseek         = seq_lseek,
+	.release        = stack_release,
+};
 #endif
 
 #ifdef CONFIG_SCHEDSTATS
@@ -2746,7 +2795,7 @@ static const struct pid_entry tgid_base_stuff[] = {
 	REG("wchan",      S_IRUGO, proc_pid_wchan_operations),
 #endif
 #ifdef CONFIG_STACKTRACE
-	ONE("stack",      S_IRUSR, proc_pid_stack),
+	REG("stack",      S_IRUSR, proc_pid_stack_operations),
 #endif
 #ifdef CONFIG_SCHEDSTATS
 	INF("schedstat",  S_IRUGO, proc_pid_schedstat),
@@ -3082,7 +3131,7 @@ static const struct pid_entry tid_base_stuff[] = {
 	REG("wchan",     S_IRUGO, proc_pid_wchan_operations),
 #endif
 #ifdef CONFIG_STACKTRACE
-	ONE("stack",      S_IRUSR, proc_pid_stack),
+	REG("stack",      S_IRUSR, proc_pid_stack_operations),
 #endif
 #ifdef CONFIG_SCHEDSTATS
 	INF("schedstat", S_IRUGO, proc_pid_schedstat),
-- 
1.7.11.7

Re: [PATCH 1/9] procfs: use flags to deny or allow access to /proc/<pid>/$entry

[Andy Lutomirski]

On Mon, May 26, 2014 at 6:27 AM, Djalal Harouni <tixxdz@opendz.org> wrote:
> Add the deny or allow flags, so we can perform proper permission checks
> and set the result accordingly. These flags are needed in case we have
> to cache the result of permission checks that are done during ->open()
> time. Later during ->read(), we can decide to allow or deny the read().
>
> The pid entries that need these flags are:
> /proc/<pid>/stat
> /proc/<pid>/wchan
> /proc/<pid>/maps  (will be handled in next patches).
>
> These files are world readable, userspace depend on that. To prevent
> ASLR leaks and to avoid breaking userspace, we follow this scheme:
>
> a) Perform permission checks during ->open()
> b) Cache the result of a) and return success
> c) Recheck the cached result during ->read()

Why is (c) needed?

>
>  /*
> + * Flags used to deny or allow current to access /proc/<pid>/$entry
> + * after proper permission checks.
> + */
> +enum {
> +       PID_ENTRY_DENY  = 0,    /* Deny access */
> +       PID_ENTRY_ALLOW = 1,    /* Allow access */
> +};

I think this would be less alarming if this were:

#define PID_ENTRY_DENY ((void *)1UL)
#define PID_ENTRY_ALLOW ((void *)2UL)

Also, I don't like DENY and ALLOW.  It's not denying and allowing.
How about PID_ENTRY_OPENER_MAY_PTRACE and
PID_ENTRY_OPENER_MAY_NOT_PTRACE?

--Andy

Re: [PATCH 2/9] procfs: add pid_entry_access() for proper checks on /proc/<pid>/*

[Andy Lutomirski]

On Mon, May 26, 2014 at 6:27 AM, Djalal Harouni <tixxdz@opendz.org> wrote:
> Add the helper pid_entry_access() to unify the permission checks during
> ->open()
>
> This is a preparation patch.
>
> Signed-off-by: Djalal Harouni <tixxdz@opendz.org>
> ---
>  fs/proc/generic.c  | 22 ++++++++++++++++++++++
>  fs/proc/internal.h |  2 ++
>  2 files changed, 24 insertions(+)
>
> diff --git a/fs/proc/generic.c b/fs/proc/generic.c
> index b7f268e..98ed927 100644
> --- a/fs/proc/generic.c
> +++ b/fs/proc/generic.c
> @@ -23,6 +23,7 @@
>  #include <linux/bitops.h>
>  #include <linux/spinlock.h>
>  #include <linux/completion.h>
> +#include <linux/ptrace.h>
>  #include <asm/uaccess.h>
>
>  #include "internal.h"
> @@ -596,3 +597,24 @@ void *PDE_DATA(const struct inode *inode)
>         return __PDE_DATA(inode);
>  }
>  EXPORT_SYMBOL(PDE_DATA);
> +
> +int pid_entry_access(struct file *filp, unsigned int mode)

pid_entry_may_ptrace, perhaps?

--Andy

Re: [PATCH 3/9] procfs: add proc_read_from_buffer() and pid_entry_read() helpers

[Andy Lutomirski]

On Mon, May 26, 2014 at 6:27 AM, Djalal Harouni <tixxdz@opendz.org> wrote:
> This patch is preparation, it adds a couple of helpers to read data and
> to get the cached permission checks during that ->read().
>
> Currently INF entries share the same code, they do not implement
> specific ->open(), only ->read() coupled with callback calls. Doing
> permission checks during ->open() will not work and will only disturb
> the INF entries that do not need permission checks. Yes not all the INF
> entries need checks, the ones that need protections are listed below:
> /proc/<pid>/wchan
> /proc/<pid>/syscall
> /proc/<pid>/{auxv,io}  (will be handled in next patches).
>
> So to have proper permission checks convert this INF entries to REG
> entries and use their open() and read() handlers to implement these
> checks. To achieve this we add the following helpers:
>
> * proc_read_from_buffer() a wrapper around simple_read_from_buffer(), it
>   makes sure that count <= PROC_BLOCK_SIZE (3*1024)
>
> * pid_entry_read(): it will get a free page and pass it to the specific
>   /proc/<pid>/$entry handler (callback). The handler is of the same
>   types of the previous INF handlers, it will only receive an extra
>   "permitted" argument that contains the cached permission check that
>   was performed during ->open().
>
>   The handler is of type:
>   typedef int (*proc_read_fn_t)(char *page,
>                                struct task_struct *task, int permitted);

[...]

This strikes me as *way* too complicated.  Why not just change
proc_info_file_operations to *always* cache may_ptrace on open?

FWIW, it's been awhile: was anything actually wrong with using f_cred,
other than the fact that it would have required adjusting the LSM
hooks?

Admittedly, I don't see anything fundamentally wrong with caching
may_ptrace on open as long as revoke in in the cards eventually, but
it's plausible that a good f_cred-based implementation would make
revoke less necessary.

--Andy

Re: [PATCH 6/9] procfs: add pid_seq_private struct to handle /proc/<pid>/{stat|stack}

[Andy Lutomirski]

On Mon, May 26, 2014 at 6:27 AM, Djalal Harouni <tixxdz@opendz.org> wrote:
> This patch is preparation to handle sensitive ONE entries:
> /proc/<pid>/stat
> /proc/<pid>/stack
>
> These files use sequence iterators and we want to keep that logic, and
> their internal handler semantics.
>
> The sequence iterators stock the inode in the seq_file->private field,
> so in order to keep the semantic and to make the cached permission
> checks available during ->read(), we add the 'pid_seq_private' struct
> that contains both the inode and the cached permission. It will be the
> one referenced in the seq_file->private.
>
> This way the internal handlers of /proc/<pid>/{stat|stack} wont change.
>
> Signed-off-by: Djalal Harouni <tixxdz@opendz.org>
> ---
>  fs/proc/internal.h | 11 +++++++++++
>  1 file changed, 11 insertions(+)
>
> diff --git a/fs/proc/internal.h b/fs/proc/internal.h
> index f5c452c..f28e4f01 100644
> --- a/fs/proc/internal.h
> +++ b/fs/proc/internal.h
> @@ -78,6 +78,17 @@ struct proc_inode {
>         struct inode vfs_inode;
>  };
>
> +/*
> + * Struct used by some /proc/<pid>/$entries that use sequence
> + * iterators.
> + * This struct will be saved in seq_file->private so seq handlers
> + * can access the inode and the cached permission checks of ->open().
> + */
> +struct pid_seq_private {
> +       int permitted;
> +       struct inode *inode;
> +};
> +
>  typedef int (*proc_read_fn_t)(char *page,
>                               struct task_struct *task, int permitted);

int opener_may_ptrace, please.

Re: [PATCH 1/9] procfs: use flags to deny or allow access to /proc/<pid>/$entry

[Djalal Harouni]

On Mon, May 26, 2014 at 09:57:16AM -0700, Andy Lutomirski wrote:
> On Mon, May 26, 2014 at 6:27 AM, Djalal Harouni <tixxdz@opendz.org> wrote:
> > Add the deny or allow flags, so we can perform proper permission checks
> > and set the result accordingly. These flags are needed in case we have
> > to cache the result of permission checks that are done during ->open()
> > time. Later during ->read(), we can decide to allow or deny the read().
> >
> > The pid entries that need these flags are:
> > /proc/<pid>/stat
> > /proc/<pid>/wchan
> > /proc/<pid>/maps  (will be handled in next patches).
> >
> > These files are world readable, userspace depend on that. To prevent
> > ASLR leaks and to avoid breaking userspace, we follow this scheme:
> >
> > a) Perform permission checks during ->open()
> > b) Cache the result of a) and return success
> > c) Recheck the cached result during ->read()
> 
> Why is (c) needed?
In order to not break these entries, some of them are world readable.

So we perform the re-check that *single* cached integer, in order to
allow access for the non-sensitive, and block or pad with zeros the
sensitive.


> >
> >  /*
> > + * Flags used to deny or allow current to access /proc/<pid>/$entry
> > + * after proper permission checks.
> > + */
> > +enum {
> > +       PID_ENTRY_DENY  = 0,    /* Deny access */
> > +       PID_ENTRY_ALLOW = 1,    /* Allow access */
> > +};
> 
> I think this would be less alarming if this were:
> 
> #define PID_ENTRY_DENY ((void *)1UL)
> #define PID_ENTRY_ALLOW ((void *)2UL)
Hmm,

I would like to keep it enum, enum is type-safe and I want to follow the
semantics of /proc/pid/stat and others:

check the patches and you will see that by making the variable 1 or 0 it
follows what's currently done, and IMHO 0 or 1 is more intuitive in this
case!

> Also, I don't like DENY and ALLOW.  It's not denying and allowing.
> How about PID_ENTRY_OPENER_MAY_PTRACE and
> PID_ENTRY_OPENER_MAY_NOT_PTRACE?
Hm, Ok I'll perhaps change this! will see what other thinks!

Thank you!

> --Andy

-- 
Djalal Harouni
http://opendz.org

Re: [PATCH 3/9] procfs: add proc_read_from_buffer() and pid_entry_read() helpers

[Djalal Harouni]

On Mon, May 26, 2014 at 10:01:20AM -0700, Andy Lutomirski wrote:
> On Mon, May 26, 2014 at 6:27 AM, Djalal Harouni <tixxdz@opendz.org> wrote:
> > This patch is preparation, it adds a couple of helpers to read data and
> > to get the cached permission checks during that ->read().
> >
> > Currently INF entries share the same code, they do not implement
> > specific ->open(), only ->read() coupled with callback calls. Doing
> > permission checks during ->open() will not work and will only disturb
> > the INF entries that do not need permission checks. Yes not all the INF
> > entries need checks, the ones that need protections are listed below:
> > /proc/<pid>/wchan
> > /proc/<pid>/syscall
> > /proc/<pid>/{auxv,io}  (will be handled in next patches).
> >
> > So to have proper permission checks convert this INF entries to REG
> > entries and use their open() and read() handlers to implement these
> > checks. To achieve this we add the following helpers:
> >
> > * proc_read_from_buffer() a wrapper around simple_read_from_buffer(), it
> >   makes sure that count <= PROC_BLOCK_SIZE (3*1024)
> >
> > * pid_entry_read(): it will get a free page and pass it to the specific
> >   /proc/<pid>/$entry handler (callback). The handler is of the same
> >   types of the previous INF handlers, it will only receive an extra
> >   "permitted" argument that contains the cached permission check that
> >   was performed during ->open().
> >
> >   The handler is of type:
> >   typedef int (*proc_read_fn_t)(char *page,
> >                                struct task_struct *task, int permitted);
> 
> [...]
> 
> This strikes me as *way* too complicated.  Why not just change
> proc_info_file_operations to *always* cache may_ptrace on open?
Not all the INF entries need permission checks during open:
The one that need it:
/proc/<pid>/{wchan|syscall}  converted in this series.
/proc/<pid>/{auxv|io}  (will be converted in next series)

The ones that do not need it:
/proc/<pid>/{limite|cmdline|shedstat|oom_score|...}

There is no reason to do it for these entries, and if you do it you will
also have to passe the cached permission checks, so I don't think that
you want to modify all the INF handlers especially the ones that do not
need it to take an extra argument.
Then you will also modify this:
in file fs/proc/internal.h the:
union proc_op {
	...
	int (*proc_read)(struct task_struct *task, char *page);
	...
 }

And then you will follow in all other places and handlers... and modify
the definition of INF entries... It's much more complicated...


This is cleaner: we modify only the ones that need proper permission and
they will use a shared helper, which will call their appropriate handler!

Their handlers will just receive an extra int argument.

Ultimately the sensitive INF files need their appropraite ->open(), they
are sensitive they need good protection, and other non-sensitive ones
can still share the same code.


> FWIW, it's been awhile: was anything actually wrong with using f_cred,
> other than the fact that it would have required adjusting the LSM
> hooks?
Adjusting the LSM hooks was one of the reason, and it can't be done
easily... and this one seems better since we cache just an integer
and we avoid to do some extra work during ->read() which can be done
during ->open()

Here we have the full context to do the appropriate checks.

> Admittedly, I don't see anything fundamentally wrong with caching
> may_ptrace on open as long as revoke in in the cards eventually, but
> it's plausible that a good f_cred-based implementation would make
> revoke less necessary.
Perhaps, but from the last discussion every one seems to want a revoke
anyway, and for the f_cred you will also need to check it during
->read() which in this case the check is done during ->open(), simple
and much more optimized for read() calls.


With a nice revoke implementation, you can even remove the ptrace checks
during ->read() just keep the cached result check during ->open() and
recheck just an integer during ->read(). This means less checks during
->read()... revoke should handle what was left....


Thanks!

> --Andy

-- 
Djalal Harouni
http://opendz.org

Re: [PATCH 3/9] procfs: add proc_read_from_buffer() and pid_entry_read() helpers

[Andy Lutomirski]

On Mon, May 26, 2014 at 10:41 AM, Djalal Harouni <tixxdz@opendz.org> wrote:
> On Mon, May 26, 2014 at 10:01:20AM -0700, Andy Lutomirski wrote:
>> On Mon, May 26, 2014 at 6:27 AM, Djalal Harouni <tixxdz@opendz.org> wrote:
>> > This patch is preparation, it adds a couple of helpers to read data and
>> > to get the cached permission checks during that ->read().
>> >
>> > Currently INF entries share the same code, they do not implement
>> > specific ->open(), only ->read() coupled with callback calls. Doing
>> > permission checks during ->open() will not work and will only disturb
>> > the INF entries that do not need permission checks. Yes not all the INF
>> > entries need checks, the ones that need protections are listed below:
>> > /proc/<pid>/wchan
>> > /proc/<pid>/syscall
>> > /proc/<pid>/{auxv,io}  (will be handled in next patches).
>> >
>> > So to have proper permission checks convert this INF entries to REG
>> > entries and use their open() and read() handlers to implement these
>> > checks. To achieve this we add the following helpers:
>> >
>> > * proc_read_from_buffer() a wrapper around simple_read_from_buffer(), it
>> >   makes sure that count <= PROC_BLOCK_SIZE (3*1024)
>> >
>> > * pid_entry_read(): it will get a free page and pass it to the specific
>> >   /proc/<pid>/$entry handler (callback). The handler is of the same
>> >   types of the previous INF handlers, it will only receive an extra
>> >   "permitted" argument that contains the cached permission check that
>> >   was performed during ->open().
>> >
>> >   The handler is of type:
>> >   typedef int (*proc_read_fn_t)(char *page,
>> >                                struct task_struct *task, int permitted);
>>
>> [...]
>>
>> This strikes me as *way* too complicated.  Why not just change
>> proc_info_file_operations to *always* cache may_ptrace on open?
> Not all the INF entries need permission checks during open:
> The one that need it:
> /proc/<pid>/{wchan|syscall}  converted in this series.
> /proc/<pid>/{auxv|io}  (will be converted in next series)
>
> The ones that do not need it:
> /proc/<pid>/{limite|cmdline|shedstat|oom_score|...}
>
> There is no reason to do it for these entries, and if you do it you will
> also have to passe the cached permission checks, so I don't think that
> you want to modify all the INF handlers especially the ones that do not
> need it to take an extra argument.

...but you don't have to pass the checks, because nothing will care
whether you passed them.

> Then you will also modify this:
> in file fs/proc/internal.h the:
> union proc_op {
>         ...
>         int (*proc_read)(struct task_struct *task, char *page);
>         ...
>  }
>
> And then you will follow in all other places and handlers... and modify
> the definition of INF entries... It's much more complicated...
>

But the final result will be much less messy and have less duplicated
code.  Your patch seems like it duplicates a bunch of code and I
suspect that Al Viro will dislike it if he reads it.

--Andy

Re: [PATCH 1/9] procfs: use flags to deny or allow access to /proc/<pid>/$entry

[Andy Lutomirski]

On Mon, May 26, 2014 at 10:21 AM, Djalal Harouni <tixxdz@opendz.org> wrote:
> On Mon, May 26, 2014 at 09:57:16AM -0700, Andy Lutomirski wrote:
>> On Mon, May 26, 2014 at 6:27 AM, Djalal Harouni <tixxdz@opendz.org> wrote:
>> > Add the deny or allow flags, so we can perform proper permission checks
>> > and set the result accordingly. These flags are needed in case we have
>> > to cache the result of permission checks that are done during ->open()
>> > time. Later during ->read(), we can decide to allow or deny the read().
>> >
>> > The pid entries that need these flags are:
>> > /proc/<pid>/stat
>> > /proc/<pid>/wchan
>> > /proc/<pid>/maps  (will be handled in next patches).
>> >
>> > These files are world readable, userspace depend on that. To prevent
>> > ASLR leaks and to avoid breaking userspace, we follow this scheme:
>> >
>> > a) Perform permission checks during ->open()
>> > b) Cache the result of a) and return success
>> > c) Recheck the cached result during ->read()
>>
>> Why is (c) needed?
> In order to not break these entries, some of them are world readable.
>
> So we perform the re-check that *single* cached integer, in order to
> allow access for the non-sensitive, and block or pad with zeros the
> sensitive.

What I mean is: why not just not re-check?  Is it to paper over the
lack of revoke.

>
>
>> >
>> >  /*
>> > + * Flags used to deny or allow current to access /proc/<pid>/$entry
>> > + * after proper permission checks.
>> > + */
>> > +enum {
>> > +       PID_ENTRY_DENY  = 0,    /* Deny access */
>> > +       PID_ENTRY_ALLOW = 1,    /* Allow access */
>> > +};
>>
>> I think this would be less alarming if this were:
>>
>> #define PID_ENTRY_DENY ((void *)1UL)
>> #define PID_ENTRY_ALLOW ((void *)2UL)
> Hmm,
>
> I would like to keep it enum, enum is type-safe and I want to follow the
> semantics of /proc/pid/stat and others:

It's not type-safe the way you're doing it, though.

--Andy

Re: [PATCH 3/9] procfs: add proc_read_from_buffer() and pid_entry_read() helpers

[Djalal Harouni]

On Mon, May 26, 2014 at 10:59:10AM -0700, Andy Lutomirski wrote:
> On Mon, May 26, 2014 at 10:41 AM, Djalal Harouni <tixxdz@opendz.org> wrote:
> > On Mon, May 26, 2014 at 10:01:20AM -0700, Andy Lutomirski wrote:
> >> On Mon, May 26, 2014 at 6:27 AM, Djalal Harouni <tixxdz@opendz.org> wrote:
> >> > This patch is preparation, it adds a couple of helpers to read data and
> >> > to get the cached permission checks during that ->read().
> >> >
> >> > Currently INF entries share the same code, they do not implement
> >> > specific ->open(), only ->read() coupled with callback calls. Doing
> >> > permission checks during ->open() will not work and will only disturb
> >> > the INF entries that do not need permission checks. Yes not all the INF
> >> > entries need checks, the ones that need protections are listed below:
> >> > /proc/<pid>/wchan
> >> > /proc/<pid>/syscall
> >> > /proc/<pid>/{auxv,io}  (will be handled in next patches).
> >> >
> >> > So to have proper permission checks convert this INF entries to REG
> >> > entries and use their open() and read() handlers to implement these
> >> > checks. To achieve this we add the following helpers:
> >> >
> >> > * proc_read_from_buffer() a wrapper around simple_read_from_buffer(), it
> >> >   makes sure that count <= PROC_BLOCK_SIZE (3*1024)
> >> >
> >> > * pid_entry_read(): it will get a free page and pass it to the specific
> >> >   /proc/<pid>/$entry handler (callback). The handler is of the same
> >> >   types of the previous INF handlers, it will only receive an extra
> >> >   "permitted" argument that contains the cached permission check that
> >> >   was performed during ->open().
> >> >
> >> >   The handler is of type:
> >> >   typedef int (*proc_read_fn_t)(char *page,
> >> >                                struct task_struct *task, int permitted);
> >>
> >> [...]
> >>
> >> This strikes me as *way* too complicated.  Why not just change
> >> proc_info_file_operations to *always* cache may_ptrace on open?
> > Not all the INF entries need permission checks during open:
> > The one that need it:
> > /proc/<pid>/{wchan|syscall}  converted in this series.
> > /proc/<pid>/{auxv|io}  (will be converted in next series)
> >
> > The ones that do not need it:
> > /proc/<pid>/{limite|cmdline|shedstat|oom_score|...}
> >
> > There is no reason to do it for these entries, and if you do it you will
> > also have to passe the cached permission checks, so I don't think that
> > you want to modify all the INF handlers especially the ones that do not
> > need it to take an extra argument.
> 
> ...but you don't have to pass the checks, because nothing will care
> whether you passed them.
No one will use them, but what you suggest is passing a dummy value that
no one cares about!

You said in the first place that this is *way* too complicated, no IHMO
going and changing all the handlers to receive a dummy argument that no one
will use is more complicated. In this case you are just suggesting to
convert all the INF entries into REG ones. Andi take a look please!

The definition of INF can't get a 'struct file_operations', only the
REG, at the end what you suggest is change all the INF entries to REG
and do a ptrace check at open() that only a small subset of these
entries will use...

Try it and you will see how much code you will change.

While the current proposed change just introduce a new helper (new code)
and adds the cached permission argument!

> > Then you will also modify this:
> > in file fs/proc/internal.h the:
> > union proc_op {
> >         ...
> >         int (*proc_read)(struct task_struct *task, char *page);
> >         ...
> >  }
> >
> > And then you will follow in all other places and handlers... and modify
> > the definition of INF entries... It's much more complicated...
> >
> 
> But the final result will be much less messy and have less duplicated
> code.  Your patch seems like it duplicates a bunch of code and I
> suspect that Al Viro will dislike it if he reads it.
Hmm, as I've said if we do that we need to convert all the INF entries
to REG ones, and remove the INF definition since it will be useless, then
convert all the handlers... And I can't consider it duplicate code,
since one is to handle non-sensitive entries and the other to handle
sensitive ones, perhaps write a new function that will get the small
duplicate instructions.


For Al Viro oh thank you :-), hope that he reads the patches first, then
if he thinks that converting all the INF entries into REG ones, and
doing ptrace check during ->open for files that do not need it is the
best solution, well I'll have just to follow and update the patches :-D

Thank you Andi!


> --Andy
> --
> To unsubscribe from this list: send the line "unsubscribe linux-fsdevel" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

-- 
Djalal Harouni
http://opendz.org

Re: [PATCH 3/9] procfs: add proc_read_from_buffer() and pid_entry_read() helpers

[Djalal Harouni]

On Mon, May 26, 2014 at 07:21:54PM +0100, Djalal Harouni wrote:
> On Mon, May 26, 2014 at 10:59:10AM -0700, Andy Lutomirski wrote:
> > On Mon, May 26, 2014 at 10:41 AM, Djalal Harouni <tixxdz@opendz.org> wrote:
> > > On Mon, May 26, 2014 at 10:01:20AM -0700, Andy Lutomirski wrote:
> > >> On Mon, May 26, 2014 at 6:27 AM, Djalal Harouni <tixxdz@opendz.org> wrote:
> > >> > This patch is preparation, it adds a couple of helpers to read data and
> > >> > to get the cached permission checks during that ->read().
> > >> >
> > >> > Currently INF entries share the same code, they do not implement
> > >> > specific ->open(), only ->read() coupled with callback calls. Doing
> > >> > permission checks during ->open() will not work and will only disturb
> > >> > the INF entries that do not need permission checks. Yes not all the INF
> > >> > entries need checks, the ones that need protections are listed below:
> > >> > /proc/<pid>/wchan
> > >> > /proc/<pid>/syscall
> > >> > /proc/<pid>/{auxv,io}  (will be handled in next patches).
> > >> >
> > >> > So to have proper permission checks convert this INF entries to REG
> > >> > entries and use their open() and read() handlers to implement these
> > >> > checks. To achieve this we add the following helpers:
> > >> >
> > >> > * proc_read_from_buffer() a wrapper around simple_read_from_buffer(), it
> > >> >   makes sure that count <= PROC_BLOCK_SIZE (3*1024)
> > >> >
> > >> > * pid_entry_read(): it will get a free page and pass it to the specific
> > >> >   /proc/<pid>/$entry handler (callback). The handler is of the same
> > >> >   types of the previous INF handlers, it will only receive an extra
> > >> >   "permitted" argument that contains the cached permission check that
> > >> >   was performed during ->open().
> > >> >
> > >> >   The handler is of type:
> > >> >   typedef int (*proc_read_fn_t)(char *page,
> > >> >                                struct task_struct *task, int permitted);
> > >>
> > >> [...]
> > >>
> > >> This strikes me as *way* too complicated.  Why not just change
> > >> proc_info_file_operations to *always* cache may_ptrace on open?
> > > Not all the INF entries need permission checks during open:
> > > The one that need it:
> > > /proc/<pid>/{wchan|syscall}  converted in this series.
> > > /proc/<pid>/{auxv|io}  (will be converted in next series)
> > >
> > > The ones that do not need it:
> > > /proc/<pid>/{limite|cmdline|shedstat|oom_score|...}
> > >
> > > There is no reason to do it for these entries, and if you do it you will
> > > also have to passe the cached permission checks, so I don't think that
> > > you want to modify all the INF handlers especially the ones that do not
> > > need it to take an extra argument.
> > 
> > ...but you don't have to pass the checks, because nothing will care
> > whether you passed them.
> No one will use them, but what you suggest is passing a dummy value that
> no one cares about!
> 
> You said in the first place that this is *way* too complicated, no IHMO
> going and changing all the handlers to receive a dummy argument that no one
> will use is more complicated. In this case you are just suggesting to
> convert all the INF entries into REG ones. Andi take a look please!
> 
> The definition of INF can't get a 'struct file_operations', only the
> REG, at the end what you suggest is change all the INF entries to REG
> and do a ptrace check at open() that only a small subset of these
> entries will use...
> 
> Try it and you will see how much code you will change.
> 
> While the current proposed change just introduce a new helper (new code)
> and adds the cached permission argument!
> 
> > > Then you will also modify this:
> > > in file fs/proc/internal.h the:
> > > union proc_op {
> > >         ...
> > >         int (*proc_read)(struct task_struct *task, char *page);
> > >         ...
> > >  }
> > >
> > > And then you will follow in all other places and handlers... and modify
> > > the definition of INF entries... It's much more complicated...
> > >
> > 
> > But the final result will be much less messy and have less duplicated
> > code.  Your patch seems like it duplicates a bunch of code and I
> > suspect that Al Viro will dislike it if he reads it.
> Hmm, as I've said if we do that we need to convert all the INF entries
> to REG ones, and remove the INF definition since it will be useless, then
> convert all the handlers... And I can't consider it duplicate code,
> since one is to handle non-sensitive entries and the other to handle
> sensitive ones, perhaps write a new function that will get the small
> duplicate instructions.
> 
> 
> For Al Viro oh thank you :-), hope that he reads the patches first, then
> if he thinks that converting all the INF entries into REG ones, and
> doing ptrace check during ->open for files that do not need it is the
> best solution, well I'll have just to follow and update the patches :-D
If I did convert all the INF to REG, well I'm sure that no one will like
the git diff stat in the first place, at least this way they will first
look at the patches and judge :-)


IIRC, Al Viro didn't like the f_cred in the first place since it touches
the seq struct, he said that it does not belong there!

So the current code is much contained, it just touches
/proc/<pid>/$entries, and I'll try to see if we can share some duplicate
instructions into a single new function!

Note that the duplication you point, is in this case is used for
sensitive files and non-sensitive files:

the old proc_info_read() and the new pid_entry_read() patch 3/9
the old proc_single_show() and the new pid_entry_show() patch 7/9

I did this to avoid changing the handlers and what was explained in the
previous email, Ahh and to reduce the git diff stat!

I'm open to proposition!

Thanks!

> Thank you Andi!
> 
> 
> > --Andy
> > --
> > To unsubscribe from this list: send the line "unsubscribe linux-fsdevel" in
> > the body of a message to majordomo@vger.kernel.org
> > More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 
> -- 
> Djalal Harouni
> http://opendz.org

-- 
Djalal Harouni
http://opendz.org

Re: [PATCH 1/9] procfs: use flags to deny or allow access to /proc/<pid>/$entry

[Djalal Harouni]

On Mon, May 26, 2014 at 11:06:40AM -0700, Andy Lutomirski wrote:
> On Mon, May 26, 2014 at 10:21 AM, Djalal Harouni <tixxdz@opendz.org> wrote:
> > On Mon, May 26, 2014 at 09:57:16AM -0700, Andy Lutomirski wrote:
> >> On Mon, May 26, 2014 at 6:27 AM, Djalal Harouni <tixxdz@opendz.org> wrote:
> >> > Add the deny or allow flags, so we can perform proper permission checks
> >> > and set the result accordingly. These flags are needed in case we have
> >> > to cache the result of permission checks that are done during ->open()
> >> > time. Later during ->read(), we can decide to allow or deny the read().
> >> >
> >> > The pid entries that need these flags are:
> >> > /proc/<pid>/stat
> >> > /proc/<pid>/wchan
> >> > /proc/<pid>/maps  (will be handled in next patches).
> >> >
> >> > These files are world readable, userspace depend on that. To prevent
> >> > ASLR leaks and to avoid breaking userspace, we follow this scheme:
> >> >
> >> > a) Perform permission checks during ->open()
> >> > b) Cache the result of a) and return success
> >> > c) Recheck the cached result during ->read()
> >>
> >> Why is (c) needed?
> > In order to not break these entries, some of them are world readable.
> >
> > So we perform the re-check that *single* cached integer, in order to
> > allow access for the non-sensitive, and block or pad with zeros the
> > sensitive.
> 
> What I mean is: why not just not re-check?  Is it to paper over the
> lack of revoke.
Ahh ok, you mean *re-check* the cached permission during ->read() since
this is necessary, and do *not* re-check ptrace capabilities during
->read()!

Indeed, this is precisely due to the lack of revoke! if we do not
re-check ptrace capabilities during ->read() we may offer this scenario
to attackers:

 open(/proc/$process_I_can_ptrace/*, O_RDONLY)
 and make the $process_I_can_ptrace exec a suid binary, this will pass
 the cached permission of ->open() and let users to read the
 /proc/<suid-exec>/* entries.

 In this case a process like "cat" which we find in all systems can be
 used to disclose sensitive data.

In the other hand if we continue to do the ptrace capability check
during read() then attackers need to find a *suid* binary that reads
from specified input in order to bypass that ptrace check during
->read() instead of using a normal program. This is a big difference!


So in the mean time, Yes we must let the re-check ptrace capability
during ->read() to reduce the attack surface. Later if there is a
revoke(), then you can remove that ptrace check and just check the cached
permission during ->read(), revoke will handle it, in case of an exec!


> >> >  /*
> >> > + * Flags used to deny or allow current to access /proc/<pid>/$entry
> >> > + * after proper permission checks.
> >> > + */
> >> > +enum {
> >> > +       PID_ENTRY_DENY  = 0,    /* Deny access */
> >> > +       PID_ENTRY_ALLOW = 1,    /* Allow access */
> >> > +};
> >>
> >> I think this would be less alarming if this were:
> >>
> >> #define PID_ENTRY_DENY ((void *)1UL)
> >> #define PID_ENTRY_ALLOW ((void *)2UL)
> > Hmm,
> >
> > I would like to keep it enum, enum is type-safe and I want to follow the
> > semantics of /proc/pid/stat and others:
> 
> It's not type-safe the way you're doing it, though.
Can you please shed some light Andy, thank you in advance!


> --Andy
> --
> To unsubscribe from this list: send the line "unsubscribe linux-fsdevel" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

-- 
Djalal Harouni
http://opendz.org

Re: [PATCH 1/9] procfs: use flags to deny or allow access to /proc/<pid>/$entry

[Andy Lutomirski]

On Mon, May 26, 2014 at 12:13 PM, Djalal Harouni <tixxdz@opendz.org> wrote:
> On Mon, May 26, 2014 at 11:06:40AM -0700, Andy Lutomirski wrote:
>> On Mon, May 26, 2014 at 10:21 AM, Djalal Harouni <tixxdz@opendz.org> wrote:
>> > I would like to keep it enum, enum is type-safe and I want to follow the
>> > semantics of /proc/pid/stat and others:
>>
>> It's not type-safe the way you're doing it, though.
> Can you please shed some light Andy, thank you in advance!

You're casting these things back and forth.  If you were storing enum
values in an enum-typed variable, great, but you're not.

--Andy

Re: [PATCH 6/9] procfs: add pid_seq_private struct to handle /proc/<pid>/{stat|stack}

[Djalal Harouni]

On Mon, May 26, 2014 at 10:02:15AM -0700, Andy Lutomirski wrote:
> On Mon, May 26, 2014 at 6:27 AM, Djalal Harouni <tixxdz@opendz.org> wrote:
> > This patch is preparation to handle sensitive ONE entries:
> > /proc/<pid>/stat
> > /proc/<pid>/stack
> >
> > These files use sequence iterators and we want to keep that logic, and
> > their internal handler semantics.
> >
> > The sequence iterators stock the inode in the seq_file->private field,
> > so in order to keep the semantic and to make the cached permission
> > checks available during ->read(), we add the 'pid_seq_private' struct
> > that contains both the inode and the cached permission. It will be the
> > one referenced in the seq_file->private.
> >
> > This way the internal handlers of /proc/<pid>/{stat|stack} wont change.
> >
> > Signed-off-by: Djalal Harouni <tixxdz@opendz.org>
> > ---
> >  fs/proc/internal.h | 11 +++++++++++
> >  1 file changed, 11 insertions(+)
> >
> > diff --git a/fs/proc/internal.h b/fs/proc/internal.h
> > index f5c452c..f28e4f01 100644
> > --- a/fs/proc/internal.h
> > +++ b/fs/proc/internal.h
> > @@ -78,6 +78,17 @@ struct proc_inode {
> >         struct inode vfs_inode;
> >  };
> >
> > +/*
> > + * Struct used by some /proc/<pid>/$entries that use sequence
> > + * iterators.
> > + * This struct will be saved in seq_file->private so seq handlers
> > + * can access the inode and the cached permission checks of ->open().
> > + */
> > +struct pid_seq_private {
> > +       int permitted;
> > +       struct inode *inode;
> > +};
> > +
> >  typedef int (*proc_read_fn_t)(char *page,
> >                               struct task_struct *task, int permitted);
> 
> int opener_may_ptrace, please.
Ok, I'll change it in v2.

Thanks

-- 
Djalal Harouni
http://opendz.org

Re: [PATCH 1/9] procfs: use flags to deny or allow access to /proc/<pid>/$entry

[Djalal Harouni]

On Mon, May 26, 2014 at 12:17:48PM -0700, Andy Lutomirski wrote:
> On Mon, May 26, 2014 at 12:13 PM, Djalal Harouni <tixxdz@opendz.org> wrote:
> > On Mon, May 26, 2014 at 11:06:40AM -0700, Andy Lutomirski wrote:
> >> On Mon, May 26, 2014 at 10:21 AM, Djalal Harouni <tixxdz@opendz.org> wrote:
> >> > I would like to keep it enum, enum is type-safe and I want to follow the
> >> > semantics of /proc/pid/stat and others:
> >>
> >> It's not type-safe the way you're doing it, though.
> > Can you please shed some light Andy, thank you in advance!
> 
> You're casting these things back and forth.  If you were storing enum
> values in an enum-typed variable, great, but you're not.
Ok I see your point! Yes, but AFAIK in this case it's perfectly safe!

We don't use large values, the one used here can be represented as an
int, and later we cast them to unsigned long to make sure that we are
using the correct value and the correct size of a pointer.

GCC is able to warn in this case of any appropriate cast:
-Wpointer-to-int-cast ...

And from Documentation/CodingStyle:
"Enums are preferred when defining several related constants."

Perhaps I'm missing something!

> --Andy

-- 
Djalal Harouni
http://opendz.org

Re: [PATCH 1/9] procfs: use flags to deny or allow access to /proc/<pid>/$entry

[Kees Cook]

On Mon, May 26, 2014 at 6:27 AM, Djalal Harouni <tixxdz@opendz.org> wrote:
> Add the deny or allow flags, so we can perform proper permission checks
> and set the result accordingly. These flags are needed in case we have
> to cache the result of permission checks that are done during ->open()
> time. Later during ->read(), we can decide to allow or deny the read().
>
> The pid entries that need these flags are:
> /proc/<pid>/stat
> /proc/<pid>/wchan
> /proc/<pid>/maps  (will be handled in next patches).
>
> These files are world readable, userspace depend on that. To prevent
> ASLR leaks and to avoid breaking userspace, we follow this scheme:
>
> a) Perform permission checks during ->open()
> b) Cache the result of a) and return success
> c) Recheck the cached result during ->read()
> d) If cached == PID_ENTRY_DENY:
>    then we replace the sensitive fields with zeros, userspace won't
>    break and sensitive fields are protected.
>
> These flags are internal to /proc/<pid>/*

Since this complex area of behavior has seen a lot of changes, I think
I'd really like to see some tests in tools/testsing/selftests/
somewhere that actually codify what the expected behaviors should be.

We have a lot of corner cases, a lot of userspace behaviors to retain,
and given how fragile this area has been, I'd love to avoid seeing
regressions. It seems like we need to test file permissions, open/read
permissions, contents, etc, under many different cases (priv, unpriv,
passing between priv/unpriv and unpriv/priv, ptrace checks, etc).

If we could do a "make run_tests" in a selftests subdirectory, it'd be
much easier to a) validate these fixes, and b) avoid regressions.

-Kees

-- 
Kees Cook
Chrome OS Security

Re: [PATCH 1/9] procfs: use flags to deny or allow access to /proc/<pid>/$entry

[Djalal Harouni]

On Tue, May 27, 2014 at 11:38:54AM -0700, Kees Cook wrote:
> On Mon, May 26, 2014 at 6:27 AM, Djalal Harouni <tixxdz@opendz.org> wrote:
> > Add the deny or allow flags, so we can perform proper permission checks
> > and set the result accordingly. These flags are needed in case we have
> > to cache the result of permission checks that are done during ->open()
> > time. Later during ->read(), we can decide to allow or deny the read().
> >
> > The pid entries that need these flags are:
> > /proc/<pid>/stat
> > /proc/<pid>/wchan
> > /proc/<pid>/maps  (will be handled in next patches).
> >
> > These files are world readable, userspace depend on that. To prevent
> > ASLR leaks and to avoid breaking userspace, we follow this scheme:
> >
> > a) Perform permission checks during ->open()
> > b) Cache the result of a) and return success
> > c) Recheck the cached result during ->read()
> > d) If cached == PID_ENTRY_DENY:
> >    then we replace the sensitive fields with zeros, userspace won't
> >    break and sensitive fields are protected.
> >
> > These flags are internal to /proc/<pid>/*
> 
> Since this complex area of behavior has seen a lot of changes, I think
> I'd really like to see some tests in tools/testsing/selftests/
> somewhere that actually codify what the expected behaviors should be.
Ok, sounds good!

> We have a lot of corner cases, a lot of userspace behaviors to retain,
> and given how fragile this area has been, I'd love to avoid seeing
> regressions. It seems like we need to test file permissions, open/read
> permissions, contents, etc, under many different cases (priv, unpriv,
> passing between priv/unpriv and unpriv/priv, ptrace checks, etc).
Yes, nice.

> If we could do a "make run_tests" in a selftests subdirectory, it'd be
> much easier to a) validate these fixes, and b) avoid regressions.
Ok!

Since I'm working on this on my free time and when time permits, please
give me some days! I'll try to handle the cases I've discussed here.

Now Kees, some of these files are still world readable and affected:
smaps, maps ... I know, it's a matter of suid binary on your distro, and
every one can exploit it. So what to do: make the tests public or write
the tests and fix these entries then at last make the tests public ?


Where should I send the tests ?

Thank you!


> -Kees
> 
> -- 
> Kees Cook
> Chrome OS Security

-- 
Djalal Harouni
http://opendz.org

Re: [PATCH 1/9] procfs: use flags to deny or allow access to /proc/<pid>/$entry

[Kees Cook]

On Wed, May 28, 2014 at 4:42 AM, Djalal Harouni <tixxdz@opendz.org> wrote:
> On Tue, May 27, 2014 at 11:38:54AM -0700, Kees Cook wrote:
>> On Mon, May 26, 2014 at 6:27 AM, Djalal Harouni <tixxdz@opendz.org> wrote:
>> > Add the deny or allow flags, so we can perform proper permission checks
>> > and set the result accordingly. These flags are needed in case we have
>> > to cache the result of permission checks that are done during ->open()
>> > time. Later during ->read(), we can decide to allow or deny the read().
>> >
>> > The pid entries that need these flags are:
>> > /proc/<pid>/stat
>> > /proc/<pid>/wchan
>> > /proc/<pid>/maps  (will be handled in next patches).
>> >
>> > These files are world readable, userspace depend on that. To prevent
>> > ASLR leaks and to avoid breaking userspace, we follow this scheme:
>> >
>> > a) Perform permission checks during ->open()
>> > b) Cache the result of a) and return success
>> > c) Recheck the cached result during ->read()
>> > d) If cached == PID_ENTRY_DENY:
>> >    then we replace the sensitive fields with zeros, userspace won't
>> >    break and sensitive fields are protected.
>> >
>> > These flags are internal to /proc/<pid>/*
>>
>> Since this complex area of behavior has seen a lot of changes, I think
>> I'd really like to see some tests in tools/testsing/selftests/
>> somewhere that actually codify what the expected behaviors should be.
> Ok, sounds good!
>
>> We have a lot of corner cases, a lot of userspace behaviors to retain,
>> and given how fragile this area has been, I'd love to avoid seeing
>> regressions. It seems like we need to test file permissions, open/read
>> permissions, contents, etc, under many different cases (priv, unpriv,
>> passing between priv/unpriv and unpriv/priv, ptrace checks, etc).
> Yes, nice.
>
>> If we could do a "make run_tests" in a selftests subdirectory, it'd be
>> much easier to a) validate these fixes, and b) avoid regressions.
> Ok!
>
> Since I'm working on this on my free time and when time permits, please
> give me some days! I'll try to handle the cases I've discussed here.
>
> Now Kees, some of these files are still world readable and affected:
> smaps, maps ... I know, it's a matter of suid binary on your distro, and
> every one can exploit it. So what to do: make the tests public or write
> the tests and fix these entries then at last make the tests public ?

I expect these tests to be public -- there is nothing secret about how
things are currently vulnerable. I think the priv vs unpriv tests can
be emulated (without root setuid) using a prctl(PR_SET_DUMPABLE, 0)
call which should give you similar protections.

> Where should I send the tests ?

They should be part of the patch series, and live in the
tools/testing/selftests/ tree of the kernel. There are plenty of
examples in there. If you have the tests as the first set of patches,
then you can show which tests start passing with each additional fix.
I would break the tests up into "what is expected to work now" that
all pass, and then add all the cases that are currently a problem that
will all fail. Then as more of the fixes land from your series, more
of those tests will pass until everything is passing.

-Kees

-- 
Kees Cook
Chrome OS Security

Re: [PATCH 1/9] procfs: use flags to deny or allow access to /proc/<pid>/$entry

[Djalal Harouni]

On Wed, May 28, 2014 at 09:59:54AM -0700, Kees Cook wrote:
> On Wed, May 28, 2014 at 4:42 AM, Djalal Harouni <tixxdz@opendz.org> wrote:
> > On Tue, May 27, 2014 at 11:38:54AM -0700, Kees Cook wrote:
> >> On Mon, May 26, 2014 at 6:27 AM, Djalal Harouni <tixxdz@opendz.org> wrote:
> >> > Add the deny or allow flags, so we can perform proper permission checks
> >> > and set the result accordingly. These flags are needed in case we have
> >> > to cache the result of permission checks that are done during ->open()
> >> > time. Later during ->read(), we can decide to allow or deny the read().
> >> >
> >> > The pid entries that need these flags are:
> >> > /proc/<pid>/stat
> >> > /proc/<pid>/wchan
> >> > /proc/<pid>/maps  (will be handled in next patches).
> >> >
> >> > These files are world readable, userspace depend on that. To prevent
> >> > ASLR leaks and to avoid breaking userspace, we follow this scheme:
> >> >
> >> > a) Perform permission checks during ->open()
> >> > b) Cache the result of a) and return success
> >> > c) Recheck the cached result during ->read()
> >> > d) If cached == PID_ENTRY_DENY:
> >> >    then we replace the sensitive fields with zeros, userspace won't
> >> >    break and sensitive fields are protected.
> >> >
> >> > These flags are internal to /proc/<pid>/*
> >>
> >> Since this complex area of behavior has seen a lot of changes, I think
> >> I'd really like to see some tests in tools/testsing/selftests/
> >> somewhere that actually codify what the expected behaviors should be.
> > Ok, sounds good!
> >
> >> We have a lot of corner cases, a lot of userspace behaviors to retain,
> >> and given how fragile this area has been, I'd love to avoid seeing
> >> regressions. It seems like we need to test file permissions, open/read
> >> permissions, contents, etc, under many different cases (priv, unpriv,
> >> passing between priv/unpriv and unpriv/priv, ptrace checks, etc).
> > Yes, nice.
> >
> >> If we could do a "make run_tests" in a selftests subdirectory, it'd be
> >> much easier to a) validate these fixes, and b) avoid regressions.
> > Ok!
> >
> > Since I'm working on this on my free time and when time permits, please
> > give me some days! I'll try to handle the cases I've discussed here.
> >
> > Now Kees, some of these files are still world readable and affected:
> > smaps, maps ... I know, it's a matter of suid binary on your distro, and
> > every one can exploit it. So what to do: make the tests public or write
> > the tests and fix these entries then at last make the tests public ?
> 
> I expect these tests to be public -- there is nothing secret about how
> things are currently vulnerable. I think the priv vs unpriv tests can
> be emulated (without root setuid) using a prctl(PR_SET_DUMPABLE, 0)
> call which should give you similar protections.
Ok, thanks for the hint!

> > Where should I send the tests ?
> 
> They should be part of the patch series, and live in the
> tools/testing/selftests/ tree of the kernel. There are plenty of
> examples in there. If you have the tests as the first set of patches,
> then you can show which tests start passing with each additional fix.
> I would break the tests up into "what is expected to work now" that
> all pass, and then add all the cases that are currently a problem that
> will all fail. Then as more of the fixes land from your series, more
> of those tests will pass until everything is passing.
Ok, will follow and do that. Thank you Kees!

> 
> -- 
> Kees Cook
> Chrome OS Security

-- 
Djalal Harouni
http://opendz.org

Re: [PATCH 3/9] procfs: add proc_read_from_buffer() and pid_entry_read() helpers

[Alexey Dobriyan]

On Mon, May 26, 2014 at 4:27 PM, Djalal Harouni <tixxdz@opendz.org> wrote:
> +/* 4K page size but our output routines use some slack for overruns */
> +#define PROC_BLOCK_SIZE        (3*1024)


This macro is the most ugly part of /proc, let's not dup it. :-)
Overruns simply should not happen.