/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 and it will live until
fput()
* Does not pin or grab any counter on any extra struct.
* 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
The following series tries to implement what I describe.
1) Add the proc_same_open_cred() helper
This function will check if the file's opener cred matches the current
cred. 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.
2) Add the proc_allow_access() helper
Check if the file's opener had enough permissions to access the target
process.
3) Make seq_file struct able to access the file's opener cred.
Since seq_file struct is used by procfs entries, embed a pointer to the
file->f_cred into the seq_file struct.
4) Add permission checks on the file's opener of /proc/*/stack
5) Add permission checks on the file's opener of /proc/*/personality
6) Add permission checks on the file's opener of /proc/*/stat
7) Improve permission checks on /proc/*/syscall
8) Finally the last patch is user_ns and seq_file cleaning.
Thanks!
[1] https://lkml.org/lkml/2013/8/26/354
[2] https://lkml.org/lkml/2013/8/31/209
Djalal Harouni (12):
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
seq_file: Make seq_file able to access the file's opener cred
seq_file: set the seq_file->f_cred during seq_open()
procfs: make /proc/*/stack 0400
procfs: add permission checks on the file's opener of /proc/*/stack
procfs: add permission checks on the file's opener of /proc/*/personality
procfs: add permission checks on the file's opener of /proc/*/stat
procfs: move PROC_BLOCK_SIZE declaration up to make it visible
procfs: improve permission checks on /proc/*/syscall
user_ns: seq_file: use the user_ns that is embedded in the f_cred struct
fs/proc/array.c | 14 ++++++-
fs/proc/base.c | 233 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++----------
fs/proc/internal.h | 3 ++
fs/seq_file.c | 4 +-
include/linux/seq_file.h | 13 +++++--
5 files changed, 238 insertions(+), 29 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]>
Cc: 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]>
Cc: 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]>
Cc: 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 f_cred field of the file struct contains the cred of current at
open time. This field can be used to get the context of open, and track
current's cred changes after.
The procfs is one of those fs that need to track current cred changes
in order to implement proper permission checks on each system call.
The procfs make use of seq_file struct and its iterators to step through
/proc objects. These iterators and seq_file helpers must be able to
access the file->f_cred to perform various permission checks at any
time.
Therefor add the f_cred field to the seq_file struct and a helper
seq_f_cred() to return it.
Cc: Kees Cook <[email protected]>
Cc: Eric W. Biederman <[email protected]>
Signed-off-by: Djalal Harouni <[email protected]>
---
include/linux/seq_file.h | 7 +++++++
1 file changed, 7 insertions(+)
diff --git a/include/linux/seq_file.h b/include/linux/seq_file.h
index 4e32edc..ec07709 100644
--- a/include/linux/seq_file.h
+++ b/include/linux/seq_file.h
@@ -13,6 +13,7 @@ struct file;
struct path;
struct inode;
struct dentry;
+struct cred;
struct user_namespace;
struct seq_file {
@@ -26,6 +27,7 @@ struct seq_file {
struct mutex lock;
const struct seq_operations *op;
int poll_event;
+ const struct cred *f_cred;
#ifdef CONFIG_USER_NS
struct user_namespace *user_ns;
#endif
@@ -133,6 +135,11 @@ int seq_put_decimal_ull(struct seq_file *m, char delimiter,
int seq_put_decimal_ll(struct seq_file *m, char delimiter,
long long num);
+static inline const struct cred *seq_f_cred(struct seq_file *seq)
+{
+ return seq->f_cred;
+}
+
static inline struct user_namespace *seq_user_ns(struct seq_file *seq)
{
#ifdef CONFIG_USER_NS
--
1.7.11.7
The f_cred field of the file struct contains the cred of current at
open time. This field can be used to get the context of open, and track
current's cred changes after.
The procfs is one of those fs that need to track current cred changes
in order to implement proper permission checks on each system call.
Set the seq_file->f_cred to file->f_cred during seq_open().
Cc: Kees Cook <[email protected]>
Cc: Eric W. Biederman <[email protected]>
Signed-off-by: Djalal Harouni <[email protected]>
---
fs/seq_file.c | 1 +
1 file changed, 1 insertion(+)
diff --git a/fs/seq_file.c b/fs/seq_file.c
index 3135c25..a5e5b98 100644
--- a/fs/seq_file.c
+++ b/fs/seq_file.c
@@ -57,6 +57,7 @@ int seq_open(struct file *file, const struct seq_operations *op)
memset(p, 0, sizeof(*p));
mutex_init(&p->lock);
p->op = op;
+ p->f_cred = file->f_cred;
#ifdef CONFIG_USER_NS
p->user_ns = file->f_cred->user_ns;
#endif
--
1.7.11.7
The /proc/*/stack contains sensitive information and currently its mode
is 0444. Change this to 0400 so the VFS will be able to block
unprivileged processes to get file descriptors on arbitrary privileged
/proc/*/stack files.
The /proc/*/stack is a /procfs ONE file that shares the same ->open()
file operation with other ONE files. Doing a ptrace_may_access() check
during open() might break userspace from accessing other ONE files
like /proc/*/stat and /proc/*/statm.
Therfore make it 0400 for now, and improve its check during ->read()
in the next following patch.
Cc: Kees Cook <[email protected]>
Cc: Eric W. Biederman <[email protected]>
Signed-off-by: Djalal Harouni <[email protected]>
---
fs/proc/base.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/fs/proc/base.c b/fs/proc/base.c
index 8d21316..bb90171 100644
--- a/fs/proc/base.c
+++ b/fs/proc/base.c
@@ -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),
@@ -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
/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().
Use proc_same_open_cred() to detect if the cred of current between
->open() and ->read() have changed, 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().
The file's opener cred are obtained by seq_f_cred() on seq_file struct.
The ptrace_may_access() + proc_allow_access() check is performed during
->read() time, where the ptrace_may_access() check should also be
performed during ->open(), however currently this is not the case.
This is due to /procfs ONE files that share the same ->open() function
proc_single_open(). Adding the ptrace_may_access() check to
proc_single_open() might break userspace from accessing other ONE files
like /proc/*/stat and /proc/*/statm.
So just perform the checks during ->read() and if current's cred have
changed, then check the file's opener cred with proc_allow_access().
This will block passing the file descriptor to a more privileged
process (e.g. a suid-exec).
Cc: Kees Cook <[email protected]>
Cc: Eric W. Biederman <[email protected]>
Signed-off-by: Djalal Harouni <[email protected]>
---
fs/proc/base.c | 26 +++++++++++++++++++-------
1 file changed, 19 insertions(+), 7 deletions(-)
diff --git a/fs/proc/base.c b/fs/proc/base.c
index bb90171..d6a17b3 100644
--- a/fs/proc/base.c
+++ b/fs/proc/base.c
@@ -402,6 +402,8 @@ static int proc_pid_stack(struct seq_file *m, struct pid_namespace *ns,
unsigned long *entries;
int err;
int i;
+ int same_cred;
+ const struct cred *fcred = seq_f_cred(m);
entries = kmalloc(MAX_STACK_TRACE_DEPTH * sizeof(*entries), GFP_KERNEL);
if (!entries)
@@ -412,18 +414,28 @@ 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(fcred);
+
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(fcred, task, PTRACE_MODE_ATTACH)) {
+ err = -EPERM;
unlock_trace(task);
+ goto free;
}
- kfree(entries);
+ 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;
}
#endif
--
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 | 16 +++++++++++++---
1 file changed, 13 insertions(+), 3 deletions(-)
diff --git a/fs/proc/base.c b/fs/proc/base.c
index d6a17b3..ed8e3f7 100644
--- a/fs/proc/base.c
+++ b/fs/proc/base.c
@@ -2656,11 +2656,21 @@ 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)
{
+ const struct cred *fcred = seq_f_cred(m);
+ 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
Some fields of the /proc/*/stat are sensitive fields that need
appropriate protection.
In addition to the classic ptrace_may_access() check. 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 these sensitive fields.
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 | 14 ++++++++++++--
1 file changed, 12 insertions(+), 2 deletions(-)
diff --git a/fs/proc/array.c b/fs/proc/array.c
index cbd0f1b..8409d52 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,20 @@ 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;
+ const struct cred *fcred = seq_f_cred(m);
+ 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 && !proc_same_open_cred(fcred))
+ permitted = proc_allow_access(fcred, 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
Move PROC_BLOCK_SIZE declaraiton up, so new code can use it.
Signed-off-by: Djalal Harouni <[email protected]>
---
fs/proc/base.c | 4 +++-
1 file changed, 3 insertions(+), 1 deletion(-)
diff --git a/fs/proc/base.c b/fs/proc/base.c
index ed8e3f7..fe02ee4 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.
@@ -738,7 +741,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)
--
1.7.11.7
Permission checks need to happen during each system call. Therefore we
need to switch the /proc/*/syscall entry from an INF node to a REG node,
to avoid breaking shared INF file operations. This way it will have its
own file operations to implement the appropriate checks.
Add the syscall_open() to check if the file's opener has enough
permission to ptrace the target task.
Add the syscall_read() to check permissions and to read target 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.
This patch also makes /proc/*/syscall 0400 so that the VFS will block
any unprivilged access right away.
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, 80 insertions(+), 7 deletions(-)
diff --git a/fs/proc/base.c b/fs/proc/base.c
index fe02ee4..e82d0a4 100644
--- a/fs/proc/base.c
+++ b/fs/proc/base.c
@@ -630,13 +630,33 @@ 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");
@@ -648,9 +668,62 @@ 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)
+{
+ ssize_t length;
+ unsigned long page;
+ struct inode *inode = file_inode(file);
+ struct task_struct *task = get_proc_task(inode);
+ int same_cred = proc_same_open_cred(file->f_cred);
+
+ 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;
+
+ length = lock_trace(task);
+ if (length)
+ goto out_free;
+
+ if (!same_cred &&
+ !proc_allow_access(file->f_cred, 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 */
/************************************************************************/
@@ -2706,7 +2779,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),
+ REG("syscall", S_IRUSR, proc_pid_syscall_operations),
#endif
INF("cmdline", S_IRUGO, proc_pid_cmdline),
ONE("stat", S_IRUGO, proc_tgid_stat),
@@ -3042,7 +3115,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),
+ 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
seq_file struct now has a reference on the file->f_cred struct which
includes a pointer on user_ns. So remove the user_ns field from seq_file
struct and use the one provided by seq_file->f_cred.
Update seq_user_ns() to return the user_ns of seq_file->f_cred.
Cc: Eric W. Biederman <[email protected]>
Signed-off-by: Djalal Harouni <[email protected]>
---
fs/seq_file.c | 3 ---
include/linux/seq_file.h | 6 ++----
2 files changed, 2 insertions(+), 7 deletions(-)
diff --git a/fs/seq_file.c b/fs/seq_file.c
index a5e5b98..ee1c36d 100644
--- a/fs/seq_file.c
+++ b/fs/seq_file.c
@@ -58,9 +58,6 @@ int seq_open(struct file *file, const struct seq_operations *op)
mutex_init(&p->lock);
p->op = op;
p->f_cred = file->f_cred;
-#ifdef CONFIG_USER_NS
- p->user_ns = file->f_cred->user_ns;
-#endif
/*
* Wrappers around seq_open(e.g. swaps_open) need to be
diff --git a/include/linux/seq_file.h b/include/linux/seq_file.h
index ec07709..5db1e39 100644
--- a/include/linux/seq_file.h
+++ b/include/linux/seq_file.h
@@ -7,6 +7,7 @@
#include <linux/mutex.h>
#include <linux/cpumask.h>
#include <linux/nodemask.h>
+#include <linux/cred.h>
struct seq_operations;
struct file;
@@ -28,9 +29,6 @@ struct seq_file {
const struct seq_operations *op;
int poll_event;
const struct cred *f_cred;
-#ifdef CONFIG_USER_NS
- struct user_namespace *user_ns;
-#endif
void *private;
};
@@ -143,7 +141,7 @@ static inline const struct cred *seq_f_cred(struct seq_file *seq)
static inline struct user_namespace *seq_user_ns(struct seq_file *seq)
{
#ifdef CONFIG_USER_NS
- return seq->user_ns;
+ return seq_f_cred(seq)->user_ns;
#else
extern struct user_namespace init_user_ns;
return &init_user_ns;
--
1.7.11.7
On Wed, Sep 25, 2013 at 1:14 PM, Djalal Harouni <[email protected]> wrote:
>
> Therefor add the f_cred field to the seq_file struct and a helper
> seq_f_cred() to return it.
I hate how you've split up this patch from the next one that actually
_initializes_ the new field.
The two patches should have been one.
I think the patch should also remove the 'user_ns' member, since it's
now available as f_cred->user_ns.
I also suspect that it would be better to just make the the new
seq_file member point to the 'struct file' instead. Sure, it's an
extra level of indirection, but the lifetime of f_cred is not as clear
otherwise. You don't increment the reference count, which is correct
*only* because 'seq_file' has the same lifetime as 'struct file', and
thus the reference count from struct file for the f_cred is
sufficient.
[ Time passes, I look over the other patches ]
Oh, and I note that you remove user_ns in patch 12. Again, I think
that's just splitting things up too much. It actually gets harder to
see what happens when you do this.
Linus
On Wed, Sep 25, 2013 at 09:14:37PM +0100, Djalal Harouni wrote:
> The f_cred field of the file struct contains the cred of current at
> open time. This field can be used to get the context of open, and track
> current's cred changes after.
>
> The procfs is one of those fs that need to track current cred changes
> in order to implement proper permission checks on each system call.
>
> The procfs make use of seq_file struct and its iterators to step through
> /proc objects. These iterators and seq_file helpers must be able to
> access the file->f_cred to perform various permission checks at any
> time.
>
> Therefor add the f_cred field to the seq_file struct and a helper
> seq_f_cred() to return it.
NAK. This is completely irrelevant for most of seq_file users and it simply
does not belong in struct seq_file.
On Wed, Sep 25, 2013 at 05:22:51PM -0700, Linus Torvalds wrote:
> On Wed, Sep 25, 2013 at 1:14 PM, Djalal Harouni <[email protected]> wrote:
> >
> > Therefor add the f_cred field to the seq_file struct and a helper
> > seq_f_cred() to return it.
>
> I hate how you've split up this patch from the next one that actually
> _initializes_ the new field.
>
> The two patches should have been one.
>
> I think the patch should also remove the 'user_ns' member, since it's
> now available as f_cred->user_ns.
>
> I also suspect that it would be better to just make the the new
> seq_file member point to the 'struct file' instead. Sure, it's an
> extra level of indirection, but the lifetime of f_cred is not as clear
> otherwise. You don't increment the reference count, which is correct
> *only* because 'seq_file' has the same lifetime as 'struct file', and
> thus the reference count from struct file for the f_cred is
> sufficient.
That's better than f_cred (or user_ns, for that matter), but... I'm
afraid that it'll get abused very soon. And I don't understand the
argument about the lifetime rules - what makes struct file ones
different from struct cred ones in that respect? Except that in this
case it's really obvious that we can't grab a reference, that is...
On Wed, Sep 25, 2013 at 3:14 PM, Djalal Harouni <[email protected]> wrote:
> The /proc/*/stack contains sensitive information and currently its mode
> is 0444. Change this to 0400 so the VFS will be able to block
> unprivileged processes to get file descriptors on arbitrary privileged
> /proc/*/stack files.
>
> The /proc/*/stack is a /procfs ONE file that shares the same ->open()
> file operation with other ONE files. Doing a ptrace_may_access() check
> during open() might break userspace from accessing other ONE files
> like /proc/*/stat and /proc/*/statm.
>
> Therfore make it 0400 for now, and improve its check during ->read()
> in the next following patch.
>
> Cc: Kees Cook <[email protected]>
> Cc: Eric W. Biederman <[email protected]>
> Signed-off-by: Djalal Harouni <[email protected]>
While the rest of the series is being discussed, I think it would be
nice to at least get this into the tree. Fixing this reduces which
processes are exposed to ASLR leaks. The rest of the series closes the
remaining holes.
I would if it would be valuable adding a test for the identified leak
conditions to some test suite? LTP perhaps?
-Kees
--
Kees Cook
Chrome OS Security
On Wed, Sep 25, 2013 at 05:22:51PM -0700, Linus Torvalds wrote:
> On Wed, Sep 25, 2013 at 1:14 PM, Djalal Harouni <[email protected]> wrote:
> >
> > Therefor add the f_cred field to the seq_file struct and a helper
> > seq_f_cred() to return it.
>
> I hate how you've split up this patch from the next one that actually
> _initializes_ the new field.
>
> The two patches should have been one.
Ok, noted.
> I think the patch should also remove the 'user_ns' member, since it's
> now available as f_cred->user_ns.
>
> I also suspect that it would be better to just make the the new
> seq_file member point to the 'struct file' instead. Sure, it's an
> extra level of indirection, but the lifetime of f_cred is not as clear
> otherwise. You don't increment the reference count, which is correct
> *only* because 'seq_file' has the same lifetime as 'struct file', and
> thus the reference count from struct file for the f_cred is
> sufficient.
Yes.
For the seq_file struct, yes we can make it point to the 'struct file'.
Or, I've also found other ways, perhaps they will make Al happy, one of
them is to use the 'struct file' as you point, but in an other way, or
change the file_operations of these sensitive files.
Please Linus see my next response to Al, thanks
> [ Time passes, I look over the other patches ]
>
> Oh, and I note that you remove user_ns in patch 12. Again, I think
> that's just splitting things up too much. It actually gets harder to
> see what happens when you do this.
Ok, will improve it.
Will also wait to see what others think, then resend as a V2
Thanks.
>
> Linus
--
Djalal Harouni
http://opendz.org
On Thu, Sep 26, 2013 at 04:02:54AM +0100, Al Viro wrote:
> On Wed, Sep 25, 2013 at 05:22:51PM -0700, Linus Torvalds wrote:
> > On Wed, Sep 25, 2013 at 1:14 PM, Djalal Harouni <[email protected]> wrote:
> > >
> > > Therefor add the f_cred field to the seq_file struct and a helper
> > > seq_f_cred() to return it.
> >
> > I hate how you've split up this patch from the next one that actually
> > _initializes_ the new field.
> >
> > The two patches should have been one.
> >
> > I think the patch should also remove the 'user_ns' member, since it's
> > now available as f_cred->user_ns.
> >
> > I also suspect that it would be better to just make the the new
> > seq_file member point to the 'struct file' instead. Sure, it's an
> > extra level of indirection, but the lifetime of f_cred is not as clear
> > otherwise. You don't increment the reference count, which is correct
> > *only* because 'seq_file' has the same lifetime as 'struct file', and
> > thus the reference count from struct file for the f_cred is
> > sufficient.
>
> That's better than f_cred (or user_ns, for that matter), but... I'm
> afraid that it'll get abused very soon. And I don't understand the
> argument about the lifetime rules - what makes struct file ones
> different from struct cred ones in that respect? Except that in this
> case it's really obvious that we can't grab a reference, that is...
Ok, I'll not argue on f_cred or user_ns as fields for seq_file struct
Al there are other solutions:
1) Use the 'struct file' as pointed by Linus, but instead make
the seq_file->private member point to it.
These ONE nodes that share the same code call:
proc_single_open()
-> single_open(filp, proc_single_show, inode);
-> seq_open()
-> seq_file->private = inode;
So instead of 'inode' we can pass the 'struct file' to single_open(),
and get the 'inode' and 'file->f_cred' later at any point.
If we go for this, then later other files like /proc/*/{maps,smaps}
that use the 'struct proc_maps_private' should also embed a pointer
to the 'struct file' in that struct. These files use seq files and their
seq_file->private point to this 'struct proc_maps_private'.
So:
Sensitive ONE files can use this solution.
Sensitive INF files need to be converted to REG files and have their own
file operations, like it's done in
[PATCH 11/12] procfs: improve permission checks on /proc/*/syscall
Other REG files will receive the 'struct file' as an arugment, and for
files that use seq files, we should find a way to embed a pointer to
the 'struct file'.
2) Like (1) but instead of using the 'struct file' we pass the adress of
&file->f_inode. We can have 'struct file' using container_of and we also
have the inode. But it will just add more extra level of indirections.
I'm not sure of this one! I don't like it, what about other
/proc/<pid>/* files ? Is this consistent ?
3) Make the sensitive files like /proc/*/{stack,stat} have their own
file_operations. These are ONE nodes that share the same code with the
other ONE files.
I've already done this for /proc/*/syscall that shares code with other
INF files:
[PATCH 11/12] procfs: improve permission checks on /proc/*/syscall
That was the only way I found to have appropriate permission checks and
to not break other files. The /proc/<pid>/auxv will also need its own
file_operations.
We can also argue that sharing code is good or not as good as we think.
Example there is extra unused code for the /proc/*/stack
proc_pid_stack() handler. This function never use its 'pid and ns'
arguments, so why bother to retrieve them!
If we go for this (3) there will be:
* More extra code but optimized for the corresponding file.
* We should not touch seq_file struct.
* These files will still continue to use seq files.
* We'll embed a pointer to the 'struct file' inside
'struct proc_maps_private', so we can protect /proc/<pid>/{maps,smaps}
files later. They use seq files, the check will be implemented in
their m_start() handler.
Personally I'll go for (3) since we'll do the same for some INF files.
Al, what do you think ?
--
Djalal Harouni
http://opendz.org
On Thu, Sep 26, 2013 at 03:43:24PM -0500, Kees Cook wrote:
> On Wed, Sep 25, 2013 at 3:14 PM, Djalal Harouni <[email protected]> wrote:
> > The /proc/*/stack contains sensitive information and currently its mode
> > is 0444. Change this to 0400 so the VFS will be able to block
> > unprivileged processes to get file descriptors on arbitrary privileged
> > /proc/*/stack files.
> >
> > The /proc/*/stack is a /procfs ONE file that shares the same ->open()
> > file operation with other ONE files. Doing a ptrace_may_access() check
> > during open() might break userspace from accessing other ONE files
> > like /proc/*/stat and /proc/*/statm.
> >
> > Therfore make it 0400 for now, and improve its check during ->read()
> > in the next following patch.
> >
> > Cc: Kees Cook <[email protected]>
> > Cc: Eric W. Biederman <[email protected]>
> > Signed-off-by: Djalal Harouni <[email protected]>
>
> While the rest of the series is being discussed, I think it would be
> nice to at least get this into the tree. Fixing this reduces which
> processes are exposed to ASLR leaks. The rest of the series closes the
> remaining holes.
>
> I would if it would be valuable adding a test for the identified leak
> conditions to some test suite? LTP perhaps?
I'm not familiar with LTP, but I guess a small program that perform I/O
redirection and execve a suid-exec will do it?
I'll try to add code comment in fs/proc/base.c
> -Kees
>
> --
> Kees Cook
> Chrome OS Security
--
Djalal Harouni
http://opendz.org
On Thu, Sep 26, 2013 at 04:02:54AM +0100, Al Viro wrote:
> On Wed, Sep 25, 2013 at 05:22:51PM -0700, Linus Torvalds wrote:
> > On Wed, Sep 25, 2013 at 1:14 PM, Djalal Harouni <[email protected]> wrote:
> > >
> > > Therefor add the f_cred field to the seq_file struct and a helper
> > > seq_f_cred() to return it.
> >
> > I hate how you've split up this patch from the next one that actually
> > _initializes_ the new field.
> >
> > The two patches should have been one.
> >
> > I think the patch should also remove the 'user_ns' member, since it's
> > now available as f_cred->user_ns.
> >
> > I also suspect that it would be better to just make the the new
> > seq_file member point to the 'struct file' instead. Sure, it's an
> > extra level of indirection, but the lifetime of f_cred is not as clear
> > otherwise. You don't increment the reference count, which is correct
> > *only* because 'seq_file' has the same lifetime as 'struct file', and
> > thus the reference count from struct file for the f_cred is
> > sufficient.
>
> That's better than f_cred (or user_ns, for that matter), but... I'm
> afraid that it'll get abused very soon. And I don't understand the
> argument about the lifetime rules - what makes struct file ones
> different from struct cred ones in that respect? Except that in this
> case it's really obvious that we can't grab a reference, that is...
For that reference count lifetime rule. Sorry I was trying to compare it
with other implemented solutions. /proc/pid/environ will increment the
mm->mm_count inside __mem_open(), thus mm_struct will stay in until
mem_release(). That's it.
In the proposed solution and as noted by Linus, the reference count
from 'struct file' is sufficient for f_cred.
Last, I'm happy with /proc/pid/environ as it is, will not touch it
unless there is a request for it, but this proposed file->f_cred
solution is better:
* Not *all* these /proc/<pid>/* files are interested in acquiring a
reference to the task's mm and increment its counters.
* It can be adapted to all the /proc/<pid>/* files, we already have the
file->f_cred
* It allow to pass file descriptors if original opener had enough
permission. It's explained in [Patch 0/12]
* It allow referencing the correct mm of the currently running
target task at read(),write()...
Where other solutions will just block/restrict this behaviour.
Thanks
--
Djalal Harouni
http://opendz.org
On Thu, Sep 26, 2013 at 03:43:24PM -0500, Kees Cook wrote:
> On Wed, Sep 25, 2013 at 3:14 PM, Djalal Harouni <[email protected]> wrote:
> > The /proc/*/stack contains sensitive information and currently its mode
> > is 0444. Change this to 0400 so the VFS will be able to block
> > unprivileged processes to get file descriptors on arbitrary privileged
> > /proc/*/stack files.
> >
> > The /proc/*/stack is a /procfs ONE file that shares the same ->open()
> > file operation with other ONE files. Doing a ptrace_may_access() check
> > during open() might break userspace from accessing other ONE files
> > like /proc/*/stat and /proc/*/statm.
> >
> > Therfore make it 0400 for now, and improve its check during ->read()
> > in the next following patch.
> >
> > Cc: Kees Cook <[email protected]>
> > Cc: Eric W. Biederman <[email protected]>
> > Signed-off-by: Djalal Harouni <[email protected]>
>
> While the rest of the series is being discussed, I think it would be
> nice to at least get this into the tree. Fixing this reduces which
> processes are exposed to ASLR leaks. The rest of the series closes the
> remaining holes.
Kees I guess it's ok to add your Acked-by for this one, for v2
--
Djalal Harouni
http://opendz.org
On Sun, Sep 29, 2013 at 3:37 AM, Djalal Harouni <[email protected]> wrote:
> On Thu, Sep 26, 2013 at 03:43:24PM -0500, Kees Cook wrote:
>> On Wed, Sep 25, 2013 at 3:14 PM, Djalal Harouni <[email protected]> wrote:
>> > The /proc/*/stack contains sensitive information and currently its mode
>> > is 0444. Change this to 0400 so the VFS will be able to block
>> > unprivileged processes to get file descriptors on arbitrary privileged
>> > /proc/*/stack files.
>> >
>> > The /proc/*/stack is a /procfs ONE file that shares the same ->open()
>> > file operation with other ONE files. Doing a ptrace_may_access() check
>> > during open() might break userspace from accessing other ONE files
>> > like /proc/*/stat and /proc/*/statm.
>> >
>> > Therfore make it 0400 for now, and improve its check during ->read()
>> > in the next following patch.
>> >
>> > Cc: Kees Cook <[email protected]>
>> > Cc: Eric W. Biederman <[email protected]>
>> > Signed-off-by: Djalal Harouni <[email protected]>
>>
>> While the rest of the series is being discussed, I think it would be
>> nice to at least get this into the tree. Fixing this reduces which
>> processes are exposed to ASLR leaks. The rest of the series closes the
>> remaining holes.
> Kees I guess it's ok to add your Acked-by for this one, for v2
Yes, please. :)
Acked-by: Kees Cook <[email protected]>
-Kees
--
Kees Cook
Chrome OS Security
On Sat, Sep 28, 2013 at 7:35 AM, Djalal Harouni <[email protected]> wrote:
> On Thu, Sep 26, 2013 at 03:43:24PM -0500, Kees Cook wrote:
>> On Wed, Sep 25, 2013 at 3:14 PM, Djalal Harouni <[email protected]> wrote:
>> > The /proc/*/stack contains sensitive information and currently its mode
>> > is 0444. Change this to 0400 so the VFS will be able to block
>> > unprivileged processes to get file descriptors on arbitrary privileged
>> > /proc/*/stack files.
>> >
>> > The /proc/*/stack is a /procfs ONE file that shares the same ->open()
>> > file operation with other ONE files. Doing a ptrace_may_access() check
>> > during open() might break userspace from accessing other ONE files
>> > like /proc/*/stat and /proc/*/statm.
>> >
>> > Therfore make it 0400 for now, and improve its check during ->read()
>> > in the next following patch.
>> >
>> > Cc: Kees Cook <[email protected]>
>> > Cc: Eric W. Biederman <[email protected]>
>> > Signed-off-by: Djalal Harouni <[email protected]>
>>
>> While the rest of the series is being discussed, I think it would be
>> nice to at least get this into the tree. Fixing this reduces which
>> processes are exposed to ASLR leaks. The rest of the series closes the
>> remaining holes.
>>
>> I would if it would be valuable adding a test for the identified leak
>> conditions to some test suite? LTP perhaps?
> I'm not familiar with LTP, but I guess a small program that perform I/O
> redirection and execve a suid-exec will do it?
It's actually a giant program. :)
http://ltp.sourceforge.net/
> I'll try to add code comment in fs/proc/base.c
Mostly I'm just thinking that this is a rather fragile area, and it
would be nice to have a set of tests that verify we haven't broken any
of the known test-cases.
-Kees
--
Kees Cook
Chrome OS Security