lkml.org 
[lkml]   [2011]   [Jun]   [20]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    Date
    From
    SubjectRe: [RFC v2] security: intoduce ptrace_task_may_access_current
    Hi Eric,

    On Mon, Jun 20, 2011 at 10:22 -0400, Eric Paris wrote:
    > >diff --git a/include/linux/capability.h b/include/linux/capability.h
    > >index c421123..cc0bcfe 100644
    > >--- a/include/linux/capability.h
    > >+++ b/include/linux/capability.h
    > >@@ -544,7 +544,9 @@ extern bool has_ns_capability(struct task_struct *t,
    > > struct user_namespace *ns, int cap);
    > > extern bool has_capability_noaudit(struct task_struct *t, int cap);
    > > extern bool capable(int cap);
    > >+extern bool task_capable(struct task_struct *task, int cap);
    > > extern bool ns_capable(struct user_namespace *ns, int cap);
    > >+extern bool ns_task_capable(struct task_struct *t, struct user_namespace *ns, int cap);
    > > extern bool task_ns_capable(struct task_struct *t, int cap);
    >
    > now we have ns_task_capable() and task_ns_capable() ? What is the
    > difference? Why do I have 2? Which one do I choose where?

    Hmmm, agreed, I didn't spot it.

    > >diff --git a/include/linux/security.h b/include/linux/security.h
    > >index 8ce59ef..fb79dd5 100644
    > >--- a/include/linux/security.h
    > >+++ b/include/linux/security.h
    > >@@ -56,7 +56,8 @@ struct user_namespace;
    > > extern int cap_capable(struct task_struct *tsk, const struct cred *cred,
    > > struct user_namespace *ns, int cap, int audit);
    > > extern int cap_settime(const struct timespec *ts, const struct timezone *tz);
    > >-extern int cap_ptrace_access_check(struct task_struct *child, unsigned int mode);
    > >+extern int cap_ptrace_access_check(struct task_struct *task, struct task_struct *child,
    > >+ unsigned int mode);
    > > extern int cap_ptrace_traceme(struct task_struct *parent);
    > > extern int cap_capget(struct task_struct *target, kernel_cap_t *effective, kernel_cap_t *inheritable, kernel_cap_t *permitted);
    > > extern int cap_capset(struct cred *new, const struct cred *old,
    > >@@ -1375,7 +1376,9 @@ static inline void security_free_mnt_opts(struct security_mnt_opts *opts)
    > > struct security_operations {
    > > char name[SECURITY_NAME_MAX + 1];
    > >
    > >- int (*ptrace_access_check) (struct task_struct *child, unsigned int mode);
    > >+ int (*ptrace_access_check) (struct task_struct *task,
    > >+ struct task_struct *child,
    > >+ unsigned int mode);
    >
    > formatting nit, this patch lines up args, it doesn't just use tabs
    > for the 2nd/3rd line.

    OK.

    > > int (*ptrace_traceme) (struct task_struct *parent);
    > > int (*capget) (struct task_struct *target,
    > > kernel_cap_t *effective,
    > >@@ -1657,6 +1660,8 @@ extern int security_module_enable(struct security_operations *ops);
    > > extern int register_security(struct security_operations *ops);
    > >
    > > /* Security operations */
    > >+int security_ptrace_task_access_check(struct task_struct *task,
    > >+ struct task_struct *child, unsigned int mode);
    >
    > I thought we agreed to not add a new ptrace_task_access_check(),
    > just fix security_ptrace_access_check() to take the new argument.

    I did it for security ops, will do it for security_ptrace_access_check()
    too.

    > > int security_ptrace_access_check(struct task_struct *child, unsigned int mode);
    > > int security_ptrace_traceme(struct task_struct *parent);
    > > int security_capget(struct task_struct *target,
    > >@@ -1667,6 +1672,10 @@ int security_capset(struct cred *new, const struct cred *old,
    > > const kernel_cap_t *effective,
    > > const kernel_cap_t *inheritable,
    > > const kernel_cap_t *permitted);
    > >+int security_task_capable(struct task_struct *task,
    > >+ struct user_namespace *ns,
    > >+ const struct cred *cred,
    > >+ int cap);
    >
    > Personally I don't love this either and think we should just
    > redefine security_capable.

    OK.

    > > int security_capable(struct user_namespace *ns, const struct cred *cred,
    > > int cap);
    > > int security_real_capable(struct task_struct *tsk, struct user_namespace *ns,
    > >@@ -1837,10 +1846,16 @@ static inline int security_init(void)
    > > return 0;
    > > }
    > >
    > >+static inline int security_ptrace_task_access_check(struct task_struct *task,
    > >+ struct task_struct *child, unsigned int mode)
    > >+{
    > >+ return cap_ptrace_access_check(task, child, mode);
    > >+}
    > >+
    > > static inline int security_ptrace_access_check(struct task_struct *child,
    > > unsigned int mode)
    > > {
    > >- return cap_ptrace_access_check(child, mode);
    > >+ return cap_ptrace_access_check(current, child, mode);
    > > }
    >
    > Lets not introduce security_ptrace_task_access_check() at all. Just
    > add the new argument to security_ptrace_access_check() and fix the
    > single caller (it looks to me like security_ptrace_access_check()
    > has no users after this patch)

    OK.

    > >
    > > static inline int security_ptrace_traceme(struct task_struct *parent)
    > >@@ -1865,10 +1880,18 @@ static inline int security_capset(struct cred *new,
    > > return cap_capset(new, old, effective, inheritable, permitted);
    > > }
    > >
    > >+static inline int security_task_capable(struct task_struct *task,
    > >+ struct user_namespace *ns,
    > >+ const struct cred *cred,
    > >+ int cap)
    > >+{
    > >+ return cap_capable(task, cred, ns, cap, SECURITY_CAP_AUDIT);
    > >+}
    > >+
    > > static inline int security_capable(struct user_namespace *ns,
    > > const struct cred *cred, int cap)
    > > {
    > >- return cap_capable(current, cred, ns, cap, SECURITY_CAP_AUDIT);
    > >+ return security_task_capable(current, ns, cred, cap);
    > > }
    >
    > There is only one caller of security_capable outside in the kernel.
    > Can we just add the task argument rather than make a new function?
    > Even if you want to retain security_capable, define it exactly like
    > this up where you declared the function and remove it everywhere
    > else in the code base.

    OK.

    > > static inline int security_real_capable(struct task_struct *tsk, struct user_namespace *ns, int cap)
    > >diff --git a/kernel/capability.c b/kernel/capability.c
    > >index 283c529..bc9b07f 100644
    > >--- a/kernel/capability.c
    > >+++ b/kernel/capability.c
    > >@@ -356,6 +356,30 @@ bool capable(int cap)
    > > }
    > > EXPORT_SYMBOL(capable);
    > >
    > >+bool task_capable(struct task_struct *task, int cap)
    > >+{
    > >+ return ns_task_capable(task,&init_user_ns, cap);
    > >+}
    > >+EXPORT_SYMBOL(task_capable);
    >
    > Why do we keep adding things like task_capable? Can't we just stop
    > adding non-lsm functions and just call the right LSM functions from
    > now on? This is my original comments mostly directed at Serge. I'm
    > to the point where I want to NAK anything new in kernel/capability.c
    > (and yes, I know i'm guilty in the paste)
    >
    > >+bool ns_task_capable(struct task_struct *task, struct user_namespace *ns, int cap)
    > >+{
    > >+ if (unlikely(!cap_valid(cap))) {
    > >+ printk(KERN_CRIT "capable() called with invalid cap=%u\n", cap);
    > >+ BUG();
    > >+ }
    > >+
    > >+ rcu_read_lock();
    > >+ if (security_task_capable(task, ns, __task_cred(task), cap) == 0) {
    > >+ rcu_read_unlock();
    > >+ current->flags |= PF_SUPERPRIV;

    (fixing my 2 copy-paste bugs, here and below)
    s/current/task/

    > >+ return true;
    > >+ }
    > >+ rcu_read_unlock();
    > >+ return false;
    > >+}
    > >+EXPORT_SYMBOL(ns_task_capable);
    >
    > Ok, NAK. I just can' stomache having a ns_task_capable() and a
    > task_ns_capable(). One of them has to be wrong.

    I'm a bit confused with numerous capable funtions too, but I thought
    they are needed for some abstraction level. I'll remove ns_task_capable().

    > >+
    > > /**
    > > * ns_capable - Determine if the current task has a superior capability in effect
    > > * @ns: The usernamespace we want the capability in
    > >@@ -369,16 +393,7 @@ EXPORT_SYMBOL(capable);
    > > */
    > > bool ns_capable(struct user_namespace *ns, int cap)
    > > {
    > >- if (unlikely(!cap_valid(cap))) {
    > >- printk(KERN_CRIT "capable() called with invalid cap=%u\n", cap);
    > >- BUG();
    > >- }
    > >-
    > >- if (security_capable(ns, current_cred(), cap) == 0) {
    > >- current->flags |= PF_SUPERPRIV;
    > >- return true;
    > >- }
    > >- return false;
    > >+ return ns_task_capable(current, ns, cap);
    > > }
    > > EXPORT_SYMBOL(ns_capable);
    > >
    > >diff --git a/kernel/ptrace.c b/kernel/ptrace.c
    > >index 2df1157..df8fe32 100644
    > >--- a/kernel/ptrace.c
    > >+++ b/kernel/ptrace.c
    > >@@ -132,9 +132,9 @@ int ptrace_check_attach(struct task_struct *child, int kill)
    > > return ret;
    > > }
    > >
    > >-int __ptrace_may_access(struct task_struct *task, unsigned int mode)
    > >+int __ptrace_may_access(struct task_struct *who, struct task_struct *task, unsigned int mode)
    > > {
    > >- const struct cred *cred = current_cred(), *tcred;
    > >+ const struct cred *cred, *tcred;
    > >
    > > /* May we inspect the given task?
    > > * This check is used both for attaching with ptrace
    > >@@ -149,6 +149,7 @@ int __ptrace_may_access(struct task_struct *task, unsigned int mode)
    > > if (task == current)

    s/current/who/


    --
    Vasiliy Kulikov
    http://www.openwall.com - bringing security into open computing environments


    \
     
     \ /
      Last update: 2011-06-20 16:43    [W:0.041 / U:0.096 seconds]
    ©2003-2016 Jasper Spaans. hosted at Digital OceanAdvertise on this site