Q: proc: disable mem_write after exec

Q: proc: disable mem_write after exec

[Oleg Nesterov]

Hello.

The last question, I promise ;)

Another change we probably need to backport,

    commit 26947f8c8f9598209001cdcd31bb2162a2e54691
    proc: disable mem_write after exec

    This change makes mem_write() observe the same constraints as mem_read().

mem_read() looks equally confusing to me, may be Linus can explain.
This self_exec_id check was added in 29f279c7 (history tree)
"v2.4.5.5 -> v2.4.5.6", the changelog can't help.

    This
    is particularly important for mem_write as an accidental leak of the fd across
    an exec could result in arbitrary modification of the target process' memory.

    ...

	@@ -850,6 +850,10 @@ static ssize_t mem_write(struct file * file, const char __user *buf,
		if (check_mem_permission(task))
			goto out;
	 
	+	copied = -EIO;
	+	if (file->private_data != (void *)((long)current->self_exec_id))
	+		goto out;
	+
		copied = -ENOMEM;
		page = (char *)__get_free_page(GFP_TEMPORARY);
		if (!page)


Could you explain this? Why this is wrong from the security pov? We are
the tracer, this was checked by check_mem_permission(). We can modify
this memory even without /proc/pid/mem.

And in any case, why do we check current's self_exec_id? I'd understand
if mem_open/mem_read/mem_writed used task->self_exec_id, see the trivial
patch below. With this patch this check means: this task has changed its
->mm after /proc/pid/mem was opened, abort. And perhaps this was the
actual intent. May be makes sense.

But the real question is, why (from the security pov) we can't simply kill
these self_exec_id checks?

Not to mention, it would be nice to remove self_exec_id/parent_exec_id too.
Ignoring mem_open(), afaics the _only_ reason we need these id's is that
linux allows clone(CLONE_PARENT | SIGKILL).

Thanks,

Oleg.


--- x/fs/proc/base.c
+++ x/fs/proc/base.c
@@ -816,7 +816,17 @@ static const struct file_operations proc
 
 static int mem_open(struct inode* inode, struct file* file)
 {
-	file->private_data = (void*)((long)current->self_exec_id);
+	struct task_struct *task;
+
+	rcu_read_lock();
+	task = pid_task(proc_pid(file->f_path.dentry->d_inode), PIDTYPE_PID);
+	if (task)
+		file->private_data = (void*)((long)task->self_exec_id);
+	rcu_read_unlock();
+
+	if (!task)
+		return -EINVAL;
+
 	/* OK to pass negative loff_t, we can catch out-of-range */
 	file->f_mode |= FMODE_UNSIGNED_OFFSET;
 	return 0;
@@ -846,7 +856,7 @@ static ssize_t mem_read(struct file * fi
 
 	ret = -EIO;
  
-	if (file->private_data != (void*)((long)current->self_exec_id))
+	if (file->private_data != (void*)((long)task->self_exec_id))
 		goto out_put;
 
 	ret = 0;
@@ -908,7 +918,7 @@ static ssize_t mem_write(struct file * f
 		goto out_free;
 
 	copied = -EIO;
-	if (file->private_data != (void *)((long)current->self_exec_id))
+	if (file->private_data != (void *)((long)task->self_exec_id))
 		goto out_mm;
 
 	copied = 0;

Re: Q: proc: disable mem_write after exec

[Stephen Wilson]

On Thu, Sep 29, 2011 at 04:17:06PM +0200, Oleg Nesterov wrote:
> The last question, I promise ;)

No worries :)

> 	@@ -850,6 +850,10 @@ static ssize_t mem_write(struct file * file, const char __user *buf,
> 		if (check_mem_permission(task))
> 			goto out;
> 	 
> 	+	copied = -EIO;
> 	+	if (file->private_data != (void *)((long)current->self_exec_id))
> 	+		goto out;
> 	+
> 		copied = -ENOMEM;
> 		page = (char *)__get_free_page(GFP_TEMPORARY);
> 		if (!page)
> 
> 
> Could you explain this? Why this is wrong from the security pov? We are
> the tracer, this was checked by check_mem_permission(). We can modify
> this memory even without /proc/pid/mem.

As far as I understand this is really just a paranoia thing, not a
security issue in any deep sense. If the fd is leaked across an exec()
then the target process memory could be written to by accident. So the
intent here was to protect against buggy userspace more than anything
else. 


> And in any case, why do we check current's self_exec_id? I'd understand
> if mem_open/mem_read/mem_writed used task->self_exec_id, see the trivial
> patch below. With this patch this check means: this task has changed its
> ->mm after /proc/pid/mem was opened, abort. And perhaps this was the
> actual intent. May be makes sense.

Perhaps, but my feeling is that the tracer should be prepared to deal
with that.  From the user perspective I think of /proc/pid/mem as being
associated with a pid, not an mm, and I can't see the benefit of going
thru a close()/open() cycle to deal with an event that can be detected
using ptrace.


> But the real question is, why (from the security pov) we can't simply kill
> these self_exec_id checks?
> 
> Not to mention, it would be nice to remove self_exec_id/parent_exec_id too.
> Ignoring mem_open(), afaics the _only_ reason we need these id's is that
> linux allows clone(CLONE_PARENT | SIGKILL).

So I think this is just a case of weighing the costs and benefits.
Personally, I think having /proc/pid/mem implicitly CLOEXEC is the
"right" thing to do.  But I wouldn't argue if the checks were dropped in
an effort to simplify code.



Thanks,


-- 
steve

Re: Q: proc: disable mem_write after exec

[Oleg Nesterov]

On 09/29, Stephen Wilson wrote:
>
> On Thu, Sep 29, 2011 at 04:17:06PM +0200, Oleg Nesterov wrote:
>
> > 	@@ -850,6 +850,10 @@ static ssize_t mem_write(struct file * file, const char __user *buf,
> > 		if (check_mem_permission(task))
> > 			goto out;
> >
> > 	+	copied = -EIO;
> > 	+	if (file->private_data != (void *)((long)current->self_exec_id))
> > 	+		goto out;
> > 	+
> > 		copied = -ENOMEM;
> > 		page = (char *)__get_free_page(GFP_TEMPORARY);
> > 		if (!page)
> >
> >
> > Could you explain this? Why this is wrong from the security pov? We are
> > the tracer, this was checked by check_mem_permission(). We can modify
> > this memory even without /proc/pid/mem.
>
> As far as I understand this is really just a paranoia thing, not a
> security issue in any deep sense. If the fd is leaked across an exec()
> then the target process memory could be written to by accident. So the
> intent here was to protect against buggy userspace more than anything
> else.

Hmm. OK, in this case I won't argue.

I thought that (rightly or not) the intent was to close the known
security problem which I do not understand.

And, I do not understand security at all, but I am wondering if the
usage of ptrace_parent() in security/ is really correct, the original
attacher can do suid-exec (and loose the control, yes).  But this is
off-topic.

> > And in any case, why do we check current's self_exec_id? I'd understand
> > if mem_open/mem_read/mem_writed used task->self_exec_id, see the trivial
> > patch below. With this patch this check means: this task has changed its
> > ->mm after /proc/pid/mem was opened, abort. And perhaps this was the
> > actual intent. May be makes sense.
>
> Perhaps, but my feeling is that the tracer should be prepared to deal
> with that.  From the user perspective I think of /proc/pid/mem as being
> associated with a pid, not an mm, and I can't see the benefit of going
> thru a close()/open() cycle to deal with an event that can be detected
> using ptrace.

Yes, agreed.

In any case we can't do this change, this can break the debugger which
uses the pre-opened file after the tracer execs. I only tried to understand
what was the actual intent.

> > But the real question is, why (from the security pov) we can't simply kill
> > these self_exec_id checks?
> >
> > Not to mention, it would be nice to remove self_exec_id/parent_exec_id too.
> > Ignoring mem_open(), afaics the _only_ reason we need these id's is that
> > linux allows clone(CLONE_PARENT | SIGKILL).
>
> So I think this is just a case of weighing the costs and benefits.
> Personally, I think having /proc/pid/mem implicitly CLOEXEC is the
> "right" thing to do.  But I wouldn't argue if the checks were dropped in
> an effort to simplify code.

OK, thanks.

Oleg.