[PATCH] credentials locking fix

From: Chris Wright <chrisw@osdl.org>

Contributions from:
Stephen Smalley <sds@epoch.ncsc.mil>
Andy Lutomirski <luto@stanford.edu>

During exec the LSM bprm_apply_creds() hooks may tranisition the program to a
new security context (like setuid binaries).  The security context of the new
task is dependent on state such as if the task is being ptraced.

ptrace_detach() doesn't take the task_lock() when clearing task->ptrace.  So
there is a race possible where a process starts off being ptraced, the
malicious ptracer detaches and if any checks agains task->ptrace are done more
than once, the results are indeterminate.

This patch ensures task_lock() is held while bprm_apply_creds() hooks are
called, keeping it safe against ptrace_attach() races.  Additionally, tests
against task->ptrace (and ->fs->count, ->files->count and ->sighand->count all
of which signify potential unsafe resource sharing during a security context
transition) are done only once the results are passed down to hooks, making it
safe against ptrace_detach() races.

Additionally:

- s/must_must_not_trace_exec/unsafe_exec/
- move unsafe_exec() call above security_bprm_apply_creds() call rather than
  in call for readability.
- fix dummy hook to honor the case where root is ptracing
- couple minor formatting/spelling fixes

diff --git a/fs/exec.c b/fs/exec.c
index 47285fe301ff33fa4c0dcc732893da0e86fda5be..f73d2c4cc1e602e7fced42659d2af67ad23e896e 100644 (file)
--- a/fs/exec.c
+++ b/fs/exec.c
@@ -919,24 +919,30 @@ int prepare_binprm(struct linux_binprm *bprm)
 
 EXPORT_SYMBOL(prepare_binprm);
 
-/*
- * This function is used to produce the new IDs and capabilities
- * from the old ones and the file's capabilities.
- *
- * The formula used for evolving capabilities is:
- *
- *       pI' = pI
- * (***) pP' = (fP & X) | (fI & pI)
- *       pE' = pP' & fE          [NB. fE is 0 or ~0]
- *
- * I=Inheritable, P=Permitted, E=Effective // p=process, f=file
- * ' indicates post-exec(), and X is the global 'cap_bset'.
- *
- */
+static inline int unsafe_exec(struct task_struct *p)
+{
+       int unsafe = 0;
+       if (p->ptrace & PT_PTRACED) {
+               if (p->ptrace & PT_PTRACE_CAP)
+                       unsafe |= LSM_UNSAFE_PTRACE_CAP;
+               else
+                       unsafe |= LSM_UNSAFE_PTRACE;
+       }
+       if (atomic_read(&p->fs->count) > 1 ||
+           atomic_read(&p->files->count) > 1 ||
+           atomic_read(&p->sighand->count) > 1)
+               unsafe |= LSM_UNSAFE_SHARE;
+
+       return unsafe;
+}
 
 void compute_creds(struct linux_binprm *bprm)
 {
-       security_bprm_apply_creds(bprm);
+       int unsafe;
+       task_lock(current);
+       unsafe = unsafe_exec(current);
+       security_bprm_apply_creds(bprm, unsafe);
+       task_unlock(current);
 }
 
 EXPORT_SYMBOL(compute_creds);

