2015-11-08 12:19:06

by Jann Horn

Subject: [PATCH] ptrace: use fsuid, fsgid, effective creds for fs access checks

By checking the effective credentials instead of the real UID /
permitted capabilities, ensure that the calling process actually
intended to use its credentials.

To ensure that all ptrace checks use the correct caller
credentials (e.g. in case out-of-tree code or newly added code
omits the PTRACE_MODE_*CREDS flag), use two new flags and
require one of them to be set.

The problem was that when a privileged task had temporarily dropped
its privileges, e.g. by calling setreuid(0, user_uid), with the
intent to perform following syscalls with the credentials of
a user, it still passed ptrace access checks that the user would
not be able to pass.

While an attacker should not be able to convince the privileged
task to perform a ptrace() syscall, this is a problem because the
ptrace access check is reused for things in procfs.

In particular, the following somewhat interesting procfs entries
only rely on ptrace access checks:

/proc/$pid/stat - uses the check for determining whether pointers
should be visible, useful for bypassing ASLR
/proc/$pid/maps - also useful for bypassing ASLR
/proc/$pid/cwd - useful for gaining access to restricted
directories that contain files with lax permissions, e.g. in
this scenario:
lrwxrwxrwx root root /proc/13020/cwd -> /root/foobar
drwx------ root root /root
drwxr-xr-x root root /root/foobar
-rw-r--r-- root root /root/foobar/secret

Therefore, on a system where a root-owned mode 6755 binary
changes its effective credentials as described and then dumps a
user-specified file, this could be used by an attacker to reveal
the memory layout of root's processes or reveal the contents of
files he is not allowed to access (through /proc/$pid/cwd).

Signed-off-by: Jann Horn <[email protected]>
---
fs/proc/array.c | 3 ++-
fs/proc/base.c | 24 ++++++++++++++----------
fs/proc/namespaces.c | 4 ++--
include/linux/ptrace.h | 17 ++++++++++++++++-
kernel/events/core.c | 2 +-
kernel/futex.c | 2 +-
kernel/futex_compat.c | 2 +-
kernel/kcmp.c | 6 ++++--
kernel/ptrace.c | 37 ++++++++++++++++++++++++++++++-------
mm/process_vm_access.c | 2 +-
security/commoncap.c | 7 ++++++-
11 files changed, 78 insertions(+), 28 deletions(-)

