[RFC][PATCH] security: split ptrace checking in proc

[RFC][PATCH] security: split ptrace checking in proc

[Stephen Smalley]

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.

The patch only changes the environ and open file checking in proc.
Other cases such as mem and maps checking still use a full ptrace
check at present.

In the SELinux case, we model such reading of process state as a
reading of the proc file labeled with the process' label.  This enables
SELinux policy to permit such reading of process state without permitting control
or manipulation of the target process, as there are a number of cases
where programs probe for such information via proc but do not need to
be able to control the target.  This restores SELinux behavior prior to
2.6.18.

Signed-off-by:  Stephen Smalley <sds@tycho.nsa.gov>

---

 fs/proc/base.c             |    4 ++--
 include/linux/ptrace.h     |    1 +
 include/linux/security.h   |   15 ++++++++++-----
 kernel/ptrace.c            |   20 +++++++++++++++++---
 security/commoncap.c       |    3 ++-
 security/dummy.c           |    3 ++-
 security/security.c        |    5 +++--
 security/selinux/hooks.c   |   13 +++++++++++--
 security/smack/smack_lsm.c |    5 +++--
 9 files changed, 51 insertions(+), 18 deletions(-)

diff --git a/fs/proc/base.c b/fs/proc/base.c
index 808cbdc..bbc74a0 100644
--- a/fs/proc/base.c
+++ b/fs/proc/base.c
@@ -499,7 +499,7 @@ static int proc_fd_access_allowed(struct inode *inode)
 	 */
 	task = get_proc_task(inode);
 	if (task) {
-		allowed = ptrace_may_attach(task);
+		allowed = ptrace_may_readstate(task);
 		put_task_struct(task);
 	}
 	return allowed;