diff --git a/include/linux/security.h b/include/linux/security.h
index 2d16f6577669fad7026d2d45d827df540b5d9fc9..5bc1ac328495e9935300c8573f44665ca82ffc19 100644 (file)
--- a/include/linux/security.h
+++ b/include/linux/security.h
@@ -44,7 +44,7 @@ extern int cap_capget (struct task_struct *target, kernel_cap_t *effective, kern
 extern int cap_capset_check (struct task_struct *target, kernel_cap_t *effective, kernel_cap_t *inheritable, kernel_cap_t *permitted);
 extern void cap_capset_set (struct task_struct *target, kernel_cap_t *effective, kernel_cap_t *inheritable, kernel_cap_t *permitted);
 extern int cap_bprm_set_security (struct linux_binprm *bprm);
-extern void cap_bprm_apply_creds (struct linux_binprm *bprm);
+extern void cap_bprm_apply_creds (struct linux_binprm *bprm, int unsafe);
 extern int cap_bprm_secureexec(struct linux_binprm *bprm);
 extern int cap_inode_setxattr(struct dentry *dentry, char *name, void *value, size_t size, int flags);
 extern int cap_inode_removexattr(struct dentry *dentry, char *name);
@@ -86,6 +86,11 @@ struct nfsctl_arg;
 struct sched_param;
 struct swap_info_struct;
 
+/* bprm_apply_creds unsafe reasons */
+#define LSM_UNSAFE_SHARE       1
+#define LSM_UNSAFE_PTRACE      2
+#define LSM_UNSAFE_PTRACE_CAP  4
+
 #ifdef CONFIG_SECURITY
 
 /**
@@ -112,6 +117,8 @@ struct swap_info_struct;
  *     also perform other state changes on the process (e.g.  closing open
  *     file descriptors to which access is no longer granted if the attributes
  *     were changed). 
+ *     bprm_apply_creds is called under task_lock.  @unsafe indicates various
+ *     reasons why it may be unsafe to change security state.
  *     @bprm contains the linux_binprm structure.
  * @bprm_set_security:
  *     Save security information in the bprm->security field, typically based
@@ -1026,7 +1033,7 @@ struct security_operations {
 
        int (*bprm_alloc_security) (struct linux_binprm * bprm);
        void (*bprm_free_security) (struct linux_binprm * bprm);
-       void (*bprm_apply_creds) (struct linux_binprm * bprm);
+       void (*bprm_apply_creds) (struct linux_binprm * bprm, int unsafe);
        int (*bprm_set_security) (struct linux_binprm * bprm);
        int (*bprm_check_security) (struct linux_binprm * bprm);
        int (*bprm_secureexec) (struct linux_binprm * bprm);
@@ -1290,9 +1297,9 @@ static inline void security_bprm_free (struct linux_binprm *bprm)
 {
        security_ops->bprm_free_security (bprm);
 }
-static inline void security_bprm_apply_creds (struct linux_binprm *bprm)
+static inline void security_bprm_apply_creds (struct linux_binprm *bprm, int unsafe)
 {
-       security_ops->bprm_apply_creds (bprm);
+       security_ops->bprm_apply_creds (bprm, unsafe);
 }
 static inline int security_bprm_set (struct linux_binprm *bprm)
 {
@@ -1962,9 +1969,9 @@ static inline int security_bprm_alloc (struct linux_binprm *bprm)
 static inline void security_bprm_free (struct linux_binprm *bprm)
 { }
 
-static inline void security_bprm_apply_creds (struct linux_binprm *bprm)
+static inline void security_bprm_apply_creds (struct linux_binprm *bprm, int unsafe)
 { 
-       cap_bprm_apply_creds (bprm);
+       cap_bprm_apply_creds (bprm, unsafe);
 }
 
 static inline int security_bprm_set (struct linux_binprm *bprm)

diff --git a/security/commoncap.c b/security/commoncap.c
index 07265810c353631f4d9a891cde742407d3cf87b9..f40fc73705d09bdae590e83d22b9ac5041da528d 100644 (file)
--- a/security/commoncap.c
+++ b/security/commoncap.c
@@ -115,15 +115,7 @@ int cap_bprm_set_security (struct linux_binprm *bprm)
        return 0;
 }
 
-static inline int must_not_trace_exec (struct task_struct *p)
-{
-       return ((p->ptrace & PT_PTRACED) && !(p->ptrace & PT_PTRACE_CAP))
-               || atomic_read(&p->fs->count) > 1
-               || atomic_read(&p->files->count) > 1
-               || atomic_read(&p->sighand->count) > 1;
-}
-
-void cap_bprm_apply_creds (struct linux_binprm *bprm)
+void cap_bprm_apply_creds (struct linux_binprm *bprm, int unsafe)
 {
        /* Derived from fs/exec.c:compute_creds. */
        kernel_cap_t new_permitted, working;
@@ -133,30 +125,25 @@ void cap_bprm_apply_creds (struct linux_binprm *bprm)
                                 current->cap_inheritable);
        new_permitted = cap_combine (new_permitted, working);
 
-       task_lock(current);
-
-       if (bprm->e_uid != current->uid || bprm->e_gid != current->gid) {
+       if (bprm->e_uid != current->uid || bprm->e_gid != current->gid ||
+           !cap_issubset (new_permitted, current->cap_permitted)) {
                current->mm->dumpable = 0;
 
-               if (must_not_trace_exec(current) && !capable(CAP_SETUID)) {
-                       bprm->e_uid = current->uid;
-                       bprm->e_gid = current->gid;
+               if (unsafe & ~LSM_UNSAFE_PTRACE_CAP) {
+                       if (!capable(CAP_SETUID)) {
+                               bprm->e_uid = current->uid;
+                               bprm->e_gid = current->gid;
+                       }
+                       if (!capable (CAP_SETPCAP)) {
+                               new_permitted = cap_intersect (new_permitted,
+                                                       current->cap_permitted);
+                       }
                }
        }
 
        current->suid = current->euid = current->fsuid = bprm->e_uid;
        current->sgid = current->egid = current->fsgid = bprm->e_gid;
 
-       if (!cap_issubset (new_permitted, current->cap_permitted)) {
-               current->mm->dumpable = 0;
-
-               if (must_not_trace_exec (current) && !capable (CAP_SETPCAP)) {
-                       new_permitted = cap_intersect (new_permitted,
-                                                      current->
-                                                      cap_permitted);
-               }
-       }
-
        /* For init, we want to retain the capabilities set
         * in the init_task struct. Thus we skip the usual
         * capability rules */
@@ -167,7 +154,6 @@ void cap_bprm_apply_creds (struct linux_binprm *bprm)
        }
 
        /* AUD: Audit candidate if current->cap_effective is set */
-       task_unlock(current);
 
        current->keep_capabilities = 0;
 }

