Re: [PATCH v3] ptrace: reintroduce usage of subjective credentials in ptrace_has_cap()

[Christian Brauner]

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