Re: [PATCH 9/9] Protect SELinux initialized state with pmalloc

[Stephen Smalley]

On 04/23/2018 08:54 AM, Igor Stoppa wrote:
> SELinux is one of the primary targets, when a system running it comes
> under attack.
> 
> The reason is that, even if an attacker ishould manage to gain root,
> SELinux will still prevent most desirable actions.
> 
> Even in a fully locked down system, SELinux still presents a vulnerability
> that is often exploited, because it is very simple to attack, once
> kernel address layout randomization has been defeated and the attacker
> has gained capability of writing to kernelunprotected data.
> 
> In various places, SELinux relies on an "initialized" internal state
> variable, to decide if the policy is loaded and tests should be
> performed. Needless to say, it's in the interest of hte attacker to turn
> it off and pretend that the policyDB is still uninitialized.
> 
> Even if recent patches move the "initialized" state inside a structure,
> it is still vulnerable.
> 
> This patch seeks to protect it, using it as demo for the pmalloc API,
> which is meant to provide additional protection to data which is likely
> to not be changed very often, if ever (after a transient).
> 
> The patch is probably in need of rework, to make it fit better with the
> new SELinux internal data structures, however it shows how to deny an
> easy target to the attacker.

I know this is just an example, but not sure why you wouldn't just protect the
entire selinux_state.  Note btw that the selinux_state encapsulation is preparatory work
for selinux namespaces [1], at which point the structure is in fact dynamically allocated
and there can be multiple instances of it.  That however is work-in-progress, highly experimental,
and might not ever make it upstream (if we can't resolve the various challenges it poses in a satisfactory
way).

[1] http://blog.namei.org/2018/01/22/lca-2018-kernel-miniconf-selinux-namespacing-slides/


