/proc/<pid>/* entries varies at runtime, appropriate permission checks
need to happen during each system call.
Currently some of these sensitive entries are protected by performing
the ptrace_may_access() check. However even with that the /proc file
descriptors can be passed to a more privileged process
(e.g. a suid-exec) which will pass the classic ptrace_may_access()
check. In general the ->open() call will be issued by an unprivileged
process while the ->read(),->write() calls by a more privileged one.
Example of these files are:
/proc/*/syscall, /proc/*/stack etc.
And any open(/proc/self/*) then suid-exec to read()/write() /proc/self/*
These files are protected during read() by the ptrace_may_access(),
however the file descriptor can be passed to a suid-exec which can be
used to read data and bypass ASLR. Of course this was discussed several
times on LKML.
Solution:
Here we propose a clean solution that uses available mechanisms. No
additions, nor new structs/memory allocation...
After a discussion about some /proc/<pid>/* file permissions [1],
Eric W. Biederman proposed to use the file->f_cred to check if current's
cred have changed [2], actually he said that Linus was looking
on using the file->f_cred to implement a similar thing! But run in
problems with Chromes sandbox ? a link please ?
So here are my experiments:
The idea is to track the cred of current. If the cred change between
->open() and read()/write() this means that current has lost or gained
some X privileges. If so, in addition to the classic ptrace_may_access()
check, perform a second check using the file's opener cred. This means,
if an unprivileged process that tries to use a privileged one
(e.g. suid-exec) to read privileged files will get caught. The original
process that opened the file does not have the appropriate permissions
to read()/write() the target /proc/<pid>/* entry.
This second check is done of course during read(),write()...
Advantages of the proposed solution:
* It uses available mechanisms: file->f_cred which is a const cred
that reference the cred of the opener.
* The file->f_cred can be easily referenced
* It allows to pass file descriptors under certain conditions:
(1) current at open time may have enough permissions
(2) current does a suid-exec or change its ruid/euid (new cred)
(3) current or suid-exec tries to read from the task /proc entry
Allow the ->read() only if the file's opener cred at (1)
have enough permissions on *this* task /proc entry during
*this* ->read() moment. Otherwise fail.
IOW allow it, if the opener does not need the help of a suid-exec to
read/write data.
Disadvantage:
* Currently only /proc/*/{stack,syscall,stat,personality} are handled.
If the solution is accepted I'll continue with other files, taking
care to not break userspace. All the facilities are provided.
* Your feedback
[1] https://lkml.org/lkml/2013/8/26/354
[2] https://lkml.org/lkml/2013/8/31/209
Change log
----------
v1->v2:
- Removed the file->f_cred member that was added to seq_file struct.
Al Viro didn't like it, and Linus suggested to have a pointer on
'file struct', so it's done by using seq_file->private
- Added Acked-by: Kees Cook to
[PATCH v2 4/9] procfs: make /proc/*/{stack,syscall} 0400
- Added suggested-by: Eric W. Biederman to
[PATCH v2 1/9] procfs: add proc_same_open_cred() to check if the cred
have changed
[PATCH v2 2/9] procfs: add proc_allow_access() to check if file's
opener may access task
[PATCH v2 3/9] procfs: Document the proposed solution to protect
procfs entries
- Patchset cleaned
Version 1 was discussed here:
https://lkml.org/lkml/2013/9/25/459
The following series tries to implement what I describe.
Djalal Harouni (9):
procfs: add proc_same_open_cred() to check if the cred have changed
procfs: add proc_allow_access() to check if file's opener may access task
procfs: Document the proposed solution to protect procfs entries
procfs: make /proc/*/{stack,syscall} 0400
procfs: make /proc entries that use seq files able to access file->f_cred
procfs: add permission checks on the file's opener of /proc/*/stat
procfs: add permission checks on the file's opener of /proc/*/personality
procfs: improve permission checks on /proc/*/stack
procfs: improve permission checks on /proc/*/syscall
fs/proc/array.c | 16 ++++++-
fs/proc/base.c | 301 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++-----------
fs/proc/internal.h | 3 ++
3 files changed, 292 insertions(+), 28 deletions(-)
Since /proc entries varies at runtime, permission checks need to
happen during each system call.
However even with that /proc file descriptors can be passed to a more
privileged process (e.g. a suid-exec) which will pass the classic
ptrace_may_access() permission check. The open() call will be issued in
general by an unprivileged process while the disclosure of sensitive
/proc information will happen using a more privileged process at
read(), write()...
The /proc need to know if the cred of the original opener matches the
cred of current in order to take the appropriate actions.
The match concerns the cred fields that are used in the
ptrace_may_access() check: uid, gid and cap_permitted.
If there is a match then the current task that tries to read/write
/proc entries has the same privileges as the task that issued the
open() call.
However if the match fails then the cred have changed which means that
perhaps we have gain or lost the privileges of processing the /proc
file descriptor. So add proc_same_open_cred() to check if the cred have
changed.
Cc: Kees Cook <[email protected]>
Suggested-by: Eric W. Biederman <[email protected]>
Signed-off-by: Djalal Harouni <[email protected]>
---
fs/proc/base.c | 29 +++++++++++++++++++++++++++++
fs/proc/internal.h | 1 +
2 files changed, 30 insertions(+)
diff --git a/fs/proc/base.c b/fs/proc/base.c
index 1485e38..e834946 100644
--- a/fs/proc/base.c
+++ b/fs/proc/base.c
@@ -139,6 +139,35 @@ struct pid_entry {
NULL, &proc_single_file_operations, \
{ .proc_show = show } )
+/**
+ * proc_same_open_cred - Check if the file's opener cred matches the
+ * current cred.
+ * @fcred: The file's opener cred (file->f_cred)
+ *
+ * Return 1 if the cred of the file's opener matches the cred of
+ * current, otherwise 0.
+ *
+ * The match concerns the cred that are used in the ptrace_may_access()
+ * check to allow /proc task access. These cred are: uid,gid and
+ * cap_permitted.
+ *
+ * This function can be used to check if /proc file descriptors where
+ * passed to a more privileged process (e.g. execs a suid executable).
+ */
+int proc_same_open_cred(const struct cred *fcred)
+{
+ const struct cred *cred = current_cred();
+ const struct user_namespace *set_ns = fcred->user_ns;
+ const struct user_namespace *subset_ns = cred->user_ns;
+
+ if (set_ns != subset_ns)
+ return 0;
+
+ return (uid_eq(fcred->uid, cred->uid) &&
+ gid_eq(fcred->gid, cred->gid) &&
+ cap_issubset(cred->cap_permitted, fcred->cap_permitted));
+}
+
/*
* Count the number of hardlinks for the pid_entry table, excluding the .
* and .. links.
diff --git a/fs/proc/internal.h b/fs/proc/internal.h
index 651d09a..e2459f4 100644
--- a/fs/proc/internal.h
+++ b/fs/proc/internal.h
@@ -159,6 +159,7 @@ extern int proc_pid_statm(struct seq_file *, struct pid_namespace *,
/*
* base.c
*/
+extern int proc_same_open_cred(const struct cred *);
extern const struct dentry_operations pid_dentry_operations;
extern int pid_getattr(struct vfsmount *, struct dentry *, struct kstat *);
extern int proc_setattr(struct dentry *, struct iattr *);
--
1.7.11.7
Since /proc entries varies at runtime, permission checks need to happen
during each system call.
However even with that /proc file descriptors can be passed to a more
privileged process (e.g. a suid-exec) which will pass the classic
ptrace_may_access() permission check. The open() call will be issued in
general by an unprivileged process while the disclosure of sensitive
/proc information will happen using a more privileged process at
read(),write()...
Therfore we need a more sophisticated check to detect if the cred of the
process have changed, and if the cred of the original opener that are
stored in the file->f_cred have enough permission to access the task's
/proc entries during read(), write()...
Add the proc_allow_access() function that will receive the file->f_cred
as an argument, and tries to check if the opener had enough permission
to access the task's /proc entries.
This function should be used with the ptrace_may_access() check.
Cc: Kees Cook <[email protected]>
Suggested-by: Eric W. Biederman <[email protected]>
Signed-off-by: Djalal Harouni <[email protected]>
---
fs/proc/base.c | 56 ++++++++++++++++++++++++++++++++++++++++++++++++++++++
fs/proc/internal.h | 2 ++
2 files changed, 58 insertions(+)
diff --git a/fs/proc/base.c b/fs/proc/base.c
index e834946..c29eeae 100644
--- a/fs/proc/base.c
+++ b/fs/proc/base.c
@@ -168,6 +168,62 @@ int proc_same_open_cred(const struct cred *fcred)
cap_issubset(cred->cap_permitted, fcred->cap_permitted));
}
+/* Returns 0 on success, -errno on denial. */
+static int __proc_allow_access(const struct cred *cred,
+ struct task_struct *task, unsigned int mode)
+{
+ int ret = 0;
+ const struct cred *tcred;
+ const struct cred *fcred = cred;
+
+ rcu_read_lock();
+ tcred = __task_cred(task);
+ if (uid_eq(fcred->uid, tcred->euid) &&
+ uid_eq(fcred->uid, tcred->suid) &&
+ uid_eq(fcred->uid, tcred->uid) &&
+ gid_eq(fcred->gid, tcred->egid) &&
+ gid_eq(fcred->gid, tcred->sgid) &&
+ gid_eq(fcred->gid, tcred->gid))
+ goto out;
+
+ if (mode & PTRACE_MODE_NOAUDIT)
+ ret = security_capable_noaudit(fcred, tcred->user_ns,
+ CAP_SYS_PTRACE);
+ else
+ ret = security_capable(fcred, tcred->user_ns,
+ CAP_SYS_PTRACE);
+
+out:
+ rcu_read_unlock();
+ return !ret ? ret : -EPERM;
+}
+
+/**
+ * proc_allow_access - Check if the file's opener had enough permissions
+ * to access the target process.
+ * @fcred: The file's opener cred (file->f_cred)
+ * @task: The target task we want to inspect
+ * @mode: The ptrace mode
+ *
+ * Return a non-zero if the file's opener had enough permissions to
+ * access the task's /proc entries.
+ *
+ * Since this function will check the permissions of the opener
+ * against the target task, it can be used to protect /proc files
+ * from opening a /proc file descriptor and do a suid-exec.
+ *
+ * Callers must hold the task->signal->cred_guard_mutex
+ */
+int proc_allow_access(const struct cred *fcred,
+ struct task_struct *task, unsigned int mode)
+{
+ int ret;
+ task_lock(task);
+ ret = __proc_allow_access(fcred, task, mode);
+ task_unlock(task);
+ return !ret;
+}
+
/*
* Count the number of hardlinks for the pid_entry table, excluding the .
* and .. links.
diff --git a/fs/proc/internal.h b/fs/proc/internal.h
index e2459f4..c3f3c34 100644
--- a/fs/proc/internal.h
+++ b/fs/proc/internal.h
@@ -159,6 +159,8 @@ extern int proc_pid_statm(struct seq_file *, struct pid_namespace *,
/*
* base.c
*/
+extern int proc_allow_access(const struct cred *,
+ struct task_struct *, unsigned int);
extern int proc_same_open_cred(const struct cred *);
extern const struct dentry_operations pid_dentry_operations;
extern int pid_getattr(struct vfsmount *, struct dentry *, struct kstat *);
--
1.7.11.7
Note the proposed solution to protect sensitive procfs entries as
code comment.
Cc: Kees Cook <[email protected]>
Suggested-by: Eric W. Biederman <[email protected]>
Signed-off-by: Djalal Harouni <[email protected]>
---
fs/proc/base.c | 11 +++++++++++
1 file changed, 11 insertions(+)
diff --git a/fs/proc/base.c b/fs/proc/base.c
index c29eeae..8d21316 100644
--- a/fs/proc/base.c
+++ b/fs/proc/base.c
@@ -102,6 +102,17 @@
*
* The classic example of a problem is opening file descriptors
* in /proc for a task before it execs a suid executable.
+ *
+ * Solution for sensitive files:
+ * At each system call: open(),read(),write()... Perform the
+ * ptrace_may_access() check.
+ *
+ * After open() and during each system call: read(),write()...
+ * If the cred of current have changed then perform the
+ * proc_allow_access() check after the ptrace_may_access() one.
+ *
+ * This way we can determine if current has gained more privileges
+ * by execs a suid executable.
*/
struct pid_entry {
--
1.7.11.7
The /proc/*/{stack,syscall} contain sensitive information and currently
its mode is 0444. Change this to 0400 so the VFS will be able to block
unprivileged processes from getting file descriptors on arbitrary
privileged /proc/*/{stack,syscall} files.
This will also avoid doing extra unnecessary ptrace checks.
Cc: Eric W. Biederman <[email protected]>
Acked-by: Kees Cook <[email protected]>
Signed-off-by: Djalal Harouni <[email protected]>
---
fs/proc/base.c | 8 ++++----
1 file changed, 4 insertions(+), 4 deletions(-)
diff --git a/fs/proc/base.c b/fs/proc/base.c
index 8d21316..54e926a 100644
--- a/fs/proc/base.c
+++ b/fs/proc/base.c
@@ -2682,7 +2682,7 @@ static const struct pid_entry tgid_base_stuff[] = {
#endif
REG("comm", S_IRUGO|S_IWUSR, proc_pid_set_comm_operations),
#ifdef CONFIG_HAVE_ARCH_TRACEHOOK
- INF("syscall", S_IRUGO, proc_pid_syscall),
+ INF("syscall", S_IRUSR, proc_pid_syscall),
#endif
INF("cmdline", S_IRUGO, proc_pid_cmdline),
ONE("stat", S_IRUGO, proc_tgid_stat),
@@ -2710,7 +2710,7 @@ static const struct pid_entry tgid_base_stuff[] = {
INF("wchan", S_IRUGO, proc_pid_wchan),
#endif
#ifdef CONFIG_STACKTRACE
- ONE("stack", S_IRUGO, proc_pid_stack),
+ ONE("stack", S_IRUSR, proc_pid_stack),
#endif
#ifdef CONFIG_SCHEDSTATS
INF("schedstat", S_IRUGO, proc_pid_schedstat),
@@ -3018,7 +3018,7 @@ static const struct pid_entry tid_base_stuff[] = {
#endif
REG("comm", S_IRUGO|S_IWUSR, proc_pid_set_comm_operations),
#ifdef CONFIG_HAVE_ARCH_TRACEHOOK
- INF("syscall", S_IRUGO, proc_pid_syscall),
+ INF("syscall", S_IRUSR, proc_pid_syscall),
#endif
INF("cmdline", S_IRUGO, proc_pid_cmdline),
ONE("stat", S_IRUGO, proc_tid_stat),
@@ -3048,7 +3048,7 @@ static const struct pid_entry tid_base_stuff[] = {
INF("wchan", S_IRUGO, proc_pid_wchan),
#endif
#ifdef CONFIG_STACKTRACE
- ONE("stack", S_IRUGO, proc_pid_stack),
+ ONE("stack", S_IRUSR, proc_pid_stack),
#endif
#ifdef CONFIG_SCHEDSTATS
INF("schedstat", S_IRUGO, proc_pid_schedstat),
--
1.7.11.7
In order to check if the cred of current have changed between ->open()
and ->read(), we must able to access the file's opener cred
'file->f_cred' at any point.
To make this possible for /proc/*/{stack,personality,stat} pass the
'file struct' as a 3rd argument to single_open() so it will be stored in
seq_file->private in seq_open(). This way these entries are able to
continue to use seq files, and access the file->f_cred easily.
This is also a preparation for the following patches which will check
the corresponding file->f_cred.
Signed-off-by: Djalal Harouni <[email protected]>
---
fs/proc/base.c | 7 +++++--
1 file changed, 5 insertions(+), 2 deletions(-)
diff --git a/fs/proc/base.c b/fs/proc/base.c
index 54e926a..d4b604d 100644
--- a/fs/proc/base.c
+++ b/fs/proc/base.c
@@ -765,7 +765,8 @@ static const struct file_operations proc_info_file_operations = {
static int proc_single_show(struct seq_file *m, void *v)
{
- struct inode *inode = m->private;
+ struct file *filp = m->private;
+ struct inode *inode = file_inode(filp);
struct pid_namespace *ns;
struct pid *pid;
struct task_struct *task;
@@ -785,7 +786,9 @@ static int proc_single_show(struct seq_file *m, void *v)
static int proc_single_open(struct inode *inode, struct file *filp)
{
- return single_open(filp, proc_single_show, inode);
+ /* Later we need inode and filp->f_cred, so pass filp as 3rd
+ * argument, it will be referenced by seq_file->private */
+ return single_open(filp, proc_single_show, filp);
}
static const struct file_operations proc_single_file_operations = {
--
1.7.11.7
Some fields of the /proc/*/stat are sensitive fields that need
appropriate protection.
However, /proc file descriptors can be passed to a more privileged
process (e.g. a suid-exec) which will pass the classic
ptrace_may_access() permission check during read().
To prevent it, use proc_same_open_cred() to detect if current's cred
have changed between ->open() and ->read(), if so, call
proc_allow_access() to check if the original file's opener had enough
permissions to read these sensitive fields. This will prevent passing
file descriptors to a more privileged process to leak data.
The patch also adds a previously missing signal->cred_guard_mutex lock.
This patch does not break userspace since it only hides the fields that
were supposed to be protected.
Cc: Kees Cook <[email protected]>
Cc: Eric W. Biederman <[email protected]>
Signed-off-by: Djalal Harouni <[email protected]>
---
fs/proc/array.c | 16 ++++++++++++++--
1 file changed, 14 insertions(+), 2 deletions(-)
diff --git a/fs/proc/array.c b/fs/proc/array.c
index cbd0f1b..f034e05 100644
--- a/fs/proc/array.c
+++ b/fs/proc/array.c
@@ -394,7 +394,7 @@ static int do_task_stat(struct seq_file *m, struct pid_namespace *ns,
char state;
pid_t ppid = 0, pgid = -1, sid = -1;
int num_threads = 0;
- int permitted;
+ int permitted = 0;
struct mm_struct *mm;
unsigned long long start_time;
unsigned long cmin_flt = 0, cmaj_flt = 0;
@@ -404,10 +404,22 @@ static int do_task_stat(struct seq_file *m, struct pid_namespace *ns,
unsigned long rsslim = 0;
char tcomm[sizeof(task->comm)];
unsigned long flags;
+ struct file *file = m->private;
+ int same_cred = proc_same_open_cred(file->f_cred);
+ unsigned int ptrace_mode = PTRACE_MODE_READ | PTRACE_MODE_NOAUDIT;
state = *get_task_state(task);
vsize = eip = esp = 0;
- permitted = ptrace_may_access(task, PTRACE_MODE_READ | PTRACE_MODE_NOAUDIT);
+
+ if (!mutex_lock_killable(&task->signal->cred_guard_mutex)) {
+ permitted = ptrace_may_access(task, ptrace_mode);
+ if (permitted && !same_cred)
+ permitted = proc_allow_access(file->f_cred,
+ task, ptrace_mode);
+
+ mutex_unlock(&task->signal->cred_guard_mutex);
+ }
+
mm = get_task_mm(task);
if (mm) {
vsize = task_vsize(mm);
--
1.7.11.7
If current's cred have changed between ->open() and ->read(), then call
proc_allow_access() to check if the original file's opener had enough
permissions to access the /proc/*/personality entry during ->read().
Cc: Kees Cook <[email protected]>
Cc: Eric W. Biederman <[email protected]>
Signed-off-by: Djalal Harouni <[email protected]>
---
fs/proc/base.c | 18 +++++++++++++++---
1 file changed, 15 insertions(+), 3 deletions(-)
diff --git a/fs/proc/base.c b/fs/proc/base.c
index d4b604d..77f5b84 100644
--- a/fs/proc/base.c
+++ b/fs/proc/base.c
@@ -2647,11 +2647,23 @@ static const struct file_operations proc_projid_map_operations = {
static int proc_pid_personality(struct seq_file *m, struct pid_namespace *ns,
struct pid *pid, struct task_struct *task)
{
+ struct file *file = m->private;
+ const struct cred *fcred = file->f_cred;
+ int same_cred = proc_same_open_cred(fcred);
int err = lock_trace(task);
- if (!err) {
- seq_printf(m, "%08x\n", task->personality);
- unlock_trace(task);
+ if (err)
+ return err;
+
+ if (!same_cred &&
+ !proc_allow_access(fcred, task, PTRACE_MODE_ATTACH)) {
+ err = -EPERM;
+ goto out;
}
+
+ seq_printf(m, "%08x\n", task->personality);
+
+out:
+ unlock_trace(task);
return err;
}
--
1.7.11.7
Permission checks need to happen during each system call. Therefore we
need to convert the /proc/*/stack entry from a ONE node to a REG node.
Doing this will make /proc/*/stack have its own file operations to
implement appropriate checks and avoid breaking shared ONE file
operations.
The patch makes sure that /proc/*/stack is still using seq files to
provide its output.
The patch adds stack_open() to check if the file's opener has enough
permission to ptrace the task during ->open().
However, even with this, /proc file descriptors can be passed to a more
privileged process (e.g. a suid-exec) which will pass the classic
ptrace_may_access() permission check during read().
To prevent this, use proc_same_open_cred() to detect if the cred of
current have changed between ->open() and ->read(), if so, then call
proc_allow_access() to check if the original file's opener had enough
privileges to access the /proc's task entries during ->read(). This will
block passing file descriptors to a more privileged process.
If the cred did not change then continue with read().
For readability, split code into another task_stack_show() function
which is used to get the stack trace of a task.
Cc: Kees Cook <[email protected]>
Cc: Eric W. Biederman <[email protected]>
Signed-off-by: Djalal Harouni <[email protected]>
---
fs/proc/base.c | 87 ++++++++++++++++++++++++++++++++++++++++++++++++++--------
1 file changed, 75 insertions(+), 12 deletions(-)
diff --git a/fs/proc/base.c b/fs/proc/base.c
index 77f5b84..b80588a 100644
--- a/fs/proc/base.c
+++ b/fs/proc/base.c
@@ -395,13 +395,14 @@ static void unlock_trace(struct task_struct *task)
#define MAX_STACK_TRACE_DEPTH 64
-static int proc_pid_stack(struct seq_file *m, struct pid_namespace *ns,
- struct pid *pid, struct task_struct *task)
+static int task_stack_show(struct seq_file *m, struct task_struct *task)
{
- struct stack_trace trace;
- unsigned long *entries;
int err;
int i;
+ int same_cred;
+ struct stack_trace trace;
+ unsigned long *entries;
+ struct file *filp = m->private;
entries = kmalloc(MAX_STACK_TRACE_DEPTH * sizeof(*entries), GFP_KERNEL);
if (!entries)
@@ -412,20 +413,82 @@ static int proc_pid_stack(struct seq_file *m, struct pid_namespace *ns,
trace.entries = entries;
trace.skip = 0;
+ same_cred = proc_same_open_cred(filp->f_cred);
+
err = lock_trace(task);
- if (!err) {
- save_stack_trace_tsk(task, &trace);
+ if (err)
+ goto free;
- for (i = 0; i < trace.nr_entries; i++) {
- seq_printf(m, "[<%pK>] %pS\n",
- (void *)entries[i], (void *)entries[i]);
- }
+ if (!same_cred &&
+ !proc_allow_access(filp->f_cred, task, PTRACE_MODE_ATTACH)) {
+ err = -EPERM;
unlock_trace(task);
+ goto free;
+ }
+
+ save_stack_trace_tsk(task, &trace);
+ unlock_trace(task);
+
+ for (i = 0; i < trace.nr_entries; i++) {
+ seq_printf(m, "[<%pK>] %pS\n",
+ (void *)entries[i], (void *)entries[i]);
}
+
+free:
kfree(entries);
+ return err;
+}
+static int stack_show(struct seq_file *m, void *v)
+{
+ int ret;
+ struct pid *pid;
+ struct task_struct *task;
+ struct file *filp = m->private;
+ struct inode *inode = file_inode(filp);
+
+ ret = -ESRCH;
+ pid = proc_pid(inode);
+ task = get_pid_task(pid, PIDTYPE_PID);
+ if (!task)
+ return ret;
+
+ ret = task_stack_show(m, task);
+
+ put_task_struct(task);
+ return ret;
+}
+
+static int stack_open(struct inode *inode, struct file *filp)
+{
+ int err = -ESRCH;
+ struct task_struct *task = get_proc_task(file_inode(filp));
+
+ if (!task)
+ return err;
+
+ err = mutex_lock_killable(&task->signal->cred_guard_mutex);
+ if (err)
+ goto out;
+
+ err = -EPERM;
+ if (ptrace_may_access(task, PTRACE_MODE_ATTACH))
+ /* We need inode and filp->f_cred, so pass filp
+ * as third argument */
+ err = single_open(filp, stack_show, filp);
+
+ mutex_unlock(&task->signal->cred_guard_mutex);
+out:
+ put_task_struct(task);
return err;
}
+
+static const struct file_operations proc_pid_stack_operations = {
+ .open = stack_open,
+ .read = seq_read,
+ .llseek = seq_lseek,
+ .release = single_release,
+};
#endif
#ifdef CONFIG_SCHEDSTATS
@@ -2725,7 +2788,7 @@ static const struct pid_entry tgid_base_stuff[] = {
INF("wchan", S_IRUGO, proc_pid_wchan),
#endif
#ifdef CONFIG_STACKTRACE
- ONE("stack", S_IRUSR, proc_pid_stack),
+ REG("stack", S_IRUSR, proc_pid_stack_operations),
#endif
#ifdef CONFIG_SCHEDSTATS
INF("schedstat", S_IRUGO, proc_pid_schedstat),
@@ -3063,7 +3126,7 @@ static const struct pid_entry tid_base_stuff[] = {
INF("wchan", S_IRUGO, proc_pid_wchan),
#endif
#ifdef CONFIG_STACKTRACE
- ONE("stack", S_IRUSR, proc_pid_stack),
+ REG("stack", S_IRUSR, proc_pid_stack_operations),
#endif
#ifdef CONFIG_SCHEDSTATS
INF("schedstat", S_IRUGO, proc_pid_schedstat),
--
1.7.11.7
Permission checks need to happen during each system call. Therefore we
need to convert the /proc/*/syscall entry from an INF node to a REG
node. Doing this will make /proc/*/syscall have its own file operations
to implement appropriate checks and avoid breaking shared INF file
operations.
Add the syscall_open() to check if the file's opener has enough
permission to ptrace the task during ->open().
Add the syscall_read() to check permissions and to read task syscall
information. If the classic ptrace_may_access() check is passed, then
check if current's cred have changed between ->open() and ->read(), if
so, call proc_allow_access() to check if the original file's opener had
enough permissions to read the task syscall info. This will block
passing the file descriptor to a more privileged process.
For readability split code into another task_syscall_read() function
which is used to get the syscall entries of the task.
Cc: Kees Cook <[email protected]>
Cc: Eric W. Biederman <[email protected]>
Signed-off-by: Djalal Harouni <[email protected]>
---
fs/proc/base.c | 93 ++++++++++++++++++++++++++++++++++++++++++++++++++++------
1 file changed, 84 insertions(+), 9 deletions(-)
diff --git a/fs/proc/base.c b/fs/proc/base.c
index b80588a..f98889d 100644
--- a/fs/proc/base.c
+++ b/fs/proc/base.c
@@ -150,6 +150,9 @@ struct pid_entry {
NULL, &proc_single_file_operations, \
{ .proc_show = show } )
+/* 4K page size but our output routines use some slack for overruns */
+#define PROC_BLOCK_SIZE (3*1024)
+
/**
* proc_same_open_cred - Check if the file's opener cred matches the
* current cred.
@@ -678,13 +681,32 @@ static int proc_pid_limits(struct task_struct *task, char *buffer)
}
#ifdef CONFIG_HAVE_ARCH_TRACEHOOK
-static int proc_pid_syscall(struct task_struct *task, char *buffer)
+static int syscall_open(struct inode *inode, struct file *filp)
+{
+ int err = -ESRCH;
+ struct task_struct *task = get_proc_task(file_inode(filp));
+
+ if (!task)
+ return err;
+
+ err = mutex_lock_killable(&task->signal->cred_guard_mutex);
+ if (err)
+ goto out;
+
+ if (!ptrace_may_access(task, PTRACE_MODE_ATTACH))
+ err = -EPERM;
+
+ mutex_unlock(&task->signal->cred_guard_mutex);
+out:
+ put_task_struct(task);
+ return err;
+}
+
+static int task_syscall_read(struct task_struct *task, char *buffer)
{
+ int res;
long nr;
unsigned long args[6], sp, pc;
- int res = lock_trace(task);
- if (res)
- return res;
if (task_current_syscall(task, &nr, args, 6, &sp, &pc))
res = sprintf(buffer, "running\n");
@@ -696,9 +718,64 @@ static int proc_pid_syscall(struct task_struct *task, char *buffer)
nr,
args[0], args[1], args[2], args[3], args[4], args[5],
sp, pc);
- unlock_trace(task);
+
return res;
}
+
+static ssize_t syscall_read(struct file *file, char __user *buf,
+ size_t count, loff_t *ppos)
+{
+ int same_cred;
+ ssize_t length;
+ unsigned long page;
+ const struct cred *fcred = file->f_cred;
+ struct inode *inode = file_inode(file);
+ struct task_struct *task = get_proc_task(inode);
+
+ length = -ESRCH;
+ if (!task)
+ return length;
+
+ if (count > PROC_BLOCK_SIZE)
+ count = PROC_BLOCK_SIZE;
+
+ length = -ENOMEM;
+ page = __get_free_page(GFP_TEMPORARY);
+ if (!page)
+ goto out;
+
+ same_cred = proc_same_open_cred(fcred);
+
+ length = lock_trace(task);
+ if (length)
+ goto out_free;
+
+ if (!same_cred &&
+ !proc_allow_access(fcred, task, PTRACE_MODE_ATTACH)) {
+ length = -EPERM;
+ unlock_trace(task);
+ goto out_free;
+ }
+
+ length = task_syscall_read(task, (char *)page);
+ unlock_trace(task);
+
+ if (length >= 0)
+ length = simple_read_from_buffer(buf, count, ppos,
+ (char *)page, length);
+
+out_free:
+ free_page(page);
+out:
+ put_task_struct(task);
+ return length;
+}
+
+static const struct file_operations proc_pid_syscall_operations = {
+ .open = syscall_open,
+ .read = syscall_read,
+ .llseek = generic_file_llseek,
+};
#endif /* CONFIG_HAVE_ARCH_TRACEHOOK */
/************************************************************************/
@@ -789,8 +866,6 @@ static const struct inode_operations proc_def_inode_operations = {
.setattr = proc_setattr,
};
-#define PROC_BLOCK_SIZE (3*1024) /* 4K page size but our output routines use some slack for overruns */
-
static ssize_t proc_info_read(struct file * file, char __user * buf,
size_t count, loff_t *ppos)
{
@@ -2760,7 +2835,7 @@ static const struct pid_entry tgid_base_stuff[] = {
#endif
REG("comm", S_IRUGO|S_IWUSR, proc_pid_set_comm_operations),
#ifdef CONFIG_HAVE_ARCH_TRACEHOOK
- INF("syscall", S_IRUSR, proc_pid_syscall),
+ REG("syscall", S_IRUSR, proc_pid_syscall_operations),
#endif
INF("cmdline", S_IRUGO, proc_pid_cmdline),
ONE("stat", S_IRUGO, proc_tgid_stat),
@@ -3096,7 +3171,7 @@ static const struct pid_entry tid_base_stuff[] = {
#endif
REG("comm", S_IRUGO|S_IWUSR, proc_pid_set_comm_operations),
#ifdef CONFIG_HAVE_ARCH_TRACEHOOK
- INF("syscall", S_IRUSR, proc_pid_syscall),
+ REG("syscall", S_IRUSR, proc_pid_syscall_operations),
#endif
INF("cmdline", S_IRUGO, proc_pid_cmdline),
ONE("stat", S_IRUGO, proc_tid_stat),
--
1.7.11.7
On 10/01/2013 01:26 PM, Djalal Harouni wrote:
> Since /proc entries varies at runtime, permission checks need to happen
> during each system call.
>
> However even with that /proc file descriptors can be passed to a more
> privileged process (e.g. a suid-exec) which will pass the classic
> ptrace_may_access() permission check. The open() call will be issued in
> general by an unprivileged process while the disclosure of sensitive
> /proc information will happen using a more privileged process at
> read(),write()...
>
> Therfore we need a more sophisticated check to detect if the cred of the
> process have changed, and if the cred of the original opener that are
> stored in the file->f_cred have enough permission to access the task's
> /proc entries during read(), write()...
>
> Add the proc_allow_access() function that will receive the file->f_cred
> as an argument, and tries to check if the opener had enough permission
> to access the task's /proc entries.
>
> This function should be used with the ptrace_may_access() check.
>
> Cc: Kees Cook <[email protected]>
> Suggested-by: Eric W. Biederman <[email protected]>
> Signed-off-by: Djalal Harouni <[email protected]>
> ---
> fs/proc/base.c | 56 ++++++++++++++++++++++++++++++++++++++++++++++++++++++
> fs/proc/internal.h | 2 ++
> 2 files changed, 58 insertions(+)
>
> diff --git a/fs/proc/base.c b/fs/proc/base.c
> index e834946..c29eeae 100644
> --- a/fs/proc/base.c
> +++ b/fs/proc/base.c
> @@ -168,6 +168,62 @@ int proc_same_open_cred(const struct cred *fcred)
> cap_issubset(cred->cap_permitted, fcred->cap_permitted));
> }
>
> +/* Returns 0 on success, -errno on denial. */
> +static int __proc_allow_access(const struct cred *cred,
> + struct task_struct *task, unsigned int mode)
> +{
> + int ret = 0;
> + const struct cred *tcred;
> + const struct cred *fcred = cred;
> +
> + rcu_read_lock();
> + tcred = __task_cred(task);
> + if (uid_eq(fcred->uid, tcred->euid) &&
> + uid_eq(fcred->uid, tcred->suid) &&
> + uid_eq(fcred->uid, tcred->uid) &&
> + gid_eq(fcred->gid, tcred->egid) &&
> + gid_eq(fcred->gid, tcred->sgid) &&
> + gid_eq(fcred->gid, tcred->gid))
> + goto out;
> +
What's this for? Is it supposed to be an optimization? If so, it looks
potentially exploitable, although I don't really understand what you're
trying to do.
--Andy
On 10/01/2013 01:26 PM, Djalal Harouni wrote:
> Some fields of the /proc/*/stat are sensitive fields that need
> appropriate protection.
>
> However, /proc file descriptors can be passed to a more privileged
> process (e.g. a suid-exec) which will pass the classic
> ptrace_may_access() permission check during read().
>
> To prevent it, use proc_same_open_cred() to detect if current's cred
> have changed between ->open() and ->read(), if so, call
> proc_allow_access() to check if the original file's opener had enough
> permissions to read these sensitive fields. This will prevent passing
> file descriptors to a more privileged process to leak data.
>
> The patch also adds a previously missing signal->cred_guard_mutex lock.
>
> This patch does not break userspace since it only hides the fields that
> were supposed to be protected.
>
> Cc: Kees Cook <[email protected]>
> Cc: Eric W. Biederman <[email protected]>
> Signed-off-by: Djalal Harouni <[email protected]>
> ---
> fs/proc/array.c | 16 ++++++++++++++--
> 1 file changed, 14 insertions(+), 2 deletions(-)
>
> diff --git a/fs/proc/array.c b/fs/proc/array.c
> index cbd0f1b..f034e05 100644
> --- a/fs/proc/array.c
> +++ b/fs/proc/array.c
> @@ -394,7 +394,7 @@ static int do_task_stat(struct seq_file *m, struct pid_namespace *ns,
> char state;
> pid_t ppid = 0, pgid = -1, sid = -1;
> int num_threads = 0;
> - int permitted;
> + int permitted = 0;
> struct mm_struct *mm;
> unsigned long long start_time;
> unsigned long cmin_flt = 0, cmaj_flt = 0;
> @@ -404,10 +404,22 @@ static int do_task_stat(struct seq_file *m, struct pid_namespace *ns,
> unsigned long rsslim = 0;
> char tcomm[sizeof(task->comm)];
> unsigned long flags;
> + struct file *file = m->private;
> + int same_cred = proc_same_open_cred(file->f_cred);
> + unsigned int ptrace_mode = PTRACE_MODE_READ | PTRACE_MODE_NOAUDIT;
>
> state = *get_task_state(task);
> vsize = eip = esp = 0;
> - permitted = ptrace_may_access(task, PTRACE_MODE_READ | PTRACE_MODE_NOAUDIT);
> +
> + if (!mutex_lock_killable(&task->signal->cred_guard_mutex)) {
> + permitted = ptrace_may_access(task, ptrace_mode);
> + if (permitted && !same_cred)
> + permitted = proc_allow_access(file->f_cred,
> + task, ptrace_mode);
> +
> + mutex_unlock(&task->signal->cred_guard_mutex);
> + }
> +
else permitted = false?
But surely this would be *much* more comprehensible if you had
proc_allow_access do the entire check.
--Andy
--Andy
On 10/01/2013 01:26 PM, Djalal Harouni wrote:
> /proc/<pid>/* entries varies at runtime, appropriate permission checks
> need to happen during each system call.
>
> Currently some of these sensitive entries are protected by performing
> the ptrace_may_access() check. However even with that the /proc file
> descriptors can be passed to a more privileged process
> (e.g. a suid-exec) which will pass the classic ptrace_may_access()
> check. In general the ->open() call will be issued by an unprivileged
> process while the ->read(),->write() calls by a more privileged one.
>
> Example of these files are:
> /proc/*/syscall, /proc/*/stack etc.
>
> And any open(/proc/self/*) then suid-exec to read()/write() /proc/self/*
>
>
> These files are protected during read() by the ptrace_may_access(),
> however the file descriptor can be passed to a suid-exec which can be
> used to read data and bypass ASLR. Of course this was discussed several
> times on LKML.
Can you elaborate on what it is that you're fixing? That is, can you
give a concrete example of what process opens what file and passes the
fd to what process?
I'm having trouble following your description.
--Andy
On Tue, Oct 01, 2013 at 06:40:41PM -0700, Andy Lutomirski wrote:
> On 10/01/2013 01:26 PM, Djalal Harouni wrote:
> > /proc/<pid>/* entries varies at runtime, appropriate permission checks
> > need to happen during each system call.
> >
> > Currently some of these sensitive entries are protected by performing
> > the ptrace_may_access() check. However even with that the /proc file
> > descriptors can be passed to a more privileged process
> > (e.g. a suid-exec) which will pass the classic ptrace_may_access()
> > check. In general the ->open() call will be issued by an unprivileged
> > process while the ->read(),->write() calls by a more privileged one.
> >
> > Example of these files are:
> > /proc/*/syscall, /proc/*/stack etc.
> >
> > And any open(/proc/self/*) then suid-exec to read()/write() /proc/self/*
> >
> >
> > These files are protected during read() by the ptrace_may_access(),
> > however the file descriptor can be passed to a suid-exec which can be
> > used to read data and bypass ASLR. Of course this was discussed several
> > times on LKML.
>
> Can you elaborate on what it is that you're fixing? That is, can you
> give a concrete example of what process opens what file and passes the
> fd to what process?
Yes, the references were already given in this email:
https://lkml.org/lkml/2013/8/31/209
This has been discussed several times on lkml:
https://lkml.org/lkml/2013/8/28/544
https://lkml.org/lkml/2013/8/28/564 (check Kees's references)
> I'm having trouble following your description.
Process open a /proc file and pass the fd to a more privilaged process
that will pass the ptrace_may_access() check, while the original process
that opened that file should fail at the ptrace_may_access()
> --Andy
>
--
Djalal Harouni
http://opendz.org
On Tue, Oct 01, 2013 at 06:36:34PM -0700, Andy Lutomirski wrote:
> On 10/01/2013 01:26 PM, Djalal Harouni wrote:
> > Since /proc entries varies at runtime, permission checks need to happen
> > during each system call.
> >
> > However even with that /proc file descriptors can be passed to a more
> > privileged process (e.g. a suid-exec) which will pass the classic
> > ptrace_may_access() permission check. The open() call will be issued in
> > general by an unprivileged process while the disclosure of sensitive
> > /proc information will happen using a more privileged process at
> > read(),write()...
> >
> > Therfore we need a more sophisticated check to detect if the cred of the
> > process have changed, and if the cred of the original opener that are
> > stored in the file->f_cred have enough permission to access the task's
> > /proc entries during read(), write()...
> >
> > Add the proc_allow_access() function that will receive the file->f_cred
> > as an argument, and tries to check if the opener had enough permission
> > to access the task's /proc entries.
> >
> > This function should be used with the ptrace_may_access() check.
> >
> > Cc: Kees Cook <[email protected]>
> > Suggested-by: Eric W. Biederman <[email protected]>
> > Signed-off-by: Djalal Harouni <[email protected]>
> > ---
> > fs/proc/base.c | 56 ++++++++++++++++++++++++++++++++++++++++++++++++++++++
> > fs/proc/internal.h | 2 ++
> > 2 files changed, 58 insertions(+)
> >
> > diff --git a/fs/proc/base.c b/fs/proc/base.c
> > index e834946..c29eeae 100644
> > --- a/fs/proc/base.c
> > +++ b/fs/proc/base.c
> > @@ -168,6 +168,62 @@ int proc_same_open_cred(const struct cred *fcred)
> > cap_issubset(cred->cap_permitted, fcred->cap_permitted));
> > }
> >
> > +/* Returns 0 on success, -errno on denial. */
> > +static int __proc_allow_access(const struct cred *cred,
> > + struct task_struct *task, unsigned int mode)
> > +{
> > + int ret = 0;
> > + const struct cred *tcred;
> > + const struct cred *fcred = cred;
> > +
> > + rcu_read_lock();
> > + tcred = __task_cred(task);
> > + if (uid_eq(fcred->uid, tcred->euid) &&
> > + uid_eq(fcred->uid, tcred->suid) &&
> > + uid_eq(fcred->uid, tcred->uid) &&
> > + gid_eq(fcred->gid, tcred->egid) &&
> > + gid_eq(fcred->gid, tcred->sgid) &&
> > + gid_eq(fcred->gid, tcred->gid))
> > + goto out;
> > +
>
> What's this for? Is it supposed to be an optimization? If so, it looks
> potentially exploitable, although I don't really understand what you're
> trying to do.
This function should be used in addition to the ptrace_may_access() one.
This will help to check if the original process that opened a
/proc/pid/* file is able to read(),write() to it.
As we have said, the file descriptor can be passed to a more privileged
process to pass the ptrace_may_access(). If we detect that current's
cred have changed between ->open() (unprivileged) and ->read()
(privileged, exec a suid-exec), then we call this helper function to
check the file's opener cred against the target task during this
->read(),->write()...
This should block vulnerabilities like the /proc/pid/mem
http://lwn.net/Articles/476947/
Since current will not have the same privileges of the process that
opened the file (cred have changed).
So:
This is not an optimization!
Can you elaborate more on the potentially exploitable please ?
It has the same logic of ptrace_may_access(). Didn't want to modifiy
ptrace_may_access() since it relies on current and its context and if we
do, we'll have to touch the capability checks also. So just add this one
here and call it only if current's cred change.
> --Andy
--
Djalal Harouni
http://opendz.org
On Tue, Oct 01, 2013 at 06:39:00PM -0700, Andy Lutomirski wrote:
> On 10/01/2013 01:26 PM, Djalal Harouni wrote:
> > Some fields of the /proc/*/stat are sensitive fields that need
> > appropriate protection.
> >
> > However, /proc file descriptors can be passed to a more privileged
> > process (e.g. a suid-exec) which will pass the classic
> > ptrace_may_access() permission check during read().
> >
> > To prevent it, use proc_same_open_cred() to detect if current's cred
> > have changed between ->open() and ->read(), if so, call
> > proc_allow_access() to check if the original file's opener had enough
> > permissions to read these sensitive fields. This will prevent passing
> > file descriptors to a more privileged process to leak data.
> >
> > The patch also adds a previously missing signal->cred_guard_mutex lock.
> >
> > This patch does not break userspace since it only hides the fields that
> > were supposed to be protected.
> >
> > Cc: Kees Cook <[email protected]>
> > Cc: Eric W. Biederman <[email protected]>
> > Signed-off-by: Djalal Harouni <[email protected]>
> > ---
> > fs/proc/array.c | 16 ++++++++++++++--
> > 1 file changed, 14 insertions(+), 2 deletions(-)
> >
> > diff --git a/fs/proc/array.c b/fs/proc/array.c
> > index cbd0f1b..f034e05 100644
> > --- a/fs/proc/array.c
> > +++ b/fs/proc/array.c
> > @@ -394,7 +394,7 @@ static int do_task_stat(struct seq_file *m, struct pid_namespace *ns,
> > char state;
> > pid_t ppid = 0, pgid = -1, sid = -1;
> > int num_threads = 0;
> > - int permitted;
> > + int permitted = 0;
> > struct mm_struct *mm;
> > unsigned long long start_time;
> > unsigned long cmin_flt = 0, cmaj_flt = 0;
> > @@ -404,10 +404,22 @@ static int do_task_stat(struct seq_file *m, struct pid_namespace *ns,
> > unsigned long rsslim = 0;
> > char tcomm[sizeof(task->comm)];
> > unsigned long flags;
> > + struct file *file = m->private;
> > + int same_cred = proc_same_open_cred(file->f_cred);
> > + unsigned int ptrace_mode = PTRACE_MODE_READ | PTRACE_MODE_NOAUDIT;
> >
> > state = *get_task_state(task);
> > vsize = eip = esp = 0;
> > - permitted = ptrace_may_access(task, PTRACE_MODE_READ | PTRACE_MODE_NOAUDIT);
> > +
> > + if (!mutex_lock_killable(&task->signal->cred_guard_mutex)) {
> > + permitted = ptrace_may_access(task, ptrace_mode);
> > + if (permitted && !same_cred)
> > + permitted = proc_allow_access(file->f_cred,
> > + task, ptrace_mode);
> > +
> > + mutex_unlock(&task->signal->cred_guard_mutex);
> > + }
> > +
>
> else permitted = false?
permitted is initialized to 0
First the original ptrace_may_access() check did not hold
cred_guard_mutex, so add it. If we can't grab mutex then let permitted
to be zero. Yes this a change in behaviour and I think it's correct, IOW
we were not able to perform the ptrace_may_access() check, otherwise
permitted will depend on checks result.
However, there is still a race here since we set the permitted value
before gathering the appropriate info about task. At the read() data moment
this target task may have been gone privileged... , acquiring an X lock
on target task, will just break/slow things, as it has been shown before...
Not to mention that the race window is small...
> But surely this would be *much* more comprehensible if you had
> proc_allow_access do the entire check.
I don't understand what you mean by "do the entire check" ?
the /proc/pid/stat file is used by "ps", "top" ... it's a vital file
We don't want to break it, otherwise "ps" will hide processes. So just
do the ptrace_may_access() check correctly with the help of
proc_allow_access() and set the permitted variable only. The patch did
not touch /proc/pid/stat fields.
I've done tests on ps and top, and they work fine.
--
Djalal Harouni
http://opendz.org
On Wed, Oct 2, 2013 at 3:55 PM, Djalal Harouni <[email protected]> wrote:
> On Tue, Oct 01, 2013 at 06:36:34PM -0700, Andy Lutomirski wrote:
>> On 10/01/2013 01:26 PM, Djalal Harouni wrote:
>> > Since /proc entries varies at runtime, permission checks need to happen
>> > during each system call.
>> >
>> > However even with that /proc file descriptors can be passed to a more
>> > privileged process (e.g. a suid-exec) which will pass the classic
>> > ptrace_may_access() permission check. The open() call will be issued in
>> > general by an unprivileged process while the disclosure of sensitive
>> > /proc information will happen using a more privileged process at
>> > read(),write()...
>> >
>> > Therfore we need a more sophisticated check to detect if the cred of the
>> > process have changed, and if the cred of the original opener that are
>> > stored in the file->f_cred have enough permission to access the task's
>> > /proc entries during read(), write()...
>> >
>> > Add the proc_allow_access() function that will receive the file->f_cred
>> > as an argument, and tries to check if the opener had enough permission
>> > to access the task's /proc entries.
>> >
>> > This function should be used with the ptrace_may_access() check.
>> >
>> > Cc: Kees Cook <[email protected]>
>> > Suggested-by: Eric W. Biederman <[email protected]>
>> > Signed-off-by: Djalal Harouni <[email protected]>
>> > ---
>> > fs/proc/base.c | 56 ++++++++++++++++++++++++++++++++++++++++++++++++++++++
>> > fs/proc/internal.h | 2 ++
>> > 2 files changed, 58 insertions(+)
>> >
>> > diff --git a/fs/proc/base.c b/fs/proc/base.c
>> > index e834946..c29eeae 100644
>> > --- a/fs/proc/base.c
>> > +++ b/fs/proc/base.c
>> > @@ -168,6 +168,62 @@ int proc_same_open_cred(const struct cred *fcred)
>> > cap_issubset(cred->cap_permitted, fcred->cap_permitted));
>> > }
>> >
>> > +/* Returns 0 on success, -errno on denial. */
>> > +static int __proc_allow_access(const struct cred *cred,
>> > + struct task_struct *task, unsigned int mode)
>> > +{
>> > + int ret = 0;
>> > + const struct cred *tcred;
>> > + const struct cred *fcred = cred;
>> > +
>> > + rcu_read_lock();
>> > + tcred = __task_cred(task);
>> > + if (uid_eq(fcred->uid, tcred->euid) &&
>> > + uid_eq(fcred->uid, tcred->suid) &&
>> > + uid_eq(fcred->uid, tcred->uid) &&
>> > + gid_eq(fcred->gid, tcred->egid) &&
>> > + gid_eq(fcred->gid, tcred->sgid) &&
>> > + gid_eq(fcred->gid, tcred->gid))
>> > + goto out;
>> > +
>>
>> What's this for? Is it supposed to be an optimization? If so, it looks
>> potentially exploitable, although I don't really understand what you're
>> trying to do.
> This function should be used in addition to the ptrace_may_access() one.
Sorry, I was unclear. I meant: what are the uid and gid checks for?
On Wed, Oct 2, 2013 at 4:14 PM, Djalal Harouni <[email protected]> wrote:
> On Tue, Oct 01, 2013 at 06:39:00PM -0700, Andy Lutomirski wrote:
>> On 10/01/2013 01:26 PM, Djalal Harouni wrote:
>> > Some fields of the /proc/*/stat are sensitive fields that need
>> > appropriate protection.
>> >
>> > However, /proc file descriptors can be passed to a more privileged
>> > process (e.g. a suid-exec) which will pass the classic
>> > ptrace_may_access() permission check during read().
>> >
>> > To prevent it, use proc_same_open_cred() to detect if current's cred
>> > have changed between ->open() and ->read(), if so, call
>> > proc_allow_access() to check if the original file's opener had enough
>> > permissions to read these sensitive fields. This will prevent passing
>> > file descriptors to a more privileged process to leak data.
>> >
>> > The patch also adds a previously missing signal->cred_guard_mutex lock.
>> >
>> > This patch does not break userspace since it only hides the fields that
>> > were supposed to be protected.
>> >
>> > Cc: Kees Cook <[email protected]>
>> > Cc: Eric W. Biederman <[email protected]>
>> > Signed-off-by: Djalal Harouni <[email protected]>
>> > ---
>> > fs/proc/array.c | 16 ++++++++++++++--
>> > 1 file changed, 14 insertions(+), 2 deletions(-)
>> >
>> > diff --git a/fs/proc/array.c b/fs/proc/array.c
>> > index cbd0f1b..f034e05 100644
>> > --- a/fs/proc/array.c
>> > +++ b/fs/proc/array.c
>> > @@ -394,7 +394,7 @@ static int do_task_stat(struct seq_file *m, struct pid_namespace *ns,
>> > char state;
>> > pid_t ppid = 0, pgid = -1, sid = -1;
>> > int num_threads = 0;
>> > - int permitted;
>> > + int permitted = 0;
>> > struct mm_struct *mm;
>> > unsigned long long start_time;
>> > unsigned long cmin_flt = 0, cmaj_flt = 0;
>> > @@ -404,10 +404,22 @@ static int do_task_stat(struct seq_file *m, struct pid_namespace *ns,
>> > unsigned long rsslim = 0;
>> > char tcomm[sizeof(task->comm)];
>> > unsigned long flags;
>> > + struct file *file = m->private;
>> > + int same_cred = proc_same_open_cred(file->f_cred);
>> > + unsigned int ptrace_mode = PTRACE_MODE_READ | PTRACE_MODE_NOAUDIT;
>> >
>> > state = *get_task_state(task);
>> > vsize = eip = esp = 0;
>> > - permitted = ptrace_may_access(task, PTRACE_MODE_READ | PTRACE_MODE_NOAUDIT);
>> > +
>> > + if (!mutex_lock_killable(&task->signal->cred_guard_mutex)) {
>> > + permitted = ptrace_may_access(task, ptrace_mode);
>> > + if (permitted && !same_cred)
>> > + permitted = proc_allow_access(file->f_cred,
>> > + task, ptrace_mode);
>> > +
>> > + mutex_unlock(&task->signal->cred_guard_mutex);
>> > + }
>> > +
>>
>> else permitted = false?
> permitted is initialized to 0
Never mind, then -- I read that wrong...
>
> First the original ptrace_may_access() check did not hold
> cred_guard_mutex, so add it. If we can't grab mutex then let permitted
> to be zero. Yes this a change in behaviour and I think it's correct, IOW
> we were not able to perform the ptrace_may_access() check, otherwise
> permitted will depend on checks result.
>
> However, there is still a race here since we set the permitted value
> before gathering the appropriate info about task. At the read() data moment
> this target task may have been gone privileged... , acquiring an X lock
> on target task, will just break/slow things, as it has been shown before...
> Not to mention that the race window is small...
>
>
>> But surely this would be *much* more comprehensible if you had
>> proc_allow_access do the entire check.
> I don't understand what you mean by "do the entire check" ?
I mean to move the entire "check current->cred and f_cred" check into
its own function rather than duplicating it at each call site.
--Andy
On Wed, Oct 2, 2013 at 3:37 PM, Djalal Harouni <[email protected]> wrote:
> On Tue, Oct 01, 2013 at 06:40:41PM -0700, Andy Lutomirski wrote:
>> On 10/01/2013 01:26 PM, Djalal Harouni wrote:
>> > /proc/<pid>/* entries varies at runtime, appropriate permission checks
>> > need to happen during each system call.
>> >
>> > Currently some of these sensitive entries are protected by performing
>> > the ptrace_may_access() check. However even with that the /proc file
>> > descriptors can be passed to a more privileged process
>> > (e.g. a suid-exec) which will pass the classic ptrace_may_access()
>> > check. In general the ->open() call will be issued by an unprivileged
>> > process while the ->read(),->write() calls by a more privileged one.
>> >
>> > Example of these files are:
>> > /proc/*/syscall, /proc/*/stack etc.
>> >
>> > And any open(/proc/self/*) then suid-exec to read()/write() /proc/self/*
>> >
>> >
>> > These files are protected during read() by the ptrace_may_access(),
>> > however the file descriptor can be passed to a suid-exec which can be
>> > used to read data and bypass ASLR. Of course this was discussed several
>> > times on LKML.
>>
>> Can you elaborate on what it is that you're fixing? That is, can you
>> give a concrete example of what process opens what file and passes the
>> fd to what process?
> Yes, the references were already given in this email:
> https://lkml.org/lkml/2013/8/31/209
>
> This has been discussed several times on lkml:
> https://lkml.org/lkml/2013/8/28/544
>
> https://lkml.org/lkml/2013/8/28/564 (check Kees's references)
>
>
>> I'm having trouble following your description.
> Process open a /proc file and pass the fd to a more privilaged process
> that will pass the ptrace_may_access() check, while the original process
> that opened that file should fail at the ptrace_may_access()
So we're talking about two kinds of attacks, right?
Type 1: Unprivileged process does something like open("/proc/1/maps",
O_RDONLY) and then passes the resulting fd to something privileged.
Type 2: Unprivileged process does something like
open("/proc/self/maps", O_RDONLY) and then forks. The parent calls
execve on something privileged.
Can we really not get away with fixing type 1 by preventing these
files from being opened in the first place and type 2 by revoking all
of these fds when a privilege-changing exec happens?
I'm not objecting to your patches so much as thinking that read(2) has
no business looking at current->cred *at all*. But maybe that ship
has already sailed.
--Andy
>
>
>> --Andy
>>
>
> --
> Djalal Harouni
> http://opendz.org
--
Andy Lutomirski
AMA Capital Management, LLC
On Wed, Oct 2, 2013 at 9:51 AM, Andy Lutomirski <[email protected]> wrote:
> On Wed, Oct 2, 2013 at 3:37 PM, Djalal Harouni <[email protected]> wrote:
>> On Tue, Oct 01, 2013 at 06:40:41PM -0700, Andy Lutomirski wrote:
>>> On 10/01/2013 01:26 PM, Djalal Harouni wrote:
>>> > /proc/<pid>/* entries varies at runtime, appropriate permission checks
>>> > need to happen during each system call.
>>> >
>>> > Currently some of these sensitive entries are protected by performing
>>> > the ptrace_may_access() check. However even with that the /proc file
>>> > descriptors can be passed to a more privileged process
>>> > (e.g. a suid-exec) which will pass the classic ptrace_may_access()
>>> > check. In general the ->open() call will be issued by an unprivileged
>>> > process while the ->read(),->write() calls by a more privileged one.
>>> >
>>> > Example of these files are:
>>> > /proc/*/syscall, /proc/*/stack etc.
>>> >
>>> > And any open(/proc/self/*) then suid-exec to read()/write() /proc/self/*
>>> >
>>> >
>>> > These files are protected during read() by the ptrace_may_access(),
>>> > however the file descriptor can be passed to a suid-exec which can be
>>> > used to read data and bypass ASLR. Of course this was discussed several
>>> > times on LKML.
>>>
>>> Can you elaborate on what it is that you're fixing? That is, can you
>>> give a concrete example of what process opens what file and passes the
>>> fd to what process?
>> Yes, the references were already given in this email:
>> https://lkml.org/lkml/2013/8/31/209
>>
>> This has been discussed several times on lkml:
>> https://lkml.org/lkml/2013/8/28/544
>>
>> https://lkml.org/lkml/2013/8/28/564 (check Kees's references)
>>
>>
>>> I'm having trouble following your description.
>> Process open a /proc file and pass the fd to a more privilaged process
>> that will pass the ptrace_may_access() check, while the original process
>> that opened that file should fail at the ptrace_may_access()
>
> So we're talking about two kinds of attacks, right?
Correct.
> Type 1: Unprivileged process does something like open("/proc/1/maps",
> O_RDONLY) and then passes the resulting fd to something privileged.
... and then leaks contents back to unprivileged process.
> Type 2: Unprivileged process does something like
> open("/proc/self/maps", O_RDONLY) and then forks. The parent calls
> execve on something privileged.
... and then parent snoops on file contents for the privileged child.
(Type 2 is solved currently, IIUC. Type 1 could be reduced in scope by
changing these file modes back to 0400.)
> Can we really not get away with fixing type 1 by preventing these
> files from being opened in the first place and type 2 by revoking all
> of these fds when a privilege-changing exec happens?
Type 1 can be done via exec as well. Instead of using a priv exec to
read an arbitrary process, read it could read its own.
I think revoking the fd would be great. Does that mechanism exist?
-Kees
--
Kees Cook
Chrome OS Security
On Wed, Oct 2, 2013 at 10:48 AM, Kees Cook <[email protected]> wrote:
> On Wed, Oct 2, 2013 at 9:51 AM, Andy Lutomirski <[email protected]> wrote:
>> On Wed, Oct 2, 2013 at 3:37 PM, Djalal Harouni <[email protected]> wrote:
>>> On Tue, Oct 01, 2013 at 06:40:41PM -0700, Andy Lutomirski wrote:
>>>> On 10/01/2013 01:26 PM, Djalal Harouni wrote:
>>>> > /proc/<pid>/* entries varies at runtime, appropriate permission checks
>>>> > need to happen during each system call.
>>>> >
>>>> > Currently some of these sensitive entries are protected by performing
>>>> > the ptrace_may_access() check. However even with that the /proc file
>>>> > descriptors can be passed to a more privileged process
>>>> > (e.g. a suid-exec) which will pass the classic ptrace_may_access()
>>>> > check. In general the ->open() call will be issued by an unprivileged
>>>> > process while the ->read(),->write() calls by a more privileged one.
>>>> >
>>>> > Example of these files are:
>>>> > /proc/*/syscall, /proc/*/stack etc.
>>>> >
>>>> > And any open(/proc/self/*) then suid-exec to read()/write() /proc/self/*
>>>> >
>>>> >
>>>> > These files are protected during read() by the ptrace_may_access(),
>>>> > however the file descriptor can be passed to a suid-exec which can be
>>>> > used to read data and bypass ASLR. Of course this was discussed several
>>>> > times on LKML.
>>>>
>>>> Can you elaborate on what it is that you're fixing? That is, can you
>>>> give a concrete example of what process opens what file and passes the
>>>> fd to what process?
>>> Yes, the references were already given in this email:
>>> https://lkml.org/lkml/2013/8/31/209
>>>
>>> This has been discussed several times on lkml:
>>> https://lkml.org/lkml/2013/8/28/544
>>>
>>> https://lkml.org/lkml/2013/8/28/564 (check Kees's references)
>>>
>>>
>>>> I'm having trouble following your description.
>>> Process open a /proc file and pass the fd to a more privilaged process
>>> that will pass the ptrace_may_access() check, while the original process
>>> that opened that file should fail at the ptrace_may_access()
>>
>> So we're talking about two kinds of attacks, right?
>
> Correct.
>
>> Type 1: Unprivileged process does something like open("/proc/1/maps",
>> O_RDONLY) and then passes the resulting fd to something privileged.
>
> ... and then leaks contents back to unprivileged process.
>
>> Type 2: Unprivileged process does something like
>> open("/proc/self/maps", O_RDONLY) and then forks. The parent calls
>> execve on something privileged.
>
> ... and then parent snoops on file contents for the privileged child.
>
> (Type 2 is solved currently, IIUC. Type 1 could be reduced in scope by
> changing these file modes back to 0400.)
>
>> Can we really not get away with fixing type 1 by preventing these
>> files from being opened in the first place and type 2 by revoking all
>> of these fds when a privilege-changing exec happens?
>
> Type 1 can be done via exec as well. Instead of using a priv exec to
> read an arbitrary process, read it could read its own.
Right.
>
> I think revoking the fd would be great. Does that mechanism exist?
There's this thing that never got merged.
http://thread.gmane.org/gmane.linux.kernel/1523331
But doing it more directly should be reasonably straightforward. Either:
(a) when a process execs and privileges change, find all the old proc
inodes, mark them dead, and unlink them, or
(b) add self_exec_id to all the proc file private_data entries (or
somewhere else). Then just make sure that they're unchanged. I think
the bug last time around was because the self_exec_id and struct pid
weren't being compared together.
(a) is probably nicer. I don't know if it'll break things. Linus
seemed to think that the Chrome sandbox was sensitive to this stuff,
but I don't know why.
--Andy
On Wed, Oct 2, 2013 at 11:00 AM, Andy Lutomirski <[email protected]> wrote:
> On Wed, Oct 2, 2013 at 10:48 AM, Kees Cook <[email protected]> wrote:
>> On Wed, Oct 2, 2013 at 9:51 AM, Andy Lutomirski <[email protected]> wrote:
>>> On Wed, Oct 2, 2013 at 3:37 PM, Djalal Harouni <[email protected]> wrote:
>>>> On Tue, Oct 01, 2013 at 06:40:41PM -0700, Andy Lutomirski wrote:
>>>>> On 10/01/2013 01:26 PM, Djalal Harouni wrote:
>>>>> > /proc/<pid>/* entries varies at runtime, appropriate permission checks
>>>>> > need to happen during each system call.
>>>>> >
>>>>> > Currently some of these sensitive entries are protected by performing
>>>>> > the ptrace_may_access() check. However even with that the /proc file
>>>>> > descriptors can be passed to a more privileged process
>>>>> > (e.g. a suid-exec) which will pass the classic ptrace_may_access()
>>>>> > check. In general the ->open() call will be issued by an unprivileged
>>>>> > process while the ->read(),->write() calls by a more privileged one.
>>>>> >
>>>>> > Example of these files are:
>>>>> > /proc/*/syscall, /proc/*/stack etc.
>>>>> >
>>>>> > And any open(/proc/self/*) then suid-exec to read()/write() /proc/self/*
>>>>> >
>>>>> >
>>>>> > These files are protected during read() by the ptrace_may_access(),
>>>>> > however the file descriptor can be passed to a suid-exec which can be
>>>>> > used to read data and bypass ASLR. Of course this was discussed several
>>>>> > times on LKML.
>>>>>
>>>>> Can you elaborate on what it is that you're fixing? That is, can you
>>>>> give a concrete example of what process opens what file and passes the
>>>>> fd to what process?
>>>> Yes, the references were already given in this email:
>>>> https://lkml.org/lkml/2013/8/31/209
>>>>
>>>> This has been discussed several times on lkml:
>>>> https://lkml.org/lkml/2013/8/28/544
>>>>
>>>> https://lkml.org/lkml/2013/8/28/564 (check Kees's references)
>>>>
>>>>
>>>>> I'm having trouble following your description.
>>>> Process open a /proc file and pass the fd to a more privilaged process
>>>> that will pass the ptrace_may_access() check, while the original process
>>>> that opened that file should fail at the ptrace_may_access()
>>>
>>> So we're talking about two kinds of attacks, right?
>>
>> Correct.
>>
>>> Type 1: Unprivileged process does something like open("/proc/1/maps",
>>> O_RDONLY) and then passes the resulting fd to something privileged.
>>
>> ... and then leaks contents back to unprivileged process.
>>
>>> Type 2: Unprivileged process does something like
>>> open("/proc/self/maps", O_RDONLY) and then forks. The parent calls
>>> execve on something privileged.
>>
>> ... and then parent snoops on file contents for the privileged child.
>>
>> (Type 2 is solved currently, IIUC. Type 1 could be reduced in scope by
>> changing these file modes back to 0400.)
>>
>>> Can we really not get away with fixing type 1 by preventing these
>>> files from being opened in the first place and type 2 by revoking all
>>> of these fds when a privilege-changing exec happens?
>>
>> Type 1 can be done via exec as well. Instead of using a priv exec to
>> read an arbitrary process, read it could read its own.
>
> Right.
>
>>
>> I think revoking the fd would be great. Does that mechanism exist?
>
> There's this thing that never got merged.
>
> http://thread.gmane.org/gmane.linux.kernel/1523331
>
> But doing it more directly should be reasonably straightforward. Either:
>
> (a) when a process execs and privileges change, find all the old proc
> inodes, mark them dead, and unlink them, or
>
> (b) add self_exec_id to all the proc file private_data entries (or
> somewhere else). Then just make sure that they're unchanged. I think
> the bug last time around was because the self_exec_id and struct pid
> weren't being compared together.
>
> (a) is probably nicer. I don't know if it'll break things. Linus
> seemed to think that the Chrome sandbox was sensitive to this stuff,
> but I don't know why.
I agree, (a) seems much cleaner. Hm, I don't think Chrome does
anything with these sensitive files (maps, stack, syscall, etc). But
let's ask Julien. :)
Julien, do you see any problem with Chrome's sandbox behavior if these
proc files would be unavailable across privilege changes?
-Kees
--
Kees Cook
Chrome OS Security
On Wed, Oct 02, 2013 at 05:51:15PM +0100, Andy Lutomirski wrote:
> On Wed, Oct 2, 2013 at 3:37 PM, Djalal Harouni <[email protected]> wrote:
> > On Tue, Oct 01, 2013 at 06:40:41PM -0700, Andy Lutomirski wrote:
> >> On 10/01/2013 01:26 PM, Djalal Harouni wrote:
> >> > /proc/<pid>/* entries varies at runtime, appropriate permission checks
> >> > need to happen during each system call.
> >> >
> >> > Currently some of these sensitive entries are protected by performing
> >> > the ptrace_may_access() check. However even with that the /proc file
> >> > descriptors can be passed to a more privileged process
> >> > (e.g. a suid-exec) which will pass the classic ptrace_may_access()
> >> > check. In general the ->open() call will be issued by an unprivileged
> >> > process while the ->read(),->write() calls by a more privileged one.
> >> >
> >> > Example of these files are:
> >> > /proc/*/syscall, /proc/*/stack etc.
> >> >
> >> > And any open(/proc/self/*) then suid-exec to read()/write() /proc/self/*
> >> >
> >> >
> >> > These files are protected during read() by the ptrace_may_access(),
> >> > however the file descriptor can be passed to a suid-exec which can be
> >> > used to read data and bypass ASLR. Of course this was discussed several
> >> > times on LKML.
> >>
> >> Can you elaborate on what it is that you're fixing? That is, can you
> >> give a concrete example of what process opens what file and passes the
> >> fd to what process?
> > Yes, the references were already given in this email:
> > https://lkml.org/lkml/2013/8/31/209
> >
> > This has been discussed several times on lkml:
> > https://lkml.org/lkml/2013/8/28/544
> >
> > https://lkml.org/lkml/2013/8/28/564 (check Kees's references)
> >
> >
> >> I'm having trouble following your description.
> > Process open a /proc file and pass the fd to a more privilaged process
> > that will pass the ptrace_may_access() check, while the original process
> > that opened that file should fail at the ptrace_may_access()
>
> So we're talking about two kinds of attacks, right?
Yes,
> Type 1: Unprivileged process does something like open("/proc/1/maps",
> O_RDONLY) and then passes the resulting fd to something privileged.
Yes,
> Type 2: Unprivileged process does something like
> open("/proc/self/maps", O_RDONLY) and then forks. The parent calls
> execve on something privileged.
Correct
>
> Can we really not get away with fixing type 1 by preventing these
> files from being opened in the first place and type 2 by revoking all
> of these fds when a privilege-changing exec happens?
Yes for 1) and that's what exactly this series try to do
* ptrace checks during ->open() for /proc/*/{stack,syscall} and 0400
* You can't do it for /proc/*/stat otherwise you will break userspace
"ps"..., ps must access /proc/1/stat etc... so the proposed solution
will work without any side effect.
And for /proc/*/maps you will perhaps break glibc under certain
situations... so just hold it for the moment and test it
later. There have been reports in the past about it.
So for 1) this series will fix it
For type 2) yes this series will also handle it, since there were a cred
change.
For fd revoking, yes that perhaps will work and it would be great, but
it does not exist :-/
Not to mention if the process do not want to revoke and pass the fd ?
is this compliant with standards ?
> I'm not objecting to your patches so much as thinking that read(2) has
> no business looking at current->cred *at all*. But maybe that ship
> has already sailed.
No that's not correct.
read(2) and during each system call current->cred must be checked,
the /proc varies at runtime, you are not sure about what are you
reading,writing...
> --Andy
>
> >
> >
> >> --Andy
> >>
> >
> > --
> > Djalal Harouni
> > http://opendz.org
>
>
>
> --
> Andy Lutomirski
> AMA Capital Management, LLC
--
Djalal Harouni
http://opendz.org
On Wed, Oct 02, 2013 at 10:48:55AM -0700, Kees Cook wrote:
> On Wed, Oct 2, 2013 at 9:51 AM, Andy Lutomirski <[email protected]> wrote:
> > On Wed, Oct 2, 2013 at 3:37 PM, Djalal Harouni <[email protected]> wrote:
> >> On Tue, Oct 01, 2013 at 06:40:41PM -0700, Andy Lutomirski wrote:
> >>> On 10/01/2013 01:26 PM, Djalal Harouni wrote:
> >>> > /proc/<pid>/* entries varies at runtime, appropriate permission checks
> >>> > need to happen during each system call.
> >>> >
> >>> > Currently some of these sensitive entries are protected by performing
> >>> > the ptrace_may_access() check. However even with that the /proc file
> >>> > descriptors can be passed to a more privileged process
> >>> > (e.g. a suid-exec) which will pass the classic ptrace_may_access()
> >>> > check. In general the ->open() call will be issued by an unprivileged
> >>> > process while the ->read(),->write() calls by a more privileged one.
> >>> >
> >>> > Example of these files are:
> >>> > /proc/*/syscall, /proc/*/stack etc.
> >>> >
> >>> > And any open(/proc/self/*) then suid-exec to read()/write() /proc/self/*
> >>> >
> >>> >
> >>> > These files are protected during read() by the ptrace_may_access(),
> >>> > however the file descriptor can be passed to a suid-exec which can be
> >>> > used to read data and bypass ASLR. Of course this was discussed several
> >>> > times on LKML.
> >>>
> >>> Can you elaborate on what it is that you're fixing? That is, can you
> >>> give a concrete example of what process opens what file and passes the
> >>> fd to what process?
> >> Yes, the references were already given in this email:
> >> https://lkml.org/lkml/2013/8/31/209
> >>
> >> This has been discussed several times on lkml:
> >> https://lkml.org/lkml/2013/8/28/544
> >>
> >> https://lkml.org/lkml/2013/8/28/564 (check Kees's references)
> >>
> >>
> >>> I'm having trouble following your description.
> >> Process open a /proc file and pass the fd to a more privilaged process
> >> that will pass the ptrace_may_access() check, while the original process
> >> that opened that file should fail at the ptrace_may_access()
> >
> > So we're talking about two kinds of attacks, right?
>
> Correct.
>
> > Type 1: Unprivileged process does something like open("/proc/1/maps",
> > O_RDONLY) and then passes the resulting fd to something privileged.
>
> ... and then leaks contents back to unprivileged process.
>
> > Type 2: Unprivileged process does something like
> > open("/proc/self/maps", O_RDONLY) and then forks. The parent calls
> > execve on something privileged.
>
> ... and then parent snoops on file contents for the privileged child.
>
> (Type 2 is solved currently, IIUC. Type 1 could be reduced in scope by
> changing these file modes back to 0400.)
Kees for 0400 on /proc/*/maps, it was reported that it could break glibc
Even with 0444 this series will catch it, take a look at /proc/*/stat
example, we just delay the check that is suposed to be done during
->open() into ->read(), if the cred change of course
--
Djalal Harouni
http://opendz.org
On Wed, Oct 02, 2013 at 11:00:26AM -0700, Andy Lutomirski wrote:
> On Wed, Oct 2, 2013 at 10:48 AM, Kees Cook <[email protected]> wrote:
> > On Wed, Oct 2, 2013 at 9:51 AM, Andy Lutomirski <[email protected]> wrote:
> >> On Wed, Oct 2, 2013 at 3:37 PM, Djalal Harouni <[email protected]> wrote:
> >>> On Tue, Oct 01, 2013 at 06:40:41PM -0700, Andy Lutomirski wrote:
> >>>> On 10/01/2013 01:26 PM, Djalal Harouni wrote:
> >>>> > /proc/<pid>/* entries varies at runtime, appropriate permission checks
> >>>> > need to happen during each system call.
> >>>> >
> >>>> > Currently some of these sensitive entries are protected by performing
> >>>> > the ptrace_may_access() check. However even with that the /proc file
> >>>> > descriptors can be passed to a more privileged process
> >>>> > (e.g. a suid-exec) which will pass the classic ptrace_may_access()
> >>>> > check. In general the ->open() call will be issued by an unprivileged
> >>>> > process while the ->read(),->write() calls by a more privileged one.
> >>>> >
> >>>> > Example of these files are:
> >>>> > /proc/*/syscall, /proc/*/stack etc.
> >>>> >
> >>>> > And any open(/proc/self/*) then suid-exec to read()/write() /proc/self/*
> >>>> >
> >>>> >
> >>>> > These files are protected during read() by the ptrace_may_access(),
> >>>> > however the file descriptor can be passed to a suid-exec which can be
> >>>> > used to read data and bypass ASLR. Of course this was discussed several
> >>>> > times on LKML.
> >>>>
> >>>> Can you elaborate on what it is that you're fixing? That is, can you
> >>>> give a concrete example of what process opens what file and passes the
> >>>> fd to what process?
> >>> Yes, the references were already given in this email:
> >>> https://lkml.org/lkml/2013/8/31/209
> >>>
> >>> This has been discussed several times on lkml:
> >>> https://lkml.org/lkml/2013/8/28/544
> >>>
> >>> https://lkml.org/lkml/2013/8/28/564 (check Kees's references)
> >>>
> >>>
> >>>> I'm having trouble following your description.
> >>> Process open a /proc file and pass the fd to a more privilaged process
> >>> that will pass the ptrace_may_access() check, while the original process
> >>> that opened that file should fail at the ptrace_may_access()
> >>
> >> So we're talking about two kinds of attacks, right?
> >
> > Correct.
> >
> >> Type 1: Unprivileged process does something like open("/proc/1/maps",
> >> O_RDONLY) and then passes the resulting fd to something privileged.
> >
> > ... and then leaks contents back to unprivileged process.
> >
> >> Type 2: Unprivileged process does something like
> >> open("/proc/self/maps", O_RDONLY) and then forks. The parent calls
> >> execve on something privileged.
> >
> > ... and then parent snoops on file contents for the privileged child.
> >
> > (Type 2 is solved currently, IIUC. Type 1 could be reduced in scope by
> > changing these file modes back to 0400.)
> >
> >> Can we really not get away with fixing type 1 by preventing these
> >> files from being opened in the first place and type 2 by revoking all
> >> of these fds when a privilege-changing exec happens?
> >
> > Type 1 can be done via exec as well. Instead of using a priv exec to
> > read an arbitrary process, read it could read its own.
>
> Right.
>
> >
> > I think revoking the fd would be great. Does that mechanism exist?
>
> There's this thing that never got merged.
>
> http://thread.gmane.org/gmane.linux.kernel/1523331
>
> But doing it more directly should be reasonably straightforward. Either:
>
> (a) when a process execs and privileges change, find all the old proc
> inodes, mark them dead, and unlink them, or
Will take a look at it.
> (b) add self_exec_id to all the proc file private_data entries (or
> somewhere else). Then just make sure that they're unchanged. I think
> the bug last time around was because the self_exec_id and struct pid
> weren't being compared together.
The bug was about self_exec_id not beeing unique. self_exec_id stuff
must be unique during life time as it's done currently in grsecurity
with exec_id.
--
Djalal Harouni
http://opendz.org
On Wed, Oct 2, 2013 at 11:22 AM, Djalal Harouni <[email protected]> wrote:
> On Wed, Oct 02, 2013 at 10:48:55AM -0700, Kees Cook wrote:
>> On Wed, Oct 2, 2013 at 9:51 AM, Andy Lutomirski <[email protected]> wrote:
>> > On Wed, Oct 2, 2013 at 3:37 PM, Djalal Harouni <[email protected]> wrote:
>> >> On Tue, Oct 01, 2013 at 06:40:41PM -0700, Andy Lutomirski wrote:
>> >>> On 10/01/2013 01:26 PM, Djalal Harouni wrote:
>> >>> > /proc/<pid>/* entries varies at runtime, appropriate permission checks
>> >>> > need to happen during each system call.
>> >>> >
>> >>> > Currently some of these sensitive entries are protected by performing
>> >>> > the ptrace_may_access() check. However even with that the /proc file
>> >>> > descriptors can be passed to a more privileged process
>> >>> > (e.g. a suid-exec) which will pass the classic ptrace_may_access()
>> >>> > check. In general the ->open() call will be issued by an unprivileged
>> >>> > process while the ->read(),->write() calls by a more privileged one.
>> >>> >
>> >>> > Example of these files are:
>> >>> > /proc/*/syscall, /proc/*/stack etc.
>> >>> >
>> >>> > And any open(/proc/self/*) then suid-exec to read()/write() /proc/self/*
>> >>> >
>> >>> >
>> >>> > These files are protected during read() by the ptrace_may_access(),
>> >>> > however the file descriptor can be passed to a suid-exec which can be
>> >>> > used to read data and bypass ASLR. Of course this was discussed several
>> >>> > times on LKML.
>> >>>
>> >>> Can you elaborate on what it is that you're fixing? That is, can you
>> >>> give a concrete example of what process opens what file and passes the
>> >>> fd to what process?
>> >> Yes, the references were already given in this email:
>> >> https://lkml.org/lkml/2013/8/31/209
>> >>
>> >> This has been discussed several times on lkml:
>> >> https://lkml.org/lkml/2013/8/28/544
>> >>
>> >> https://lkml.org/lkml/2013/8/28/564 (check Kees's references)
>> >>
>> >>
>> >>> I'm having trouble following your description.
>> >> Process open a /proc file and pass the fd to a more privilaged process
>> >> that will pass the ptrace_may_access() check, while the original process
>> >> that opened that file should fail at the ptrace_may_access()
>> >
>> > So we're talking about two kinds of attacks, right?
>>
>> Correct.
>>
>> > Type 1: Unprivileged process does something like open("/proc/1/maps",
>> > O_RDONLY) and then passes the resulting fd to something privileged.
>>
>> ... and then leaks contents back to unprivileged process.
>>
>> > Type 2: Unprivileged process does something like
>> > open("/proc/self/maps", O_RDONLY) and then forks. The parent calls
>> > execve on something privileged.
>>
>> ... and then parent snoops on file contents for the privileged child.
>>
>> (Type 2 is solved currently, IIUC. Type 1 could be reduced in scope by
>> changing these file modes back to 0400.)
> Kees for 0400 on /proc/*/maps, it was reported that it could break glibc
I didn't mean maps should be 0400. The maps file is already handled
differently (pinning mm at open time). I didn't think it was one of
the problematic files.
Regardless, glibc uses /proc/self/maps, which would be fine here, right?
-Kees
--
Kees Cook
Chrome OS Security
On Wed, Oct 02, 2013 at 07:26:43PM +0100, Djalal Harouni wrote:
> On Wed, Oct 02, 2013 at 11:00:26AM -0700, Andy Lutomirski wrote:
> > On Wed, Oct 2, 2013 at 10:48 AM, Kees Cook <[email protected]> wrote:
> > > I think revoking the fd would be great. Does that mechanism exist?
> >
> > There's this thing that never got merged.
> >
> > http://thread.gmane.org/gmane.linux.kernel/1523331
> >
> > But doing it more directly should be reasonably straightforward. Either:
> >
> > (a) when a process execs and privileges change, find all the old proc
> > inodes, mark them dead, and unlink them, or
> Will take a look at it.
>
> > (b) add self_exec_id to all the proc file private_data entries (or
> > somewhere else). Then just make sure that they're unchanged. I think
> > the bug last time around was because the self_exec_id and struct pid
> > weren't being compared together.
> The bug was about self_exec_id not beeing unique. self_exec_id stuff
> must be unique during life time as it's done currently in grsecurity
> with exec_id.
I forget to mention that I've already proposed this (b) solution, but after
the discussion with Eric, I came to the conclusion that we could allow
the fd to be passed to the process if the original opener had enough
privileges. In either case we want to check the privileges, otherwise ps
and other tool will not report data if target tasks do execve.
--
Djalal Harouni
http://opendz.org
On Wed, Oct 02, 2013 at 11:35:45AM -0700, Kees Cook wrote:
> On Wed, Oct 2, 2013 at 11:22 AM, Djalal Harouni <[email protected]> wrote:
> > On Wed, Oct 02, 2013 at 10:48:55AM -0700, Kees Cook wrote:
> >> On Wed, Oct 2, 2013 at 9:51 AM, Andy Lutomirski <[email protected]> wrote:
> >> > On Wed, Oct 2, 2013 at 3:37 PM, Djalal Harouni <[email protected]> wrote:
> >> >> On Tue, Oct 01, 2013 at 06:40:41PM -0700, Andy Lutomirski wrote:
> >> >>> On 10/01/2013 01:26 PM, Djalal Harouni wrote:
> >> >>> > /proc/<pid>/* entries varies at runtime, appropriate permission checks
> >> >>> > need to happen during each system call.
> >> >>> >
> >> >>> > Currently some of these sensitive entries are protected by performing
> >> >>> > the ptrace_may_access() check. However even with that the /proc file
> >> >>> > descriptors can be passed to a more privileged process
> >> >>> > (e.g. a suid-exec) which will pass the classic ptrace_may_access()
> >> >>> > check. In general the ->open() call will be issued by an unprivileged
> >> >>> > process while the ->read(),->write() calls by a more privileged one.
> >> >>> >
> >> >>> > Example of these files are:
> >> >>> > /proc/*/syscall, /proc/*/stack etc.
> >> >>> >
> >> >>> > And any open(/proc/self/*) then suid-exec to read()/write() /proc/self/*
> >> >>> >
> >> >>> >
> >> >>> > These files are protected during read() by the ptrace_may_access(),
> >> >>> > however the file descriptor can be passed to a suid-exec which can be
> >> >>> > used to read data and bypass ASLR. Of course this was discussed several
> >> >>> > times on LKML.
> >> >>>
> >> >>> Can you elaborate on what it is that you're fixing? That is, can you
> >> >>> give a concrete example of what process opens what file and passes the
> >> >>> fd to what process?
> >> >> Yes, the references were already given in this email:
> >> >> https://lkml.org/lkml/2013/8/31/209
> >> >>
> >> >> This has been discussed several times on lkml:
> >> >> https://lkml.org/lkml/2013/8/28/544
> >> >>
> >> >> https://lkml.org/lkml/2013/8/28/564 (check Kees's references)
> >> >>
> >> >>
> >> >>> I'm having trouble following your description.
> >> >> Process open a /proc file and pass the fd to a more privilaged process
> >> >> that will pass the ptrace_may_access() check, while the original process
> >> >> that opened that file should fail at the ptrace_may_access()
> >> >
> >> > So we're talking about two kinds of attacks, right?
> >>
> >> Correct.
> >>
> >> > Type 1: Unprivileged process does something like open("/proc/1/maps",
> >> > O_RDONLY) and then passes the resulting fd to something privileged.
> >>
> >> ... and then leaks contents back to unprivileged process.
> >>
> >> > Type 2: Unprivileged process does something like
> >> > open("/proc/self/maps", O_RDONLY) and then forks. The parent calls
> >> > execve on something privileged.
> >>
> >> ... and then parent snoops on file contents for the privileged child.
> >>
> >> (Type 2 is solved currently, IIUC. Type 1 could be reduced in scope by
> >> changing these file modes back to 0400.)
> > Kees for 0400 on /proc/*/maps, it was reported that it could break glibc
>
> I didn't mean maps should be 0400. The maps file is already handled
> differently (pinning mm at open time). I didn't think it was one of
> the problematic files.
Kees currently all these files can be used to leak data, except for
/proc/*/{mem,environ}
These are the only one that pin the mm at open time. But I'm not sure
that this solution will work for /proc/*/maps since they need vma info
which will be perhaps freed if task execv, Need to check it.
> Regardless, glibc uses /proc/self/maps, which would be fine here, right?
I did not touch /proc/self/maps and others, but I'm planning to fix them
if this solution is accepted.
I'll do the same thing as in /proc/*/stat for maps, let it be 0444, and
try to delay the check to ->read(). So during ->read() perform
ptrace_may_access() on currenct's cred and process_allow_access() on
file's opener cred. This should work.
> -Kees
>
> --
> Kees Cook
> Chrome OS Security
--
Djalal Harouni
http://opendz.org
On Wed, Oct 02, 2013 at 05:46:19PM +0100, Andy Lutomirski wrote:
> On Wed, Oct 2, 2013 at 4:14 PM, Djalal Harouni <[email protected]> wrote:
> > On Tue, Oct 01, 2013 at 06:39:00PM -0700, Andy Lutomirski wrote:
> >> On 10/01/2013 01:26 PM, Djalal Harouni wrote:
> >> > Some fields of the /proc/*/stat are sensitive fields that need
> >> > appropriate protection.
> >> >
> >> > However, /proc file descriptors can be passed to a more privileged
> >> > process (e.g. a suid-exec) which will pass the classic
> >> > ptrace_may_access() permission check during read().
> >> >
> >> > To prevent it, use proc_same_open_cred() to detect if current's cred
> >> > have changed between ->open() and ->read(), if so, call
> >> > proc_allow_access() to check if the original file's opener had enough
> >> > permissions to read these sensitive fields. This will prevent passing
> >> > file descriptors to a more privileged process to leak data.
> >> >
> >> > The patch also adds a previously missing signal->cred_guard_mutex lock.
> >> >
> >> > This patch does not break userspace since it only hides the fields that
> >> > were supposed to be protected.
> >> >
> >> > Cc: Kees Cook <[email protected]>
> >> > Cc: Eric W. Biederman <[email protected]>
> >> > Signed-off-by: Djalal Harouni <[email protected]>
> >> > ---
> >> > fs/proc/array.c | 16 ++++++++++++++--
> >> > 1 file changed, 14 insertions(+), 2 deletions(-)
> >> >
> >> > diff --git a/fs/proc/array.c b/fs/proc/array.c
> >> > index cbd0f1b..f034e05 100644
> >> > --- a/fs/proc/array.c
> >> > +++ b/fs/proc/array.c
> >> > @@ -394,7 +394,7 @@ static int do_task_stat(struct seq_file *m, struct pid_namespace *ns,
> >> > char state;
> >> > pid_t ppid = 0, pgid = -1, sid = -1;
> >> > int num_threads = 0;
> >> > - int permitted;
> >> > + int permitted = 0;
> >> > struct mm_struct *mm;
> >> > unsigned long long start_time;
> >> > unsigned long cmin_flt = 0, cmaj_flt = 0;
> >> > @@ -404,10 +404,22 @@ static int do_task_stat(struct seq_file *m, struct pid_namespace *ns,
> >> > unsigned long rsslim = 0;
> >> > char tcomm[sizeof(task->comm)];
> >> > unsigned long flags;
> >> > + struct file *file = m->private;
> >> > + int same_cred = proc_same_open_cred(file->f_cred);
> >> > + unsigned int ptrace_mode = PTRACE_MODE_READ | PTRACE_MODE_NOAUDIT;
> >> >
> >> > state = *get_task_state(task);
> >> > vsize = eip = esp = 0;
> >> > - permitted = ptrace_may_access(task, PTRACE_MODE_READ | PTRACE_MODE_NOAUDIT);
> >> > +
> >> > + if (!mutex_lock_killable(&task->signal->cred_guard_mutex)) {
> >> > + permitted = ptrace_may_access(task, ptrace_mode);
> >> > + if (permitted && !same_cred)
> >> > + permitted = proc_allow_access(file->f_cred,
> >> > + task, ptrace_mode);
> >> > +
> >> > + mutex_unlock(&task->signal->cred_guard_mutex);
> >> > + }
> >> > +
> >>
> >> else permitted = false?
> > permitted is initialized to 0
>
> Never mind, then -- I read that wrong...
>
> >
> > First the original ptrace_may_access() check did not hold
> > cred_guard_mutex, so add it. If we can't grab mutex then let permitted
> > to be zero. Yes this a change in behaviour and I think it's correct, IOW
> > we were not able to perform the ptrace_may_access() check, otherwise
> > permitted will depend on checks result.
> >
> > However, there is still a race here since we set the permitted value
> > before gathering the appropriate info about task. At the read() data moment
> > this target task may have been gone privileged... , acquiring an X lock
> > on target task, will just break/slow things, as it has been shown before...
> > Not to mention that the race window is small...
> >
> >
> >> But surely this would be *much* more comprehensible if you had
> >> proc_allow_access do the entire check.
> > I don't understand what you mean by "do the entire check" ?
>
> I mean to move the entire "check current->cred and f_cred" check into
> its own function rather than duplicating it at each call site.
We can perhaps do this, yes.
In other places the check is done and protected by
lock_trace()/unlock_trace(). Will need to see if we can do it.
> --Andy
--
Djalal Harouni
http://opendz.org
On Wed, Oct 2, 2013 at 11:48 AM, Djalal Harouni <[email protected]> wrote:
> On Wed, Oct 02, 2013 at 11:35:45AM -0700, Kees Cook wrote:
>> On Wed, Oct 2, 2013 at 11:22 AM, Djalal Harouni <[email protected]> wrote:
>> > On Wed, Oct 02, 2013 at 10:48:55AM -0700, Kees Cook wrote:
>> >> On Wed, Oct 2, 2013 at 9:51 AM, Andy Lutomirski <[email protected]> wrote:
>> >> > On Wed, Oct 2, 2013 at 3:37 PM, Djalal Harouni <[email protected]> wrote:
>> >> >> On Tue, Oct 01, 2013 at 06:40:41PM -0700, Andy Lutomirski wrote:
>> >> >>> On 10/01/2013 01:26 PM, Djalal Harouni wrote:
>> >> >>> > /proc/<pid>/* entries varies at runtime, appropriate permission checks
>> >> >>> > need to happen during each system call.
>> >> >>> >
>> >> >>> > Currently some of these sensitive entries are protected by performing
>> >> >>> > the ptrace_may_access() check. However even with that the /proc file
>> >> >>> > descriptors can be passed to a more privileged process
>> >> >>> > (e.g. a suid-exec) which will pass the classic ptrace_may_access()
>> >> >>> > check. In general the ->open() call will be issued by an unprivileged
>> >> >>> > process while the ->read(),->write() calls by a more privileged one.
>> >> >>> >
>> >> >>> > Example of these files are:
>> >> >>> > /proc/*/syscall, /proc/*/stack etc.
>> >> >>> >
>> >> >>> > And any open(/proc/self/*) then suid-exec to read()/write() /proc/self/*
>> >> >>> >
>> >> >>> >
>> >> >>> > These files are protected during read() by the ptrace_may_access(),
>> >> >>> > however the file descriptor can be passed to a suid-exec which can be
>> >> >>> > used to read data and bypass ASLR. Of course this was discussed several
>> >> >>> > times on LKML.
>> >> >>>
>> >> >>> Can you elaborate on what it is that you're fixing? That is, can you
>> >> >>> give a concrete example of what process opens what file and passes the
>> >> >>> fd to what process?
>> >> >> Yes, the references were already given in this email:
>> >> >> https://lkml.org/lkml/2013/8/31/209
>> >> >>
>> >> >> This has been discussed several times on lkml:
>> >> >> https://lkml.org/lkml/2013/8/28/544
>> >> >>
>> >> >> https://lkml.org/lkml/2013/8/28/564 (check Kees's references)
>> >> >>
>> >> >>
>> >> >>> I'm having trouble following your description.
>> >> >> Process open a /proc file and pass the fd to a more privilaged process
>> >> >> that will pass the ptrace_may_access() check, while the original process
>> >> >> that opened that file should fail at the ptrace_may_access()
>> >> >
>> >> > So we're talking about two kinds of attacks, right?
>> >>
>> >> Correct.
>> >>
>> >> > Type 1: Unprivileged process does something like open("/proc/1/maps",
>> >> > O_RDONLY) and then passes the resulting fd to something privileged.
>> >>
>> >> ... and then leaks contents back to unprivileged process.
>> >>
>> >> > Type 2: Unprivileged process does something like
>> >> > open("/proc/self/maps", O_RDONLY) and then forks. The parent calls
>> >> > execve on something privileged.
>> >>
>> >> ... and then parent snoops on file contents for the privileged child.
>> >>
>> >> (Type 2 is solved currently, IIUC. Type 1 could be reduced in scope by
>> >> changing these file modes back to 0400.)
>> > Kees for 0400 on /proc/*/maps, it was reported that it could break glibc
>>
>> I didn't mean maps should be 0400. The maps file is already handled
>> differently (pinning mm at open time). I didn't think it was one of
>> the problematic files.
> Kees currently all these files can be used to leak data, except for
> /proc/*/{mem,environ}
>
> These are the only one that pin the mm at open time. But I'm not sure
> that this solution will work for /proc/*/maps since they need vma info
> which will be perhaps freed if task execv, Need to check it.
Ah, yes, you're totally right. I had misremembered.
-Kees
--
Kees Cook
Chrome OS Security
* Djalal Harouni <[email protected]> wrote:
> > Regardless, glibc uses /proc/self/maps, which would be fine here, right?
>
> I did not touch /proc/self/maps and others, but I'm planning to fix them
> if this solution is accepted.
>
> I'll do the same thing as in /proc/*/stat for maps, let it be 0444, and
> try to delay the check to ->read(). So during ->read() perform
> ptrace_may_access() on currenct's cred and process_allow_access() on
> file's opener cred. This should work.
Aren't read() time checks fundamentally unrobust? We generally don't do
locking on read()s, so such checks - in the general case - are pretty
racy.
Now procfs might be special, as by its nature of a pseudofilesystem it's
far more atomic than other filesystems, but still IMHO it's a lot more
robust security design wise to revoke access to objects you should not
have a reference to when a security boundary is crossed, than letting
stuff leak through and sprinking all the potential cases that may leak
information with permission checks ...
It's also probably faster: security boundary crossings are relatively rare
slowpaths, while read()s are much more common...
So please first get consensus on this fundamental design question before
spreading your solution to more areas.
Thanks,
Ingo
* Djalal Harouni <[email protected]> wrote:
> * You can't do it for /proc/*/stat otherwise you will break userspace
> "ps"..., ps must access /proc/1/stat etc... so the proposed solution
> will work without any side effect.
The thing is, returning -EINVAL is not the only way to reject access to
privileged information!
In the /proc/1/stat case a compatibility quirk can solve the problem:
create a special 'dummy' process inode for invalid accesses and give it to
ps, with all fields present but zero.
> And for /proc/*/maps you will perhaps break glibc under certain
> situations... so just hold it for the moment and test it
> later. There have been reports in the past about it.
Same deal: just create a dummy compat-quirk maps inode with constant, zero
information contents to placate old user-space:
00000000-00000000 ---p 00000000 00:00 0
[ Or whatever line is needed to minimally not break old userspace. ]
But don't leak privileged information!
( Maybe add a CONFIG_PROC_FS_COMPAT_QUIRKS Kconfig option, default-y for
now, that new/sane userspace can turn off. )
Thanks,
Ingo
On Thu, Oct 03, 2013 at 08:12:44AM +0200, Ingo Molnar wrote:
>
> * Djalal Harouni <[email protected]> wrote:
>
> > > Regardless, glibc uses /proc/self/maps, which would be fine here, right?
> >
> > I did not touch /proc/self/maps and others, but I'm planning to fix them
> > if this solution is accepted.
> >
> > I'll do the same thing as in /proc/*/stat for maps, let it be 0444, and
> > try to delay the check to ->read(). So during ->read() perform
> > ptrace_may_access() on currenct's cred and process_allow_access() on
> > file's opener cred. This should work.
>
> Aren't read() time checks fundamentally unrobust? We generally don't do
> locking on read()s, so such checks - in the general case - are pretty
> racy.
For procfs yes, read() time checks are unrobust, the general agreement on
procfs is checks should be performed during each syscall.
For the locking on read()/write() IMHO there should be locking by design
for /proc/pid/* entries. Here we are speaking about content that varies,
data attached to other processes, so there is already some locking
mechanism, and for sensitive stuff, we must hold the cred mutex. This
is the standard from the old days of procfs.
And yes some of them are racy, but we can improve it, delay the checks.
>From old Linux git history, before the initial git repository build, I
found that some important checks were done right after gathering the info.
> Now procfs might be special, as by its nature of a pseudofilesystem it's
> far more atomic than other filesystems, but still IMHO it's a lot more
> robust security design wise to revoke access to objects you should not
> have a reference to when a security boundary is crossed, than letting
> stuff leak through and sprinking all the potential cases that may leak
> information with permission checks ...
I agree, but those access should also be checked at the beginning, IOW
during ->open(). revoke will not help if revoke is not involved at all,
the task /proc entries may still be valide (no execve).
Currently security boundary is crossed for example arbitrary /proc/*/stack
(and others).
1) The target task does not do an execve (no revoke).
2) current task will open these files and *want* and *will* pass the fd to a
more privileged process to pass the ptrace check which is done only during
->read().
> It's also probably faster: security boundary crossings are relatively rare
> slowpaths, while read()s are much more common...
Hmm, These two are related? can't get rid of permission checks
especially on this pseudofilesystem!
> So please first get consensus on this fundamental design question before
> spreading your solution to more areas.
Of course, I did clean the patchset to prove that it will work, and I
only implemented full protection for /proc/*/{stack,syscall,stat} other
files will wait.
But Ingo you can't ignore the fact that:
/proc/*/{stack,syscall} are 0444 mode
/proc/*/{stack,syscall} do not have ptrace_may_access() during open()
/proc/*/{stack,syscall} have the ptrace_may_access() during read()
Every unprivileged process will have an fd on arbitrary privileged
files, then pass fd to a more privileged process...
https://lkml.org/lkml/2013/8/28/544
We need 0400 VFS checks, and ptrace capability check during ->open().
So here revoke is not invloved at all, we'll have to close these issues.
Any thoughts please ? patch submitted but... I can split it again to
try to close these issues. Then wait to see for the file->f_cred or revoke
design ?
> Thanks,
>
> Ingo
--
Djalal Harouni
http://opendz.org
On Thu, Oct 03, 2013 at 08:22:56AM +0200, Ingo Molnar wrote:
>
> * Djalal Harouni <[email protected]> wrote:
>
> > * You can't do it for /proc/*/stat otherwise you will break userspace
> > "ps"..., ps must access /proc/1/stat etc... so the proposed solution
> > will work without any side effect.
>
> The thing is, returning -EINVAL is not the only way to reject access to
> privileged information!
> In the /proc/1/stat case a compatibility quirk can solve the problem:
> create a special 'dummy' process inode for invalid accesses and give it to
> ps, with all fields present but zero.
Hmm, we already return zero for the fields that must be protected.
Already done.
Not all fields need to be zero ? If so, yes it could be done as you
propose and avoid the 'if permitted' test each time... but we don't want
to do it
> > And for /proc/*/maps you will perhaps break glibc under certain
> > situations... so just hold it for the moment and test it
> > later. There have been reports in the past about it.
>
> Same deal: just create a dummy compat-quirk maps inode with constant, zero
> information contents to placate old user-space:
>
> 00000000-00000000 ---p 00000000 00:00 0
>
> [ Or whatever line is needed to minimally not break old userspace. ]
>
> But don't leak privileged information!
>
> ( Maybe add a CONFIG_PROC_FS_COMPAT_QUIRKS Kconfig option, default-y for
> now, that new/sane userspace can turn off. )
Yes, that could work, but I'm not sure (it depends on what glibc is
doing and what info it needs)
With the right permission checks, and glibc tests, this will be nice!
> Thanks,
>
> Ingo
--
Djalal Harouni
http://opendz.org
* Djalal Harouni <[email protected]> wrote:
> On Thu, Oct 03, 2013 at 08:22:56AM +0200, Ingo Molnar wrote:
> >
> > * Djalal Harouni <[email protected]> wrote:
> >
> > > * You can't do it for /proc/*/stat otherwise you will break userspace
> > > "ps"..., ps must access /proc/1/stat etc... so the proposed solution
> > > will work without any side effect.
> >
> > The thing is, returning -EINVAL is not the only way to reject access to
> > privileged information!
>
> > In the /proc/1/stat case a compatibility quirk can solve the problem:
> > create a special 'dummy' process inode for invalid accesses and give
> > it to ps, with all fields present but zero.
>
> Hmm, we already return zero for the fields that must be protected.
> Already done.
>
> Not all fields need to be zero ? If so, yes it could be done as you
> propose and avoid the 'if permitted' test each time... but we don't want
> to do it
Indeed some fields need to be available, for utilities like 'top' to work.
Thanks,
Ingo
On Wed, Oct 02, 2013 at 05:44:17PM +0100, Andy Lutomirski wrote:
> On Wed, Oct 2, 2013 at 3:55 PM, Djalal Harouni <[email protected]> wrote:
> > On Tue, Oct 01, 2013 at 06:36:34PM -0700, Andy Lutomirski wrote:
> >> On 10/01/2013 01:26 PM, Djalal Harouni wrote:
> >> > Since /proc entries varies at runtime, permission checks need to happen
> >> > during each system call.
> >> >
> >> > However even with that /proc file descriptors can be passed to a more
> >> > privileged process (e.g. a suid-exec) which will pass the classic
> >> > ptrace_may_access() permission check. The open() call will be issued in
> >> > general by an unprivileged process while the disclosure of sensitive
> >> > /proc information will happen using a more privileged process at
> >> > read(),write()...
> >> >
> >> > Therfore we need a more sophisticated check to detect if the cred of the
> >> > process have changed, and if the cred of the original opener that are
> >> > stored in the file->f_cred have enough permission to access the task's
> >> > /proc entries during read(), write()...
> >> >
> >> > Add the proc_allow_access() function that will receive the file->f_cred
> >> > as an argument, and tries to check if the opener had enough permission
> >> > to access the task's /proc entries.
> >> >
> >> > This function should be used with the ptrace_may_access() check.
> >> >
> >> > Cc: Kees Cook <[email protected]>
> >> > Suggested-by: Eric W. Biederman <[email protected]>
> >> > Signed-off-by: Djalal Harouni <[email protected]>
> >> > ---
> >> > fs/proc/base.c | 56 ++++++++++++++++++++++++++++++++++++++++++++++++++++++
> >> > fs/proc/internal.h | 2 ++
> >> > 2 files changed, 58 insertions(+)
> >> >
> >> > diff --git a/fs/proc/base.c b/fs/proc/base.c
> >> > index e834946..c29eeae 100644
> >> > --- a/fs/proc/base.c
> >> > +++ b/fs/proc/base.c
> >> > @@ -168,6 +168,62 @@ int proc_same_open_cred(const struct cred *fcred)
> >> > cap_issubset(cred->cap_permitted, fcred->cap_permitted));
> >> > }
> >> >
> >> > +/* Returns 0 on success, -errno on denial. */
> >> > +static int __proc_allow_access(const struct cred *cred,
> >> > + struct task_struct *task, unsigned int mode)
> >> > +{
> >> > + int ret = 0;
> >> > + const struct cred *tcred;
> >> > + const struct cred *fcred = cred;
> >> > +
> >> > + rcu_read_lock();
> >> > + tcred = __task_cred(task);
> >> > + if (uid_eq(fcred->uid, tcred->euid) &&
> >> > + uid_eq(fcred->uid, tcred->suid) &&
> >> > + uid_eq(fcred->uid, tcred->uid) &&
> >> > + gid_eq(fcred->gid, tcred->egid) &&
> >> > + gid_eq(fcred->gid, tcred->sgid) &&
> >> > + gid_eq(fcred->gid, tcred->gid))
> >> > + goto out;
> >> > +
> >>
> >> What's this for? Is it supposed to be an optimization? If so, it looks
> >> potentially exploitable, although I don't really understand what you're
> >> trying to do.
> > This function should be used in addition to the ptrace_may_access() one.
>
> Sorry, I was unclear. I meant: what are the uid and gid checks for?
The uid/gid are checks of the current (reader) on the target task, like
the ptrace checks. fcred here is the cred of current at open time.
--
Djalal Harouni
http://opendz.org
On Thu, Oct 3, 2013 at 3:36 PM, Djalal Harouni <[email protected]> wrote:
> On Wed, Oct 02, 2013 at 05:44:17PM +0100, Andy Lutomirski wrote:
>> On Wed, Oct 2, 2013 at 3:55 PM, Djalal Harouni <[email protected]> wrote:
>> > On Tue, Oct 01, 2013 at 06:36:34PM -0700, Andy Lutomirski wrote:
>> >> On 10/01/2013 01:26 PM, Djalal Harouni wrote:
>> >> > Since /proc entries varies at runtime, permission checks need to happen
>> >> > during each system call.
>> >> >
>> >> > However even with that /proc file descriptors can be passed to a more
>> >> > privileged process (e.g. a suid-exec) which will pass the classic
>> >> > ptrace_may_access() permission check. The open() call will be issued in
>> >> > general by an unprivileged process while the disclosure of sensitive
>> >> > /proc information will happen using a more privileged process at
>> >> > read(),write()...
>> >> >
>> >> > Therfore we need a more sophisticated check to detect if the cred of the
>> >> > process have changed, and if the cred of the original opener that are
>> >> > stored in the file->f_cred have enough permission to access the task's
>> >> > /proc entries during read(), write()...
>> >> >
>> >> > Add the proc_allow_access() function that will receive the file->f_cred
>> >> > as an argument, and tries to check if the opener had enough permission
>> >> > to access the task's /proc entries.
>> >> >
>> >> > This function should be used with the ptrace_may_access() check.
>> >> >
>> >> > Cc: Kees Cook <[email protected]>
>> >> > Suggested-by: Eric W. Biederman <[email protected]>
>> >> > Signed-off-by: Djalal Harouni <[email protected]>
>> >> > ---
>> >> > fs/proc/base.c | 56 ++++++++++++++++++++++++++++++++++++++++++++++++++++++
>> >> > fs/proc/internal.h | 2 ++
>> >> > 2 files changed, 58 insertions(+)
>> >> >
>> >> > diff --git a/fs/proc/base.c b/fs/proc/base.c
>> >> > index e834946..c29eeae 100644
>> >> > --- a/fs/proc/base.c
>> >> > +++ b/fs/proc/base.c
>> >> > @@ -168,6 +168,62 @@ int proc_same_open_cred(const struct cred *fcred)
>> >> > cap_issubset(cred->cap_permitted, fcred->cap_permitted));
>> >> > }
>> >> >
>> >> > +/* Returns 0 on success, -errno on denial. */
>> >> > +static int __proc_allow_access(const struct cred *cred,
>> >> > + struct task_struct *task, unsigned int mode)
>> >> > +{
>> >> > + int ret = 0;
>> >> > + const struct cred *tcred;
>> >> > + const struct cred *fcred = cred;
>> >> > +
>> >> > + rcu_read_lock();
>> >> > + tcred = __task_cred(task);
>> >> > + if (uid_eq(fcred->uid, tcred->euid) &&
>> >> > + uid_eq(fcred->uid, tcred->suid) &&
>> >> > + uid_eq(fcred->uid, tcred->uid) &&
>> >> > + gid_eq(fcred->gid, tcred->egid) &&
>> >> > + gid_eq(fcred->gid, tcred->sgid) &&
>> >> > + gid_eq(fcred->gid, tcred->gid))
>> >> > + goto out;
>> >> > +
>> >>
>> >> What's this for? Is it supposed to be an optimization? If so, it looks
>> >> potentially exploitable, although I don't really understand what you're
>> >> trying to do.
>> > This function should be used in addition to the ptrace_may_access() one.
>>
>> Sorry, I was unclear. I meant: what are the uid and gid checks for?
> The uid/gid are checks of the current (reader) on the target task, like
> the ptrace checks. fcred here is the cred of current at open time.
>
This isn't a faithful copy of __ptrace_may_access -- the real function
gives LSMs a chance to veto ptracing. That's critical even without
LSMs because cap_ptrace_access_check needs to get called. (Think
about setcap'd programs instead of setuid programs.)
To fix this, I think you'll need to actually invoke
__ptrace_may_access. That will be a mess because you don't have a
task_struct to pass in, so you'll have to refactor the code to
separately check for task==current and for cred-based permissions.
That, in turn, will mean that you need to get the LSMs to play along,
which includes Yama. To fix that, you'll probably need to check
yama's task-based constraints at open time, which may be at least as
complicated as the revoke-based approach.
--Andy
On Thu, Oct 3, 2013 at 1:29 PM, Djalal Harouni <[email protected]> wrote:
> On Thu, Oct 03, 2013 at 08:12:44AM +0200, Ingo Molnar wrote:
>>
>> * Djalal Harouni <[email protected]> wrote:
>>
>> > > Regardless, glibc uses /proc/self/maps, which would be fine here, right?
>> >
>> > I did not touch /proc/self/maps and others, but I'm planning to fix them
>> > if this solution is accepted.
>> >
>> > I'll do the same thing as in /proc/*/stat for maps, let it be 0444, and
>> > try to delay the check to ->read(). So during ->read() perform
>> > ptrace_may_access() on currenct's cred and process_allow_access() on
>> > file's opener cred. This should work.
>>
>> Aren't read() time checks fundamentally unrobust? We generally don't do
>> locking on read()s, so such checks - in the general case - are pretty
>> racy.
> For procfs yes, read() time checks are unrobust, the general agreement on
> procfs is checks should be performed during each syscall.
>
> For the locking on read()/write() IMHO there should be locking by design
> for /proc/pid/* entries. Here we are speaking about content that varies,
> data attached to other processes, so there is already some locking
> mechanism, and for sensitive stuff, we must hold the cred mutex. This
> is the standard from the old days of procfs.
>
>
> And yes some of them are racy, but we can improve it, delay the checks.
>
> From old Linux git history, before the initial git repository build, I
> found that some important checks were done right after gathering the info.
>
>
>> Now procfs might be special, as by its nature of a pseudofilesystem it's
>> far more atomic than other filesystems, but still IMHO it's a lot more
>
>
>> robust security design wise to revoke access to objects you should not
>> have a reference to when a security boundary is crossed, than letting
>> stuff leak through and sprinking all the potential cases that may leak
>> information with permission checks ...
> I agree, but those access should also be checked at the beginning, IOW
> during ->open(). revoke will not help if revoke is not involved at all,
> the task /proc entries may still be valide (no execve).
>
> Currently security boundary is crossed for example arbitrary /proc/*/stack
> (and others).
> 1) The target task does not do an execve (no revoke).
> 2) current task will open these files and *want* and *will* pass the fd to a
> more privileged process to pass the ptrace check which is done only during
> ->read().
What does this? Or are you saying that this is a bad thing?
(And *please* don't write software that *depends* on different
processes having different read()/write() permissions on the *same*
struct file. I've already found multiple privilege escalations based
on that, and I'm pretty sure I can find some more.)
>
>
>> It's also probably faster: security boundary crossings are relatively rare
>> slowpaths, while read()s are much more common...
> Hmm, These two are related? can't get rid of permission checks
> especially on this pseudofilesystem!
>
>
>> So please first get consensus on this fundamental design question before
>> spreading your solution to more areas.
> Of course, I did clean the patchset to prove that it will work, and I
> only implemented full protection for /proc/*/{stack,syscall,stat} other
> files will wait.
>
> But Ingo you can't ignore the fact that:
> /proc/*/{stack,syscall} are 0444 mode
> /proc/*/{stack,syscall} do not have ptrace_may_access() during open()
> /proc/*/{stack,syscall} have the ptrace_may_access() during read()
I think everyone agrees that this is broken. We don't agree on the
fix check. (Also, as described in my other email, your approach may
be really hard to get right.)
--Andy
On Thu, Oct 03, 2013 at 04:15:43PM +0100, Andy Lutomirski wrote:
> On Thu, Oct 3, 2013 at 1:29 PM, Djalal Harouni <[email protected]> wrote:
> > On Thu, Oct 03, 2013 at 08:12:44AM +0200, Ingo Molnar wrote:
> >> Now procfs might be special, as by its nature of a pseudofilesystem it's
> >> far more atomic than other filesystems, but still IMHO it's a lot more
> >
> >
> >> robust security design wise to revoke access to objects you should not
> >> have a reference to when a security boundary is crossed, than letting
> >> stuff leak through and sprinking all the potential cases that may leak
> >> information with permission checks ...
> > I agree, but those access should also be checked at the beginning, IOW
> > during ->open(). revoke will not help if revoke is not involved at all,
> > the task /proc entries may still be valide (no execve).
> >
> > Currently security boundary is crossed for example arbitrary /proc/*/stack
> > (and others).
> > 1) The target task does not do an execve (no revoke).
> > 2) current task will open these files and *want* and *will* pass the fd to a
> > more privileged process to pass the ptrace check which is done only during
> > ->read().
>
> What does this? Or are you saying that this is a bad thing?
I'm not sure to understand you, revoke if implemented correctly is not a
bad thing! In the other hand, here I try to explain what if the target task
did not execve, revoke will never be involved, file descriptors are
still valid!
> (And *please* don't write software that *depends* on different
> processes having different read()/write() permissions on the *same*
> struct file. I've already found multiple privilege escalations based
> on that, and I'm pretty sure I can find some more.)
Sorry, can't follow you here! examples related to what we discuss here ?
> >
> >
> >> It's also probably faster: security boundary crossings are relatively rare
> >> slowpaths, while read()s are much more common...
> > Hmm, These two are related? can't get rid of permission checks
> > especially on this pseudofilesystem!
> >
> >
> >> So please first get consensus on this fundamental design question before
> >> spreading your solution to more areas.
> > Of course, I did clean the patchset to prove that it will work, and I
> > only implemented full protection for /proc/*/{stack,syscall,stat} other
> > files will wait.
> >
> > But Ingo you can't ignore the fact that:
> > /proc/*/{stack,syscall} are 0444 mode
> > /proc/*/{stack,syscall} do not have ptrace_may_access() during open()
> > /proc/*/{stack,syscall} have the ptrace_may_access() during read()
>
> I think everyone agrees that this is broken. We don't agree on the
> fix check. (Also, as described in my other email, your approach may
> be really hard to get right.)
Well, yes we don't agree perhaps on the fix, but currently there are no
other fixes, will be happy to see other propositions! these files have
been vulnerable for years now...
And for the record it's not my approache. Please just read the emails
correctly. It was proposed and suggested by Eric and perhaps Linus.
I did an experiment with it, and found it easy without any extra
overhead: If cred have changed do extra checkes on the original opener.
It will let you pass file descritors if cred did not change.
Where is this other email that says this approach is hard?
It's not hard, very minor change and it works. Perhaps there is a
better solution yes, but currently it's not implemented!
If you see any bug or if it's not right: then please show us an example,
that's it.
Thanks Andy
> --Andy
--
Djalal Harouni
http://opendz.org
On Thu, Oct 3, 2013 at 4:40 PM, Djalal Harouni <[email protected]> wrote:
> On Thu, Oct 03, 2013 at 04:15:43PM +0100, Andy Lutomirski wrote:
>> On Thu, Oct 3, 2013 at 1:29 PM, Djalal Harouni <[email protected]> wrote:
>> > On Thu, Oct 03, 2013 at 08:12:44AM +0200, Ingo Molnar wrote:
>> >> Now procfs might be special, as by its nature of a pseudofilesystem it's
>> >> far more atomic than other filesystems, but still IMHO it's a lot more
>> >
>> >
>> >> robust security design wise to revoke access to objects you should not
>> >> have a reference to when a security boundary is crossed, than letting
>> >> stuff leak through and sprinking all the potential cases that may leak
>> >> information with permission checks ...
>> > I agree, but those access should also be checked at the beginning, IOW
>> > during ->open(). revoke will not help if revoke is not involved at all,
>> > the task /proc entries may still be valide (no execve).
>> >
>> > Currently security boundary is crossed for example arbitrary /proc/*/stack
>> > (and others).
>> > 1) The target task does not do an execve (no revoke).
>> > 2) current task will open these files and *want* and *will* pass the fd to a
>> > more privileged process to pass the ptrace check which is done only during
>> > ->read().
>>
>> What does this? Or are you saying that this is a bad thing?
> I'm not sure to understand you, revoke if implemented correctly is not a
> bad thing! In the other hand, here I try to explain what if the target task
> did not execve, revoke will never be involved, file descriptors are
> still valid!
Ah. You're saying that both revoke and checking permissions at open
time (or using f_cred) is important. I think I agree. (Except that,
arguably, /proc/self/stat should always be fully functional even if
passed to a different process and yama is in use. This seems minor.)
>
>
>> (And *please* don't write software that *depends* on different
>> processes having different read()/write() permissions on the *same*
>> struct file. I've already found multiple privilege escalations based
>> on that, and I'm pretty sure I can find some more.)
> Sorry, can't follow you here! examples related to what we discuss here ?
There were various bugs (CVE-2013-1959) in /proc/pid/uid_map, etc,
that were exploitable to obtain uid 0. They happened because write()
checked its caller's credentials.
>
>
>> >
>> >
>> >> It's also probably faster: security boundary crossings are relatively rare
>> >> slowpaths, while read()s are much more common...
>> > Hmm, These two are related? can't get rid of permission checks
>> > especially on this pseudofilesystem!
>> >
>> >
>> >> So please first get consensus on this fundamental design question before
>> >> spreading your solution to more areas.
>> > Of course, I did clean the patchset to prove that it will work, and I
>> > only implemented full protection for /proc/*/{stack,syscall,stat} other
>> > files will wait.
>> >
>> > But Ingo you can't ignore the fact that:
>> > /proc/*/{stack,syscall} are 0444 mode
>> > /proc/*/{stack,syscall} do not have ptrace_may_access() during open()
>> > /proc/*/{stack,syscall} have the ptrace_may_access() during read()
>>
>> I think everyone agrees that this is broken. We don't agree on the
>> fix check. (Also, as described in my other email, your approach may
>> be really hard to get right.)
> Well, yes we don't agree perhaps on the fix, but currently there are no
> other fixes, will be happy to see other propositions! these files have
> been vulnerable for years now...
>
> And for the record it's not my approache. Please just read the emails
> correctly. It was proposed and suggested by Eric and perhaps Linus.
>
> I did an experiment with it, and found it easy without any extra
> overhead: If cred have changed do extra checkes on the original opener.
> It will let you pass file descritors if cred did not change.
>
>
> Where is this other email that says this approach is hard?
> It's not hard, very minor change and it works. Perhaps there is a
> better solution yes, but currently it's not implemented!
I just sent it a couple minutes ago -- it may not have made it yet.
It's here, though:
http://www.openwall.com/lists/kernel-hardening/2013/10/03/9
--Andy
(Andy sorry for the delay, real life...)
On Thu, Oct 03, 2013 at 04:50:54PM +0100, Andy Lutomirski wrote:
> On Thu, Oct 3, 2013 at 4:40 PM, Djalal Harouni <[email protected]> wrote:
> > On Thu, Oct 03, 2013 at 04:15:43PM +0100, Andy Lutomirski wrote:
> >> On Thu, Oct 3, 2013 at 1:29 PM, Djalal Harouni <[email protected]> wrote:
> >> > On Thu, Oct 03, 2013 at 08:12:44AM +0200, Ingo Molnar wrote:
> >> >> Now procfs might be special, as by its nature of a pseudofilesystem it's
> >> >> far more atomic than other filesystems, but still IMHO it's a lot more
> >> >
> >> >
> >> >> robust security design wise to revoke access to objects you should not
> >> >> have a reference to when a security boundary is crossed, than letting
> >> >> stuff leak through and sprinking all the potential cases that may leak
> >> >> information with permission checks ...
> >> > I agree, but those access should also be checked at the beginning, IOW
> >> > during ->open(). revoke will not help if revoke is not involved at all,
> >> > the task /proc entries may still be valide (no execve).
> >> >
> >> > Currently security boundary is crossed for example arbitrary /proc/*/stack
> >> > (and others).
> >> > 1) The target task does not do an execve (no revoke).
> >> > 2) current task will open these files and *want* and *will* pass the fd to a
> >> > more privileged process to pass the ptrace check which is done only during
> >> > ->read().
> >>
> >> What does this? Or are you saying that this is a bad thing?
> > I'm not sure to understand you, revoke if implemented correctly is not a
> > bad thing! In the other hand, here I try to explain what if the target task
> > did not execve, revoke will never be involved, file descriptors are
> > still valid!
>
> Ah. You're saying that both revoke and checking permissions at open
> time (or using f_cred) is important. I think I agree. (Except that,
> arguably, /proc/self/stat should always be fully functional even if
> passed to a different process and yama is in use. This seems minor.)
Yes, ok
And I do agree on the /proc/self/stat, it should always work, and it
does with this series. Permissions on f_cred are checked only if
current's cred change between ->open() and ->read(), and this check may
succeed, it depends on f_cred! so /proc/self/stat will work.
> >
> >
> >> (And *please* don't write software that *depends* on different
> >> processes having different read()/write() permissions on the *same*
> >> struct file. I've already found multiple privilege escalations based
> >> on that, and I'm pretty sure I can find some more.)
> > Sorry, can't follow you here! examples related to what we discuss here ?
>
> There were various bugs (CVE-2013-1959) in /proc/pid/uid_map, etc,
> that were exploitable to obtain uid 0. They happened because write()
> checked its caller's credentials.
Ok, will recheck all of them soon. Thanks Andy.
Oh Andy, take a look at commit 935d8aabd4331f47 by Linus
Add file_ns_capable() helper function for open-time capability checking
Don't you see the same change as from my patches:
file_ns_capable() it uses the file->f_cred ! and yes it uses
security_capable() as with the patches I proposed... but in the code I
touched there is a need for security_capable_noaudit() also, I think.
Same logic! file->f_cred is already beeing/planned to be used.
That also goes for commit 6708075f104c3c9b0 by Eric,
userns: Don't let unprivileged users trick privileged users into setting the id_map
So Andy, what do you think? file->f_cred is already used to fix urgent
vulnerabilities, and now, everyone here knows that /proc/*/{stack,maps}
cab be used to leak ASLR...
[...]
> >> > Of course, I did clean the patchset to prove that it will work, and I
> >> > only implemented full protection for /proc/*/{stack,syscall,stat} other
> >> > files will wait.
> >> >
> >> > But Ingo you can't ignore the fact that:
> >> > /proc/*/{stack,syscall} are 0444 mode
> >> > /proc/*/{stack,syscall} do not have ptrace_may_access() during open()
> >> > /proc/*/{stack,syscall} have the ptrace_may_access() during read()
> >>
> >> I think everyone agrees that this is broken. We don't agree on the
> >> fix check. (Also, as described in my other email, your approach may
> >> be really hard to get right.)
> > Well, yes we don't agree perhaps on the fix, but currently there are no
> > other fixes, will be happy to see other propositions! these files have
> > been vulnerable for years now...
> >
> > And for the record it's not my approache. Please just read the emails
> > correctly. It was proposed and suggested by Eric and perhaps Linus.
> >
> > I did an experiment with it, and found it easy without any extra
> > overhead: If cred have changed do extra checkes on the original opener.
> > It will let you pass file descritors if cred did not change.
> >
> >
> > Where is this other email that says this approach is hard?
> > It's not hard, very minor change and it works. Perhaps there is a
> > better solution yes, but currently it's not implemented!
>
> I just sent it a couple minutes ago -- it may not have made it yet.
> It's here, though:
>
> http://www.openwall.com/lists/kernel-hardening/2013/10/03/9
Sorry, will respond, thanks.
> --Andy
--
Djalal Harouni
http://opendz.org
On Thu, Oct 03, 2013 at 04:12:37PM +0100, Andy Lutomirski wrote:
> On Thu, Oct 3, 2013 at 3:36 PM, Djalal Harouni <[email protected]> wrote:
> > On Wed, Oct 02, 2013 at 05:44:17PM +0100, Andy Lutomirski wrote:
> >> On Wed, Oct 2, 2013 at 3:55 PM, Djalal Harouni <[email protected]> wrote:
> >> > On Tue, Oct 01, 2013 at 06:36:34PM -0700, Andy Lutomirski wrote:
> >> >> On 10/01/2013 01:26 PM, Djalal Harouni wrote:
> >> >> > Since /proc entries varies at runtime, permission checks need to happen
> >> >> > during each system call.
> >> >> >
> >> >> > However even with that /proc file descriptors can be passed to a more
> >> >> > privileged process (e.g. a suid-exec) which will pass the classic
> >> >> > ptrace_may_access() permission check. The open() call will be issued in
> >> >> > general by an unprivileged process while the disclosure of sensitive
> >> >> > /proc information will happen using a more privileged process at
> >> >> > read(),write()...
> >> >> >
> >> >> > Therfore we need a more sophisticated check to detect if the cred of the
> >> >> > process have changed, and if the cred of the original opener that are
> >> >> > stored in the file->f_cred have enough permission to access the task's
> >> >> > /proc entries during read(), write()...
> >> >> >
> >> >> > Add the proc_allow_access() function that will receive the file->f_cred
> >> >> > as an argument, and tries to check if the opener had enough permission
> >> >> > to access the task's /proc entries.
> >> >> >
> >> >> > This function should be used with the ptrace_may_access() check.
> >> >> >
> >> >> > Cc: Kees Cook <[email protected]>
> >> >> > Suggested-by: Eric W. Biederman <[email protected]>
> >> >> > Signed-off-by: Djalal Harouni <[email protected]>
> >> >> > ---
> >> >> > fs/proc/base.c | 56 ++++++++++++++++++++++++++++++++++++++++++++++++++++++
> >> >> > fs/proc/internal.h | 2 ++
> >> >> > 2 files changed, 58 insertions(+)
> >> >> >
> >> >> > diff --git a/fs/proc/base.c b/fs/proc/base.c
> >> >> > index e834946..c29eeae 100644
> >> >> > --- a/fs/proc/base.c
> >> >> > +++ b/fs/proc/base.c
> >> >> > @@ -168,6 +168,62 @@ int proc_same_open_cred(const struct cred *fcred)
> >> >> > cap_issubset(cred->cap_permitted, fcred->cap_permitted));
> >> >> > }
> >> >> >
> >> >> > +/* Returns 0 on success, -errno on denial. */
> >> >> > +static int __proc_allow_access(const struct cred *cred,
> >> >> > + struct task_struct *task, unsigned int mode)
> >> >> > +{
> >> >> > + int ret = 0;
> >> >> > + const struct cred *tcred;
> >> >> > + const struct cred *fcred = cred;
> >> >> > +
> >> >> > + rcu_read_lock();
> >> >> > + tcred = __task_cred(task);
> >> >> > + if (uid_eq(fcred->uid, tcred->euid) &&
> >> >> > + uid_eq(fcred->uid, tcred->suid) &&
> >> >> > + uid_eq(fcred->uid, tcred->uid) &&
> >> >> > + gid_eq(fcred->gid, tcred->egid) &&
> >> >> > + gid_eq(fcred->gid, tcred->sgid) &&
> >> >> > + gid_eq(fcred->gid, tcred->gid))
> >> >> > + goto out;
> >> >> > +
> >> >>
> >> >> What's this for? Is it supposed to be an optimization? If so, it looks
> >> >> potentially exploitable, although I don't really understand what you're
> >> >> trying to do.
> >> > This function should be used in addition to the ptrace_may_access() one.
> >>
> >> Sorry, I was unclear. I meant: what are the uid and gid checks for?
> > The uid/gid are checks of the current (reader) on the target task, like
> > the ptrace checks. fcred here is the cred of current at open time.
> >
>
> This isn't a faithful copy of __ptrace_may_access -- the real function
> gives LSMs a chance to veto ptracing. That's critical even without
> LSMs because cap_ptrace_access_check needs to get called. (Think
> about setcap'd programs instead of setuid programs.)
Yes, I already did this, not only setuid, capabilities also are handled
See the whole patch, please!
Yes, and speaking about LSMs I've mentioned in my patches and doc, that
the proposed function proc_allow_access() should be used after
ptrace_may_access(). proc_allow_access() is not a replacement for
ptrace_may_access(), it should be used *after* it.
So cap_ptrace_access_check() is called, and before the file->f_cred
checks. The LSM veto is already there.
Don't forget that proc_allow_access() is not for ptracing, it's targeted
to the procfs entries! and who opened the file.
Finally take a look at what you say about: cap_ptrace_access_check()
it does:
1)
if (cred->user_ns == child_cred->user_ns &&
cap_issubset(child_cred->cap_permitted, cred->cap_permitted))
return 0
2)
if (ns_capable(child_cred->user_ns, CAP_SYS_PTRACE))
return 0
ns_capable() calls security_capable().
And take a look at what I did, Andy please:
[PATCH v2 1/9] procfs: add proc_same_open_cred() to check if the cred
have changed
proc_same_open_cred() returns 1 on success
1) for proc_same_open_cred()
if (f_cred->user_ns != cred->user_ns)
return 0
return (uid_eq(fcred->uid, cred->uid) &&
gid_eq(fcred->gid, cred->gid) &&
cap_issubset(cred->cap_permitted, f_cred->cap_permitted));
So it handles the (1) of cap_ptrace_access_check()
If this previous proc_same_open_cred() returns 0 (cred have changed)
goto (2) [PATCH v2 2/9] procfs: add proc_allow_access() to check if
file's opener may access task
proc_allow_access() returns 1 on success
2) It does the uid/gid checks which is complete
Later it does security_capable() check which will do:
security_ops->capable(f_cred, ns, cap, SECURITY_CAP_AUDIT);
Take the old f_cred of file opener and do the capability check
during this moment, ->read(),->write()
So LSM support, and if LSM is disabled then we have the cap_capable()
which will go and check userns and capabilities.
Andy if you see that I've missed something please let me know
So Andy the solution is complete and I think ! can you please recheck
(I'll also recheck it).
> To fix this, I think you'll need to actually invoke
> __ptrace_may_access. That will be a mess because you don't have a
> task_struct to pass in, so you'll have to refactor the code to
> separately check for task==current and for cred-based permissions.
> That, in turn, will mean that you need to get the LSMs to play along,
> which includes Yama. To fix that, you'll probably need to check
> yama's task-based constraints at open time, which may be at least as
> complicated as the revoke-based approach.
As I've demonstrated above, no mess and it's not compilcated, very easy
to support :-)
Thanks Andy
> --Andy
--
Djalal Harouni
http://opendz.org
On Thu, Oct 3, 2013 at 12:29 PM, Djalal Harouni <[email protected]> wrote:
> On Thu, Oct 03, 2013 at 04:12:37PM +0100, Andy Lutomirski wrote:
>> On Thu, Oct 3, 2013 at 3:36 PM, Djalal Harouni <[email protected]> wrote:
>> > On Wed, Oct 02, 2013 at 05:44:17PM +0100, Andy Lutomirski wrote:
>> >> On Wed, Oct 2, 2013 at 3:55 PM, Djalal Harouni <[email protected]> wrote:
>> >> > On Tue, Oct 01, 2013 at 06:36:34PM -0700, Andy Lutomirski wrote:
>> >> >> On 10/01/2013 01:26 PM, Djalal Harouni wrote:
>> >> >> > Since /proc entries varies at runtime, permission checks need to happen
>> >> >> > during each system call.
>> >> >> >
>> >> >> > However even with that /proc file descriptors can be passed to a more
>> >> >> > privileged process (e.g. a suid-exec) which will pass the classic
>> >> >> > ptrace_may_access() permission check. The open() call will be issued in
>> >> >> > general by an unprivileged process while the disclosure of sensitive
>> >> >> > /proc information will happen using a more privileged process at
>> >> >> > read(),write()...
>> >> >> >
>> >> >> > Therfore we need a more sophisticated check to detect if the cred of the
>> >> >> > process have changed, and if the cred of the original opener that are
>> >> >> > stored in the file->f_cred have enough permission to access the task's
>> >> >> > /proc entries during read(), write()...
>> >> >> >
>> >> >> > Add the proc_allow_access() function that will receive the file->f_cred
>> >> >> > as an argument, and tries to check if the opener had enough permission
>> >> >> > to access the task's /proc entries.
>> >> >> >
>> >> >> > This function should be used with the ptrace_may_access() check.
>> >> >> >
>> >> >> > Cc: Kees Cook <[email protected]>
>> >> >> > Suggested-by: Eric W. Biederman <[email protected]>
>> >> >> > Signed-off-by: Djalal Harouni <[email protected]>
>> >> >> > ---
>> >> >> > fs/proc/base.c | 56 ++++++++++++++++++++++++++++++++++++++++++++++++++++++
>> >> >> > fs/proc/internal.h | 2 ++
>> >> >> > 2 files changed, 58 insertions(+)
>> >> >> >
>> >> >> > diff --git a/fs/proc/base.c b/fs/proc/base.c
>> >> >> > index e834946..c29eeae 100644
>> >> >> > --- a/fs/proc/base.c
>> >> >> > +++ b/fs/proc/base.c
>> >> >> > @@ -168,6 +168,62 @@ int proc_same_open_cred(const struct cred *fcred)
>> >> >> > cap_issubset(cred->cap_permitted, fcred->cap_permitted));
>> >> >> > }
>> >> >> >
>> >> >> > +/* Returns 0 on success, -errno on denial. */
>> >> >> > +static int __proc_allow_access(const struct cred *cred,
>> >> >> > + struct task_struct *task, unsigned int mode)
>> >> >> > +{
>> >> >> > + int ret = 0;
>> >> >> > + const struct cred *tcred;
>> >> >> > + const struct cred *fcred = cred;
>> >> >> > +
>> >> >> > + rcu_read_lock();
>> >> >> > + tcred = __task_cred(task);
>> >> >> > + if (uid_eq(fcred->uid, tcred->euid) &&
>> >> >> > + uid_eq(fcred->uid, tcred->suid) &&
>> >> >> > + uid_eq(fcred->uid, tcred->uid) &&
>> >> >> > + gid_eq(fcred->gid, tcred->egid) &&
>> >> >> > + gid_eq(fcred->gid, tcred->sgid) &&
>> >> >> > + gid_eq(fcred->gid, tcred->gid))
>> >> >> > + goto out;
>> >> >> > +
>> >> >>
>> >> >> What's this for? Is it supposed to be an optimization? If so, it looks
>> >> >> potentially exploitable, although I don't really understand what you're
>> >> >> trying to do.
>> >> > This function should be used in addition to the ptrace_may_access() one.
>> >>
>> >> Sorry, I was unclear. I meant: what are the uid and gid checks for?
>> > The uid/gid are checks of the current (reader) on the target task, like
>> > the ptrace checks. fcred here is the cred of current at open time.
>> >
>>
>> This isn't a faithful copy of __ptrace_may_access -- the real function
>> gives LSMs a chance to veto ptracing. That's critical even without
>> LSMs because cap_ptrace_access_check needs to get called. (Think
>> about setcap'd programs instead of setuid programs.)
> Yes, I already did this, not only setuid, capabilities also are handled
> See the whole patch, please!
>
>
> Yes, and speaking about LSMs I've mentioned in my patches and doc, that
> the proposed function proc_allow_access() should be used after
> ptrace_may_access(). proc_allow_access() is not a replacement for
> ptrace_may_access(), it should be used *after* it.
>
> So cap_ptrace_access_check() is called, and before the file->f_cred
> checks. The LSM veto is already there.
It's possible that I've misunderstood your patches, but I really don't
see where you're calling into LSMs to give them a chance to veto
access by *f_cred*.
> 1) for proc_same_open_cred()
> if (f_cred->user_ns != cred->user_ns)
> return 0
>
> return (uid_eq(fcred->uid, cred->uid) &&
> gid_eq(fcred->gid, cred->gid) &&
> cap_issubset(cred->cap_permitted, f_cred->cap_permitted));
>
> So it handles the (1) of cap_ptrace_access_check()
No. This just means that, if there's a possibility that the caps are
wrong, you invoke ptrace_allow_access, which *does not re-check
capabilities*.
--Andy
On Thu, Oct 03, 2013 at 12:37:49PM -0700, Andy Lutomirski wrote:
> On Thu, Oct 3, 2013 at 12:29 PM, Djalal Harouni <[email protected]> wrote:
> > On Thu, Oct 03, 2013 at 04:12:37PM +0100, Andy Lutomirski wrote:
> >> On Thu, Oct 3, 2013 at 3:36 PM, Djalal Harouni <[email protected]> wrote:
> >> > On Wed, Oct 02, 2013 at 05:44:17PM +0100, Andy Lutomirski wrote:
> >> >> On Wed, Oct 2, 2013 at 3:55 PM, Djalal Harouni <[email protected]> wrote:
> >> >> > On Tue, Oct 01, 2013 at 06:36:34PM -0700, Andy Lutomirski wrote:
> >> >> >> On 10/01/2013 01:26 PM, Djalal Harouni wrote:
> >> >> >> > Since /proc entries varies at runtime, permission checks need to happen
> >> >> >> > during each system call.
> >> >> >> >
> >> >> >> > However even with that /proc file descriptors can be passed to a more
> >> >> >> > privileged process (e.g. a suid-exec) which will pass the classic
> >> >> >> > ptrace_may_access() permission check. The open() call will be issued in
> >> >> >> > general by an unprivileged process while the disclosure of sensitive
> >> >> >> > /proc information will happen using a more privileged process at
> >> >> >> > read(),write()...
> >> >> >> >
> >> >> >> > Therfore we need a more sophisticated check to detect if the cred of the
> >> >> >> > process have changed, and if the cred of the original opener that are
> >> >> >> > stored in the file->f_cred have enough permission to access the task's
> >> >> >> > /proc entries during read(), write()...
> >> >> >> >
> >> >> >> > Add the proc_allow_access() function that will receive the file->f_cred
> >> >> >> > as an argument, and tries to check if the opener had enough permission
> >> >> >> > to access the task's /proc entries.
> >> >> >> >
> >> >> >> > This function should be used with the ptrace_may_access() check.
> >> >> >> >
> >> >> >> > Cc: Kees Cook <[email protected]>
> >> >> >> > Suggested-by: Eric W. Biederman <[email protected]>
> >> >> >> > Signed-off-by: Djalal Harouni <[email protected]>
> >> >> >> > ---
> >> >> >> > fs/proc/base.c | 56 ++++++++++++++++++++++++++++++++++++++++++++++++++++++
> >> >> >> > fs/proc/internal.h | 2 ++
> >> >> >> > 2 files changed, 58 insertions(+)
> >> >> >> >
> >> >> >> > diff --git a/fs/proc/base.c b/fs/proc/base.c
> >> >> >> > index e834946..c29eeae 100644
> >> >> >> > --- a/fs/proc/base.c
> >> >> >> > +++ b/fs/proc/base.c
> >> >> >> > @@ -168,6 +168,62 @@ int proc_same_open_cred(const struct cred *fcred)
> >> >> >> > cap_issubset(cred->cap_permitted, fcred->cap_permitted));
> >> >> >> > }
> >> >> >> >
> >> >> >> > +/* Returns 0 on success, -errno on denial. */
> >> >> >> > +static int __proc_allow_access(const struct cred *cred,
> >> >> >> > + struct task_struct *task, unsigned int mode)
> >> >> >> > +{
> >> >> >> > + int ret = 0;
> >> >> >> > + const struct cred *tcred;
> >> >> >> > + const struct cred *fcred = cred;
> >> >> >> > +
> >> >> >> > + rcu_read_lock();
> >> >> >> > + tcred = __task_cred(task);
> >> >> >> > + if (uid_eq(fcred->uid, tcred->euid) &&
> >> >> >> > + uid_eq(fcred->uid, tcred->suid) &&
> >> >> >> > + uid_eq(fcred->uid, tcred->uid) &&
> >> >> >> > + gid_eq(fcred->gid, tcred->egid) &&
> >> >> >> > + gid_eq(fcred->gid, tcred->sgid) &&
> >> >> >> > + gid_eq(fcred->gid, tcred->gid))
> >> >> >> > + goto out;
> >> >> >> > +
> >> >> >>
> >> >> >> What's this for? Is it supposed to be an optimization? If so, it looks
> >> >> >> potentially exploitable, although I don't really understand what you're
> >> >> >> trying to do.
> >> >> > This function should be used in addition to the ptrace_may_access() one.
> >> >>
> >> >> Sorry, I was unclear. I meant: what are the uid and gid checks for?
> >> > The uid/gid are checks of the current (reader) on the target task, like
> >> > the ptrace checks. fcred here is the cred of current at open time.
> >> >
> >>
> >> This isn't a faithful copy of __ptrace_may_access -- the real function
> >> gives LSMs a chance to veto ptracing. That's critical even without
> >> LSMs because cap_ptrace_access_check needs to get called. (Think
> >> about setcap'd programs instead of setuid programs.)
> > Yes, I already did this, not only setuid, capabilities also are handled
> > See the whole patch, please!
> >
> >
> > Yes, and speaking about LSMs I've mentioned in my patches and doc, that
> > the proposed function proc_allow_access() should be used after
> > ptrace_may_access(). proc_allow_access() is not a replacement for
> > ptrace_may_access(), it should be used *after* it.
> >
> > So cap_ptrace_access_check() is called, and before the file->f_cred
> > checks. The LSM veto is already there.
>
> It's possible that I've misunderstood your patches, but I really don't
> see where you're calling into LSMs to give them a chance to veto
> access by *f_cred*.
Ahh ok, I see, but why you want absolutly to put *f_cred* in this ?
That's not its job, LSM veto is handled during read() correctly before
proc_allow_access() and f_cred check. And if you want to do it correctly
then f_cred should be handled during its time, during ->open().
The correct way to handle it: ptrace_may_access() during ->open() and
each syscall for sensitive files.
Why add and speak about all this complexity where the correct check is
just add ptrace_may_access() during ->open() ? using *f_cred* in this
context and bring it here is not a valid argument IMO.
>
> > 1) for proc_same_open_cred()
> > if (f_cred->user_ns != cred->user_ns)
> > return 0
> >
> > return (uid_eq(fcred->uid, cred->uid) &&
> > gid_eq(fcred->gid, cred->gid) &&
> > cap_issubset(cred->cap_permitted, f_cred->cap_permitted));
> >
> > So it handles the (1) of cap_ptrace_access_check()
>
> No. This just means that, if there's a possibility that the caps are
> wrong, you invoke ptrace_allow_access, which *does not re-check
> capabilities*.
Ohh Andy, we do check capabilities, please Andy are you looking to the
code ?
If the uid/gid match fails we do check capability, if it succeed why we
would check them ? same user!
+static int __proc_allow_access(const struct cred *cred,
+ struct task_struct *task, unsigned int
mode)
+{
+ int ret = 0;
+ const struct cred *tcred;
+ const struct cred *fcred = cred;
+
+ rcu_read_lock();
+ tcred = __task_cred(task);
+ if (uid_eq(fcred->uid, tcred->euid) &&
+ uid_eq(fcred->uid, tcred->suid) &&
+ uid_eq(fcred->uid, tcred->uid) &&
+ gid_eq(fcred->gid, tcred->egid) &&
+ gid_eq(fcred->gid, tcred->sgid) &&
+ gid_eq(fcred->gid, tcred->gid))
+ goto out;
+
+ if (mode & PTRACE_MODE_NOAUDIT)
+ ret = security_capable_noaudit(fcred, tcred->user_ns,
+ CAP_SYS_PTRACE);
+ else
+ ret = security_capable(fcred, tcred->user_ns,
+ CAP_SYS_PTRACE);
+
+out:
+ rcu_read_unlock();
+ return !ret ? ret : -EPERM;
+}
The patch was posted, this is a re-post!
> --Andy
--
Djalal Harouni
http://opendz.org
On Thu, Oct 3, 2013 at 1:13 PM, Djalal Harouni <[email protected]> wrote:
> On Thu, Oct 03, 2013 at 12:37:49PM -0700, Andy Lutomirski wrote:
>> On Thu, Oct 3, 2013 at 12:29 PM, Djalal Harouni <[email protected]> wrote:
>> > On Thu, Oct 03, 2013 at 04:12:37PM +0100, Andy Lutomirski wrote:
>> >> On Thu, Oct 3, 2013 at 3:36 PM, Djalal Harouni <[email protected]> wrote:
>> >> > On Wed, Oct 02, 2013 at 05:44:17PM +0100, Andy Lutomirski wrote:
>> >> >> On Wed, Oct 2, 2013 at 3:55 PM, Djalal Harouni <[email protected]> wrote:
>> >> >> > On Tue, Oct 01, 2013 at 06:36:34PM -0700, Andy Lutomirski wrote:
>> >> >> >> On 10/01/2013 01:26 PM, Djalal Harouni wrote:
>> >> >> >> > Since /proc entries varies at runtime, permission checks need to happen
>> >> >> >> > during each system call.
>> >> >> >> >
>> >> >> >> > However even with that /proc file descriptors can be passed to a more
>> >> >> >> > privileged process (e.g. a suid-exec) which will pass the classic
>> >> >> >> > ptrace_may_access() permission check. The open() call will be issued in
>> >> >> >> > general by an unprivileged process while the disclosure of sensitive
>> >> >> >> > /proc information will happen using a more privileged process at
>> >> >> >> > read(),write()...
>> >> >> >> >
>> >> >> >> > Therfore we need a more sophisticated check to detect if the cred of the
>> >> >> >> > process have changed, and if the cred of the original opener that are
>> >> >> >> > stored in the file->f_cred have enough permission to access the task's
>> >> >> >> > /proc entries during read(), write()...
>> >> >> >> >
>> >> >> >> > Add the proc_allow_access() function that will receive the file->f_cred
>> >> >> >> > as an argument, and tries to check if the opener had enough permission
>> >> >> >> > to access the task's /proc entries.
>> >> >> >> >
>> >> >> >> > This function should be used with the ptrace_may_access() check.
>> >> >> >> >
>> >> >> >> > Cc: Kees Cook <[email protected]>
>> >> >> >> > Suggested-by: Eric W. Biederman <[email protected]>
>> >> >> >> > Signed-off-by: Djalal Harouni <[email protected]>
>> >> >> >> > ---
>> >> >> >> > fs/proc/base.c | 56 ++++++++++++++++++++++++++++++++++++++++++++++++++++++
>> >> >> >> > fs/proc/internal.h | 2 ++
>> >> >> >> > 2 files changed, 58 insertions(+)
>> >> >> >> >
>> >> >> >> > diff --git a/fs/proc/base.c b/fs/proc/base.c
>> >> >> >> > index e834946..c29eeae 100644
>> >> >> >> > --- a/fs/proc/base.c
>> >> >> >> > +++ b/fs/proc/base.c
>> >> >> >> > @@ -168,6 +168,62 @@ int proc_same_open_cred(const struct cred *fcred)
>> >> >> >> > cap_issubset(cred->cap_permitted, fcred->cap_permitted));
>> >> >> >> > }
>> >> >> >> >
>> >> >> >> > +/* Returns 0 on success, -errno on denial. */
>> >> >> >> > +static int __proc_allow_access(const struct cred *cred,
>> >> >> >> > + struct task_struct *task, unsigned int mode)
>> >> >> >> > +{
>> >> >> >> > + int ret = 0;
>> >> >> >> > + const struct cred *tcred;
>> >> >> >> > + const struct cred *fcred = cred;
>> >> >> >> > +
>> >> >> >> > + rcu_read_lock();
>> >> >> >> > + tcred = __task_cred(task);
>> >> >> >> > + if (uid_eq(fcred->uid, tcred->euid) &&
>> >> >> >> > + uid_eq(fcred->uid, tcred->suid) &&
>> >> >> >> > + uid_eq(fcred->uid, tcred->uid) &&
>> >> >> >> > + gid_eq(fcred->gid, tcred->egid) &&
>> >> >> >> > + gid_eq(fcred->gid, tcred->sgid) &&
>> >> >> >> > + gid_eq(fcred->gid, tcred->gid))
>> >> >> >> > + goto out;
>> >> >> >> > +
>> >> >> >>
>> >> >> >> What's this for? Is it supposed to be an optimization? If so, it looks
>> >> >> >> potentially exploitable, although I don't really understand what you're
>> >> >> >> trying to do.
>> >> >> > This function should be used in addition to the ptrace_may_access() one.
>> >> >>
>> >> >> Sorry, I was unclear. I meant: what are the uid and gid checks for?
>> >> > The uid/gid are checks of the current (reader) on the target task, like
>> >> > the ptrace checks. fcred here is the cred of current at open time.
>> >> >
>> >>
>> >> This isn't a faithful copy of __ptrace_may_access -- the real function
>> >> gives LSMs a chance to veto ptracing. That's critical even without
>> >> LSMs because cap_ptrace_access_check needs to get called. (Think
>> >> about setcap'd programs instead of setuid programs.)
>> > Yes, I already did this, not only setuid, capabilities also are handled
>> > See the whole patch, please!
>> >
>> >
>> > Yes, and speaking about LSMs I've mentioned in my patches and doc, that
>> > the proposed function proc_allow_access() should be used after
>> > ptrace_may_access(). proc_allow_access() is not a replacement for
>> > ptrace_may_access(), it should be used *after* it.
>> >
>> > So cap_ptrace_access_check() is called, and before the file->f_cred
>> > checks. The LSM veto is already there.
>>
>> It's possible that I've misunderstood your patches, but I really don't
>> see where you're calling into LSMs to give them a chance to veto
>> access by *f_cred*.
> Ahh ok, I see, but why you want absolutly to put *f_cred* in this ?
>
> That's not its job, LSM veto is handled during read() correctly before
> proc_allow_access() and f_cred check. And if you want to do it correctly
> then f_cred should be handled during its time, during ->open().
> The correct way to handle it: ptrace_may_access() during ->open() and
> each syscall for sensitive files.
>
> Why add and speak about all this complexity where the correct check is
> just add ptrace_may_access() during ->open() ? using *f_cred* in this
> context and bring it here is not a valid argument IMO.
I don't want to put f_cred into this. I'd rather the patches just
check everything at open() time. Doing that will require some form of
revocation, I think.
Your current patches use f_cred, and they seem to do it wrong. So
please either fix it, stop using f_cred, or explain why it it's okay
despite not invoking LSM in the expected way.
--Andy
On Wed, Oct 2, 2013 at 11:07 AM, Kees Cook <[email protected]> wrote:
>
> On Wed, Oct 2, 2013 at 11:00 AM, Andy Lutomirski <[email protected]> wrote:
> > On Wed, Oct 2, 2013 at 10:48 AM, Kees Cook <[email protected]> wrote:
> >> On Wed, Oct 2, 2013 at 9:51 AM, Andy Lutomirski <[email protected]> wrote:
> >>> On Wed, Oct 2, 2013 at 3:37 PM, Djalal Harouni <[email protected]> wrote:
> >>>> On Tue, Oct 01, 2013 at 06:40:41PM -0700, Andy Lutomirski wrote:
> >>>>> On 10/01/2013 01:26 PM, Djalal Harouni wrote:
> >>>>> > /proc/<pid>/* entries varies at runtime, appropriate permission checks
> >>>>> > need to happen during each system call.
> >>>>> >
> >>>>> > Currently some of these sensitive entries are protected by performing
> >>>>> > the ptrace_may_access() check. However even with that the /proc file
> >>>>> > descriptors can be passed to a more privileged process
> >>>>> > (e.g. a suid-exec) which will pass the classic ptrace_may_access()
> >>>>> > check. In general the ->open() call will be issued by an unprivileged
> >>>>> > process while the ->read(),->write() calls by a more privileged one.
> >>>>> >
> >>>>> > Example of these files are:
> >>>>> > /proc/*/syscall, /proc/*/stack etc.
> >>>>> >
> >>>>> > And any open(/proc/self/*) then suid-exec to read()/write() /proc/self/*
> >>>>> >
> >>>>> >
> >>>>> > These files are protected during read() by the ptrace_may_access(),
> >>>>> > however the file descriptor can be passed to a suid-exec which can be
> >>>>> > used to read data and bypass ASLR. Of course this was discussed several
> >>>>> > times on LKML.
> >>>>>
> >>>>> Can you elaborate on what it is that you're fixing? That is, can you
> >>>>> give a concrete example of what process opens what file and passes the
> >>>>> fd to what process?
> >>>> Yes, the references were already given in this email:
> >>>> https://lkml.org/lkml/2013/8/31/209
> >>>>
> >>>> This has been discussed several times on lkml:
> >>>> https://lkml.org/lkml/2013/8/28/544
> >>>>
> >>>> https://lkml.org/lkml/2013/8/28/564 (check Kees's references)
> >>>>
> >>>>
> >>>>> I'm having trouble following your description.
> >>>> Process open a /proc file and pass the fd to a more privilaged process
> >>>> that will pass the ptrace_may_access() check, while the original process
> >>>> that opened that file should fail at the ptrace_may_access()
> >>>
> >>> So we're talking about two kinds of attacks, right?
> >>
> >> Correct.
> >>
> >>> Type 1: Unprivileged process does something like open("/proc/1/maps",
> >>> O_RDONLY) and then passes the resulting fd to something privileged.
> >>
> >> ... and then leaks contents back to unprivileged process.
> >>
> >>> Type 2: Unprivileged process does something like
> >>> open("/proc/self/maps", O_RDONLY) and then forks. The parent calls
> >>> execve on something privileged.
> >>
> >> ... and then parent snoops on file contents for the privileged child.
> >>
> >> (Type 2 is solved currently, IIUC. Type 1 could be reduced in scope by
> >> changing these file modes back to 0400.)
> >>
> >>> Can we really not get away with fixing type 1 by preventing these
> >>> files from being opened in the first place and type 2 by revoking all
> >>> of these fds when a privilege-changing exec happens?
> >>
> >> Type 1 can be done via exec as well. Instead of using a priv exec to
> >> read an arbitrary process, read it could read its own.
> >
> > Right.
> >
> >>
> >> I think revoking the fd would be great. Does that mechanism exist?
> >
> > There's this thing that never got merged.
> >
> > http://thread.gmane.org/gmane.linux.kernel/1523331
> >
> > But doing it more directly should be reasonably straightforward. Either:
> >
> > (a) when a process execs and privileges change, find all the old proc
> > inodes, mark them dead, and unlink them, or
> >
> > (b) add self_exec_id to all the proc file private_data entries (or
> > somewhere else). Then just make sure that they're unchanged. I think
> > the bug last time around was because the self_exec_id and struct pid
> > weren't being compared together.
> >
> > (a) is probably nicer. I don't know if it'll break things. Linus
> > seemed to think that the Chrome sandbox was sensitive to this stuff,
> > but I don't know why.
>
> I agree, (a) seems much cleaner. Hm, I don't think Chrome does
> anything with these sensitive files (maps, stack, syscall, etc). But
> let's ask Julien. :)
>
> Julien, do you see any problem with Chrome's sandbox behavior if these
> proc files would be unavailable across privilege changes?
There is nothing that currently jumps to mind in Chromium. However,
anything that breaks "file descriptors are capabilities" inevitably
ends-up breaking something.
For instance, I could easily imagine breakage because a process uses
PR_SET_DUMPABLE (more so than, say, transitions to uid 0) while its
/proc entries are being monitored by another part of the same
application.
Please cc:me on patches.
Julien
On Thu, Oct 03, 2013 at 02:09:55PM -0700, Andy Lutomirski wrote:
> On Thu, Oct 3, 2013 at 1:13 PM, Djalal Harouni <[email protected]> wrote:
> > On Thu, Oct 03, 2013 at 12:37:49PM -0700, Andy Lutomirski wrote:
> >> On Thu, Oct 3, 2013 at 12:29 PM, Djalal Harouni <[email protected]> wrote:
> >> > On Thu, Oct 03, 2013 at 04:12:37PM +0100, Andy Lutomirski wrote:
> >> >> On Thu, Oct 3, 2013 at 3:36 PM, Djalal Harouni <[email protected]> wrote:
> >> > Yes, I already did this, not only setuid, capabilities also are handled
> >> > See the whole patch, please!
> >> >
> >> >
> >> > Yes, and speaking about LSMs I've mentioned in my patches and doc, that
> >> > the proposed function proc_allow_access() should be used after
> >> > ptrace_may_access(). proc_allow_access() is not a replacement for
> >> > ptrace_may_access(), it should be used *after* it.
> >> >
> >> > So cap_ptrace_access_check() is called, and before the file->f_cred
> >> > checks. The LSM veto is already there.
> >>
> >> It's possible that I've misunderstood your patches, but I really don't
> >> see where you're calling into LSMs to give them a chance to veto
> >> access by *f_cred*.
> > Ahh ok, I see, but why you want absolutly to put *f_cred* in this ?
> >
> > That's not its job, LSM veto is handled during read() correctly before
> > proc_allow_access() and f_cred check. And if you want to do it correctly
> > then f_cred should be handled during its time, during ->open().
> > The correct way to handle it: ptrace_may_access() during ->open() and
> > each syscall for sensitive files.
> >
> > Why add and speak about all this complexity where the correct check is
> > just add ptrace_may_access() during ->open() ? using *f_cred* in this
> > context and bring it here is not a valid argument IMO.
>
> I don't want to put f_cred into this. I'd rather the patches just
> check everything at open() time. Doing that will require some form of
> revocation, I think.
>
> Your current patches use f_cred, and they seem to do it wrong. So
The current patches block and protect the current attacks correctly,
without overhead.
Example:
proc_uid_map_write()
-> map_write()
-> file_ns_capable()
-> security_capable(file->f_cred, ns, cap)
file_ns_capable() added in commit 935d8aabd4331 by Linus
Add file_ns_capable() helper function for open-time capability checking
That also goes for commit 6708075f104c3c9b0 by Eric,
userns: Don't let unprivileged users trick privileged users into setting
the id_map
The proc_allow_access() function that I've proposed has the same logic
of file_ns_capable(), We can even put file_ns_capable() inside
proc_allow_access(). We'll add support of security_capable_noaudit()
inside file_ns_capable() and proc_allow_access() will be much more
better.
file_ns_capable() checks where a capability was there,
proc_allow_access() checks where they have same uid + if capability was
there.
> please either fix it, stop using f_cred, or explain why it it's okay
> despite not invoking LSM in the expected way.
I've already explained it.
LSM is handled by ptrace_may_access() which should be called during
->open() to handle f_cred, and during ->read() to handle current's cred.
proc_allow_access() can't be called during ->open(), there is not reason
for that.
Now if you have read the patch correctly, perhaps you have noticed again
that's is already done!
[PATCH v2 8/9] procfs: improve permission checks on /proc/*/stack
[PATCH v2 9/9] procfs: improve permission checks on /proc/*/syscall
Both of these are sensitive files, it's like you are ptracing the
target! so did you check that for these two files I've added their
corresponding ->open(), stack_open() and syscall_open() that will
perform the ptrace_may_access() check and handle f_cred during open()
correctly.
So already there ;-)
Now you want to argue about /proc/*/stat, go ahead:
1) A vital sensitive file
2) any checks on ->open() will break userspace
3) *without* LSM support, there is no a ptrace_may_access() check during
->open() => will break userspace: top, ps ....
So we don't even go to LSM
4) /proc/*/stat is not like /proc/*/{syscall,stack}
5) If you want to do it, good luck make sure that you don't break "ps",
"top", but what you are saying thus not make sense at all! since that
LSM check depend on ptrace_may_access(), and the kernel without LSM
don't do it.
6) This is another problem, again not to f_cred after ->open()
Your problem: is why ptrace_may_access() not called during ->open() ?
to have that LSM check on cred == f_cred
Answer: already done for sensitive files: stack,syscall
Before I forget:
1) cap_ptrace_access_check() as shown in the previous email, is already
emulated!
2) If LSM is there, then it will do correctly its security_capable()
check on f_cred as with the new function file_ns_capable()
Feel free to respond to these argument!
> --Andy
--
Djalal Harouni
http://opendz.org
On Thu, Oct 03, 2013 at 08:12:44AM +0200, Ingo Molnar wrote:
> So please first get consensus on this fundamental design question before
> spreading your solution to more areas.
Check file_ns_capable() added in commit 935d8aabd4331 by Linus
Add file_ns_capable() helper function for open-time capability checking
commit 6708075f104c3c9b0 by Eric,
userns: Don't let unprivileged users trick privileged users into setting
the id_map
So they add file_ns_capable() to inspect file->f_cred during ->write()
The difference between the function I've added proc_allow_access() and
file_ns_capable() is that proc_allow_access() will check if it's
absolutely the same user, otherwise fallback to security_capable() which
is the heart of file_ns_capable()
So it's already been done and proposed! this is an easy solution to
detect if current's cred have changed.
> Thanks,
>
> Ingo
--
Djalal Harouni
http://opendz.org
On Fri, Oct 4, 2013 at 9:59 AM, Djalal Harouni <[email protected]> wrote:
> On Thu, Oct 03, 2013 at 02:09:55PM -0700, Andy Lutomirski wrote:
>> On Thu, Oct 3, 2013 at 1:13 PM, Djalal Harouni <[email protected]> wrote:
>> > On Thu, Oct 03, 2013 at 12:37:49PM -0700, Andy Lutomirski wrote:
>> >> On Thu, Oct 3, 2013 at 12:29 PM, Djalal Harouni <[email protected]> wrote:
>> >> > On Thu, Oct 03, 2013 at 04:12:37PM +0100, Andy Lutomirski wrote:
>> >> >> On Thu, Oct 3, 2013 at 3:36 PM, Djalal Harouni <[email protected]> wrote:
>> >> > Yes, I already did this, not only setuid, capabilities also are handled
>> >> > See the whole patch, please!
>> >> >
>> >> >
>> >> > Yes, and speaking about LSMs I've mentioned in my patches and doc, that
>> >> > the proposed function proc_allow_access() should be used after
>> >> > ptrace_may_access(). proc_allow_access() is not a replacement for
>> >> > ptrace_may_access(), it should be used *after* it.
>> >> >
>> >> > So cap_ptrace_access_check() is called, and before the file->f_cred
>> >> > checks. The LSM veto is already there.
>> >>
>> >> It's possible that I've misunderstood your patches, but I really don't
>> >> see where you're calling into LSMs to give them a chance to veto
>> >> access by *f_cred*.
>> > Ahh ok, I see, but why you want absolutly to put *f_cred* in this ?
>> >
>> > That's not its job, LSM veto is handled during read() correctly before
>> > proc_allow_access() and f_cred check. And if you want to do it correctly
>> > then f_cred should be handled during its time, during ->open().
>> > The correct way to handle it: ptrace_may_access() during ->open() and
>> > each syscall for sensitive files.
>> >
>> > Why add and speak about all this complexity where the correct check is
>> > just add ptrace_may_access() during ->open() ? using *f_cred* in this
>> > context and bring it here is not a valid argument IMO.
>>
>> I don't want to put f_cred into this. I'd rather the patches just
>> check everything at open() time. Doing that will require some form of
>> revocation, I think.
>>
>> Your current patches use f_cred, and they seem to do it wrong. So
> The current patches block and protect the current attacks correctly,
> without overhead.
>
> Example:
> proc_uid_map_write()
> -> map_write()
> -> file_ns_capable()
> -> security_capable(file->f_cred, ns, cap)
>
> file_ns_capable() added in commit 935d8aabd4331 by Linus
> Add file_ns_capable() helper function for open-time capability checking
>
> That also goes for commit 6708075f104c3c9b0 by Eric,
> userns: Don't let unprivileged users trick privileged users into setting
> the id_map
>
> The proc_allow_access() function that I've proposed has the same logic
> of file_ns_capable(), We can even put file_ns_capable() inside
> proc_allow_access(). We'll add support of security_capable_noaudit()
> inside file_ns_capable() and proc_allow_access() will be much more
> better.
>
> file_ns_capable() checks where a capability was there,
> proc_allow_access() checks where they have same uid + if capability was
> there.
>
>> please either fix it, stop using f_cred, or explain why it it's okay
>> despite not invoking LSM in the expected way.
> I've already explained it.
>
> LSM is handled by ptrace_may_access() which should be called during
> ->open() to handle f_cred, and during ->read() to handle current's cred.
This is getting tiresome. This patch (2/9) has my NAK. The other
patches depend on it, so I will not ack them. (The maintainers may or
may not care about my NAK -- that's their business.)
Your code is *wrong* for even the simple case of /proc/*/syscall. Consider:
Start with two processes, a and b, both normal tasks started by an
unprivileged user. Process a opens /proc/<b's pid>/syscall. All
checks pass. Process b execs a setcap'd binary. So b's uid and gid
do not change.
Then process a redirects stdin to that existing /proc/<b's pid>/stack
fd. Here's the bug in your patch: process a can *still* read that fd.
Why? Because *you're not checking that a's capabilities are a
superset of b's*. That code lives in the LSM infrastructure. You
need to call it if you want to keep the general approach you're
trying. You can't fix this just by checking for CAP_PTRACE, because
then you'll break SELinux.
This is messy, and it's why I think that you'd be better off doing
this by revoking the fd on exec instead.
--Andy
On Fri, Oct 04, 2013 at 04:40:01PM +0100, Andy Lutomirski wrote:
> On Fri, Oct 4, 2013 at 9:59 AM, Djalal Harouni <[email protected]> wrote:
> > On Thu, Oct 03, 2013 at 02:09:55PM -0700, Andy Lutomirski wrote:
> >> On Thu, Oct 3, 2013 at 1:13 PM, Djalal Harouni <[email protected]> wrote:
> >> > On Thu, Oct 03, 2013 at 12:37:49PM -0700, Andy Lutomirski wrote:
> >> >> On Thu, Oct 3, 2013 at 12:29 PM, Djalal Harouni <[email protected]> wrote:
> > The current patches block and protect the current attacks correctly,
> > without overhead.
> >
> > Example:
> > proc_uid_map_write()
> > -> map_write()
> > -> file_ns_capable()
> > -> security_capable(file->f_cred, ns, cap)
> >
> > file_ns_capable() added in commit 935d8aabd4331 by Linus
> > Add file_ns_capable() helper function for open-time capability checking
> >
> > That also goes for commit 6708075f104c3c9b0 by Eric,
> > userns: Don't let unprivileged users trick privileged users into setting
> > the id_map
> >
> > The proc_allow_access() function that I've proposed has the same logic
> > of file_ns_capable(), We can even put file_ns_capable() inside
> > proc_allow_access(). We'll add support of security_capable_noaudit()
> > inside file_ns_capable() and proc_allow_access() will be much more
> > better.
> >
> > file_ns_capable() checks where a capability was there,
> > proc_allow_access() checks where they have same uid + if capability was
> > there.
> >
> >> please either fix it, stop using f_cred, or explain why it it's okay
> >> despite not invoking LSM in the expected way.
> > I've already explained it.
> >
> > LSM is handled by ptrace_may_access() which should be called during
> > ->open() to handle f_cred, and during ->read() to handle current's cred.
>
> This is getting tiresome. This patch (2/9) has my NAK. The other
> patches depend on it, so I will not ack them. (The maintainers may or
> may not care about my NAK -- that's their business.)
Yeh tiresome, especially for fixing some leaks that affect the kernel
and ASLR for a long time now...
Ok response below:
> Your code is *wrong* for even the simple case of /proc/*/syscall. Consider:
>
> Start with two processes, a and b, both normal tasks started by an
> unprivileged user. Process a opens /proc/<b's pid>/syscall. All
> checks pass. Process b execs a setcap'd binary. So b's uid and gid
> do not change.
>
> Then process a redirects stdin to that existing /proc/<b's pid>/stack
You mean /proc/<b's pid>/syscall, ok let see
> fd. Here's the bug in your patch: process a can *still* read that fd.
> Why? Because *you're not checking that a's capabilities are a
> superset of b's*. That code lives in the LSM infrastructure. You
Please see with me:
I've made it clear that proc_allow_access() should be used after
ptrace_may_access() and documented it!
The reader process a will have a ptrace_may_access() check against the
target process b (the setcap'd binary) during that ->read()
->read()
-> syscall_read()
-> lock_trace()
-> ptrace_may_access()
-> security_ptrace_access_check()
So the scenario you describe here is perfectly handled.
But your point on the *capabilities superset* is correct, YES.
What if a, the reader who execve a setcap'd binary. I admit there is a
window here :-)
Please see:
I did the check in the proc_same_open_cred() function:
return (uid_eq(fcred->uid, cred->uid) &&
gid_eq(fcred->gid, cred->gid) &&
cap_issubset(cred->cap_permitted, fcred->cap_permitted));
Check if this is the same uid/gid and the capabilities superset!
But in the proc_allow_access() the capabilities superset is missing.
So to fix it:
1) if proc_same_open_cred() detects that cred have changed between
->open() and ->read() then abort, return zero, the ->read(),write()...
2) Improve the proc_allow_access() check by:
if this is the same user namespace then check uid/gid of f_cred on
target cred task, and the capabilities superset:
cap_issubset(tcred->cap_permitted, fcred->cap_permitted));
If it fails let security_capable() or file_ns_capable() do its magic.
The capabilities superset check are available, they are not LSM
specific.
> need to call it if you want to keep the general approach you're
> trying. You can't fix this just by checking for CAP_PTRACE, because
> then you'll break SELinux.
IIUC, then SELinux is already broken!
You do you realize that these patches are adding a bench of
ptrace_may_access() during ->open() to give LSM a chance to inspect
f_cred and other things.
> This is messy, and it's why I think that you'd be better off doing
> this by revoking the fd on exec instead.
As I've said I'm not against revoke. If it was there I would use it!
Not implemented! and perhaps it will be complex...
Please respond, and perhaps you should reconsider your NAK! since you
were involved in the patches that were committed, and used the
file->f_cred approach
Thanks
> --Andy
--
Djalal Harouni
http://opendz.org
On Fri, Oct 4, 2013 at 7:23 PM, Djalal Harouni <[email protected]> wrote:
> On Fri, Oct 04, 2013 at 04:40:01PM +0100, Andy Lutomirski wrote:
>> On Fri, Oct 4, 2013 at 9:59 AM, Djalal Harouni <[email protected]> wrote:
>> > On Thu, Oct 03, 2013 at 02:09:55PM -0700, Andy Lutomirski wrote:
>> >> On Thu, Oct 3, 2013 at 1:13 PM, Djalal Harouni <[email protected]> wrote:
>> >> > On Thu, Oct 03, 2013 at 12:37:49PM -0700, Andy Lutomirski wrote:
>> >> >> On Thu, Oct 3, 2013 at 12:29 PM, Djalal Harouni <[email protected]> wrote:
>> > The current patches block and protect the current attacks correctly,
>> > without overhead.
>> >
>> > Example:
>> > proc_uid_map_write()
>> > -> map_write()
>> > -> file_ns_capable()
>> > -> security_capable(file->f_cred, ns, cap)
>> >
>> > file_ns_capable() added in commit 935d8aabd4331 by Linus
>> > Add file_ns_capable() helper function for open-time capability checking
>> >
>> > That also goes for commit 6708075f104c3c9b0 by Eric,
>> > userns: Don't let unprivileged users trick privileged users into setting
>> > the id_map
>> >
>> > The proc_allow_access() function that I've proposed has the same logic
>> > of file_ns_capable(), We can even put file_ns_capable() inside
>> > proc_allow_access(). We'll add support of security_capable_noaudit()
>> > inside file_ns_capable() and proc_allow_access() will be much more
>> > better.
>> >
>> > file_ns_capable() checks where a capability was there,
>> > proc_allow_access() checks where they have same uid + if capability was
>> > there.
>> >
>> >> please either fix it, stop using f_cred, or explain why it it's okay
>> >> despite not invoking LSM in the expected way.
>> > I've already explained it.
>> >
>> > LSM is handled by ptrace_may_access() which should be called during
>> > ->open() to handle f_cred, and during ->read() to handle current's cred.
>>
>> This is getting tiresome. This patch (2/9) has my NAK. The other
>> patches depend on it, so I will not ack them. (The maintainers may or
>> may not care about my NAK -- that's their business.)
> Yeh tiresome, especially for fixing some leaks that affect the kernel
> and ASLR for a long time now...
>
> Ok response below:
>
>> Your code is *wrong* for even the simple case of /proc/*/syscall. Consider:
>>
>> Start with two processes, a and b, both normal tasks started by an
>> unprivileged user. Process a opens /proc/<b's pid>/syscall. All
>> checks pass. Process b execs a setcap'd binary. So b's uid and gid
>> do not change.
>>
>> Then process a redirects stdin to that existing /proc/<b's pid>/stack
> You mean /proc/<b's pid>/syscall, ok let see
>
>> fd. Here's the bug in your patch: process a can *still* read that fd.
>> Why? Because *you're not checking that a's capabilities are a
>> superset of b's*. That code lives in the LSM infrastructure. You
> Please see with me:
Sorry, I described the obviously broken scenario incorrectly. Your
patch breaks (in the absence of things like selinux) if a exec
something setuid root.
[...]
>
> I did the check in the proc_same_open_cred() function:
> return (uid_eq(fcred->uid, cred->uid) &&
> gid_eq(fcred->gid, cred->gid) &&
> cap_issubset(cred->cap_permitted, fcred->cap_permitted));
Which has nothing to do with anything. If that check fails, you're
just going on to a different, WRONG check/.
>
> Check if this is the same uid/gid and the capabilities superset!
>
> But in the proc_allow_access() the capabilities superset is missing.
>
>
> So to fix it:
> 1) if proc_same_open_cred() detects that cred have changed between
> ->open() and ->read() then abort, return zero, the ->read(),write()...
IMO yuck.
>
>
> 2) Improve the proc_allow_access() check by:
> if this is the same user namespace then check uid/gid of f_cred on
> target cred task, and the capabilities superset:
> cap_issubset(tcred->cap_permitted, fcred->cap_permitted));
>
> If it fails let security_capable() or file_ns_capable() do its magic.
>
NAK. You need to actually call the LSM. What if the reason to fail
the request isn't that the writer gained capabilities -- what if the
writer's selinux label changed?
>
>
> The capabilities superset check are available, they are not LSM
> specific.
NO. This is getting ridiculous. You can't just re-implement what the
commoncap code does because you don't want to call the LSM hook. You
need to call the LSM hook.
>
>> need to call it if you want to keep the general approach you're
>> trying. You can't fix this just by checking for CAP_PTRACE, because
>> then you'll break SELinux.
> IIUC, then SELinux is already broken!
>
> You do you realize that these patches are adding a bench of
> ptrace_may_access() during ->open() to give LSM a chance to inspect
> f_cred and other things.
Yes. But they're messy, they open-code incorrect checks, and they are
sufficiently incomplete as fixes for the vulnerability that you're
better off not doing them, because they'll just muddy the waters.
--Andy
>
>
>> This is messy, and it's why I think that you'd be better off doing
>> this by revoking the fd on exec instead.
> As I've said I'm not against revoke. If it was there I would use it!
>
> Not implemented! and perhaps it will be complex...
>
>
> Please respond, and perhaps you should reconsider your NAK! since you
> were involved in the patches that were committed, and used the
> file->f_cred approach
Yes, I was. That particular thing is fine, because there's no LSM
hook involved in the first case. If anyone ever adds one, they'll
have to invoke it in the appropriate places.
--Andy
On Fri, Oct 04, 2013 at 07:34:08PM +0100, Andy Lutomirski wrote:
> On Fri, Oct 4, 2013 at 7:23 PM, Djalal Harouni <[email protected]> wrote:
> > On Fri, Oct 04, 2013 at 04:40:01PM +0100, Andy Lutomirski wrote:
> >> On Fri, Oct 4, 2013 at 9:59 AM, Djalal Harouni <[email protected]> wrote:
> >> > On Thu, Oct 03, 2013 at 02:09:55PM -0700, Andy Lutomirski wrote:
[...]
> Sorry, I described the obviously broken scenario incorrectly. Your
> patch breaks (in the absence of things like selinux) if a exec
> something setuid root.
>
> [...]
>
> >
> > I did the check in the proc_same_open_cred() function:
> > return (uid_eq(fcred->uid, cred->uid) &&
> > gid_eq(fcred->gid, cred->gid) &&
> > cap_issubset(cred->cap_permitted, fcred->cap_permitted));
>
> Which has nothing to do with anything. If that check fails, you're
> just going on to a different, WRONG check/.
>
> >
> > Check if this is the same uid/gid and the capabilities superset!
> >
> > But in the proc_allow_access() the capabilities superset is missing.
> >
> >
> > So to fix it:
> > 1) if proc_same_open_cred() detects that cred have changed between
> > ->open() and ->read() then abort, return zero, the ->read(),write()...
>
> IMO yuck.
>
> >
> >
> > 2) Improve the proc_allow_access() check by:
> > if this is the same user namespace then check uid/gid of f_cred on
> > target cred task, and the capabilities superset:
> > cap_issubset(tcred->cap_permitted, fcred->cap_permitted));
> >
> > If it fails let security_capable() or file_ns_capable() do its magic.
> >
>
> NAK. You need to actually call the LSM. What if the reason to fail
> the request isn't that the writer gained capabilities -- what if the
> writer's selinux label changed?
Sorry I can't follow you here! Can you be more explicit please?
For me we are already doing this during ptrace_may_access() on each
syscall, which will call LSM to inspect the privileges on each ->open(),
->write()... So LSM hooks are already called. If you want to have more
LSM hooks, then perhaps that's another problem?
--
Djalal Harouni
http://opendz.org
On Fri, Oct 4, 2013 at 12:11 PM, Djalal Harouni <[email protected]> wrote:
> On Fri, Oct 04, 2013 at 07:34:08PM +0100, Andy Lutomirski wrote:
>> On Fri, Oct 4, 2013 at 7:23 PM, Djalal Harouni <[email protected]> wrote:
>> > On Fri, Oct 04, 2013 at 04:40:01PM +0100, Andy Lutomirski wrote:
>> >> On Fri, Oct 4, 2013 at 9:59 AM, Djalal Harouni <[email protected]> wrote:
>> >> > On Thu, Oct 03, 2013 at 02:09:55PM -0700, Andy Lutomirski wrote:
> [...]
>> Sorry, I described the obviously broken scenario incorrectly. Your
>> patch breaks (in the absence of things like selinux) if a exec
>> something setuid root.
>>
>> [...]
>>
>> >
>> > I did the check in the proc_same_open_cred() function:
>> > return (uid_eq(fcred->uid, cred->uid) &&
>> > gid_eq(fcred->gid, cred->gid) &&
>> > cap_issubset(cred->cap_permitted, fcred->cap_permitted));
>>
>> Which has nothing to do with anything. If that check fails, you're
>> just going on to a different, WRONG check/.
>>
>> >
>> > Check if this is the same uid/gid and the capabilities superset!
>> >
>> > But in the proc_allow_access() the capabilities superset is missing.
>> >
>> >
>> > So to fix it:
>> > 1) if proc_same_open_cred() detects that cred have changed between
>> > ->open() and ->read() then abort, return zero, the ->read(),write()...
>>
>> IMO yuck.
>>
>> >
>> >
>> > 2) Improve the proc_allow_access() check by:
>> > if this is the same user namespace then check uid/gid of f_cred on
>> > target cred task, and the capabilities superset:
>> > cap_issubset(tcred->cap_permitted, fcred->cap_permitted));
>> >
>> > If it fails let security_capable() or file_ns_capable() do its magic.
>> >
>>
>> NAK. You need to actually call the LSM. What if the reason to fail
>> the request isn't that the writer gained capabilities -- what if the
>> writer's selinux label changed?
> Sorry I can't follow you here! Can you be more explicit please?
>
> For me we are already doing this during ptrace_may_access() on each
> syscall, which will call LSM to inspect the privileges on each ->open(),
> ->write()... So LSM hooks are already called. If you want to have more
> LSM hooks, then perhaps that's another problem?
Can you show me where, in your code, LSMs are asked whether the
process calling read() is permitted to ptrace the process that the
proc file points at?
--Andy
On Fri, Oct 04, 2013 at 12:16:26PM -0700, Andy Lutomirski wrote:
> On Fri, Oct 4, 2013 at 12:11 PM, Djalal Harouni <[email protected]> wrote:
> > On Fri, Oct 04, 2013 at 07:34:08PM +0100, Andy Lutomirski wrote:
> >> On Fri, Oct 4, 2013 at 7:23 PM, Djalal Harouni <[email protected]> wrote:
> >> > On Fri, Oct 04, 2013 at 04:40:01PM +0100, Andy Lutomirski wrote:
> >> >> On Fri, Oct 4, 2013 at 9:59 AM, Djalal Harouni <[email protected]> wrote:
> >> >> > On Thu, Oct 03, 2013 at 02:09:55PM -0700, Andy Lutomirski wrote:
> > [...]
> >> Sorry, I described the obviously broken scenario incorrectly. Your
> >> patch breaks (in the absence of things like selinux) if a exec
> >> something setuid root.
> >>
> >> [...]
> >>
> >> >
> >> > I did the check in the proc_same_open_cred() function:
> >> > return (uid_eq(fcred->uid, cred->uid) &&
> >> > gid_eq(fcred->gid, cred->gid) &&
> >> > cap_issubset(cred->cap_permitted, fcred->cap_permitted));
> >>
> >> Which has nothing to do with anything. If that check fails, you're
> >> just going on to a different, WRONG check/.
> >>
> >> >
> >> > Check if this is the same uid/gid and the capabilities superset!
> >> >
> >> > But in the proc_allow_access() the capabilities superset is missing.
> >> >
> >> >
> >> > So to fix it:
> >> > 1) if proc_same_open_cred() detects that cred have changed between
> >> > ->open() and ->read() then abort, return zero, the ->read(),write()...
> >>
> >> IMO yuck.
> >>
> >> >
> >> >
> >> > 2) Improve the proc_allow_access() check by:
> >> > if this is the same user namespace then check uid/gid of f_cred on
> >> > target cred task, and the capabilities superset:
> >> > cap_issubset(tcred->cap_permitted, fcred->cap_permitted));
> >> >
> >> > If it fails let security_capable() or file_ns_capable() do its magic.
> >> >
> >>
> >> NAK. You need to actually call the LSM. What if the reason to fail
> >> the request isn't that the writer gained capabilities -- what if the
> >> writer's selinux label changed?
> > Sorry I can't follow you here! Can you be more explicit please?
> >
> > For me we are already doing this during ptrace_may_access() on each
> > syscall, which will call LSM to inspect the privileges on each ->open(),
> > ->write()... So LSM hooks are already called. If you want to have more
> > LSM hooks, then perhaps that's another problem?
>
> Can you show me where, in your code, LSMs are asked whether the
> process calling read() is permitted to ptrace the process that the
> proc file points at?
Yes.
[PATCH v2 9/9] procfs: improve permission checks on /proc/*/syscall
->read()
->syscall_read()
->lock_trace()
->ptrace_may_access()
->__ptrace_may_access()
->security_ptrace_access_check()
->yama_ptrace_access_check()
->security_ops->ptrace_access_check()
And also for patch:
[PATCH v2 8/9] procfs: improve permission checks on /proc/*/stack
And during ->open() and ->read()
So sorry Andy, I don't follow what you are describing.
> --Andy
--
Djalal Harouni
http://opendz.org
On Fri, Oct 4, 2013 at 12:27 PM, Djalal Harouni <[email protected]> wrote:
> On Fri, Oct 04, 2013 at 12:16:26PM -0700, Andy Lutomirski wrote:
>> On Fri, Oct 4, 2013 at 12:11 PM, Djalal Harouni <[email protected]> wrote:
>> > On Fri, Oct 04, 2013 at 07:34:08PM +0100, Andy Lutomirski wrote:
>> >> On Fri, Oct 4, 2013 at 7:23 PM, Djalal Harouni <[email protected]> wrote:
>> >> > On Fri, Oct 04, 2013 at 04:40:01PM +0100, Andy Lutomirski wrote:
>> >> >> On Fri, Oct 4, 2013 at 9:59 AM, Djalal Harouni <[email protected]> wrote:
>> >> >> > On Thu, Oct 03, 2013 at 02:09:55PM -0700, Andy Lutomirski wrote:
>> > [...]
>> >> Sorry, I described the obviously broken scenario incorrectly. Your
>> >> patch breaks (in the absence of things like selinux) if a exec
>> >> something setuid root.
>> >>
>> >> [...]
>> >>
>> >> >
>> >> > I did the check in the proc_same_open_cred() function:
>> >> > return (uid_eq(fcred->uid, cred->uid) &&
>> >> > gid_eq(fcred->gid, cred->gid) &&
>> >> > cap_issubset(cred->cap_permitted, fcred->cap_permitted));
>> >>
>> >> Which has nothing to do with anything. If that check fails, you're
>> >> just going on to a different, WRONG check/.
>> >>
>> >> >
>> >> > Check if this is the same uid/gid and the capabilities superset!
>> >> >
>> >> > But in the proc_allow_access() the capabilities superset is missing.
>> >> >
>> >> >
>> >> > So to fix it:
>> >> > 1) if proc_same_open_cred() detects that cred have changed between
>> >> > ->open() and ->read() then abort, return zero, the ->read(),write()...
>> >>
>> >> IMO yuck.
>> >>
>> >> >
>> >> >
>> >> > 2) Improve the proc_allow_access() check by:
>> >> > if this is the same user namespace then check uid/gid of f_cred on
>> >> > target cred task, and the capabilities superset:
>> >> > cap_issubset(tcred->cap_permitted, fcred->cap_permitted));
>> >> >
>> >> > If it fails let security_capable() or file_ns_capable() do its magic.
>> >> >
>> >>
>> >> NAK. You need to actually call the LSM. What if the reason to fail
>> >> the request isn't that the writer gained capabilities -- what if the
>> >> writer's selinux label changed?
>> > Sorry I can't follow you here! Can you be more explicit please?
>> >
>> > For me we are already doing this during ptrace_may_access() on each
>> > syscall, which will call LSM to inspect the privileges on each ->open(),
>> > ->write()... So LSM hooks are already called. If you want to have more
>> > LSM hooks, then perhaps that's another problem?
>>
>> Can you show me where, in your code, LSMs are asked whether the
>> process calling read() is permitted to ptrace the process that the
>> proc file points at?
> Yes.
> [PATCH v2 9/9] procfs: improve permission checks on /proc/*/syscall
>
> ->read()
> ->syscall_read()
> ->lock_trace()
> ->ptrace_may_access()
> ->__ptrace_may_access()
> ->security_ptrace_access_check()
> ->yama_ptrace_access_check()
> ->security_ops->ptrace_access_check()
>
>
> And also for patch:
> [PATCH v2 8/9] procfs: improve permission checks on /proc/*/stack
>
> And during ->open() and ->read()
>
>
> So sorry Andy, I don't follow what you are describing.
And what parameters are you passing to security_ptrace_access_check?
It's supposed to be f_cred, right? Because you want to make sure
that, if the opener had some low-privilege label, the target has
execed and gotten a more secure label, and the reader has a
high-privilege label, that the opener's label is checked against the
target's new label.
--Andy
>
>> --Andy
>
> --
> Djalal Harouni
> http://opendz.org
--
Andy Lutomirski
AMA Capital Management, LLC
On Fri, Oct 04, 2013 at 12:32:09PM -0700, Andy Lutomirski wrote:
> On Fri, Oct 4, 2013 at 12:27 PM, Djalal Harouni <[email protected]> wrote:
> > On Fri, Oct 04, 2013 at 12:16:26PM -0700, Andy Lutomirski wrote:
> >> On Fri, Oct 4, 2013 at 12:11 PM, Djalal Harouni <[email protected]> wrote:
> >> > On Fri, Oct 04, 2013 at 07:34:08PM +0100, Andy Lutomirski wrote:
> >> >> On Fri, Oct 4, 2013 at 7:23 PM, Djalal Harouni <[email protected]> wrote:
> >> >> > On Fri, Oct 04, 2013 at 04:40:01PM +0100, Andy Lutomirski wrote:
> >> >> >> On Fri, Oct 4, 2013 at 9:59 AM, Djalal Harouni <[email protected]> wrote:
> >> >> >> > On Thu, Oct 03, 2013 at 02:09:55PM -0700, Andy Lutomirski wrote:
> >> > [...]
> >> >> Sorry, I described the obviously broken scenario incorrectly. Your
> >> >> patch breaks (in the absence of things like selinux) if a exec
> >> >> something setuid root.
> >> >>
> >> >> [...]
> >> >>
> >> >> >
> >> >> > I did the check in the proc_same_open_cred() function:
> >> >> > return (uid_eq(fcred->uid, cred->uid) &&
> >> >> > gid_eq(fcred->gid, cred->gid) &&
> >> >> > cap_issubset(cred->cap_permitted, fcred->cap_permitted));
> >> >>
> >> >> Which has nothing to do with anything. If that check fails, you're
> >> >> just going on to a different, WRONG check/.
> >> >>
> >> >> >
> >> >> > Check if this is the same uid/gid and the capabilities superset!
> >> >> >
> >> >> > But in the proc_allow_access() the capabilities superset is missing.
> >> >> >
> >> >> >
> >> >> > So to fix it:
> >> >> > 1) if proc_same_open_cred() detects that cred have changed between
> >> >> > ->open() and ->read() then abort, return zero, the ->read(),write()...
> >> >>
> >> >> IMO yuck.
> >> >>
> >> >> >
> >> >> >
> >> >> > 2) Improve the proc_allow_access() check by:
> >> >> > if this is the same user namespace then check uid/gid of f_cred on
> >> >> > target cred task, and the capabilities superset:
> >> >> > cap_issubset(tcred->cap_permitted, fcred->cap_permitted));
> >> >> >
> >> >> > If it fails let security_capable() or file_ns_capable() do its magic.
> >> >> >
> >> >>
> >> >> NAK. You need to actually call the LSM. What if the reason to fail
> >> >> the request isn't that the writer gained capabilities -- what if the
> >> >> writer's selinux label changed?
> >> > Sorry I can't follow you here! Can you be more explicit please?
> >> >
> >> > For me we are already doing this during ptrace_may_access() on each
> >> > syscall, which will call LSM to inspect the privileges on each ->open(),
> >> > ->write()... So LSM hooks are already called. If you want to have more
> >> > LSM hooks, then perhaps that's another problem?
> >>
> >> Can you show me where, in your code, LSMs are asked whether the
> >> process calling read() is permitted to ptrace the process that the
> >> proc file points at?
> > Yes.
> > [PATCH v2 9/9] procfs: improve permission checks on /proc/*/syscall
> >
> > ->read()
> > ->syscall_read()
> > ->lock_trace()
> > ->ptrace_may_access()
> > ->__ptrace_may_access()
> > ->security_ptrace_access_check()
> > ->yama_ptrace_access_check()
> > ->security_ops->ptrace_access_check()
> >
> >
> > And also for patch:
> > [PATCH v2 8/9] procfs: improve permission checks on /proc/*/stack
> >
> > And during ->open() and ->read()
> >
> >
> > So sorry Andy, I don't follow what you are describing.
>
> And what parameters are you passing to security_ptrace_access_check?
> It's supposed to be f_cred, right? Because you want to make sure
> that, if the opener had some low-privilege label, the target has
> execed and gotten a more secure label, and the reader has a
> high-privilege label, that the opener's label is checked against the
> target's new label.
The current's cred each time.
Is there some mechanism to check what you describe?
> --Andy
>
> >
> >> --Andy
> >
> > --
> > Djalal Harouni
> > http://opendz.org
>
>
>
> --
> Andy Lutomirski
> AMA Capital Management, LLC
--
Djalal Harouni
http://opendz.org
On Fri, Oct 4, 2013 at 12:41 PM, Djalal Harouni <[email protected]> wrote:
> On Fri, Oct 04, 2013 at 12:32:09PM -0700, Andy Lutomirski wrote:
>> On Fri, Oct 4, 2013 at 12:27 PM, Djalal Harouni <[email protected]> wrote:
>> > On Fri, Oct 04, 2013 at 12:16:26PM -0700, Andy Lutomirski wrote:
>> >> On Fri, Oct 4, 2013 at 12:11 PM, Djalal Harouni <[email protected]> wrote:
>> >> > On Fri, Oct 04, 2013 at 07:34:08PM +0100, Andy Lutomirski wrote:
>> >> >> On Fri, Oct 4, 2013 at 7:23 PM, Djalal Harouni <[email protected]> wrote:
>> >> >> > On Fri, Oct 04, 2013 at 04:40:01PM +0100, Andy Lutomirski wrote:
>> >> >> >> On Fri, Oct 4, 2013 at 9:59 AM, Djalal Harouni <[email protected]> wrote:
>> >> >> >> > On Thu, Oct 03, 2013 at 02:09:55PM -0700, Andy Lutomirski wrote:
>> >> > [...]
>> >> >> Sorry, I described the obviously broken scenario incorrectly. Your
>> >> >> patch breaks (in the absence of things like selinux) if a exec
>> >> >> something setuid root.
>> >> >>
>> >> >> [...]
>> >> >>
>> >> >> >
>> >> >> > I did the check in the proc_same_open_cred() function:
>> >> >> > return (uid_eq(fcred->uid, cred->uid) &&
>> >> >> > gid_eq(fcred->gid, cred->gid) &&
>> >> >> > cap_issubset(cred->cap_permitted, fcred->cap_permitted));
>> >> >>
>> >> >> Which has nothing to do with anything. If that check fails, you're
>> >> >> just going on to a different, WRONG check/.
>> >> >>
>> >> >> >
>> >> >> > Check if this is the same uid/gid and the capabilities superset!
>> >> >> >
>> >> >> > But in the proc_allow_access() the capabilities superset is missing.
>> >> >> >
>> >> >> >
>> >> >> > So to fix it:
>> >> >> > 1) if proc_same_open_cred() detects that cred have changed between
>> >> >> > ->open() and ->read() then abort, return zero, the ->read(),write()...
>> >> >>
>> >> >> IMO yuck.
>> >> >>
>> >> >> >
>> >> >> >
>> >> >> > 2) Improve the proc_allow_access() check by:
>> >> >> > if this is the same user namespace then check uid/gid of f_cred on
>> >> >> > target cred task, and the capabilities superset:
>> >> >> > cap_issubset(tcred->cap_permitted, fcred->cap_permitted));
>> >> >> >
>> >> >> > If it fails let security_capable() or file_ns_capable() do its magic.
>> >> >> >
>> >> >>
>> >> >> NAK. You need to actually call the LSM. What if the reason to fail
>> >> >> the request isn't that the writer gained capabilities -- what if the
>> >> >> writer's selinux label changed?
>> >> > Sorry I can't follow you here! Can you be more explicit please?
>> >> >
>> >> > For me we are already doing this during ptrace_may_access() on each
>> >> > syscall, which will call LSM to inspect the privileges on each ->open(),
>> >> > ->write()... So LSM hooks are already called. If you want to have more
>> >> > LSM hooks, then perhaps that's another problem?
>> >>
>> >> Can you show me where, in your code, LSMs are asked whether the
>> >> process calling read() is permitted to ptrace the process that the
>> >> proc file points at?
>> > Yes.
>> > [PATCH v2 9/9] procfs: improve permission checks on /proc/*/syscall
>> >
>> > ->read()
>> > ->syscall_read()
>> > ->lock_trace()
>> > ->ptrace_may_access()
>> > ->__ptrace_may_access()
>> > ->security_ptrace_access_check()
>> > ->yama_ptrace_access_check()
>> > ->security_ops->ptrace_access_check()
>> >
>> >
>> > And also for patch:
>> > [PATCH v2 8/9] procfs: improve permission checks on /proc/*/stack
>> >
>> > And during ->open() and ->read()
>> >
>> >
>> > So sorry Andy, I don't follow what you are describing.
>>
>> And what parameters are you passing to security_ptrace_access_check?
>> It's supposed to be f_cred, right? Because you want to make sure
>> that, if the opener had some low-privilege label, the target has
>> execed and gotten a more secure label, and the reader has a
>> high-privilege label, that the opener's label is checked against the
>> target's new label.
> The current's cred each time.
Exactly. Hence the NAK.
>
> Is there some mechanism to check what you describe?
>
No. You could try to add one, but getting it to be compatible with
YAMA might be really messy.
Or you could see if destroying and recreating all the inodes on exec
or some other revoke-like approach would work.
--Andy
Andy Lutomirski <[email protected]> writes:
> On Fri, Oct 4, 2013 at 12:41 PM, Djalal Harouni <[email protected]> wrote:
>> On Fri, Oct 04, 2013 at 12:32:09PM -0700, Andy Lutomirski wrote:
>>> On Fri, Oct 4, 2013 at 12:27 PM, Djalal Harouni <[email protected]> wrote:
>>> > So sorry Andy, I don't follow what you are describing.
>>>
>>> And what parameters are you passing to security_ptrace_access_check?
>>> It's supposed to be f_cred, right? Because you want to make sure
>>> that, if the opener had some low-privilege label, the target has
>>> execed and gotten a more secure label, and the reader has a
>>> high-privilege label, that the opener's label is checked against the
>>> target's new label.
>> The current's cred each time.
>
> Exactly. Hence the NAK.
>
>>
>> Is there some mechanism to check what you describe?
>>
>
> No. You could try to add one, but getting it to be compatible with
> YAMA might be really messy.
>
> Or you could see if destroying and recreating all the inodes on exec
> or some other revoke-like approach would work.
This is a revoke like approach, and yes proc has a fully functional
revoke infrastructure. Right now that revoke is based on the process
going away. The problem challenge is that the process is morphing.
The practical question is which runtime checks do we want to perform.
If we can say in no uncertain terms that short of a suid exec that
no calls (such as setuid) can change the process permissions beyond
our ability to access the file, we can detect and exec and use that
as a signal.
Alternatively we may to look at a processes credentials and in all
cases where those change use that as a signal that the file must
be reopened.
Right now the model that we do a full permission check at every system
call because the morphing process may cause problems. If analysis can
be done to show that we can use a simpler check than a full permission
check that would be grand.
The problem is not lack of techinical infrastructure (revoke). The
problem is a question of which tests are sufficient.
Eric
On Fri, Oct 4, 2013 at 3:55 PM, Eric W. Biederman <[email protected]> wrote:
> Andy Lutomirski <[email protected]> writes:
>
>> On Fri, Oct 4, 2013 at 12:41 PM, Djalal Harouni <[email protected]> wrote:
>>> On Fri, Oct 04, 2013 at 12:32:09PM -0700, Andy Lutomirski wrote:
>>>> On Fri, Oct 4, 2013 at 12:27 PM, Djalal Harouni <[email protected]> wrote:
>
>>>> > So sorry Andy, I don't follow what you are describing.
>>>>
>>>> And what parameters are you passing to security_ptrace_access_check?
>>>> It's supposed to be f_cred, right? Because you want to make sure
>>>> that, if the opener had some low-privilege label, the target has
>>>> execed and gotten a more secure label, and the reader has a
>>>> high-privilege label, that the opener's label is checked against the
>>>> target's new label.
>>> The current's cred each time.
>>
>> Exactly. Hence the NAK.
>>
>>>
>>> Is there some mechanism to check what you describe?
>>>
>>
>> No. You could try to add one, but getting it to be compatible with
>> YAMA might be really messy.
>>
>> Or you could see if destroying and recreating all the inodes on exec
>> or some other revoke-like approach would work.
>
> This is a revoke like approach, and yes proc has a fully functional
> revoke infrastructure. Right now that revoke is based on the process
> going away. The problem challenge is that the process is morphing.
>
> The practical question is which runtime checks do we want to perform.
>
> If we can say in no uncertain terms that short of a suid exec that
> no calls (such as setuid) can change the process permissions beyond
> our ability to access the file, we can detect and exec and use that
> as a signal.
If you could ptrace a process before it calls setuid and you can't
after it calls setuid, then this is stupid and doesn't matter -- once
you've pwned a process, you retain your pwnership at least until exec.
So yes, except that it's not just suid exec. It's any exec that any
LSM would not do if no_new_privs were set.
>
> Alternatively we may to look at a processes credentials and in all
> cases where those change use that as a signal that the file must
> be reopened.
Hmm. So why don't we just do a revoke whenever an exec that changes
cred happens? (This will have some false positives, like unsharing
userns, I think, but we probably don't care.)
>
> Right now the model that we do a full permission check at every system
> call because the morphing process may cause problems. If analysis can
> be done to show that we can use a simpler check than a full permission
> check that would be grand.
>
> The problem is not lack of techinical infrastructure (revoke). The
> problem is a question of which tests are sufficient.
Can you point us at that infrastructure? My limited grepping skills
didn't spot it.
I'd really like a solution where there are no read or write
implementations in the entire kernel that check permissions. Failing
that, just getting it for procfs would be nice. (uid_map, etc will
probably need to be revoked on unshare for this to work.)
--Andy
On Fri, Oct 4, 2013 at 3:59 PM, Andy Lutomirski <[email protected]> wrote:
>
> I'd really like a solution where there are no read or write
> implementations in the entire kernel that check permissions. Failing
> that, just getting it for procfs would be nice. (uid_map, etc will
> probably need to be revoked on unshare for this to work.)
By "check permissions" I mean using anything but f_cred.
uid_map won't need any form of revoke, though -- the stuct file
already points at a particular target ns. I wonder why the
CAP_SYS_ADMIN check is in map_write instead of open, though.
--Andy
Andy Lutomirski <[email protected]> writes:
> On Fri, Oct 4, 2013 at 3:55 PM, Eric W. Biederman <[email protected]> wrote:
>> Andy Lutomirski <[email protected]> writes:
>>
>>> On Fri, Oct 4, 2013 at 12:41 PM, Djalal Harouni <[email protected]> wrote:
>>>> On Fri, Oct 04, 2013 at 12:32:09PM -0700, Andy Lutomirski wrote:
>>>>> On Fri, Oct 4, 2013 at 12:27 PM, Djalal Harouni <[email protected]> wrote:
>>
>>>>> > So sorry Andy, I don't follow what you are describing.
>>>>>
>>>>> And what parameters are you passing to security_ptrace_access_check?
>>>>> It's supposed to be f_cred, right? Because you want to make sure
>>>>> that, if the opener had some low-privilege label, the target has
>>>>> execed and gotten a more secure label, and the reader has a
>>>>> high-privilege label, that the opener's label is checked against the
>>>>> target's new label.
>>>> The current's cred each time.
>>>
>>> Exactly. Hence the NAK.
>>>
>>>>
>>>> Is there some mechanism to check what you describe?
>>>>
>>>
>>> No. You could try to add one, but getting it to be compatible with
>>> YAMA might be really messy.
>>>
>>> Or you could see if destroying and recreating all the inodes on exec
>>> or some other revoke-like approach would work.
>>
>> This is a revoke like approach, and yes proc has a fully functional
>> revoke infrastructure. Right now that revoke is based on the process
>> going away. The problem challenge is that the process is morphing.
>>
>> The practical question is which runtime checks do we want to perform.
>>
>> If we can say in no uncertain terms that short of a suid exec that
>> no calls (such as setuid) can change the process permissions beyond
>> our ability to access the file, we can detect and exec and use that
>> as a signal.
>
> If you could ptrace a process before it calls setuid and you can't
> after it calls setuid, then this is stupid and doesn't matter -- once
> you've pwned a process, you retain your pwnership at least until exec.
That is a reasonable principle to work from. Still I am seeing cases
where dumpable can change in principle if not in fact with a cred
change.
But yes exec does appear to be the event to focus on.
> So yes, except that it's not just suid exec. It's any exec that any
> LSM would not do if no_new_privs were set.
>> Alternatively we may to look at a processes credentials and in all
>> cases where those change use that as a signal that the file must
>> be reopened.
>
> Hmm. So why don't we just do a revoke whenever an exec that changes
> cred happens? (This will have some false positives, like unsharing
> userns, I think, but we probably don't care.)
But because it is proc and because people do crazy things we have to
investigate and test before we merge something like that. I believe
there was a patch not long ago that would fail an access if the openers
and and the accessors creds which blew up rather quickly.
But it is definitely worth looking at.
>> Right now the model that we do a full permission check at every system
>> call because the morphing process may cause problems. If analysis can
>> be done to show that we can use a simpler check than a full permission
>> check that would be grand.
>>
>> The problem is not lack of techinical infrastructure (revoke). The
>> problem is a question of which tests are sufficient.
>
> Can you point us at that infrastructure? My limited grepping skills
> didn't spot it.
There are about 3 implementations in proc. One for sysctl, one for the
generic files and one for the pid files.
For pid files it is a lazy implementation. Look at get_proc_task
and pid_revalidate.
The code that handles proc generic files and the related code that
handles the removal of sysfs files is more like a classic revoke.
> I'd really like a solution where there are no read or write
> implementations in the entire kernel that check permissions. Failing
> that, just getting it for procfs would be nice. (uid_map, etc will
> probably need to be revoked on unshare for this to work.)
I am certainly in favor of open acting as a capability grant and
arranging things so permission checks are not needed later.
uid_map has the interesting case that it doesn't know what permissions
you need until after you have written the data. Mapping just your one
uid doesn't require any of root privileges. So to move that permission
check into open it would have to become a flag. Allow to write
arbitrary uids, which we could test at write time.
But that is a weird case and let's concentrate on the common case of
proc files.
Eric
On Fri, Oct 04, 2013 at 03:17:08PM -0700, Andy Lutomirski wrote:
> On Fri, Oct 4, 2013 at 12:41 PM, Djalal Harouni <[email protected]> wrote:
> > On Fri, Oct 04, 2013 at 12:32:09PM -0700, Andy Lutomirski wrote:
> >> On Fri, Oct 4, 2013 at 12:27 PM, Djalal Harouni <[email protected]> wrote:
> >> > On Fri, Oct 04, 2013 at 12:16:26PM -0700, Andy Lutomirski wrote:
> >> >> On Fri, Oct 4, 2013 at 12:11 PM, Djalal Harouni <[email protected]> wrote:
> >> >> > On Fri, Oct 04, 2013 at 07:34:08PM +0100, Andy Lutomirski wrote:
> >> >> >> On Fri, Oct 4, 2013 at 7:23 PM, Djalal Harouni <[email protected]> wrote:
> >> >> >> > On Fri, Oct 04, 2013 at 04:40:01PM +0100, Andy Lutomirski wrote:
> >> >> >> >> On Fri, Oct 4, 2013 at 9:59 AM, Djalal Harouni <[email protected]> wrote:
> >> >> >> >> > On Thu, Oct 03, 2013 at 02:09:55PM -0700, Andy Lutomirski wrote:
> >> >> > [...]
> >> >> >> Sorry, I described the obviously broken scenario incorrectly. Your
> >> >> >> patch breaks (in the absence of things like selinux) if a exec
> >> >> >> something setuid root.
> >> >> >>
> >> >> >> [...]
> >> >> >>
> >> >> >> >
> >> >> >> > I did the check in the proc_same_open_cred() function:
> >> >> >> > return (uid_eq(fcred->uid, cred->uid) &&
> >> >> >> > gid_eq(fcred->gid, cred->gid) &&
> >> >> >> > cap_issubset(cred->cap_permitted, fcred->cap_permitted));
> >> >> >>
> >> >> >> Which has nothing to do with anything. If that check fails, you're
> >> >> >> just going on to a different, WRONG check/.
> >> >> >>
> >> >> >> >
> >> >> >> > Check if this is the same uid/gid and the capabilities superset!
> >> >> >> >
> >> >> >> > But in the proc_allow_access() the capabilities superset is missing.
> >> >> >> >
> >> >> >> >
> >> >> >> > So to fix it:
> >> >> >> > 1) if proc_same_open_cred() detects that cred have changed between
> >> >> >> > ->open() and ->read() then abort, return zero, the ->read(),write()...
> >> >> >>
> >> >> >> IMO yuck.
> >> >> >>
> >> >> >> >
> >> >> >> >
> >> >> >> > 2) Improve the proc_allow_access() check by:
> >> >> >> > if this is the same user namespace then check uid/gid of f_cred on
> >> >> >> > target cred task, and the capabilities superset:
> >> >> >> > cap_issubset(tcred->cap_permitted, fcred->cap_permitted));
> >> >> >> >
> >> >> >> > If it fails let security_capable() or file_ns_capable() do its magic.
> >> >> >> >
> >> >> >>
> >> >> >> NAK. You need to actually call the LSM. What if the reason to fail
> >> >> >> the request isn't that the writer gained capabilities -- what if the
> >> >> >> writer's selinux label changed?
> >> >> > Sorry I can't follow you here! Can you be more explicit please?
> >> >> >
> >> >> > For me we are already doing this during ptrace_may_access() on each
> >> >> > syscall, which will call LSM to inspect the privileges on each ->open(),
> >> >> > ->write()... So LSM hooks are already called. If you want to have more
> >> >> > LSM hooks, then perhaps that's another problem?
> >> >>
> >> >> Can you show me where, in your code, LSMs are asked whether the
> >> >> process calling read() is permitted to ptrace the process that the
> >> >> proc file points at?
> >> > Yes.
> >> > [PATCH v2 9/9] procfs: improve permission checks on /proc/*/syscall
> >> >
> >> > ->read()
> >> > ->syscall_read()
> >> > ->lock_trace()
> >> > ->ptrace_may_access()
> >> > ->__ptrace_may_access()
> >> > ->security_ptrace_access_check()
> >> > ->yama_ptrace_access_check()
> >> > ->security_ops->ptrace_access_check()
> >> >
> >> >
> >> > And also for patch:
> >> > [PATCH v2 8/9] procfs: improve permission checks on /proc/*/stack
> >> >
> >> > And during ->open() and ->read()
> >> >
> >> >
> >> > So sorry Andy, I don't follow what you are describing.
> >>
> >> And what parameters are you passing to security_ptrace_access_check?
> >> It's supposed to be f_cred, right? Because you want to make sure
> >> that, if the opener had some low-privilege label, the target has
> >> execed and gotten a more secure label, and the reader has a
> >> high-privilege label, that the opener's label is checked against the
> >> target's new label.
> > The current's cred each time.
>
> Exactly. Hence the NAK.
But Having two LSM Hooks there is really not practical!
Note to mention some of these redundancy checks...
> >
> > Is there some mechanism to check what you describe?
> >
>
> No. You could try to add one, but getting it to be compatible with
> YAMA might be really messy.
LSM is limitted in this situation, and it can't work with YAMA, or
perhaps YAMA will just return -EPERM
So this LSM protections are currently vulnerable too!
> Or you could see if destroying and recreating all the inodes on exec
> or some other revoke-like approach would work.
>
> --Andy
--
Djalal Harouni
http://opendz.org
On Sat, Oct 5, 2013 at 6:23 AM, Djalal Harouni <[email protected]> wrote:
> On Fri, Oct 04, 2013 at 03:17:08PM -0700, Andy Lutomirski wrote:
>>
>> Exactly. Hence the NAK.
> But Having two LSM Hooks there is really not practical!
It'd doable *if* it turns out that it's the right solution.
But revoke seems much more likely to be simple, comprehensible, and
obviously correct to me.
--Andy
>
> Note to mention some of these redundancy checks...
>
>> >
>> > Is there some mechanism to check what you describe?
>> >
>>
>> No. You could try to add one, but getting it to be compatible with
>> YAMA might be really messy.
> LSM is limitted in this situation, and it can't work with YAMA, or
> perhaps YAMA will just return -EPERM
>
> So this LSM protections are currently vulnerable too!
>
>
>> Or you could see if destroying and recreating all the inodes on exec
>> or some other revoke-like approach would work.
>>
>> --Andy
>
> --
> Djalal Harouni
> http://opendz.org
--
Andy Lutomirski
AMA Capital Management, LLC
On Fri, Oct 04, 2013 at 05:35:22PM -0700, Eric W. Biederman wrote:
> Andy Lutomirski <[email protected]> writes:
>
> > On Fri, Oct 4, 2013 at 3:55 PM, Eric W. Biederman <[email protected]> wrote:
> >> Andy Lutomirski <[email protected]> writes:
> >>
> >>> On Fri, Oct 4, 2013 at 12:41 PM, Djalal Harouni <[email protected]> wrote:
> >>>> On Fri, Oct 04, 2013 at 12:32:09PM -0700, Andy Lutomirski wrote:
> >>>>> On Fri, Oct 4, 2013 at 12:27 PM, Djalal Harouni <[email protected]> wrote:
> >>
> >>>>> > So sorry Andy, I don't follow what you are describing.
> >>>>>
> >>>>> And what parameters are you passing to security_ptrace_access_check?
> >>>>> It's supposed to be f_cred, right? Because you want to make sure
> >>>>> that, if the opener had some low-privilege label, the target has
> >>>>> execed and gotten a more secure label, and the reader has a
> >>>>> high-privilege label, that the opener's label is checked against the
> >>>>> target's new label.
> >>>> The current's cred each time.
> >>>
> >>> Exactly. Hence the NAK.
> >>>
> >>>>
> >>>> Is there some mechanism to check what you describe?
> >>>>
> >>>
> >>> No. You could try to add one, but getting it to be compatible with
> >>> YAMA might be really messy.
> >>>
> >>> Or you could see if destroying and recreating all the inodes on exec
> >>> or some other revoke-like approach would work.
> >>
> >> This is a revoke like approach, and yes proc has a fully functional
> >> revoke infrastructure. Right now that revoke is based on the process
> >> going away. The problem challenge is that the process is morphing.
> >>
> >> The practical question is which runtime checks do we want to perform.
> >>
> >> If we can say in no uncertain terms that short of a suid exec that
> >> no calls (such as setuid) can change the process permissions beyond
> >> our ability to access the file, we can detect and exec and use that
> >> as a signal.
> >
> > If you could ptrace a process before it calls setuid and you can't
> > after it calls setuid, then this is stupid and doesn't matter -- once
> > you've pwned a process, you retain your pwnership at least until exec.
>
> That is a reasonable principle to work from. Still I am seeing cases
> where dumpable can change in principle if not in fact with a cred
> change.
>
> But yes exec does appear to be the event to focus on.
>
> > So yes, except that it's not just suid exec. It's any exec that any
> > LSM would not do if no_new_privs were set.
>
> >> Alternatively we may to look at a processes credentials and in all
> >> cases where those change use that as a signal that the file must
> >> be reopened.
> >
> > Hmm. So why don't we just do a revoke whenever an exec that changes
> > cred happens? (This will have some false positives, like unsharing
> > userns, I think, but we probably don't care.)
>
> But because it is proc and because people do crazy things we have to
> investigate and test before we merge something like that. I believe
> there was a patch not long ago that would fail an access if the openers
> and and the accessors creds which blew up rather quickly.
>
> But it is definitely worth looking at.
We can track current exec or even target exec, use the percpu id solution
that was discussed here, and it seems Ingo like the solution:
https://lkml.org/lkml/2012/3/11/23
Grsecurity already do this!
Now to not break things and allow file descriptor to be passed, we couple
the exec id with the *cred and capability superset* check
The advantage of this solution would be:
* We'll have only a *one* ptrace_may_access(),LSM and permission check
during ->open() or ->read()/write()...
A one check will do the job.
We do this by:
1) During ->open() store the opener exec id somewhere...
2) Call ptrace_may_access() and LSM check during ->open() or later
during ->read(),write()...
3) Then during ->read(),write(): check if current's exec id matches the
opener exec id, if not then there was an execve and continue with the
cred superset check using file->f_cred, the logic here is to check if
opener is privileged than current:
* Same user namespace: check same uid, gid and capability superset
* Different user namespaces: check if opener is an ancestor and its
file->f_cred->euid is the owner...
IOW make sure that opener is more privileged than current, or with
the same privileges!
If it fails we return 0, end of file! otherwise allow the operation.
So if opener execve a more privileged process, the read() will fail.
This will make it easy to have a *one* ptrace_may_access() during all
syscalls!
Thoughts ?
--
Djalal Harouni
http://opendz.org
On Mon, Oct 07, 2013 at 02:41:33PM -0700, Andy Lutomirski wrote:
> On Sat, Oct 5, 2013 at 6:23 AM, Djalal Harouni <[email protected]> wrote:
> > On Fri, Oct 04, 2013 at 03:17:08PM -0700, Andy Lutomirski wrote:
> >>
> >> Exactly. Hence the NAK.
> > But Having two LSM Hooks there is really not practical!
>
> It'd doable *if* it turns out that it's the right solution.
>
> But revoke seems much more likely to be simple, comprehensible, and
> obviously correct to me.
Yes Andy, I agree, revoke is much better!
But it will not handle or fix all the situations, as I've said what if
revoke is not invloved here? there is no an execve from the target task!
Remember:
/proc/*/{stat,maps} and perhaps others have 0444 and don't have ptrace
checks during ->open() to not break some userspace... especially
/proc/*/stat file
So you will have an fd on these privileged files!
Current will execve a privileged process, and pass ptrace_may_access()
checks during ->read()...
Here revoke is not involved at all! so it will not fix these files and
they will continue to be vulnerable.
IMO to fix them, we must have the correct ptrace_may_access() check and
this involves: current doing an execve + current's cred
BTW, Andy we already return 0 (end of file) for /proc/*/mem
->read()
->mem_read()
->mem_rw()
if (!atomic_inc_not_zero(&mm->mm_users))
return 0
So can this be considered some sort of simple revoke?
--
Djalal Harouni
http://opendz.org
On Wed, Oct 09, 2013 at 11:54:02AM +0100, Djalal Harouni wrote:
> On Mon, Oct 07, 2013 at 02:41:33PM -0700, Andy Lutomirski wrote:
> > On Sat, Oct 5, 2013 at 6:23 AM, Djalal Harouni <[email protected]> wrote:
> > > On Fri, Oct 04, 2013 at 03:17:08PM -0700, Andy Lutomirski wrote:
> > >>
> > >> Exactly. Hence the NAK.
> > > But Having two LSM Hooks there is really not practical!
> >
> > It'd doable *if* it turns out that it's the right solution.
> >
> > But revoke seems much more likely to be simple, comprehensible, and
> > obviously correct to me.
> Yes Andy, I agree, revoke is much better!
>
> But it will not handle or fix all the situations, as I've said what if
> revoke is not invloved here? there is no an execve from the target task!
>
>
> Remember:
> /proc/*/{stat,maps} and perhaps others have 0444 and don't have ptrace
> checks during ->open() to not break some userspace... especially
> /proc/*/stat file
>
>
> So you will have an fd on these privileged files!
>
> Current will execve a privileged process, and pass ptrace_may_access()
> checks during ->read()...
>
> Here revoke is not involved at all! so it will not fix these files and
> they will continue to be vulnerable.
>
> IMO to fix them, we must have the correct ptrace_may_access() check and
> this involves: current doing an execve + current's cred
>
>
>
> BTW, Andy we already return 0 (end of file) for /proc/*/mem
> ->read()
> ->mem_read()
> ->mem_rw()
> if (!atomic_inc_not_zero(&mm->mm_users))
> return 0
>
> So can this be considered some sort of simple revoke?
Or create dummy compat-quirk maps inode as Ingo put it in the other mail:
00000000-00000000 ---p 00000000 00:00 0
...
For /proc/*/maps files, to not break userspace
--
Djalal Harouni
http://opendz.org
On Wed, Oct 9, 2013 at 11:54 AM, Djalal Harouni <[email protected]> wrote:
> On Mon, Oct 07, 2013 at 02:41:33PM -0700, Andy Lutomirski wrote:
>> On Sat, Oct 5, 2013 at 6:23 AM, Djalal Harouni <[email protected]> wrote:
>> > On Fri, Oct 04, 2013 at 03:17:08PM -0700, Andy Lutomirski wrote:
>> >>
>> >> Exactly. Hence the NAK.
>> > But Having two LSM Hooks there is really not practical!
>>
>> It'd doable *if* it turns out that it's the right solution.
>>
>> But revoke seems much more likely to be simple, comprehensible, and
>> obviously correct to me.
> Yes Andy, I agree, revoke is much better!
>
> But it will not handle or fix all the situations, as I've said what if
> revoke is not invloved here? there is no an execve from the target task!
>
>
> Remember:
> /proc/*/{stat,maps} and perhaps others have 0444 and don't have ptrace
> checks during ->open() to not break some userspace... especially
> /proc/*/stat file
For /proc/*/stat: check permissions when opening. If the opener
passes the ptrace check, set a flag in file->private_data indicating
that this struct file has permission.
For /proc/*/maps: either fail the open if the check fails or set a
flag that causes the resulting struct file to be useless.
>
>
> So you will have an fd on these privileged files!
>
> Current will execve a privileged process, and pass ptrace_may_access()
> checks during ->read()...
>
> Here revoke is not involved at all! so it will not fix these files and
> they will continue to be vulnerable.
>
> IMO to fix them, we must have the correct ptrace_may_access() check and
> this involves: current doing an execve + current's cred
>
>
>
> BTW, Andy we already return 0 (end of file) for /proc/*/mem
> ->read()
> ->mem_read()
> ->mem_rw()
> if (!atomic_inc_not_zero(&mm->mm_users))
> return 0
>
> So can this be considered some sort of simple revoke?
>
Apparently not. I haven't looked at the code, but I just tried it.
The fd from /proc/<pid>/maps survives an exec of the process it's
pointing at. That means that either the mm changing has no effect on
the struct file or that an unshared mm survives exec.
--Andy
On Wed, Oct 09, 2013 at 06:27:22PM +0100, Andy Lutomirski wrote:
> On Wed, Oct 9, 2013 at 11:54 AM, Djalal Harouni <[email protected]> wrote:
> > On Mon, Oct 07, 2013 at 02:41:33PM -0700, Andy Lutomirski wrote:
> >> On Sat, Oct 5, 2013 at 6:23 AM, Djalal Harouni <[email protected]> wrote:
> >> > On Fri, Oct 04, 2013 at 03:17:08PM -0700, Andy Lutomirski wrote:
> >> >>
> >> >> Exactly. Hence the NAK.
> >> > But Having two LSM Hooks there is really not practical!
> >>
> >> It'd doable *if* it turns out that it's the right solution.
> >>
> >> But revoke seems much more likely to be simple, comprehensible, and
> >> obviously correct to me.
> > Yes Andy, I agree, revoke is much better!
> >
> > But it will not handle or fix all the situations, as I've said what if
> > revoke is not invloved here? there is no an execve from the target task!
> >
> >
> > Remember:
> > /proc/*/{stat,maps} and perhaps others have 0444 and don't have ptrace
> > checks during ->open() to not break some userspace... especially
> > /proc/*/stat file
>
> For /proc/*/stat: check permissions when opening. If the opener
> passes the ptrace check, set a flag in file->private_data indicating
> that this struct file has permission.
That will fix it, but it will need some extra work since
file->private_data is already used to store seq_file structs!
> For /proc/*/maps: either fail the open if the check fails or set a
> flag that causes the resulting struct file to be useless.
Not sure about this, we need to inspect glibc
> >
> >
> > So you will have an fd on these privileged files!
> >
> > Current will execve a privileged process, and pass ptrace_may_access()
> > checks during ->read()...
> >
> > Here revoke is not involved at all! so it will not fix these files and
> > they will continue to be vulnerable.
> >
> > IMO to fix them, we must have the correct ptrace_may_access() check and
> > this involves: current doing an execve + current's cred
> >
> >
> >
> > BTW, Andy we already return 0 (end of file) for /proc/*/mem
> > ->read()
> > ->mem_read()
> > ->mem_rw()
> > if (!atomic_inc_not_zero(&mm->mm_users))
> > return 0
> >
> > So can this be considered some sort of simple revoke?
> >
>
> Apparently not. I haven't looked at the code, but I just tried it.
> The fd from /proc/<pid>/maps survives an exec of the process it's
> pointing at. That means that either the mm changing has no effect on
> the struct file or that an unshared mm survives exec.
yes the old mm (during ->open()) will survive but not vma, ->read() will
return zero
In the mean time, to close some of these vulnerabilities, I'll submit
another patch to prevent open() arbitrary /proc/*/{stack,syscall}
1) Make them 0400
2) Add the ptrace_may_access() during ->open() for ptrace capabilities
and LSM checks
It would be nice to get your Acked-by Andy!
--
Djalal Harouni
http://opendz.org