On Wed, Dec 31, 2008 at 03:15:42PM +0000, David Howells wrote: > > Here's an improved patch. It differentiates the use of objective and > subjective capabilities by making capable() only check current's subjective > caps, but making has_capability() check only the objective caps of whatever > process is specified. > > It's a bit more involved, but I think it's the right thing to do. > > David > > --- > From: David Howells <dhowells@xxxxxxxxxx> > Subject: [PATCH] CRED: Fix regression in cap_capable() as shown up by sys_faccessat() > > Fix a regression in cap_capable() due to: > > commit 5ff7711e635b32f0a1e558227d030c7e45b4a465 By the way, this should be 3b11a1decef07c19443d24ae926982bc8ec9f4c0, which is the patch that was committed by James Morris and is in Linus's tree. I assume 5ff7711... is the commitid in your tree and that James applied it as a patch instead of pulling from you. --b. > Author: David Howells <dhowells@xxxxxxxxxx> > Date: Wed Dec 31 02:52:28 2008 +0000 > > CRED: Differentiate objective and effective subjective credentials on a task > > > The problem is that the above patch allows a process to have two sets of > credentials, and for the most part uses the subjective credentials when > accessing current's creds. > > There is, however, one exception: cap_capable(), and thus capable(), uses the > real/objective credentials of the target task, whether or not it is the current > task. > > Ordinarily this doesn't matter, since usually the two cred pointers in current > point to the same set of creds. However, sys_faccessat() makes use of this > facility to override the credentials of the calling process to make its test, > without affecting the creds as seen from other processes. > > One of the things sys_faccessat() does is to make an adjustment to the > effective capabilities mask, which cap_capable(), as it stands, then ignores. > > The affected capability check is in generic_permission(): > > if (!(mask & MAY_EXEC) || execute_ok(inode)) > if (capable(CAP_DAC_OVERRIDE)) > return 0; > > > This change splits capable() from has_capability() down into the commoncap and > SELinux code. The capable() security op now only deals with the current > process, and uses the current process's subjective creds. A new security op - > task_capable() - is introduced that can check any task's objective creds. > > strictly the capable() security op is superfluous with the presence of the > task_capable() op, however it should be faster to call the capable() op since > two fewer arguments need be passed down through the various layers. > > > This can be tested by compiling the following program from the XFS testsuite: > > /* > * t_access_root.c - trivial test program to show permission bug. > * > * Written by Michael Kerrisk - copyright ownership not pursued. > * Sourced from: http://linux.derkeiler.com/Mailing-Lists/Kernel/2003-10/6030.html > */ > #include <limits.h> > #include <unistd.h> > #include <stdio.h> > #include <stdlib.h> > #include <fcntl.h> > #include <sys/stat.h> > > #define UID 500 > #define GID 100 > #define PERM 0 > #define TESTPATH "/tmp/t_access" > > static void > errExit(char *msg) > { > perror(msg); > exit(EXIT_FAILURE); > } /* errExit */ > > static void > accessTest(char *file, int mask, char *mstr) > { > printf("access(%s, %s) returns %d\n", file, mstr, access(file, mask)); > } /* accessTest */ > > int > main(int argc, char *argv[]) > { > int fd, perm, uid, gid; > char *testpath; > char cmd[PATH_MAX + 20]; > > testpath = (argc > 1) ? argv[1] : TESTPATH; > perm = (argc > 2) ? strtoul(argv[2], NULL, 8) : PERM; > uid = (argc > 3) ? atoi(argv[3]) : UID; > gid = (argc > 4) ? atoi(argv[4]) : GID; > > unlink(testpath); > > fd = open(testpath, O_RDWR | O_CREAT, 0); > if (fd == -1) errExit("open"); > > if (fchown(fd, uid, gid) == -1) errExit("fchown"); > if (fchmod(fd, perm) == -1) errExit("fchmod"); > close(fd); > > snprintf(cmd, sizeof(cmd), "ls -l %s", testpath); > system(cmd); > > if (seteuid(uid) == -1) errExit("seteuid"); > > accessTest(testpath, 0, "0"); > accessTest(testpath, R_OK, "R_OK"); > accessTest(testpath, W_OK, "W_OK"); > accessTest(testpath, X_OK, "X_OK"); > accessTest(testpath, R_OK | W_OK, "R_OK | W_OK"); > accessTest(testpath, R_OK | X_OK, "R_OK | X_OK"); > accessTest(testpath, W_OK | X_OK, "W_OK | X_OK"); > accessTest(testpath, R_OK | W_OK | X_OK, "R_OK | W_OK | X_OK"); > > exit(EXIT_SUCCESS); > } /* main */ > > > This can be run against an Ext3 filesystem as well as against an XFS > filesystem. If successful, it will show: > > [root@andromeda src]# ./t_access_root /tmp/xxx 0 4043 4043 > ---------- 1 dhowells dhowells 0 2008-12-31 03:00 /tmp/xxx > access(/tmp/xxx, 0) returns 0 > access(/tmp/xxx, R_OK) returns 0 > access(/tmp/xxx, W_OK) returns 0 > access(/tmp/xxx, X_OK) returns -1 > access(/tmp/xxx, R_OK | W_OK) returns 0 > access(/tmp/xxx, R_OK | X_OK) returns -1 > access(/tmp/xxx, W_OK | X_OK) returns -1 > access(/tmp/xxx, R_OK | W_OK | X_OK) returns -1 > > If unsuccessful, it will show: > > [root@andromeda src]# ./t_access_root /tmp/xxx 0 4043 4043 > ---------- 1 dhowells dhowells 0 2008-12-31 02:56 /tmp/xxx > access(/tmp/xxx, 0) returns 0 > access(/tmp/xxx, R_OK) returns -1 > access(/tmp/xxx, W_OK) returns -1 > access(/tmp/xxx, X_OK) returns -1 > access(/tmp/xxx, R_OK | W_OK) returns -1 > access(/tmp/xxx, R_OK | X_OK) returns -1 > access(/tmp/xxx, W_OK | X_OK) returns -1 > access(/tmp/xxx, R_OK | W_OK | X_OK) returns -1 > > I've also tested the fix with the SELinux and syscalls LTP testsuites. > > Signed-off-by: David Howells <dhowells@xxxxxxxxxx> > --- > > include/linux/capability.h | 17 +++++++++++++-- > include/linux/security.h | 49 ++++++++++++++++++++++++++++++++++++-------- > kernel/capability.c | 2 +- > security/capability.c | 1 + > security/commoncap.c | 42 ++++++++++++++++++++++++++------------ > security/root_plug.c | 1 + > security/security.c | 25 +++++++++++++++++++--- > security/selinux/hooks.c | 26 ++++++++++++++++++----- > security/smack/smack_lsm.c | 1 + > 9 files changed, 129 insertions(+), 35 deletions(-) > > > diff --git a/include/linux/capability.h b/include/linux/capability.h > index e22f48c..5b8a132 100644 > --- a/include/linux/capability.h > +++ b/include/linux/capability.h > @@ -529,8 +529,21 @@ extern const kernel_cap_t __cap_init_eff_set; > * > * Note that this does not set PF_SUPERPRIV on the task. > */ > -#define has_capability(t, cap) (security_capable((t), (cap)) == 0) > -#define has_capability_noaudit(t, cap) (security_capable_noaudit((t), (cap)) == 0) > +#define has_capability(t, cap) (security_task_capable((t), (cap)) == 0) > + > +/** > + * has_capability_noaudit - Determine if a task has a superior capability available (unaudited) > + * @t: The task in question > + * @cap: The capability to be tested for > + * > + * Return true if the specified task has the given superior capability > + * currently in effect, false if not, but don't write an audit message for the > + * check. > + * > + * Note that this does not set PF_SUPERPRIV on the task. > + */ > +#define has_capability_noaudit(t, cap) \ > + (security_task_capable_noaudit((t), (cap)) == 0) > > extern int capable(int cap); > > diff --git a/include/linux/security.h b/include/linux/security.h > index 3416cb8..76989b8 100644 > --- a/include/linux/security.h > +++ b/include/linux/security.h > @@ -48,7 +48,9 @@ struct audit_krule; > * These functions are in security/capability.c and are used > * as the default capabilities functions > */ > -extern int cap_capable(struct task_struct *tsk, int cap, int audit); > +extern int cap_capable(int cap, int audit); > +extern int cap_task_capable(struct task_struct *tsk, const struct cred *cred, > + int cap, int audit); > extern int cap_settime(struct timespec *ts, struct timezone *tz); > extern int cap_ptrace_may_access(struct task_struct *child, unsigned int mode); > extern int cap_ptrace_traceme(struct task_struct *parent); > @@ -1195,9 +1197,18 @@ static inline void security_free_mnt_opts(struct security_mnt_opts *opts) > * @permitted contains the permitted capability set. > * Return 0 and update @new if permission is granted. > * @capable: > - * Check whether the @tsk process has the @cap capability. > + * Check whether the current process has the @cap capability in its > + * subjective/effective credentials. > + * @cap contains the capability <include/linux/capability.h>. > + * @audit: Whether to write an audit message or not > + * Return 0 if the capability is granted for @tsk. > + * @task_capable: > + * Check whether the @tsk process has the @cap capability in its > + * objective/real credentials. > * @tsk contains the task_struct for the process. > + * @cred contains the credentials to use. > * @cap contains the capability <include/linux/capability.h>. > + * @audit: Whether to write an audit message or not > * Return 0 if the capability is granted for @tsk. > * @acct: > * Check permission before enabling or disabling process accounting. If > @@ -1290,7 +1301,9 @@ struct security_operations { > const kernel_cap_t *effective, > const kernel_cap_t *inheritable, > const kernel_cap_t *permitted); > - int (*capable) (struct task_struct *tsk, int cap, int audit); > + int (*capable) (int cap, int audit); > + int (*task_capable) (struct task_struct *tsk, const struct cred *cred, > + int cap, int audit); > int (*acct) (struct file *file); > int (*sysctl) (struct ctl_table *table, int op); > int (*quotactl) (int cmds, int type, int id, struct super_block *sb); > @@ -1556,8 +1569,9 @@ int security_capset(struct cred *new, const struct cred *old, > const kernel_cap_t *effective, > const kernel_cap_t *inheritable, > const kernel_cap_t *permitted); > -int security_capable(struct task_struct *tsk, int cap); > -int security_capable_noaudit(struct task_struct *tsk, int cap); > +int security_capable(int cap); > +int security_task_capable(struct task_struct *tsk, int cap); > +int security_task_capable_noaudit(struct task_struct *tsk, int cap); > int security_acct(struct file *file); > int security_sysctl(struct ctl_table *table, int op); > int security_quotactl(int cmds, int type, int id, struct super_block *sb); > @@ -1754,14 +1768,31 @@ static inline int security_capset(struct cred *new, > return cap_capset(new, old, effective, inheritable, permitted); > } > > -static inline int security_capable(struct task_struct *tsk, int cap) > +static inline int security_capable(int cap) > { > - return cap_capable(tsk, cap, SECURITY_CAP_AUDIT); > + return cap_capable(cap, SECURITY_CAP_AUDIT); > } > > -static inline int security_capable_noaudit(struct task_struct *tsk, int cap) > +static inline int security_task_capable(struct task_struct *tsk, int cap) > { > - return cap_capable(tsk, cap, SECURITY_CAP_NOAUDIT); > + int ret; > + > + rcu_read_lock(); > + ret = cap_task_capable(tsk, __task_cred(tsk), cap, SECURITY_CAP_AUDIT); > + rcu_read_unlock(); > + return ret; > +} > + > +static inline > +int security_task_capable_noaudit(struct task_struct *tsk, int cap) > +{ > + int ret; > + > + rcu_read_lock(); > + ret = cap_task_capable(tsk, __task_cred(tsk), cap, > + SECURITY_CAP_NOAUDIT); > + rcu_read_unlock(); > + return ret; > } > > static inline int security_acct(struct file *file) > diff --git a/kernel/capability.c b/kernel/capability.c > index 36b4b4d..df62f53 100644 > --- a/kernel/capability.c > +++ b/kernel/capability.c > @@ -308,7 +308,7 @@ int capable(int cap) > BUG(); > } > > - if (has_capability(current, cap)) { > + if (security_capable(cap) == 0) { > current->flags |= PF_SUPERPRIV; > return 1; > } > diff --git a/security/capability.c b/security/capability.c > index 2dce66f..fd1493d 100644 > --- a/security/capability.c > +++ b/security/capability.c > @@ -826,6 +826,7 @@ void security_fixup_ops(struct security_operations *ops) > set_to_cap_if_null(ops, capset); > set_to_cap_if_null(ops, acct); > set_to_cap_if_null(ops, capable); > + set_to_cap_if_null(ops, task_capable); > set_to_cap_if_null(ops, quotactl); > set_to_cap_if_null(ops, quota_on); > set_to_cap_if_null(ops, sysctl); > diff --git a/security/commoncap.c b/security/commoncap.c > index 7971354..7f0b2a6 100644 > --- a/security/commoncap.c > +++ b/security/commoncap.c > @@ -43,28 +43,44 @@ int cap_netlink_recv(struct sk_buff *skb, int cap) > EXPORT_SYMBOL(cap_netlink_recv); > > /** > - * cap_capable - Determine whether a task has a particular effective capability > - * @tsk: The task to query > + * cap_capable - Determine whether current has a particular effective capability > * @cap: The capability to check for > * @audit: Whether to write an audit message or not > * > * Determine whether the nominated task has the specified capability amongst > - * its effective set, returning 0 if it does, -ve if it does not. > + * its effective set, returning 0 if it does, -ve if it does not. Note that > + * this uses current's subjective/effective credentials. > * > * NOTE WELL: cap_capable() cannot be used like the kernel's capable() > * function. That is, it has the reverse semantics: cap_capable() returns 0 > * when a task has a capability, but the kernel's capable() returns 1 for this > * case. > */ > -int cap_capable(struct task_struct *tsk, int cap, int audit) > +int cap_capable(int cap, int audit) > { > - __u32 cap_raised; > + return cap_raised(current_cap(), cap) ? 0 : -EPERM; > +} > > - /* Derived from include/linux/sched.h:capable. */ > - rcu_read_lock(); > - cap_raised = cap_raised(__task_cred(tsk)->cap_effective, cap); > - rcu_read_unlock(); > - return cap_raised ? 0 : -EPERM; > +/** > + * cap_has_capability - Determine whether a task has a particular effective capability > + * @tsk: The task to query > + * @cred: The credentials to use > + * @cap: The capability to check for > + * @audit: Whether to write an audit message or not > + * > + * Determine whether the nominated task has the specified capability amongst > + * its effective set, returning 0 if it does, -ve if it does not. Note that > + * this uses the task's objective/real credentials. > + * > + * NOTE WELL: cap_has_capability() cannot be used like the kernel's > + * has_capability() function. That is, it has the reverse semantics: > + * cap_has_capability() returns 0 when a task has a capability, but the > + * kernel's has_capability() returns 1 for this case. > + */ > +int cap_task_capable(struct task_struct *tsk, const struct cred *cred, int cap, > + int audit) > +{ > + return cap_raised(cred->cap_effective, cap) ? 0 : -EPERM; > } > > /** > @@ -160,7 +176,7 @@ static inline int cap_inh_is_capped(void) > /* they are so limited unless the current task has the CAP_SETPCAP > * capability > */ > - if (cap_capable(current, CAP_SETPCAP, SECURITY_CAP_AUDIT) == 0) > + if (cap_capable(CAP_SETPCAP, SECURITY_CAP_AUDIT) == 0) > return 0; > #endif > return 1; > @@ -869,7 +885,7 @@ int cap_task_prctl(int option, unsigned long arg2, unsigned long arg3, > & (new->securebits ^ arg2)) /*[1]*/ > || ((new->securebits & SECURE_ALL_LOCKS & ~arg2)) /*[2]*/ > || (arg2 & ~(SECURE_ALL_LOCKS | SECURE_ALL_BITS)) /*[3]*/ > - || (cap_capable(current, CAP_SETPCAP, SECURITY_CAP_AUDIT) != 0) /*[4]*/ > + || (cap_capable(CAP_SETPCAP, SECURITY_CAP_AUDIT) != 0) /*[4]*/ > /* > * [1] no changing of bits that are locked > * [2] no unlocking of locks > @@ -950,7 +966,7 @@ int cap_vm_enough_memory(struct mm_struct *mm, long pages) > { > int cap_sys_admin = 0; > > - if (cap_capable(current, CAP_SYS_ADMIN, SECURITY_CAP_NOAUDIT) == 0) > + if (cap_capable(CAP_SYS_ADMIN, SECURITY_CAP_NOAUDIT) == 0) > cap_sys_admin = 1; > return __vm_enough_memory(mm, pages, cap_sys_admin); > } > diff --git a/security/root_plug.c b/security/root_plug.c > index 40fb4f1..559578f 100644 > --- a/security/root_plug.c > +++ b/security/root_plug.c > @@ -77,6 +77,7 @@ static struct security_operations rootplug_security_ops = { > .capget = cap_capget, > .capset = cap_capset, > .capable = cap_capable, > + .task_capable = cap_task_capable, > > .bprm_set_creds = cap_bprm_set_creds, > > diff --git a/security/security.c b/security/security.c > index d85dbb3..9bbc8e5 100644 > --- a/security/security.c > +++ b/security/security.c > @@ -154,14 +154,31 @@ int security_capset(struct cred *new, const struct cred *old, > effective, inheritable, permitted); > } > > -int security_capable(struct task_struct *tsk, int cap) > +int security_capable(int cap) > { > - return security_ops->capable(tsk, cap, SECURITY_CAP_AUDIT); > + return security_ops->capable(cap, SECURITY_CAP_AUDIT); > } > > -int security_capable_noaudit(struct task_struct *tsk, int cap) > +int security_task_capable(struct task_struct *tsk, int cap) > { > - return security_ops->capable(tsk, cap, SECURITY_CAP_NOAUDIT); > + const struct cred *cred; > + int ret; > + > + cred = get_task_cred(tsk); > + ret = security_ops->task_capable(tsk, cred, cap, SECURITY_CAP_AUDIT); > + put_cred(cred); > + return ret; > +} > + > +int security_task_capable_noaudit(struct task_struct *tsk, int cap) > +{ > + const struct cred *cred; > + int ret; > + > + cred = get_task_cred(tsk); > + ret = security_ops->task_capable(tsk, cred, cap, SECURITY_CAP_NOAUDIT); > + put_cred(cred); > + return ret; > } > > int security_acct(struct file *file) > diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c > index dbeaa78..5a66cd3 100644 > --- a/security/selinux/hooks.c > +++ b/security/selinux/hooks.c > @@ -1433,12 +1433,13 @@ static int current_has_perm(const struct task_struct *tsk, > > /* Check whether a task is allowed to use a capability. */ > static int task_has_capability(struct task_struct *tsk, > + const struct cred *cred, > int cap, int audit) > { > struct avc_audit_data ad; > struct av_decision avd; > u16 sclass; > - u32 sid = task_sid(tsk); > + u32 sid = cred_sid(cred); > u32 av = CAP_TO_MASK(cap); > int rc; > > @@ -1865,15 +1866,27 @@ static int selinux_capset(struct cred *new, const struct cred *old, > return cred_has_perm(old, new, PROCESS__SETCAP); > } > > -static int selinux_capable(struct task_struct *tsk, int cap, int audit) > +static int selinux_capable(int cap, int audit) > +{ > + int rc; > + > + rc = secondary_ops->capable(cap, audit); > + if (rc) > + return rc; > + > + return task_has_capability(current, current_cred(), cap, audit); > +} > + > +static int selinux_task_capable(struct task_struct *tsk, > + const struct cred *cred, int cap, int audit) > { > int rc; > > - rc = secondary_ops->capable(tsk, cap, audit); > + rc = secondary_ops->task_capable(tsk, cred, cap, audit); > if (rc) > return rc; > > - return task_has_capability(tsk, cap, audit); > + return task_has_capability(tsk, cred, cap, audit); > } > > static int selinux_sysctl_get_sid(ctl_table *table, u16 tclass, u32 *sid) > @@ -2037,7 +2050,7 @@ static int selinux_vm_enough_memory(struct mm_struct *mm, long pages) > { > int rc, cap_sys_admin = 0; > > - rc = selinux_capable(current, CAP_SYS_ADMIN, SECURITY_CAP_NOAUDIT); > + rc = selinux_capable(CAP_SYS_ADMIN, SECURITY_CAP_NOAUDIT); > if (rc == 0) > cap_sys_admin = 1; > > @@ -2880,7 +2893,7 @@ static int selinux_inode_getsecurity(const struct inode *inode, const char *name > * and lack of permission just means that we fall back to the > * in-core context value, not a denial. > */ > - error = selinux_capable(current, CAP_MAC_ADMIN, SECURITY_CAP_NOAUDIT); > + error = selinux_capable(CAP_MAC_ADMIN, SECURITY_CAP_NOAUDIT); > if (!error) > error = security_sid_to_context_force(isec->sid, &context, > &size); > @@ -5568,6 +5581,7 @@ static struct security_operations selinux_ops = { > .capset = selinux_capset, > .sysctl = selinux_sysctl, > .capable = selinux_capable, > + .task_capable = selinux_task_capable, > .quotactl = selinux_quotactl, > .quota_on = selinux_quota_on, > .syslog = selinux_syslog, > diff --git a/security/smack/smack_lsm.c b/security/smack/smack_lsm.c > index 1b5551d..8e53d43 100644 > --- a/security/smack/smack_lsm.c > +++ b/security/smack/smack_lsm.c > @@ -2628,6 +2628,7 @@ struct security_operations smack_ops = { > .capget = cap_capget, > .capset = cap_capset, > .capable = cap_capable, > + .task_capable = cap_task_capable, > .syslog = smack_syslog, > .settime = cap_settime, > .vm_enough_memory = cap_vm_enough_memory, > -- > To unsubscribe from this list: send the line "unsubscribe linux-fsdevel" in > the body of a message to majordomo@xxxxxxxxxxxxxxx > More majordomo info at http://vger.kernel.org/majordomo-info.html -- To unsubscribe from this list: send the line "unsubscribe linux-fsdevel" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
