This series has grown... :P
As discussed with Linus and Andy, we need to reset the stack rlimit
before we do memory layouts when execing a privilege-gaining (e.g.
setuid) program. To do this, we need to know the results of the
bprm_secureexec hook before memory layouts. As it turns out, this
can be made _mostly_ trivial by collapsing bprm_secureexec into
bprm_set_creds.
The LSMs using bprm_secureexec nearly always save state between
bprm_set_creds and bprm_secureexec. In the face of multiple calls to
bprm_set_creds (via prepare_binprm() calls from binfmt_script, etc),
all LSMs except commoncap only pay attention to the first call, so
that aligns well with collapsing bprm_secureexec into bprm_set_creds.
The commoncaps, though, needs to check the _last_ bprm_set_creds, so
this series just swaps one bprm flag for another (cap_effective is no
longer needed to save state between bprm_set_creds and bprm_secureexec,
but we do need to keep a separate state, so we add the cap_elevated flag).
Once secureexec is available to setup_new_exec() before the memory
layout, we can add an rlimit sanity-check for setuid execs. (With no
need to clean up since we're past the point of no return.)
Along the way, this fixes comments, renames a variable, and consolidates
dumpability and pdeath_signal clearing, which includes some commit log
archeology to examine the subtle differences between what we had and
what we need.
I'd appreciate some extra eyes on this to make sure this isn't broken
in some special way. Looking at the diffstat, even after all my long
comments, this is a net reduction in lines. :)
Given this crosses a bunch of areas, I think this is likely best to
go via the -mm tree, which is where nearly all of my prior exec work
has lived too.
Thanks!
-Kees
----------------------------------------------------------------
Kees Cook (15):
binfmt: Introduce secureexec flag
exec: Rename bprm->cred_prepared to called_set_creds
apparmor: Refactor to remove bprm_secureexec hook
selinux: Refactor to remove bprm_secureexec hook
smack: Refactor to remove bprm_secureexec hook
commoncap: Refactor to remove bprm_secureexec hook
commoncap: Move cap_elevated calculation into bprm_set_creds
LSM: drop bprm_secureexec hook
exec: Correct comments about "point of no return"
exec: Use secureexec for setting dumpability
exec: Use secureexec for clearing pdeath_signal
smack: Remove redundant pdeath_signal clearing
exec: Consolidate dumpability logic
exec: Use sane stack rlimit under secureexec
exec: Consolidate pdeath_signal clearing
fs/binfmt_elf.c | 2 +-
fs/binfmt_elf_fdpic.c | 2 +-
fs/binfmt_flat.c | 2 +-
fs/exec.c | 56 ++++++++++++++++++++++++++++----------
include/linux/binfmts.h | 24 ++++++++++++----
include/linux/lsm_hooks.h | 14 ++++------
include/linux/security.h | 7 -----
security/apparmor/domain.c | 24 ++--------------
security/apparmor/include/domain.h | 1 -
security/apparmor/include/file.h | 3 --
security/apparmor/lsm.c | 1 -
security/commoncap.c | 50 ++++++++--------------------------
security/security.c | 5 ----
security/selinux/hooks.c | 26 ++++--------------
security/smack/smack_lsm.c | 34 ++---------------------
security/tomoyo/tomoyo.c | 2 +-
16 files changed, 91 insertions(+), 162 deletions(-)
v3:
- collapse brpm_secureexec into bprm_set_creds; ebiederm.
- continue to improve various comments
v2:
- fix missed current_security() uses in LSMs.
- research/consolidate dumpability setting logic
- research/consolidate pdeath_signal clearing logic
- split up logical steps a little more for easier review (and bisection)
- fix some old broken comments
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: Serge Hallyn <[email protected]>
Cc: Andy Lutomirski <[email protected]>
Signed-off-by: Kees Cook <[email protected]>
---
fs/exec.c | 7 +++++++
include/linux/binfmts.h | 7 +++++++
security/commoncap.c | 12 ++++++++----
3 files changed, 22 insertions(+), 4 deletions(-)
diff --git a/fs/exec.c b/fs/exec.c
index 925c85a45d97..53ffa739fb58 100644
--- a/fs/exec.c
+++ b/fs/exec.c
@@ -1330,6 +1330,13 @@ EXPORT_SYMBOL(would_dump);
void setup_new_exec(struct linux_binprm * 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);
/* This is the point of no return */
diff --git a/include/linux/binfmts.h b/include/linux/binfmts.h
index 36be5a67517a..a82f5edf23f9 100644
--- a/include/linux/binfmts.h
+++ b/include/linux/binfmts.h
@@ -35,6 +35,13 @@ struct linux_binprm {
* 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
* and to set AT_SECURE auxv for glibc.
diff --git a/security/commoncap.c b/security/commoncap.c
index 7abebd782d5e..abb6050c8083 100644
--- a/security/commoncap.c
+++ b/security/commoncap.c
@@ -481,6 +481,8 @@ static int get_file_caps(struct linux_binprm *bprm, bool *effective, bool *has_c
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 @@ int cap_bprm_set_creds(struct linux_binprm *bprm)
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 @@ int cap_bprm_set_creds(struct linux_binprm *bprm)
* 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),
--
2.7.4
Instead of a separate function, open-code the cap_elevated test, which
lets us entirely remove bprm->cap_effective (to use the local "effective"
variable instead), and more accurately examine euid/egid changes via the
existing local "is_setid".
Cc: Serge Hallyn <[email protected]>
Cc: Andy Lutomirski <[email protected]>
Signed-off-by: Kees Cook <[email protected]>
---
include/linux/binfmts.h | 3 ---
security/commoncap.c | 52 ++++++++++---------------------------------------
2 files changed, 10 insertions(+), 45 deletions(-)
diff --git a/include/linux/binfmts.h b/include/linux/binfmts.h
index a82f5edf23f9..b2c1563091e5 100644
--- a/include/linux/binfmts.h
+++ b/include/linux/binfmts.h
@@ -31,9 +31,6 @@ struct linux_binprm {
* binfmt_script/misc).
*/
called_set_creds:1,
- 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
diff --git a/security/commoncap.c b/security/commoncap.c
index abb6050c8083..d8e26fb9781d 100644
--- a/security/commoncap.c
+++ b/security/commoncap.c
@@ -285,15 +285,6 @@ int cap_capset(struct cred *new,
return 0;
}
-/*
- * Clear proposed capability sets for execve().
- */
-static inline void bprm_clear_caps(struct linux_binprm *bprm)
-{
- cap_clear(bprm->cred->cap_permitted);
- bprm->cap_effective = false;
-}
-
/**
* cap_inode_need_killpriv - Determine if inode change affects privileges
* @dentry: The inode/dentry in being changed with change marked ATTR_KILL_PRIV
@@ -443,7 +434,7 @@ static int get_file_caps(struct linux_binprm *bprm, bool *effective, bool *has_c
int rc = 0;
struct cpu_vfs_cap_data vcaps;
- bprm_clear_caps(bprm);
+ cap_clear(bprm->cred->cap_permitted);
if (!file_caps_enabled)
return 0;
@@ -476,13 +467,11 @@ static int get_file_caps(struct linux_binprm *bprm, bool *effective, bool *has_c
out:
if (rc)
- bprm_clear_caps(bprm);
+ cap_clear(bprm->cred->cap_permitted);
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
@@ -587,8 +576,6 @@ int cap_bprm_set_creds(struct linux_binprm *bprm)
if (WARN_ON(!cap_ambient_invariant_ok(new)))
return -EPERM;
- bprm->cap_effective = effective;
-
/*
* Audit candidate if current->cap_effective is set
*
@@ -617,35 +604,16 @@ int cap_bprm_set_creds(struct linux_binprm *bprm)
return -EPERM;
/* Check for privilege-elevated exec. */
- bprm->cap_elevated = is_secureexec(bprm);
-
- return 0;
-}
-
-/**
- * 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
- * if it is not.
- *
- * The credentials have been committed by this point, and so are no longer
- * available through @bprm->cred.
- */
-static int is_secureexec(struct linux_binprm *bprm)
-{
- const struct cred *cred = bprm->cred;
- kuid_t root_uid = make_kuid(cred->user_ns, 0);
-
- if (!uid_eq(cred->uid, root_uid)) {
- if (bprm->cap_effective)
- return 1;
- if (!cap_issubset(cred->cap_permitted, cred->cap_ambient))
- return 1;
+ bprm->cap_elevated = 0;
+ if (is_setid) {
+ bprm->cap_elevated = 1;
+ } else if (!uid_eq(new->uid, root_uid)) {
+ if (effective ||
+ !cap_issubset(new->cap_permitted, new->cap_ambient))
+ bprm->cap_elevated = 1;
}
- return (!uid_eq(cred->euid, cred->uid) ||
- !gid_eq(cred->egid, cred->gid));
+ return 0;
}
/**
--
2.7.4
Since it's already valid to set dumpability in the early part of
setup_new_exec(), we can consolidate the logic into a single place.
The BINPRM_FLAGS_ENFORCE_NONDUMP is set during would_dump() calls
before setup_new_exec(), so its test is safe to move as well.
Signed-off-by: Kees Cook <[email protected]>
---
fs/exec.c | 11 +++++------
1 file changed, 5 insertions(+), 6 deletions(-)
diff --git a/fs/exec.c b/fs/exec.c
index 81793a193d92..45418d6ec583 100644
--- a/fs/exec.c
+++ b/fs/exec.c
@@ -1353,10 +1353,12 @@ void setup_new_exec(struct linux_binprm * bprm)
current->sas_ss_sp = current->sas_ss_size = 0;
- if (!bprm->secureexec)
- set_dumpable(current->mm, SUID_DUMP_USER);
- else
+ /* Figure out dumpability. */
+ if (bprm->interp_flags & BINPRM_FLAGS_ENFORCE_NONDUMP ||
+ bprm->secureexec)
set_dumpable(current->mm, suid_dumpable);
+ else
+ set_dumpable(current->mm, SUID_DUMP_USER);
arch_setup_new_exec();
perf_event_exec();
@@ -1370,9 +1372,6 @@ void setup_new_exec(struct linux_binprm * bprm)
if (bprm->secureexec) {
current->pdeath_signal = 0;
- } else {
- if (bprm->interp_flags & BINPRM_FLAGS_ENFORCE_NONDUMP)
- set_dumpable(current->mm, suid_dumpable);
}
/* An exec changes our domain. We are no longer part of the thread
--
2.7.4
Instead of an additional secureexec check for pdeath_signal, just move it
up into the initial secureexec test. Neither perf nor arch code touches
pdeath_signal, so the relocation shouldn't change anything.
Signed-off-by: Kees Cook <[email protected]>
---
fs/exec.c | 7 +++----
1 file changed, 3 insertions(+), 4 deletions(-)
diff --git a/fs/exec.c b/fs/exec.c
index 4227c1e56566..9525ac15b897 100644
--- a/fs/exec.c
+++ b/fs/exec.c
@@ -1350,6 +1350,9 @@ void setup_new_exec(struct linux_binprm * bprm)
bprm->secureexec |= bprm->cap_elevated;
if (bprm->secureexec) {
+ /* Make sure parent cannot signal privileged process. */
+ current->pdeath_signal = 0;
+
/*
* For secureexec, reset the stack limit to sane default to
* avoid bad behavior from the prior rlimits. This has to
@@ -1382,10 +1385,6 @@ void setup_new_exec(struct linux_binprm * bprm)
*/
current->mm->task_size = TASK_SIZE;
- if (bprm->secureexec) {
- current->pdeath_signal = 0;
- }
-
/* An exec changes our domain. We are no longer part of the thread
group */
current->self_exec_id++;
--
2.7.4
For a secureexec, before memory layout selection has happened, reset the
stack rlimit to something sane to avoid the caller having control over
the resulting layouts.
$ ulimit -s
8192
$ ulimit -s unlimited
$ /bin/sh -c 'ulimit -s'
unlimited
$ sudo /bin/sh -c 'ulimit -s'
8192
Cc: Linus Torvalds <[email protected]>
Signed-off-by: Kees Cook <[email protected]>
---
fs/exec.c | 12 ++++++++++++
1 file changed, 12 insertions(+)
diff --git a/fs/exec.c b/fs/exec.c
index 45418d6ec583..4227c1e56566 100644
--- a/fs/exec.c
+++ b/fs/exec.c
@@ -1349,6 +1349,18 @@ void setup_new_exec(struct linux_binprm * bprm)
*/
bprm->secureexec |= bprm->cap_elevated;
+ if (bprm->secureexec) {
+ /*
+ * For secureexec, reset the stack limit to sane default to
+ * avoid bad behavior from the prior rlimits. This has to
+ * happen before arch_pick_mmap_layout(), which examines
+ * RLIMIT_STACK, but after the point of no return to avoid
+ * needing to clean up the change on failure.
+ */
+ if (current->signal->rlim[RLIMIT_STACK].rlim_cur > _STK_LIM)
+ current->signal->rlim[RLIMIT_STACK].rlim_cur = _STK_LIM;
+ }
+
arch_pick_mmap_layout(current->mm);
current->sas_ss_sp = current->sas_ss_size = 0;
--
2.7.4
In commit 221af7f87b97 ("Split 'flush_old_exec' into two functions"),
the comment about the point of no return should have stayed in
flush_old_exec() since it refers to "bprm->mm = NULL;" line, but prior
changes in commits c89681ed7d0e ("remove steal_locks()"), and
fd8328be874f ("sanitize handling of shared descriptor tables in failing
execve()") made it look like it meant the current->sas_ss_sp line instead.
The comment was referring to the fact that once bprm->mm is NULL, all
failures from a binfmt load_binary hook (e.g. load_elf_binary), will
get SEGV raised against current. Move this comment and expand the
explanation a bit, putting it above the assignment this time, and add
details about the true nature of "point of no return" being the call
to flush_old_exec() itself.
This also removes an erroneous commet about when credentials are being
installed. That has its own dedicated function, install_exec_creds(),
which carries a similar (and correct) comment, so remove the bogus comment
where installation is not actually happening.
Cc: David Howells <[email protected]>
Cc: Eric W. Biederman <[email protected]>
Signed-off-by: Kees Cook <[email protected]>
---
fs/exec.c | 16 +++++++++++++---
1 file changed, 13 insertions(+), 3 deletions(-)
diff --git a/fs/exec.c b/fs/exec.c
index 53ffa739fb58..f9480d3e0b82 100644
--- a/fs/exec.c
+++ b/fs/exec.c
@@ -1258,6 +1258,12 @@ void __set_task_comm(struct task_struct *tsk, const char *buf, bool exec)
perf_event_comm(tsk, exec);
}
+/*
+ * Calling this is the point of no return. None of the failures will be
+ * seen by userspace since either the process is already taking a fatal
+ * signal (via de_thread() or coredump), or will have SEGV raised
+ * (after exec_mmap()) by search_binary_handlers (see below).
+ */
int flush_old_exec(struct linux_binprm * bprm)
{
int retval;
@@ -1285,7 +1291,13 @@ int flush_old_exec(struct linux_binprm * bprm)
if (retval)
goto out;
- bprm->mm = NULL; /* We're using it now */
+ /*
+ * After clearing bprm->mm (to mark that current is using the
+ * prepared mm now), we have nothing left of the original
+ * process. If anything from here on returns an error, the check
+ * in search_binary_handler() will SEGV current.
+ */
+ bprm->mm = NULL;
set_fs(USER_DS);
current->flags &= ~(PF_RANDOMIZE | PF_FORKNOEXEC | PF_KTHREAD |
@@ -1339,7 +1351,6 @@ void setup_new_exec(struct linux_binprm * bprm)
arch_pick_mmap_layout(current->mm);
- /* This is the point of no return */
current->sas_ss_sp = current->sas_ss_size = 0;
if (uid_eq(current_euid(), current_uid()) && gid_eq(current_egid(), current_gid()))
@@ -1357,7 +1368,6 @@ void setup_new_exec(struct linux_binprm * bprm)
*/
current->mm->task_size = TASK_SIZE;
- /* install the new credentials */
if (!uid_eq(bprm->cred->uid, current_euid()) ||
!gid_eq(bprm->cred->gid, current_egid())) {
current->pdeath_signal = 0;
--
2.7.4
This removes the bprm_secureexec hook since the logic has been folded into
the bprm_set_creds hook for all LSMs now.
Cc: James Morris <[email protected]>
Cc: Eric W. Biederman <[email protected]>
Signed-off-by: Kees Cook <[email protected]>
---
fs/binfmt_elf.c | 1 -
fs/binfmt_elf_fdpic.c | 1 -
include/linux/lsm_hooks.h | 14 +++++---------
include/linux/security.h | 7 -------
security/security.c | 5 -----
5 files changed, 5 insertions(+), 23 deletions(-)
diff --git a/fs/binfmt_elf.c b/fs/binfmt_elf.c
index 991e4de3515f..7f6ec4dac13d 100644
--- a/fs/binfmt_elf.c
+++ b/fs/binfmt_elf.c
@@ -254,7 +254,6 @@ create_elf_tables(struct linux_binprm *bprm, struct elfhdr *exec,
NEW_AUX_ENT(AT_EUID, from_kuid_munged(cred->user_ns, cred->euid));
NEW_AUX_ENT(AT_GID, from_kgid_munged(cred->user_ns, cred->gid));
NEW_AUX_ENT(AT_EGID, from_kgid_munged(cred->user_ns, cred->egid));
- bprm->secureexec |= security_bprm_secureexec(bprm);
NEW_AUX_ENT(AT_SECURE, bprm->secureexec);
NEW_AUX_ENT(AT_RANDOM, (elf_addr_t)(unsigned long)u_rand_bytes);
#ifdef ELF_HWCAP2
diff --git a/fs/binfmt_elf_fdpic.c b/fs/binfmt_elf_fdpic.c
index c88b35d4a6b3..5aa9199dfb13 100644
--- a/fs/binfmt_elf_fdpic.c
+++ b/fs/binfmt_elf_fdpic.c
@@ -650,7 +650,6 @@ static int create_elf_fdpic_tables(struct linux_binprm *bprm,
NEW_AUX_ENT(AT_EUID, (elf_addr_t) from_kuid_munged(cred->user_ns, cred->euid));
NEW_AUX_ENT(AT_GID, (elf_addr_t) from_kgid_munged(cred->user_ns, cred->gid));
NEW_AUX_ENT(AT_EGID, (elf_addr_t) from_kgid_munged(cred->user_ns, cred->egid));
- bprm->secureexec |= security_bprm_secureexec(bprm);
NEW_AUX_ENT(AT_SECURE, bprm->secureexec);
NEW_AUX_ENT(AT_EXECFN, bprm->exec);
diff --git a/include/linux/lsm_hooks.h b/include/linux/lsm_hooks.h
index 080f34e66017..2ddc1c7e8923 100644
--- a/include/linux/lsm_hooks.h
+++ b/include/linux/lsm_hooks.h
@@ -40,7 +40,11 @@
* interpreters. The hook can tell whether it has already been called by
* checking to see if @bprm->security is non-NULL. If so, then the hook
* may decide either to retain the security information saved earlier or
- * to replace it.
+ * to replace it. The hook must set @bprm->secureexec to 1 if a "secure
+ * exec" has happened as a result of this hook call. The flag is used to
+ * indicate the need for a sanitized execution environment, and is also
+ * passed in the ELF auxiliary table on the initial stack to indicate
+ * whether libc should enable secure mode.
* @bprm contains the linux_binprm structure.
* Return 0 if the hook is successful and permission is granted.
* @bprm_check_security:
@@ -68,12 +72,6 @@
* linux_binprm structure. This hook is a good place to perform state
* changes on the process such as clearing out non-inheritable signal
* state. This is called immediately after commit_creds().
- * @bprm_secureexec:
- * Return a boolean value (0 or 1) indicating whether a "secure exec"
- * is required. The flag is passed in the auxiliary table
- * on the initial stack to the ELF interpreter to indicate whether libc
- * should enable secure mode.
- * @bprm contains the linux_binprm structure.
*
* Security hooks for filesystem operations.
*
@@ -1368,7 +1366,6 @@ union security_list_options {
int (*bprm_set_creds)(struct linux_binprm *bprm);
int (*bprm_check_security)(struct linux_binprm *bprm);
- int (*bprm_secureexec)(struct linux_binprm *bprm);
void (*bprm_committing_creds)(struct linux_binprm *bprm);
void (*bprm_committed_creds)(struct linux_binprm *bprm);
@@ -1680,7 +1677,6 @@ struct security_hook_heads {
struct list_head vm_enough_memory;
struct list_head bprm_set_creds;
struct list_head bprm_check_security;
- struct list_head bprm_secureexec;
struct list_head bprm_committing_creds;
struct list_head bprm_committed_creds;
struct list_head sb_alloc_security;
diff --git a/include/linux/security.h b/include/linux/security.h
index af675b576645..133c41bb666d 100644
--- a/include/linux/security.h
+++ b/include/linux/security.h
@@ -80,7 +80,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);
@@ -223,7 +222,6 @@ int security_bprm_set_creds(struct linux_binprm *bprm);
int security_bprm_check(struct linux_binprm *bprm);
void security_bprm_committing_creds(struct linux_binprm *bprm);
void security_bprm_committed_creds(struct linux_binprm *bprm);
-int security_bprm_secureexec(struct linux_binprm *bprm);
int security_sb_alloc(struct super_block *sb);
void security_sb_free(struct super_block *sb);
int security_sb_copy_data(char *orig, char *copy);
@@ -515,11 +513,6 @@ 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);
-}
-
static inline int security_sb_alloc(struct super_block *sb)
{
return 0;
diff --git a/security/security.c b/security/security.c
index b9fea3999cf8..750b83186869 100644
--- a/security/security.c
+++ b/security/security.c
@@ -311,11 +311,6 @@ void security_bprm_committed_creds(struct linux_binprm *bprm)
call_void_hook(bprm_committed_creds, bprm);
}
-int security_bprm_secureexec(struct linux_binprm *bprm)
-{
- return call_int_hook(bprm_secureexec, 0, bprm);
-}
-
int security_sb_alloc(struct super_block *sb)
{
return call_int_hook(sb_alloc_security, 0, sb);
--
2.7.4
Like dumpability, clearing pdeath_signal happens both in setup_new_exec()
and later in commit_creds(). The test in setup_new_exec() is different
from all other privilege comparisons, though: it is checking the new cred
(bprm) uid vs the old cred (current) euid. This appears to be a bug,
introduced by commit a6f76f23d297 ("CRED: Make execve() take advantage of
copy-on-write credentials"):
- if (bprm->e_uid != current_euid() ||
- bprm->e_gid != current_egid()) {
- set_dumpable(current->mm, suid_dumpable);
+ /* install the new credentials */
+ if (bprm->cred->uid != current_euid() ||
+ bprm->cred->gid != current_egid()) {
It was bprm euid vs current euid (and egids), but the effective got
dropped. Nothing in the exec flow changes bprm->cred->uid (nor gid).
The call traces are:
prepare_bprm_creds()
prepare_exec_creds()
prepare_creds()
memcpy(new_creds, old_creds, ...)
security_prepare_creds() (unimplemented by commoncap)
...
prepare_binprm()
bprm_fill_uid()
resets euid/egid to current euid/egid
sets euid/egid on bprm based on set*id file bits
security_bprm_set_creds()
cap_bprm_set_creds()
handle all caps-based manipulations
so this test is effectively a test of current_uid() vs current_euid(),
which is wrong, just like the prior dumpability tests were wrong.
The commit log says "Clear pdeath_signal and set dumpable on
certain circumstances that may not be covered by commit_creds()." This
may be meaning the earlier old euid vs new euid (and egid) test that
got changed.
Luckily, as with dumpability, this is all masked by commit_creds()
which performs old/new euid and egid tests and clears pdeath_signal.
And again, like dumpability, we should include LSM secureexec logic for
pdeath_signal clearing. For example, Smack goes out of its way to clear
pdeath_signal when it finds a secureexec condition.
Cc: David Howells <[email protected]>
Signed-off-by: Kees Cook <[email protected]>
---
fs/exec.c | 3 +--
1 file changed, 1 insertion(+), 2 deletions(-)
diff --git a/fs/exec.c b/fs/exec.c
index 5241c8f25f5d..81793a193d92 100644
--- a/fs/exec.c
+++ b/fs/exec.c
@@ -1368,8 +1368,7 @@ void setup_new_exec(struct linux_binprm * bprm)
*/
current->mm->task_size = TASK_SIZE;
- if (!uid_eq(bprm->cred->uid, current_euid()) ||
- !gid_eq(bprm->cred->gid, current_egid())) {
+ if (bprm->secureexec) {
current->pdeath_signal = 0;
} else {
if (bprm->interp_flags & BINPRM_FLAGS_ENFORCE_NONDUMP)
--
2.7.4
This removes the redundant pdeath_signal clearing in Smack: the check in
smack_bprm_committing_creds() matches the check in smack_bprm_set_creds()
(which used to be in the now-removed smack_bprm_securexec() hook) and
since secureexec is now being checked for clearing pdeath_signal, this
is redundant to the common exec code.
Cc: Casey Schaufler <[email protected]>
Signed-off-by: Kees Cook <[email protected]>
---
security/smack/smack_lsm.c | 15 ---------------
1 file changed, 15 deletions(-)
diff --git a/security/smack/smack_lsm.c b/security/smack/smack_lsm.c
index 4f1967be3d20..4e33c650b224 100644
--- a/security/smack/smack_lsm.c
+++ b/security/smack/smack_lsm.c
@@ -957,20 +957,6 @@ static int smack_bprm_set_creds(struct linux_binprm *bprm)
return 0;
}
-/**
- * smack_bprm_committing_creds - Prepare to install the new credentials
- * from bprm.
- *
- * @bprm: binprm for exec
- */
-static void smack_bprm_committing_creds(struct linux_binprm *bprm)
-{
- struct task_smack *bsp = bprm->cred->security;
-
- if (bsp->smk_task != bsp->smk_forked)
- current->pdeath_signal = 0;
-}
-
/*
* Inode hooks
*/
@@ -4633,7 +4619,6 @@ static struct security_hook_list smack_hooks[] __lsm_ro_after_init = {
LSM_HOOK_INIT(sb_parse_opts_str, smack_parse_opts_str),
LSM_HOOK_INIT(bprm_set_creds, smack_bprm_set_creds),
- LSM_HOOK_INIT(bprm_committing_creds, smack_bprm_committing_creds),
LSM_HOOK_INIT(inode_alloc_security, smack_inode_alloc_security),
LSM_HOOK_INIT(inode_free_security, smack_inode_free_security),
--
2.7.4
The Smack bprm_secureexec hook can be merged with the bprm_set_creds
hook since it's dealing with the same information, and all of the details
are finalized during the first call to the bprm_set_creds hook via
prepare_binprm() (subsequent calls due to binfmt_script, etc, are ignored
via bprm->called_set_creds).
Here, the test can just happen at the end of the bprm_set_creds hook,
and the bprm_secureexec hook can be dropped.
Cc: Casey Schaufler <[email protected]>
Signed-off-by: Kees Cook <[email protected]>
---
security/smack/smack_lsm.c | 21 ++++-----------------
1 file changed, 4 insertions(+), 17 deletions(-)
diff --git a/security/smack/smack_lsm.c b/security/smack/smack_lsm.c
index 7d4b2e221124..4f1967be3d20 100644
--- a/security/smack/smack_lsm.c
+++ b/security/smack/smack_lsm.c
@@ -950,6 +950,10 @@ static int smack_bprm_set_creds(struct linux_binprm *bprm)
bsp->smk_task = isp->smk_task;
bprm->per_clear |= PER_CLEAR_ON_SETID;
+ /* Decide if this is a secure exec. */
+ if (bsp->smk_task != bsp->smk_forked)
+ bprm->secureexec = 1;
+
return 0;
}
@@ -967,22 +971,6 @@ static void smack_bprm_committing_creds(struct linux_binprm *bprm)
current->pdeath_signal = 0;
}
-/**
- * smack_bprm_secureexec - Return the decision to use secureexec.
- * @bprm: binprm for exec
- *
- * Returns 0 on success.
- */
-static int smack_bprm_secureexec(struct linux_binprm *bprm)
-{
- struct task_smack *tsp = current_security();
-
- if (tsp->smk_task != tsp->smk_forked)
- return 1;
-
- return 0;
-}
-
/*
* Inode hooks
*/
@@ -4646,7 +4634,6 @@ static struct security_hook_list smack_hooks[] __lsm_ro_after_init = {
LSM_HOOK_INIT(bprm_set_creds, smack_bprm_set_creds),
LSM_HOOK_INIT(bprm_committing_creds, smack_bprm_committing_creds),
- LSM_HOOK_INIT(bprm_secureexec, smack_bprm_secureexec),
LSM_HOOK_INIT(inode_alloc_security, smack_inode_alloc_security),
LSM_HOOK_INIT(inode_free_security, smack_inode_free_security),
--
2.7.4
The AppArmor bprm_secureexec hook can be merged with the bprm_set_creds
hook since it's dealing with the same information, and all of the details
are finalized during the first call to the bprm_set_creds hook via
prepare_binprm() (subsequent calls due to binfmt_script, etc, are ignored
via bprm->called_set_creds).
Here, all the comments describe how secureexec is actually calculated
during bprm_set_creds, so this actually does it, drops the bprm flag that
was being used internally by AppArmor, and drops the bprm_secureexec hook.
Cc: John Johansen <[email protected]>
Signed-off-by: Kees Cook <[email protected]>
---
security/apparmor/domain.c | 22 +---------------------
security/apparmor/include/domain.h | 1 -
security/apparmor/include/file.h | 3 ---
security/apparmor/lsm.c | 1 -
4 files changed, 1 insertion(+), 26 deletions(-)
diff --git a/security/apparmor/domain.c b/security/apparmor/domain.c
index 878407e023e3..1a1b1ec89d9d 100644
--- a/security/apparmor/domain.c
+++ b/security/apparmor/domain.c
@@ -485,14 +485,11 @@ int apparmor_bprm_set_creds(struct linux_binprm *bprm)
*
* Cases 2 and 3 are marked as requiring secure exec
* (unless policy specified "unsafe exec")
- *
- * bprm->unsafe is used to cache the AA_X_UNSAFE permission
- * to avoid having to recompute in secureexec
*/
if (!(perms.xindex & AA_X_UNSAFE)) {
AA_DEBUG("scrubbing environment variables for %s profile=%s\n",
name, new_profile->base.hname);
- bprm->unsafe |= AA_SECURE_X_NEEDED;
+ bprm->secureexec = 1;
}
apply:
/* when transitioning profiles clear unsafe personality bits */
@@ -521,23 +518,6 @@ int apparmor_bprm_set_creds(struct linux_binprm *bprm)
}
/**
- * apparmor_bprm_secureexec - determine if secureexec is needed
- * @bprm: binprm for exec (NOT NULL)
- *
- * Returns: %1 if secureexec is needed else %0
- */
-int apparmor_bprm_secureexec(struct linux_binprm *bprm)
-{
- /* the decision to use secure exec is computed in set_creds
- * and stored in bprm->unsafe.
- */
- if (bprm->unsafe & AA_SECURE_X_NEEDED)
- return 1;
-
- return 0;
-}
-
-/**
* apparmor_bprm_committing_creds - do task cleanup on committing new creds
* @bprm: binprm for the exec (NOT NULL)
*/
diff --git a/security/apparmor/include/domain.h b/security/apparmor/include/domain.h
index 30544729878a..2495af293587 100644
--- a/security/apparmor/include/domain.h
+++ b/security/apparmor/include/domain.h
@@ -24,7 +24,6 @@ struct aa_domain {
};
int apparmor_bprm_set_creds(struct linux_binprm *bprm);
-int apparmor_bprm_secureexec(struct linux_binprm *bprm);
void apparmor_bprm_committing_creds(struct linux_binprm *bprm);
void apparmor_bprm_committed_creds(struct linux_binprm *bprm);
diff --git a/security/apparmor/include/file.h b/security/apparmor/include/file.h
index 38f821bf49b6..076ac4501d97 100644
--- a/security/apparmor/include/file.h
+++ b/security/apparmor/include/file.h
@@ -66,9 +66,6 @@ struct path;
#define AA_X_INHERIT 0x4000
#define AA_X_UNCONFINED 0x8000
-/* AA_SECURE_X_NEEDED - is passed in the bprm->unsafe field */
-#define AA_SECURE_X_NEEDED 0x8000
-
/* need to make conditional which ones are being set */
struct path_cond {
kuid_t uid;
diff --git a/security/apparmor/lsm.c b/security/apparmor/lsm.c
index 8f3c0f7aca5a..291c7126350f 100644
--- a/security/apparmor/lsm.c
+++ b/security/apparmor/lsm.c
@@ -624,7 +624,6 @@ static struct security_hook_list apparmor_hooks[] __lsm_ro_after_init = {
LSM_HOOK_INIT(bprm_set_creds, apparmor_bprm_set_creds),
LSM_HOOK_INIT(bprm_committing_creds, apparmor_bprm_committing_creds),
LSM_HOOK_INIT(bprm_committed_creds, apparmor_bprm_committed_creds),
- LSM_HOOK_INIT(bprm_secureexec, apparmor_bprm_secureexec),
LSM_HOOK_INIT(task_setrlimit, apparmor_task_setrlimit),
};
--
2.7.4
The examination of "current" to decide dumpability is wrong. This was a
check of and euid/uid (or egid/gid) mismatch in the existing process,
not the newly created one. This appears to stretch back into even the
"history.git" tree. Luckily, dumpability is later set in commit_creds().
In earlier kernel versions before creds existed, similar checks also
existed late in the exec flow, covering up the mistake as far back as I
could find.
Note that because the commit_creds() check examines differences of euid,
uid, egid, gid, and capabilities between the old and new creds, it would
look like the setup_new_exec() dumpability test could be entirely removed.
However, the secureexec test may cover a different set of tests (specific
to the LSMs) than what commit_creds() checks for. So, fix this test to
use secureexec (the removed euid tests are redundant to the commoncap
secureexec checks now).
Cc: David Howells <[email protected]>
Signed-off-by: Kees Cook <[email protected]>
---
fs/exec.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/fs/exec.c b/fs/exec.c
index f9480d3e0b82..5241c8f25f5d 100644
--- a/fs/exec.c
+++ b/fs/exec.c
@@ -1353,7 +1353,7 @@ void setup_new_exec(struct linux_binprm * bprm)
current->sas_ss_sp = current->sas_ss_size = 0;
- if (uid_eq(current_euid(), current_uid()) && gid_eq(current_egid(), current_gid()))
+ if (!bprm->secureexec)
set_dumpable(current->mm, SUID_DUMP_USER);
else
set_dumpable(current->mm, suid_dumpable);
--
2.7.4
The bprm_secureexec hook can be moved earlier. Right now, it is called
during create_elf_tables(), via load_binary(), via search_binary_handler(),
via exec_binprm(). Nearly all (see exception below) state used by
bprm_secureexec is created during the bprm_set_creds hook, called from
prepare_binprm().
For all LSMs (except commoncaps described next), only the first execution
of bprm_set_creds takes any effect (they all check bprm->cred_prepared which
prepare_binprm() sets after the first call to the bprm_set_creds hook).
However, all these LSMs also only do anything with bprm_secureexec when
they detected a secure state during their first run of bprm_set_creds.
Therefore, it is functionally identical to move the detection into
bprm_set_creds, since the results from secureexec here only need to be
based on the first call to the LSM's bprm_set_creds hook.
The single exception is that the commoncaps secureexec hook also examines
euid/uid and egid/gid differences which are controlled by bprm_fill_uid(),
via prepare_binprm(), which can be called multiple times (e.g.
binfmt_script, binfmt_misc), and may clear the euid/egid for the final
load (i.e. the script interpreter). However, while commoncaps specifically
ignores bprm->cred_prepared, and runs its bprm_set_creds hook each time
prepare_binprm() may get called, it needs to base the secureexec decision
on the final call to bprm_set_creds. As a result, it will need special
handling.
To begin this refactoring, this adds the secureexec flag to the bprm
struct, which will eventually be used in place of the LSM hook.
Cc: David Howells <[email protected]>
Signed-off-by: Kees Cook <[email protected]>
---
fs/binfmt_elf.c | 3 ++-
fs/binfmt_elf_fdpic.c | 3 ++-
include/linux/binfmts.h | 8 +++++++-
3 files changed, 11 insertions(+), 3 deletions(-)
diff --git a/fs/binfmt_elf.c b/fs/binfmt_elf.c
index 5075fd5c62c8..991e4de3515f 100644
--- a/fs/binfmt_elf.c
+++ b/fs/binfmt_elf.c
@@ -254,7 +254,8 @@ create_elf_tables(struct linux_binprm *bprm, struct elfhdr *exec,
NEW_AUX_ENT(AT_EUID, from_kuid_munged(cred->user_ns, cred->euid));
NEW_AUX_ENT(AT_GID, from_kgid_munged(cred->user_ns, cred->gid));
NEW_AUX_ENT(AT_EGID, from_kgid_munged(cred->user_ns, cred->egid));
- NEW_AUX_ENT(AT_SECURE, security_bprm_secureexec(bprm));
+ bprm->secureexec |= security_bprm_secureexec(bprm);
+ NEW_AUX_ENT(AT_SECURE, bprm->secureexec);
NEW_AUX_ENT(AT_RANDOM, (elf_addr_t)(unsigned long)u_rand_bytes);
#ifdef ELF_HWCAP2
NEW_AUX_ENT(AT_HWCAP2, ELF_HWCAP2);
diff --git a/fs/binfmt_elf_fdpic.c b/fs/binfmt_elf_fdpic.c
index cf93a4fad012..c88b35d4a6b3 100644
--- a/fs/binfmt_elf_fdpic.c
+++ b/fs/binfmt_elf_fdpic.c
@@ -650,7 +650,8 @@ static int create_elf_fdpic_tables(struct linux_binprm *bprm,
NEW_AUX_ENT(AT_EUID, (elf_addr_t) from_kuid_munged(cred->user_ns, cred->euid));
NEW_AUX_ENT(AT_GID, (elf_addr_t) from_kgid_munged(cred->user_ns, cred->gid));
NEW_AUX_ENT(AT_EGID, (elf_addr_t) from_kgid_munged(cred->user_ns, cred->egid));
- NEW_AUX_ENT(AT_SECURE, security_bprm_secureexec(bprm));
+ bprm->secureexec |= security_bprm_secureexec(bprm);
+ NEW_AUX_ENT(AT_SECURE, bprm->secureexec);
NEW_AUX_ENT(AT_EXECFN, bprm->exec);
#ifdef ARCH_DLINFO
diff --git a/include/linux/binfmts.h b/include/linux/binfmts.h
index 05488da3aee9..9508b5f83c7e 100644
--- a/include/linux/binfmts.h
+++ b/include/linux/binfmts.h
@@ -27,9 +27,15 @@ struct linux_binprm {
unsigned int
cred_prepared:1,/* true if creds already prepared (multiple
* preps happen for interpreters) */
- cap_effective:1;/* true if has elevated effective capabilities,
+ cap_effective:1,/* true if has elevated effective capabilities,
* false if not; except for init which inherits
* its parent's caps anyway */
+ /*
+ * Set by bprm_set_creds hook to indicate a privilege-gaining
+ * exec has happened. Used to sanitize execution environment
+ * and to set AT_SECURE auxv for glibc.
+ */
+ secureexec:1;
#ifdef __alpha__
unsigned int taso:1;
#endif
--
2.7.4
The SELinux bprm_secureexec hook can be merged with the bprm_set_creds
hook since it's dealing with the same information, and all of the details
are finalized during the first call to the bprm_set_creds hook via
prepare_binprm() (subsequent calls due to binfmt_script, etc, are ignored
via bprm->called_set_creds).
Here, the test can just happen at the end of the bprm_set_creds hook,
and the bprm_secureexec hook can be dropped.
Cc: Paul Moore <[email protected]>
Cc: Stephen Smalley <[email protected]>
Signed-off-by: Kees Cook <[email protected]>
---
security/selinux/hooks.c | 24 +++++-------------------
1 file changed, 5 insertions(+), 19 deletions(-)
diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c
index 0f1450a06b02..18038f73a2f7 100644
--- a/security/selinux/hooks.c
+++ b/security/selinux/hooks.c
@@ -2413,30 +2413,17 @@ static int selinux_bprm_set_creds(struct linux_binprm *bprm)
/* Clear any possibly unsafe personality bits on exec: */
bprm->per_clear |= PER_CLEAR_ON_SETID;
- }
-
- return 0;
-}
-
-static int selinux_bprm_secureexec(struct linux_binprm *bprm)
-{
- const struct task_security_struct *tsec = current_security();
- u32 sid, osid;
- int atsecure = 0;
-
- sid = tsec->sid;
- osid = tsec->osid;
- if (osid != sid) {
/* Enable secure mode for SIDs transitions unless
the noatsecure permission is granted between
the two SIDs, i.e. ahp returns 0. */
- atsecure = avc_has_perm(osid, sid,
- SECCLASS_PROCESS,
- PROCESS__NOATSECURE, NULL);
+ rc = avc_has_perm(old_tsec->sid, new_tsec->sid,
+ SECCLASS_PROCESS, PROCESS__NOATSECURE,
+ NULL);
+ bprm->secureexec |= !!rc;
}
- return !!atsecure;
+ return 0;
}
static int match_file(const void *p, struct file *file, unsigned fd)
@@ -6151,7 +6138,6 @@ static struct security_hook_list selinux_hooks[] __lsm_ro_after_init = {
LSM_HOOK_INIT(bprm_set_creds, selinux_bprm_set_creds),
LSM_HOOK_INIT(bprm_committing_creds, selinux_bprm_committing_creds),
LSM_HOOK_INIT(bprm_committed_creds, selinux_bprm_committed_creds),
- LSM_HOOK_INIT(bprm_secureexec, selinux_bprm_secureexec),
LSM_HOOK_INIT(sb_alloc_security, selinux_sb_alloc_security),
LSM_HOOK_INIT(sb_free_security, selinux_sb_free_security),
--
2.7.4
The cred_prepared bprm flag has a misleading name. It has nothing to do
with the bprm_prepare_cred hook, and actually tracks if bprm_set_creds has
been called. Rename this flag and improve its comment.
Cc: David Howells <[email protected]>
Cc: John Johansen <[email protected]>
Cc: Paul Moore <[email protected]>
Cc: Stephen Smalley <[email protected]>
Cc: Casey Schaufler <[email protected]>
Cc: James Morris <[email protected]>
Signed-off-by: Kees Cook <[email protected]>
---
fs/binfmt_flat.c | 2 +-
fs/exec.c | 2 +-
include/linux/binfmts.h | 8 ++++++--
security/apparmor/domain.c | 2 +-
security/selinux/hooks.c | 2 +-
security/smack/smack_lsm.c | 2 +-
security/tomoyo/tomoyo.c | 2 +-
7 files changed, 12 insertions(+), 8 deletions(-)
diff --git a/fs/binfmt_flat.c b/fs/binfmt_flat.c
index 2edcefc0a294..a722530cc468 100644
--- a/fs/binfmt_flat.c
+++ b/fs/binfmt_flat.c
@@ -885,7 +885,7 @@ static int load_flat_shared_library(int id, struct lib_info *libs)
* as we're past the point of no return and are dealing with shared
* libraries.
*/
- bprm.cred_prepared = 1;
+ bprm.called_set_creds = 1;
res = prepare_binprm(&bprm);
diff --git a/fs/exec.c b/fs/exec.c
index 904199086490..925c85a45d97 100644
--- a/fs/exec.c
+++ b/fs/exec.c
@@ -1547,7 +1547,7 @@ int prepare_binprm(struct linux_binprm *bprm)
retval = security_bprm_set_creds(bprm);
if (retval)
return retval;
- bprm->cred_prepared = 1;
+ bprm->called_set_creds = 1;
memset(bprm->buf, 0, BINPRM_BUF_SIZE);
return kernel_read(bprm->file, 0, bprm->buf, BINPRM_BUF_SIZE);
diff --git a/include/linux/binfmts.h b/include/linux/binfmts.h
index 9508b5f83c7e..36be5a67517a 100644
--- a/include/linux/binfmts.h
+++ b/include/linux/binfmts.h
@@ -25,8 +25,12 @@ struct linux_binprm {
struct mm_struct *mm;
unsigned long p; /* current top of mem */
unsigned int
- cred_prepared:1,/* true if creds already prepared (multiple
- * preps happen for interpreters) */
+ /*
+ * True after the bprm_set_creds hook has been called once
+ * (multiple calls can be made via prepare_binprm() for
+ * binfmt_script/misc).
+ */
+ called_set_creds:1,
cap_effective:1,/* true if has elevated effective capabilities,
* false if not; except for init which inherits
* its parent's caps anyway */
diff --git a/security/apparmor/domain.c b/security/apparmor/domain.c
index 001e133a3c8c..878407e023e3 100644
--- a/security/apparmor/domain.c
+++ b/security/apparmor/domain.c
@@ -350,7 +350,7 @@ int apparmor_bprm_set_creds(struct linux_binprm *bprm)
const char *name = NULL, *info = NULL;
int error = 0;
- if (bprm->cred_prepared)
+ if (bprm->called_set_creds)
return 0;
ctx = cred_ctx(bprm->cred);
diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c
index 819fd6858b49..0f1450a06b02 100644
--- a/security/selinux/hooks.c
+++ b/security/selinux/hooks.c
@@ -2327,7 +2327,7 @@ static int selinux_bprm_set_creds(struct linux_binprm *bprm)
/* SELinux context only depends on initial program or script and not
* the script interpreter */
- if (bprm->cred_prepared)
+ if (bprm->called_set_creds)
return 0;
old_tsec = current_security();
diff --git a/security/smack/smack_lsm.c b/security/smack/smack_lsm.c
index 658f5d8c7e76..7d4b2e221124 100644
--- a/security/smack/smack_lsm.c
+++ b/security/smack/smack_lsm.c
@@ -917,7 +917,7 @@ static int smack_bprm_set_creds(struct linux_binprm *bprm)
struct superblock_smack *sbsp;
int rc;
- if (bprm->cred_prepared)
+ if (bprm->called_set_creds)
return 0;
isp = inode->i_security;
diff --git a/security/tomoyo/tomoyo.c b/security/tomoyo/tomoyo.c
index 130b4fa4f65f..d25b705360e0 100644
--- a/security/tomoyo/tomoyo.c
+++ b/security/tomoyo/tomoyo.c
@@ -76,7 +76,7 @@ static int tomoyo_bprm_set_creds(struct linux_binprm *bprm)
* Do only if this function is called for the first time of an execve
* operation.
*/
- if (bprm->cred_prepared)
+ if (bprm->called_set_creds)
return 0;
#ifndef CONFIG_SECURITY_TOMOYO_OMIT_USERSPACE_LOADER
/*
--
2.7.4
On Tue, 18 Jul 2017, Kees Cook wrote:
>
> This series has grown... :P
Hmm. It may be bigger, but I like it a lot better. Each step now looks
fairly obvious and is well documented.
I don't love the timing of it, but I think I'd be willing to just pull
this in before rc2 as a "we need to do this sooner or later anyway and
probably mark much of it for stable" kind of thing.
But it would be really good to get people to look at the series,
particularly Andy and the LSM folks..
Linus
On 07/18/2017 03:25 PM, Kees Cook wrote:
> The AppArmor bprm_secureexec hook can be merged with the bprm_set_creds
> hook since it's dealing with the same information, and all of the details
> are finalized during the first call to the bprm_set_creds hook via
> prepare_binprm() (subsequent calls due to binfmt_script, etc, are ignored
> via bprm->called_set_creds).
>
> Here, all the comments describe how secureexec is actually calculated
> during bprm_set_creds, so this actually does it, drops the bprm flag that
> was being used internally by AppArmor, and drops the bprm_secureexec hook.
>
> Cc: John Johansen <[email protected]>
> Signed-off-by: Kees Cook <[email protected]>
Acked-by: John Johansen <[email protected]>
> ---
> security/apparmor/domain.c | 22 +---------------------
> security/apparmor/include/domain.h | 1 -
> security/apparmor/include/file.h | 3 ---
> security/apparmor/lsm.c | 1 -
> 4 files changed, 1 insertion(+), 26 deletions(-)
>
> diff --git a/security/apparmor/domain.c b/security/apparmor/domain.c
> index 878407e023e3..1a1b1ec89d9d 100644
> --- a/security/apparmor/domain.c
> +++ b/security/apparmor/domain.c
> @@ -485,14 +485,11 @@ int apparmor_bprm_set_creds(struct linux_binprm *bprm)
> *
> * Cases 2 and 3 are marked as requiring secure exec
> * (unless policy specified "unsafe exec")
> - *
> - * bprm->unsafe is used to cache the AA_X_UNSAFE permission
> - * to avoid having to recompute in secureexec
> */
> if (!(perms.xindex & AA_X_UNSAFE)) {
> AA_DEBUG("scrubbing environment variables for %s profile=%s\n",
> name, new_profile->base.hname);
> - bprm->unsafe |= AA_SECURE_X_NEEDED;
> + bprm->secureexec = 1;
> }
> apply:
> /* when transitioning profiles clear unsafe personality bits */
> @@ -521,23 +518,6 @@ int apparmor_bprm_set_creds(struct linux_binprm *bprm)
> }
>
> /**
> - * apparmor_bprm_secureexec - determine if secureexec is needed
> - * @bprm: binprm for exec (NOT NULL)
> - *
> - * Returns: %1 if secureexec is needed else %0
> - */
> -int apparmor_bprm_secureexec(struct linux_binprm *bprm)
> -{
> - /* the decision to use secure exec is computed in set_creds
> - * and stored in bprm->unsafe.
> - */
> - if (bprm->unsafe & AA_SECURE_X_NEEDED)
> - return 1;
> -
> - return 0;
> -}
> -
> -/**
> * apparmor_bprm_committing_creds - do task cleanup on committing new creds
> * @bprm: binprm for the exec (NOT NULL)
> */
> diff --git a/security/apparmor/include/domain.h b/security/apparmor/include/domain.h
> index 30544729878a..2495af293587 100644
> --- a/security/apparmor/include/domain.h
> +++ b/security/apparmor/include/domain.h
> @@ -24,7 +24,6 @@ struct aa_domain {
> };
>
> int apparmor_bprm_set_creds(struct linux_binprm *bprm);
> -int apparmor_bprm_secureexec(struct linux_binprm *bprm);
> void apparmor_bprm_committing_creds(struct linux_binprm *bprm);
> void apparmor_bprm_committed_creds(struct linux_binprm *bprm);
>
> diff --git a/security/apparmor/include/file.h b/security/apparmor/include/file.h
> index 38f821bf49b6..076ac4501d97 100644
> --- a/security/apparmor/include/file.h
> +++ b/security/apparmor/include/file.h
> @@ -66,9 +66,6 @@ struct path;
> #define AA_X_INHERIT 0x4000
> #define AA_X_UNCONFINED 0x8000
>
> -/* AA_SECURE_X_NEEDED - is passed in the bprm->unsafe field */
> -#define AA_SECURE_X_NEEDED 0x8000
> -
> /* need to make conditional which ones are being set */
> struct path_cond {
> kuid_t uid;
> diff --git a/security/apparmor/lsm.c b/security/apparmor/lsm.c
> index 8f3c0f7aca5a..291c7126350f 100644
> --- a/security/apparmor/lsm.c
> +++ b/security/apparmor/lsm.c
> @@ -624,7 +624,6 @@ static struct security_hook_list apparmor_hooks[] __lsm_ro_after_init = {
> LSM_HOOK_INIT(bprm_set_creds, apparmor_bprm_set_creds),
> LSM_HOOK_INIT(bprm_committing_creds, apparmor_bprm_committing_creds),
> LSM_HOOK_INIT(bprm_committed_creds, apparmor_bprm_committed_creds),
> - LSM_HOOK_INIT(bprm_secureexec, apparmor_bprm_secureexec),
>
> LSM_HOOK_INIT(task_setrlimit, apparmor_task_setrlimit),
> };
>
On 07/18/2017 03:25 PM, Kees Cook wrote:
> This removes the bprm_secureexec hook since the logic has been folded into
> the bprm_set_creds hook for all LSMs now.
>
> Cc: James Morris <[email protected]>
> Cc: Eric W. Biederman <[email protected]>
> Signed-off-by: Kees Cook <[email protected]>
looks good
Reviewed-by: John Johansen <[email protected]>
> ---
> fs/binfmt_elf.c | 1 -
> fs/binfmt_elf_fdpic.c | 1 -
> include/linux/lsm_hooks.h | 14 +++++---------
> include/linux/security.h | 7 -------
> security/security.c | 5 -----
> 5 files changed, 5 insertions(+), 23 deletions(-)
>
> diff --git a/fs/binfmt_elf.c b/fs/binfmt_elf.c
> index 991e4de3515f..7f6ec4dac13d 100644
> --- a/fs/binfmt_elf.c
> +++ b/fs/binfmt_elf.c
> @@ -254,7 +254,6 @@ create_elf_tables(struct linux_binprm *bprm, struct elfhdr *exec,
> NEW_AUX_ENT(AT_EUID, from_kuid_munged(cred->user_ns, cred->euid));
> NEW_AUX_ENT(AT_GID, from_kgid_munged(cred->user_ns, cred->gid));
> NEW_AUX_ENT(AT_EGID, from_kgid_munged(cred->user_ns, cred->egid));
> - bprm->secureexec |= security_bprm_secureexec(bprm);
> NEW_AUX_ENT(AT_SECURE, bprm->secureexec);
> NEW_AUX_ENT(AT_RANDOM, (elf_addr_t)(unsigned long)u_rand_bytes);
> #ifdef ELF_HWCAP2
> diff --git a/fs/binfmt_elf_fdpic.c b/fs/binfmt_elf_fdpic.c
> index c88b35d4a6b3..5aa9199dfb13 100644
> --- a/fs/binfmt_elf_fdpic.c
> +++ b/fs/binfmt_elf_fdpic.c
> @@ -650,7 +650,6 @@ static int create_elf_fdpic_tables(struct linux_binprm *bprm,
> NEW_AUX_ENT(AT_EUID, (elf_addr_t) from_kuid_munged(cred->user_ns, cred->euid));
> NEW_AUX_ENT(AT_GID, (elf_addr_t) from_kgid_munged(cred->user_ns, cred->gid));
> NEW_AUX_ENT(AT_EGID, (elf_addr_t) from_kgid_munged(cred->user_ns, cred->egid));
> - bprm->secureexec |= security_bprm_secureexec(bprm);
> NEW_AUX_ENT(AT_SECURE, bprm->secureexec);
> NEW_AUX_ENT(AT_EXECFN, bprm->exec);
>
> diff --git a/include/linux/lsm_hooks.h b/include/linux/lsm_hooks.h
> index 080f34e66017..2ddc1c7e8923 100644
> --- a/include/linux/lsm_hooks.h
> +++ b/include/linux/lsm_hooks.h
> @@ -40,7 +40,11 @@
> * interpreters. The hook can tell whether it has already been called by
> * checking to see if @bprm->security is non-NULL. If so, then the hook
> * may decide either to retain the security information saved earlier or
> - * to replace it.
> + * to replace it. The hook must set @bprm->secureexec to 1 if a "secure
> + * exec" has happened as a result of this hook call. The flag is used to
> + * indicate the need for a sanitized execution environment, and is also
> + * passed in the ELF auxiliary table on the initial stack to indicate
> + * whether libc should enable secure mode.
> * @bprm contains the linux_binprm structure.
> * Return 0 if the hook is successful and permission is granted.
> * @bprm_check_security:
> @@ -68,12 +72,6 @@
> * linux_binprm structure. This hook is a good place to perform state
> * changes on the process such as clearing out non-inheritable signal
> * state. This is called immediately after commit_creds().
> - * @bprm_secureexec:
> - * Return a boolean value (0 or 1) indicating whether a "secure exec"
> - * is required. The flag is passed in the auxiliary table
> - * on the initial stack to the ELF interpreter to indicate whether libc
> - * should enable secure mode.
> - * @bprm contains the linux_binprm structure.
> *
> * Security hooks for filesystem operations.
> *
> @@ -1368,7 +1366,6 @@ union security_list_options {
>
> int (*bprm_set_creds)(struct linux_binprm *bprm);
> int (*bprm_check_security)(struct linux_binprm *bprm);
> - int (*bprm_secureexec)(struct linux_binprm *bprm);
> void (*bprm_committing_creds)(struct linux_binprm *bprm);
> void (*bprm_committed_creds)(struct linux_binprm *bprm);
>
> @@ -1680,7 +1677,6 @@ struct security_hook_heads {
> struct list_head vm_enough_memory;
> struct list_head bprm_set_creds;
> struct list_head bprm_check_security;
> - struct list_head bprm_secureexec;
> struct list_head bprm_committing_creds;
> struct list_head bprm_committed_creds;
> struct list_head sb_alloc_security;
> diff --git a/include/linux/security.h b/include/linux/security.h
> index af675b576645..133c41bb666d 100644
> --- a/include/linux/security.h
> +++ b/include/linux/security.h
> @@ -80,7 +80,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);
> @@ -223,7 +222,6 @@ int security_bprm_set_creds(struct linux_binprm *bprm);
> int security_bprm_check(struct linux_binprm *bprm);
> void security_bprm_committing_creds(struct linux_binprm *bprm);
> void security_bprm_committed_creds(struct linux_binprm *bprm);
> -int security_bprm_secureexec(struct linux_binprm *bprm);
> int security_sb_alloc(struct super_block *sb);
> void security_sb_free(struct super_block *sb);
> int security_sb_copy_data(char *orig, char *copy);
> @@ -515,11 +513,6 @@ 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);
> -}
> -
> static inline int security_sb_alloc(struct super_block *sb)
> {
> return 0;
> diff --git a/security/security.c b/security/security.c
> index b9fea3999cf8..750b83186869 100644
> --- a/security/security.c
> +++ b/security/security.c
> @@ -311,11 +311,6 @@ void security_bprm_committed_creds(struct linux_binprm *bprm)
> call_void_hook(bprm_committed_creds, bprm);
> }
>
> -int security_bprm_secureexec(struct linux_binprm *bprm)
> -{
> - return call_int_hook(bprm_secureexec, 0, bprm);
> -}
> -
> int security_sb_alloc(struct super_block *sb)
> {
> return call_int_hook(sb_alloc_security, 0, sb);
>
On 07/18/2017 03:25 PM, Kees Cook wrote:
> The bprm_secureexec hook can be moved earlier. Right now, it is called
> during create_elf_tables(), via load_binary(), via search_binary_handler(),
> via exec_binprm(). Nearly all (see exception below) state used by
> bprm_secureexec is created during the bprm_set_creds hook, called from
> prepare_binprm().
>
> For all LSMs (except commoncaps described next), only the first execution
> of bprm_set_creds takes any effect (they all check bprm->cred_prepared which
> prepare_binprm() sets after the first call to the bprm_set_creds hook).
> However, all these LSMs also only do anything with bprm_secureexec when
> they detected a secure state during their first run of bprm_set_creds.
> Therefore, it is functionally identical to move the detection into
> bprm_set_creds, since the results from secureexec here only need to be
> based on the first call to the LSM's bprm_set_creds hook.
>
> The single exception is that the commoncaps secureexec hook also examines
> euid/uid and egid/gid differences which are controlled by bprm_fill_uid(),
> via prepare_binprm(), which can be called multiple times (e.g.
> binfmt_script, binfmt_misc), and may clear the euid/egid for the final
> load (i.e. the script interpreter). However, while commoncaps specifically
> ignores bprm->cred_prepared, and runs its bprm_set_creds hook each time
> prepare_binprm() may get called, it needs to base the secureexec decision
> on the final call to bprm_set_creds. As a result, it will need special
> handling.
>
> To begin this refactoring, this adds the secureexec flag to the bprm
> struct, which will eventually be used in place of the LSM hook.
>
> Cc: David Howells <[email protected]>
> Signed-off-by: Kees Cook <[email protected]>
looks good
Reviewed-by: John Johansen <[email protected]>
> ---
> fs/binfmt_elf.c | 3 ++-
> fs/binfmt_elf_fdpic.c | 3 ++-
> include/linux/binfmts.h | 8 +++++++-
> 3 files changed, 11 insertions(+), 3 deletions(-)
>
> diff --git a/fs/binfmt_elf.c b/fs/binfmt_elf.c
> index 5075fd5c62c8..991e4de3515f 100644
> --- a/fs/binfmt_elf.c
> +++ b/fs/binfmt_elf.c
> @@ -254,7 +254,8 @@ create_elf_tables(struct linux_binprm *bprm, struct elfhdr *exec,
> NEW_AUX_ENT(AT_EUID, from_kuid_munged(cred->user_ns, cred->euid));
> NEW_AUX_ENT(AT_GID, from_kgid_munged(cred->user_ns, cred->gid));
> NEW_AUX_ENT(AT_EGID, from_kgid_munged(cred->user_ns, cred->egid));
> - NEW_AUX_ENT(AT_SECURE, security_bprm_secureexec(bprm));
> + bprm->secureexec |= security_bprm_secureexec(bprm);
> + NEW_AUX_ENT(AT_SECURE, bprm->secureexec);
> NEW_AUX_ENT(AT_RANDOM, (elf_addr_t)(unsigned long)u_rand_bytes);
> #ifdef ELF_HWCAP2
> NEW_AUX_ENT(AT_HWCAP2, ELF_HWCAP2);
> diff --git a/fs/binfmt_elf_fdpic.c b/fs/binfmt_elf_fdpic.c
> index cf93a4fad012..c88b35d4a6b3 100644
> --- a/fs/binfmt_elf_fdpic.c
> +++ b/fs/binfmt_elf_fdpic.c
> @@ -650,7 +650,8 @@ static int create_elf_fdpic_tables(struct linux_binprm *bprm,
> NEW_AUX_ENT(AT_EUID, (elf_addr_t) from_kuid_munged(cred->user_ns, cred->euid));
> NEW_AUX_ENT(AT_GID, (elf_addr_t) from_kgid_munged(cred->user_ns, cred->gid));
> NEW_AUX_ENT(AT_EGID, (elf_addr_t) from_kgid_munged(cred->user_ns, cred->egid));
> - NEW_AUX_ENT(AT_SECURE, security_bprm_secureexec(bprm));
> + bprm->secureexec |= security_bprm_secureexec(bprm);
> + NEW_AUX_ENT(AT_SECURE, bprm->secureexec);
> NEW_AUX_ENT(AT_EXECFN, bprm->exec);
>
> #ifdef ARCH_DLINFO
> diff --git a/include/linux/binfmts.h b/include/linux/binfmts.h
> index 05488da3aee9..9508b5f83c7e 100644
> --- a/include/linux/binfmts.h
> +++ b/include/linux/binfmts.h
> @@ -27,9 +27,15 @@ struct linux_binprm {
> unsigned int
> cred_prepared:1,/* true if creds already prepared (multiple
> * preps happen for interpreters) */
> - cap_effective:1;/* true if has elevated effective capabilities,
> + cap_effective:1,/* true if has elevated effective capabilities,
> * false if not; except for init which inherits
> * its parent's caps anyway */
> + /*
> + * Set by bprm_set_creds hook to indicate a privilege-gaining
> + * exec has happened. Used to sanitize execution environment
> + * and to set AT_SECURE auxv for glibc.
> + */
> + secureexec:1;
> #ifdef __alpha__
> unsigned int taso:1;
> #endif
>
On 07/18/2017 03:25 PM, Kees Cook wrote:
> The cred_prepared bprm flag has a misleading name. It has nothing to do
> with the bprm_prepare_cred hook, and actually tracks if bprm_set_creds has
> been called. Rename this flag and improve its comment.
>
> Cc: David Howells <[email protected]>
> Cc: John Johansen <[email protected]>
> Cc: Paul Moore <[email protected]>
> Cc: Stephen Smalley <[email protected]>
> Cc: Casey Schaufler <[email protected]>
> Cc: James Morris <[email protected]>
> Signed-off-by: Kees Cook <[email protected]>
looks good
Acked-by: John Johansen <[email protected]>
> ---
> fs/binfmt_flat.c | 2 +-
> fs/exec.c | 2 +-
> include/linux/binfmts.h | 8 ++++++--
> security/apparmor/domain.c | 2 +-
> security/selinux/hooks.c | 2 +-
> security/smack/smack_lsm.c | 2 +-
> security/tomoyo/tomoyo.c | 2 +-
> 7 files changed, 12 insertions(+), 8 deletions(-)
>
> diff --git a/fs/binfmt_flat.c b/fs/binfmt_flat.c
> index 2edcefc0a294..a722530cc468 100644
> --- a/fs/binfmt_flat.c
> +++ b/fs/binfmt_flat.c
> @@ -885,7 +885,7 @@ static int load_flat_shared_library(int id, struct lib_info *libs)
> * as we're past the point of no return and are dealing with shared
> * libraries.
> */
> - bprm.cred_prepared = 1;
> + bprm.called_set_creds = 1;
>
> res = prepare_binprm(&bprm);
>
> diff --git a/fs/exec.c b/fs/exec.c
> index 904199086490..925c85a45d97 100644
> --- a/fs/exec.c
> +++ b/fs/exec.c
> @@ -1547,7 +1547,7 @@ int prepare_binprm(struct linux_binprm *bprm)
> retval = security_bprm_set_creds(bprm);
> if (retval)
> return retval;
> - bprm->cred_prepared = 1;
> + bprm->called_set_creds = 1;
>
> memset(bprm->buf, 0, BINPRM_BUF_SIZE);
> return kernel_read(bprm->file, 0, bprm->buf, BINPRM_BUF_SIZE);
> diff --git a/include/linux/binfmts.h b/include/linux/binfmts.h
> index 9508b5f83c7e..36be5a67517a 100644
> --- a/include/linux/binfmts.h
> +++ b/include/linux/binfmts.h
> @@ -25,8 +25,12 @@ struct linux_binprm {
> struct mm_struct *mm;
> unsigned long p; /* current top of mem */
> unsigned int
> - cred_prepared:1,/* true if creds already prepared (multiple
> - * preps happen for interpreters) */
> + /*
> + * True after the bprm_set_creds hook has been called once
> + * (multiple calls can be made via prepare_binprm() for
> + * binfmt_script/misc).
> + */
> + called_set_creds:1,
> cap_effective:1,/* true if has elevated effective capabilities,
> * false if not; except for init which inherits
> * its parent's caps anyway */
> diff --git a/security/apparmor/domain.c b/security/apparmor/domain.c
> index 001e133a3c8c..878407e023e3 100644
> --- a/security/apparmor/domain.c
> +++ b/security/apparmor/domain.c
> @@ -350,7 +350,7 @@ int apparmor_bprm_set_creds(struct linux_binprm *bprm)
> const char *name = NULL, *info = NULL;
> int error = 0;
>
> - if (bprm->cred_prepared)
> + if (bprm->called_set_creds)
> return 0;
>
> ctx = cred_ctx(bprm->cred);
> diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c
> index 819fd6858b49..0f1450a06b02 100644
> --- a/security/selinux/hooks.c
> +++ b/security/selinux/hooks.c
> @@ -2327,7 +2327,7 @@ static int selinux_bprm_set_creds(struct linux_binprm *bprm)
>
> /* SELinux context only depends on initial program or script and not
> * the script interpreter */
> - if (bprm->cred_prepared)
> + if (bprm->called_set_creds)
> return 0;
>
> old_tsec = current_security();
> diff --git a/security/smack/smack_lsm.c b/security/smack/smack_lsm.c
> index 658f5d8c7e76..7d4b2e221124 100644
> --- a/security/smack/smack_lsm.c
> +++ b/security/smack/smack_lsm.c
> @@ -917,7 +917,7 @@ static int smack_bprm_set_creds(struct linux_binprm *bprm)
> struct superblock_smack *sbsp;
> int rc;
>
> - if (bprm->cred_prepared)
> + if (bprm->called_set_creds)
> return 0;
>
> isp = inode->i_security;
> diff --git a/security/tomoyo/tomoyo.c b/security/tomoyo/tomoyo.c
> index 130b4fa4f65f..d25b705360e0 100644
> --- a/security/tomoyo/tomoyo.c
> +++ b/security/tomoyo/tomoyo.c
> @@ -76,7 +76,7 @@ static int tomoyo_bprm_set_creds(struct linux_binprm *bprm)
> * Do only if this function is called for the first time of an execve
> * operation.
> */
> - if (bprm->cred_prepared)
> + if (bprm->called_set_creds)
> return 0;
> #ifndef CONFIG_SECURITY_TOMOYO_OMIT_USERSPACE_LOADER
> /*
>
Kees Cook <[email protected]> writes:
> In commit 221af7f87b97 ("Split 'flush_old_exec' into two functions"),
> the comment about the point of no return should have stayed in
> flush_old_exec() since it refers to "bprm->mm = NULL;" line, but prior
> changes in commits c89681ed7d0e ("remove steal_locks()"), and
> fd8328be874f ("sanitize handling of shared descriptor tables in failing
> execve()") made it look like it meant the current->sas_ss_sp line instead.
>
> The comment was referring to the fact that once bprm->mm is NULL, all
> failures from a binfmt load_binary hook (e.g. load_elf_binary), will
> get SEGV raised against current. Move this comment and expand the
> explanation a bit, putting it above the assignment this time, and add
> details about the true nature of "point of no return" being the call
> to flush_old_exec() itself.
>
> This also removes an erroneous commet about when credentials are being
> installed. That has its own dedicated function, install_exec_creds(),
> which carries a similar (and correct) comment, so remove the bogus comment
> where installation is not actually happening.
Acked-by: "Eric W. Biederman" <[email protected]>
>
> Cc: David Howells <[email protected]>
> Cc: Eric W. Biederman <[email protected]>
> Signed-off-by: Kees Cook <[email protected]>
> ---
> fs/exec.c | 16 +++++++++++++---
> 1 file changed, 13 insertions(+), 3 deletions(-)
>
> diff --git a/fs/exec.c b/fs/exec.c
> index 53ffa739fb58..f9480d3e0b82 100644
> --- a/fs/exec.c
> +++ b/fs/exec.c
> @@ -1258,6 +1258,12 @@ void __set_task_comm(struct task_struct *tsk, const char *buf, bool exec)
> perf_event_comm(tsk, exec);
> }
>
> +/*
> + * Calling this is the point of no return. None of the failures will be
> + * seen by userspace since either the process is already taking a fatal
> + * signal (via de_thread() or coredump), or will have SEGV raised
> + * (after exec_mmap()) by search_binary_handlers (see below).
> + */
> int flush_old_exec(struct linux_binprm * bprm)
> {
> int retval;
> @@ -1285,7 +1291,13 @@ int flush_old_exec(struct linux_binprm * bprm)
> if (retval)
> goto out;
>
> - bprm->mm = NULL; /* We're using it now */
> + /*
> + * After clearing bprm->mm (to mark that current is using the
> + * prepared mm now), we have nothing left of the original
> + * process. If anything from here on returns an error, the check
> + * in search_binary_handler() will SEGV current.
> + */
> + bprm->mm = NULL;
>
> set_fs(USER_DS);
> current->flags &= ~(PF_RANDOMIZE | PF_FORKNOEXEC | PF_KTHREAD |
> @@ -1339,7 +1351,6 @@ void setup_new_exec(struct linux_binprm * bprm)
>
> arch_pick_mmap_layout(current->mm);
>
> - /* This is the point of no return */
> current->sas_ss_sp = current->sas_ss_size = 0;
>
> if (uid_eq(current_euid(), current_uid()) && gid_eq(current_egid(), current_gid()))
> @@ -1357,7 +1368,6 @@ void setup_new_exec(struct linux_binprm * bprm)
> */
> current->mm->task_size = TASK_SIZE;
>
> - /* install the new credentials */
> if (!uid_eq(bprm->cred->uid, current_euid()) ||
> !gid_eq(bprm->cred->gid, current_egid())) {
> current->pdeath_signal = 0;
On Tue, Jul 18, 2017 at 3:25 PM, Kees Cook <[email protected]> wrote:
> To begin this refactoring, this adds the secureexec flag to the bprm
> struct, which will eventually be used in place of the LSM hook.
I'm very confused. See below. Maybe a later patch will unconfuse me.
>
> Cc: David Howells <[email protected]>
> Signed-off-by: Kees Cook <[email protected]>
> ---
> fs/binfmt_elf.c | 3 ++-
> fs/binfmt_elf_fdpic.c | 3 ++-
> include/linux/binfmts.h | 8 +++++++-
> 3 files changed, 11 insertions(+), 3 deletions(-)
>
> diff --git a/fs/binfmt_elf.c b/fs/binfmt_elf.c
> index 5075fd5c62c8..991e4de3515f 100644
> --- a/fs/binfmt_elf.c
> +++ b/fs/binfmt_elf.c
> @@ -254,7 +254,8 @@ create_elf_tables(struct linux_binprm *bprm, struct elfhdr *exec,
> NEW_AUX_ENT(AT_EUID, from_kuid_munged(cred->user_ns, cred->euid));
> NEW_AUX_ENT(AT_GID, from_kgid_munged(cred->user_ns, cred->gid));
> NEW_AUX_ENT(AT_EGID, from_kgid_munged(cred->user_ns, cred->egid));
> - NEW_AUX_ENT(AT_SECURE, security_bprm_secureexec(bprm));
> + bprm->secureexec |= security_bprm_secureexec(bprm);
> + NEW_AUX_ENT(AT_SECURE, bprm->secureexec);
This is ->load_binary ...
> diff --git a/include/linux/binfmts.h b/include/linux/binfmts.h
> index 05488da3aee9..9508b5f83c7e 100644
> --- a/include/linux/binfmts.h
> +++ b/include/linux/binfmts.h
> @@ -27,9 +27,15 @@ struct linux_binprm {
> unsigned int
> cred_prepared:1,/* true if creds already prepared (multiple
> * preps happen for interpreters) */
> - cap_effective:1;/* true if has elevated effective capabilities,
> + cap_effective:1,/* true if has elevated effective capabilities,
> * false if not; except for init which inherits
> * its parent's caps anyway */
> + /*
> + * Set by bprm_set_creds hook to indicate a privilege-gaining
> + * exec has happened. Used to sanitize execution environment
> + * and to set AT_SECURE auxv for glibc.
> + */
... which is not bprm_set_creds().
What am I missing here?
On Tue, Jul 18, 2017 at 3:25 PM, Kees Cook <[email protected]> wrote:
> The cred_prepared bprm flag has a misleading name. It has nothing to do
> with the bprm_prepare_cred hook, and actually tracks if bprm_set_creds has
> been called. Rename this flag and improve its comment.
>
> Cc: David Howells <[email protected]>
> Cc: John Johansen <[email protected]>
> Cc: Paul Moore <[email protected]>
> Cc: Stephen Smalley <[email protected]>
> Cc: Casey Schaufler <[email protected]>
> Cc: James Morris <[email protected]>
> Signed-off-by: Kees Cook <[email protected]>
> ---
> fs/binfmt_flat.c | 2 +-
> fs/exec.c | 2 +-
> include/linux/binfmts.h | 8 ++++++--
> security/apparmor/domain.c | 2 +-
> security/selinux/hooks.c | 2 +-
> security/smack/smack_lsm.c | 2 +-
> security/tomoyo/tomoyo.c | 2 +-
> 7 files changed, 12 insertions(+), 8 deletions(-)
>
> diff --git a/fs/binfmt_flat.c b/fs/binfmt_flat.c
> index 2edcefc0a294..a722530cc468 100644
> --- a/fs/binfmt_flat.c
> +++ b/fs/binfmt_flat.c
> @@ -885,7 +885,7 @@ static int load_flat_shared_library(int id, struct lib_info *libs)
> * as we're past the point of no return and are dealing with shared
> * libraries.
> */
> - bprm.cred_prepared = 1;
> + bprm.called_set_creds = 1;
WTF is this? It's not, strictly speaking, a bug in this patch, but
it's nonsensical. Is it fixed (presuably deleted) later?
Otherwise looks good.
On Tue, Jul 18, 2017 at 3:25 PM, Kees Cook <[email protected]> wrote:
> 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.
Reviewed-by: Andy Lutomirski <[email protected]>
with the redundant caveat that...
> --- a/fs/exec.c
> +++ b/fs/exec.c
> @@ -1330,6 +1330,13 @@ EXPORT_SYMBOL(would_dump);
>
> void setup_new_exec(struct linux_binprm * 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;
> +
...the weird placement of the other assignments to bprm->secureexec
makes this exceedingly confusing.
On Tue, Jul 18, 2017 at 3:25 PM, Kees Cook <[email protected]> wrote:
> Instead of a separate function, open-code the cap_elevated test, which
> lets us entirely remove bprm->cap_effective (to use the local "effective"
> variable instead), and more accurately examine euid/egid changes via the
> existing local "is_setid".
...
> -static int is_secureexec(struct linux_binprm *bprm)
> -{
> - const struct cred *cred = bprm->cred;
> - kuid_t root_uid = make_kuid(cred->user_ns, 0);
> -
> - if (!uid_eq(cred->uid, root_uid)) {
> - if (bprm->cap_effective)
> - return 1;
> - if (!cap_issubset(cred->cap_permitted, cred->cap_ambient))
> - return 1;
> + bprm->cap_elevated = 0;
> + if (is_setid) {
> + bprm->cap_elevated = 1;
> + } else if (!uid_eq(new->uid, root_uid)) {
> + if (effective ||
> + !cap_issubset(new->cap_permitted, new->cap_ambient))
> + bprm->cap_elevated = 1;
> }
>
> - return (!uid_eq(cred->euid, cred->uid) ||
> - !gid_eq(cred->egid, cred->gid));
> + return 0;
I think this matches the old behavior. IOW it looks right.
On Tue, Jul 18, 2017 at 03:25:21PM -0700, Kees Cook wrote:
> This series has grown... :P
>
> As discussed with Linus and Andy, we need to reset the stack rlimit
> before we do memory layouts when execing a privilege-gaining (e.g.
> setuid) program. To do this, we need to know the results of the
> bprm_secureexec hook before memory layouts. As it turns out, this
> can be made _mostly_ trivial by collapsing bprm_secureexec into
> bprm_set_creds.
>
> The LSMs using bprm_secureexec nearly always save state between
> bprm_set_creds and bprm_secureexec. In the face of multiple calls to
> bprm_set_creds (via prepare_binprm() calls from binfmt_script, etc),
> all LSMs except commoncap only pay attention to the first call, so
> that aligns well with collapsing bprm_secureexec into bprm_set_creds.
> The commoncaps, though, needs to check the _last_ bprm_set_creds, so
> this series just swaps one bprm flag for another (cap_effective is no
> longer needed to save state between bprm_set_creds and bprm_secureexec,
> but we do need to keep a separate state, so we add the cap_elevated flag).
>
> Once secureexec is available to setup_new_exec() before the memory
> layout, we can add an rlimit sanity-check for setuid execs. (With no
> need to clean up since we're past the point of no return.)
>
> Along the way, this fixes comments, renames a variable, and consolidates
> dumpability and pdeath_signal clearing, which includes some commit log
> archeology to examine the subtle differences between what we had and
> what we need.
>
> I'd appreciate some extra eyes on this to make sure this isn't broken
> in some special way. Looking at the diffstat, even after all my long
> comments, this is a net reduction in lines. :)
>
> Given this crosses a bunch of areas, I think this is likely best to
> go via the -mm tree, which is where nearly all of my prior exec work
> has lived too.
>
> Thanks!
>
> -Kees
> ----------------------------------------------------------------
> Kees Cook (15):
> binfmt: Introduce secureexec flag
> exec: Rename bprm->cred_prepared to called_set_creds
> apparmor: Refactor to remove bprm_secureexec hook
> selinux: Refactor to remove bprm_secureexec hook
> smack: Refactor to remove bprm_secureexec hook
> commoncap: Refactor to remove bprm_secureexec hook
> commoncap: Move cap_elevated calculation into bprm_set_creds
> LSM: drop bprm_secureexec hook
> exec: Correct comments about "point of no return"
> exec: Use secureexec for setting dumpability
> exec: Use secureexec for clearing pdeath_signal
> smack: Remove redundant pdeath_signal clearing
> exec: Consolidate dumpability logic
> exec: Use sane stack rlimit under secureexec
> exec: Consolidate pdeath_signal clearing
Thanks, the set looks good to me,
Acked-by: Serge Hallyn <[email protected]>
Have you had a chance to run the ltp caps tests against this?
-serge
On Tue, Jul 18, 2017 at 6:06 PM, Andy Lutomirski <[email protected]> wrote:
> On Tue, Jul 18, 2017 at 3:25 PM, Kees Cook <[email protected]> wrote:
>> The cred_prepared bprm flag has a misleading name. It has nothing to do
>> with the bprm_prepare_cred hook, and actually tracks if bprm_set_creds has
>> been called. Rename this flag and improve its comment.
>>
>> Cc: David Howells <[email protected]>
>> Cc: John Johansen <[email protected]>
>> Cc: Paul Moore <[email protected]>
>> Cc: Stephen Smalley <[email protected]>
>> Cc: Casey Schaufler <[email protected]>
>> Cc: James Morris <[email protected]>
>> Signed-off-by: Kees Cook <[email protected]>
>> ---
>> fs/binfmt_flat.c | 2 +-
>> fs/exec.c | 2 +-
>> include/linux/binfmts.h | 8 ++++++--
>> security/apparmor/domain.c | 2 +-
>> security/selinux/hooks.c | 2 +-
>> security/smack/smack_lsm.c | 2 +-
>> security/tomoyo/tomoyo.c | 2 +-
>> 7 files changed, 12 insertions(+), 8 deletions(-)
>>
>> diff --git a/fs/binfmt_flat.c b/fs/binfmt_flat.c
>> index 2edcefc0a294..a722530cc468 100644
>> --- a/fs/binfmt_flat.c
>> +++ b/fs/binfmt_flat.c
>> @@ -885,7 +885,7 @@ static int load_flat_shared_library(int id, struct lib_info *libs)
>> * as we're past the point of no return and are dealing with shared
>> * libraries.
>> */
>> - bprm.cred_prepared = 1;
>> + bprm.called_set_creds = 1;
>
> WTF is this? It's not, strictly speaking, a bug in this patch, but
> it's nonsensical. Is it fixed (presuably deleted) later?
binfmt_flat looks crazy, but I haven't seen any distros that enable it.
> Otherwise looks good.
Thanks!
-Kees
--
Kees Cook
Pixel Security
On Tue, Jul 18, 2017 at 6:10 PM, Andy Lutomirski <[email protected]> wrote:
> On Tue, Jul 18, 2017 at 3:25 PM, Kees Cook <[email protected]> wrote:
>> 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.
>
> Reviewed-by: Andy Lutomirski <[email protected]>
>
> with the redundant caveat that...
>
>> --- a/fs/exec.c
>> +++ b/fs/exec.c
>> @@ -1330,6 +1330,13 @@ EXPORT_SYMBOL(would_dump);
>>
>> void setup_new_exec(struct linux_binprm * 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;
>> +
>
> ...the weird placement of the other assignments to bprm->secureexec
> makes this exceedingly confusing.
Any thoughts on how I could improve this? The main take-away is that
commoncap's secureexec is special, and this was the cleanest way I
could find to deal with it...
-Kees
--
Kees Cook
Pixel Security
On Tue, Jul 18, 2017 at 8:22 PM, Serge E. Hallyn <[email protected]> wrote:
> On Tue, Jul 18, 2017 at 03:25:21PM -0700, Kees Cook wrote:
>> This series has grown... :P
>>
>> As discussed with Linus and Andy, we need to reset the stack rlimit
>> before we do memory layouts when execing a privilege-gaining (e.g.
>> setuid) program. To do this, we need to know the results of the
>> bprm_secureexec hook before memory layouts. As it turns out, this
>> can be made _mostly_ trivial by collapsing bprm_secureexec into
>> bprm_set_creds.
>>
>> The LSMs using bprm_secureexec nearly always save state between
>> bprm_set_creds and bprm_secureexec. In the face of multiple calls to
>> bprm_set_creds (via prepare_binprm() calls from binfmt_script, etc),
>> all LSMs except commoncap only pay attention to the first call, so
>> that aligns well with collapsing bprm_secureexec into bprm_set_creds.
>> The commoncaps, though, needs to check the _last_ bprm_set_creds, so
>> this series just swaps one bprm flag for another (cap_effective is no
>> longer needed to save state between bprm_set_creds and bprm_secureexec,
>> but we do need to keep a separate state, so we add the cap_elevated flag).
>>
>> Once secureexec is available to setup_new_exec() before the memory
>> layout, we can add an rlimit sanity-check for setuid execs. (With no
>> need to clean up since we're past the point of no return.)
>>
>> Along the way, this fixes comments, renames a variable, and consolidates
>> dumpability and pdeath_signal clearing, which includes some commit log
>> archeology to examine the subtle differences between what we had and
>> what we need.
>>
>> I'd appreciate some extra eyes on this to make sure this isn't broken
>> in some special way. Looking at the diffstat, even after all my long
>> comments, this is a net reduction in lines. :)
>>
>> Given this crosses a bunch of areas, I think this is likely best to
>> go via the -mm tree, which is where nearly all of my prior exec work
>> has lived too.
>>
>> Thanks!
>>
>> -Kees
>> ----------------------------------------------------------------
>> Kees Cook (15):
>> binfmt: Introduce secureexec flag
>> exec: Rename bprm->cred_prepared to called_set_creds
>> apparmor: Refactor to remove bprm_secureexec hook
>> selinux: Refactor to remove bprm_secureexec hook
>> smack: Refactor to remove bprm_secureexec hook
>> commoncap: Refactor to remove bprm_secureexec hook
>> commoncap: Move cap_elevated calculation into bprm_set_creds
>> LSM: drop bprm_secureexec hook
>> exec: Correct comments about "point of no return"
>> exec: Use secureexec for setting dumpability
>> exec: Use secureexec for clearing pdeath_signal
>> smack: Remove redundant pdeath_signal clearing
>> exec: Consolidate dumpability logic
>> exec: Use sane stack rlimit under secureexec
>> exec: Consolidate pdeath_signal clearing
>
> Thanks, the set looks good to me,
Thanks!
> Acked-by: Serge Hallyn <[email protected]>
>
> Have you had a chance to run the ltp caps tests against this?
The LTP caps tests I could find are these:
sudo ./runltp -f syscalls -s cap
sudo ./runltp -f securebits
sudo ./runltp -f cap_bounds
sudo ./runltp -f filecaps
They all run successfully. Was there other stuff from LTP?
And, FWIW, the kernel selftests for capabilities and exec continue to pass too.
-Kees
--
Kees Cook
Pixel Security
On Tue, 18 Jul 2017, Kees Cook wrote:
> The cred_prepared bprm flag has a misleading name. It has nothing to do
> with the bprm_prepare_cred hook, and actually tracks if bprm_set_creds has
> been called. Rename this flag and improve its comment.
>
> Cc: David Howells <[email protected]>
> Cc: John Johansen <[email protected]>
> Cc: Paul Moore <[email protected]>
> Cc: Stephen Smalley <[email protected]>
> Cc: Casey Schaufler <[email protected]>
> Cc: James Morris <[email protected]>
> Signed-off-by: Kees Cook <[email protected]>
Acked-by: James Morris <[email protected]>
--
James Morris
<[email protected]>
On Tue, 18 Jul 2017, Kees Cook wrote:
> The AppArmor bprm_secureexec hook can be merged with the bprm_set_creds
> hook since it's dealing with the same information, and all of the details
> are finalized during the first call to the bprm_set_creds hook via
> prepare_binprm() (subsequent calls due to binfmt_script, etc, are ignored
> via bprm->called_set_creds).
>
> Here, all the comments describe how secureexec is actually calculated
> during bprm_set_creds, so this actually does it, drops the bprm flag that
> was being used internally by AppArmor, and drops the bprm_secureexec hook.
>
> Cc: John Johansen <[email protected]>
> Signed-off-by: Kees Cook <[email protected]>
Reviewed-by: James Morris <[email protected]>
--
James Morris
<[email protected]>
On Tue, 18 Jul 2017, Kees Cook wrote:
> 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: Serge Hallyn <[email protected]>
> Cc: Andy Lutomirski <[email protected]>
> Signed-off-by: Kees Cook <[email protected]>
Acked-by: James Morris <[email protected]>
--
James Morris
<[email protected]>
On Tue, 18 Jul 2017, Kees Cook wrote:
> Instead of a separate function, open-code the cap_elevated test, which
> lets us entirely remove bprm->cap_effective (to use the local "effective"
> variable instead), and more accurately examine euid/egid changes via the
> existing local "is_setid".
>
> Cc: Serge Hallyn <[email protected]>
> Cc: Andy Lutomirski <[email protected]>
> Signed-off-by: Kees Cook <[email protected]>
Reviewed-by: James Morris <[email protected]>
--
James Morris
<[email protected]>
On Tue, 18 Jul 2017, Kees Cook wrote:
> This removes the bprm_secureexec hook since the logic has been folded into
> the bprm_set_creds hook for all LSMs now.
>
> Cc: James Morris <[email protected]>
> Cc: Eric W. Biederman <[email protected]>
> Signed-off-by: Kees Cook <[email protected]>
> ---
> fs/binfmt_elf.c | 1 -
> fs/binfmt_elf_fdpic.c | 1 -
> include/linux/lsm_hooks.h | 14 +++++---------
> include/linux/security.h | 7 -------
> security/security.c | 5 -----
> 5 files changed, 5 insertions(+), 23 deletions(-)
Acked-by: James Morris <[email protected]>
--
James Morris
<[email protected]>
On Tue, 18 Jul 2017, Kees Cook wrote:
> For a secureexec, before memory layout selection has happened, reset the
> stack rlimit to something sane to avoid the caller having control over
> the resulting layouts.
>
> $ ulimit -s
> 8192
> $ ulimit -s unlimited
> $ /bin/sh -c 'ulimit -s'
> unlimited
> $ sudo /bin/sh -c 'ulimit -s'
> 8192
>
> Cc: Linus Torvalds <[email protected]>
> Signed-off-by: Kees Cook <[email protected]>
Reviewed-by: James Morris <[email protected]>
--
James Morris
<[email protected]>
On Tue, Jul 18, 2017 at 6:25 PM, Kees Cook <[email protected]> wrote:
> The cred_prepared bprm flag has a misleading name. It has nothing to do
> with the bprm_prepare_cred hook, and actually tracks if bprm_set_creds has
> been called. Rename this flag and improve its comment.
>
> Cc: David Howells <[email protected]>
> Cc: John Johansen <[email protected]>
> Cc: Paul Moore <[email protected]>
> Cc: Stephen Smalley <[email protected]>
> Cc: Casey Schaufler <[email protected]>
> Cc: James Morris <[email protected]>
> Signed-off-by: Kees Cook <[email protected]>
> ---
> fs/binfmt_flat.c | 2 +-
> fs/exec.c | 2 +-
> include/linux/binfmts.h | 8 ++++++--
> security/apparmor/domain.c | 2 +-
> security/selinux/hooks.c | 2 +-
> security/smack/smack_lsm.c | 2 +-
> security/tomoyo/tomoyo.c | 2 +-
> 7 files changed, 12 insertions(+), 8 deletions(-)
Acked-by: Paul Moore <[email protected]>
--
paul moore
http://www.paul-moore.com
On Tue, Jul 18, 2017 at 6:25 PM, Kees Cook <[email protected]> wrote:
> The SELinux bprm_secureexec hook can be merged with the bprm_set_creds
> hook since it's dealing with the same information, and all of the details
> are finalized during the first call to the bprm_set_creds hook via
> prepare_binprm() (subsequent calls due to binfmt_script, etc, are ignored
> via bprm->called_set_creds).
>
> Here, the test can just happen at the end of the bprm_set_creds hook,
> and the bprm_secureexec hook can be dropped.
>
> Cc: Paul Moore <[email protected]>
> Cc: Stephen Smalley <[email protected]>
> Signed-off-by: Kees Cook <[email protected]>
> ---
> security/selinux/hooks.c | 24 +++++-------------------
> 1 file changed, 5 insertions(+), 19 deletions(-)
This seems reasonable in the context of the other changes.
Stephen just posted an AT_SECURE test for the selinux-testsuite on the
SELinux mailing list, it would be nice to ensure that this patchset
doesn't run afoul of that.
Acked-by: Paul Moore <[email protected]>
> diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c
> index 0f1450a06b02..18038f73a2f7 100644
> --- a/security/selinux/hooks.c
> +++ b/security/selinux/hooks.c
> @@ -2413,30 +2413,17 @@ static int selinux_bprm_set_creds(struct linux_binprm *bprm)
>
> /* Clear any possibly unsafe personality bits on exec: */
> bprm->per_clear |= PER_CLEAR_ON_SETID;
> - }
> -
> - return 0;
> -}
> -
> -static int selinux_bprm_secureexec(struct linux_binprm *bprm)
> -{
> - const struct task_security_struct *tsec = current_security();
> - u32 sid, osid;
> - int atsecure = 0;
> -
> - sid = tsec->sid;
> - osid = tsec->osid;
>
> - if (osid != sid) {
> /* Enable secure mode for SIDs transitions unless
> the noatsecure permission is granted between
> the two SIDs, i.e. ahp returns 0. */
> - atsecure = avc_has_perm(osid, sid,
> - SECCLASS_PROCESS,
> - PROCESS__NOATSECURE, NULL);
> + rc = avc_has_perm(old_tsec->sid, new_tsec->sid,
> + SECCLASS_PROCESS, PROCESS__NOATSECURE,
> + NULL);
> + bprm->secureexec |= !!rc;
> }
>
> - return !!atsecure;
> + return 0;
> }
>
> static int match_file(const void *p, struct file *file, unsigned fd)
> @@ -6151,7 +6138,6 @@ static struct security_hook_list selinux_hooks[] __lsm_ro_after_init = {
> LSM_HOOK_INIT(bprm_set_creds, selinux_bprm_set_creds),
> LSM_HOOK_INIT(bprm_committing_creds, selinux_bprm_committing_creds),
> LSM_HOOK_INIT(bprm_committed_creds, selinux_bprm_committed_creds),
> - LSM_HOOK_INIT(bprm_secureexec, selinux_bprm_secureexec),
>
> LSM_HOOK_INIT(sb_alloc_security, selinux_sb_alloc_security),
> LSM_HOOK_INIT(sb_free_security, selinux_sb_free_security),
> --
> 2.7.4
--
paul moore
http://www.paul-moore.com
On Wed, Jul 19, 2017 at 8:03 PM, Paul Moore <[email protected]> wrote:
> On Tue, Jul 18, 2017 at 6:25 PM, Kees Cook <[email protected]> wrote:
>> The SELinux bprm_secureexec hook can be merged with the bprm_set_creds
>> hook since it's dealing with the same information, and all of the details
>> are finalized during the first call to the bprm_set_creds hook via
>> prepare_binprm() (subsequent calls due to binfmt_script, etc, are ignored
>> via bprm->called_set_creds).
>>
>> Here, the test can just happen at the end of the bprm_set_creds hook,
>> and the bprm_secureexec hook can be dropped.
>>
>> Cc: Paul Moore <[email protected]>
>> Cc: Stephen Smalley <[email protected]>
>> Signed-off-by: Kees Cook <[email protected]>
>> ---
>> security/selinux/hooks.c | 24 +++++-------------------
>> 1 file changed, 5 insertions(+), 19 deletions(-)
>
> This seems reasonable in the context of the other changes.
>
> Stephen just posted an AT_SECURE test for the selinux-testsuite on the
> SELinux mailing list, it would be nice to ensure that this patchset
> doesn't run afoul of that.
Quick follow-up: I just merged Stephen's test into the test suite:
* https://github.com/SELinuxProject/selinux-testsuite
> Acked-by: Paul Moore <[email protected]>
>
>> diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c
>> index 0f1450a06b02..18038f73a2f7 100644
>> --- a/security/selinux/hooks.c
>> +++ b/security/selinux/hooks.c
>> @@ -2413,30 +2413,17 @@ static int selinux_bprm_set_creds(struct linux_binprm *bprm)
>>
>> /* Clear any possibly unsafe personality bits on exec: */
>> bprm->per_clear |= PER_CLEAR_ON_SETID;
>> - }
>> -
>> - return 0;
>> -}
>> -
>> -static int selinux_bprm_secureexec(struct linux_binprm *bprm)
>> -{
>> - const struct task_security_struct *tsec = current_security();
>> - u32 sid, osid;
>> - int atsecure = 0;
>> -
>> - sid = tsec->sid;
>> - osid = tsec->osid;
>>
>> - if (osid != sid) {
>> /* Enable secure mode for SIDs transitions unless
>> the noatsecure permission is granted between
>> the two SIDs, i.e. ahp returns 0. */
>> - atsecure = avc_has_perm(osid, sid,
>> - SECCLASS_PROCESS,
>> - PROCESS__NOATSECURE, NULL);
>> + rc = avc_has_perm(old_tsec->sid, new_tsec->sid,
>> + SECCLASS_PROCESS, PROCESS__NOATSECURE,
>> + NULL);
>> + bprm->secureexec |= !!rc;
>> }
>>
>> - return !!atsecure;
>> + return 0;
>> }
>>
>> static int match_file(const void *p, struct file *file, unsigned fd)
>> @@ -6151,7 +6138,6 @@ static struct security_hook_list selinux_hooks[] __lsm_ro_after_init = {
>> LSM_HOOK_INIT(bprm_set_creds, selinux_bprm_set_creds),
>> LSM_HOOK_INIT(bprm_committing_creds, selinux_bprm_committing_creds),
>> LSM_HOOK_INIT(bprm_committed_creds, selinux_bprm_committed_creds),
>> - LSM_HOOK_INIT(bprm_secureexec, selinux_bprm_secureexec),
>>
>> LSM_HOOK_INIT(sb_alloc_security, selinux_sb_alloc_security),
>> LSM_HOOK_INIT(sb_free_security, selinux_sb_free_security),
>> --
>> 2.7.4
>
> --
> paul moore
> http://www.paul-moore.com
--
paul moore
http://www.paul-moore.com
On Wed, Jul 19, 2017 at 5:19 PM, Paul Moore <[email protected]> wrote:
> On Wed, Jul 19, 2017 at 8:03 PM, Paul Moore <[email protected]> wrote:
>> On Tue, Jul 18, 2017 at 6:25 PM, Kees Cook <[email protected]> wrote:
>>> The SELinux bprm_secureexec hook can be merged with the bprm_set_creds
>>> hook since it's dealing with the same information, and all of the details
>>> are finalized during the first call to the bprm_set_creds hook via
>>> prepare_binprm() (subsequent calls due to binfmt_script, etc, are ignored
>>> via bprm->called_set_creds).
>>>
>>> Here, the test can just happen at the end of the bprm_set_creds hook,
>>> and the bprm_secureexec hook can be dropped.
>>>
>>> Cc: Paul Moore <[email protected]>
>>> Cc: Stephen Smalley <[email protected]>
>>> Signed-off-by: Kees Cook <[email protected]>
>>> ---
>>> security/selinux/hooks.c | 24 +++++-------------------
>>> 1 file changed, 5 insertions(+), 19 deletions(-)
>>
>> This seems reasonable in the context of the other changes.
>>
>> Stephen just posted an AT_SECURE test for the selinux-testsuite on the
>> SELinux mailing list, it would be nice to ensure that this patchset
>> doesn't run afoul of that.
>
> Quick follow-up: I just merged Stephen's test into the test suite:
>
> * https://github.com/SELinuxProject/selinux-testsuite
Is there a quick how-to on just running the AT_SECURE test?
-Kees
--
Kees Cook
Pixel Security
On Tue, Jul 18, 2017 at 6:10 PM, Andy Lutomirski <[email protected]> wrote:
> On Tue, Jul 18, 2017 at 3:25 PM, Kees Cook <[email protected]> wrote:
>> 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.
>
> Reviewed-by: Andy Lutomirski <[email protected]>
>
> with the redundant caveat that...
>
>> --- a/fs/exec.c
>> +++ b/fs/exec.c
>> @@ -1330,6 +1330,13 @@ EXPORT_SYMBOL(would_dump);
>>
>> void setup_new_exec(struct linux_binprm * 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;
>> +
>
> ...the weird placement of the other assignments to bprm->secureexec
> makes this exceedingly confusing.
Can you just put the bprm->secureexec |=
security_bprm_secureexec(bprm); assignment in prepare_binprm() right
after security_bprm_set_creds()? This would make patch 1 make sense
and make this make sense too, I think. Or is there some reason why it
wouldn't work? If the latter, I think the patch descriptions and
comments should maybe be fixed up.
On Wed, Jul 19, 2017 at 9:37 PM, Kees Cook <[email protected]> wrote:
> On Wed, Jul 19, 2017 at 5:19 PM, Paul Moore <[email protected]> wrote:
>> On Wed, Jul 19, 2017 at 8:03 PM, Paul Moore <[email protected]> wrote:
>>> On Tue, Jul 18, 2017 at 6:25 PM, Kees Cook <[email protected]> wrote:
>>>> The SELinux bprm_secureexec hook can be merged with the bprm_set_creds
>>>> hook since it's dealing with the same information, and all of the details
>>>> are finalized during the first call to the bprm_set_creds hook via
>>>> prepare_binprm() (subsequent calls due to binfmt_script, etc, are ignored
>>>> via bprm->called_set_creds).
>>>>
>>>> Here, the test can just happen at the end of the bprm_set_creds hook,
>>>> and the bprm_secureexec hook can be dropped.
>>>>
>>>> Cc: Paul Moore <[email protected]>
>>>> Cc: Stephen Smalley <[email protected]>
>>>> Signed-off-by: Kees Cook <[email protected]>
>>>> ---
>>>> security/selinux/hooks.c | 24 +++++-------------------
>>>> 1 file changed, 5 insertions(+), 19 deletions(-)
>>>
>>> This seems reasonable in the context of the other changes.
>>>
>>> Stephen just posted an AT_SECURE test for the selinux-testsuite on the
>>> SELinux mailing list, it would be nice to ensure that this patchset
>>> doesn't run afoul of that.
>>
>> Quick follow-up: I just merged Stephen's test into the test suite:
>>
>> * https://github.com/SELinuxProject/selinux-testsuite
>
> Is there a quick how-to on just running the AT_SECURE test?
You'll need a functional SELinux system to start, I run it against
Fedora Rawhide regularly* with various development kernels, but recent
stable Fedora releases should work too. Occasionally I hear of people
running it on Debian, but I haven't had a Debian SELinux system in
some time so I can't say for certain everything is 100% working there.
Once you've gotten a working system in enforcing mode, read the README
file in the test suite to install the necessary dependencies (look in
the "Userland and Base Policy" section), then build the tests/policy
(you should be able to skip this step, as the make dependencies will
handle it, but it is nice to do it separately to make sure you have
the build dependencies sorted):
# make
... load the test policy
# make -C policy load
... run the tests:
# cd tests/atsecure
# ./test
... optionally uninstall the test policy:
# make -C policy unload
In some ways it is easier to just run the entire test suite:
# make
# make test
Alternatively, if you've got a fairly recent git repo with all the
patches merged I can build a test kernel and give it a shot for you,
although fair warning it may take a day or two for me to get to it.
* It is worth noting that the current 4.13-rcX releases have two bugs
that affect the selinux-testsuite. The worst is a kernel panic due to
a bug in overlayfs' xattr code, there is a patch available to fix it,
but as of yesterday it hadn't yet hit Linus tree (I can dig it up if
you need it). The second issue related to IPsec and getting peer
label information over UDP connections, I haven't had a chance to sort
that out yet, but at least it isn't a kernel panic.
--
paul moore
http://www.paul-moore.com
On Thu, Jul 20, 2017 at 6:42 AM, Paul Moore <[email protected]> wrote:
> Alternatively, if you've got a fairly recent git repo with all the
> patches merged I can build a test kernel and give it a shot for you,
> although fair warning it may take a day or two for me to get to it.
Hurm, I think this will take quite a bit of time for me to set up. :P
If you have a chance, I'd appreciate it if you could test the series.
It's currently based on v4.12:
https://git.kernel.org/pub/scm/linux/kernel/git/kees/linux.git/log/?h=kspp/setuid-rlimits/secureexec-no-hook
If it doesn't work out or takes too much time I can work on setting up
the test environment next week (travelling at the moment).
Thanks for the details!
-Kees
--
Kees Cook
Pixel Security
On Thu, Jul 20, 2017 at 1:06 PM, Kees Cook <[email protected]> wrote:
> On Thu, Jul 20, 2017 at 6:42 AM, Paul Moore <[email protected]> wrote:
>> Alternatively, if you've got a fairly recent git repo with all the
>> patches merged I can build a test kernel and give it a shot for you,
>> although fair warning it may take a day or two for me to get to it.
>
> Hurm, I think this will take quite a bit of time for me to set up. :P
> If you have a chance, I'd appreciate it if you could test the series.
> It's currently based on v4.12:
> https://git.kernel.org/pub/scm/linux/kernel/git/kees/linux.git/log/?h=kspp/setuid-rlimits/secureexec-no-hook
>
> If it doesn't work out or takes too much time I can work on setting up
> the test environment next week (travelling at the moment).
Building a kernel now, in case anyone on Fedora wants to play with it,
you can find it here (when it finishes):
* https://copr.fedorainfracloud.org/coprs/pcmoore/kernel-testing/build/581947
--
paul moore
http://www.paul-moore.com
On Thu, Jul 20, 2017 at 4:42 PM, Paul Moore <[email protected]> wrote:
> On Thu, Jul 20, 2017 at 1:06 PM, Kees Cook <[email protected]> wrote:
>> On Thu, Jul 20, 2017 at 6:42 AM, Paul Moore <[email protected]> wrote:
>>> Alternatively, if you've got a fairly recent git repo with all the
>>> patches merged I can build a test kernel and give it a shot for you,
>>> although fair warning it may take a day or two for me to get to it.
>>
>> Hurm, I think this will take quite a bit of time for me to set up. :P
>> If you have a chance, I'd appreciate it if you could test the series.
>> It's currently based on v4.12:
>> https://git.kernel.org/pub/scm/linux/kernel/git/kees/linux.git/log/?h=kspp/setuid-rlimits/secureexec-no-hook
>>
>> If it doesn't work out or takes too much time I can work on setting up
>> the test environment next week (travelling at the moment).
>
> Building a kernel now, in case anyone on Fedora wants to play with it,
> you can find it here (when it finishes):
>
> * https://copr.fedorainfracloud.org/coprs/pcmoore/kernel-testing/build/581947
Quick follow up, the kernel above passes the selinux-testsuite atsecure test.
--
paul moore
http://www.paul-moore.com
On Fri, Jul 21, 2017 at 8:40 AM, Paul Moore <[email protected]> wrote:
> On Thu, Jul 20, 2017 at 4:42 PM, Paul Moore <[email protected]> wrote:
>> On Thu, Jul 20, 2017 at 1:06 PM, Kees Cook <[email protected]> wrote:
>>> On Thu, Jul 20, 2017 at 6:42 AM, Paul Moore <[email protected]> wrote:
>>>> Alternatively, if you've got a fairly recent git repo with all the
>>>> patches merged I can build a test kernel and give it a shot for you,
>>>> although fair warning it may take a day or two for me to get to it.
>>>
>>> Hurm, I think this will take quite a bit of time for me to set up. :P
>>> If you have a chance, I'd appreciate it if you could test the series.
>>> It's currently based on v4.12:
>>> https://git.kernel.org/pub/scm/linux/kernel/git/kees/linux.git/log/?h=kspp/setuid-rlimits/secureexec-no-hook
>>>
>>> If it doesn't work out or takes too much time I can work on setting up
>>> the test environment next week (travelling at the moment).
>>
>> Building a kernel now, in case anyone on Fedora wants to play with it,
>> you can find it here (when it finishes):
>>
>> * https://copr.fedorainfracloud.org/coprs/pcmoore/kernel-testing/build/581947
>
> Quick follow up, the kernel above passes the selinux-testsuite atsecure test.
Awesome, thanks for taking the time to test it. :) Can I add your Tested-by?
-Kees
--
Kees Cook
Pixel Security
On Fri, Jul 21, 2017 at 1:37 PM, Kees Cook <[email protected]> wrote:
> On Fri, Jul 21, 2017 at 8:40 AM, Paul Moore <[email protected]> wrote:
>> On Thu, Jul 20, 2017 at 4:42 PM, Paul Moore <[email protected]> wrote:
>>> On Thu, Jul 20, 2017 at 1:06 PM, Kees Cook <[email protected]> wrote:
>>>> On Thu, Jul 20, 2017 at 6:42 AM, Paul Moore <[email protected]> wrote:
>>>>> Alternatively, if you've got a fairly recent git repo with all the
>>>>> patches merged I can build a test kernel and give it a shot for you,
>>>>> although fair warning it may take a day or two for me to get to it.
>>>>
>>>> Hurm, I think this will take quite a bit of time for me to set up. :P
>>>> If you have a chance, I'd appreciate it if you could test the series.
>>>> It's currently based on v4.12:
>>>> https://git.kernel.org/pub/scm/linux/kernel/git/kees/linux.git/log/?h=kspp/setuid-rlimits/secureexec-no-hook
>>>>
>>>> If it doesn't work out or takes too much time I can work on setting up
>>>> the test environment next week (travelling at the moment).
>>>
>>> Building a kernel now, in case anyone on Fedora wants to play with it,
>>> you can find it here (when it finishes):
>>>
>>> * https://copr.fedorainfracloud.org/coprs/pcmoore/kernel-testing/build/581947
>>
>> Quick follow up, the kernel above passes the selinux-testsuite atsecure test.
>
> Awesome, thanks for taking the time to test it. :) Can I add your Tested-by?
Sorry, I should have included that, here ya go:
Tested-by: Paul Moore <[email protected]>
--
paul moore
http://www.paul-moore.com
On Tue, Jul 18, 2017 at 3:25 PM, Kees Cook <[email protected]> wrote:
> The Smack bprm_secureexec hook can be merged with the bprm_set_creds
> hook since it's dealing with the same information, and all of the details
> are finalized during the first call to the bprm_set_creds hook via
> prepare_binprm() (subsequent calls due to binfmt_script, etc, are ignored
> via bprm->called_set_creds).
>
> Here, the test can just happen at the end of the bprm_set_creds hook,
> and the bprm_secureexec hook can be dropped.
>
> Cc: Casey Schaufler <[email protected]>
> Signed-off-by: Kees Cook <[email protected]>
How does this look to you, Casey? I've only got a few unreviewed
patches in this series. Two touch Smack. :)
Thanks!
-Kees
> ---
> security/smack/smack_lsm.c | 21 ++++-----------------
> 1 file changed, 4 insertions(+), 17 deletions(-)
>
> diff --git a/security/smack/smack_lsm.c b/security/smack/smack_lsm.c
> index 7d4b2e221124..4f1967be3d20 100644
> --- a/security/smack/smack_lsm.c
> +++ b/security/smack/smack_lsm.c
> @@ -950,6 +950,10 @@ static int smack_bprm_set_creds(struct linux_binprm *bprm)
> bsp->smk_task = isp->smk_task;
> bprm->per_clear |= PER_CLEAR_ON_SETID;
>
> + /* Decide if this is a secure exec. */
> + if (bsp->smk_task != bsp->smk_forked)
> + bprm->secureexec = 1;
> +
> return 0;
> }
>
> @@ -967,22 +971,6 @@ static void smack_bprm_committing_creds(struct linux_binprm *bprm)
> current->pdeath_signal = 0;
> }
>
> -/**
> - * smack_bprm_secureexec - Return the decision to use secureexec.
> - * @bprm: binprm for exec
> - *
> - * Returns 0 on success.
> - */
> -static int smack_bprm_secureexec(struct linux_binprm *bprm)
> -{
> - struct task_smack *tsp = current_security();
> -
> - if (tsp->smk_task != tsp->smk_forked)
> - return 1;
> -
> - return 0;
> -}
> -
> /*
> * Inode hooks
> */
> @@ -4646,7 +4634,6 @@ static struct security_hook_list smack_hooks[] __lsm_ro_after_init = {
>
> LSM_HOOK_INIT(bprm_set_creds, smack_bprm_set_creds),
> LSM_HOOK_INIT(bprm_committing_creds, smack_bprm_committing_creds),
> - LSM_HOOK_INIT(bprm_secureexec, smack_bprm_secureexec),
>
> LSM_HOOK_INIT(inode_alloc_security, smack_inode_alloc_security),
> LSM_HOOK_INIT(inode_free_security, smack_inode_free_security),
> --
> 2.7.4
>
--
Kees Cook
Pixel Security
On Tue, Jul 18, 2017 at 3:25 PM, Kees Cook <[email protected]> wrote:
> The examination of "current" to decide dumpability is wrong. This was a
> check of and euid/uid (or egid/gid) mismatch in the existing process,
> not the newly created one. This appears to stretch back into even the
> "history.git" tree. Luckily, dumpability is later set in commit_creds().
> In earlier kernel versions before creds existed, similar checks also
> existed late in the exec flow, covering up the mistake as far back as I
> could find.
>
> Note that because the commit_creds() check examines differences of euid,
> uid, egid, gid, and capabilities between the old and new creds, it would
> look like the setup_new_exec() dumpability test could be entirely removed.
> However, the secureexec test may cover a different set of tests (specific
> to the LSMs) than what commit_creds() checks for. So, fix this test to
> use secureexec (the removed euid tests are redundant to the commoncap
> secureexec checks now).
>
> Cc: David Howells <[email protected]>
> Signed-off-by: Kees Cook <[email protected]>
David (or anyone else), how does this (and the following undiscussed
patches) look? I only have a few unreviewed patches in this series,
and I'd like to get some more eyes on it.
Thanks!
-Kees
> ---
> fs/exec.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/fs/exec.c b/fs/exec.c
> index f9480d3e0b82..5241c8f25f5d 100644
> --- a/fs/exec.c
> +++ b/fs/exec.c
> @@ -1353,7 +1353,7 @@ void setup_new_exec(struct linux_binprm * bprm)
>
> current->sas_ss_sp = current->sas_ss_size = 0;
>
> - if (uid_eq(current_euid(), current_uid()) && gid_eq(current_egid(), current_gid()))
> + if (!bprm->secureexec)
> set_dumpable(current->mm, SUID_DUMP_USER);
> else
> set_dumpable(current->mm, suid_dumpable);
> --
> 2.7.4
>
--
Kees Cook
Pixel Security
On 7/25/2017 8:58 PM, Kees Cook wrote:
> On Tue, Jul 18, 2017 at 3:25 PM, Kees Cook <[email protected]> wrote:
>> The Smack bprm_secureexec hook can be merged with the bprm_set_creds
>> hook since it's dealing with the same information, and all of the details
>> are finalized during the first call to the bprm_set_creds hook via
>> prepare_binprm() (subsequent calls due to binfmt_script, etc, are ignored
>> via bprm->called_set_creds).
>>
>> Here, the test can just happen at the end of the bprm_set_creds hook,
>> and the bprm_secureexec hook can be dropped.
>>
>> Cc: Casey Schaufler <[email protected]>
>> Signed-off-by: Kees Cook <[email protected]>
> How does this look to you, Casey? I've only got a few unreviewed
> patches in this series. Two touch Smack. :)
The eyes don't see any problems, but I haven't had a chance
to try it out.
>
> Thanks!
>
> -Kees
>
>> ---
>> security/smack/smack_lsm.c | 21 ++++-----------------
>> 1 file changed, 4 insertions(+), 17 deletions(-)
>>
>> diff --git a/security/smack/smack_lsm.c b/security/smack/smack_lsm.c
>> index 7d4b2e221124..4f1967be3d20 100644
>> --- a/security/smack/smack_lsm.c
>> +++ b/security/smack/smack_lsm.c
>> @@ -950,6 +950,10 @@ static int smack_bprm_set_creds(struct linux_binprm *bprm)
>> bsp->smk_task = isp->smk_task;
>> bprm->per_clear |= PER_CLEAR_ON_SETID;
>>
>> + /* Decide if this is a secure exec. */
>> + if (bsp->smk_task != bsp->smk_forked)
>> + bprm->secureexec = 1;
>> +
>> return 0;
>> }
>>
>> @@ -967,22 +971,6 @@ static void smack_bprm_committing_creds(struct linux_binprm *bprm)
>> current->pdeath_signal = 0;
>> }
>>
>> -/**
>> - * smack_bprm_secureexec - Return the decision to use secureexec.
>> - * @bprm: binprm for exec
>> - *
>> - * Returns 0 on success.
>> - */
>> -static int smack_bprm_secureexec(struct linux_binprm *bprm)
>> -{
>> - struct task_smack *tsp = current_security();
>> -
>> - if (tsp->smk_task != tsp->smk_forked)
>> - return 1;
>> -
>> - return 0;
>> -}
>> -
>> /*
>> * Inode hooks
>> */
>> @@ -4646,7 +4634,6 @@ static struct security_hook_list smack_hooks[] __lsm_ro_after_init = {
>>
>> LSM_HOOK_INIT(bprm_set_creds, smack_bprm_set_creds),
>> LSM_HOOK_INIT(bprm_committing_creds, smack_bprm_committing_creds),
>> - LSM_HOOK_INIT(bprm_secureexec, smack_bprm_secureexec),
>>
>> LSM_HOOK_INIT(inode_alloc_security, smack_inode_alloc_security),
>> LSM_HOOK_INIT(inode_free_security, smack_inode_free_security),
>> --
>> 2.7.4
>>
>
>
.
On Wed, Jul 19, 2017 at 9:53 PM, Andy Lutomirski <[email protected]> wrote:
> On Tue, Jul 18, 2017 at 6:10 PM, Andy Lutomirski <[email protected]> wrote:
>> On Tue, Jul 18, 2017 at 3:25 PM, Kees Cook <[email protected]> wrote:
>>> 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.
>>
>> Reviewed-by: Andy Lutomirski <[email protected]>
>>
>> with the redundant caveat that...
>>
>>> --- a/fs/exec.c
>>> +++ b/fs/exec.c
>>> @@ -1330,6 +1330,13 @@ EXPORT_SYMBOL(would_dump);
>>>
>>> void setup_new_exec(struct linux_binprm * 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;
>>> +
>>
>> ...the weird placement of the other assignments to bprm->secureexec
>> makes this exceedingly confusing.
>
> Can you just put the bprm->secureexec |=
> security_bprm_secureexec(bprm); assignment in prepare_binprm() right
> after security_bprm_set_creds()? This would make patch 1 make sense
> and make this make sense too, I think. Or is there some reason why it
> wouldn't work? If the latter, I think the patch descriptions and
> comments should maybe be fixed up.
Yeah, I'll make this change for the next version. It makes things a
little less ugly in the series. In this version I was trying to focus
on eliminating the LSM hook instead of first moving it (to
setup_new_exec()) and then moving it a second time (to the
bprm_set_creds() hook).
Have you had a chance to review the later consolidation patches? So
far no one else has reviewed those. (David, any chance you have some
time too?) I'd love to get at least some Reviewed-bys for them...
-Kees
--
Kees Cook
Pixel Security
On Mon, Jul 31, 2017 at 3:43 PM, Kees Cook <[email protected]> wrote:
> On Wed, Jul 19, 2017 at 9:53 PM, Andy Lutomirski <[email protected]> wrote:
>> On Tue, Jul 18, 2017 at 6:10 PM, Andy Lutomirski <[email protected]> wrote:
>>> On Tue, Jul 18, 2017 at 3:25 PM, Kees Cook <[email protected]> wrote:
>>>> 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.
>>>
>>> Reviewed-by: Andy Lutomirski <[email protected]>
>>>
>>> with the redundant caveat that...
>>>
>>>> --- a/fs/exec.c
>>>> +++ b/fs/exec.c
>>>> @@ -1330,6 +1330,13 @@ EXPORT_SYMBOL(would_dump);
>>>>
>>>> void setup_new_exec(struct linux_binprm * 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;
>>>> +
>>>
>>> ...the weird placement of the other assignments to bprm->secureexec
>>> makes this exceedingly confusing.
>>
>> Can you just put the bprm->secureexec |=
>> security_bprm_secureexec(bprm); assignment in prepare_binprm() right
>> after security_bprm_set_creds()? This would make patch 1 make sense
>> and make this make sense too, I think. Or is there some reason why it
>> wouldn't work? If the latter, I think the patch descriptions and
>> comments should maybe be fixed up.
>
> Yeah, I'll make this change for the next version. It makes things a
> little less ugly in the series. In this version I was trying to focus
> on eliminating the LSM hook instead of first moving it (to
> setup_new_exec()) and then moving it a second time (to the
> bprm_set_creds() hook).
>
> Have you had a chance to review the later consolidation patches? So
> far no one else has reviewed those. (David, any chance you have some
> time too?) I'd love to get at least some Reviewed-bys for them...
I looked briefly. I'll try to look more closely tomorrow.
--Andy