Re: [PATCH v4 1/2] selinux: add brief info to policydb

[Stephen Smalley]

On Tue, 2017-05-16 at 03:22 +0900, Sebastien Buisson wrote:
> Add policybrief field to struct policydb. It holds a brief info
> of the policydb, made of colon separated name and value pairs
> that give information about how the policy is applied in the
> security module(s).
> Note that the ordering of the fields in the string may change.
> 
> Policy brief is computed every time the policy is loaded, and when
> enforce or checkreqprot are changed.
> 
> Add security_policy_brief hook to give access to policy brief to
> the rest of the kernel. Lustre client makes use of this information
> to detect changes to the policy, and forward it to Lustre servers.
> Depending on how the policy is enforced on Lustre client side,
> Lustre servers can refuse connection.
> 
> Signed-off-by: Sebastien Buisson <sbuisson@xxxxxxx>
> ---
> Âinclude/linux/lsm_hooks.hÂÂÂÂÂÂÂÂÂÂÂ| 17 +++++++
> Âinclude/linux/security.hÂÂÂÂÂÂÂÂÂÂÂÂ|ÂÂ7 +++
> Âsecurity/security.cÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ|ÂÂ8 ++++
> Âsecurity/selinux/hooks.cÂÂÂÂÂÂÂÂÂÂÂÂ|ÂÂ7 +++
> Âsecurity/selinux/include/security.h |ÂÂ2 +
> Âsecurity/selinux/selinuxfs.cÂÂÂÂÂÂÂÂ|ÂÂ2 +
> Âsecurity/selinux/ss/policydb.cÂÂÂÂÂÂ| 88
> +++++++++++++++++++++++++++++++++++++
> Âsecurity/selinux/ss/policydb.hÂÂÂÂÂÂ|ÂÂ3 ++
> Âsecurity/selinux/ss/services.cÂÂÂÂÂÂ| 68
> ++++++++++++++++++++++++++++
> Â9 files changed, 202 insertions(+)
> 

> diff --git a/security/selinux/ss/policydb.c
> b/security/selinux/ss/policydb.c
> index 0080122..8ce7f89 100644
> --- a/security/selinux/ss/policydb.c
> +++ b/security/selinux/ss/policydb.c
> @@ -32,13 +32,20 @@
> Â#include <linux/errno.h>
> Â#include <linux/audit.h>
> Â#include <linux/flex_array.h>
> +#include <crypto/hash.h>
> Â#include "security.h"
> Â
> Â#include "policydb.h"
> Â#include "conditional.h"
> Â#include "mls.h"
> +#include "objsec.h"
> Â#include "services.h"
> Â
> +static unsigned int policybrief_hash_size;
> +static struct crypto_shash *policybrief_tfm;
> +static const char policybrief_hash_alg[] = "sha256";
> +unsigned int policybrief_len;
> +
> Â#define _DEBUG_HASHES
> Â
> Â#ifdef DEBUG_HASHES
> @@ -879,6 +886,8 @@ void policydb_destroy(struct policydb *p)
> Â	ebitmap_destroy(&p->filename_trans_ttypes);
> Â	ebitmap_destroy(&p->policycaps);
> Â	ebitmap_destroy(&p->permissive_map);
> +
> +	kfree(p->policybrief);
> Â}
> Â
> Â/*
> @@ -2220,6 +2229,52 @@ static int ocontext_read(struct policydb *p,
> struct policydb_compat_info *info,
> Â}
> Â
> Â/*
> + * Compute summary of a policy database binary representation file,
> + * and store it into a policy database structure.
> + */
> +static int policydb_brief(struct policydb *policydb, void *ptr)
> +{
> +	struct policy_file *fp = ptr;
> +	u8 *hashval;
> +	int rc = 0;
> +
> +	if (policydb->policybrief)
> +		return -EINVAL;

Should this be a BUG_ON()?

> +
> +	hashval = kmalloc(policybrief_hash_size, GFP_KERNEL);
> +	if (hashval == NULL)
> +		return -ENOMEM;
> +
> +	{
> +		SHASH_DESC_ON_STACK(desc, policybrief_tfm);

You should be able to move this up to the parent scope now that
policybrief_tfm is allocated at init.  No need for a nested block then.

> +		desc->tfm = policybrief_tfm;
> +		desc->flags = 0;
> +		rc = crypto_shash_digest(desc, fp->data, fp->len,
> hashval);
> +		if (rc) {
> +			printk(KERN_ERR "Failed crypto_shash_digest:
> %d\n", rc);
> +			goto out_free;
> +		}
> +	}
> +
> +	/* policy brief is in the form:
> +	Â* selinux(enforce=<0 or 1>:checkreqprot=<0 or
> 1>:<hashalg>=<checksum>)
> +	Â*/
> +	policydb->policybrief = kasprintf(GFP_KERNEL,
> +					ÂÂ"selinux(enforce=%d:checkr
> eqprot=%d:%s=%*phN)",
> +					ÂÂselinux_enforcing,
> +					ÂÂselinux_checkreqprot,
> +					ÂÂpolicybrief_hash_alg,
> +					ÂÂpolicybrief_hash_size,
> hashval);
> +	if (policydb->policybrief == NULL)
> +		rc = -ENOMEM;
> +
> +out_free:
> +	kfree(hashval);
> +
> +	return rc;
> +}
> +
> +/*
> Â * Read the configuration data from a policy database binary
> Â * representation file into a policy database structure.
> Â */
> @@ -2238,6 +2293,11 @@ int policydb_read(struct policydb *p, void
> *fp)
> Â	if (rc)
> Â		return rc;
> Â
> +	/* Compute summary of policy, and store it in policydb */
> +	rc = policydb_brief(p, fp);
> +	if (rc)
> +		goto bad;
> +
> Â	/* Read the magic number and string length. */
> Â	rc = next_entry(buf, fp, sizeof(u32) * 2);
> Â	if (rc)
> @@ -3456,3 +3516,31 @@ int policydb_write(struct policydb *p, void
> *fp)
> Â
> Â	return 0;
> Â}
> +
> +static int __init init_policybrief_hash(void)
> +{
> +	struct crypto_shash *tfm;
> +
> +	if (!selinux_enabled)
> +		return 0;
> +
> +	tfm = crypto_alloc_shash(policybrief_hash_alg, 0, 0);
> +	if (IS_ERR(tfm)) {
> +		printk(KERN_ERR "Failed to alloc crypto hash %s\n",
> +		ÂÂÂÂÂÂÂpolicybrief_hash_alg);
> +		return PTR_ERR(tfm);
> +	}
> +
> +	policybrief_tfm = tfm;
> +	policybrief_hash_size =
> crypto_shash_digestsize(policybrief_tfm);
> +
> +	/* policy brief is in the form:
> +	Â* selinux(enforce=<0 or 1>:checkreqprot=<0 or
> 1>:<hashalg>=<checksum>)
> +	Â*/
> +	policybrief_len = 35 + strlen(policybrief_hash_alg) +
> +		2*policybrief_hash_size;

Might as well include the + 1 for the terminating NUL here?

> +
> +	return 0;
> +}
> +
> +late_initcall(init_policybrief_hash);

