commoncap: Refactor to remove bprm_secureexec hook

The commoncap implementation of the bprm_secureexec hook is the only LSM
that depends on the final call to its bprm_set_creds hook (since it may
be called for multiple files, it ignores bprm->called_set_creds). As a
result, it cannot safely _clear_ bprm->secureexec since other LSMs may
have set it.  Instead, remove the bprm_secureexec hook by introducing a
new flag to bprm specific to commoncap: cap_elevated. This is similar to
cap_effective, but that is used for a specific subset of elevated
privileges, and exists solely to track state from bprm_set_creds to
bprm_secureexec. As such, it will be removed in the next patch.

Here, set the new bprm->cap_elevated flag when setuid/setgid has happened
from bprm_fill_uid() or fscapabilities have been prepared. This temporarily
moves the bprm_secureexec hook to a static inline. The helper will be
removed in the next patch; this makes the step easier to review and bisect,
since this does not introduce any changes to inputs nor outputs to the
"elevated privileges" calculation.

The new flag is merged with the bprm->secureexec flag in setup_new_exec()
since this marks the end of any further prepare_binprm() calls.

Cc: Andy Lutomirski <luto@kernel.org>
Signed-off-by: Kees Cook <keescook@chromium.org>
Reviewed-by: Andy Lutomirski <luto@kernel.org>
Acked-by: James Morris <james.l.morris@oracle.com>
Acked-by: Serge Hallyn <serge@hallyn.com>

diff --git a/fs/exec.c b/fs/exec.c
index 0f361115c88f52b9b4cfa7bc497435701c8235ba..1536bc4502ccffbd4e4f15bfdea68daf1d623d2c 100644 (file)
--- a/fs/exec.c
+++ b/fs/exec.c
@@ -1345,6 +1345,13 @@ void setup_new_exec(struct linux_binprm * bprm)
 {
        bprm->secureexec |= security_bprm_secureexec(bprm);
 
+       /*
+        * Once here, prepare_binrpm() will not be called any more, so
+        * the final state of setuid/setgid/fscaps can be merged into the
+        * secureexec flag.
+        */
+       bprm->secureexec |= bprm->cap_elevated;
+
        arch_pick_mmap_layout(current->mm);
 
        current->sas_ss_sp = current->sas_ss_size = 0;

diff --git a/include/linux/binfmts.h b/include/linux/binfmts.h
index 16838ba7ee752bd18236b900c81f679c227de330..213c61fa37804ca0964345ff47eae709abe518f7 100644 (file)
--- a/include/linux/binfmts.h
+++ b/include/linux/binfmts.h
@@ -34,6 +34,13 @@ struct linux_binprm {
                cap_effective:1,/* true if has elevated effective capabilities,
                                 * false if not; except for init which inherits
                                 * its parent's caps anyway */
+               /*
+                * True if most recent call to the commoncaps bprm_set_creds
+                * hook (due to multiple prepare_binprm() calls from the
+                * binfmt_script/misc handlers) resulted in elevated
+                * privileges.
+                */
+               cap_elevated:1,
                /*
                 * Set by bprm_set_creds hook to indicate a privilege-gaining
                 * exec has happened. Used to sanitize execution environment

diff --git a/include/linux/security.h b/include/linux/security.h
index b6ea1dc9cc9d2b9e4f1342d6268da1bdb9301be8..f89832ccdf552d0cd30988cddbfd90b58c3eab46 100644 (file)
--- a/include/linux/security.h
+++ b/include/linux/security.h
@@ -85,7 +85,6 @@ extern int cap_capset(struct cred *new, const struct cred *old,
                      const kernel_cap_t *inheritable,
                      const kernel_cap_t *permitted);
 extern int cap_bprm_set_creds(struct linux_binprm *bprm);
-extern int cap_bprm_secureexec(struct linux_binprm *bprm);
 extern int cap_inode_setxattr(struct dentry *dentry, const char *name,
                              const void *value, size_t size, int flags);
 extern int cap_inode_removexattr(struct dentry *dentry, const char *name);
@@ -543,7 +542,7 @@ static inline void security_bprm_committed_creds(struct linux_binprm *bprm)
 
 static inline int security_bprm_secureexec(struct linux_binprm *bprm)
 {
-       return cap_bprm_secureexec(bprm);
+       return 0;
 }
 
 static inline int security_sb_alloc(struct super_block *sb)

diff --git a/security/commoncap.c b/security/commoncap.c
index 7abebd782d5e02d0c024ca0b2b1b189b1f4f83d1..abb6050c8083af06d6b773e33ab50e3a8e3838ee 100644 (file)
--- a/security/commoncap.c
+++ b/security/commoncap.c
@@ -481,6 +481,8 @@ out:
        return rc;
 }
 
+static int is_secureexec(struct linux_binprm *bprm);
+
 /**
  * cap_bprm_set_creds - Set up the proposed credentials for execve().
  * @bprm: The execution parameters, including the proposed creds
@@ -614,11 +616,14 @@ skip:
        if (WARN_ON(!cap_ambient_invariant_ok(new)))
                return -EPERM;
 
+       /* Check for privilege-elevated exec. */
+       bprm->cap_elevated = is_secureexec(bprm);
+
        return 0;
 }
 
 /**
- * cap_bprm_secureexec - Determine whether a secure execution is required
+ * is_secureexec - Determine whether a secure execution is required
  * @bprm: The execution parameters
  *
  * Determine whether a secure execution is required, return 1 if it is, and 0
@@ -627,9 +632,9 @@ skip:
  * The credentials have been committed by this point, and so are no longer
  * available through @bprm->cred.
  */
-int cap_bprm_secureexec(struct linux_binprm *bprm)
+static int is_secureexec(struct linux_binprm *bprm)
 {
-       const struct cred *cred = current_cred();
+       const struct cred *cred = bprm->cred;
        kuid_t root_uid = make_kuid(cred->user_ns, 0);
 
        if (!uid_eq(cred->uid, root_uid)) {
@@ -1079,7 +1084,6 @@ struct security_hook_list capability_hooks[] __lsm_ro_after_init = {
        LSM_HOOK_INIT(capget, cap_capget),
        LSM_HOOK_INIT(capset, cap_capset),
        LSM_HOOK_INIT(bprm_set_creds, cap_bprm_set_creds),
-       LSM_HOOK_INIT(bprm_secureexec, cap_bprm_secureexec),
        LSM_HOOK_INIT(inode_need_killpriv, cap_inode_need_killpriv),
        LSM_HOOK_INIT(inode_killpriv, cap_inode_killpriv),
        LSM_HOOK_INIT(mmap_addr, cap_mmap_addr),