Re: [PATCH] proc: maps protection

Kees Cook
2007-03-07T03:37:18Z

On Tue, Mar 06, 2007 at 06:59:42PM -0800, Andrew Morton wrote:
> On Tue, 6 Mar 2007 18:13:35 -0800 Kees Cook <kees@outflux.net> wrote:
>
> > On Tue, Mar 06, 2007 at 05:56:09PM -0800, Andrew Morton wrote:
> > > On Tue, 6 Mar 2007 17:22:34 -0800
> > > Kees Cook <kees@outflux.net> wrote:
> > >
> > > > This is a continuation of a much earlier discussion[1]. As I
> > > > understand, the problem is:
> > >
> > > This sounds like a really good way of breaking lots and lots of people's
> > > expensively-developed stuff. In ways which we won't discover until a year
> > > after we shipped it.
> > >
> > > So nope, sorry. Need to find a compatible way of doing this. Perhaps a
> > > kernel boot parameter or a /proc knob.
> >
> > Do you have examples of things in the kernel that I can use as a
> > starting point?
>
> No, I don't think this has precedent.
>
> > Would something like /proc/sys/kernel/maps_protect be
> > reasonable?
>
> Yes, that sounds reasonable.
>
> An alternative is to do it with elf headers, perhaps - let the process
> specify what protections it wants in some manner.
>
> > If an acceptable toggle is made, would you consider it being enabled by
> > default (i.e. "tighter security by default")?
>
> Again, that sounds risky.

[Adding Cc:lkml]

How about using a reduced check, as is done for fd and environ? This
would allow root-running system monitors to still do their job.
Effectively, this changes the test from "is ptracing" to just "can
ptrace".

If this still isn't considered safe, I'll add the maps_protect file...

---
task_mmu.c | 16 +++++++++++++++-
task_nommu.c | 6 ++++++
2 files changed, 21 insertions(+), 1 deletion(-)
---
diff --git a/fs/proc/task_mmu.c b/fs/proc/task_mmu.c
index 7445980..7c9aad3 100644
--- a/fs/proc/task_mmu.c
+++ b/fs/proc/task_mmu.c
@@ -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 (!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 @@ const 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 (!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)
diff --git a/fs/proc/task_nommu.c b/fs/proc/task_nommu.c
index 7cddf6b..c5783b7 100644
--- a/fs/proc/task_nommu.c
+++ b/fs/proc/task_nommu.c
@@ -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 (!ptrace_may_attach(task))
+ return -EACCES;
+
return nommu_vma_show(m, vml->vma);
}

--
Kees Cook @outflux.net