> diff --git a/security/selinux/ss/services.c
> b/security/selinux/ss/services.c
> index 60d9b02..32150fb 100644
> --- a/security/selinux/ss/services.c
> +++ b/security/selinux/ss/services.c
> @@ -2170,6 +2170,74 @@ size_t security_policydb_len(void)
> Â}
> Â
> Â/**
> + * security_policydb_brief - Get policydb brief
> + * @brief: pointer to buffer holding brief
> + * @len: in: brief buffer length if no alloc, out: brief string len
> + * @alloc: whether to allocate buffer for brief or not
> + *
> + * On success 0 is returned , or negative value on error.
> + **/
> +int security_policydb_brief(char **brief, size_t *len, bool alloc)
> +{
> +	if (!ss_initialized || brief == NULL)
> +		return -EINVAL;
> +
> +	if (alloc) {
> +		/* *brief must be kfreed by caller in this case */

What's the point in putting such a comment here in the code; it is
essentially interface documentation?

> +		*brief = kzalloc(policybrief_len + 1, GFP_KERNEL);
> +	/*
> +	Â* if !alloc, caller must pass a buffer that
> +	Â* can hold policybrief_len+1 chars
> +	Â*/

Ditto.  And why not just include the + 1 in the policybrief_len in the
first place?

> +	} else if (*len < policybrief_len + 1) {
> +		/* put in *len the string size we need to write */
> +		*len = policybrief_len;

The caller needs to allocate a buffer of policybrief_len+1, so might as
well include the +1 in the length we return to the caller.  Less
opportunity for error.

> +		return -ENAMETOOLONG;
> +	}
> +
> +	if (*brief == NULL)
> +		return -ENOMEM;
> +
> +	read_lock(&policy_rwlock);
> +	*len = policybrief_len;
> +	strncpy(*brief, policydb.policybrief, *len);

Why can't we just do a simple strcpy() here?

> +	read_unlock(&policy_rwlock);
> +
> +	return 0;
> +}
> +
> +void security_policydb_update_info(u32 requested)
> +{
> +	/* policy brief is in the form:
> +	Â* selinux(enforce=<0 or 1>:checkreqprot=<0 or
> 1>:<hashalg>=<checksum>)
> +	Â*/
> +	char enforce[] = "enforce=";
> +	char checkreqprot[] = "checkreqprot=";
> +	char *p, *str;
> +	int val;
> +
> +	if (!ss_initialized)
> +		return;
> +
> +	if (requested == SECURITY__SETENFORCE) {
> +		str = enforce;
> +		val = selinux_enforcing;
> +	} else if (requested == SECURITY__SETCHECKREQPROT) {
> +		str = checkreqprot;
> +		val = selinux_checkreqprot;
> +	}
> +
> +	/* update global policydb, needs write lock */
> +	write_lock_irq(&policy_rwlock);
> +	p = strstr(policydb.policybrief, str);
> +	if (p) {
> +		p += strlen(str);
> +		*p = '0' + val;
> +	}
> +	write_unlock_irq(&policy_rwlock);
> +}
> +
> +/**
> Â * security_port_sid - Obtain the SID for a port.
> Â * @protocol: protocol number
> Â * @port: port number