On January 17, 2020 10:15:04 PM GMT+01:00, Kees Cook <keescook@xxxxxxxxxxxx> wrote:
>On Fri, Jan 17, 2020 at 11:57:18AM +0100, Christian Brauner wrote:
>> -static int ptrace_has_cap(struct user_namespace *ns, unsigned int
>mode)
>> +static int ptrace_has_cap(const struct cred *cred, struct
>user_namespace *ns,
>> + unsigned int mode)
>> {
>> - if (mode & PTRACE_MODE_NOAUDIT)
>> - return has_ns_capability_noaudit(current, ns, CAP_SYS_PTRACE);
>> - else
>> - return has_ns_capability(current, ns, CAP_SYS_PTRACE);
>> + return security_capable(cred, ns, CAP_SYS_PTRACE,
>> + (mode & PTRACE_MODE_NOAUDIT) ? CAP_OPT_NOAUDIT :
>> + CAP_OPT_NONE);
>> }
>
>Eek, no. I think this inverts the check.
>
>Before:
>bool has_ns_capability(struct task_struct *t,
> struct user_namespace *ns, int cap)
>{
> ...
> ret = security_capable(__task_cred(t), ns, cap, CAP_OPT_NONE);
> ...
> return (ret == 0);
>}
>
>static int ptrace_has_cap(struct user_namespace *ns, unsigned int mode)
>{
> ...
> return has_ns_capability(current, ns, CAP_SYS_PTRACE);
>}
>
>After:
>static int ptrace_has_cap(const struct cred *cred, struct
>user_namespace *ns,
> unsigned int mode)
>{
> return security_capable(cred, ns, CAP_SYS_PTRACE,
> (mode & PTRACE_MODE_NOAUDIT) ? CAP_OPT_NOAUDIT :
> CAP_OPT_NONE);
>}
>
>Note lack of "== 0" on the security_capable() return value, but it's
>needed. To avoid confusion, I think ptrace_has_cap() should likely
>return bool too.
>
>-Kees
Ok, I'll make it bool. Can I retain your reviewed-by or do you want to provide a new one?
I want to have this in mainline asap because this is a cve waiting to happen as soon as io_uring for open and openat lands in v5.6.
I plan on sending a on sending a pr before Sunday.
Christian
