On Fri, 28 Jan 2022 at 02:47, Paul Moore <paul@xxxxxxxxxxxxxx> wrote: > > On Thu, Jan 27, 2022 at 8:42 AM Chris PeBenito <pebenito@xxxxxxxx> wrote: > > On 1/26/22 17:51, Paul Moore wrote: > > > On Tue, Jan 25, 2022 at 9:59 AM Christian Göttsche > > > <cgzones@xxxxxxxxxxxxxx> wrote: > > >> > > >> In case a setuid or setgid binary is mislabeled with a generic context, > > >> either via a policy mistake or a move by the distribution package, > > >> executing it will be checked by the file permission execute_no_trans on > > >> the generic file context (e.g. bin_t). The setuid(2)/setgid(2) syscall > > >> within will then be checked against the unchanged caller process > > >> context, which might have been granted the capability permission setuid/ > > >> setgid to initially drop privileges. To avoid that scenario split the > > >> execute_no_trans permission in case of a setuid/setgid binary into a new > > >> permission execute_sxid_no_trans. > > >> > > >> For backward compatibility this behavior is contained in a new policy > > >> capability. > > >> > > >> Signed-off-by: Christian Göttsche <cgzones@xxxxxxxxxxxxxx> > > >> --- > > >> security/selinux/hooks.c | 9 ++++++++- > > >> security/selinux/include/classmap.h | 2 +- > > >> security/selinux/include/policycap.h | 1 + > > >> security/selinux/include/policycap_names.h | 3 ++- > > >> security/selinux/include/security.h | 8 ++++++++ > > >> 5 files changed, 20 insertions(+), 3 deletions(-) > > > > > > Adding the refpolicy list to this thread as their opinion seems > > > particularly relevant to this discussion. > > > > > > FWIW, this looks reasonable to me but I would like to hear what others > > > have to say. > > > > I think this a band-aid to cover up the real problem, which is the mislabeled files. > > It's hard to disagree with that, and the kernel is probably the wrong > place to apply a band-aid unless it is the only option left. > Adding a new datapoint to this RFC: An unprivileged user can via the setuid binary newgrp(1) write arbitrary content to files he can open in write mode but not actually write to, e.g. /proc/sys/kernel/ns_last_pid [1]. This also is reproducible on Fedora where /usr/bin/newgrp has the generic context bin_t, so the write (and capable check) happens in the caller context of unconfined_t. With the proposed permission split applied and instead of granting unconfined_t the new permission execute_sxid_no_trans but relying on an intermediate template domain $1_newgrp_t the access would have been denied, due to lack of permissions of the templated newgrp domain. With a generic file type of bin_t newgrp would not be executable for callers otherwise. Most setuid binaries are already labeled with a private type, newgrp and pkexec are the only ones left on a head-less Fedora system. One could still argue this is a policy neglect, but normally policy neglects result in missing permissions and reduced functionality, not in unintended privilege escalation. [1]: https://github.com/shadow-maint/shadow/pull/758 > > >> diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c > > >> index 5b6895e4fc29..b825fee39a70 100644 > > >> --- a/security/selinux/hooks.c > > >> +++ b/security/selinux/hooks.c > > >> @@ -2348,9 +2348,16 @@ static int selinux_bprm_creds_for_exec(struct linux_binprm *bprm) > > >> ad.u.file = bprm->file; > > >> > > >> if (new_tsec->sid == old_tsec->sid) { > > >> + u32 perm; > > >> + > > >> + if (selinux_policycap_execute_sxid_no_trans() && is_sxid(inode->i_mode)) > > >> + perm = FILE__EXECUTE_SXID_NO_TRANS; > > >> + else > > >> + perm = FILE__EXECUTE_NO_TRANS; > > >> + > > >> rc = avc_has_perm(&selinux_state, > > >> old_tsec->sid, isec->sid, > > >> - SECCLASS_FILE, FILE__EXECUTE_NO_TRANS, &ad); > > >> + SECCLASS_FILE, perm, &ad); > > >> if (rc) > > >> return rc; > > >> } else { > > >> diff --git a/security/selinux/include/classmap.h b/security/selinux/include/classmap.h > > >> index 35aac62a662e..53a1eeeb86fb 100644 > > >> --- a/security/selinux/include/classmap.h > > >> +++ b/security/selinux/include/classmap.h > > >> @@ -65,7 +65,7 @@ struct security_class_mapping secclass_map[] = { > > >> "quotaget", "watch", NULL } }, > > >> { "file", > > >> { COMMON_FILE_PERMS, > > >> - "execute_no_trans", "entrypoint", NULL } }, > > >> + "execute_no_trans", "entrypoint", "execute_sxid_no_trans", NULL } }, > > >> { "dir", > > >> { COMMON_FILE_PERMS, "add_name", "remove_name", > > >> "reparent", "search", "rmdir", NULL } }, > > >> diff --git a/security/selinux/include/policycap.h b/security/selinux/include/policycap.h > > >> index 2ec038efbb03..23929dc3e1db 100644 > > >> --- a/security/selinux/include/policycap.h > > >> +++ b/security/selinux/include/policycap.h > > >> @@ -11,6 +11,7 @@ enum { > > >> POLICYDB_CAPABILITY_CGROUPSECLABEL, > > >> POLICYDB_CAPABILITY_NNP_NOSUID_TRANSITION, > > >> POLICYDB_CAPABILITY_GENFS_SECLABEL_SYMLINKS, > > >> + POLICYDB_CAPABILITY_EXECUTE_SXID_NO_TRANS, > > >> __POLICYDB_CAPABILITY_MAX > > >> }; > > >> #define POLICYDB_CAPABILITY_MAX (__POLICYDB_CAPABILITY_MAX - 1) > > >> diff --git a/security/selinux/include/policycap_names.h b/security/selinux/include/policycap_names.h > > >> index b89289f092c9..4c014c2cf352 100644 > > >> --- a/security/selinux/include/policycap_names.h > > >> +++ b/security/selinux/include/policycap_names.h > > >> @@ -12,7 +12,8 @@ const char *selinux_policycap_names[__POLICYDB_CAPABILITY_MAX] = { > > >> "always_check_network", > > >> "cgroup_seclabel", > > >> "nnp_nosuid_transition", > > >> - "genfs_seclabel_symlinks" > > >> + "genfs_seclabel_symlinks", > > >> + "execute_sxid_no_trans", > > >> }; > > >> > > >> #endif /* _SELINUX_POLICYCAP_NAMES_H_ */ > > >> diff --git a/security/selinux/include/security.h b/security/selinux/include/security.h > > >> index ac0ece01305a..ab95241b6b7b 100644 > > >> --- a/security/selinux/include/security.h > > >> +++ b/security/selinux/include/security.h > > >> @@ -219,6 +219,14 @@ static inline bool selinux_policycap_genfs_seclabel_symlinks(void) > > >> return READ_ONCE(state->policycap[POLICYDB_CAPABILITY_GENFS_SECLABEL_SYMLINKS]); > > >> } > > >> > > >> +static inline bool selinux_policycap_execute_sxid_no_trans(void) > > >> +{ > > >> + struct selinux_state *state = &selinux_state; > > >> + > > >> + return READ_ONCE(state->policycap[POLICYDB_CAPABILITY_EXECUTE_SXID_NO_TRANS]); > > >> +} > > >> + > > >> + > > >> struct selinux_policy_convert_data; > > >> > > >> struct selinux_load_state { > > >> -- > > >> 2.34.1 > > >> > > > > > > > > > > > > -- > > Chris PeBenito > > > > -- > paul-moore.com