diff --git a/fs/proc/array.c b/fs/proc/array.c
index f60f012..00928c6 100644
--- a/fs/proc/array.c
+++ b/fs/proc/array.c
@@ -395,7 +395,8 @@ static int do_task_stat(struct seq_file *m, struct pid_namespace *ns,

state = *get_task_state(task);
vsize = eip = esp = 0;
- permitted = ptrace_may_access(task, PTRACE_MODE_READ | PTRACE_MODE_NOAUDIT);
+ permitted = ptrace_may_access(task,
+ PTRACE_MODE_READ | PTRACE_MODE_NOAUDIT | PTRACE_MODE_FSCREDS);
mm = get_task_mm(task);
if (mm) {
vsize = task_vsize(mm);
diff --git a/fs/proc/base.c b/fs/proc/base.c
index b25eee4..d6b9475 100644
--- a/fs/proc/base.c
+++ b/fs/proc/base.c
@@ -403,7 +403,8 @@ static const struct file_operations proc_pid_cmdline_ops = {
static int proc_pid_auxv(struct seq_file *m, struct pid_namespace *ns,
struct pid *pid, struct task_struct *task)
{
- struct mm_struct *mm = mm_access(task, PTRACE_MODE_READ);
+ struct mm_struct *mm = mm_access(task,
+ PTRACE_MODE_READ | PTRACE_MODE_FSCREDS);
if (mm && !IS_ERR(mm)) {
unsigned int nwords = 0;
do {
@@ -431,7 +432,8 @@ static int proc_pid_wchan(struct seq_file *m, struct pid_namespace *ns,
wchan = get_wchan(task);

if (lookup_symbol_name(wchan, symname) < 0) {
- if (!ptrace_may_access(task, PTRACE_MODE_READ))
+ if (!ptrace_may_access(task,
+ PTRACE_MODE_READ | PTRACE_MODE_FSCREDS))
return 0;
seq_printf(m, "%lu", wchan);
} else {
@@ -447,7 +449,8 @@ static int lock_trace(struct task_struct *task)
int err = mutex_lock_killable(&task->signal->cred_guard_mutex);
if (err)
return err;
- if (!ptrace_may_access(task, PTRACE_MODE_ATTACH)) {
+ if (!ptrace_may_access(task,
+ PTRACE_MODE_ATTACH | PTRACE_MODE_FSCREDS)) {
mutex_unlock(&task->signal->cred_guard_mutex);
return -EPERM;
}
@@ -700,7 +703,8 @@ static int proc_fd_access_allowed(struct inode *inode)
*/
task = get_proc_task(inode);
if (task) {
- allowed = ptrace_may_access(task, PTRACE_MODE_READ);
+ allowed = ptrace_may_access(task,
+ PTRACE_MODE_READ | PTRACE_MODE_FSCREDS);
put_task_struct(task);
}
return allowed;
@@ -735,7 +739,7 @@ static bool has_pid_permissions(struct pid_namespace *pid,
return true;
if (in_group_p(pid->pid_gid))
return true;
- return ptrace_may_access(task, PTRACE_MODE_READ);
+ return ptrace_may_access(task, PTRACE_MODE_READ | PTRACE_MODE_FSCREDS);
}


@@ -812,7 +816,7 @@ struct mm_struct *proc_mem_open(struct inode *inode, unsigned int mode)
struct mm_struct *mm = ERR_PTR(-ESRCH);

if (task) {
- mm = mm_access(task, mode);
+ mm = mm_access(task, mode | PTRACE_MODE_FSCREDS);
put_task_struct(task);

if (!IS_ERR_OR_NULL(mm)) {
@@ -1849,7 +1853,7 @@ static int map_files_d_revalidate(struct dentry *dentry, unsigned int flags)
if (!task)
goto out_notask;

- mm = mm_access(task, PTRACE_MODE_READ);
+ mm = mm_access(task, PTRACE_MODE_READ | PTRACE_MODE_FSCREDS);
if (IS_ERR_OR_NULL(mm))
goto out;

@@ -2000,7 +2004,7 @@ static struct dentry *proc_map_files_lookup(struct inode *dir,
goto out;

result = -EACCES;
- if (!ptrace_may_access(task, PTRACE_MODE_READ))
+ if (!ptrace_may_access(task, PTRACE_MODE_READ | PTRACE_MODE_FSCREDS))
goto out_put_task;

result = -ENOENT;
@@ -2053,7 +2057,7 @@ proc_map_files_readdir(struct file *file, struct dir_context *ctx)
goto out;

ret = -EACCES;
- if (!ptrace_may_access(task, PTRACE_MODE_READ))
+ if (!ptrace_may_access(task, PTRACE_MODE_READ | PTRACE_MODE_FSCREDS))
goto out_put_task;

ret = 0;
@@ -2522,7 +2526,7 @@ static int do_io_accounting(struct task_struct *task, struct seq_file *m, int wh
if (result)
return result;

- if (!ptrace_may_access(task, PTRACE_MODE_READ)) {
+ if (!ptrace_may_access(task, PTRACE_MODE_READ | PTRACE_MODE_FSCREDS)) {
result = -EACCES;
goto out_unlock;
}
diff --git a/fs/proc/namespaces.c b/fs/proc/namespaces.c
index f6e8354..0cbe012 100644
--- a/fs/proc/namespaces.c
+++ b/fs/proc/namespaces.c
@@ -42,7 +42,7 @@ static const char *proc_ns_follow_link(struct dentry *dentry, void **cookie)
if (!task)
return error;

- if (ptrace_may_access(task, PTRACE_MODE_READ)) {
+ if (ptrace_may_access(task, PTRACE_MODE_READ | PTRACE_MODE_FSCREDS)) {
error = ns_get_path(&ns_path, task, ns_ops);
if (!error)
nd_jump_link(&ns_path);
@@ -63,7 +63,7 @@ static int proc_ns_readlink(struct dentry *dentry, char __user *buffer, int bufl
if (!task)
return res;

- if (ptrace_may_access(task, PTRACE_MODE_READ)) {
+ if (ptrace_may_access(task, PTRACE_MODE_READ | PTRACE_MODE_FSCREDS)) {
res = ns_get_name(name, sizeof(name), task, ns_ops);
if (res >= 0)
res = readlink_copy(buffer, buflen, name);
diff --git a/include/linux/ptrace.h b/include/linux/ptrace.h
index 061265f..63f4a3c 100644
--- a/include/linux/ptrace.h
+++ b/include/linux/ptrace.h
@@ -57,7 +57,22 @@ extern void exit_ptrace(struct task_struct *tracer, struct list_head *dead);
#define PTRACE_MODE_READ 0x01
#define PTRACE_MODE_ATTACH 0x02
#define PTRACE_MODE_NOAUDIT 0x04
-/* Returns true on success, false on denial. */
+#define PTRACE_MODE_FSCREDS 0x08
+#define PTRACE_MODE_REALCREDS 0x10
+/**
+ * ptrace_may_access - check whether the caller is permitted to access
+ * a target task.
+ * @task: target task
+ * @mode: selects type of access and caller credentials
+ *
+ * Returns true on success, false on denial.
+ *
+ * One of the flags PTRACE_MODE_FSCREDS and PTRACE_MODE_REALCREDS must
+ * be set in @mode to specify whether the access was requested through
+ * a filesystem syscall (should use effective capabilities and fsuid
+ * of the caller) or through an explicit syscall such as
+ * process_vm_writev or ptrace (and should use the real credentials).
+ */
extern bool ptrace_may_access(struct task_struct *task, unsigned int mode);

static inline int ptrace_reparented(struct task_struct *child)
diff --git a/kernel/events/core.c b/kernel/events/core.c
index b11756f..0decdf3 100644
--- a/kernel/events/core.c
+++ b/kernel/events/core.c
@@ -3384,7 +3384,7 @@ find_lively_task_by_vpid(pid_t vpid)

/* Reuse ptrace permission checks for now. */
err = -EACCES;
- if (!ptrace_may_access(task, PTRACE_MODE_READ))
+ if (!ptrace_may_access(task, PTRACE_MODE_READ | PTRACE_MODE_REALCREDS))
goto errout;

return task;
diff --git a/kernel/futex.c b/kernel/futex.c
index 6e443ef..11e467e 100644
--- a/kernel/futex.c
+++ b/kernel/futex.c
@@ -2872,7 +2872,7 @@ SYSCALL_DEFINE3(get_robust_list, int, pid,
}

ret = -EPERM;
- if (!ptrace_may_access(p, PTRACE_MODE_READ))
+ if (!ptrace_may_access(p, PTRACE_MODE_READ | PTRACE_MODE_REALCREDS))
goto err_unlock;

head = p->robust_list;
diff --git a/kernel/futex_compat.c b/kernel/futex_compat.c
index 55c8c93..5596298 100644
--- a/kernel/futex_compat.c
+++ b/kernel/futex_compat.c
@@ -155,7 +155,7 @@ COMPAT_SYSCALL_DEFINE3(get_robust_list, int, pid,
}

ret = -EPERM;
- if (!ptrace_may_access(p, PTRACE_MODE_READ))
+ if (!ptrace_may_access(p, PTRACE_MODE_READ | PTRACE_MODE_REALCREDS))
goto err_unlock;

head = p->compat_robust_list;
diff --git a/kernel/kcmp.c b/kernel/kcmp.c
index 0aa69ea..4a568e4 100644
--- a/kernel/kcmp.c
+++ b/kernel/kcmp.c
@@ -122,8 +122,10 @@ SYSCALL_DEFINE5(kcmp, pid_t, pid1, pid_t, pid2, int, type,
&task2->signal->cred_guard_mutex);
if (ret)
goto err;
- if (!ptrace_may_access(task1, PTRACE_MODE_READ) ||
- !ptrace_may_access(task2, PTRACE_MODE_READ)) {
+ if (!ptrace_may_access(task1,
+ PTRACE_MODE_READ | PTRACE_MODE_REALCREDS) ||
+ !ptrace_may_access(task2,
+ PTRACE_MODE_READ | PTRACE_MODE_REALCREDS)) {
ret = -EPERM;
goto err_unlock;
}
diff --git a/kernel/ptrace.c b/kernel/ptrace.c
index 787320d..28f007b 100644
--- a/kernel/ptrace.c
+++ b/kernel/ptrace.c
@@ -219,6 +219,13 @@ static int ptrace_has_cap(struct user_namespace *ns, unsigned int mode)
static int __ptrace_may_access(struct task_struct *task, unsigned int mode)
{
const struct cred *cred = current_cred(), *tcred;
+ kuid_t caller_uid;
+ kgid_t caller_gid;
+
+ if (!(mode & PTRACE_MODE_FSCREDS) != !(mode & PTRACE_MODE_REALCREDS)) {
+ WARN(1, "denying ptrace access check without PTRACE_MODE_*CREDS\n");
+ return -EPERM;
+ }

/* May we inspect the given task?
* This check is used both for attaching with ptrace
@@ -233,13 +240,28 @@ static int __ptrace_may_access(struct task_struct *task, unsigned int mode)
if (same_thread_group(task, current))
return 0;
rcu_read_lock();
+ if (mode & PTRACE_MODE_FSCREDS) {
+ caller_uid = cred->fsuid;
+ caller_gid = cred->fsgid;
+ } else {
+ /*
+ * Using the euid would make more sense here, but something
+ * in userland might rely on the old behavior, and this
+ * shouldn't be a security problem since
+ * PTRACE_MODE_REALCREDS implies that the caller explicitly
+ * used a syscall that requests access to another process
+ * (and not a filesystem syscall to procfs).
+ */
+ caller_uid = cred->uid;
+ caller_gid = cred->gid;
+ }
tcred = __task_cred(task);
- if (uid_eq(cred->uid, tcred->euid) &&
- uid_eq(cred->uid, tcred->suid) &&
- uid_eq(cred->uid, tcred->uid) &&
- gid_eq(cred->gid, tcred->egid) &&
- gid_eq(cred->gid, tcred->sgid) &&
- gid_eq(cred->gid, tcred->gid))
+ if (uid_eq(caller_uid, tcred->euid) &&
+ uid_eq(caller_uid, tcred->suid) &&
+ uid_eq(caller_uid, tcred->uid) &&
+ gid_eq(caller_gid, tcred->egid) &&
+ gid_eq(caller_gid, tcred->sgid) &&
+ gid_eq(caller_gid, tcred->gid))
goto ok;
if (ptrace_has_cap(tcred->user_ns, mode))
goto ok;
@@ -306,7 +328,8 @@ static int ptrace_attach(struct task_struct *task, long request,
goto out;

task_lock(task);
- retval = __ptrace_may_access(task, PTRACE_MODE_ATTACH);
+ retval = __ptrace_may_access(task,
+ PTRACE_MODE_ATTACH | PTRACE_MODE_REALCREDS);
task_unlock(task);
if (retval)
goto unlock_creds;
diff --git a/mm/process_vm_access.c b/mm/process_vm_access.c
index e88d071..a2be821 100644
--- a/mm/process_vm_access.c
+++ b/mm/process_vm_access.c
@@ -194,7 +194,7 @@ static ssize_t process_vm_rw_core(pid_t pid, struct iov_iter *iter,
goto free_proc_pages;
}

- mm = mm_access(task, PTRACE_MODE_ATTACH);
+ mm = mm_access(task, PTRACE_MODE_ATTACH | PTRACE_MODE_REALCREDS);
if (!mm || IS_ERR(mm)) {
rc = IS_ERR(mm) ? PTR_ERR(mm) : -ESRCH;
/*
diff --git a/security/commoncap.c b/security/commoncap.c
index 1832cf7..48071ed 100644
--- a/security/commoncap.c
+++ b/security/commoncap.c
@@ -137,12 +137,17 @@ int cap_ptrace_access_check(struct task_struct *child, unsigned int mode)
{
int ret = 0;
const struct cred *cred, *child_cred;
+ const kernel_cap_t *caller_caps;

rcu_read_lock();
cred = current_cred();
child_cred = __task_cred(child);
+ if (mode & PTRACE_MODE_FSCREDS)
+ caller_caps = &cred->cap_effective;
+ else
+ caller_caps = &cred->cap_permitted;
if (cred->user_ns == child_cred->user_ns &&
- cap_issubset(child_cred->cap_permitted, cred->cap_permitted))
+ cap_issubset(child_cred->cap_permitted, *caller_caps))
goto out;
if (ns_capable(child_cred->user_ns, CAP_SYS_PTRACE))
goto out;
--
2.1.4

2015-11-09 20:55:59

by Andrew Morton

Subject: Re: [PATCH] ptrace: use fsuid, fsgid, effective creds for fs access checks

On Sun, 8 Nov 2015 13:08:36 +0100 Jann Horn <[email protected]> wrote:

> By checking the effective credentials instead of the real UID /
> permitted capabilities, ensure that the calling process actually
> intended to use its credentials.
>
> To ensure that all ptrace checks use the correct caller
> credentials (e.g. in case out-of-tree code or newly added code
> omits the PTRACE_MODE_*CREDS flag), use two new flags and
> require one of them to be set.
>
> The problem was that when a privileged task had temporarily dropped
> its privileges, e.g. by calling setreuid(0, user_uid), with the
> intent to perform following syscalls with the credentials of
> a user, it still passed ptrace access checks that the user would
> not be able to pass.
>
> While an attacker should not be able to convince the privileged
> task to perform a ptrace() syscall, this is a problem because the
> ptrace access check is reused for things in procfs.
>
> In particular, the following somewhat interesting procfs entries
> only rely on ptrace access checks:
>
> /proc/$pid/stat - uses the check for determining whether pointers
> should be visible, useful for bypassing ASLR
> /proc/$pid/maps - also useful for bypassing ASLR
> /proc/$pid/cwd - useful for gaining access to restricted
> directories that contain files with lax permissions, e.g. in
> this scenario:
> lrwxrwxrwx root root /proc/13020/cwd -> /root/foobar
> drwx------ root root /root
> drwxr-xr-x root root /root/foobar
> -rw-r--r-- root root /root/foobar/secret
>
> Therefore, on a system where a root-owned mode 6755 binary
> changes its effective credentials as described and then dumps a
> user-specified file, this could be used by an attacker to reveal
> the memory layout of root's processes or reveal the contents of
> files he is not allowed to access (through /proc/$pid/cwd).

I'll await reviewer input on this one. Meanwhile, a bunch of
minor(ish) things...

> --- a/fs/proc/array.c
> +++ b/fs/proc/array.c
> @@ -395,7 +395,8 @@ static int do_task_stat(struct seq_file *m, struct pid_namespace *ns,
>
> state = *get_task_state(task);
> vsize = eip = esp = 0;
> - permitted = ptrace_may_access(task, PTRACE_MODE_READ | PTRACE_MODE_NOAUDIT);
> + permitted = ptrace_may_access(task,
> + PTRACE_MODE_READ | PTRACE_MODE_NOAUDIT | PTRACE_MODE_FSCREDS);

There's lots of ugliness in the patch to do with fitting code into 80 cols.
Can we do

#define PTRACE_foo (PTRACE_MODE_READ|PTRACE_MODE_FSCREDS)

to avoid all that?

> --- a/include/linux/ptrace.h
> +++ b/include/linux/ptrace.h
> @@ -57,7 +57,22 @@ extern void exit_ptrace(struct task_struct *tracer, struct list_head *dead);
> #define PTRACE_MODE_READ 0x01
> #define PTRACE_MODE_ATTACH 0x02
> #define PTRACE_MODE_NOAUDIT 0x04
> -/* Returns true on success, false on denial. */
> +#define PTRACE_MODE_FSCREDS 0x08
> +#define PTRACE_MODE_REALCREDS 0x10
> +/**
> + * ptrace_may_access - check whether the caller is permitted to access
> + * a target task.
> + * @task: target task
> + * @mode: selects type of access and caller credentials
> + *
> + * Returns true on success, false on denial.
> + *
> + * One of the flags PTRACE_MODE_FSCREDS and PTRACE_MODE_REALCREDS must
> + * be set in @mode to specify whether the access was requested through
> + * a filesystem syscall (should use effective capabilities and fsuid
> + * of the caller) or through an explicit syscall such as
> + * process_vm_writev or ptrace (and should use the real credentials).
> + */
> extern bool ptrace_may_access(struct task_struct *task, unsigned int mode);

It is unconventional to put the kernedoc in the header - people have
been trained to look for it in the .c file.

> +++ b/kernel/ptrace.c
> @@ -219,6 +219,13 @@ static int ptrace_has_cap(struct user_namespace *ns, unsigned int mode)
> static int __ptrace_may_access(struct task_struct *task, unsigned int mode)
> {
> const struct cred *cred = current_cred(), *tcred;
> + kuid_t caller_uid;
> + kgid_t caller_gid;
> +
> + if (!(mode & PTRACE_MODE_FSCREDS) != !(mode & PTRACE_MODE_REALCREDS)) {

So setting either one of these and not the other is an error. How
come?

> + WARN(1, "denying ptrace access check without PTRACE_MODE_*CREDS\n");

This warning cannot be triggered by malicious userspace, I trust?

2015-11-09 21:06:38

by Willy Tarreau

Subject: Re: [PATCH] ptrace: use fsuid, fsgid, effective creds for fs access checks

On Mon, Nov 09, 2015 at 12:55:54PM -0800, Andrew Morton wrote:
> > --- a/fs/proc/array.c
> > +++ b/fs/proc/array.c
> > @@ -395,7 +395,8 @@ static int do_task_stat(struct seq_file *m, struct pid_namespace *ns,
> >
> > state = *get_task_state(task);
> > vsize = eip = esp = 0;
> > - permitted = ptrace_may_access(task, PTRACE_MODE_READ | PTRACE_MODE_NOAUDIT);
> > + permitted = ptrace_may_access(task,
> > + PTRACE_MODE_READ | PTRACE_MODE_NOAUDIT | PTRACE_MODE_FSCREDS);
>
> There's lots of ugliness in the patch to do with fitting code into 80 cols.
> Can we do
>
> #define PTRACE_foo (PTRACE_MODE_READ|PTRACE_MODE_FSCREDS)
>
> to avoid all that?

Or even simply bypass the 80-cols rule. Making code ugly or less easy
to read for sake of an arbitrary rule is often not fun, and that's even
more so when it comes to security fixes that people are expected to
easily understand next time they put their fingers there.

Willy

2015-11-09 21:12:11

by Jann Horn

Subject: Re: [PATCH] ptrace: use fsuid, fsgid, effective creds for fs access checks

On Mon, Nov 09, 2015 at 12:55:54PM -0800, Andrew Morton wrote:
> On Sun, 8 Nov 2015 13:08:36 +0100 Jann Horn <[email protected]> wrote:
>
> > By checking the effective credentials instead of the real UID /
> > permitted capabilities, ensure that the calling process actually
> > intended to use its credentials.
> >
> > To ensure that all ptrace checks use the correct caller
> > credentials (e.g. in case out-of-tree code or newly added code
> > omits the PTRACE_MODE_*CREDS flag), use two new flags and
> > require one of them to be set.
> >
> > The problem was that when a privileged task had temporarily dropped
> > its privileges, e.g. by calling setreuid(0, user_uid), with the
> > intent to perform following syscalls with the credentials of
> > a user, it still passed ptrace access checks that the user would
> > not be able to pass.
> >
> > While an attacker should not be able to convince the privileged
> > task to perform a ptrace() syscall, this is a problem because the
> > ptrace access check is reused for things in procfs.
> >
> > In particular, the following somewhat interesting procfs entries
> > only rely on ptrace access checks:
> >
> > /proc/$pid/stat - uses the check for determining whether pointers
> > should be visible, useful for bypassing ASLR
> > /proc/$pid/maps - also useful for bypassing ASLR
> > /proc/$pid/cwd - useful for gaining access to restricted
> > directories that contain files with lax permissions, e.g. in
> > this scenario:
> > lrwxrwxrwx root root /proc/13020/cwd -> /root/foobar
> > drwx------ root root /root
> > drwxr-xr-x root root /root/foobar
> > -rw-r--r-- root root /root/foobar/secret
> >
> > Therefore, on a system where a root-owned mode 6755 binary
> > changes its effective credentials as described and then dumps a
> > user-specified file, this could be used by an attacker to reveal
> > the memory layout of root's processes or reveal the contents of
> > files he is not allowed to access (through /proc/$pid/cwd).
>
> I'll await reviewer input on this one. Meanwhile, a bunch of
> minor(ish) things...
>
> > --- a/fs/proc/array.c
> > +++ b/fs/proc/array.c
> > @@ -395,7 +395,8 @@ static int do_task_stat(struct seq_file *m, struct pid_namespace *ns,
> >
> > state = *get_task_state(task);
> > vsize = eip = esp = 0;
> > - permitted = ptrace_may_access(task, PTRACE_MODE_READ | PTRACE_MODE_NOAUDIT);
> > + permitted = ptrace_may_access(task,
> > + PTRACE_MODE_READ | PTRACE_MODE_NOAUDIT | PTRACE_MODE_FSCREDS);
>
> There's lots of ugliness in the patch to do with fitting code into 80 cols.

I agree.


> Can we do
>
> #define PTRACE_foo (PTRACE_MODE_READ|PTRACE_MODE_FSCREDS)
>
> to avoid all that?

Hm. All combinations of the PTRACE_MODE_*CREDS flags with
PTRACE_MODE_{READ,ATTACH} plus optionally PTRACE_MODE_NOAUDIT
make sense, I think. So your suggestion would be to create
four new #defines
PTRACE_MODE_{READ,ATTACH}_{FSCREDS,REALCREDS} and then let
callers OR in the PTRACE_MODE_NOAUDIT flag if needed?


> > --- a/include/linux/ptrace.h
> > +++ b/include/linux/ptrace.h
> > @@ -57,7 +57,22 @@ extern void exit_ptrace(struct task_struct *tracer, struct list_head *dead);
> > #define PTRACE_MODE_READ 0x01
> > #define PTRACE_MODE_ATTACH 0x02
> > #define PTRACE_MODE_NOAUDIT 0x04
> > -/* Returns true on success, false on denial. */
> > +#define PTRACE_MODE_FSCREDS 0x08
> > +#define PTRACE_MODE_REALCREDS 0x10
> > +/**
> > + * ptrace_may_access - check whether the caller is permitted to access
> > + * a target task.
> > + * @task: target task
> > + * @mode: selects type of access and caller credentials
> > + *
> > + * Returns true on success, false on denial.
> > + *
> > + * One of the flags PTRACE_MODE_FSCREDS and PTRACE_MODE_REALCREDS must
> > + * be set in @mode to specify whether the access was requested through
> > + * a filesystem syscall (should use effective capabilities and fsuid
> > + * of the caller) or through an explicit syscall such as
> > + * process_vm_writev or ptrace (and should use the real credentials).
> > + */
> > extern bool ptrace_may_access(struct task_struct *task, unsigned int mode);
>
> It is unconventional to put the kernedoc in the header - people have
> been trained to look for it in the .c file.

OK, will fix that. I thought it would be appropriate to put it in the
header since that one-line comment was already there.


> > +++ b/kernel/ptrace.c
> > @@ -219,6 +219,13 @@ static int ptrace_has_cap(struct user_namespace *ns, unsigned int mode)
> > static int __ptrace_may_access(struct task_struct *task, unsigned int mode)
> > {
> > const struct cred *cred = current_cred(), *tcred;
> > + kuid_t caller_uid;
> > + kgid_t caller_gid;
> > +
> > + if (!(mode & PTRACE_MODE_FSCREDS) != !(mode & PTRACE_MODE_REALCREDS)) {
>
> So setting either one of these and not the other is an error. How
> come?

Oh. Sorry about that. I only added PTRACE_MODE_REALCREDS in this iteration
of the patch and forgot to re-test afterwards. It is supposed to be the
other way around, so that you need to set exactly one. s/!=/==/


> > + WARN(1, "denying ptrace access check without PTRACE_MODE_*CREDS\n");
>
> This warning cannot be triggered by malicious userspace, I trust?

Yeah, the ptrace access check flags should come from kernelspace only.
My patch modifies all callers of mm_access / ptrace_may_access so that
exactly one of the new flags is added, and the mode argument is always
a constant.

Attachments:

2015-11-09 21:19:06

by Andrew Morton

Subject: Re: [PATCH] ptrace: use fsuid, fsgid, effective creds for fs access checks

On Mon, 9 Nov 2015 22:12:09 +0100 Jann Horn <[email protected]> wrote:
>
> > Can we do
> >
> > #define PTRACE_foo (PTRACE_MODE_READ|PTRACE_MODE_FSCREDS)
> >
> > to avoid all that?
>
> Hm. All combinations of the PTRACE_MODE_*CREDS flags with
> PTRACE_MODE_{READ,ATTACH} plus optionally PTRACE_MODE_NOAUDIT
> make sense, I think. So your suggestion would be to create
> four new #defines
> PTRACE_MODE_{READ,ATTACH}_{FSCREDS,REALCREDS} and then let
> callers OR in the PTRACE_MODE_NOAUDIT flag if needed?

If these flag combinations have an identifiable concept behind them then
sure, it makes sense to capture that via a well-chosen identifier.

2015-12-06 02:04:38

by Jann Horn

Subject: [PATCH] ptrace: use fsuid, fsgid, effective creds for fs access checks

By checking the effective credentials instead of the real UID /
permitted capabilities, ensure that the calling process actually
intended to use its credentials.

To ensure that all ptrace checks use the correct caller
credentials (e.g. in case out-of-tree code or newly added code
omits the PTRACE_MODE_*CREDS flag), use two new flags and
require one of them to be set.

The problem was that when a privileged task had temporarily dropped
its privileges, e.g. by calling setreuid(0, user_uid), with the
intent to perform following syscalls with the credentials of
a user, it still passed ptrace access checks that the user would
not be able to pass.

While an attacker should not be able to convince the privileged
task to perform a ptrace() syscall, this is a problem because the
ptrace access check is reused for things in procfs.

In particular, the following somewhat interesting procfs entries
only rely on ptrace access checks:

/proc/$pid/stat - uses the check for determining whether pointers
should be visible, useful for bypassing ASLR
/proc/$pid/maps - also useful for bypassing ASLR
/proc/$pid/cwd - useful for gaining access to restricted
directories that contain files with lax permissions, e.g. in
this scenario:
lrwxrwxrwx root root /proc/13020/cwd -> /root/foobar
drwx------ root root /root
drwxr-xr-x root root /root/foobar
-rw-r--r-- root root /root/foobar/secret

Therefore, on a system where a root-owned mode 6755 binary
changes its effective credentials as described and then dumps a
user-specified file, this could be used by an attacker to reveal
the memory layout of root's processes or reveal the contents of
files he is not allowed to access (through /proc/$pid/cwd).

Signed-off-by: Jann Horn <[email protected]>
---
fs/proc/array.c | 2 +-
fs/proc/base.c | 21 +++++++++++----------
fs/proc/namespaces.c | 4 ++--
include/linux/ptrace.h | 24 +++++++++++++++++++++++-
kernel/events/core.c | 2 +-
kernel/futex.c | 2 +-
kernel/futex_compat.c | 2 +-
kernel/kcmp.c | 4 ++--
kernel/ptrace.c | 36 +++++++++++++++++++++++++++++-------
mm/process_vm_access.c | 2 +-
security/commoncap.c | 7 ++++++-
11 files changed, 78 insertions(+), 28 deletions(-)

diff --git a/fs/proc/array.c b/fs/proc/array.c
index d73291f..b6c00ce 100644
--- a/fs/proc/array.c
+++ b/fs/proc/array.c
@@ -395,7 +395,7 @@ static int do_task_stat(struct seq_file *m, struct pid_namespace *ns,

state = *get_task_state(task);
vsize = eip = esp = 0;
- permitted = ptrace_may_access(task, PTRACE_MODE_READ | PTRACE_MODE_NOAUDIT);
+ permitted = ptrace_may_access(task, PTRACE_MODE_READ_FSCREDS | PTRACE_MODE_NOAUDIT);
mm = get_task_mm(task);
if (mm) {
vsize = task_vsize(mm);
diff --git a/fs/proc/base.c b/fs/proc/base.c
index bd3e9e6..c0a2f29 100644
--- a/fs/proc/base.c
+++ b/fs/proc/base.c
@@ -403,7 +403,7 @@ static const struct file_operations proc_pid_cmdline_ops = {
static int proc_pid_auxv(struct seq_file *m, struct pid_namespace *ns,
struct pid *pid, struct task_struct *task)
{
- struct mm_struct *mm = mm_access(task, PTRACE_MODE_READ);
+ struct mm_struct *mm = mm_access(task, PTRACE_MODE_READ_FSCREDS);
if (mm && !IS_ERR(mm)) {
unsigned int nwords = 0;
do {
@@ -430,7 +430,8 @@ static int proc_pid_wchan(struct seq_file *m, struct pid_namespace *ns,

wchan = get_wchan(task);

- if (wchan && ptrace_may_access(task, PTRACE_MODE_READ) && !lookup_symbol_name(wchan, symname))
+ if (wchan && ptrace_may_access(task, PTRACE_MODE_READ_FSCREDS)
+ && !lookup_symbol_name(wchan, symname))
seq_printf(m, "%s", symname);
else
seq_putc(m, '0');
@@ -444,7 +445,7 @@ static int lock_trace(struct task_struct *task)
int err = mutex_lock_killable(&task->signal->cred_guard_mutex);
if (err)
return err;
- if (!ptrace_may_access(task, PTRACE_MODE_ATTACH)) {
+ if (!ptrace_may_access(task, PTRACE_MODE_ATTACH_FSCREDS)) {
mutex_unlock(&task->signal->cred_guard_mutex);
return -EPERM;
}
@@ -697,7 +698,7 @@ static int proc_fd_access_allowed(struct inode *inode)
*/
task = get_proc_task(inode);
if (task) {
- allowed = ptrace_may_access(task, PTRACE_MODE_READ);
+ allowed = ptrace_may_access(task, PTRACE_MODE_READ_FSCREDS);
put_task_struct(task);
}
return allowed;
@@ -732,7 +733,7 @@ static bool has_pid_permissions(struct pid_namespace *pid,
return true;
if (in_group_p(pid->pid_gid))
return true;
- return ptrace_may_access(task, PTRACE_MODE_READ);
+ return ptrace_may_access(task, PTRACE_MODE_READ_FSCREDS);
}


@@ -809,7 +810,7 @@ struct mm_struct *proc_mem_open(struct inode *inode, unsigned int mode)
struct mm_struct *mm = ERR_PTR(-ESRCH);

if (task) {
- mm = mm_access(task, mode);
+ mm = mm_access(task, mode | PTRACE_MODE_FSCREDS);
put_task_struct(task);

if (!IS_ERR_OR_NULL(mm)) {
@@ -1856,7 +1857,7 @@ static int map_files_d_revalidate(struct dentry *dentry, unsigned int flags)
if (!task)
goto out_notask;

- mm = mm_access(task, PTRACE_MODE_READ);
+ mm = mm_access(task, PTRACE_MODE_READ_FSCREDS);
if (IS_ERR_OR_NULL(mm))
goto out;

@@ -2007,7 +2008,7 @@ static struct dentry *proc_map_files_lookup(struct inode *dir,
goto out;

result = -EACCES;
- if (!ptrace_may_access(task, PTRACE_MODE_READ))
+ if (!ptrace_may_access(task, PTRACE_MODE_READ_FSCREDS))
goto out_put_task;

result = -ENOENT;
@@ -2060,7 +2061,7 @@ proc_map_files_readdir(struct file *file, struct dir_context *ctx)
goto out;

ret = -EACCES;
- if (!ptrace_may_access(task, PTRACE_MODE_READ))
+ if (!ptrace_may_access(task, PTRACE_MODE_READ_FSCREDS))
goto out_put_task;

ret = 0;
@@ -2529,7 +2530,7 @@ static int do_io_accounting(struct task_struct *task, struct seq_file *m, int wh
if (result)
return result;

- if (!ptrace_may_access(task, PTRACE_MODE_READ)) {
+ if (!ptrace_may_access(task, PTRACE_MODE_READ_FSCREDS)) {
result = -EACCES;
goto out_unlock;
}
diff --git a/fs/proc/namespaces.c b/fs/proc/namespaces.c
index f6e8354..0cbe012 100644
--- a/fs/proc/namespaces.c
+++ b/fs/proc/namespaces.c
@@ -42,7 +42,7 @@ static const char *proc_ns_follow_link(struct dentry *dentry, void **cookie)
if (!task)
return error;

- if (ptrace_may_access(task, PTRACE_MODE_READ)) {
+ if (ptrace_may_access(task, PTRACE_MODE_READ | PTRACE_MODE_FSCREDS)) {
error = ns_get_path(&ns_path, task, ns_ops);
if (!error)
nd_jump_link(&ns_path);
@@ -63,7 +63,7 @@ static int proc_ns_readlink(struct dentry *dentry, char __user *buffer, int bufl
if (!task)
return res;

- if (ptrace_may_access(task, PTRACE_MODE_READ)) {
+ if (ptrace_may_access(task, PTRACE_MODE_READ | PTRACE_MODE_FSCREDS)) {
res = ns_get_name(name, sizeof(name), task, ns_ops);
if (res >= 0)
res = readlink_copy(buffer, buflen, name);
diff --git a/include/linux/ptrace.h b/include/linux/ptrace.h
index 061265f..504c98a 100644
--- a/include/linux/ptrace.h
+++ b/include/linux/ptrace.h
@@ -57,7 +57,29 @@ extern void exit_ptrace(struct task_struct *tracer, struct list_head *dead);
#define PTRACE_MODE_READ 0x01
#define PTRACE_MODE_ATTACH 0x02
#define PTRACE_MODE_NOAUDIT 0x04
-/* Returns true on success, false on denial. */
+#define PTRACE_MODE_FSCREDS 0x08
+#define PTRACE_MODE_REALCREDS 0x10
+
+/* shorthands for READ/ATTACH and FSCREDS/REALCREDS combinations */
+#define PTRACE_MODE_READ_FSCREDS (PTRACE_MODE_READ | PTRACE_MODE_FSCREDS)
+#define PTRACE_MODE_READ_REALCREDS (PTRACE_MODE_READ | PTRACE_MODE_REALCREDS)
+#define PTRACE_MODE_ATTACH_FSCREDS (PTRACE_MODE_ATTACH | PTRACE_MODE_FSCREDS)
+#define PTRACE_MODE_ATTACH_REALCREDS (PTRACE_MODE_ATTACH | PTRACE_MODE_REALCREDS)
+
+/**
+ * ptrace_may_access - check whether the caller is permitted to access
+ * a target task.
+ * @task: target task
+ * @mode: selects type of access and caller credentials
+ *
+ * Returns true on success, false on denial.
+ *
+ * One of the flags PTRACE_MODE_FSCREDS and PTRACE_MODE_REALCREDS must
+ * be set in @mode to specify whether the access was requested through
+ * a filesystem syscall (should use effective capabilities and fsuid
+ * of the caller) or through an explicit syscall such as
+ * process_vm_writev or ptrace (and should use the real credentials).
+ */
extern bool ptrace_may_access(struct task_struct *task, unsigned int mode);

static inline int ptrace_reparented(struct task_struct *child)
diff --git a/kernel/events/core.c b/kernel/events/core.c
index 36babfd..565e41a 100644
--- a/kernel/events/core.c
+++ b/kernel/events/core.c
@@ -3430,7 +3430,7 @@ find_lively_task_by_vpid(pid_t vpid)

/* Reuse ptrace permission checks for now. */
err = -EACCES;
- if (!ptrace_may_access(task, PTRACE_MODE_READ))
+ if (!ptrace_may_access(task, PTRACE_MODE_READ_REALCREDS))
goto errout;

return task;
diff --git a/kernel/futex.c b/kernel/futex.c
index 684d754..495a1d0 100644
--- a/kernel/futex.c
+++ b/kernel/futex.c
@@ -2881,7 +2881,7 @@ SYSCALL_DEFINE3(get_robust_list, int, pid,
}

ret = -EPERM;
- if (!ptrace_may_access(p, PTRACE_MODE_READ))
+ if (!ptrace_may_access(p, PTRACE_MODE_READ_REALCREDS))
goto err_unlock;

head = p->robust_list;
diff --git a/kernel/futex_compat.c b/kernel/futex_compat.c
index 55c8c93..4ae3232 100644
--- a/kernel/futex_compat.c
+++ b/kernel/futex_compat.c
@@ -155,7 +155,7 @@ COMPAT_SYSCALL_DEFINE3(get_robust_list, int, pid,
}

ret = -EPERM;
- if (!ptrace_may_access(p, PTRACE_MODE_READ))
+ if (!ptrace_may_access(p, PTRACE_MODE_READ_REALCREDS))
goto err_unlock;

head = p->compat_robust_list;
diff --git a/kernel/kcmp.c b/kernel/kcmp.c
index 0aa69ea..3a47fa9 100644
--- a/kernel/kcmp.c
+++ b/kernel/kcmp.c
@@ -122,8 +122,8 @@ SYSCALL_DEFINE5(kcmp, pid_t, pid1, pid_t, pid2, int, type,
&task2->signal->cred_guard_mutex);
if (ret)
goto err;
- if (!ptrace_may_access(task1, PTRACE_MODE_READ) ||
- !ptrace_may_access(task2, PTRACE_MODE_READ)) {
+ if (!ptrace_may_access(task1, PTRACE_MODE_READ_REALCREDS) ||
+ !ptrace_may_access(task2, PTRACE_MODE_READ_REALCREDS)) {
ret = -EPERM;
goto err_unlock;
}
diff --git a/kernel/ptrace.c b/kernel/ptrace.c
index b760bae..21aef51 100644
--- a/kernel/ptrace.c
+++ b/kernel/ptrace.c
@@ -219,6 +219,13 @@ static int ptrace_has_cap(struct user_namespace *ns, unsigned int mode)
static int __ptrace_may_access(struct task_struct *task, unsigned int mode)
{
const struct cred *cred = current_cred(), *tcred;
+ kuid_t caller_uid;
+ kgid_t caller_gid;
+
+ if (!(mode & PTRACE_MODE_FSCREDS) == !(mode & PTRACE_MODE_REALCREDS)) {
+ WARN(1, "denying ptrace access check without PTRACE_MODE_*CREDS\n");
+ return -EPERM;
+ }

/* May we inspect the given task?
* This check is used both for attaching with ptrace
@@ -233,13 +240,28 @@ static int __ptrace_may_access(struct task_struct *task, unsigned int mode)
if (same_thread_group(task, current))
return 0;
rcu_read_lock();
+ if (mode & PTRACE_MODE_FSCREDS) {
+ caller_uid = cred->fsuid;
+ caller_gid = cred->fsgid;
+ } else {
+ /*
+ * Using the euid would make more sense here, but something
+ * in userland might rely on the old behavior, and this
+ * shouldn't be a security problem since
+ * PTRACE_MODE_REALCREDS implies that the caller explicitly
+ * used a syscall that requests access to another process
+ * (and not a filesystem syscall to procfs).
+ */
+ caller_uid = cred->uid;
+ caller_gid = cred->gid;
+ }
tcred = __task_cred(task);
- if (uid_eq(cred->uid, tcred->euid) &&
- uid_eq(cred->uid, tcred->suid) &&
- uid_eq(cred->uid, tcred->uid) &&
- gid_eq(cred->gid, tcred->egid) &&
- gid_eq(cred->gid, tcred->sgid) &&
- gid_eq(cred->gid, tcred->gid))
+ if (uid_eq(caller_uid, tcred->euid) &&
+ uid_eq(caller_uid, tcred->suid) &&
+ uid_eq(caller_uid, tcred->uid) &&
+ gid_eq(caller_gid, tcred->egid) &&
+ gid_eq(caller_gid, tcred->sgid) &&
+ gid_eq(caller_gid, tcred->gid))
goto ok;
if (ptrace_has_cap(tcred->user_ns, mode))
goto ok;
@@ -306,7 +328,7 @@ static int ptrace_attach(struct task_struct *task, long request,
goto out;

task_lock(task);
- retval = __ptrace_may_access(task, PTRACE_MODE_ATTACH);
+ retval = __ptrace_may_access(task, PTRACE_MODE_ATTACH_REALCREDS);
task_unlock(task);
if (retval)
goto unlock_creds;
diff --git a/mm/process_vm_access.c b/mm/process_vm_access.c
index e88d071..5d453e5 100644
--- a/mm/process_vm_access.c
+++ b/mm/process_vm_access.c
@@ -194,7 +194,7 @@ static ssize_t process_vm_rw_core(pid_t pid, struct iov_iter *iter,
goto free_proc_pages;
}

- mm = mm_access(task, PTRACE_MODE_ATTACH);
+ mm = mm_access(task, PTRACE_MODE_ATTACH_REALCREDS);
if (!mm || IS_ERR(mm)) {
rc = IS_ERR(mm) ? PTR_ERR(mm) : -ESRCH;
/*
diff --git a/security/commoncap.c b/security/commoncap.c
index 1832cf7..48071ed 100644
--- a/security/commoncap.c
+++ b/security/commoncap.c
@@ -137,12 +137,17 @@ int cap_ptrace_access_check(struct task_struct *child, unsigned int mode)
{
int ret = 0;
const struct cred *cred, *child_cred;
+ const kernel_cap_t *caller_caps;

rcu_read_lock();
cred = current_cred();
child_cred = __task_cred(child);
+ if (mode & PTRACE_MODE_FSCREDS)
+ caller_caps = &cred->cap_effective;
+ else
+ caller_caps = &cred->cap_permitted;
if (cred->user_ns == child_cred->user_ns &&
- cap_issubset(child_cred->cap_permitted, cred->cap_permitted))
+ cap_issubset(child_cred->cap_permitted, *caller_caps))
goto out;
if (ns_capable(child_cred->user_ns, CAP_SYS_PTRACE))
goto out;
--
2.1.4

2015-12-07 20:32:11

by Kees Cook

Subject: Re: [PATCH] ptrace: use fsuid, fsgid, effective creds for fs access checks

On Sat, Dec 5, 2015 at 6:04 PM, Jann Horn <[email protected]> wrote:
> By checking the effective credentials instead of the real UID /
> permitted capabilities, ensure that the calling process actually
> intended to use its credentials.
>
> To ensure that all ptrace checks use the correct caller
> credentials (e.g. in case out-of-tree code or newly added code
> omits the PTRACE_MODE_*CREDS flag), use two new flags and
> require one of them to be set.
>
> The problem was that when a privileged task had temporarily dropped
> its privileges, e.g. by calling setreuid(0, user_uid), with the
> intent to perform following syscalls with the credentials of
> a user, it still passed ptrace access checks that the user would
> not be able to pass.
>
> While an attacker should not be able to convince the privileged
> task to perform a ptrace() syscall, this is a problem because the
> ptrace access check is reused for things in procfs.
>
> In particular, the following somewhat interesting procfs entries
> only rely on ptrace access checks:
>
> /proc/$pid/stat - uses the check for determining whether pointers
> should be visible, useful for bypassing ASLR
> /proc/$pid/maps - also useful for bypassing ASLR
> /proc/$pid/cwd - useful for gaining access to restricted
> directories that contain files with lax permissions, e.g. in
> this scenario:
> lrwxrwxrwx root root /proc/13020/cwd -> /root/foobar
> drwx------ root root /root
> drwxr-xr-x root root /root/foobar
> -rw-r--r-- root root /root/foobar/secret
>
> Therefore, on a system where a root-owned mode 6755 binary
> changes its effective credentials as described and then dumps a
> user-specified file, this could be used by an attacker to reveal
> the memory layout of root's processes or reveal the contents of
> files he is not allowed to access (through /proc/$pid/cwd).
>
> Signed-off-by: Jann Horn <[email protected]>
> ---
> fs/proc/array.c | 2 +-
> fs/proc/base.c | 21 +++++++++++----------
> fs/proc/namespaces.c | 4 ++--
> include/linux/ptrace.h | 24 +++++++++++++++++++++++-
> kernel/events/core.c | 2 +-
> kernel/futex.c | 2 +-
> kernel/futex_compat.c | 2 +-
> kernel/kcmp.c | 4 ++--
> kernel/ptrace.c | 36 +++++++++++++++++++++++++++++-------
> mm/process_vm_access.c | 2 +-
> security/commoncap.c | 7 ++++++-
> 11 files changed, 78 insertions(+), 28 deletions(-)
>
> diff --git a/fs/proc/array.c b/fs/proc/array.c
> index d73291f..b6c00ce 100644
> --- a/fs/proc/array.c
> +++ b/fs/proc/array.c
> @@ -395,7 +395,7 @@ static int do_task_stat(struct seq_file *m, struct pid_namespace *ns,
>
> state = *get_task_state(task);
> vsize = eip = esp = 0;
> - permitted = ptrace_may_access(task, PTRACE_MODE_READ | PTRACE_MODE_NOAUDIT);
> + permitted = ptrace_may_access(task, PTRACE_MODE_READ_FSCREDS | PTRACE_MODE_NOAUDIT);
> mm = get_task_mm(task);
> if (mm) {
> vsize = task_vsize(mm);
> diff --git a/fs/proc/base.c b/fs/proc/base.c
> index bd3e9e6..c0a2f29 100644
> --- a/fs/proc/base.c
> +++ b/fs/proc/base.c
> @@ -403,7 +403,7 @@ static const struct file_operations proc_pid_cmdline_ops = {
> static int proc_pid_auxv(struct seq_file *m, struct pid_namespace *ns,
> struct pid *pid, struct task_struct *task)
> {
> - struct mm_struct *mm = mm_access(task, PTRACE_MODE_READ);
> + struct mm_struct *mm = mm_access(task, PTRACE_MODE_READ_FSCREDS);
> if (mm && !IS_ERR(mm)) {
> unsigned int nwords = 0;
> do {
> @@ -430,7 +430,8 @@ static int proc_pid_wchan(struct seq_file *m, struct pid_namespace *ns,
>
> wchan = get_wchan(task);
>
> - if (wchan && ptrace_may_access(task, PTRACE_MODE_READ) && !lookup_symbol_name(wchan, symname))
> + if (wchan && ptrace_may_access(task, PTRACE_MODE_READ_FSCREDS)
> + && !lookup_symbol_name(wchan, symname))
> seq_printf(m, "%s", symname);
> else
> seq_putc(m, '0');
> @@ -444,7 +445,7 @@ static int lock_trace(struct task_struct *task)
> int err = mutex_lock_killable(&task->signal->cred_guard_mutex);
> if (err)
> return err;
> - if (!ptrace_may_access(task, PTRACE_MODE_ATTACH)) {
> + if (!ptrace_may_access(task, PTRACE_MODE_ATTACH_FSCREDS)) {
> mutex_unlock(&task->signal->cred_guard_mutex);
> return -EPERM;
> }
> @@ -697,7 +698,7 @@ static int proc_fd_access_allowed(struct inode *inode)
> */
> task = get_proc_task(inode);
> if (task) {
> - allowed = ptrace_may_access(task, PTRACE_MODE_READ);
> + allowed = ptrace_may_access(task, PTRACE_MODE_READ_FSCREDS);
> put_task_struct(task);
> }
> return allowed;
> @@ -732,7 +733,7 @@ static bool has_pid_permissions(struct pid_namespace *pid,
> return true;
> if (in_group_p(pid->pid_gid))
> return true;
> - return ptrace_may_access(task, PTRACE_MODE_READ);
> + return ptrace_may_access(task, PTRACE_MODE_READ_FSCREDS);
> }
>
>
> @@ -809,7 +810,7 @@ struct mm_struct *proc_mem_open(struct inode *inode, unsigned int mode)
> struct mm_struct *mm = ERR_PTR(-ESRCH);
>
> if (task) {
> - mm = mm_access(task, mode);
> + mm = mm_access(task, mode | PTRACE_MODE_FSCREDS);
> put_task_struct(task);
>
> if (!IS_ERR_OR_NULL(mm)) {
> @@ -1856,7 +1857,7 @@ static int map_files_d_revalidate(struct dentry *dentry, unsigned int flags)
> if (!task)
> goto out_notask;
>
> - mm = mm_access(task, PTRACE_MODE_READ);
> + mm = mm_access(task, PTRACE_MODE_READ_FSCREDS);
> if (IS_ERR_OR_NULL(mm))
> goto out;
>
> @@ -2007,7 +2008,7 @@ static struct dentry *proc_map_files_lookup(struct inode *dir,
> goto out;
>
> result = -EACCES;
> - if (!ptrace_may_access(task, PTRACE_MODE_READ))
> + if (!ptrace_may_access(task, PTRACE_MODE_READ_FSCREDS))
> goto out_put_task;
>
> result = -ENOENT;
> @@ -2060,7 +2061,7 @@ proc_map_files_readdir(struct file *file, struct dir_context *ctx)
> goto out;
>
> ret = -EACCES;
> - if (!ptrace_may_access(task, PTRACE_MODE_READ))
> + if (!ptrace_may_access(task, PTRACE_MODE_READ_FSCREDS))
> goto out_put_task;
>
> ret = 0;
> @@ -2529,7 +2530,7 @@ static int do_io_accounting(struct task_struct *task, struct seq_file *m, int wh
> if (result)
> return result;
>
> - if (!ptrace_may_access(task, PTRACE_MODE_READ)) {
> + if (!ptrace_may_access(task, PTRACE_MODE_READ_FSCREDS)) {
> result = -EACCES;
> goto out_unlock;
> }
> diff --git a/fs/proc/namespaces.c b/fs/proc/namespaces.c
> index f6e8354..0cbe012 100644
> --- a/fs/proc/namespaces.c
> +++ b/fs/proc/namespaces.c
> @@ -42,7 +42,7 @@ static const char *proc_ns_follow_link(struct dentry *dentry, void **cookie)
> if (!task)
> return error;
>
> - if (ptrace_may_access(task, PTRACE_MODE_READ)) {
> + if (ptrace_may_access(task, PTRACE_MODE_READ | PTRACE_MODE_FSCREDS)) {

This should maybe use the PTRACE_MODE_READ_FSCREDS macro?

> error = ns_get_path(&ns_path, task, ns_ops);
> if (!error)
> nd_jump_link(&ns_path);
> @@ -63,7 +63,7 @@ static int proc_ns_readlink(struct dentry *dentry, char __user *buffer, int bufl
> if (!task)
> return res;
>
> - if (ptrace_may_access(task, PTRACE_MODE_READ)) {
> + if (ptrace_may_access(task, PTRACE_MODE_READ | PTRACE_MODE_FSCREDS)) {

same here?

> res = ns_get_name(name, sizeof(name), task, ns_ops);
> if (res >= 0)
> res = readlink_copy(buffer, buflen, name);
> diff --git a/include/linux/ptrace.h b/include/linux/ptrace.h
> index 061265f..504c98a 100644
> --- a/include/linux/ptrace.h
> +++ b/include/linux/ptrace.h
> @@ -57,7 +57,29 @@ extern void exit_ptrace(struct task_struct *tracer, struct list_head *dead);
> #define PTRACE_MODE_READ 0x01
> #define PTRACE_MODE_ATTACH 0x02
> #define PTRACE_MODE_NOAUDIT 0x04
> -/* Returns true on success, false on denial. */
> +#define PTRACE_MODE_FSCREDS 0x08
> +#define PTRACE_MODE_REALCREDS 0x10
> +
> +/* shorthands for READ/ATTACH and FSCREDS/REALCREDS combinations */
> +#define PTRACE_MODE_READ_FSCREDS (PTRACE_MODE_READ | PTRACE_MODE_FSCREDS)
> +#define PTRACE_MODE_READ_REALCREDS (PTRACE_MODE_READ | PTRACE_MODE_REALCREDS)
> +#define PTRACE_MODE_ATTACH_FSCREDS (PTRACE_MODE_ATTACH | PTRACE_MODE_FSCREDS)
> +#define PTRACE_MODE_ATTACH_REALCREDS (PTRACE_MODE_ATTACH | PTRACE_MODE_REALCREDS)
> +
> +/**
> + * ptrace_may_access - check whether the caller is permitted to access
> + * a target task.
> + * @task: target task
> + * @mode: selects type of access and caller credentials
> + *
> + * Returns true on success, false on denial.
> + *
> + * One of the flags PTRACE_MODE_FSCREDS and PTRACE_MODE_REALCREDS must
> + * be set in @mode to specify whether the access was requested through
> + * a filesystem syscall (should use effective capabilities and fsuid
> + * of the caller) or through an explicit syscall such as
> + * process_vm_writev or ptrace (and should use the real credentials).
> + */
> extern bool ptrace_may_access(struct task_struct *task, unsigned int mode);
>
> static inline int ptrace_reparented(struct task_struct *child)
> diff --git a/kernel/events/core.c b/kernel/events/core.c
> index 36babfd..565e41a 100644
> --- a/kernel/events/core.c
> +++ b/kernel/events/core.c
> @@ -3430,7 +3430,7 @@ find_lively_task_by_vpid(pid_t vpid)
>
> /* Reuse ptrace permission checks for now. */
> err = -EACCES;
> - if (!ptrace_may_access(task, PTRACE_MODE_READ))
> + if (!ptrace_may_access(task, PTRACE_MODE_READ_REALCREDS))
> goto errout;
>
> return task;
> diff --git a/kernel/futex.c b/kernel/futex.c
> index 684d754..495a1d0 100644
> --- a/kernel/futex.c
> +++ b/kernel/futex.c
> @@ -2881,7 +2881,7 @@ SYSCALL_DEFINE3(get_robust_list, int, pid,
> }
>
> ret = -EPERM;
> - if (!ptrace_may_access(p, PTRACE_MODE_READ))
> + if (!ptrace_may_access(p, PTRACE_MODE_READ_REALCREDS))
> goto err_unlock;
>
> head = p->robust_list;
> diff --git a/kernel/futex_compat.c b/kernel/futex_compat.c
> index 55c8c93..4ae3232 100644
> --- a/kernel/futex_compat.c
> +++ b/kernel/futex_compat.c
> @@ -155,7 +155,7 @@ COMPAT_SYSCALL_DEFINE3(get_robust_list, int, pid,
> }
>
> ret = -EPERM;
> - if (!ptrace_may_access(p, PTRACE_MODE_READ))
> + if (!ptrace_may_access(p, PTRACE_MODE_READ_REALCREDS))
> goto err_unlock;
>
> head = p->compat_robust_list;
> diff --git a/kernel/kcmp.c b/kernel/kcmp.c
> index 0aa69ea..3a47fa9 100644
> --- a/kernel/kcmp.c
> +++ b/kernel/kcmp.c
> @@ -122,8 +122,8 @@ SYSCALL_DEFINE5(kcmp, pid_t, pid1, pid_t, pid2, int, type,
> &task2->signal->cred_guard_mutex);
> if (ret)
> goto err;
> - if (!ptrace_may_access(task1, PTRACE_MODE_READ) ||
> - !ptrace_may_access(task2, PTRACE_MODE_READ)) {
> + if (!ptrace_may_access(task1, PTRACE_MODE_READ_REALCREDS) ||
> + !ptrace_may_access(task2, PTRACE_MODE_READ_REALCREDS)) {
> ret = -EPERM;
> goto err_unlock;
> }
> diff --git a/kernel/ptrace.c b/kernel/ptrace.c
> index b760bae..21aef51 100644
> --- a/kernel/ptrace.c
> +++ b/kernel/ptrace.c
> @@ -219,6 +219,13 @@ static int ptrace_has_cap(struct user_namespace *ns, unsigned int mode)
> static int __ptrace_may_access(struct task_struct *task, unsigned int mode)
> {
> const struct cred *cred = current_cred(), *tcred;
> + kuid_t caller_uid;
> + kgid_t caller_gid;
> +
> + if (!(mode & PTRACE_MODE_FSCREDS) == !(mode & PTRACE_MODE_REALCREDS)) {
> + WARN(1, "denying ptrace access check without PTRACE_MODE_*CREDS\n");
> + return -EPERM;
> + }
>
> /* May we inspect the given task?
> * This check is used both for attaching with ptrace
> @@ -233,13 +240,28 @@ static int __ptrace_may_access(struct task_struct *task, unsigned int mode)
> if (same_thread_group(task, current))
> return 0;
> rcu_read_lock();
> + if (mode & PTRACE_MODE_FSCREDS) {
> + caller_uid = cred->fsuid;
> + caller_gid = cred->fsgid;
> + } else {
> + /*
> + * Using the euid would make more sense here, but something
> + * in userland might rely on the old behavior, and this
> + * shouldn't be a security problem since
> + * PTRACE_MODE_REALCREDS implies that the caller explicitly
> + * used a syscall that requests access to another process
> + * (and not a filesystem syscall to procfs).
> + */
> + caller_uid = cred->uid;
> + caller_gid = cred->gid;
> + }
> tcred = __task_cred(task);
> - if (uid_eq(cred->uid, tcred->euid) &&
> - uid_eq(cred->uid, tcred->suid) &&
> - uid_eq(cred->uid, tcred->uid) &&
> - gid_eq(cred->gid, tcred->egid) &&
> - gid_eq(cred->gid, tcred->sgid) &&
> - gid_eq(cred->gid, tcred->gid))
> + if (uid_eq(caller_uid, tcred->euid) &&
> + uid_eq(caller_uid, tcred->suid) &&
> + uid_eq(caller_uid, tcred->uid) &&
> + gid_eq(caller_gid, tcred->egid) &&
> + gid_eq(caller_gid, tcred->sgid) &&
> + gid_eq(caller_gid, tcred->gid))
> goto ok;
> if (ptrace_has_cap(tcred->user_ns, mode))
> goto ok;
> @@ -306,7 +328,7 @@ static int ptrace_attach(struct task_struct *task, long request,
> goto out;
>
> task_lock(task);
> - retval = __ptrace_may_access(task, PTRACE_MODE_ATTACH);
> + retval = __ptrace_may_access(task, PTRACE_MODE_ATTACH_REALCREDS);
> task_unlock(task);
> if (retval)
> goto unlock_creds;
> diff --git a/mm/process_vm_access.c b/mm/process_vm_access.c
> index e88d071..5d453e5 100644
> --- a/mm/process_vm_access.c
> +++ b/mm/process_vm_access.c
> @@ -194,7 +194,7 @@ static ssize_t process_vm_rw_core(pid_t pid, struct iov_iter *iter,
> goto free_proc_pages;
> }
>
> - mm = mm_access(task, PTRACE_MODE_ATTACH);
> + mm = mm_access(task, PTRACE_MODE_ATTACH_REALCREDS);
> if (!mm || IS_ERR(mm)) {
> rc = IS_ERR(mm) ? PTR_ERR(mm) : -ESRCH;
> /*
> diff --git a/security/commoncap.c b/security/commoncap.c
> index 1832cf7..48071ed 100644
> --- a/security/commoncap.c
> +++ b/security/commoncap.c
> @@ -137,12 +137,17 @@ int cap_ptrace_access_check(struct task_struct *child, unsigned int mode)
> {
> int ret = 0;
> const struct cred *cred, *child_cred;
> + const kernel_cap_t *caller_caps;
>
> rcu_read_lock();
> cred = current_cred();
> child_cred = __task_cred(child);
> + if (mode & PTRACE_MODE_FSCREDS)
> + caller_caps = &cred->cap_effective;
> + else
> + caller_caps = &cred->cap_permitted;
> if (cred->user_ns == child_cred->user_ns &&
> - cap_issubset(child_cred->cap_permitted, cred->cap_permitted))
> + cap_issubset(child_cred->cap_permitted, *caller_caps))
> goto out;
> if (ns_capable(child_cred->user_ns, CAP_SYS_PTRACE))
> goto out;
> --
> 2.1.4
>

Thanks for working on this! I think it's done after the macro fix.

-Kees

--
Kees Cook
Chrome OS & Brillo Security

2015-12-07 20:37:35

by Jann Horn

Subject: Re: [PATCH] ptrace: use fsuid, fsgid, effective creds for fs access checks

On Mon, Dec 07, 2015 at 12:32:06PM -0800, Kees Cook wrote:
> On Sat, Dec 5, 2015 at 6:04 PM, Jann Horn <[email protected]> wrote:
[...]
> > - if (ptrace_may_access(task, PTRACE_MODE_READ)) {
> > + if (ptrace_may_access(task, PTRACE_MODE_READ | PTRACE_MODE_FSCREDS)) {
>
> This should maybe use the PTRACE_MODE_READ_FSCREDS macro?

Oh, yes. I don't know how I missed that. :/


> > error = ns_get_path(&ns_path, task, ns_ops);
> > if (!error)
> > nd_jump_link(&ns_path);
> > @@ -63,7 +63,7 @@ static int proc_ns_readlink(struct dentry *dentry, char __user *buffer, int bufl
> > if (!task)
> > return res;
> >
> > - if (ptrace_may_access(task, PTRACE_MODE_READ)) {
> > + if (ptrace_may_access(task, PTRACE_MODE_READ | PTRACE_MODE_FSCREDS)) {
>
> same here?

Yes.

Attachments:

2015-12-07 21:24:29

by Jann Horn

Subject: Re: [PATCH] ptrace: use fsuid, fsgid, effective creds for fs access checks

Whoops. After Kees pointed out my last mistake, I decided to grep around a bit to make sure
I didn't miss anything else and noticed that apparently, Yama and Smack aren't completely
aware that the ptrace access mode can have flags ORed in? Until now, it was just the
NOAUDIT flag for /proc/$pid/stat, but with my patch, that would have been broken completely
as far as I can tell. I don't use either of those LSMs and didn't test with them.

Can the LSM maintainers have a look at this and say whether this looks okay now?

2015-12-07 21:24:58

by Jann Horn

Subject: [PATCH 1/2] security: let security modules use PTRACE_MODE_* with bitmasks

It looks like smack and yama weren't aware that the ptrace mode
can have flags ORed into it - PTRACE_MODE_NOAUDIT until now, but
only for /proc/$pid/stat, and with the PTRACE_MODE_*CREDS patch,
all modes have flags ORed into them.

Signed-off-by: Jann Horn <[email protected]>
---
security/smack/smack_lsm.c | 8 +++-----
security/yama/yama_lsm.c | 4 ++--
2 files changed, 5 insertions(+), 7 deletions(-)

diff --git a/security/smack/smack_lsm.c b/security/smack/smack_lsm.c
index ff81026..7c57c7f 100644
--- a/security/smack/smack_lsm.c
+++ b/security/smack/smack_lsm.c
@@ -398,12 +398,10 @@ static int smk_copy_relabel(struct list_head *nhead, struct list_head *ohead,
*/
static inline unsigned int smk_ptrace_mode(unsigned int mode)
{
- switch (mode) {
- case PTRACE_MODE_READ:
- return MAY_READ;
- case PTRACE_MODE_ATTACH:
+ if (mode & PTRACE_MODE_ATTACH)
return MAY_READWRITE;
- }
+ if (mode & PTRACE_MODE_READ)
+ return MAY_READ;

return 0;
}
diff --git a/security/yama/yama_lsm.c b/security/yama/yama_lsm.c
index d3c19c9..cb6ed10 100644
--- a/security/yama/yama_lsm.c
+++ b/security/yama/yama_lsm.c
@@ -281,7 +281,7 @@ static int yama_ptrace_access_check(struct task_struct *child,
int rc = 0;

/* require ptrace target be a child of ptracer on attach */
- if (mode == PTRACE_MODE_ATTACH) {
+ if (mode & PTRACE_MODE_ATTACH) {
switch (ptrace_scope) {
case YAMA_SCOPE_DISABLED:
/* No additional restrictions. */
@@ -307,7 +307,7 @@ static int yama_ptrace_access_check(struct task_struct *child,
}
}

- if (rc) {
+ if (rc && (mode & PTRACE_MODE_NOAUDIT) == 0) {
printk_ratelimited(KERN_NOTICE
"ptrace of pid %d was attempted by: %s (pid %d)\n",
child->pid, current->comm, current->pid);
--
2.1.4

2015-12-07 21:24:36

by Jann Horn

Subject: [PATCH 2/2] ptrace: use fsuid, fsgid, effective creds for fs access checks

By checking the effective credentials instead of the real UID /
permitted capabilities, ensure that the calling process actually
intended to use its credentials.

To ensure that all ptrace checks use the correct caller
credentials (e.g. in case out-of-tree code or newly added code
omits the PTRACE_MODE_*CREDS flag), use two new flags and
require one of them to be set.

The problem was that when a privileged task had temporarily dropped
its privileges, e.g. by calling setreuid(0, user_uid), with the
intent to perform following syscalls with the credentials of
a user, it still passed ptrace access checks that the user would
not be able to pass.

While an attacker should not be able to convince the privileged
task to perform a ptrace() syscall, this is a problem because the
ptrace access check is reused for things in procfs.

In particular, the following somewhat interesting procfs entries
only rely on ptrace access checks:

/proc/$pid/stat - uses the check for determining whether pointers
should be visible, useful for bypassing ASLR
/proc/$pid/maps - also useful for bypassing ASLR
/proc/$pid/cwd - useful for gaining access to restricted
directories that contain files with lax permissions, e.g. in
this scenario:
lrwxrwxrwx root root /proc/13020/cwd -> /root/foobar
drwx------ root root /root
drwxr-xr-x root root /root/foobar
-rw-r--r-- root root /root/foobar/secret

Therefore, on a system where a root-owned mode 6755 binary
changes its effective credentials as described and then dumps a
user-specified file, this could be used by an attacker to reveal
the memory layout of root's processes or reveal the contents of
files he is not allowed to access (through /proc/$pid/cwd).

Signed-off-by: Jann Horn <[email protected]>
---
fs/proc/array.c | 2 +-
fs/proc/base.c | 21 +++++++++++----------
fs/proc/namespaces.c | 4 ++--
include/linux/ptrace.h | 24 +++++++++++++++++++++++-
kernel/events/core.c | 2 +-
kernel/futex.c | 2 +-
kernel/futex_compat.c | 2 +-
kernel/kcmp.c | 4 ++--
kernel/ptrace.c | 36 +++++++++++++++++++++++++++++-------
mm/process_vm_access.c | 2 +-
security/commoncap.c | 7 ++++++-
11 files changed, 78 insertions(+), 28 deletions(-)

diff --git a/fs/proc/array.c b/fs/proc/array.c
index d73291f..b6c00ce 100644
--- a/fs/proc/array.c
+++ b/fs/proc/array.c
@@ -395,7 +395,7 @@ static int do_task_stat(struct seq_file *m, struct pid_namespace *ns,

state = *get_task_state(task);
vsize = eip = esp = 0;
- permitted = ptrace_may_access(task, PTRACE_MODE_READ | PTRACE_MODE_NOAUDIT);
+ permitted = ptrace_may_access(task, PTRACE_MODE_READ_FSCREDS | PTRACE_MODE_NOAUDIT);
mm = get_task_mm(task);
if (mm) {
vsize = task_vsize(mm);
diff --git a/fs/proc/base.c b/fs/proc/base.c
index bd3e9e6..c0a2f29 100644
--- a/fs/proc/base.c
+++ b/fs/proc/base.c
@@ -403,7 +403,7 @@ static const struct file_operations proc_pid_cmdline_ops = {
static int proc_pid_auxv(struct seq_file *m, struct pid_namespace *ns,
struct pid *pid, struct task_struct *task)
{
- struct mm_struct *mm = mm_access(task, PTRACE_MODE_READ);
+ struct mm_struct *mm = mm_access(task, PTRACE_MODE_READ_FSCREDS);
if (mm && !IS_ERR(mm)) {
unsigned int nwords = 0;
do {
@@ -430,7 +430,8 @@ static int proc_pid_wchan(struct seq_file *m, struct pid_namespace *ns,

wchan = get_wchan(task);

- if (wchan && ptrace_may_access(task, PTRACE_MODE_READ) && !lookup_symbol_name(wchan, symname))
+ if (wchan && ptrace_may_access(task, PTRACE_MODE_READ_FSCREDS)
+ && !lookup_symbol_name(wchan, symname))
seq_printf(m, "%s", symname);
else
seq_putc(m, '0');
@@ -444,7 +445,7 @@ static int lock_trace(struct task_struct *task)
int err = mutex_lock_killable(&task->signal->cred_guard_mutex);
if (err)
return err;
- if (!ptrace_may_access(task, PTRACE_MODE_ATTACH)) {
+ if (!ptrace_may_access(task, PTRACE_MODE_ATTACH_FSCREDS)) {
mutex_unlock(&task->signal->cred_guard_mutex);
return -EPERM;
}
@@ -697,7 +698,7 @@ static int proc_fd_access_allowed(struct inode *inode)
*/
task = get_proc_task(inode);
if (task) {
- allowed = ptrace_may_access(task, PTRACE_MODE_READ);
+ allowed = ptrace_may_access(task, PTRACE_MODE_READ_FSCREDS);
put_task_struct(task);
}
return allowed;
@@ -732,7 +733,7 @@ static bool has_pid_permissions(struct pid_namespace *pid,
return true;
if (in_group_p(pid->pid_gid))
return true;
- return ptrace_may_access(task, PTRACE_MODE_READ);
+ return ptrace_may_access(task, PTRACE_MODE_READ_FSCREDS);
}


@@ -809,7 +810,7 @@ struct mm_struct *proc_mem_open(struct inode *inode, unsigned int mode)
struct mm_struct *mm = ERR_PTR(-ESRCH);

if (task) {
- mm = mm_access(task, mode);
+ mm = mm_access(task, mode | PTRACE_MODE_FSCREDS);
put_task_struct(task);

if (!IS_ERR_OR_NULL(mm)) {
@@ -1856,7 +1857,7 @@ static int map_files_d_revalidate(struct dentry *dentry, unsigned int flags)
if (!task)
goto out_notask;

- mm = mm_access(task, PTRACE_MODE_READ);
+ mm = mm_access(task, PTRACE_MODE_READ_FSCREDS);
if (IS_ERR_OR_NULL(mm))
goto out;

@@ -2007,7 +2008,7 @@ static struct dentry *proc_map_files_lookup(struct inode *dir,
goto out;

result = -EACCES;
- if (!ptrace_may_access(task, PTRACE_MODE_READ))
+ if (!ptrace_may_access(task, PTRACE_MODE_READ_FSCREDS))
goto out_put_task;

result = -ENOENT;
@@ -2060,7 +2061,7 @@ proc_map_files_readdir(struct file *file, struct dir_context *ctx)
goto out;

ret = -EACCES;
- if (!ptrace_may_access(task, PTRACE_MODE_READ))
+ if (!ptrace_may_access(task, PTRACE_MODE_READ_FSCREDS))
goto out_put_task;

ret = 0;
@@ -2529,7 +2530,7 @@ static int do_io_accounting(struct task_struct *task, struct seq_file *m, int wh
if (result)
return result;

- if (!ptrace_may_access(task, PTRACE_MODE_READ)) {
+ if (!ptrace_may_access(task, PTRACE_MODE_READ_FSCREDS)) {
result = -EACCES;
goto out_unlock;
}
diff --git a/fs/proc/namespaces.c b/fs/proc/namespaces.c
index f6e8354..1b0ea4a 100644
--- a/fs/proc/namespaces.c
+++ b/fs/proc/namespaces.c
@@ -42,7 +42,7 @@ static const char *proc_ns_follow_link(struct dentry *dentry, void **cookie)
if (!task)
return error;

- if (ptrace_may_access(task, PTRACE_MODE_READ)) {
+ if (ptrace_may_access(task, PTRACE_MODE_READ_FSCREDS)) {
error = ns_get_path(&ns_path, task, ns_ops);
if (!error)
nd_jump_link(&ns_path);
@@ -63,7 +63,7 @@ static int proc_ns_readlink(struct dentry *dentry, char __user *buffer, int bufl
if (!task)
return res;

- if (ptrace_may_access(task, PTRACE_MODE_READ)) {
+ if (ptrace_may_access(task, PTRACE_MODE_READ_FSCREDS)) {
res = ns_get_name(name, sizeof(name), task, ns_ops);
if (res >= 0)
res = readlink_copy(buffer, buflen, name);
diff --git a/include/linux/ptrace.h b/include/linux/ptrace.h
index 061265f..504c98a 100644
--- a/include/linux/ptrace.h
+++ b/include/linux/ptrace.h
@@ -57,7 +57,29 @@ extern void exit_ptrace(struct task_struct *tracer, struct list_head *dead);
#define PTRACE_MODE_READ 0x01
#define PTRACE_MODE_ATTACH 0x02
#define PTRACE_MODE_NOAUDIT 0x04
-/* Returns true on success, false on denial. */
+#define PTRACE_MODE_FSCREDS 0x08
+#define PTRACE_MODE_REALCREDS 0x10
+
+/* shorthands for READ/ATTACH and FSCREDS/REALCREDS combinations */
+#define PTRACE_MODE_READ_FSCREDS (PTRACE_MODE_READ | PTRACE_MODE_FSCREDS)
+#define PTRACE_MODE_READ_REALCREDS (PTRACE_MODE_READ | PTRACE_MODE_REALCREDS)
+#define PTRACE_MODE_ATTACH_FSCREDS (PTRACE_MODE_ATTACH | PTRACE_MODE_FSCREDS)
+#define PTRACE_MODE_ATTACH_REALCREDS (PTRACE_MODE_ATTACH | PTRACE_MODE_REALCREDS)
+
+/**
+ * ptrace_may_access - check whether the caller is permitted to access
+ * a target task.
+ * @task: target task
+ * @mode: selects type of access and caller credentials
+ *
+ * Returns true on success, false on denial.
+ *
+ * One of the flags PTRACE_MODE_FSCREDS and PTRACE_MODE_REALCREDS must
+ * be set in @mode to specify whether the access was requested through
+ * a filesystem syscall (should use effective capabilities and fsuid
+ * of the caller) or through an explicit syscall such as
+ * process_vm_writev or ptrace (and should use the real credentials).
+ */
extern bool ptrace_may_access(struct task_struct *task, unsigned int mode);

static inline int ptrace_reparented(struct task_struct *child)
diff --git a/kernel/events/core.c b/kernel/events/core.c
index 36babfd..565e41a 100644
--- a/kernel/events/core.c
+++ b/kernel/events/core.c
@@ -3430,7 +3430,7 @@ find_lively_task_by_vpid(pid_t vpid)

/* Reuse ptrace permission checks for now. */
err = -EACCES;
- if (!ptrace_may_access(task, PTRACE_MODE_READ))
+ if (!ptrace_may_access(task, PTRACE_MODE_READ_REALCREDS))
goto errout;

return task;
diff --git a/kernel/futex.c b/kernel/futex.c
index 684d754..495a1d0 100644
--- a/kernel/futex.c
+++ b/kernel/futex.c
@@ -2881,7 +2881,7 @@ SYSCALL_DEFINE3(get_robust_list, int, pid,
}

ret = -EPERM;
- if (!ptrace_may_access(p, PTRACE_MODE_READ))
+ if (!ptrace_may_access(p, PTRACE_MODE_READ_REALCREDS))
goto err_unlock;

head = p->robust_list;
diff --git a/kernel/futex_compat.c b/kernel/futex_compat.c
index 55c8c93..4ae3232 100644
--- a/kernel/futex_compat.c
+++ b/kernel/futex_compat.c
@@ -155,7 +155,7 @@ COMPAT_SYSCALL_DEFINE3(get_robust_list, int, pid,
}

ret = -EPERM;
- if (!ptrace_may_access(p, PTRACE_MODE_READ))
+ if (!ptrace_may_access(p, PTRACE_MODE_READ_REALCREDS))
goto err_unlock;

head = p->compat_robust_list;
diff --git a/kernel/kcmp.c b/kernel/kcmp.c
index 0aa69ea..3a47fa9 100644
--- a/kernel/kcmp.c
+++ b/kernel/kcmp.c
@@ -122,8 +122,8 @@ SYSCALL_DEFINE5(kcmp, pid_t, pid1, pid_t, pid2, int, type,
&task2->signal->cred_guard_mutex);
if (ret)
goto err;
- if (!ptrace_may_access(task1, PTRACE_MODE_READ) ||
- !ptrace_may_access(task2, PTRACE_MODE_READ)) {
+ if (!ptrace_may_access(task1, PTRACE_MODE_READ_REALCREDS) ||
+ !ptrace_may_access(task2, PTRACE_MODE_READ_REALCREDS)) {
ret = -EPERM;
goto err_unlock;
}
diff --git a/kernel/ptrace.c b/kernel/ptrace.c
index b760bae..21aef51 100644
--- a/kernel/ptrace.c
+++ b/kernel/ptrace.c
@@ -219,6 +219,13 @@ static int ptrace_has_cap(struct user_namespace *ns, unsigned int mode)
static int __ptrace_may_access(struct task_struct *task, unsigned int mode)
{
const struct cred *cred = current_cred(), *tcred;
+ kuid_t caller_uid;
+ kgid_t caller_gid;
+
+ if (!(mode & PTRACE_MODE_FSCREDS) == !(mode & PTRACE_MODE_REALCREDS)) {
+ WARN(1, "denying ptrace access check without PTRACE_MODE_*CREDS\n");
+ return -EPERM;
+ }

/* May we inspect the given task?
* This check is used both for attaching with ptrace
@@ -233,13 +240,28 @@ static int __ptrace_may_access(struct task_struct *task, unsigned int mode)
if (same_thread_group(task, current))
return 0;
rcu_read_lock();
+ if (mode & PTRACE_MODE_FSCREDS) {
+ caller_uid = cred->fsuid;
+ caller_gid = cred->fsgid;
+ } else {
+ /*
+ * Using the euid would make more sense here, but something
+ * in userland might rely on the old behavior, and this
+ * shouldn't be a security problem since
+ * PTRACE_MODE_REALCREDS implies that the caller explicitly
+ * used a syscall that requests access to another process
+ * (and not a filesystem syscall to procfs).
+ */
+ caller_uid = cred->uid;
+ caller_gid = cred->gid;
+ }
tcred = __task_cred(task);
- if (uid_eq(cred->uid, tcred->euid) &&
- uid_eq(cred->uid, tcred->suid) &&
- uid_eq(cred->uid, tcred->uid) &&
- gid_eq(cred->gid, tcred->egid) &&
- gid_eq(cred->gid, tcred->sgid) &&
- gid_eq(cred->gid, tcred->gid))
+ if (uid_eq(caller_uid, tcred->euid) &&
+ uid_eq(caller_uid, tcred->suid) &&
+ uid_eq(caller_uid, tcred->uid) &&
+ gid_eq(caller_gid, tcred->egid) &&
+ gid_eq(caller_gid, tcred->sgid) &&
+ gid_eq(caller_gid, tcred->gid))
goto ok;
if (ptrace_has_cap(tcred->user_ns, mode))
goto ok;
@@ -306,7 +328,7 @@ static int ptrace_attach(struct task_struct *task, long request,
goto out;

task_lock(task);
- retval = __ptrace_may_access(task, PTRACE_MODE_ATTACH);
+ retval = __ptrace_may_access(task, PTRACE_MODE_ATTACH_REALCREDS);
task_unlock(task);
if (retval)
goto unlock_creds;
diff --git a/mm/process_vm_access.c b/mm/process_vm_access.c
index e88d071..5d453e5 100644
--- a/mm/process_vm_access.c
+++ b/mm/process_vm_access.c
@@ -194,7 +194,7 @@ static ssize_t process_vm_rw_core(pid_t pid, struct iov_iter *iter,
goto free_proc_pages;
}

- mm = mm_access(task, PTRACE_MODE_ATTACH);
+ mm = mm_access(task, PTRACE_MODE_ATTACH_REALCREDS);
if (!mm || IS_ERR(mm)) {
rc = IS_ERR(mm) ? PTR_ERR(mm) : -ESRCH;
/*
diff --git a/security/commoncap.c b/security/commoncap.c
index 1832cf7..48071ed 100644
--- a/security/commoncap.c
+++ b/security/commoncap.c
@@ -137,12 +137,17 @@ int cap_ptrace_access_check(struct task_struct *child, unsigned int mode)
{
int ret = 0;
const struct cred *cred, *child_cred;
+ const kernel_cap_t *caller_caps;

rcu_read_lock();
cred = current_cred();
child_cred = __task_cred(child);
+ if (mode & PTRACE_MODE_FSCREDS)
+ caller_caps = &cred->cap_effective;
+ else
+ caller_caps = &cred->cap_permitted;
if (cred->user_ns == child_cred->user_ns &&
- cap_issubset(child_cred->cap_permitted, cred->cap_permitted))
+ cap_issubset(child_cred->cap_permitted, *caller_caps))
goto out;
if (ns_capable(child_cred->user_ns, CAP_SYS_PTRACE))
goto out;
--
2.1.4

2015-12-07 21:27:56

by Kees Cook

Subject: Re: [PATCH 2/2] ptrace: use fsuid, fsgid, effective creds for fs access checks

On Mon, Dec 7, 2015 at 1:25 PM, Jann Horn <[email protected]> wrote:
> By checking the effective credentials instead of the real UID /
> permitted capabilities, ensure that the calling process actually
> intended to use its credentials.
>
> To ensure that all ptrace checks use the correct caller
> credentials (e.g. in case out-of-tree code or newly added code
> omits the PTRACE_MODE_*CREDS flag), use two new flags and
> require one of them to be set.
>
> The problem was that when a privileged task had temporarily dropped
> its privileges, e.g. by calling setreuid(0, user_uid), with the
> intent to perform following syscalls with the credentials of
> a user, it still passed ptrace access checks that the user would
> not be able to pass.
>
> While an attacker should not be able to convince the privileged
> task to perform a ptrace() syscall, this is a problem because the
> ptrace access check is reused for things in procfs.
>
> In particular, the following somewhat interesting procfs entries
> only rely on ptrace access checks:
>
> /proc/$pid/stat - uses the check for determining whether pointers
> should be visible, useful for bypassing ASLR
> /proc/$pid/maps - also useful for bypassing ASLR
> /proc/$pid/cwd - useful for gaining access to restricted
> directories that contain files with lax permissions, e.g. in
> this scenario:
> lrwxrwxrwx root root /proc/13020/cwd -> /root/foobar
> drwx------ root root /root
> drwxr-xr-x root root /root/foobar
> -rw-r--r-- root root /root/foobar/secret
>
> Therefore, on a system where a root-owned mode 6755 binary
> changes its effective credentials as described and then dumps a
> user-specified file, this could be used by an attacker to reveal
> the memory layout of root's processes or reveal the contents of
> files he is not allowed to access (through /proc/$pid/cwd).
>
> Signed-off-by: Jann Horn <[email protected]>

Acked-by: Kees Cook <[email protected]>

-Kees

> ---
> fs/proc/array.c | 2 +-
> fs/proc/base.c | 21 +++++++++++----------
> fs/proc/namespaces.c | 4 ++--
> include/linux/ptrace.h | 24 +++++++++++++++++++++++-
> kernel/events/core.c | 2 +-
> kernel/futex.c | 2 +-
> kernel/futex_compat.c | 2 +-
> kernel/kcmp.c | 4 ++--
> kernel/ptrace.c | 36 +++++++++++++++++++++++++++++-------
> mm/process_vm_access.c | 2 +-
> security/commoncap.c | 7 ++++++-
> 11 files changed, 78 insertions(+), 28 deletions(-)
>
> diff --git a/fs/proc/array.c b/fs/proc/array.c
> index d73291f..b6c00ce 100644
> --- a/fs/proc/array.c
> +++ b/fs/proc/array.c
> @@ -395,7 +395,7 @@ static int do_task_stat(struct seq_file *m, struct pid_namespace *ns,
>
> state = *get_task_state(task);
> vsize = eip = esp = 0;
> - permitted = ptrace_may_access(task, PTRACE_MODE_READ | PTRACE_MODE_NOAUDIT);
> + permitted = ptrace_may_access(task, PTRACE_MODE_READ_FSCREDS | PTRACE_MODE_NOAUDIT);
> mm = get_task_mm(task);
> if (mm) {
> vsize = task_vsize(mm);
> diff --git a/fs/proc/base.c b/fs/proc/base.c
> index bd3e9e6..c0a2f29 100644
> --- a/fs/proc/base.c
> +++ b/fs/proc/base.c
> @@ -403,7 +403,7 @@ static const struct file_operations proc_pid_cmdline_ops = {
> static int proc_pid_auxv(struct seq_file *m, struct pid_namespace *ns,
> struct pid *pid, struct task_struct *task)
> {
> - struct mm_struct *mm = mm_access(task, PTRACE_MODE_READ);
> + struct mm_struct *mm = mm_access(task, PTRACE_MODE_READ_FSCREDS);
> if (mm && !IS_ERR(mm)) {
> unsigned int nwords = 0;
> do {
> @@ -430,7 +430,8 @@ static int proc_pid_wchan(struct seq_file *m, struct pid_namespace *ns,
>
> wchan = get_wchan(task);
>
> - if (wchan && ptrace_may_access(task, PTRACE_MODE_READ) && !lookup_symbol_name(wchan, symname))
> + if (wchan && ptrace_may_access(task, PTRACE_MODE_READ_FSCREDS)
> + && !lookup_symbol_name(wchan, symname))
> seq_printf(m, "%s", symname);
> else
> seq_putc(m, '0');
> @@ -444,7 +445,7 @@ static int lock_trace(struct task_struct *task)
> int err = mutex_lock_killable(&task->signal->cred_guard_mutex);
> if (err)
> return err;
> - if (!ptrace_may_access(task, PTRACE_MODE_ATTACH)) {
> + if (!ptrace_may_access(task, PTRACE_MODE_ATTACH_FSCREDS)) {
> mutex_unlock(&task->signal->cred_guard_mutex);
> return -EPERM;
> }
> @@ -697,7 +698,7 @@ static int proc_fd_access_allowed(struct inode *inode)
> */
> task = get_proc_task(inode);
> if (task) {
> - allowed = ptrace_may_access(task, PTRACE_MODE_READ);
> + allowed = ptrace_may_access(task, PTRACE_MODE_READ_FSCREDS);
> put_task_struct(task);
> }
> return allowed;
> @@ -732,7 +733,7 @@ static bool has_pid_permissions(struct pid_namespace *pid,
> return true;
> if (in_group_p(pid->pid_gid))
> return true;
> - return ptrace_may_access(task, PTRACE_MODE_READ);
> + return ptrace_may_access(task, PTRACE_MODE_READ_FSCREDS);
> }
>
>
> @@ -809,7 +810,7 @@ struct mm_struct *proc_mem_open(struct inode *inode, unsigned int mode)
> struct mm_struct *mm = ERR_PTR(-ESRCH);
>
> if (task) {
> - mm = mm_access(task, mode);
> + mm = mm_access(task, mode | PTRACE_MODE_FSCREDS);
> put_task_struct(task);
>
> if (!IS_ERR_OR_NULL(mm)) {
> @@ -1856,7 +1857,7 @@ static int map_files_d_revalidate(struct dentry *dentry, unsigned int flags)
> if (!task)
> goto out_notask;
>
> - mm = mm_access(task, PTRACE_MODE_READ);
> + mm = mm_access(task, PTRACE_MODE_READ_FSCREDS);
> if (IS_ERR_OR_NULL(mm))
> goto out;
>
> @@ -2007,7 +2008,7 @@ static struct dentry *proc_map_files_lookup(struct inode *dir,
> goto out;
>
> result = -EACCES;
> - if (!ptrace_may_access(task, PTRACE_MODE_READ))
> + if (!ptrace_may_access(task, PTRACE_MODE_READ_FSCREDS))
> goto out_put_task;
>
> result = -ENOENT;
> @@ -2060,7 +2061,7 @@ proc_map_files_readdir(struct file *file, struct dir_context *ctx)
> goto out;
>
> ret = -EACCES;
> - if (!ptrace_may_access(task, PTRACE_MODE_READ))
> + if (!ptrace_may_access(task, PTRACE_MODE_READ_FSCREDS))
> goto out_put_task;
>
> ret = 0;
> @@ -2529,7 +2530,7 @@ static int do_io_accounting(struct task_struct *task, struct seq_file *m, int wh
> if (result)
> return result;
>
> - if (!ptrace_may_access(task, PTRACE_MODE_READ)) {
> + if (!ptrace_may_access(task, PTRACE_MODE_READ_FSCREDS)) {
> result = -EACCES;
> goto out_unlock;
> }
> diff --git a/fs/proc/namespaces.c b/fs/proc/namespaces.c
> index f6e8354..1b0ea4a 100644
> --- a/fs/proc/namespaces.c
> +++ b/fs/proc/namespaces.c
> @@ -42,7 +42,7 @@ static const char *proc_ns_follow_link(struct dentry *dentry, void **cookie)
> if (!task)
> return error;
>
> - if (ptrace_may_access(task, PTRACE_MODE_READ)) {
> + if (ptrace_may_access(task, PTRACE_MODE_READ_FSCREDS)) {
> error = ns_get_path(&ns_path, task, ns_ops);
> if (!error)
> nd_jump_link(&ns_path);
> @@ -63,7 +63,7 @@ static int proc_ns_readlink(struct dentry *dentry, char __user *buffer, int bufl
> if (!task)
> return res;
>
> - if (ptrace_may_access(task, PTRACE_MODE_READ)) {
> + if (ptrace_may_access(task, PTRACE_MODE_READ_FSCREDS)) {
> res = ns_get_name(name, sizeof(name), task, ns_ops);
> if (res >= 0)
> res = readlink_copy(buffer, buflen, name);
> diff --git a/include/linux/ptrace.h b/include/linux/ptrace.h
> index 061265f..504c98a 100644
> --- a/include/linux/ptrace.h
> +++ b/include/linux/ptrace.h
> @@ -57,7 +57,29 @@ extern void exit_ptrace(struct task_struct *tracer, struct list_head *dead);
> #define PTRACE_MODE_READ 0x01
> #define PTRACE_MODE_ATTACH 0x02
> #define PTRACE_MODE_NOAUDIT 0x04
> -/* Returns true on success, false on denial. */
> +#define PTRACE_MODE_FSCREDS 0x08
> +#define PTRACE_MODE_REALCREDS 0x10
> +
> +/* shorthands for READ/ATTACH and FSCREDS/REALCREDS combinations */
> +#define PTRACE_MODE_READ_FSCREDS (PTRACE_MODE_READ | PTRACE_MODE_FSCREDS)
> +#define PTRACE_MODE_READ_REALCREDS (PTRACE_MODE_READ | PTRACE_MODE_REALCREDS)
> +#define PTRACE_MODE_ATTACH_FSCREDS (PTRACE_MODE_ATTACH | PTRACE_MODE_FSCREDS)
> +#define PTRACE_MODE_ATTACH_REALCREDS (PTRACE_MODE_ATTACH | PTRACE_MODE_REALCREDS)
> +
> +/**
> + * ptrace_may_access - check whether the caller is permitted to access
> + * a target task.
> + * @task: target task
> + * @mode: selects type of access and caller credentials
> + *
> + * Returns true on success, false on denial.
> + *
> + * One of the flags PTRACE_MODE_FSCREDS and PTRACE_MODE_REALCREDS must
> + * be set in @mode to specify whether the access was requested through
> + * a filesystem syscall (should use effective capabilities and fsuid
> + * of the caller) or through an explicit syscall such as
> + * process_vm_writev or ptrace (and should use the real credentials).
> + */
> extern bool ptrace_may_access(struct task_struct *task, unsigned int mode);
>
> static inline int ptrace_reparented(struct task_struct *child)
> diff --git a/kernel/events/core.c b/kernel/events/core.c
> index 36babfd..565e41a 100644
> --- a/kernel/events/core.c
> +++ b/kernel/events/core.c
> @@ -3430,7 +3430,7 @@ find_lively_task_by_vpid(pid_t vpid)
>
> /* Reuse ptrace permission checks for now. */
> err = -EACCES;
> - if (!ptrace_may_access(task, PTRACE_MODE_READ))
> + if (!ptrace_may_access(task, PTRACE_MODE_READ_REALCREDS))
> goto errout;
>
> return task;
> diff --git a/kernel/futex.c b/kernel/futex.c
> index 684d754..495a1d0 100644
> --- a/kernel/futex.c
> +++ b/kernel/futex.c
> @@ -2881,7 +2881,7 @@ SYSCALL_DEFINE3(get_robust_list, int, pid,
> }
>
> ret = -EPERM;
> - if (!ptrace_may_access(p, PTRACE_MODE_READ))
> + if (!ptrace_may_access(p, PTRACE_MODE_READ_REALCREDS))
> goto err_unlock;
>
> head = p->robust_list;
> diff --git a/kernel/futex_compat.c b/kernel/futex_compat.c
> index 55c8c93..4ae3232 100644
> --- a/kernel/futex_compat.c
> +++ b/kernel/futex_compat.c
> @@ -155,7 +155,7 @@ COMPAT_SYSCALL_DEFINE3(get_robust_list, int, pid,
> }
>
> ret = -EPERM;
> - if (!ptrace_may_access(p, PTRACE_MODE_READ))
> + if (!ptrace_may_access(p, PTRACE_MODE_READ_REALCREDS))
> goto err_unlock;
>
> head = p->compat_robust_list;
> diff --git a/kernel/kcmp.c b/kernel/kcmp.c
> index 0aa69ea..3a47fa9 100644
> --- a/kernel/kcmp.c
> +++ b/kernel/kcmp.c
> @@ -122,8 +122,8 @@ SYSCALL_DEFINE5(kcmp, pid_t, pid1, pid_t, pid2, int, type,
> &task2->signal->cred_guard_mutex);
> if (ret)
> goto err;
> - if (!ptrace_may_access(task1, PTRACE_MODE_READ) ||
> - !ptrace_may_access(task2, PTRACE_MODE_READ)) {
> + if (!ptrace_may_access(task1, PTRACE_MODE_READ_REALCREDS) ||
> + !ptrace_may_access(task2, PTRACE_MODE_READ_REALCREDS)) {
> ret = -EPERM;
> goto err_unlock;
> }
> diff --git a/kernel/ptrace.c b/kernel/ptrace.c
> index b760bae..21aef51 100644
> --- a/kernel/ptrace.c
> +++ b/kernel/ptrace.c
> @@ -219,6 +219,13 @@ static int ptrace_has_cap(struct user_namespace *ns, unsigned int mode)
> static int __ptrace_may_access(struct task_struct *task, unsigned int mode)
> {
> const struct cred *cred = current_cred(), *tcred;
> + kuid_t caller_uid;
> + kgid_t caller_gid;
> +
> + if (!(mode & PTRACE_MODE_FSCREDS) == !(mode & PTRACE_MODE_REALCREDS)) {
> + WARN(1, "denying ptrace access check without PTRACE_MODE_*CREDS\n");
> + return -EPERM;
> + }
>
> /* May we inspect the given task?
> * This check is used both for attaching with ptrace
> @@ -233,13 +240,28 @@ static int __ptrace_may_access(struct task_struct *task, unsigned int mode)
> if (same_thread_group(task, current))
> return 0;
> rcu_read_lock();
> + if (mode & PTRACE_MODE_FSCREDS) {
> + caller_uid = cred->fsuid;
> + caller_gid = cred->fsgid;
> + } else {
> + /*
> + * Using the euid would make more sense here, but something
> + * in userland might rely on the old behavior, and this
> + * shouldn't be a security problem since
> + * PTRACE_MODE_REALCREDS implies that the caller explicitly
> + * used a syscall that requests access to another process
> + * (and not a filesystem syscall to procfs).
> + */
> + caller_uid = cred->uid;
> + caller_gid = cred->gid;
> + }
> tcred = __task_cred(task);
> - if (uid_eq(cred->uid, tcred->euid) &&
> - uid_eq(cred->uid, tcred->suid) &&
> - uid_eq(cred->uid, tcred->uid) &&
> - gid_eq(cred->gid, tcred->egid) &&
> - gid_eq(cred->gid, tcred->sgid) &&
> - gid_eq(cred->gid, tcred->gid))
> + if (uid_eq(caller_uid, tcred->euid) &&
> + uid_eq(caller_uid, tcred->suid) &&
> + uid_eq(caller_uid, tcred->uid) &&
> + gid_eq(caller_gid, tcred->egid) &&
> + gid_eq(caller_gid, tcred->sgid) &&
> + gid_eq(caller_gid, tcred->gid))
> goto ok;
> if (ptrace_has_cap(tcred->user_ns, mode))
> goto ok;
> @@ -306,7 +328,7 @@ static int ptrace_attach(struct task_struct *task, long request,
> goto out;
>
> task_lock(task);
> - retval = __ptrace_may_access(task, PTRACE_MODE_ATTACH);
> + retval = __ptrace_may_access(task, PTRACE_MODE_ATTACH_REALCREDS);
> task_unlock(task);
> if (retval)
> goto unlock_creds;
> diff --git a/mm/process_vm_access.c b/mm/process_vm_access.c
> index e88d071..5d453e5 100644
> --- a/mm/process_vm_access.c
> +++ b/mm/process_vm_access.c
> @@ -194,7 +194,7 @@ static ssize_t process_vm_rw_core(pid_t pid, struct iov_iter *iter,
> goto free_proc_pages;
> }
>
> - mm = mm_access(task, PTRACE_MODE_ATTACH);
> + mm = mm_access(task, PTRACE_MODE_ATTACH_REALCREDS);
> if (!mm || IS_ERR(mm)) {
> rc = IS_ERR(mm) ? PTR_ERR(mm) : -ESRCH;
> /*
> diff --git a/security/commoncap.c b/security/commoncap.c
> index 1832cf7..48071ed 100644
> --- a/security/commoncap.c
> +++ b/security/commoncap.c
> @@ -137,12 +137,17 @@ int cap_ptrace_access_check(struct task_struct *child, unsigned int mode)
> {
> int ret = 0;
> const struct cred *cred, *child_cred;
> + const kernel_cap_t *caller_caps;
>
> rcu_read_lock();
> cred = current_cred();
> child_cred = __task_cred(child);
> + if (mode & PTRACE_MODE_FSCREDS)
> + caller_caps = &cred->cap_effective;
> + else
> + caller_caps = &cred->cap_permitted;
> if (cred->user_ns == child_cred->user_ns &&
> - cap_issubset(child_cred->cap_permitted, cred->cap_permitted))
> + cap_issubset(child_cred->cap_permitted, *caller_caps))
> goto out;
> if (ns_capable(child_cred->user_ns, CAP_SYS_PTRACE))
> goto out;
> --
> 2.1.4
>



--
Kees Cook
Chrome OS & Brillo Security

2015-12-07 21:30:58

by Kees Cook

Subject: Re: [PATCH 1/2] security: let security modules use PTRACE_MODE_* with bitmasks

On Mon, Dec 7, 2015 at 1:25 PM, Jann Horn <[email protected]> wrote:
> It looks like smack and yama weren't aware that the ptrace mode
> can have flags ORed into it - PTRACE_MODE_NOAUDIT until now, but
> only for /proc/$pid/stat, and with the PTRACE_MODE_*CREDS patch,
> all modes have flags ORed into them.
>
> Signed-off-by: Jann Horn <[email protected]>

Acked-by: Kees Cook <[email protected]>

-Kees

> ---
> security/smack/smack_lsm.c | 8 +++-----
> security/yama/yama_lsm.c | 4 ++--
> 2 files changed, 5 insertions(+), 7 deletions(-)
>
> diff --git a/security/smack/smack_lsm.c b/security/smack/smack_lsm.c
> index ff81026..7c57c7f 100644
> --- a/security/smack/smack_lsm.c
> +++ b/security/smack/smack_lsm.c
> @@ -398,12 +398,10 @@ static int smk_copy_relabel(struct list_head *nhead, struct list_head *ohead,
> */
> static inline unsigned int smk_ptrace_mode(unsigned int mode)
> {
> - switch (mode) {
> - case PTRACE_MODE_READ:
> - return MAY_READ;
> - case PTRACE_MODE_ATTACH:
> + if (mode & PTRACE_MODE_ATTACH)
> return MAY_READWRITE;
> - }
> + if (mode & PTRACE_MODE_READ)
> + return MAY_READ;
>
> return 0;
> }
> diff --git a/security/yama/yama_lsm.c b/security/yama/yama_lsm.c
> index d3c19c9..cb6ed10 100644
> --- a/security/yama/yama_lsm.c
> +++ b/security/yama/yama_lsm.c
> @@ -281,7 +281,7 @@ static int yama_ptrace_access_check(struct task_struct *child,
> int rc = 0;
>
> /* require ptrace target be a child of ptracer on attach */
> - if (mode == PTRACE_MODE_ATTACH) {
> + if (mode & PTRACE_MODE_ATTACH) {
> switch (ptrace_scope) {
> case YAMA_SCOPE_DISABLED:
> /* No additional restrictions. */
> @@ -307,7 +307,7 @@ static int yama_ptrace_access_check(struct task_struct *child,
> }
> }
>
> - if (rc) {
> + if (rc && (mode & PTRACE_MODE_NOAUDIT) == 0) {
> printk_ratelimited(KERN_NOTICE
> "ptrace of pid %d was attempted by: %s (pid %d)\n",
> child->pid, current->comm, current->pid);
> --
> 2.1.4
>



--
Kees Cook
Chrome OS & Brillo Security

2015-12-07 21:52:36

by Casey Schaufler

Subject: Re: [PATCH 1/2] security: let security modules use PTRACE_MODE_* with bitmasks

On 12/7/2015 1:25 PM, Jann Horn wrote:
> It looks like smack and yama weren't aware that the ptrace mode
> can have flags ORed into it - PTRACE_MODE_NOAUDIT until now, but
> only for /proc/$pid/stat, and with the PTRACE_MODE_*CREDS patch,
> all modes have flags ORed into them.
>
> Signed-off-by: Jann Horn <[email protected]>

Acked-by: Casey Schaufler <[email protected]>

> ---
> security/smack/smack_lsm.c | 8 +++-----
> security/yama/yama_lsm.c | 4 ++--
> 2 files changed, 5 insertions(+), 7 deletions(-)
>
> diff --git a/security/smack/smack_lsm.c b/security/smack/smack_lsm.c
> index ff81026..7c57c7f 100644
> --- a/security/smack/smack_lsm.c
> +++ b/security/smack/smack_lsm.c
> @@ -398,12 +398,10 @@ static int smk_copy_relabel(struct list_head *nhead, struct list_head *ohead,
> */
> static inline unsigned int smk_ptrace_mode(unsigned int mode)
> {
> - switch (mode) {
> - case PTRACE_MODE_READ:
> - return MAY_READ;
> - case PTRACE_MODE_ATTACH:
> + if (mode & PTRACE_MODE_ATTACH)
> return MAY_READWRITE;
> - }
> + if (mode & PTRACE_MODE_READ)
> + return MAY_READ;
>
> return 0;
> }
> diff --git a/security/yama/yama_lsm.c b/security/yama/yama_lsm.c
> index d3c19c9..cb6ed10 100644
> --- a/security/yama/yama_lsm.c
> +++ b/security/yama/yama_lsm.c
> @@ -281,7 +281,7 @@ static int yama_ptrace_access_check(struct task_struct *child,
> int rc = 0;
>
> /* require ptrace target be a child of ptracer on attach */
> - if (mode == PTRACE_MODE_ATTACH) {
> + if (mode & PTRACE_MODE_ATTACH) {
> switch (ptrace_scope) {
> case YAMA_SCOPE_DISABLED:
> /* No additional restrictions. */
> @@ -307,7 +307,7 @@ static int yama_ptrace_access_check(struct task_struct *child,
> }
> }
>
> - if (rc) {
> + if (rc && (mode & PTRACE_MODE_NOAUDIT) == 0) {
> printk_ratelimited(KERN_NOTICE
> "ptrace of pid %d was attempted by: %s (pid %d)\n",
> child->pid, current->comm, current->pid);