diff --git a/security/dummy.c b/security/dummy.c
index c34991da5d6e5964bb5acce53eb6d1b1252302b0..4e12451e8a744fee4578d0c76f4b46d6a578e0ab 100644 (file)
--- a/security/dummy.c
+++ b/security/dummy.c
@@ -171,21 +171,12 @@ static void dummy_bprm_free_security (struct linux_binprm *bprm)
        return;
 }
 
-static inline int must_not_trace_exec (struct task_struct *p)
+static void dummy_bprm_apply_creds (struct linux_binprm *bprm, int unsafe)
 {
-       return ((p->ptrace & PT_PTRACED) && !(p->ptrace & PT_PTRACE_CAP))
-               || atomic_read(&p->fs->count) > 1
-               || atomic_read(&p->files->count) > 1
-               || atomic_read(&p->sighand->count) > 1;
-}
-
-static void dummy_bprm_apply_creds (struct linux_binprm *bprm)
-{
-       task_lock(current);
        if (bprm->e_uid != current->uid || bprm->e_gid != current->gid) {
                current->mm->dumpable = 0;
 
-               if (must_not_trace_exec(current) && !capable(CAP_SETUID)) {
+               if ((unsafe & ~LSM_UNSAFE_PTRACE_CAP) && !capable(CAP_SETUID)) {
                        bprm->e_uid = current->uid;
                        bprm->e_gid = current->gid;
                }
@@ -193,8 +184,6 @@ static void dummy_bprm_apply_creds (struct linux_binprm *bprm)
 
        current->suid = current->euid = current->fsuid = bprm->e_uid;
        current->sgid = current->egid = current->fsgid = bprm->e_gid;
-
-       task_unlock(current);
 }
 
 static int dummy_bprm_set_security (struct linux_binprm *bprm)

diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c
index 2273c5f851b4c4797306a583cd44597de023400f..f5228889e166221206ae2bf3036e36c7d563da77 100644 (file)
--- a/security/selinux/hooks.c
+++ b/security/selinux/hooks.c
@@ -1745,7 +1745,7 @@ static inline void flush_unauthorized_files(struct files_struct * files)
        spin_unlock(&files->file_lock);
 }
 
-static void selinux_bprm_apply_creds(struct linux_binprm *bprm)
+static void selinux_bprm_apply_creds(struct linux_binprm *bprm, int unsafe)
 {
        struct task_security_struct *tsec, *psec;
        struct bprm_security_struct *bsec;
@@ -1755,7 +1755,7 @@ static void selinux_bprm_apply_creds(struct linux_binprm *bprm)
        struct rlimit *rlim, *initrlim;
        int rc, i;
 
-       secondary_ops->bprm_apply_creds(bprm);
+       secondary_ops->bprm_apply_creds(bprm, unsafe);
 
        tsec = current->security;
 
@@ -1766,22 +1766,22 @@ static void selinux_bprm_apply_creds(struct linux_binprm *bprm)
        if (tsec->sid != sid) {
                /* Check for shared state.  If not ok, leave SID
                   unchanged and kill. */
-               if ((atomic_read(&current->fs->count) > 1 ||
-                    atomic_read(&current->files->count) > 1 ||
-                    atomic_read(&current->sighand->count) > 1)) {
-                       rc = avc_has_perm(tsec->sid, sid,
+               if (unsafe & LSM_UNSAFE_SHARE) {
+                       rc = avc_has_perm_noaudit(tsec->sid, sid,
                                          SECCLASS_PROCESS, PROCESS__SHARE,
-                                         NULL, NULL);
+                                         NULL, &avd);
                        if (rc) {
+                               task_unlock(current);
+                               avc_audit(tsec->sid, sid, SECCLASS_PROCESS,
+                                   PROCESS__SHARE, &avd, rc, NULL);
                                force_sig_specific(SIGKILL, current);
-                               return;
+                               goto lock_out;
                        }
                }
 
                /* Check for ptracing, and update the task SID if ok.
                   Otherwise, leave SID unchanged and kill. */
-               task_lock(current);
-               if (current->ptrace & PT_PTRACED) {
+               if (unsafe & (LSM_UNSAFE_PTRACE | LSM_UNSAFE_PTRACE_CAP)) {
                        psec = current->parent->security;
                        rc = avc_has_perm_noaudit(psec->sid, sid,
                                          SECCLASS_PROCESS, PROCESS__PTRACE,
@@ -1793,7 +1793,7 @@ static void selinux_bprm_apply_creds(struct linux_binprm *bprm)
                                  PROCESS__PTRACE, &avd, rc, NULL);
                        if (rc) {
                                force_sig_specific(SIGKILL, current);
-                               return;
+                               goto lock_out;
                        }
                } else {
                        tsec->sid = sid;
@@ -1846,6 +1846,10 @@ static void selinux_bprm_apply_creds(struct linux_binprm *bprm)
                /* Wake up the parent if it is waiting so that it can
                   recheck wait permission to the new task SID. */
                wake_up_interruptible(&current->parent->wait_chldexit);
+
+lock_out:
+               task_lock(current);
+               return;
        }
 }