Message-Id: <1210687270.6206.129.camel@moss-spartans.epoch.ncsc.mil>
Date:	Tue, 13 May 2008 10:01:10 -0400
From:	Stephen Smalley <sds@...ho.nsa.gov>
To:	casey@...aufler-ca.com
Cc:	lsm <linux-security-module@...r.kernel.org>,
	Chris Wright <chrisw@...s-sol.org>,
	James Morris <jmorris@...ei.org>,
	Eric Paris <eparis@...isplace.org>,
	lkml <linux-kernel@...r.kernel.org>
Subject: Re: [RFC][PATCH] security:  split ptrace checking in proc


On Mon, 2008-05-12 at 07:06 -0700, Casey Schaufler wrote:
> --- Stephen Smalley <sds@...ho.nsa.gov> wrote:
> 
> > Enable security modules to distinguish reading of process state
> > information from full ptrace access by introducing a distinct helper
> > function for such checks and passing a boolean flag down to the
> > security_ptrace hook.  This allows security modules to permit access
> > to reading process state without granting full ptrace access.
> 
> This will obviously suffice, but why pass a boolean instead
> of the access actually desired? What I mean is that instead
> of passing a read-only flag, you could pass in READ or READWRITE
> to indicate which access you require. Although I don't have
> a case in mind, it seems that your interface is unnecessarily
> contrained if you have a read-only boolean.

It isn't quite a read vs. readwrite distinction.  A normal ptrace attach
implies full control of the target process (which goes even beyond
simple read/write to control flow).  Read access to /proc/pid/mem (the
process memory) has also always required full ptrace access, and we
aren't changing that situation with this patch as I'm not aware of any
need to allow it w/o allowing full ptrace access.

What the patch is doing is enabling distinctions to be made by security
modules (without disturbing the base DAC or capability checks) between
full ptrace access and limited reading of specific elements of state,
such as reading /proc/pid/environ or reading the special
symlinks /proc/pid/exec and /proc/pid/fd/<n>.  We often encounter the
latter in various programs that do not need to be able to ptrace the
target process (e.g. monitoring programs, procps, lsof, PolicyKit).  At
present we have to choose between allowing full ptrace in policy (more
permissive than required/desired) or breaking functionality (or in some
cases, just silencing the denials via dontaudit but this can hide
genuine attacks).

> > diff --git a/include/linux/ptrace.h b/include/linux/ptrace.h
> > index f98501b..f8a5e75 100644
> > --- a/include/linux/ptrace.h
> > +++ b/include/linux/ptrace.h
> > @@ -97,6 +97,7 @@ extern void __ptrace_unlink(struct task_struct *child);
> >  extern void ptrace_untrace(struct task_struct *child);
> >  extern int ptrace_may_attach(struct task_struct *task);
> >  extern int __ptrace_may_attach(struct task_struct *task);
> > +extern int ptrace_may_readstate(struct task_struct *task);
> 
> I would prefer a mode parameter to ptrace_may_attach to the
> specific function for read access. Again, what you have will
> work for your case, but may lead to yet another interface later
> if someone wants a slightly different check.

As above, it isn't quite a read vs. readwrite mode distinction (which is
why I called it ptrace_may_readstate rather than just ptrace_may_read),
and the advantage of implementing it via a new interface is that we only
need to update the callers where we want to apply this different
checking, leaving all other callers unmodified and unaffected.  So while
I could do it the way you describe, I'm not sure it would yield a better
result.  Maybe others can chime in with their opinions.

-- 
Stephen Smalley
National Security Agency

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Powered by blists - more mailing lists