| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Date: Mon, 12 May 2008 07:06:34 -0700 (PDT)
From: Casey Schaufler <casey@...aufler-ca.com>
To: Stephen Smalley <sds@...ho.nsa.gov>,
lsm <linux-security-module@...r.kernel.org>,
Chris Wright <chrisw@...s-sol.org>,
James Morris <jmorris@...ei.org>,
Eric Paris <eparis@...isplace.org>,
Casey Schaufler <casey@...aufler-ca.com>
Cc: lkml <linux-kernel@...r.kernel.org>
Subject: Re: [RFC][PATCH] security: split ptrace checking in proc
--- 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.
> 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@...ho.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@...aufler-ca.com
--
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