proc: maps protection

PATCH In review Kees Cook 1 participant 2 messages Mar 5, 2007 – Mar 6, 2007

Subsystems (2)

Proc filesystem Memory mapping

lore

Share & tools

lore

Kees Cook

author

2007-03-05 20:35 UTC #0 lore raw

PATCH 1/1 proc: maps protection

Implement the same logic for the checks done on /proc/$pid/mem, but extend them to /proc/$pid/{maps,smaps,numa_maps}. This means that only processes and their ptrace parents can read their maps files.

Signed-off-by Kees Cook <[email protected]>

---

This is a continuation of a much earlier discussion[1]. As I understand, the problem is:

- "maps" should not be world-readable, especially if programs expect any kind of ASLR protection from local attackers.
- "maps" cannot just be 0400 because "-D_FORTIFY_SOURCE=2 -O2" makes glibc check the maps when %n is in a *printf call, and a setuid(getuid()) process wouldn't be able to read its own maps file.

It seemed that one solution would be to protect the maps file in the same way that the mem file is. So, that's what this patch does.

Running this patch locally, gdb and the setuid glibc tests appear happy. What do others think of this?

[1] http://marc.theaimsgroup.com/?l=linux-kernel&m=113796842515034&w=2

---
 base.c       |    9 ---------
 internal.h   |    9 +++++++++
 task_mmu.c   |   16 +++++++++++++++-
 task_nommu.c |    6 ++++++
 4 files changed, 30 insertions(+), 10 deletions(-)

---

fs/proc/base.c +0 -9

index 1a979ea..9bf7585 100644
@@ -65,8 +65,6 @@
 #include <linux/rcupdate.h>
 #include <linux/kallsyms.h>
 #include <linux/mount.h>
-#include <linux/security.h>
-#include <linux/ptrace.h>
 #include <linux/seccomp.h>
 #include <linux/cpuset.h>
 #include <linux/audit.h>

@@ -189,13 +187,6 @@ static int proc_root_link(struct inode *inode, struct dentry **dentry, struct vf
 	return result;
 }
 
