* [PATCH] selinux: Permit transitions under NO_NEW_PRIVS or NOSUID under certain, circumstances. @ 2014-06-12 19:18 Stephen Smalley 2014-06-12 19:28 ` Andy Lutomirski 0 siblings, 1 reply; 11+ messages in thread From: Stephen Smalley @ 2014-06-12 19:18 UTC (permalink / raw) To: SELinux-NSA; +Cc: Andy Lutomirski [-- Attachment #1: Type: text/plain, Size: 0 bytes --] [-- Attachment #2: 0001-Permit-transitions-under-NO_NEW_PRIVS-or-NOSUID-unde.patch --] [-- Type: text/x-patch, Size: 3571 bytes --] >From 731c593d0813128eb6d3a62658038681b6e1cf9a Mon Sep 17 00:00:00 2001 From: Stephen Smalley <sds@tycho.nsa.gov> Date: Thu, 12 Jun 2014 08:17:48 -0400 Subject: [PATCH] Permit transitions under NO_NEW_PRIVS or NOSUID under certain circumstances. If the caller SID is bounded by the callee SID or if the caller SID is allowed to perform a dynamic transition (setcon) to the callee SID, then allowing the transition to occur poses no risk of privilege escalation and we can therefore safely allow the transition to occur. Add these two exemptions for both the case where a transition was explicitly requested by the application and the case where an automatic transition is defined in policy. Signed-off-by: Stephen Smalley <sds@tycho.nsa.gov> Acked-by: Andy Lutomirski <luto@amacapital.net> --- security/selinux/hooks.c | 54 +++++++++++++++++++++++++++++++++++++----------- 1 file changed, 42 insertions(+), 12 deletions(-) diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c index 83d06db..d5e8dc5 100644 --- a/security/selinux/hooks.c +++ b/security/selinux/hooks.c @@ -2086,6 +2086,36 @@ static int selinux_vm_enough_memory(struct mm_struct *mm, long pages) /* binprm security operations */ +static int check_nnp_nosuid(const struct linux_binprm *bprm, + const struct task_security_struct *old_tsec, + const struct task_security_struct *new_tsec) +{ + int nnp = (bprm->unsafe & LSM_UNSAFE_NO_NEW_PRIVS); + int nosuid = (bprm->file->f_path.mnt->mnt_flags & MNT_NOSUID); + int rc; + + if (!nnp && !nosuid) + return 0; /* neither NNP nor nosuid */ + + if (new_tsec->sid == old_tsec->sid) + return 0; /* No change in credentials */ + + rc = security_bounded_transition(old_tsec->sid, new_tsec->sid); + if (rc == 0) + return 0; /* allowed via bounded transition */ + + /* Only allow if dyntransition permission aka setcon() is allowed. */ + rc = avc_has_perm(old_tsec->sid, new_tsec->sid, SECCLASS_PROCESS, + PROCESS__DYNTRANSITION, NULL); + if (rc) { + if (nnp) + return -EPERM; + else + return -EACCES; + } + return 0; +} + static int selinux_bprm_set_creds(struct linux_binprm *bprm) { const struct task_security_struct *old_tsec; @@ -2122,14 +2152,10 @@ static int selinux_bprm_set_creds(struct linux_binprm *bprm) /* Reset exec SID on execve. */ new_tsec->exec_sid = 0; - /* - * Minimize confusion: if no_new_privs or nosuid and a - * transition is explicitly requested, then fail the exec. - */ - if (bprm->unsafe & LSM_UNSAFE_NO_NEW_PRIVS) - return -EPERM; - if (bprm->file->f_path.mnt->mnt_flags & MNT_NOSUID) - return -EACCES; + /* Fail on NNP or nosuid if not an allowed transition. */ + rc = check_nnp_nosuid(bprm, old_tsec, new_tsec); + if (rc) + return rc; } else { /* Check for a default transition on this program. */ rc = security_transition_sid(old_tsec->sid, isec->sid, @@ -2137,15 +2163,19 @@ static int selinux_bprm_set_creds(struct linux_binprm *bprm) &new_tsec->sid); if (rc) return rc; + + /* + * Fallback to old SID on NNP or nosuid if not an allowed + * transition. + */ + rc = check_nnp_nosuid(bprm, old_tsec, new_tsec); + if (rc) + new_tsec->sid = old_tsec->sid; } ad.type = LSM_AUDIT_DATA_PATH; ad.u.path = bprm->file->f_path; - if ((bprm->file->f_path.mnt->mnt_flags & MNT_NOSUID) || - (bprm->unsafe & LSM_UNSAFE_NO_NEW_PRIVS)) - new_tsec->sid = old_tsec->sid; - if (new_tsec->sid == old_tsec->sid) { rc = avc_has_perm(old_tsec->sid, isec->sid, SECCLASS_FILE, FILE__EXECUTE_NO_TRANS, &ad); -- 1.9.3 ^ permalink raw reply related [flat|nested] 11+ messages in thread
* Re: [PATCH] selinux: Permit transitions under NO_NEW_PRIVS or NOSUID under certain, circumstances. 2014-06-12 19:18 [PATCH] selinux: Permit transitions under NO_NEW_PRIVS or NOSUID under certain, circumstances Stephen Smalley @ 2014-06-12 19:28 ` Andy Lutomirski 2014-06-12 19:29 ` [PATCH v2] selinux: Permit exec transitions under NO_NEW_PRIVS or NOSUID under certain circumstances Stephen Smalley 0 siblings, 1 reply; 11+ messages in thread From: Andy Lutomirski @ 2014-06-12 19:28 UTC (permalink / raw) To: Stephen Smalley; +Cc: SELinux-NSA On Thu, Jun 12, 2014 at 12:18 PM, Stephen Smalley <sds@tycho.nsa.gov> wrote: > One more minor nit: "If the caller SID is bounded by the callee SID" Is that backwards? I think the code is correct, but I'm less convinced by the changelog. -- Andy Lutomirski AMA Capital Management, LLC ^ permalink raw reply [flat|nested] 11+ messages in thread
* [PATCH v2] selinux: Permit exec transitions under NO_NEW_PRIVS or NOSUID under certain circumstances. 2014-06-12 19:28 ` Andy Lutomirski @ 2014-06-12 19:29 ` Stephen Smalley 2014-06-16 15:25 ` [PATCH] selinux-testsuite: Add tests for exec transitions under NO_NEW_PRIVS Stephen Smalley 2014-06-19 20:04 ` [PATCH v2] selinux: Permit exec transitions under NO_NEW_PRIVS or NOSUID under certain circumstances Paul Moore 0 siblings, 2 replies; 11+ messages in thread From: Stephen Smalley @ 2014-06-12 19:29 UTC (permalink / raw) To: Andy Lutomirski; +Cc: SELinux-NSA [-- Attachment #1: Type: text/plain, Size: 47 bytes --] v2 - fix patch description to match the code. [-- Attachment #2: 0001-selinux-Permit-exec-transitions-under-NO_NEW_PRIVS-o.patch --] [-- Type: text/x-patch, Size: 3588 bytes --] >From c1fa21950c5c3eb0dd97ae5145fa3d3f04adc5c4 Mon Sep 17 00:00:00 2001 From: Stephen Smalley <sds@tycho.nsa.gov> Date: Thu, 12 Jun 2014 08:17:48 -0400 Subject: [PATCH] selinux: Permit exec transitions under NO_NEW_PRIVS or NOSUID under certain circumstances. If the callee SID is bounded by the caller SID or if the caller SID is allowed to perform a dynamic transition (setcon) to the callee SID, then allowing the transition to occur poses no risk of privilege escalation and we can therefore safely allow the transition to occur. Add these two exemptions for both the case where a transition was explicitly requested by the application and the case where an automatic transition is defined in policy. Signed-off-by: Stephen Smalley <sds@tycho.nsa.gov> Acked-by: Andy Lutomirski <luto@amacapital.net> --- security/selinux/hooks.c | 54 +++++++++++++++++++++++++++++++++++++----------- 1 file changed, 42 insertions(+), 12 deletions(-) diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c index 83d06db..d5e8dc5 100644 --- a/security/selinux/hooks.c +++ b/security/selinux/hooks.c @@ -2086,6 +2086,36 @@ static int selinux_vm_enough_memory(struct mm_struct *mm, long pages) /* binprm security operations */ +static int check_nnp_nosuid(const struct linux_binprm *bprm, + const struct task_security_struct *old_tsec, + const struct task_security_struct *new_tsec) +{ + int nnp = (bprm->unsafe & LSM_UNSAFE_NO_NEW_PRIVS); + int nosuid = (bprm->file->f_path.mnt->mnt_flags & MNT_NOSUID); + int rc; + + if (!nnp && !nosuid) + return 0; /* neither NNP nor nosuid */ + + if (new_tsec->sid == old_tsec->sid) + return 0; /* No change in credentials */ + + rc = security_bounded_transition(old_tsec->sid, new_tsec->sid); + if (rc == 0) + return 0; /* allowed via bounded transition */ + + /* Only allow if dyntransition permission aka setcon() is allowed. */ + rc = avc_has_perm(old_tsec->sid, new_tsec->sid, SECCLASS_PROCESS, + PROCESS__DYNTRANSITION, NULL); + if (rc) { + if (nnp) + return -EPERM; + else + return -EACCES; + } + return 0; +} + static int selinux_bprm_set_creds(struct linux_binprm *bprm) { const struct task_security_struct *old_tsec; @@ -2122,14 +2152,10 @@ static int selinux_bprm_set_creds(struct linux_binprm *bprm) /* Reset exec SID on execve. */ new_tsec->exec_sid = 0; - /* - * Minimize confusion: if no_new_privs or nosuid and a - * transition is explicitly requested, then fail the exec. - */ - if (bprm->unsafe & LSM_UNSAFE_NO_NEW_PRIVS) - return -EPERM; - if (bprm->file->f_path.mnt->mnt_flags & MNT_NOSUID) - return -EACCES; + /* Fail on NNP or nosuid if not an allowed transition. */ + rc = check_nnp_nosuid(bprm, old_tsec, new_tsec); + if (rc) + return rc; } else { /* Check for a default transition on this program. */ rc = security_transition_sid(old_tsec->sid, isec->sid, @@ -2137,15 +2163,19 @@ static int selinux_bprm_set_creds(struct linux_binprm *bprm) &new_tsec->sid); if (rc) return rc; + + /* + * Fallback to old SID on NNP or nosuid if not an allowed + * transition. + */ + rc = check_nnp_nosuid(bprm, old_tsec, new_tsec); + if (rc) + new_tsec->sid = old_tsec->sid; } ad.type = LSM_AUDIT_DATA_PATH; ad.u.path = bprm->file->f_path; - if ((bprm->file->f_path.mnt->mnt_flags & MNT_NOSUID) || - (bprm->unsafe & LSM_UNSAFE_NO_NEW_PRIVS)) - new_tsec->sid = old_tsec->sid; - if (new_tsec->sid == old_tsec->sid) { rc = avc_has_perm(old_tsec->sid, isec->sid, SECCLASS_FILE, FILE__EXECUTE_NO_TRANS, &ad); -- 1.8.3.1 ^ permalink raw reply related [flat|nested] 11+ messages in thread
* [PATCH] selinux-testsuite: Add tests for exec transitions under NO_NEW_PRIVS 2014-06-12 19:29 ` [PATCH v2] selinux: Permit exec transitions under NO_NEW_PRIVS or NOSUID under certain circumstances Stephen Smalley @ 2014-06-16 15:25 ` Stephen Smalley 2014-06-19 20:04 ` [PATCH v2] selinux: Permit exec transitions under NO_NEW_PRIVS or NOSUID under certain circumstances Paul Moore 1 sibling, 0 replies; 11+ messages in thread From: Stephen Smalley @ 2014-06-16 15:25 UTC (permalink / raw) To: Andy Lutomirski; +Cc: SELinux-NSA [-- Attachment #1: Type: text/plain, Size: 376 bytes --] Tests 1-4 will fail on existing kernels without the patch entitled "selinux: Permit exec transitions under NO_NEW_PRIVS or NOSUID under certain circumstances.". Tests 5-6 should always pass. We shouldn't apply it to the selinux-testsuite until after the kernel change is merged, and then we should make it dependent on the first kernel version that will include the change. [-- Attachment #2: 0001-Add-tests-for-exec-transitions-under-NO_NEW_PRIVS.patch --] [-- Type: text/x-patch, Size: 8442 bytes --] >From 66e8681c88a4cb0e3d84ed92f59e9812ba4a3003 Mon Sep 17 00:00:00 2001 From: Stephen Smalley <sds@tycho.nsa.gov> Date: Fri, 13 Jun 2014 12:40:16 -0400 Subject: [PATCH] Add tests for exec transitions under NO_NEW_PRIVS. Signed-off-by: Stephen Smalley <sds@tycho.nsa.gov> --- policy/Makefile | 2 +- policy/test_nnp.te | 48 +++++++++++++++++++++++++++++++++++++++++++++++ tests/Makefile | 2 +- tests/nnp/Makefile | 7 +++++++ tests/nnp/checkcon.c | 40 +++++++++++++++++++++++++++++++++++++++ tests/nnp/execnnp.c | 25 +++++++++++++++++++++++++ tests/nnp/test | 53 ++++++++++++++++++++++++++++++++++++++++++++++++++++ 7 files changed, 175 insertions(+), 2 deletions(-) create mode 100644 policy/test_nnp.te create mode 100644 tests/nnp/Makefile create mode 100644 tests/nnp/checkcon.c create mode 100644 tests/nnp/execnnp.c create mode 100755 tests/nnp/test diff --git a/policy/Makefile b/policy/Makefile index a0a6c88..683f454 100644 --- a/policy/Makefile +++ b/policy/Makefile @@ -14,7 +14,7 @@ TARGETS = \ test_entrypoint.te test_execshare.te test_exectrace.te \ test_execute_no_trans.te test_fdreceive.te test_file.te \ test_inherit.te test_ioctl.te test_ipc.te test_link.te test_mkdir.te \ - test_open.te test_ptrace.te test_readlink.te \ + test_nnp.te test_open.te test_ptrace.te test_readlink.te \ test_relabel.te test_rename.te test_rxdir.te test_setattr.te \ test_setnice.te test_sigkill.te test_stat.te test_sysctl.te \ test_task_create.te test_task_getpgid.te test_task_getsched.te \ diff --git a/policy/test_nnp.te b/policy/test_nnp.te new file mode 100644 index 0000000..8ab798f --- /dev/null +++ b/policy/test_nnp.te @@ -0,0 +1,48 @@ +################################# +# +# Policy for testing NO_NEW_PRIVS transitions. +# + +# A domain bounded by the unconfined domain. +type test_nnp_bounded_t; +domain_type(test_nnp_bounded_t) +typeattribute test_nnp_bounded_t testdomain; +typebounds unconfined_t test_nnp_bounded_t; + +# The entrypoint type for this domain. +type test_nnp_bounded_exec_t; +files_type(test_nnp_bounded_exec_t) +domain_entry_file(test_nnp_bounded_t, test_nnp_bounded_exec_t) + +# Run it! This should succeed on patched kernels, fail on old ones. +unconfined_runs_test(test_nnp_bounded_t) +unconfined_run_to(test_nnp_bounded_t, test_nnp_bounded_exec_t) + +# A domain that can be reached via setcon by the start domain. +type test_nnp_dyntransition_t; +domain_type(test_nnp_dyntransition_t) +typeattribute test_nnp_dyntransition_t testdomain; +allow unconfined_t test_nnp_dyntransition_t:process dyntransition; + +# The entrypoint type for this domain. +type test_nnp_dyntransition_exec_t; +files_type(test_nnp_dyntransition_exec_t) +domain_entry_file(test_nnp_dyntransition_t, test_nnp_dyntransition_exec_t) + +# Run it! This should succeed on patched kernels, fail on old ones. +unconfined_runs_test(test_nnp_dyntransition_t) +unconfined_run_to(test_nnp_dyntransition_t, test_nnp_dyntransition_exec_t) + +# A domain that is neither bounded nor reachable via setcon from the start domain. +type test_nnp_neither_t; +domain_type(test_nnp_neither_t) +typeattribute test_nnp_neither_t testdomain; + +# The entrypoint type for this domain. +type test_nnp_neither_exec_t; +files_type(test_nnp_neither_exec_t) +domain_entry_file(test_nnp_neither_t, test_nnp_neither_exec_t) + +# Run it! This should fail always. +unconfined_runs_test(test_nnp_neither_t) +unconfined_run_to(test_nnp_neither_t, test_nnp_neither_exec_t) diff --git a/tests/Makefile b/tests/Makefile index 5886403..4cfecdf 100644 --- a/tests/Makefile +++ b/tests/Makefile @@ -1,6 +1,6 @@ REDHAT_RELEASE=$(shell rpm -q redhat-release) -SUBDIRS=domain_trans entrypoint execshare exectrace execute_no_trans fdreceive inherit link mkdir msg open ptrace readlink relabel rename rxdir sem setattr setnice shm sigkill stat sysctl task_create task_setnice task_setscheduler task_getscheduler task_getsid task_getpgid task_setpgid wait file ioctl capable_file capable_net capable_sys dyntrans dyntrace bounds +SUBDIRS=domain_trans entrypoint execshare exectrace execute_no_trans fdreceive inherit link mkdir msg nnp open ptrace readlink relabel rename rxdir sem setattr setnice shm sigkill stat sysctl task_create task_setnice task_setscheduler task_getscheduler task_getsid task_getpgid task_setpgid wait file ioctl capable_file capable_net capable_sys dyntrans dyntrace bounds #SUBDIRS=socket unix_socket unix_secure ifeq (redhat-release-4, $(findstring redhat-release-4, $(REDHAT_RELEASE))) diff --git a/tests/nnp/Makefile b/tests/nnp/Makefile new file mode 100644 index 0000000..4e8e400 --- /dev/null +++ b/tests/nnp/Makefile @@ -0,0 +1,7 @@ +TARGETS=execnnp checkcon + +LDLIBS += -lselinux + +all: $(TARGETS) +clean: + rm -f $(TARGETS) diff --git a/tests/nnp/checkcon.c b/tests/nnp/checkcon.c new file mode 100644 index 0000000..5d992ca --- /dev/null +++ b/tests/nnp/checkcon.c @@ -0,0 +1,40 @@ +#include <stdio.h> +#include <stdlib.h> +#include <string.h> +#include <unistd.h> +#include <selinux/selinux.h> +#include <selinux/context.h> + +int main(int argc, char **argv) +{ + char *con = NULL; + context_t c; + const char *type; + int rc; + + if (argc != 2) { + fprintf(stderr, "usage: %s expected-type\n", argv[0]); + exit(-1); + } + + if (getcon(&con) < 0) { + perror("getcon"); + exit(-1); + } + + c = context_new(con); + if (!c) { + perror("context_new"); + exit(-1); + } + + type = context_type_get(c); + if (!type) { + perror("context_type_get"); + exit(-1); + + } + + rc = strcmp(type, argv[1]); + exit(rc); +} diff --git a/tests/nnp/execnnp.c b/tests/nnp/execnnp.c new file mode 100644 index 0000000..9de5967 --- /dev/null +++ b/tests/nnp/execnnp.c @@ -0,0 +1,25 @@ +#include <stdio.h> +#include <stdlib.h> +#include <string.h> +#include <unistd.h> +#include <sys/prctl.h> + +int main(int argc, char **argv) +{ + int rc; + + if (argc < 2) { + fprintf(stderr, "usage: %s command [args...]\n", argv[0]); + exit(-1); + } + + rc = prctl(PR_SET_NO_NEW_PRIVS, 1, 0, 0, 0); + if (rc < 0) { + perror("prctl PR_SET_NO_NEW_PRIVS"); + exit(-1); + } + + execvp(argv[1], &argv[1]); + perror(argv[1]); + exit(1); +} diff --git a/tests/nnp/test b/tests/nnp/test new file mode 100755 index 0000000..eb2c522 --- /dev/null +++ b/tests/nnp/test @@ -0,0 +1,53 @@ +#!/usr/bin/perl + +# Depends on kernel patch with the following subject line: +# selinux: Permit exec transitions under NO_NEW_PRIVS or NOSUID under certain circumstances. + +use Test; +BEGIN { plan tests => 6} + +$basedir = $0; $basedir =~ s|(.*)/[^/]*|$1|; + +# Remove any leftover programs from prior failed runs. +system("rm -f $basedir/true"); + +# Set entrypoint type for bounded domain. +system("chcon -t test_nnp_bounded_exec_t $basedir/checkcon"); + +# Transition to bounded type via setexec. +$result = system("$basedir/execnnp runcon -t test_nnp_bounded_t $basedir/checkcon test_nnp_bounded_t 2>&1"); +ok($result,0); #this should pass + +# Automatic transition to bounded domain via exec. +$result = system("$basedir/execnnp $basedir/checkcon test_nnp_bounded_t 2>&1"); +ok($result,0); #this should pass + +# Set entrypoint type for dyntransition domain. +system("chcon -t test_nnp_dyntransition_exec_t $basedir/checkcon"); + +# Transition to dyntransition domain via setexec. +$result = system("$basedir/execnnp runcon -t test_nnp_dyntransition_t $basedir/checkcon test_nnp_dyntransition_t 2>&1"); +ok($result,0); #this should pass + +# Automatic transition to dyntransition domain via exec. +$result = system("$basedir/execnnp $basedir/checkcon test_nnp_dyntransition_t 2>&1"); +ok($result,0); #this should pass + +# Use true as an entrypoint program to test ability to exec at all. +system("cp /bin/true $basedir/true"); + +# Set entrypoint type for neither bounded nor dyntransition domain. +system("chcon -t test_nnp_neither_exec_t $basedir/checkcon $basedir/true"); + +# Transition to neither domain via setexec. +$result = system("$basedir/execnnp runcon -t test_nnp_neither_t $basedir/true 2>&1"); +ok($result); #this should fail + +# Automatic transition to neither domain via exec. +$result = system("$basedir/execnnp $basedir/checkcon test_nnp_neither_t 2>&1"); +ok($result); #this should fail + +# Cleanup. +system("rm -f $basedir/true"); + +exit; -- 1.9.3 ^ permalink raw reply related [flat|nested] 11+ messages in thread
* Re: [PATCH v2] selinux: Permit exec transitions under NO_NEW_PRIVS or NOSUID under certain circumstances. 2014-06-12 19:29 ` [PATCH v2] selinux: Permit exec transitions under NO_NEW_PRIVS or NOSUID under certain circumstances Stephen Smalley 2014-06-16 15:25 ` [PATCH] selinux-testsuite: Add tests for exec transitions under NO_NEW_PRIVS Stephen Smalley @ 2014-06-19 20:04 ` Paul Moore 2014-06-19 20:51 ` Paul Moore 1 sibling, 1 reply; 11+ messages in thread From: Paul Moore @ 2014-06-19 20:04 UTC (permalink / raw) To: Stephen Smalley; +Cc: SELinux-NSA, Andy Lutomirski On Thursday, June 12, 2014 03:29:04 PM Stephen Smalley wrote: > v2 - fix patch description to match the code. Okay Stephen, I suppose you should get some special consideration, but is posting your patches inline really that hard :) --- >From c1fa21950c5c3eb0dd97ae5145fa3d3f04adc5c4 Mon Sep 17 00:00:00 2001 From: Stephen Smalley <sds@tycho.nsa.gov> Date: Thu, 12 Jun 2014 08:17:48 -0400 Subject: [PATCH] selinux: Permit exec transitions under NO_NEW_PRIVS or NOSUID under certain circumstances. If the callee SID is bounded by the caller SID or if the caller SID is allowed to perform a dynamic transition (setcon) to the callee SID, then allowing the transition to occur poses no risk of privilege escalation and we can therefore safely allow the transition to occur. Add these two exemptions for both the case where a transition was explicitly requested by the application and the case where an automatic transition is defined in policy. Signed-off-by: Stephen Smalley <sds@tycho.nsa.gov> Acked-by: Andy Lutomirski <luto@amacapital.net> --- security/selinux/hooks.c | 54 +++++++++++++++++++++++++++++++++++++----------- 1 file changed, 42 insertions(+), 12 deletions(-) diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c index 83d06db..d5e8dc5 100644 --- a/security/selinux/hooks.c +++ b/security/selinux/hooks.c @@ -2086,6 +2086,36 @@ static int selinux_vm_enough_memory(struct mm_struct *mm, long pages) /* binprm security operations */ +static int check_nnp_nosuid(const struct linux_binprm *bprm, + const struct task_security_struct *old_tsec, + const struct task_security_struct *new_tsec) +{ + int nnp = (bprm->unsafe & LSM_UNSAFE_NO_NEW_PRIVS); + int nosuid = (bprm->file->f_path.mnt->mnt_flags & MNT_NOSUID); + int rc; + + if (!nnp && !nosuid) + return 0; /* neither NNP nor nosuid */ + + if (new_tsec->sid == old_tsec->sid) + return 0; /* No change in credentials */ + + rc = security_bounded_transition(old_tsec->sid, new_tsec->sid); + if (rc == 0) + return 0; /* allowed via bounded transition */ + + /* Only allow if dyntransition permission aka setcon() is allowed. */ + rc = avc_has_perm(old_tsec->sid, new_tsec->sid, SECCLASS_PROCESS, + PROCESS__DYNTRANSITION, NULL); + if (rc) { + if (nnp) + return -EPERM; + else + return -EACCES; + } + return 0; +} + static int selinux_bprm_set_creds(struct linux_binprm *bprm) { const struct task_security_struct *old_tsec; @@ -2122,14 +2152,10 @@ static int selinux_bprm_set_creds(struct linux_binprm *bprm) /* Reset exec SID on execve. */ new_tsec->exec_sid = 0; - /* - * Minimize confusion: if no_new_privs or nosuid and a - * transition is explicitly requested, then fail the exec. - */ - if (bprm->unsafe & LSM_UNSAFE_NO_NEW_PRIVS) - return -EPERM; - if (bprm->file->f_path.mnt->mnt_flags & MNT_NOSUID) - return -EACCES; + /* Fail on NNP or nosuid if not an allowed transition. */ + rc = check_nnp_nosuid(bprm, old_tsec, new_tsec); + if (rc) + return rc; } else { /* Check for a default transition on this program. */ rc = security_transition_sid(old_tsec->sid, isec->sid, @@ -2137,15 +2163,19 @@ static int selinux_bprm_set_creds(struct linux_binprm *bprm) &new_tsec->sid); if (rc) return rc; + + /* + * Fallback to old SID on NNP or nosuid if not an allowed + * transition. + */ + rc = check_nnp_nosuid(bprm, old_tsec, new_tsec); + if (rc) + new_tsec->sid = old_tsec->sid; } ad.type = LSM_AUDIT_DATA_PATH; ad.u.path = bprm->file->f_path; - if ((bprm->file->f_path.mnt->mnt_flags & MNT_NOSUID) || - (bprm->unsafe & LSM_UNSAFE_NO_NEW_PRIVS)) - new_tsec->sid = old_tsec->sid; - if (new_tsec->sid == old_tsec->sid) { rc = avc_has_perm(old_tsec->sid, isec->sid, SECCLASS_FILE, FILE__EXECUTE_NO_TRANS, &ad); -- 1.8.3.1 ^ permalink raw reply related [flat|nested] 11+ messages in thread
* Re: [PATCH v2] selinux: Permit exec transitions under NO_NEW_PRIVS or NOSUID under certain circumstances. 2014-06-19 20:04 ` [PATCH v2] selinux: Permit exec transitions under NO_NEW_PRIVS or NOSUID under certain circumstances Paul Moore @ 2014-06-19 20:51 ` Paul Moore 2014-06-23 17:23 ` Stephen Smalley 0 siblings, 1 reply; 11+ messages in thread From: Paul Moore @ 2014-06-19 20:51 UTC (permalink / raw) To: Stephen Smalley, Andy Lutomirski; +Cc: SELinux-NSA On Thursday, June 19, 2014 04:04:18 PM Paul Moore wrote: > On Thursday, June 12, 2014 03:29:04 PM Stephen Smalley wrote: > > v2 - fix patch description to match the code. > > Okay Stephen, I suppose you should get some special consideration, but is > posting your patches inline really that hard :) > > --- > From c1fa21950c5c3eb0dd97ae5145fa3d3f04adc5c4 Mon Sep 17 00:00:00 2001 > From: Stephen Smalley <sds@tycho.nsa.gov> > Date: Thu, 12 Jun 2014 08:17:48 -0400 > Subject: [PATCH] selinux: Permit exec transitions under NO_NEW_PRIVS or > NOSUID under certain circumstances. > > If the callee SID is bounded by the caller SID or if the caller SID is > allowed to perform a dynamic transition (setcon) to the callee SID, then > allowing the transition to occur poses no risk of privilege escalation and > we can therefore safely allow the transition to occur. Add these two > exemptions for both the case where a transition was explicitly requested by > the application and the case where an automatic transition is defined in > policy. > > Signed-off-by: Stephen Smalley <sds@tycho.nsa.gov> > Acked-by: Andy Lutomirski <luto@amacapital.net> Comments below ... > diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c > index 83d06db..d5e8dc5 100644 > --- a/security/selinux/hooks.c > +++ b/security/selinux/hooks.c > @@ -2086,6 +2086,36 @@ static int selinux_vm_enough_memory(struct mm_struct > *mm, long pages) > > /* binprm security operations */ > > +static int check_nnp_nosuid(const struct linux_binprm *bprm, > + const struct task_security_struct *old_tsec, > + const struct task_security_struct *new_tsec) > +{ > + int nnp = (bprm->unsafe & LSM_UNSAFE_NO_NEW_PRIVS); > + int nosuid = (bprm->file->f_path.mnt->mnt_flags & MNT_NOSUID); > + int rc; > + > + if (!nnp && !nosuid) > + return 0; /* neither NNP nor nosuid */ > + > + if (new_tsec->sid == old_tsec->sid) > + return 0; /* No change in credentials */ > + > + rc = security_bounded_transition(old_tsec->sid, new_tsec->sid); > + if (rc == 0) > + return 0; /* allowed via bounded transition */ I think there might be an audit issue here; security_bounded_transition() will generate an audit record on failure which probably isn't what we want in this case. Other than that, this seems reasonable, even in the face of NNP, and as others point out, standard DAC capabilities do something similar. > + /* Only allow if dyntransition permission aka setcon() is allowed. */ > + rc = avc_has_perm(old_tsec->sid, new_tsec->sid, SECCLASS_PROCESS, > + PROCESS__DYNTRANSITION, NULL); > + if (rc) { > + if (nnp) > + return -EPERM; > + else > + return -EACCES; > + } > + return 0; I know this dyntransition/NNP/NOSUID interaction has been discussed quite a bit off-list (mostly while I was away, my apologies), but I haven't seen a lot on-list and while the descriptions hints at it the code itself doesn't elaborate on why this is "OK". I'd like to see some comments about why it is okay to allow the transition, regardless of NNP/NOSUID, if the dyntransition is allowed. I'd also like to see a quick comment about why we return -EPERM/- EACCES. There was a lot of discussion around these points and I want to make sure the ideas aren't lost. -- paul moore www.paul-moore.com ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH v2] selinux: Permit exec transitions under NO_NEW_PRIVS or NOSUID under certain circumstances. 2014-06-19 20:51 ` Paul Moore @ 2014-06-23 17:23 ` Stephen Smalley 2014-06-23 18:25 ` Andy Lutomirski 0 siblings, 1 reply; 11+ messages in thread From: Stephen Smalley @ 2014-06-23 17:23 UTC (permalink / raw) To: Paul Moore, Andy Lutomirski; +Cc: SELinux-NSA On 06/19/2014 04:51 PM, Paul Moore wrote: > On Thursday, June 19, 2014 04:04:18 PM Paul Moore wrote: >> On Thursday, June 12, 2014 03:29:04 PM Stephen Smalley wrote: >>> v2 - fix patch description to match the code. >> >> Okay Stephen, I suppose you should get some special consideration, but is >> posting your patches inline really that hard :) >> >> --- >> From c1fa21950c5c3eb0dd97ae5145fa3d3f04adc5c4 Mon Sep 17 00:00:00 2001 >> From: Stephen Smalley <sds@tycho.nsa.gov> >> Date: Thu, 12 Jun 2014 08:17:48 -0400 >> Subject: [PATCH] selinux: Permit exec transitions under NO_NEW_PRIVS or >> NOSUID under certain circumstances. >> >> If the callee SID is bounded by the caller SID or if the caller SID is >> allowed to perform a dynamic transition (setcon) to the callee SID, then >> allowing the transition to occur poses no risk of privilege escalation and >> we can therefore safely allow the transition to occur. Add these two >> exemptions for both the case where a transition was explicitly requested by >> the application and the case where an automatic transition is defined in >> policy. >> >> Signed-off-by: Stephen Smalley <sds@tycho.nsa.gov> >> Acked-by: Andy Lutomirski <luto@amacapital.net> > > Comments below ... > >> diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c >> index 83d06db..d5e8dc5 100644 >> --- a/security/selinux/hooks.c >> +++ b/security/selinux/hooks.c >> @@ -2086,6 +2086,36 @@ static int selinux_vm_enough_memory(struct mm_struct >> *mm, long pages) >> >> /* binprm security operations */ >> >> +static int check_nnp_nosuid(const struct linux_binprm *bprm, >> + const struct task_security_struct *old_tsec, >> + const struct task_security_struct *new_tsec) >> +{ >> + int nnp = (bprm->unsafe & LSM_UNSAFE_NO_NEW_PRIVS); >> + int nosuid = (bprm->file->f_path.mnt->mnt_flags & MNT_NOSUID); >> + int rc; >> + >> + if (!nnp && !nosuid) >> + return 0; /* neither NNP nor nosuid */ >> + >> + if (new_tsec->sid == old_tsec->sid) >> + return 0; /* No change in credentials */ >> + >> + rc = security_bounded_transition(old_tsec->sid, new_tsec->sid); >> + if (rc == 0) >> + return 0; /* allowed via bounded transition */ > > I think there might be an audit issue here; security_bounded_transition() will > generate an audit record on failure which probably isn't what we want in this > case. > > Other than that, this seems reasonable, even in the face of NNP, and as others > point out, standard DAC capabilities do something similar. > >> + /* Only allow if dyntransition permission aka setcon() is allowed. */ >> + rc = avc_has_perm(old_tsec->sid, new_tsec->sid, SECCLASS_PROCESS, >> + PROCESS__DYNTRANSITION, NULL); >> + if (rc) { >> + if (nnp) >> + return -EPERM; >> + else >> + return -EACCES; >> + } >> + return 0; > > I know this dyntransition/NNP/NOSUID interaction has been discussed quite a > bit off-list (mostly while I was away, my apologies), but I haven't seen a lot > on-list and while the descriptions hints at it the code itself doesn't > elaborate on why this is "OK". I'd like to see some comments about why it is > okay to allow the transition, regardless of NNP/NOSUID, if the dyntransition > is allowed. I'd also like to see a quick comment about why we return -EPERM/- > EACCES. > > There was a lot of discussion around these points and I want to make sure the > ideas aren't lost. The claim was that dyntransition is sufficient since it would allow the caller to setcon() to the new domain directly and therefore perform any action in that domain. Thus, allowing it to transition to that domain upon exec under NNP or on a file in a nosuid mount does not enable it to do anything it could not already do directly. However, this presumes that the policy writer does not merely add dyntransition to the caller domain upon encountering the avc denial in this situation without thinking about the implications and deciding whether to truly trust the caller domain in this way. Which is likely a dangerous assumption, especially for people who write policy via audit2allow. At least with the bounded transition, you have to explicitly declare that the new domain is bounded by the caller domain and the kernel will then enforce the restriction that the new domain is not granted any permission not allowed to the caller domain. I'm also unclear as to whether this in fact solves the actual problem for sandbox -X. So I'd drop this patch for now at least. ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH v2] selinux: Permit exec transitions under NO_NEW_PRIVS or NOSUID under certain circumstances. 2014-06-23 17:23 ` Stephen Smalley @ 2014-06-23 18:25 ` Andy Lutomirski 2014-06-23 19:48 ` Daniel J Walsh 0 siblings, 1 reply; 11+ messages in thread From: Andy Lutomirski @ 2014-06-23 18:25 UTC (permalink / raw) To: Stephen Smalley, Daniel Walsh; +Cc: SELinux-NSA On Mon, Jun 23, 2014 at 10:23 AM, Stephen Smalley <sds@tycho.nsa.gov> wrote: > On 06/19/2014 04:51 PM, Paul Moore wrote: >> On Thursday, June 19, 2014 04:04:18 PM Paul Moore wrote: >>> On Thursday, June 12, 2014 03:29:04 PM Stephen Smalley wrote: >>>> v2 - fix patch description to match the code. >>> >>> Okay Stephen, I suppose you should get some special consideration, but is >>> posting your patches inline really that hard :) >>> >>> --- >>> From c1fa21950c5c3eb0dd97ae5145fa3d3f04adc5c4 Mon Sep 17 00:00:00 2001 >>> From: Stephen Smalley <sds@tycho.nsa.gov> >>> Date: Thu, 12 Jun 2014 08:17:48 -0400 >>> Subject: [PATCH] selinux: Permit exec transitions under NO_NEW_PRIVS or >>> NOSUID under certain circumstances. >>> >>> If the callee SID is bounded by the caller SID or if the caller SID is >>> allowed to perform a dynamic transition (setcon) to the callee SID, then >>> allowing the transition to occur poses no risk of privilege escalation and >>> we can therefore safely allow the transition to occur. Add these two >>> exemptions for both the case where a transition was explicitly requested by >>> the application and the case where an automatic transition is defined in >>> policy. >>> >>> Signed-off-by: Stephen Smalley <sds@tycho.nsa.gov> >>> Acked-by: Andy Lutomirski <luto@amacapital.net> >> >> Comments below ... >> >>> diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c >>> index 83d06db..d5e8dc5 100644 >>> --- a/security/selinux/hooks.c >>> +++ b/security/selinux/hooks.c >>> @@ -2086,6 +2086,36 @@ static int selinux_vm_enough_memory(struct mm_struct >>> *mm, long pages) >>> >>> /* binprm security operations */ >>> >>> +static int check_nnp_nosuid(const struct linux_binprm *bprm, >>> + const struct task_security_struct *old_tsec, >>> + const struct task_security_struct *new_tsec) >>> +{ >>> + int nnp = (bprm->unsafe & LSM_UNSAFE_NO_NEW_PRIVS); >>> + int nosuid = (bprm->file->f_path.mnt->mnt_flags & MNT_NOSUID); >>> + int rc; >>> + >>> + if (!nnp && !nosuid) >>> + return 0; /* neither NNP nor nosuid */ >>> + >>> + if (new_tsec->sid == old_tsec->sid) >>> + return 0; /* No change in credentials */ >>> + >>> + rc = security_bounded_transition(old_tsec->sid, new_tsec->sid); >>> + if (rc == 0) >>> + return 0; /* allowed via bounded transition */ >> >> I think there might be an audit issue here; security_bounded_transition() will >> generate an audit record on failure which probably isn't what we want in this >> case. >> >> Other than that, this seems reasonable, even in the face of NNP, and as others >> point out, standard DAC capabilities do something similar. >> >>> + /* Only allow if dyntransition permission aka setcon() is allowed. */ >>> + rc = avc_has_perm(old_tsec->sid, new_tsec->sid, SECCLASS_PROCESS, >>> + PROCESS__DYNTRANSITION, NULL); >>> + if (rc) { >>> + if (nnp) >>> + return -EPERM; >>> + else >>> + return -EACCES; >>> + } >>> + return 0; >> >> I know this dyntransition/NNP/NOSUID interaction has been discussed quite a >> bit off-list (mostly while I was away, my apologies), but I haven't seen a lot >> on-list and while the descriptions hints at it the code itself doesn't >> elaborate on why this is "OK". I'd like to see some comments about why it is >> okay to allow the transition, regardless of NNP/NOSUID, if the dyntransition >> is allowed. I'd also like to see a quick comment about why we return -EPERM/- >> EACCES. >> >> There was a lot of discussion around these points and I want to make sure the >> ideas aren't lost. > > The claim was that dyntransition is sufficient since it would allow the > caller to setcon() to the new domain directly and therefore perform any > action in that domain. Thus, allowing it to transition to that domain > upon exec under NNP or on a file in a nosuid mount does not enable it to > do anything it could not already do directly. > > However, this presumes that the policy writer does not merely add > dyntransition to the caller domain upon encountering the avc denial in > this situation without thinking about the implications and deciding > whether to truly trust the caller domain in this way. Which is likely a > dangerous assumption, especially for people who write policy via > audit2allow. > > At least with the bounded transition, you have to explicitly declare > that the new domain is bounded by the caller domain and the kernel will > then enforce the restriction that the new domain is not granted any > permission not allowed to the caller domain. > How about making the change just for bounded transitions, then? No one will write the policy if the kernel doesn't support it. > I'm also unclear as to whether this in fact solves the actual problem > for sandbox -X. Dunno. dwalsh, does it? > > So I'd drop this patch for now at least. --Andy ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH v2] selinux: Permit exec transitions under NO_NEW_PRIVS or NOSUID under certain circumstances. 2014-06-23 18:25 ` Andy Lutomirski @ 2014-06-23 19:48 ` Daniel J Walsh 2014-06-23 19:52 ` Stephen Smalley 0 siblings, 1 reply; 11+ messages in thread From: Daniel J Walsh @ 2014-06-23 19:48 UTC (permalink / raw) To: Andy Lutomirski, Stephen Smalley; +Cc: SELinux-NSA We can use it to fix sandbox. sandbox -X xterm can do the following unconfined_t -> seunshare_t -> sandbox_x_t Then I allow sandbox_x_t to setcur to sandbox_x_client_t. sandbox_x_t can run the xserver, and the confined app will run as sandbox_x_client_t. If I am allowed to transition to a tighter domain, I can get the scripts to work. On 06/23/2014 02:25 PM, Andy Lutomirski wrote: > On Mon, Jun 23, 2014 at 10:23 AM, Stephen Smalley <sds@tycho.nsa.gov> wrote: >> On 06/19/2014 04:51 PM, Paul Moore wrote: >>> On Thursday, June 19, 2014 04:04:18 PM Paul Moore wrote: >>>> On Thursday, June 12, 2014 03:29:04 PM Stephen Smalley wrote: >>>>> v2 - fix patch description to match the code. >>>> Okay Stephen, I suppose you should get some special consideration, but is >>>> posting your patches inline really that hard :) >>>> >>>> --- >>>> From c1fa21950c5c3eb0dd97ae5145fa3d3f04adc5c4 Mon Sep 17 00:00:00 2001 >>>> From: Stephen Smalley <sds@tycho.nsa.gov> >>>> Date: Thu, 12 Jun 2014 08:17:48 -0400 >>>> Subject: [PATCH] selinux: Permit exec transitions under NO_NEW_PRIVS or >>>> NOSUID under certain circumstances. >>>> >>>> If the callee SID is bounded by the caller SID or if the caller SID is >>>> allowed to perform a dynamic transition (setcon) to the callee SID, then >>>> allowing the transition to occur poses no risk of privilege escalation and >>>> we can therefore safely allow the transition to occur. Add these two >>>> exemptions for both the case where a transition was explicitly requested by >>>> the application and the case where an automatic transition is defined in >>>> policy. >>>> >>>> Signed-off-by: Stephen Smalley <sds@tycho.nsa.gov> >>>> Acked-by: Andy Lutomirski <luto@amacapital.net> >>> Comments below ... >>> >>>> diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c >>>> index 83d06db..d5e8dc5 100644 >>>> --- a/security/selinux/hooks.c >>>> +++ b/security/selinux/hooks.c >>>> @@ -2086,6 +2086,36 @@ static int selinux_vm_enough_memory(struct mm_struct >>>> *mm, long pages) >>>> >>>> /* binprm security operations */ >>>> >>>> +static int check_nnp_nosuid(const struct linux_binprm *bprm, >>>> + const struct task_security_struct *old_tsec, >>>> + const struct task_security_struct *new_tsec) >>>> +{ >>>> + int nnp = (bprm->unsafe & LSM_UNSAFE_NO_NEW_PRIVS); >>>> + int nosuid = (bprm->file->f_path.mnt->mnt_flags & MNT_NOSUID); >>>> + int rc; >>>> + >>>> + if (!nnp && !nosuid) >>>> + return 0; /* neither NNP nor nosuid */ >>>> + >>>> + if (new_tsec->sid == old_tsec->sid) >>>> + return 0; /* No change in credentials */ >>>> + >>>> + rc = security_bounded_transition(old_tsec->sid, new_tsec->sid); >>>> + if (rc == 0) >>>> + return 0; /* allowed via bounded transition */ >>> I think there might be an audit issue here; security_bounded_transition() will >>> generate an audit record on failure which probably isn't what we want in this >>> case. >>> >>> Other than that, this seems reasonable, even in the face of NNP, and as others >>> point out, standard DAC capabilities do something similar. >>> >>>> + /* Only allow if dyntransition permission aka setcon() is allowed. */ >>>> + rc = avc_has_perm(old_tsec->sid, new_tsec->sid, SECCLASS_PROCESS, >>>> + PROCESS__DYNTRANSITION, NULL); >>>> + if (rc) { >>>> + if (nnp) >>>> + return -EPERM; >>>> + else >>>> + return -EACCES; >>>> + } >>>> + return 0; >>> I know this dyntransition/NNP/NOSUID interaction has been discussed quite a >>> bit off-list (mostly while I was away, my apologies), but I haven't seen a lot >>> on-list and while the descriptions hints at it the code itself doesn't >>> elaborate on why this is "OK". I'd like to see some comments about why it is >>> okay to allow the transition, regardless of NNP/NOSUID, if the dyntransition >>> is allowed. I'd also like to see a quick comment about why we return -EPERM/- >>> EACCES. >>> >>> There was a lot of discussion around these points and I want to make sure the >>> ideas aren't lost. >> The claim was that dyntransition is sufficient since it would allow the >> caller to setcon() to the new domain directly and therefore perform any >> action in that domain. Thus, allowing it to transition to that domain >> upon exec under NNP or on a file in a nosuid mount does not enable it to >> do anything it could not already do directly. >> >> However, this presumes that the policy writer does not merely add >> dyntransition to the caller domain upon encountering the avc denial in >> this situation without thinking about the implications and deciding >> whether to truly trust the caller domain in this way. Which is likely a >> dangerous assumption, especially for people who write policy via >> audit2allow. >> >> At least with the bounded transition, you have to explicitly declare >> that the new domain is bounded by the caller domain and the kernel will >> then enforce the restriction that the new domain is not granted any >> permission not allowed to the caller domain. >> > How about making the change just for bounded transitions, then? No > one will write the policy if the kernel doesn't support it. > >> I'm also unclear as to whether this in fact solves the actual problem >> for sandbox -X. > Dunno. dwalsh, does it? > >> So I'd drop this patch for now at least. > --Andy > _______________________________________________ > Selinux mailing list > Selinux@tycho.nsa.gov > To unsubscribe, send email to Selinux-leave@tycho.nsa.gov. > To get help, send an email containing "help" to Selinux-request@tycho.nsa.gov. > > ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH v2] selinux: Permit exec transitions under NO_NEW_PRIVS or NOSUID under certain circumstances. 2014-06-23 19:48 ` Daniel J Walsh @ 2014-06-23 19:52 ` Stephen Smalley 2014-06-24 12:42 ` Daniel J Walsh 0 siblings, 1 reply; 11+ messages in thread From: Stephen Smalley @ 2014-06-23 19:52 UTC (permalink / raw) To: Daniel J Walsh, Andy Lutomirski; +Cc: SELinux-NSA On 06/23/2014 03:48 PM, Daniel J Walsh wrote: > We can use it to fix sandbox. > > sandbox -X xterm can do the following > > unconfined_t -> seunshare_t -> sandbox_x_t Then I allow sandbox_x_t to > setcur to sandbox_x_client_t. sandbox_x_t can run the xserver, and the > confined app will run as sandbox_x_client_t. If I am allowed to > transition to a tighter domain, I can get the scripts to work. Which of those transitions are on exec and which ones are via explicit setcon() call? Would it work if you couldn't allow it via dyntransition but had to specify typebounds <callingdomain> <newdomain>; instead? > > On 06/23/2014 02:25 PM, Andy Lutomirski wrote: >> On Mon, Jun 23, 2014 at 10:23 AM, Stephen Smalley <sds@tycho.nsa.gov> wrote: >>> On 06/19/2014 04:51 PM, Paul Moore wrote: >>>> On Thursday, June 19, 2014 04:04:18 PM Paul Moore wrote: >>>>> On Thursday, June 12, 2014 03:29:04 PM Stephen Smalley wrote: >>>>>> v2 - fix patch description to match the code. >>>>> Okay Stephen, I suppose you should get some special consideration, but is >>>>> posting your patches inline really that hard :) >>>>> >>>>> --- >>>>> From c1fa21950c5c3eb0dd97ae5145fa3d3f04adc5c4 Mon Sep 17 00:00:00 2001 >>>>> From: Stephen Smalley <sds@tycho.nsa.gov> >>>>> Date: Thu, 12 Jun 2014 08:17:48 -0400 >>>>> Subject: [PATCH] selinux: Permit exec transitions under NO_NEW_PRIVS or >>>>> NOSUID under certain circumstances. >>>>> >>>>> If the callee SID is bounded by the caller SID or if the caller SID is >>>>> allowed to perform a dynamic transition (setcon) to the callee SID, then >>>>> allowing the transition to occur poses no risk of privilege escalation and >>>>> we can therefore safely allow the transition to occur. Add these two >>>>> exemptions for both the case where a transition was explicitly requested by >>>>> the application and the case where an automatic transition is defined in >>>>> policy. >>>>> >>>>> Signed-off-by: Stephen Smalley <sds@tycho.nsa.gov> >>>>> Acked-by: Andy Lutomirski <luto@amacapital.net> >>>> Comments below ... >>>> >>>>> diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c >>>>> index 83d06db..d5e8dc5 100644 >>>>> --- a/security/selinux/hooks.c >>>>> +++ b/security/selinux/hooks.c >>>>> @@ -2086,6 +2086,36 @@ static int selinux_vm_enough_memory(struct mm_struct >>>>> *mm, long pages) >>>>> >>>>> /* binprm security operations */ >>>>> >>>>> +static int check_nnp_nosuid(const struct linux_binprm *bprm, >>>>> + const struct task_security_struct *old_tsec, >>>>> + const struct task_security_struct *new_tsec) >>>>> +{ >>>>> + int nnp = (bprm->unsafe & LSM_UNSAFE_NO_NEW_PRIVS); >>>>> + int nosuid = (bprm->file->f_path.mnt->mnt_flags & MNT_NOSUID); >>>>> + int rc; >>>>> + >>>>> + if (!nnp && !nosuid) >>>>> + return 0; /* neither NNP nor nosuid */ >>>>> + >>>>> + if (new_tsec->sid == old_tsec->sid) >>>>> + return 0; /* No change in credentials */ >>>>> + >>>>> + rc = security_bounded_transition(old_tsec->sid, new_tsec->sid); >>>>> + if (rc == 0) >>>>> + return 0; /* allowed via bounded transition */ >>>> I think there might be an audit issue here; security_bounded_transition() will >>>> generate an audit record on failure which probably isn't what we want in this >>>> case. >>>> >>>> Other than that, this seems reasonable, even in the face of NNP, and as others >>>> point out, standard DAC capabilities do something similar. >>>> >>>>> + /* Only allow if dyntransition permission aka setcon() is allowed. */ >>>>> + rc = avc_has_perm(old_tsec->sid, new_tsec->sid, SECCLASS_PROCESS, >>>>> + PROCESS__DYNTRANSITION, NULL); >>>>> + if (rc) { >>>>> + if (nnp) >>>>> + return -EPERM; >>>>> + else >>>>> + return -EACCES; >>>>> + } >>>>> + return 0; >>>> I know this dyntransition/NNP/NOSUID interaction has been discussed quite a >>>> bit off-list (mostly while I was away, my apologies), but I haven't seen a lot >>>> on-list and while the descriptions hints at it the code itself doesn't >>>> elaborate on why this is "OK". I'd like to see some comments about why it is >>>> okay to allow the transition, regardless of NNP/NOSUID, if the dyntransition >>>> is allowed. I'd also like to see a quick comment about why we return -EPERM/- >>>> EACCES. >>>> >>>> There was a lot of discussion around these points and I want to make sure the >>>> ideas aren't lost. >>> The claim was that dyntransition is sufficient since it would allow the >>> caller to setcon() to the new domain directly and therefore perform any >>> action in that domain. Thus, allowing it to transition to that domain >>> upon exec under NNP or on a file in a nosuid mount does not enable it to >>> do anything it could not already do directly. >>> >>> However, this presumes that the policy writer does not merely add >>> dyntransition to the caller domain upon encountering the avc denial in >>> this situation without thinking about the implications and deciding >>> whether to truly trust the caller domain in this way. Which is likely a >>> dangerous assumption, especially for people who write policy via >>> audit2allow. >>> >>> At least with the bounded transition, you have to explicitly declare >>> that the new domain is bounded by the caller domain and the kernel will >>> then enforce the restriction that the new domain is not granted any >>> permission not allowed to the caller domain. >>> >> How about making the change just for bounded transitions, then? No >> one will write the policy if the kernel doesn't support it. >> >>> I'm also unclear as to whether this in fact solves the actual problem >>> for sandbox -X. >> Dunno. dwalsh, does it? >> >>> So I'd drop this patch for now at least. >> --Andy >> _______________________________________________ >> Selinux mailing list >> Selinux@tycho.nsa.gov >> To unsubscribe, send email to Selinux-leave@tycho.nsa.gov. >> To get help, send an email containing "help" to Selinux-request@tycho.nsa.gov. >> >> > > ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH v2] selinux: Permit exec transitions under NO_NEW_PRIVS or NOSUID under certain circumstances. 2014-06-23 19:52 ` Stephen Smalley @ 2014-06-24 12:42 ` Daniel J Walsh 0 siblings, 0 replies; 11+ messages in thread From: Daniel J Walsh @ 2014-06-24 12:42 UTC (permalink / raw) To: Stephen Smalley, Andy Lutomirski; +Cc: SELinux-NSA On 06/23/2014 03:52 PM, Stephen Smalley wrote: > On 06/23/2014 03:48 PM, Daniel J Walsh wrote: >> We can use it to fix sandbox. >> >> sandbox -X xterm can do the following >> >> unconfined_t -> seunshare_t -> sandbox_x_t Then I allow sandbox_x_t to >> setcur to sandbox_x_client_t. sandbox_x_t can run the xserver, and the >> confined app will run as sandbox_x_client_t. If I am allowed to >> transition to a tighter domain, I can get the scripts to work. > Which of those transitions are on exec and which ones are via explicit > setcon() call? seunshare will call setcon. sandbox_x_t would transition on exec when executing a new script or executing sandbox_file_t would transition to sandbox_x_client_t. sandbox_x_client_t would be a subset of sandbox_x_t, basically not able to talk to X. Of course if this was written in CIL it would be a lot easier to do. :^) > Would it work if you couldn't allow it via dyntransition but had to > specify typebounds <callingdomain> <newdomain>; > instead? > >> On 06/23/2014 02:25 PM, Andy Lutomirski wrote: >>> On Mon, Jun 23, 2014 at 10:23 AM, Stephen Smalley <sds@tycho.nsa.gov> wrote: >>>> On 06/19/2014 04:51 PM, Paul Moore wrote: >>>>> On Thursday, June 19, 2014 04:04:18 PM Paul Moore wrote: >>>>>> On Thursday, June 12, 2014 03:29:04 PM Stephen Smalley wrote: >>>>>>> v2 - fix patch description to match the code. >>>>>> Okay Stephen, I suppose you should get some special consideration, but is >>>>>> posting your patches inline really that hard :) >>>>>> >>>>>> --- >>>>>> From c1fa21950c5c3eb0dd97ae5145fa3d3f04adc5c4 Mon Sep 17 00:00:00 2001 >>>>>> From: Stephen Smalley <sds@tycho.nsa.gov> >>>>>> Date: Thu, 12 Jun 2014 08:17:48 -0400 >>>>>> Subject: [PATCH] selinux: Permit exec transitions under NO_NEW_PRIVS or >>>>>> NOSUID under certain circumstances. >>>>>> >>>>>> If the callee SID is bounded by the caller SID or if the caller SID is >>>>>> allowed to perform a dynamic transition (setcon) to the callee SID, then >>>>>> allowing the transition to occur poses no risk of privilege escalation and >>>>>> we can therefore safely allow the transition to occur. Add these two >>>>>> exemptions for both the case where a transition was explicitly requested by >>>>>> the application and the case where an automatic transition is defined in >>>>>> policy. >>>>>> >>>>>> Signed-off-by: Stephen Smalley <sds@tycho.nsa.gov> >>>>>> Acked-by: Andy Lutomirski <luto@amacapital.net> >>>>> Comments below ... >>>>> >>>>>> diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c >>>>>> index 83d06db..d5e8dc5 100644 >>>>>> --- a/security/selinux/hooks.c >>>>>> +++ b/security/selinux/hooks.c >>>>>> @@ -2086,6 +2086,36 @@ static int selinux_vm_enough_memory(struct mm_struct >>>>>> *mm, long pages) >>>>>> >>>>>> /* binprm security operations */ >>>>>> >>>>>> +static int check_nnp_nosuid(const struct linux_binprm *bprm, >>>>>> + const struct task_security_struct *old_tsec, >>>>>> + const struct task_security_struct *new_tsec) >>>>>> +{ >>>>>> + int nnp = (bprm->unsafe & LSM_UNSAFE_NO_NEW_PRIVS); >>>>>> + int nosuid = (bprm->file->f_path.mnt->mnt_flags & MNT_NOSUID); >>>>>> + int rc; >>>>>> + >>>>>> + if (!nnp && !nosuid) >>>>>> + return 0; /* neither NNP nor nosuid */ >>>>>> + >>>>>> + if (new_tsec->sid == old_tsec->sid) >>>>>> + return 0; /* No change in credentials */ >>>>>> + >>>>>> + rc = security_bounded_transition(old_tsec->sid, new_tsec->sid); >>>>>> + if (rc == 0) >>>>>> + return 0; /* allowed via bounded transition */ >>>>> I think there might be an audit issue here; security_bounded_transition() will >>>>> generate an audit record on failure which probably isn't what we want in this >>>>> case. >>>>> >>>>> Other than that, this seems reasonable, even in the face of NNP, and as others >>>>> point out, standard DAC capabilities do something similar. >>>>> >>>>>> + /* Only allow if dyntransition permission aka setcon() is allowed. */ >>>>>> + rc = avc_has_perm(old_tsec->sid, new_tsec->sid, SECCLASS_PROCESS, >>>>>> + PROCESS__DYNTRANSITION, NULL); >>>>>> + if (rc) { >>>>>> + if (nnp) >>>>>> + return -EPERM; >>>>>> + else >>>>>> + return -EACCES; >>>>>> + } >>>>>> + return 0; >>>>> I know this dyntransition/NNP/NOSUID interaction has been discussed quite a >>>>> bit off-list (mostly while I was away, my apologies), but I haven't seen a lot >>>>> on-list and while the descriptions hints at it the code itself doesn't >>>>> elaborate on why this is "OK". I'd like to see some comments about why it is >>>>> okay to allow the transition, regardless of NNP/NOSUID, if the dyntransition >>>>> is allowed. I'd also like to see a quick comment about why we return -EPERM/- >>>>> EACCES. >>>>> >>>>> There was a lot of discussion around these points and I want to make sure the >>>>> ideas aren't lost. >>>> The claim was that dyntransition is sufficient since it would allow the >>>> caller to setcon() to the new domain directly and therefore perform any >>>> action in that domain. Thus, allowing it to transition to that domain >>>> upon exec under NNP or on a file in a nosuid mount does not enable it to >>>> do anything it could not already do directly. >>>> >>>> However, this presumes that the policy writer does not merely add >>>> dyntransition to the caller domain upon encountering the avc denial in >>>> this situation without thinking about the implications and deciding >>>> whether to truly trust the caller domain in this way. Which is likely a >>>> dangerous assumption, especially for people who write policy via >>>> audit2allow. >>>> >>>> At least with the bounded transition, you have to explicitly declare >>>> that the new domain is bounded by the caller domain and the kernel will >>>> then enforce the restriction that the new domain is not granted any >>>> permission not allowed to the caller domain. >>>> >>> How about making the change just for bounded transitions, then? No >>> one will write the policy if the kernel doesn't support it. >>> >>>> I'm also unclear as to whether this in fact solves the actual problem >>>> for sandbox -X. >>> Dunno. dwalsh, does it? >>> >>>> So I'd drop this patch for now at least. >>> --Andy >>> _______________________________________________ >>> Selinux mailing list >>> Selinux@tycho.nsa.gov >>> To unsubscribe, send email to Selinux-leave@tycho.nsa.gov. >>> To get help, send an email containing "help" to Selinux-request@tycho.nsa.gov. >>> >>> >> > _______________________________________________ > Selinux mailing list > Selinux@tycho.nsa.gov > To unsubscribe, send email to Selinux-leave@tycho.nsa.gov. > To get help, send an email containing "help" to Selinux-request@tycho.nsa.gov. > > ^ permalink raw reply [flat|nested] 11+ messages in thread
end of thread, other threads:[~2014-06-24 12:42 UTC | newest] Thread overview: 11+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2014-06-12 19:18 [PATCH] selinux: Permit transitions under NO_NEW_PRIVS or NOSUID under certain, circumstances Stephen Smalley 2014-06-12 19:28 ` Andy Lutomirski 2014-06-12 19:29 ` [PATCH v2] selinux: Permit exec transitions under NO_NEW_PRIVS or NOSUID under certain circumstances Stephen Smalley 2014-06-16 15:25 ` [PATCH] selinux-testsuite: Add tests for exec transitions under NO_NEW_PRIVS Stephen Smalley 2014-06-19 20:04 ` [PATCH v2] selinux: Permit exec transitions under NO_NEW_PRIVS or NOSUID under certain circumstances Paul Moore 2014-06-19 20:51 ` Paul Moore 2014-06-23 17:23 ` Stephen Smalley 2014-06-23 18:25 ` Andy Lutomirski 2014-06-23 19:48 ` Daniel J Walsh 2014-06-23 19:52 ` Stephen Smalley 2014-06-24 12:42 ` Daniel J Walsh
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.