What is the justification for needing to grab task_capability_lock when
applying bprm caps to current during exec->compute_creds?
If I'm not mistaken, we do in fact need the below patch to prevent
weirdnesses between fork and capset/capget. It also addresses a FIXME
in fs/open.c where sys_access needs to set capabilities by hand without
the lock. My first implementation actually introduced PF_LOCK_CAPS
which prevent capset from succeeding during the sys_access, but taking
a task flag for that seemed wrong.
Below is compile-tested, nothing more.
So do we need this or am I wrong about the lock's purpose?
-serge
>From f128cb4f97ad8d1e735917ffbdca01769cb52fd8 Mon Sep 17 00:00:00 2001
From: [EMAIL PROTECTED] <[EMAIL PROTECTED](none)>
Date: Mon, 17 Sep 2007 11:59:33 -0700
Subject: [PATCH 1/1] capabilities: properly use task_capability_lock
Since capabilities are comprised of several values, concurrent access
needs to be protected. This hasn't been properly done... at least
since late 2.4 kernels.
Some of the potential races now possible include:
capget with exec (worse case corrupt capget output)
capset with exec (could be bad)
sys_access with capset (could be bad)
This patch tries to protect from these races by exporting the
task_capability_lock so it can be used when needed. For
sys_access, if someone capsets us in between our task_capability_lock
holds, then that person had priv to do so, and we just don't reset
the cap_effective to our original value. (The sys_access results
may however be invalid since we may not have had the right capabilities).
Signed-off-by: Serge Hallyn <[EMAIL PROTECTED]>
---
fs/open.c | 23 ++++++++++++-----------
include/linux/capability.h | 3 +++
kernel/capability.c | 2 +-
security/commoncap.c | 5 +++++
5 files changed, 23 insertions(+), 15 deletions(-)
diff --git a/fs/open.c b/fs/open.c
index 23f334d..358759c 100644
--- a/fs/open.c
+++ b/fs/open.c
@@ -422,7 +422,7 @@ asmlinkage long sys_faccessat(int dfd, const char __user
*filename, int mode)
{
struct nameidata nd;
int old_fsuid, old_fsgid;
- kernel_cap_t old_cap;
+ kernel_cap_t old_cap, tmp_cap;
int res;
if (mode & ~S_IRWXO) /* where's F_OK, X_OK, W_OK, R_OK? */
@@ -430,23 +430,21 @@ asmlinkage long sys_faccessat(int dfd, const char __user
*filename, int mode)
old_fsuid = current->fsuid;
old_fsgid = current->fsgid;
- old_cap = current->cap_effective;
-
current->fsuid = current->uid;
current->fsgid = current->gid;
/*
* Clear the capabilities if we switch to a non-root user
- *
- * FIXME: There is a race here against sys_capset. The
- * capabilities can change yet we will restore the old
- * value below. We should hold task_capabilities_lock,
- * but we cannot because user_path_walk can sleep.
*/
+ spin_lock(&task_capability_lock);
+ old_cap = current->cap_effective;
if (current->uid)
- cap_clear(current->cap_effective);
+ cap_clear(tmp_cap);
else
- current->cap_effective = current->cap_permitted;
+ tmp_cap = current->cap_permitted;
+
+ current->cap_effective = tmp_cap;
+ spin_unlock(&task_capability_lock);
res = __user_walk_fd(dfd, filename, LOOKUP_FOLLOW|LOOKUP_ACCESS, &nd);
if (res)
@@ -466,7 +464,10 @@ out_path_release:
out:
current->fsuid = old_fsuid;
current->fsgid = old_fsgid;
- current->cap_effective = old_cap;
+ spin_lock(&task_capability_lock);
+ if (current->cap_effective == tmp_cap)
+ current->cap_effective = old_cap;
+ spin_unlock(&task_capability_lock);
return res;
}
diff --git a/include/linux/capability.h b/include/linux/capability.h
index 7a8d7ad..3b295c5 100644
--- a/include/linux/capability.h
+++ b/include/linux/capability.h
@@ -31,6 +31,9 @@ struct task_struct;
#define _LINUX_CAPABILITY_VERSION 0x19980330
+#include <linux/spinlock.h>
+extern spinlock_t task_capability_lock;
+
typedef struct __user_cap_header_struct {
__u32 version;
int pid;
diff --git a/kernel/capability.c b/kernel/capability.c
index d4377c5..ed40e97 100644
--- a/kernel/capability.c
+++ b/kernel/capability.c
@@ -19,7 +19,7 @@
* This lock protects task->cap_* for all tasks including current.
* Locking rule: acquire this prior to tasklist_lock.
*/
-static DEFINE_SPINLOCK(task_capability_lock);
+DEFINE_SPINLOCK(task_capability_lock);
/*
* For sys_getproccap() and sys_setproccap(), any of the three
diff --git a/security/commoncap.c b/security/commoncap.c
index 43f9027..69b2e31 100644
--- a/security/commoncap.c
+++ b/security/commoncap.c
@@ -120,6 +120,7 @@ static inline int cap_inh_is_capped(void) { return 1; }
#endif /* def CONFIG_SECURITY_FILE_CAPABILITIES */
+/* Called with task_capability_lock held */
int cap_capset_check (struct task_struct *target, kernel_cap_t *effective,
kernel_cap_t *inheritable, kernel_cap_t *permitted)
{
@@ -149,6 +150,7 @@ int cap_capset_check (struct task_struct *target,
kernel_cap_t *effective,
return 0;
}
+/* Called with task_capability_lock held */
void cap_capset_set (struct task_struct *target, kernel_cap_t *effective,
kernel_cap_t *inheritable, kernel_cap_t *permitted)
{
@@ -307,6 +309,8 @@ void cap_bprm_apply_creds (struct linux_binprm *bprm, int
unsafe)
/* Derived from fs/exec.c:compute_creds. */
kernel_cap_t new_permitted, working;
+ spin_lock(&task_capability_lock);
+
new_permitted = cap_intersect (bprm->cap_permitted, cap_bset);
working = cap_intersect (bprm->cap_inheritable,
current->cap_inheritable);
@@ -344,6 +348,7 @@ void cap_bprm_apply_creds (struct linux_binprm *bprm, int
unsafe)
/* AUD: Audit candidate if current->cap_effective is set */
current->keep_capabilities = 0;
+ spin_unlock(&task_capability_lock);
}
int cap_bprm_secureexec (struct linux_binprm *bprm)
--
1.5.1
-
To unsubscribe from this list: send the line "unsubscribe
linux-security-module" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