-#define MAY_PTRACE(task) \
-	(task == current || \
-	(task->parent == current && \
-	(task->ptrace & PT_PTRACED) && \
-	 (task->state == TASK_STOPPED || task->state == TASK_TRACED) && \
-	 security_ptrace(current,task) == 0))
-
 static int proc_pid_environ(struct task_struct *task, char * buffer)
 {
 	int res = 0;

fs/proc/internal.h +9 -0

index 987c773..3c5ccc9 100644
@@ -10,6 +10,8 @@
  */
 
 #include <linux/proc_fs.h>
+#include <linux/security.h>
+#include <linux/ptrace.h>
 
 struct vmalloc_info {
 	unsigned long	used;

@@ -31,6 +33,13 @@ do {						\
 extern int nommu_vma_show(struct seq_file *, struct vm_area_struct *);
 #endif
 
+#define MAY_PTRACE(task) \
+        (task == current || \
+        (task->parent == current && \
+         (task->ptrace & PT_PTRACED) && \
+         (task->state == TASK_STOPPED || task->state == TASK_TRACED) && \
+         security_ptrace(current,task) == 0))
+
 extern void create_seq_entry(char *name, mode_t mode, const struct file_operations *f);
 extern int proc_exe_link(struct inode *, struct dentry **, struct vfsmount **);
 extern int proc_tid_stat(struct task_struct *,  char *);

fs/proc/task_mmu.c +15 -1

index 55ade0d..85486d4 100644
@@ -134,6 +134,9 @@ static int show_map_internal(struct seq_file *m, void *v, struct mem_size_stats
 	dev_t dev = 0;
 	int len;
 
+	if (!MAY_PTRACE(task) || !ptrace_may_attach(task))
+		return -EACCES;
+
 	if (file) {
 		struct inode *inode = vma->vm_file->f_path.dentry->d_inode;
 		dev = inode->i_sb->s_dev;

@@ -444,11 +447,22 @@ struct file_operations proc_maps_operations = {
 #ifdef CONFIG_NUMA
 extern int show_numa_map(struct seq_file *m, void *v);
 
+static int show_numa_map_checked(struct seq_file *m, void *v)
+{
+	struct proc_maps_private *priv = m->private;
+	struct task_struct *task = priv->task;
+
+	if (!MAY_PTRACE(task) || !ptrace_may_attach(task))
+		return -EACCES;
+	
+	return show_numa_map(m, v);
+}
+
 static struct seq_operations proc_pid_numa_maps_op = {
         .start  = m_start,
         .next   = m_next,
         .stop   = m_stop,
-        .show   = show_numa_map
+        .show   = show_numa_map_checked
 };
 
 static int numa_maps_open(struct inode *inode, struct file *file)

fs/proc/task_nommu.c +6 -0

index fcc5caf..985a6ff 100644
@@ -143,6 +143,12 @@ out:
 static int show_map(struct seq_file *m, void *_vml)
 {
 	struct vm_list_struct *vml = _vml;
+	struct proc_maps_private *priv = m->private;
+	struct task_struct *task = priv->task;
+	
+	if (!MAY_PTRACE(task) || !ptrace_may_attach(task))
+		return -EPERM;
+
 	return nommu_vma_show(m, vml->vma);
 }

Signature · 1 line

Kees Cook

Kees Cook

author

2007-03-06 00:45 UTC #1 lore raw

Signed-off-by Kees Cook <[email protected]>

Signed-off-by Arjan van de Ven <[email protected]>

---

Quoted 1 line · Kees Cook

What do others think of this?

Whoops, Arjan caught a mistake, new patch included, using -EACCES everywhere. (I had a straggling -EPERM from an earlier version.)

---

fs/proc/base.c +0 -9

index 1a979ea..9bf7585 100644
@@ -65,8 +65,6 @@
 #include <linux/rcupdate.h>
 #include <linux/kallsyms.h>
 #include <linux/mount.h>
-#include <linux/security.h>
-#include <linux/ptrace.h>
 #include <linux/seccomp.h>
 #include <linux/cpuset.h>
 #include <linux/audit.h>

@@ -189,13 +187,6 @@ static int proc_root_link(struct inode *inode, struct dentry **dentry, struct vf
 	return result;
 }
 
-#define MAY_PTRACE(task) \
-	(task == current || \
-	(task->parent == current && \
-	(task->ptrace & PT_PTRACED) && \
-	 (task->state == TASK_STOPPED || task->state == TASK_TRACED) && \
-	 security_ptrace(current,task) == 0))
-
 static int proc_pid_environ(struct task_struct *task, char * buffer)
 {
 	int res = 0;

fs/proc/internal.h +9 -0

index 987c773..3c5ccc9 100644
@@ -10,6 +10,8 @@
  */
 
 #include <linux/proc_fs.h>
+#include <linux/security.h>
+#include <linux/ptrace.h>
 
 struct vmalloc_info {
 	unsigned long	used;

@@ -31,6 +33,13 @@ do {						\
 extern int nommu_vma_show(struct seq_file *, struct vm_area_struct *);
 #endif
 
+#define MAY_PTRACE(task) \
+        (task == current || \
+        (task->parent == current && \
+         (task->ptrace & PT_PTRACED) && \
+         (task->state == TASK_STOPPED || task->state == TASK_TRACED) && \
+         security_ptrace(current,task) == 0))
+
 extern void create_seq_entry(char *name, mode_t mode, const struct file_operations *f);
 extern int proc_exe_link(struct inode *, struct dentry **, struct vfsmount **);
 extern int proc_tid_stat(struct task_struct *,  char *);

fs/proc/task_mmu.c +15 -1

index 55ade0d..85486d4 100644
@@ -134,6 +134,9 @@ static int show_map_internal(struct seq_file *m, void *v, struct mem_size_stats
 	dev_t dev = 0;
 	int len;
 
+	if (!MAY_PTRACE(task) || !ptrace_may_attach(task))
+		return -EACCES;
+
 	if (file) {
 		struct inode *inode = vma->vm_file->f_path.dentry->d_inode;
 		dev = inode->i_sb->s_dev;

@@ -444,11 +447,22 @@ struct file_operations proc_maps_operations = {
 #ifdef CONFIG_NUMA
 extern int show_numa_map(struct seq_file *m, void *v);
 
+static int show_numa_map_checked(struct seq_file *m, void *v)
+{
+	struct proc_maps_private *priv = m->private;
+	struct task_struct *task = priv->task;
+
+	if (!MAY_PTRACE(task) || !ptrace_may_attach(task))
+		return -EACCES;
+	
+	return show_numa_map(m, v);
+}
+
 static struct seq_operations proc_pid_numa_maps_op = {
         .start  = m_start,
         .next   = m_next,
         .stop   = m_stop,
-        .show   = show_numa_map
+        .show   = show_numa_map_checked
 };
 
 static int numa_maps_open(struct inode *inode, struct file *file)

fs/proc/task_nommu.c +6 -0

index fcc5caf..985a6ff 100644
@@ -143,6 +143,12 @@ out:
 static int show_map(struct seq_file *m, void *_vml)
 {
 	struct vm_list_struct *vml = _vml;
+	struct proc_maps_private *priv = m->private;
+	struct task_struct *task = priv->task;
+	
+	if (!MAY_PTRACE(task) || !ptrace_may_attach(task))
+		return -EACCES;
+
 	return nommu_vma_show(m, vml->vma);
 }

Signature · 1 line

Kees Cook