@@ -885,7 +885,7 @@ static ssize_t environ_read(struct file *file, char __user *buf,
 	if (!task)
 		goto out_no_task;
 
-	if (!ptrace_may_attach(task))
+	if (!ptrace_may_readstate(task))
 		goto out;
 
 	ret = -ENOMEM;
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);
 
 static inline int ptrace_reparented(struct task_struct *child)
 {
diff --git a/include/linux/security.h b/include/linux/security.h
index 50737c7..8841322 100644
--- a/include/linux/security.h
+++ b/include/linux/security.h
@@ -46,7 +46,8 @@ struct audit_krule;
  */
 extern int cap_capable(struct task_struct *tsk, int cap);
 extern int cap_settime(struct timespec *ts, struct timezone *tz);
-extern int cap_ptrace(struct task_struct *parent, struct task_struct *child);
+extern int cap_ptrace(struct task_struct *parent, struct task_struct *child,
+		      bool readstate);
 extern int cap_capget(struct task_struct *target, kernel_cap_t *effective, kernel_cap_t *inheritable, kernel_cap_t *permitted);
 extern int cap_capset_check(struct task_struct *target, kernel_cap_t *effective, kernel_cap_t *inheritable, kernel_cap_t *permitted);
 extern void cap_capset_set(struct task_struct *target, kernel_cap_t *effective, kernel_cap_t *inheritable, kernel_cap_t *permitted);
@@ -1170,6 +1171,7 @@ static inline void security_free_mnt_opts(struct security_mnt_opts *opts)
  *	attributes would be changed by the execve.
  *	@parent contains the task_struct structure for parent process.
  *	@child contains the task_struct structure for child process.
+ *	@readstate is true if this is only a check for reading state from proc.
  *	Return 0 if permission is granted.
  * @capget:
  *	Get the @effective, @inheritable, and @permitted capability sets for
@@ -1295,7 +1297,8 @@ static inline void security_free_mnt_opts(struct security_mnt_opts *opts)
 struct security_operations {
 	char name[SECURITY_NAME_MAX + 1];
 
-	int (*ptrace) (struct task_struct *parent, struct task_struct *child);
+	int (*ptrace) (struct task_struct *parent, struct task_struct *child,
+		       bool readstate);
 	int (*capget) (struct task_struct *target,
 		       kernel_cap_t *effective,
 		       kernel_cap_t *inheritable, kernel_cap_t *permitted);
@@ -1573,7 +1576,8 @@ extern struct dentry *securityfs_create_dir(const char *name, struct dentry *par
 extern void securityfs_remove(struct dentry *dentry);
 
 /* Security operations */
-int security_ptrace(struct task_struct *parent, struct task_struct *child);
+int security_ptrace(struct task_struct *parent, struct task_struct *child,
+		    bool readstate);
 int security_capget(struct task_struct *target,
 		    kernel_cap_t *effective,
 		    kernel_cap_t *inheritable,
@@ -1755,9 +1759,10 @@ static inline int security_init(void)
 	return 0;
 }
 
-static inline int security_ptrace(struct task_struct *parent, struct task_struct *child)
+static inline int security_ptrace(struct task_struct *parent,
+				  struct task_struct *child, bool readstate)
 {
-	return cap_ptrace(parent, child);
+	return cap_ptrace(parent, child, readstate);
 }
 
 static inline int security_capget(struct task_struct *target,
diff --git a/kernel/ptrace.c b/kernel/ptrace.c
index 6c19e94..4b8b3d4 100644
--- a/kernel/ptrace.c
+++ b/kernel/ptrace.c
@@ -121,7 +121,7 @@ int ptrace_check_attach(struct task_struct *child, int kill)
 	return ret;
 }
 
-int __ptrace_may_attach(struct task_struct *task)
+static int ptrace_may_inspect(struct task_struct *task, bool readstate)
 {
 	/* May we inspect the given task?
 	 * This check is used both for attaching with ptrace
@@ -148,7 +148,12 @@ int __ptrace_may_attach(struct task_struct *task)
 	if (!dumpable && !capable(CAP_SYS_PTRACE))
 		return -EPERM;
 
-	return security_ptrace(current, task);
+	return security_ptrace(current, task, readstate);
+}
+
+int __ptrace_may_attach(struct task_struct *task)
+{
+	return ptrace_may_inspect(task, false);
 }
 
 int ptrace_may_attach(struct task_struct *task)
@@ -160,6 +165,15 @@ int ptrace_may_attach(struct task_struct *task)
 	return !err;
 }
 
+int ptrace_may_readstate(struct task_struct *task)
+{
+	int err;
+	task_lock(task);
+	err = ptrace_may_inspect(task, true);
+	task_unlock(task);
+	return !err;
+}
+
 int ptrace_attach(struct task_struct *task)
 {
 	int retval;
@@ -494,7 +508,7 @@ int ptrace_traceme(void)
 	 */
 	task_lock(current);
 	if (!(current->ptrace & PT_PTRACED)) {
-		ret = security_ptrace(current->parent, current);
+		ret = security_ptrace(current->parent, current, false);
 		/*
 		 * Set the ptrace bit in the process ptrace flags.
 		 */
diff --git a/security/commoncap.c b/security/commoncap.c
index 5edabc7..5cdb370 100644
--- a/security/commoncap.c
+++ b/security/commoncap.c
@@ -63,7 +63,8 @@ int cap_settime(struct timespec *ts, struct timezone *tz)
 	return 0;
 }
 
-int cap_ptrace (struct task_struct *parent, struct task_struct *child)
+int cap_ptrace (struct task_struct *parent, struct task_struct *child,
+		bool readstate)
 {
 	/* Derived from arch/i386/kernel/ptrace.c:sys_ptrace. */
 	if (!cap_issubset(child->cap_permitted, parent->cap_permitted) &&
diff --git a/security/dummy.c b/security/dummy.c
index f50c6c3..94b5836 100644
--- a/security/dummy.c
+++ b/security/dummy.c
@@ -28,7 +28,8 @@
 #include <linux/ptrace.h>
 #include <linux/file.h>
 
-static int dummy_ptrace (struct task_struct *parent, struct task_struct *child)
+static int dummy_ptrace (struct task_struct *parent, struct task_struct *child,
+			 bool readstate)
 {
 	return 0;
 }
diff --git a/security/security.c b/security/security.c
index 59838a9..7867665 100644
--- a/security/security.c
+++ b/security/security.c
@@ -161,9 +161,10 @@ int mod_reg_security(const char *name, struct security_operations *ops)
 
 /* Security operations */
 
-int security_ptrace(struct task_struct *parent, struct task_struct *child)
+int security_ptrace(struct task_struct *parent, struct task_struct *child,
+		    bool readstate)
 {
-	return security_ops->ptrace(parent, child);
+	return security_ops->ptrace(parent, child, readstate);
 }
 
 int security_capget(struct task_struct *target,
diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c
index 59c6e98..d30bb92 100644
--- a/security/selinux/hooks.c
+++ b/security/selinux/hooks.c
@@ -1682,14 +1682,23 @@ static inline u32 file_to_av(struct file *file)
 
 /* Hook functions begin here. */
 
-static int selinux_ptrace(struct task_struct *parent, struct task_struct *child)
+static int selinux_ptrace(struct task_struct *parent,
+			  struct task_struct *child,
+			  bool readstate)
 {
 	int rc;
 
-	rc = secondary_ops->ptrace(parent, child);
+	rc = secondary_ops->ptrace(parent, child, readstate);
 	if (rc)
 		return rc;
 
+	if (readstate) {
+		struct task_security_struct *tsec = parent->security;
+		struct task_security_struct *csec = child->security;
+		return avc_has_perm(tsec->sid, csec->sid,
+				    SECCLASS_FILE, FILE__READ, NULL);
+	}
+
 	return task_has_perm(parent, child, PROCESS__PTRACE);
 }
 
diff --git a/security/smack/smack_lsm.c b/security/smack/smack_lsm.c
index b5c8f92..88f158e 100644
--- a/security/smack/smack_lsm.c
+++ b/security/smack/smack_lsm.c
@@ -95,11 +95,12 @@ struct inode_smack *new_inode_smack(char *smack)
  *
  * Do the capability checks, and require read and write.
  */
-static int smack_ptrace(struct task_struct *ptp, struct task_struct *ctp)
+static int smack_ptrace(struct task_struct *ptp, struct task_struct *ctp,
+			bool readstate)
 {
 	int rc;
 
-	rc = cap_ptrace(ptp, ctp);
+	rc = cap_ptrace(ptp, ctp, readstate);
 	if (rc != 0)
 		return rc;
 

-- 
Stephen Smalley
National Security Agency

Re: [RFC][PATCH] security: split ptrace checking in proc

[Casey Schaufler]

--- Stephen Smalley <sds@tycho.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.

> The patch only changes the environ and open file checking in proc.
> Other cases such as mem and maps checking still use a full ptrace
> check at present.
> 
> In the SELinux case, we model such reading of process state as a
> reading of the proc file labeled with the process' label.  This enables
> SELinux policy to permit such reading of process state without permitting
> control
> or manipulation of the target process, as there are a number of cases
> where programs probe for such information via proc but do not need to
> be able to control the target.  This restores SELinux behavior prior to
> 2.6.18.

All quite reasonable. Although I wouldn't do it myself, I could
imagine an LSM that would want a finer granularity on ptrace.

> Signed-off-by:  Stephen Smalley <sds@tycho.nsa.gov>
> 
> ---
> 
>  fs/proc/base.c             |    4 ++--
>  include/linux/ptrace.h     |    1 +
>  include/linux/security.h   |   15 ++++++++++-----
>  kernel/ptrace.c            |   20 +++++++++++++++++---
>  security/commoncap.c       |    3 ++-
>  security/dummy.c           |    3 ++-
>  security/security.c        |    5 +++--
>  security/selinux/hooks.c   |   13 +++++++++++--
>  security/smack/smack_lsm.c |    5 +++--
>  9 files changed, 51 insertions(+), 18 deletions(-)
> 
> diff --git a/fs/proc/base.c b/fs/proc/base.c
> index 808cbdc..bbc74a0 100644
> --- a/fs/proc/base.c
> +++ b/fs/proc/base.c
> @@ -499,7 +499,7 @@ static int proc_fd_access_allowed(struct inode *inode)
>  	 */
>  	task = get_proc_task(inode);
>  	if (task) {
> -		allowed = ptrace_may_attach(task);
> +		allowed = ptrace_may_readstate(task);
>  		put_task_struct(task);
>  	}
>  	return allowed;
> @@ -885,7 +885,7 @@ static ssize_t environ_read(struct file *file, char
> __user *buf,
>  	if (!task)
>  		goto out_no_task;
>  
> -	if (!ptrace_may_attach(task))
> +	if (!ptrace_may_readstate(task))
>  		goto out;
>  
>  	ret = -ENOMEM;
> 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.

>  static inline int ptrace_reparented(struct task_struct *child)
>  {
> diff --git a/include/linux/security.h b/include/linux/security.h
> index 50737c7..8841322 100644
> --- a/include/linux/security.h
> +++ b/include/linux/security.h
> @@ -46,7 +46,8 @@ struct audit_krule;
>   */
>  extern int cap_capable(struct task_struct *tsk, int cap);
>  extern int cap_settime(struct timespec *ts, struct timezone *tz);
> -extern int cap_ptrace(struct task_struct *parent, struct task_struct
> *child);
> +extern int cap_ptrace(struct task_struct *parent, struct task_struct *child,
> +		      bool readstate);
>  extern int cap_capget(struct task_struct *target, kernel_cap_t *effective,
> kernel_cap_t *inheritable, kernel_cap_t *permitted);
>  extern int cap_capset_check(struct task_struct *target, kernel_cap_t
> *effective, kernel_cap_t *inheritable, kernel_cap_t *permitted);
>  extern void cap_capset_set(struct task_struct *target, kernel_cap_t
> *effective, kernel_cap_t *inheritable, kernel_cap_t *permitted);
> @@ -1170,6 +1171,7 @@ static inline void security_free_mnt_opts(struct
> security_mnt_opts *opts)
>   *	attributes would be changed by the execve.
>   *	@parent contains the task_struct structure for parent process.
>   *	@child contains the task_struct structure for child process.
> + *	@readstate is true if this is only a check for reading state from proc.
>   *	Return 0 if permission is granted.
>   * @capget:
>   *	Get the @effective, @inheritable, and @permitted capability sets for
> @@ -1295,7 +1297,8 @@ static inline void security_free_mnt_opts(struct
> security_mnt_opts *opts)
>  struct security_operations {
>  	char name[SECURITY_NAME_MAX + 1];
>  
> -	int (*ptrace) (struct task_struct *parent, struct task_struct *child);
> +	int (*ptrace) (struct task_struct *parent, struct task_struct *child,
> +		       bool readstate);
>  	int (*capget) (struct task_struct *target,
>  		       kernel_cap_t *effective,
>  		       kernel_cap_t *inheritable, kernel_cap_t *permitted);
> @@ -1573,7 +1576,8 @@ extern struct dentry *securityfs_create_dir(const char
> *name, struct dentry *par
>  extern void securityfs_remove(struct dentry *dentry);
>  
>  /* Security operations */
> -int security_ptrace(struct task_struct *parent, struct task_struct *child);
> +int security_ptrace(struct task_struct *parent, struct task_struct *child,
> +		    bool readstate);
>  int security_capget(struct task_struct *target,
>  		    kernel_cap_t *effective,
>  		    kernel_cap_t *inheritable,
> @@ -1755,9 +1759,10 @@ static inline int security_init(void)
>  	return 0;
>  }
>  
> -static inline int security_ptrace(struct task_struct *parent, struct
> task_struct *child)
> +static inline int security_ptrace(struct task_struct *parent,
> +				  struct task_struct *child, bool readstate)
>  {
> -	return cap_ptrace(parent, child);
> +	return cap_ptrace(parent, child, readstate);
>  }
>  
>  static inline int security_capget(struct task_struct *target,
> diff --git a/kernel/ptrace.c b/kernel/ptrace.c
> index 6c19e94..4b8b3d4 100644
> --- a/kernel/ptrace.c
> +++ b/kernel/ptrace.c
> @@ -121,7 +121,7 @@ int ptrace_check_attach(struct task_struct *child, int
> kill)
>  	return ret;
>  }
>  
> -int __ptrace_may_attach(struct task_struct *task)
> +static int ptrace_may_inspect(struct task_struct *task, bool readstate)
>  {
>  	/* May we inspect the given task?
>  	 * This check is used both for attaching with ptrace
> @@ -148,7 +148,12 @@ int __ptrace_may_attach(struct task_struct *task)
>  	if (!dumpable && !capable(CAP_SYS_PTRACE))
>  		return -EPERM;
>  
> -	return security_ptrace(current, task);
> +	return security_ptrace(current, task, readstate);
> +}
> +
> +int __ptrace_may_attach(struct task_struct *task)
> +{
> +	return ptrace_may_inspect(task, false);
>  }
>  
>  int ptrace_may_attach(struct task_struct *task)
> @@ -160,6 +165,15 @@ int ptrace_may_attach(struct task_struct *task)
>  	return !err;
>  }
>  
> +int ptrace_may_readstate(struct task_struct *task)
> +{
> +	int err;
> +	task_lock(task);
> +	err = ptrace_may_inspect(task, true);
> +	task_unlock(task);
> +	return !err;
> +}
> +
>  int ptrace_attach(struct task_struct *task)
>  {
>  	int retval;
> @@ -494,7 +508,7 @@ int ptrace_traceme(void)
>  	 */
>  	task_lock(current);
>  	if (!(current->ptrace & PT_PTRACED)) {
> -		ret = security_ptrace(current->parent, current);
> +		ret = security_ptrace(current->parent, current, false);
>  		/*
>  		 * Set the ptrace bit in the process ptrace flags.
>  		 */
> diff --git a/security/commoncap.c b/security/commoncap.c
> index 5edabc7..5cdb370 100644
> --- a/security/commoncap.c
> +++ b/security/commoncap.c
> @@ -63,7 +63,8 @@ int cap_settime(struct timespec *ts, struct timezone *tz)
>  	return 0;
>  }
>  
> -int cap_ptrace (struct task_struct *parent, struct task_struct *child)
> +int cap_ptrace (struct task_struct *parent, struct task_struct *child,
> +		bool readstate)
>  {
>  	/* Derived from arch/i386/kernel/ptrace.c:sys_ptrace. */
>  	if (!cap_issubset(child->cap_permitted, parent->cap_permitted) &&
> diff --git a/security/dummy.c b/security/dummy.c
> index f50c6c3..94b5836 100644
> --- a/security/dummy.c
> +++ b/security/dummy.c
> @@ -28,7 +28,8 @@
>  #include <linux/ptrace.h>
>  #include <linux/file.h>
>  
> -static int dummy_ptrace (struct task_struct *parent, struct task_struct
> *child)
> +static int dummy_ptrace (struct task_struct *parent, struct task_struct
> *child,
> +			 bool readstate)
>  {
>  	return 0;
>  }
> diff --git a/security/security.c b/security/security.c
> index 59838a9..7867665 100644
> --- a/security/security.c
> +++ b/security/security.c
> @@ -161,9 +161,10 @@ int mod_reg_security(const char *name, struct
> security_operations *ops)
>  
>  /* Security operations */
>  
> -int security_ptrace(struct task_struct *parent, struct task_struct *child)
> +int security_ptrace(struct task_struct *parent, struct task_struct *child,
> +		    bool readstate)
>  {
> -	return security_ops->ptrace(parent, child);
> +	return security_ops->ptrace(parent, child, readstate);
>  }
>  
>  int security_capget(struct task_struct *target,
> diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c
> index 59c6e98..d30bb92 100644
> --- a/security/selinux/hooks.c
> +++ b/security/selinux/hooks.c
> @@ -1682,14 +1682,23 @@ static inline u32 file_to_av(struct file *file)
>  
>  /* Hook functions begin here. */
>  
> -static int selinux_ptrace(struct task_struct *parent, struct task_struct
> *child)
> +static int selinux_ptrace(struct task_struct *parent,
> +			  struct task_struct *child,
> +			  bool readstate)
>  {
>  	int rc;
>  
> -	rc = secondary_ops->ptrace(parent, child);
> +	rc = secondary_ops->ptrace(parent, child, readstate);
>  	if (rc)
>  		return rc;
>  
> +	if (readstate) {
> +		struct task_security_struct *tsec = parent->security;
> +		struct task_security_struct *csec = child->security;
> +		return avc_has_perm(tsec->sid, csec->sid,
> +				    SECCLASS_FILE, FILE__READ, NULL);
> +	}
> +
>  	return task_has_perm(parent, child, PROCESS__PTRACE);
>  }
>  
> diff --git a/security/smack/smack_lsm.c b/security/smack/smack_lsm.c
> index b5c8f92..88f158e 100644
> --- a/security/smack/smack_lsm.c
> +++ b/security/smack/smack_lsm.c
> @@ -95,11 +95,12 @@ struct inode_smack *new_inode_smack(char *smack)
>   *
>   * Do the capability checks, and require read and write.
>   */
> -static int smack_ptrace(struct task_struct *ptp, struct task_struct *ctp)
> +static int smack_ptrace(struct task_struct *ptp, struct task_struct *ctp,
> +			bool readstate)
>  {
>  	int rc;
>  
> -	rc = cap_ptrace(ptp, ctp);
> +	rc = cap_ptrace(ptp, ctp, readstate);
>  	if (rc != 0)
>  		return rc;

Delta my previous comments, this looks fine.

>  
> 
> -- 
> Stephen Smalley
> National Security Agency
> 
> 
> 


Casey Schaufler
casey@schaufler-ca.com

Re: [RFC][PATCH] security: split ptrace checking in proc

[Stephen Smalley]

On Mon, 2008-05-12 at 07:06 -0700, Casey Schaufler wrote:
> --- Stephen Smalley <sds@tycho.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.

A normal ptrace attach implies full read-write (and more - control of
execution flow) access.  Also, access to /proc/pid/mem (i.e. the process
memory) has always required full ptrace access, even if only for read
(and mem_write is disabled by default).  We aren't changing that
situation with this patch.

What we are doing in this patch is enabling distinctions to be made by
security modules (without disturbing the base DAC/capabilities logic)
between such full ptrace access and reading of state such
as /proc/pid/environ, /proc/pid/exe and /proc/pid/fd/.  We often
encounter the latter in various programs that do not in fact need or
want to be able to ptrace the target process (e.g. monitoring programs,
killproc, mingetty, PolicyKit).  At present, we have to choose between
allowing this in policy (more permissive than required/desired) or
breaking functionality.  This was effectively a regression introduced
into SELinux by unrelated changes to proc back in 2.6.18 to tighten up
the DAC checking, which we aren't disturbing here.

> > 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.

That would require updating all callers to pass the mode vs. only
changing a few callers.  Also, it isn't just an access mode distinction
per se but rather a distinction between read access to parts of the
process state vs. full access to the process state, with the former
required by a variety of programs and the latter only required/desired
for full ptrace purposes.  The mem_read case is an example where we are
still applying a full ptrace check even though we are only reading (but
there we are reading the full process memory vs. just parts of its state
like its environ, exe link, and fd links).

I could do it the way you describe, but I'm not sure it will yield a
better result.  Maybe others can chime in with their preferences and I
can go with whatever consensus emerges.

-- 
Stephen Smalley
National Security Agency

Re: [RFC][PATCH] security: split ptrace checking in proc

[Stephen Smalley]

On Mon, 2008-05-12 at 07:06 -0700, Casey Schaufler wrote:
> --- Stephen Smalley <sds@tycho.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

Re: [RFC][PATCH] security: split ptrace checking in proc

[Chris Wright]

* Stephen Smalley (sds@tycho.nsa.gov) wrote:
> 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.

It is slightly ad-hoc.  Is it just the audit messages that you described
that made you pick environ and fd, or was there more specific (threat
based) reasoning?  Would /proc/pid/fd/ + genfs + e.g. anonfd be a little
wider than just readstate?

Perhaps you could update the comments in ptrace_may_inspect() to clarify.

thanks,
-chris

Re: [RFC][PATCH] security: split ptrace checking in proc

[Stephen Smalley]

On Wed, 2008-05-14 at 02:15 -0700, Chris Wright wrote:
> * Stephen Smalley (sds@tycho.nsa.gov) wrote:
> > 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.
> 
> It is slightly ad-hoc.  Is it just the audit messages that you described
> that made you pick environ and fd, or was there more specific (threat
> based) reasoning?  Would /proc/pid/fd/ + genfs + e.g. anonfd be a little
> wider than just readstate?

Well, it is being driven by experience with what applications try to
access w/o requiring full ptrace access, but also by a threat-based
reasoning that it is less dangerous to grant limited read access to
parts of the process state than to grant complete read access to its
entire memory image or full control of the target process.

Not entirely sure what you mean by the latter question.

> Perhaps you could update the comments in ptrace_may_inspect() to clarify.

Ok, will do.

-- 
Stephen Smalley
National Security Agency

Re: [RFC][PATCH] security: split ptrace checking in proc

[Chris Wright]

* Stephen Smalley (sds@tycho.nsa.gov) wrote:
> On Wed, 2008-05-14 at 02:15 -0700, Chris Wright wrote:
> > It is slightly ad-hoc.  Is it just the audit messages that you described
> > that made you pick environ and fd, or was there more specific (threat
> > based) reasoning?  Would /proc/pid/fd/ + genfs + e.g. anonfd be a little
> > wider than just readstate?
> 
> Well, it is being driven by experience with what applications try to
> access w/o requiring full ptrace access, but also by a threat-based
> reasoning that it is less dangerous to grant limited read access to
> parts of the process state than to grant complete read access to its
> entire memory image or full control of the target process.
> 
> Not entirely sure what you mean by the latter question.

fd/ access gives a view in the ->files, which could include rather
internal bits like pipes, sockets, or anonfd descriptors -- things w/out
external handles.  That view includes ability to open the fd (similar
to dup()) and use it (granted subject to further security checks, but
they may be quite generic at that point).

thanks,
-chris

Re: [RFC][PATCH] security: split ptrace checking in proc

[Stephen Smalley]

On Wed, 2008-05-14 at 08:28 -0700, Chris Wright wrote:
> * Stephen Smalley (sds@tycho.nsa.gov) wrote:
> > On Wed, 2008-05-14 at 02:15 -0700, Chris Wright wrote:
> > > It is slightly ad-hoc.  Is it just the audit messages that you described
> > > that made you pick environ and fd, or was there more specific (threat
> > > based) reasoning?  Would /proc/pid/fd/ + genfs + e.g. anonfd be a little
> > > wider than just readstate?
> > 
> > Well, it is being driven by experience with what applications try to
> > access w/o requiring full ptrace access, but also by a threat-based
> > reasoning that it is less dangerous to grant limited read access to
> > parts of the process state than to grant complete read access to its
> > entire memory image or full control of the target process.
> > 
> > Not entirely sure what you mean by the latter question.
> 
> fd/ access gives a view in the ->files, which could include rather
> internal bits like pipes, sockets, or anonfd descriptors -- things w/out
> external handles.  That view includes ability to open the fd (similar
> to dup()) and use it (granted subject to further security checks, but
> they may be quite generic at that point).

What do you mean by "generic" in the above?  Just the fact that there
wouldn't be any distinction between such access and access to a
descriptor received explicitly via local IPC from the target task?

Ok, so perhaps the only distinction that makes sense is read vs.
write/control, with all checks within proc except mem_write using the
former and ptrace_attach and mem_write using the latter?

-- 
Stephen Smalley
National Security Agency

Re: [RFC][PATCH] security: split ptrace checking in proc

[Chris Wright]

* Stephen Smalley (sds@tycho.nsa.gov) wrote:
> What do you mean by "generic" in the above?  Just the fact that there
> wouldn't be any distinction between such access and access to a
> descriptor received explicitly via local IPC from the target task?

Basically, yeah.

> Ok, so perhaps the only distinction that makes sense is read vs.
> write/control, with all checks within proc except mem_write using the
> former and ptrace_attach and mem_write using the latter?

Yeah, that's what I was wondering, because maps seems to fall into the
readstate category as much as fd/ does (probably fdinfo/ is closer to
maps).

thanks,
-chris