On January 17, 2020 10:15:04 PM GMT+01:00, Kees Cook keescook@chromium.org 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