linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH v3] ptrace: reintroduce usage of subjective credentials in ptrace_has_cap()
@ 2020-01-17 10:57 Christian Brauner
  2020-01-17 21:15 ` Kees Cook
  0 siblings, 1 reply; 3+ messages in thread
From: Christian Brauner @ 2020-01-17 10:57 UTC (permalink / raw)
  To: linux-kernel, Kees Cook, Jann Horn, Oleg Nesterov
  Cc: stable, Serge Hallyn, Christian Brauner, Eric Paris

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.
This switches it to use security_capable() because we only call
ptrace_has_cap() in ptrace_may_access() and in there we already have a
stable reference to the calling tasks creds under rcu_read_lock() so
there's no need to go through another series of dereferences and rcu
locking done in ns_capable{_noaudit}().

As one example where this might be particularly problematic, Jann pointed
out that in combination with the upcoming IORING_OP_OPENAT feature, this
bug might allow unprivileged users to bypass the capability checks while
asynchronously opening files like /proc/*/mem, because the capability
checks for this would be performed against kernel credentials.

Cc: Kees Cook <keescook@chromium.org>
Cc: Oleg Nesterov <oleg@redhat.com>
Cc: Eric Paris <eparis@redhat.com>
Cc: stable@vger.kernel.org
Reviewed-by: Serge Hallyn <serge@hallyn.com>
Reviewed-by: Jann Horn <jannh@google.com>
Fixes: 69f594a38967 ("ptrace: do not audit capability check when outputing /proc/pid/stat")
Signed-off-by: Christian Brauner <christian.brauner@ubuntu.com>
---
/* v1 */
Link: https://lore.kernel.org/r/20200115171736.16994-1-christian.brauner@ubuntu.com

/* v2 */
Link: https://lore.kernel.org/r/20200116224518.30598-1-christian.brauner@ubuntu.com
- Christian Brauner <christian.brauner@ubuntu.com>:
  - fix incorrect CAP_OPT_NOAUDIT, CAPT_OPT_NONE order

/* v3 */
- Kees Cook <keescook@chromium.org>:
  - remove misleading reference to cread guard mutex from commit message
  - replace if-branches with ternary ?: operator
---
 kernel/ptrace.c | 14 +++++++-------
 1 file changed, 7 insertions(+), 7 deletions(-)

diff --git a/kernel/ptrace.c b/kernel/ptrace.c
index cb9ddcc08119..6eb3ccf180e0 100644
--- a/kernel/ptrace.c
+++ b/kernel/ptrace.c
@@ -264,12 +264,12 @@ static int ptrace_check_attach(struct task_struct *child, bool ignore_state)
 	return ret;
 }
 
-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);
 }
 
 /* Returns 0 on success, -errno on denial. */
@@ -321,7 +321,7 @@ static int __ptrace_may_access(struct task_struct *task, unsigned int mode)
 	    gid_eq(caller_gid, tcred->sgid) &&
 	    gid_eq(caller_gid, tcred->gid))
 		goto ok;
-	if (ptrace_has_cap(tcred->user_ns, mode))
+	if (ptrace_has_cap(cred, tcred->user_ns, mode))
 		goto ok;
 	rcu_read_unlock();
 	return -EPERM;
@@ -340,7 +340,7 @@ static int __ptrace_may_access(struct task_struct *task, unsigned int mode)
 	mm = task->mm;
 	if (mm &&
 	    ((get_dumpable(mm) != SUID_DUMP_USER) &&
-	     !ptrace_has_cap(mm->user_ns, mode)))
+	     !ptrace_has_cap(cred, mm->user_ns, mode)))
 	    return -EPERM;
 
 	return security_ptrace_access_check(task, mode);

base-commit: b3a987b0264d3ddbb24293ebff10eddfc472f653
-- 
2.25.0


^ permalink raw reply related	[flat|nested] 3+ messages in thread

* Re: [PATCH v3] ptrace: reintroduce usage of subjective credentials in ptrace_has_cap()
  2020-01-17 10:57 [PATCH v3] ptrace: reintroduce usage of subjective credentials in ptrace_has_cap() Christian Brauner
@ 2020-01-17 21:15 ` Kees Cook
  2020-01-17 21:35   ` Christian Brauner
  0 siblings, 1 reply; 3+ messages in thread
From: Kees Cook @ 2020-01-17 21:15 UTC (permalink / raw)
  To: Christian Brauner
  Cc: linux-kernel, Jann Horn, Oleg Nesterov, stable, Serge Hallyn, Eric Paris

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

-- 
Kees Cook

^ permalink raw reply	[flat|nested] 3+ messages in thread

* Re: [PATCH v3] ptrace: reintroduce usage of subjective credentials in ptrace_has_cap()
  2020-01-17 21:15 ` Kees Cook
@ 2020-01-17 21:35   ` Christian Brauner
  0 siblings, 0 replies; 3+ messages in thread
From: Christian Brauner @ 2020-01-17 21:35 UTC (permalink / raw)
  To: Kees Cook
  Cc: linux-kernel, Jann Horn, Oleg Nesterov, stable, Serge Hallyn, Eric Paris

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

^ permalink raw reply	[flat|nested] 3+ messages in thread

end of thread, other threads:[~2020-01-17 21:35 UTC | newest]

Thread overview: 3+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-01-17 10:57 [PATCH v3] ptrace: reintroduce usage of subjective credentials in ptrace_has_cap() Christian Brauner
2020-01-17 21:15 ` Kees Cook
2020-01-17 21:35   ` Christian Brauner

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).