Based Linus's comments, my intent is to carry this series for -next and
send it as a distinct pull request during the v4.14 merge window. I'm
happy to adjust this plan as needed.
Changes from v4:
- fixed typo introduced during patch reordering
- rebased to v4.13-rc2 (mainly apparmor changes)
- fixed bisect error if building only to patch 7 and 8 under !CONFIG_SECURITY
- added more Acks/Reviews
Main cover letter contents:
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.
Several folks have looked at this already (thank you!) but I'd appreciate
any other eyes on this to make sure it isn't broken in some special
way. Looking at the diffstat, even after all my long comments, this is
a net reduction in lines. :)
Thanks!
-Kees
----------------------------------------------------------------
Kees Cook (15):
exec: Rename bprm->cred_prepared to called_set_creds
exec: Correct comments about "point of no return"
binfmt: Introduce secureexec flag
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: 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 | 21 ++------------
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(+), 159 deletions(-)
v5:
- fixed typo introduced during patch reordering
- rebased to v4.13-rc2 (mainly apparmor changes)
- fixed bisect error if building only to patch 7 and 8 under !CONFIG_SECURITY
- added more Acks/Reviews
v4:
- add {Ack,Review,Test}ed-bys
- reorder patches to move trivial refactoring to the front
- move secureexec flag set earlier in the series to setup_new_exec(); amluto
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
--
To unsubscribe from this list: send the line "unsubscribe linux-security-module" in
the body of a message to majordomo at vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
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 <dhowells@redhat.com>
Cc: Eric W. Biederman <redacted>
Signed-off-by: Kees Cook <redacted>
Acked-by: "Eric W. Biederman" <redacted>
Acked-by: Serge Hallyn <serge@hallyn.com>
---
fs/exec.c | 16 +++++++++++++---
1 file changed, 13 insertions(+), 3 deletions(-)
@@ -1286,7 +1292,13 @@ int flush_old_exec(struct linux_binprm * bprm)if(retval)gotoout;-bprm->mm=NULL;/* We're using it now */+/*+*Afterclearingbprm->mm(tomarkthatcurrentisusingthe+*preparedmmnow),wehavenothingleftoftheoriginal+*process.Ifanythingfromhereonreturnsanerror,thecheck+*insearch_binary_handler()willSEGVcurrent.+*/+bprm->mm=NULL;set_fs(USER_DS);current->flags&=~(PF_RANDOMIZE|PF_FORKNOEXEC|PF_KTHREAD|
@@ -1333,7 +1345,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()))
@@ -1351,7 +1362,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
--
To unsubscribe from this list: send the line "unsubscribe linux-security-module" in
the body of a message to majordomo at vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
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->called_set_creds
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, and calls the secureexec hook during setup_new_exec(). This is
safe since all the cred work is finished (and past the point of no return).
This explicit call will be removed in later patches once the hook has been
removed.
Cc: David Howells <dhowells@redhat.com>
Signed-off-by: Kees Cook <redacted>
Reviewed-by: John Johansen <john.johansen@canonical.com>
Acked-by: Serge Hallyn <serge@hallyn.com>
Reviewed-by: James Morris <redacted>
---
fs/binfmt_elf.c | 2 +-
fs/binfmt_elf_fdpic.c | 2 +-
fs/exec.c | 2 ++
include/linux/binfmts.h | 8 +++++++-
4 files changed, 11 insertions(+), 3 deletions(-)
@@ -31,9 +31,15 @@ struct linux_binprm {*binfmt_script/misc).*/called_set_creds:1,-cap_effective:1;/* true if has elevated effective capabilities,+cap_effective:1,/* true if has elevated effective capabilities,*falseifnot;exceptforinitwhichinherits*itsparent'scapsanyway*/+/*+*Setbybprm_set_credshooktoindicateaprivilege-gaining+*exechashappened.Usedtosanitizeexecutionenvironment+*andtosetAT_SECUREauxvforglibc.+*/+secureexec:1;#ifdef __alpha__unsignedinttaso:1;#endif
--
2.7.4
--
To unsubscribe from this list: send the line "unsubscribe linux-security-module" in
the body of a message to majordomo at vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
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 <dhowells@redhat.com>
Signed-off-by: Kees Cook <redacted>
Acked-by: Serge Hallyn <serge@hallyn.com>
Reviewed-by: James Morris <redacted>
---
fs/exec.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
--
2.7.4
--
To unsubscribe from this list: send the line "unsubscribe linux-security-module" in
the body of a message to majordomo at vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
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 <redacted>
Acked-by: Serge Hallyn <serge@hallyn.com>
Reviewed-by: James Morris <redacted>
---
fs/exec.c | 11 +++++------
1 file changed, 5 insertions(+), 6 deletions(-)
@@ -1371,9 +1373,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
--
To unsubscribe from this list: send the line "unsubscribe linux-security-module" in
the body of a message to majordomo at vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
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 <redacted>
Acked-by: Serge Hallyn <serge@hallyn.com>
---
fs/exec.c | 7 +++----
1 file changed, 3 insertions(+), 4 deletions(-)
@@ -1351,6 +1351,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;+/**Forsecureexec,resetthestacklimittosanedefaultto*avoidbadbehaviorfromthepriorrlimits.Thishasto
@@ -1383,10 +1386,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 threadgroup*/current->self_exec_id++;
--
2.7.4
--
To unsubscribe from this list: send the line "unsubscribe linux-security-module" in
the body of a message to majordomo at vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
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 <dhowells@redhat.com>
Cc: Stephen Smalley <redacted>
Cc: Casey Schaufler <casey@schaufler-ca.com>
Signed-off-by: Kees Cook <redacted>
Acked-by: John Johansen <john.johansen@canonical.com>
Acked-by: James Morris <redacted>
Acked-by: Paul Moore <paul@paul-moore.com>
Acked-by: Serge Hallyn <serge@hallyn.com>
---
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(-)
@@ -25,8 +25,12 @@ struct linux_binprm {structmm_struct*mm;unsignedlongp;/* current top of mem */unsignedint-cred_prepared:1,/* true if creds already prepared (multiple-*prepshappenforinterpreters)*/+/*+*Trueafterthebprm_set_credshookhasbeencalledonce+*(multiplecallscanbemadeviaprepare_binprm()for+*binfmt_script/misc).+*/+called_set_creds:1,cap_effective:1;/* true if has elevated effective capabilities,*falseifnot;exceptforinitwhichinherits*itsparent'scapsanyway*/
@@ -2356,7 +2356,7 @@ static int selinux_bprm_set_creds(struct linux_binprm *bprm)/* SELinux context only depends on initial program or script and not*thescriptinterpreter*/-if(bprm->cred_prepared)+if(bprm->called_set_creds)return0;old_tsec=current_security();
@@ -76,7 +76,7 @@ static int tomoyo_bprm_set_creds(struct linux_binprm *bprm)*Doonlyifthisfunctioniscalledforthefirsttimeofanexecve*operation.*/-if(bprm->cred_prepared)+if(bprm->called_set_creds)return0;#ifndef CONFIG_SECURITY_TOMOYO_OMIT_USERSPACE_LOADER/*
--
2.7.4
--
To unsubscribe from this list: send the line "unsubscribe linux-security-module" in
the body of a message to majordomo at vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
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.
Signed-off-by: Kees Cook <redacted>
Acked-by: John Johansen <john.johansen@canonical.com>
Reviewed-by: James Morris <redacted>
Acked-by: Serge Hallyn <serge@hallyn.com>
---
security/apparmor/domain.c | 19 +------------------
security/apparmor/include/domain.h | 1 -
security/apparmor/include/file.h | 3 ---
security/apparmor/lsm.c | 1 -
4 files changed, 1 insertion(+), 23 deletions(-)
@@ -807,7 +807,7 @@ int apparmor_bprm_set_creds(struct linux_binprm *bprm)aa_label_printk(new,GFP_ATOMIC);dbg_printk("\n");}-bprm->unsafe|=AA_SECURE_X_NEEDED;+bprm->secureexec=1;}if(label->proxy!=new->proxy){
@@ -843,23 +843,6 @@ int apparmor_bprm_set_creds(struct linux_binprm *bprm)gotodone;}-/**-*apparmor_bprm_secureexec-determineifsecureexecisneeded-*@bprm:binprmforexec(NOTNULL)-*-*Returns:%1ifsecureexecisneededelse%0-*/-intapparmor_bprm_secureexec(structlinux_binprm*bprm)-{-/* the decision to use secure exec is computed in set_creds-*andstoredinbprm->unsafe.-*/-if(bprm->unsafe&AA_SECURE_X_NEEDED)-return1;--return0;-}-/**Functionsforselfdirectedprofilechange*/
@@ -101,9 +101,6 @@ static inline struct aa_label *aa_get_file_label(struct aa_file_ctx *ctx)#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 */structpath_cond{kuid_tuid;
--
2.7.4
--
To unsubscribe from this list: send the line "unsubscribe linux-security-module" in
the body of a message to majordomo at vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
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: Stephen Smalley <redacted>
Signed-off-by: Kees Cook <redacted>
Acked-by: Paul Moore <paul@paul-moore.com>
Tested-by: Paul Moore <paul@paul-moore.com>
Acked-by: Serge Hallyn <serge@hallyn.com>
Reviewed-by: James Morris <redacted>
Reviewed-by: Andy Lutomirski <luto@kernel.org>
---
security/selinux/hooks.c | 24 +++++-------------------
1 file changed, 5 insertions(+), 19 deletions(-)
--
2.7.4
--
To unsubscribe from this list: send the line "unsubscribe linux-security-module" in
the body of a message to majordomo at vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
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.
Signed-off-by: Kees Cook <redacted>
Acked-by: Serge Hallyn <serge@hallyn.com>
Reviewed-by: James Morris <redacted>
Reviewed-by: Casey Schaufler <casey@schaufler-ca.com>
---
security/smack/smack_lsm.c | 21 ++++-----------------
1 file changed, 4 insertions(+), 17 deletions(-)
@@ -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;+return0;}
--
2.7.4
--
To unsubscribe from this list: send the line "unsubscribe linux-security-module" in
the body of a message to majordomo at vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
The commoncap implementation of the bprm_secureexec hook is the only LSM
that depends on the final call to its bprm_set_creds hook (since it may
be called for multiple files, it ignores bprm->called_set_creds). As a
result, it cannot safely _clear_ bprm->secureexec since other LSMs may
have set it. Instead, remove the bprm_secureexec hook by introducing a
new flag to bprm specific to commoncap: cap_elevated. This is similar to
cap_effective, but that is used for a specific subset of elevated
privileges, and exists solely to track state from bprm_set_creds to
bprm_secureexec. As such, it will be removed in the next patch.
Here, set the new bprm->cap_elevated flag when setuid/setgid has happened
from bprm_fill_uid() or fscapabilities have been prepared. This temporarily
moves the bprm_secureexec hook to a static inline. The helper will be
removed in the next patch; this makes the step easier to review and bisect,
since this does not introduce any changes to inputs nor outputs to the
"elevated privileges" calculation.
The new flag is merged with the bprm->secureexec flag in setup_new_exec()
since this marks the end of any further prepare_binprm() calls.
Cc: Andy Lutomirski <luto@kernel.org>
Signed-off-by: Kees Cook <redacted>
Reviewed-by: Andy Lutomirski <luto@kernel.org>
Acked-by: James Morris <redacted>
Acked-by: Serge Hallyn <serge@hallyn.com>
---
fs/exec.c | 7 +++++++
include/linux/binfmts.h | 7 +++++++
include/linux/security.h | 3 +--
security/commoncap.c | 12 ++++++++----
4 files changed, 23 insertions(+), 6 deletions(-)
--
2.7.4
--
To unsubscribe from this list: send the line "unsubscribe linux-security-module" in
the body of a message to majordomo at vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
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".
The following LTP tests were run to validate the changes:
# ./runltp -f syscalls -s cap
# ./runltp -f securebits
# ./runltp -f cap_bounds
# ./runltp -f filecaps
All kernel selftests for capabilities and exec continue to pass as well.
Signed-off-by: Kees Cook <redacted>
Reviewed-by: James Morris <redacted>
Acked-by: Serge Hallyn <serge@hallyn.com>
Reviewed-by: Andy Lutomirski <luto@kernel.org>
---
include/linux/binfmts.h | 3 ---
security/commoncap.c | 52 ++++++++++---------------------------------------
2 files changed, 10 insertions(+), 45 deletions(-)
@@ -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;-/**Auditcandidateifcurrent->cap_effectiveisset*
@@ -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);--return0;-}--/**-*is_secureexec-Determinewhetherasecureexecutionisrequired-*@bprm:Theexecutionparameters-*-*Determinewhetherasecureexecutionisrequired,return1ifitis,and0-*ifitisnot.-*-*Thecredentialshavebeencommittedbythispoint,andsoarenolonger-*availablethrough@bprm->cred.-*/-staticintis_secureexec(structlinux_binprm*bprm)-{-conststructcred*cred=bprm->cred;-kuid_troot_uid=make_kuid(cred->user_ns,0);--if(!uid_eq(cred->uid,root_uid)){-if(bprm->cap_effective)-return1;-if(!cap_issubset(cred->cap_permitted,cred->cap_ambient))-return1;+bprm->cap_elevated=0;+if(is_setid){+bprm->cap_elevated=1;+}elseif(!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));+return0;}/**
--
2.7.4
--
To unsubscribe from this list: send the line "unsubscribe linux-security-module" in
the body of a message to majordomo at vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
This removes the bprm_secureexec hook since the logic has been folded into
the bprm_set_creds hook for all LSMs now.
Cc: Eric W. Biederman <redacted>
Signed-off-by: Kees Cook <redacted>
Reviewed-by: John Johansen <john.johansen@canonical.com>
Acked-by: James Morris <redacted>
Acked-by: Serge Hallyn <serge@hallyn.com>
---
fs/exec.c | 2 --
include/linux/lsm_hooks.h | 14 +++++---------
include/linux/security.h | 6 ------
security/security.c | 5 -----
4 files changed, 5 insertions(+), 22 deletions(-)
@@ -43,7 +43,11 @@*interpreters.Thehookcantellwhetherithasalreadybeencalledby*checkingtoseeif@bprm->securityisnon-NULL.Ifso,thenthehook*maydecideeithertoretainthesecurityinformationsavedearlieror-*toreplaceit.+*toreplaceit.Thehookmustset@bprm->secureexecto1ifa"secure+*exec" has happened as a result of this hook call. The flag is used to+*indicatetheneedforasanitizedexecutionenvironment,andisalso+*passedintheELFauxiliarytableontheinitialstacktoindicate+*whetherlibcshouldenablesecuremode.*@bprmcontainsthelinux_binprmstructure.*Return0ifthehookissuccessfulandpermissionisgranted.*@bprm_check_security:
--
2.7.4
--
To unsubscribe from this list: send the line "unsubscribe linux-security-module" in
the body of a message to majordomo at vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
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);
+ 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 <dhowells@redhat.com>
Signed-off-by: Kees Cook <redacted>
Acked-by: Serge Hallyn <serge@hallyn.com>
Reviewed-by: James Morris <redacted>
---
fs/exec.c | 3 +--
1 file changed, 1 insertion(+), 2 deletions(-)
--
2.7.4
--
To unsubscribe from this list: send the line "unsubscribe linux-security-module" in
the body of a message to majordomo at vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
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.
Signed-off-by: Kees Cook <redacted>
Acked-by: Serge Hallyn <serge@hallyn.com>
Reviewed-by: James Morris <redacted>
Reviewed-by: Casey Schaufler <casey@schaufler-ca.com>
---
security/smack/smack_lsm.c | 15 ---------------
1 file changed, 15 deletions(-)
--
2.7.4
--
To unsubscribe from this list: send the line "unsubscribe linux-security-module" in
the body of a message to majordomo at vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
--
2.7.4
--
To unsubscribe from this list: send the line "unsubscribe linux-security-module" in
the body of a message to majordomo at vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html