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

From: Stephen Smalley
Date: Tue May 16 2017 - 16:37:08 EST

Next message: Stefano Stabellini: "Re: [PATCH 05/18] xen/pvcalls: connect to a frontend"
Previous message: Chris Packham: "Re: [PATCH v4 2/3] hwmon: (adt7475) temperature smoothing"
In reply to: Sebastien Buisson: "[PATCH v5 2/2] selinux: expose policy brief via selinuxfs"
Next in thread: Sebastien Buisson: "Re: [PATCH v5 1/2] selinux: add brief info to policydb"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]

On Tue, 2017-05-16 at 18:51 +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. It is useful for any network or
> distributed file system that cares about how SELinux is enforced
> on its client nodes. This information is used to detect changes
> to the policy on file system client nodes, and can be forwarded
> to file system server nodes. Depending on how the policy is
> enforced on client side, server can refuse connection.
>
> Signed-off-by: Sebastien Buisson <sbuisson@xxxxxxx>
> ---
> Âinclude/linux/lsm_hooks.hÂÂÂÂÂÂÂÂÂÂÂ| 20 +++++++++
> Â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ÂÂÂÂÂÂ| 85
> +++++++++++++++++++++++++++++++++++++
> Âsecurity/selinux/ss/policydb.hÂÂÂÂÂÂ|ÂÂ3 ++
> Âsecurity/selinux/ss/services.cÂÂÂÂÂÂ| 67
> +++++++++++++++++++++++++++++
> Â9 files changed, 201 insertions(+)
>
> diff --git a/include/linux/lsm_hooks.h b/include/linux/lsm_hooks.h
> index 1aa6333..e4dc2b4 100644
> --- a/include/linux/lsm_hooks.h
> +++ b/include/linux/lsm_hooks.h
> @@ -1331,6 +1331,24 @@
> Â * @inode we wish to get the security context of.
> Â * @ctx is a pointer in which to place the allocated security
> context.
> Â * @ctxlen points to the place to put the length of @ctx.
> + *
> + * Security hooks for policy brief
> + *
> + * @policy_brief:
> + *
> + * Returns a string containing a brief info of the policydb.
> The string
> + * contains 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.
> + *
> + * @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
> + * If @alloc, *brief must be kfreed by caller.
> + * If not @alloc, caller must pass a buffer that can hold
> policy brief
> + * info (including terminating NUL).
> + * On success 0 is returned , or negative value on error.
> + *
> Â * This is the main security structure.
> Â */
> Â
> @@ -1562,6 +1580,7 @@
> Â int (*inode_setsecctx)(struct dentry *dentry, void *ctx, u32
> ctxlen);
> Â int (*inode_getsecctx)(struct inode *inode, void **ctx, u32
> *ctxlen);
> Â
> + int (*policy_brief)(char **brief, size_t *len, bool alloc);
> Â#ifdef CONFIG_SECURITY_NETWORK
> Â int (*unix_stream_connect)(struct sock *sock, struct sock
> *other,
> Â struct sock *newsk);
> @@ -1806,6 +1825,7 @@ struct security_hook_heads {
> Â struct list_head inode_notifysecctx;
> Â struct list_head inode_setsecctx;
> Â struct list_head inode_getsecctx;
> + struct list_head policy_brief;
> Â#ifdef CONFIG_SECURITY_NETWORK
> Â struct list_head unix_stream_connect;
> Â struct list_head unix_may_send;
> diff --git a/include/linux/security.h b/include/linux/security.h
> index 97df7ba..35ac97f 100644
> --- a/include/linux/security.h
> +++ b/include/linux/security.h
> @@ -376,6 +376,8 @@ int security_sem_semop(struct sem_array *sma,
> struct sembuf *sops,
> Âint security_inode_notifysecctx(struct inode *inode, void *ctx, u32
> ctxlen);
> Âint security_inode_setsecctx(struct dentry *dentry, void *ctx, u32
> ctxlen);
> Âint security_inode_getsecctx(struct inode *inode, void **ctx, u32
> *ctxlen);
> +
> +int security_policy_brief(char **brief, size_t *len, bool alloc);
> Â#else /* CONFIG_SECURITY */
> Âstruct security_mnt_opts {
> Â};
> @@ -1159,6 +1161,11 @@ static inline int
> security_inode_getsecctx(struct inode *inode, void **ctx, u32
> Â{
> Â return -EOPNOTSUPP;
> Â}
> +
> +static inline int security_policy_brief(char **brief, size_t *len,
> bool alloc)
> +{
> + return -EOPNOTSUPP;
> +}
> Â#endif /* CONFIG_SECURITY */
> Â
> Â#ifdef CONFIG_SECURITY_NETWORK
> diff --git a/security/security.c b/security/security.c
> index d6d18a3..46052d3 100644
> --- a/security/security.c
> +++ b/security/security.c
> @@ -1269,6 +1269,12 @@ int security_inode_getsecctx(struct inode
> *inode, void **ctx, u32 *ctxlen)
> Â}
> ÂEXPORT_SYMBOL(security_inode_getsecctx);
> Â
> +int security_policy_brief(char **brief, size_t *len, bool alloc)
> +{
> + return call_int_hook(policy_brief, -EOPNOTSUPP, brief, len,
> alloc);
> +}
> +EXPORT_SYMBOL(security_policy_brief);
> +
> Â#ifdef CONFIG_SECURITY_NETWORK
> Â
> Âint security_unix_stream_connect(struct sock *sock, struct sock
> *other, struct sock *newsk)
> @@ -1868,6 +1874,8 @@ struct security_hook_heads security_hook_heads
> __lsm_ro_after_init = {
> Â LIST_HEAD_INIT(security_hook_heads.inode_setsecctx),
> Â .inode_getsecctx =
> Â LIST_HEAD_INIT(security_hook_heads.inode_getsecctx),
> + .policy_brief =
> + LIST_HEAD_INIT(security_hook_heads.policy_brief),
> Â#ifdef CONFIG_SECURITY_NETWORK
> Â .unix_stream_connect =
> Â LIST_HEAD_INIT(security_hook_heads.unix_stream_conne
> ct),
> diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c
> index e67a526..da245e8 100644
> --- a/security/selinux/hooks.c
> +++ b/security/selinux/hooks.c
> @@ -6063,6 +6063,11 @@ static int selinux_inode_getsecctx(struct
> inode *inode, void **ctx, u32 *ctxlen)
> Â *ctxlen = len;
> Â return 0;
> Â}
> +
> +static int selinux_policy_brief(char **brief, size_t *len, bool
> alloc)
> +{
> + return security_policydb_brief(brief, len, alloc);
> +}
> Â#ifdef CONFIG_KEYS
> Â
> Âstatic int selinux_key_alloc(struct key *k, const struct cred *cred,
> @@ -6277,6 +6282,8 @@ static int selinux_key_getsecurity(struct key
> *key, char **_buffer)
> Â LSM_HOOK_INIT(inode_setsecctx, selinux_inode_setsecctx),
> Â LSM_HOOK_INIT(inode_getsecctx, selinux_inode_getsecctx),
> Â
> + LSM_HOOK_INIT(policy_brief, selinux_policy_brief),
> +
> Â LSM_HOOK_INIT(unix_stream_connect,
> selinux_socket_unix_stream_connect),
> Â LSM_HOOK_INIT(unix_may_send, selinux_socket_unix_may_send),
> Â
> diff --git a/security/selinux/include/security.h
> b/security/selinux/include/security.h
> index f979c35..a0d4d7d 100644
> --- a/security/selinux/include/security.h
> +++ b/security/selinux/include/security.h
> @@ -97,6 +97,8 @@ enum {
> Âint security_load_policy(void *data, size_t len);
> Âint security_read_policy(void **data, size_t *len);
> Âsize_t security_policydb_len(void);
> +int security_policydb_brief(char **brief, size_t *len, bool alloc);
> +void security_policydb_update_info(u32 requested);
> Â
> Âint security_policycap_supported(unsigned int req_cap);
> Â
> diff --git a/security/selinux/selinuxfs.c
> b/security/selinux/selinuxfs.c
> index ce71718..e8fe914 100644
> --- a/security/selinux/selinuxfs.c
> +++ b/security/selinux/selinuxfs.c
> @@ -159,6 +159,7 @@ static ssize_t sel_write_enforce(struct file
> *file, const char __user *buf,
> Â from_kuid(&init_user_ns,
> audit_get_loginuid(current)),
> Â audit_get_sessionid(current));
> Â selinux_enforcing = new_value;
> + security_policydb_update_info(SECURITY__SETENFORCE);
> Â if (selinux_enforcing)
> Â avc_ss_reset(0);
> Â selnl_notify_setenforce(selinux_enforcing);
> @@ -621,6 +622,7 @@ static ssize_t sel_write_checkreqprot(struct file
> *file, const char __user *buf,
> Â goto out;
> Â
> Â selinux_checkreqprot = new_value ? 1 : 0;
> + security_policydb_update_info(SECURITY__SETCHECKREQPROT);
> Â length = count;
> Âout:
> Â kfree(page);
> diff --git a/security/selinux/ss/policydb.c
> b/security/selinux/ss/policydb.c
> index 0080122..83309a4 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,49 @@ 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)
> +{
> + SHASH_DESC_ON_STACK(desc, policybrief_tfm);
> + struct policy_file *fp = ptr;
> + u8 *hashval;
> + int rc = 0;
> +
> + BUG_ON(policydb->policybrief);
> +
> + hashval = kmalloc(policybrief_hash_size, GFP_KERNEL);
> + if (hashval == NULL)
> + return -ENOMEM;
> +
> + 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);

kasprintf() calls vsnprintf() twice, once to compute the length and
once to output the string. But you already precomputed the length
during init in policybrief_len. So kmalloc() + snprintf() should
suffice. You can also then check that snprintf() returns the expected
value.

> + 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 +2290,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 +3513,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 + 1;
> +
> + return 0;
> +}
> +
> +late_initcall(init_policybrief_hash);
> diff --git a/security/selinux/ss/policydb.h
> b/security/selinux/ss/policydb.h
> index 725d594..2e5048c 100644
> --- a/security/selinux/ss/policydb.h
> +++ b/security/selinux/ss/policydb.h
> @@ -293,6 +293,8 @@ struct policydb {
> Â size_t len;
> Â
> Â unsigned int policyvers;
> + /* summary computed on the policy */
> + unsigned char *policybrief;
> Â
> Â unsigned int reject_unknown : 1;
> Â unsigned int allow_unknown : 1;
> @@ -309,6 +311,7 @@ struct policydb {
> Âextern int policydb_role_isvalid(struct policydb *p, unsigned int
> role);
> Âextern int policydb_read(struct policydb *p, void *fp);
> Âextern int policydb_write(struct policydb *p, void *fp);
> +extern unsigned int policybrief_len;
> Â
> Â#define PERM_SYMTAB_SIZE 32
> Â
> diff --git a/security/selinux/ss/services.c
> b/security/selinux/ss/services.c
> index 60d9b02..67eb80d 100644
> --- a/security/selinux/ss/services.c
> +++ b/security/selinux/ss/services.c
> @@ -2170,6 +2170,73 @@ 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
> + *
> + * If @alloc, *brief must be kfreed by caller.
> + * If not @alloc, caller must pass a buffer that can hold
> policybrief_len
> + * chars (including terminating NUL).
> + * 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 = kzalloc(policybrief_len, GFP_KERNEL);
> + } else if (*len < policybrief_len) {
> + /* put in *len the string size we need to write */
> + *len = policybrief_len;
> + return -ENAMETOOLONG;
> + }
> +
> + if (*brief == NULL)
> + return -ENOMEM;
> +
> + read_lock(&policy_rwlock);
> + strcpy(*brief, policydb.policybrief);
> + /* *len is the length of the output string */
> + *len = policybrief_len - 1;

Is there a particular reason to not just return policybrief_len here as
well, for consistency in the interface? How do you intend to use this
value in the caller?

> + 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

Next message: Stefano Stabellini: "Re: [PATCH 05/18] xen/pvcalls: connect to a frontend"
Previous message: Chris Packham: "Re: [PATCH v4 2/3] hwmon: (adt7475) temperature smoothing"
In reply to: Sebastien Buisson: "[PATCH v5 2/2] selinux: expose policy brief via selinuxfs"
Next in thread: Sebastien Buisson: "Re: [PATCH v5 1/2] selinux: add brief info to policydb"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]