On Fri, Jan 17, 2020 at 06:16:23AM +0100, Christian Brauner wrote:
On Thu, Jan 16, 2020 at 06:29:26PM -0800, Kees Cook wrote:
On Thu, Jan 16, 2020 at 11:45:18PM +0100, Christian Brauner wrote:
Commit 69f594a38967 ("ptrace: do not audit capability check when outputing /proc/pid/stat") introduced the ability to opt out of audit messages for accesses to various proc files since they are not violations of policy. While doing so it somehow switched the check from ns_capable() to has_ns_capability{_noaudit}(). That means it switched from checking the subjective credentials of the task to using the objective credentials. I couldn't find the original lkml thread and so I don't know why this switch was done. But it seems wrong since ptrace_has_cap() is currently only used in ptrace_may_access(). And it's used to check whether the calling task (subject) has the CAP_SYS_PTRACE capability in the provided user namespace to operate on the target task (object). According to the cred.h comments this would mean the subjective credentials of the calling task need to be used.
I don't follow this description. As far as I can see, both the current code and your patch end up using current's cred, yes? I'm not following the subjective/objective change mentioned here.
Before: bool has_ns_capability(struct task_struct *t, struct user_namespace *ns, int cap) { int ret;
rcu_read_lock(); ret = security_capable(__task_cred(t), ns, cap, CAP_OPT_NONE);If I'm not mistaken, you're looking at the cuplrit: "__task_cred()": [...] #define __task_cred(task) \ rcu_dereference((task)->real_cred)
Ah! Yes, thank you. cred vs real_cred. That's what I missed!
However, I'm still trying to see where cred_guard_mutex() comes into play for callers of ptrace_may_access(). I see it for the object ("task" arg in ptrace_may_access()), but if this is dealing with the cred on current, it's just the RCU read lock protecting it (which I think is fine here), but seems confusing in the commit log.
Ah, right. I'll drop that from the commit message and place in the rcu lock.
Thanks for the clarification. With that adjusted, please consider it:
Reviewed-by: Kees Cook keescook@chromium.org
(I wonder how hard it might be to build some self-tests for this to catch future glitches...)