> 
> In case the kernel is compiled with JOP safeguards, then it becomes far
> harder for the attacker to jump into the middle of the function which
> calls pmalloc_rare_write, to alter the state.
> 
> Signed-off-by: Igor Stoppa <igor.stoppa@xxxxxxxxxx>
> ---
>  security/selinux/hooks.c            | 12 ++++-----
>  security/selinux/include/security.h |  2 +-
>  security/selinux/ss/services.c      | 51 +++++++++++++++++++++++--------------
>  3 files changed, 39 insertions(+), 26 deletions(-)
> 
> diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c
> index 4cafe6a19167..6049f80115bc 100644
> --- a/security/selinux/hooks.c
> +++ b/security/selinux/hooks.c
> @@ -285,7 +285,7 @@ static int __inode_security_revalidate(struct inode *inode,
>  
>  	might_sleep_if(may_sleep);
>  
> -	if (selinux_state.initialized &&
> +	if (*ss_initialized_ptr &&
>  	    isec->initialized != LABEL_INITIALIZED) {
>  		if (!may_sleep)
>  			return -ECHILD;
> @@ -612,7 +612,7 @@ static int selinux_get_mnt_opts(const struct super_block *sb,
>  	if (!(sbsec->flags & SE_SBINITIALIZED))
>  		return -EINVAL;
>  
> -	if (!selinux_state.initialized)
> +	if (!*ss_initialized_ptr)
>  		return -EINVAL;
>  
>  	/* make sure we always check enough bits to cover the mask */
> @@ -735,7 +735,7 @@ static int selinux_set_mnt_opts(struct super_block *sb,
>  
>  	mutex_lock(&sbsec->lock);
>  
> -	if (!selinux_state.initialized) {
> +	if (!*ss_initialized_ptr) {
>  		if (!num_opts) {
>  			/* Defer initialization until selinux_complete_init,
>  			   after the initial policy is loaded and the security
> @@ -1022,7 +1022,7 @@ static int selinux_sb_clone_mnt_opts(const struct super_block *oldsb,
>  	 * if the parent was able to be mounted it clearly had no special lsm
>  	 * mount options.  thus we can safely deal with this superblock later
>  	 */
> -	if (!selinux_state.initialized)
> +	if (!*ss_initialized_ptr)
>  		return 0;
>  
>  	/*
> @@ -3040,7 +3040,7 @@ static int selinux_inode_init_security(struct inode *inode, struct inode *dir,
>  		isec->initialized = LABEL_INITIALIZED;
>  	}
>  
> -	if (!selinux_state.initialized || !(sbsec->flags & SBLABEL_MNT))
> +	if (!*ss_initialized_ptr || !(sbsec->flags & SBLABEL_MNT))
>  		return -EOPNOTSUPP;
>  
>  	if (name)
> @@ -7253,7 +7253,7 @@ static void selinux_nf_ip_exit(void)
>  #ifdef CONFIG_SECURITY_SELINUX_DISABLE
>  int selinux_disable(struct selinux_state *state)
>  {
> -	if (state->initialized) {
> +	if (*ss_initialized_ptr) {
>  		/* Not permitted after initial policy load. */
>  		return -EINVAL;
>  	}
> diff --git a/security/selinux/include/security.h b/security/selinux/include/security.h
> index 23e762d529fa..ec7debb143be 100644
> --- a/security/selinux/include/security.h
> +++ b/security/selinux/include/security.h
> @@ -96,13 +96,13 @@ extern char *selinux_policycap_names[__POLICYDB_CAPABILITY_MAX];
>  struct selinux_avc;
>  struct selinux_ss;
>  
> +extern bool *ss_initialized_ptr;
>  struct selinux_state {
>  	bool disabled;
>  #ifdef CONFIG_SECURITY_SELINUX_DEVELOP
>  	bool enforcing;
>  #endif
>  	bool checkreqprot;
> -	bool initialized;
>  	bool policycap[__POLICYDB_CAPABILITY_MAX];
>  	struct selinux_avc *avc;
>  	struct selinux_ss *ss;
> diff --git a/security/selinux/ss/services.c b/security/selinux/ss/services.c
> index 8057e19dc15f..c09ca6f9b269 100644
> --- a/security/selinux/ss/services.c
> +++ b/security/selinux/ss/services.c
> @@ -52,6 +52,7 @@
>  #include <linux/selinux.h>
>  #include <linux/flex_array.h>
>  #include <linux/vmalloc.h>
> +#include <linux/pmalloc.h>
>  #include <net/netlabel.h>
>  
>  #include "flask.h"
> @@ -80,10 +81,20 @@ char *selinux_policycap_names[__POLICYDB_CAPABILITY_MAX] = {
>  	"nnp_nosuid_transition"
>  };
>  
> +bool *ss_initialized_ptr __ro_after_init;
> +static struct pmalloc_pool *selinux_pool;
>  static struct selinux_ss selinux_ss;
>  
>  void selinux_ss_init(struct selinux_ss **ss)
>  {
> +	selinux_pool = pmalloc_create_pool(PMALLOC_RW);
> +	if (unlikely(!selinux_pool))
> +		panic("SELinux: unable to create pmalloc pool.");
> +	ss_initialized_ptr = pmalloc(selinux_pool, sizeof(bool));
> +	if (unlikely(!ss_initialized_ptr))
> +		panic("SElinux: unable to allocate from pmalloc pool.");
> +	*ss_initialized_ptr = false;
> +	pmalloc_protect_pool(selinux_pool);
>  	rwlock_init(&selinux_ss.policy_rwlock);
>  	mutex_init(&selinux_ss.status_lock);
>  	*ss = &selinux_ss;
> @@ -772,7 +783,7 @@ static int security_compute_validatetrans(struct selinux_state *state,
>  	int rc = 0;
>  
>  
> -	if (!state->initialized)
> +	if (!*ss_initialized_ptr)
>  		return 0;
>  
>  	read_lock(&state->ss->policy_rwlock);
> @@ -872,7 +883,7 @@ int security_bounded_transition(struct selinux_state *state,
>  	int index;
>  	int rc;
>  
> -	if (!state->initialized)
> +	if (!*ss_initialized_ptr)
>  		return 0;
>  
>  	read_lock(&state->ss->policy_rwlock);
> @@ -1032,7 +1043,7 @@ void security_compute_xperms_decision(struct selinux_state *state,
>  	memset(xpermd->dontaudit->p, 0, sizeof(xpermd->dontaudit->p));
>  
>  	read_lock(&state->ss->policy_rwlock);
> -	if (!state->initialized)
> +	if (!*ss_initialized_ptr)
>  		goto allow;
>  
>  	policydb = &state->ss->policydb;
> @@ -1121,7 +1132,7 @@ void security_compute_av(struct selinux_state *state,
>  	read_lock(&state->ss->policy_rwlock);
>  	avd_init(state, avd);
>  	xperms->len = 0;
> -	if (!state->initialized)
> +	if (!*ss_initialized_ptr)
>  		goto allow;
>  
>  	policydb = &state->ss->policydb;
> @@ -1175,7 +1186,7 @@ void security_compute_av_user(struct selinux_state *state,
>  
>  	read_lock(&state->ss->policy_rwlock);
>  	avd_init(state, avd);
> -	if (!state->initialized)
> +	if (!*ss_initialized_ptr)
>  		goto allow;
>  
>  	policydb = &state->ss->policydb;
> @@ -1294,7 +1305,7 @@ static int security_sid_to_context_core(struct selinux_state *state,
>  		*scontext = NULL;
>  	*scontext_len  = 0;
>  
> -	if (!state->initialized) {
> +	if (!*ss_initialized_ptr) {
>  		if (sid <= SECINITSID_NUM) {
>  			char *scontextp;
>  
> @@ -1466,7 +1477,7 @@ static int security_context_to_sid_core(struct selinux_state *state,
>  	if (!scontext2)
>  		return -ENOMEM;
>  
> -	if (!state->initialized) {
> +	if (!*ss_initialized_ptr) {
>  		int i;
>  
>  		for (i = 1; i < SECINITSID_NUM; i++) {
> @@ -1648,7 +1659,7 @@ static int security_compute_sid(struct selinux_state *state,
>  	int rc = 0;
>  	bool sock;
>  
> -	if (!state->initialized) {
> +	if (!*ss_initialized_ptr) {
>  		switch (orig_tclass) {
>  		case SECCLASS_PROCESS: /* kernel value */
>  			*out_sid = ssid;
> @@ -2128,7 +2139,8 @@ int security_load_policy(struct selinux_state *state, void *data, size_t len)
>  	policydb = &state->ss->policydb;
>  	sidtab = &state->ss->sidtab;
>  
> -	if (!state->initialized) {
> +	if (!*ss_initialized_ptr) {
> +		bool dummy_initialized = true;
>  		rc = policydb_read(policydb, fp);
>  		if (rc)
>  			goto out;
> @@ -2148,7 +2160,8 @@ int security_load_policy(struct selinux_state *state, void *data, size_t len)
>  		}
>  
>  		security_load_policycaps(state);
> -		state->initialized = 1;
> +		pmalloc_rare_write(selinux_pool, ss_initialized_ptr,
> +				   &dummy_initialized, sizeof(bool));
>  		seqno = ++state->ss->latest_granting;
>  		selinux_complete_init();
>  		avc_ss_reset(state->avc, seqno);
> @@ -2578,7 +2591,7 @@ int security_get_user_sids(struct selinux_state *state,
>  	*sids = NULL;
>  	*nel = 0;
>  
> -	if (!state->initialized)
> +	if (!*ss_initialized_ptr)
>  		goto out;
>  
>  	read_lock(&state->ss->policy_rwlock);
> @@ -2812,7 +2825,7 @@ int security_get_bools(struct selinux_state *state,
>  	struct policydb *policydb;
>  	int i, rc;
>  
> -	if (!state->initialized) {
> +	if (!*ss_initialized_ptr) {
>  		*len = 0;
>  		*names = NULL;
>  		*values = NULL;
> @@ -2987,7 +3000,7 @@ int security_sid_mls_copy(struct selinux_state *state,
>  	int rc;
>  
>  	rc = 0;
> -	if (!state->initialized || !policydb->mls_enabled) {
> +	if (!*ss_initialized_ptr || !policydb->mls_enabled) {
>  		*new_sid = sid;
>  		goto out;
>  	}
> @@ -3094,7 +3107,7 @@ int security_net_peersid_resolve(struct selinux_state *state,
>  	/*
>  	 * We don't need to check initialized here since the only way both
>  	 * nlbl_sid and xfrm_sid are not equal to SECSID_NULL would be if the
> -	 * security server was initialized and state->initialized was true.
> +	 * security server was initialized and *ss_initialized_ptr was true.
>  	 */
>  	if (!policydb->mls_enabled)
>  		return 0;
> @@ -3149,7 +3162,7 @@ int security_get_classes(struct selinux_state *state,
>  	struct policydb *policydb = &state->ss->policydb;
>  	int rc;
>  
> -	if (!state->initialized) {
> +	if (!*ss_initialized_ptr) {
>  		*nclasses = 0;
>  		*classes = NULL;
>  		return 0;
> @@ -3298,7 +3311,7 @@ int selinux_audit_rule_init(u32 field, u32 op, char *rulestr, void **vrule)
>  
>  	*rule = NULL;
>  
> -	if (!state->initialized)
> +	if (!*ss_initialized_ptr)
>  		return -EOPNOTSUPP;
>  
>  	switch (field) {
> @@ -3598,7 +3611,7 @@ int security_netlbl_secattr_to_sid(struct selinux_state *state,
>  	struct context *ctx;
>  	struct context ctx_new;
>  
> -	if (!state->initialized) {
> +	if (!*ss_initialized_ptr) {
>  		*sid = SECSID_NULL;
>  		return 0;
>  	}
> @@ -3665,7 +3678,7 @@ int security_netlbl_sid_to_secattr(struct selinux_state *state,
>  	int rc;
>  	struct context *ctx;
>  
> -	if (!state->initialized)
> +	if (!*ss_initialized_ptr)
>  		return 0;
>  
>  	read_lock(&state->ss->policy_rwlock);
> @@ -3704,7 +3717,7 @@ int security_read_policy(struct selinux_state *state,
>  	int rc;
>  	struct policy_file fp;
>  
> -	if (!state->initialized)
> +	if (!*ss_initialized_ptr)
>  		return -EINVAL;
>  
>  	*len = security_policydb_len(state);
>