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

From: Stephen Smalley
Date: Thu May 11 2017 - 16:18:30 EST

Next message: Guenter Roeck: "Re: Threads stuck in zap_pid_ns_processes()"
Previous message: Pavel Machek: "Re: [PATCH] x86/power/64: Use char arrays for asm function names"
In reply to: Casey Schaufler: "Re: [PATCH v3 1/2] selinux: add brief info to policydb"
Next in thread: Casey Schaufler: "Re: [PATCH v3 1/2] selinux: add brief info to policydb"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]

On Thu, 2017-05-11 at 08:56 -0700, Casey Schaufler wrote:
> On 5/11/2017 5:59 AM, Sebastien Buisson wrote:
> > Add policybrief field to struct policydb. It holds a brief info
> > of the policydb, in the following form:
> > <0 or 1 for enforce>:<0 or 1 for checkreqprot>:<hashalg>=<checksum>
> > 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ÂÂÂÂÂÂÂÂÂÂÂ| 16 ++++++++
> > Âinclude/linux/security.hÂÂÂÂÂÂÂÂÂÂÂÂ|ÂÂ7 ++++
> > Âsecurity/security.cÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ|ÂÂ6 +++
> > Âsecurity/selinux/hooks.cÂÂÂÂÂÂÂÂÂÂÂÂ|ÂÂ7 ++++
> > Âsecurity/selinux/include/security.h |ÂÂ2 +
> > Âsecurity/selinux/selinuxfs.cÂÂÂÂÂÂÂÂ|ÂÂ2 +
> > Âsecurity/selinux/ss/policydb.cÂÂÂÂÂÂ| 76
> > +++++++++++++++++++++++++++++++++++++
> > Âsecurity/selinux/ss/policydb.hÂÂÂÂÂÂ|ÂÂ2 +
> > Âsecurity/selinux/ss/services.cÂÂÂÂÂÂ| 62
> > ++++++++++++++++++++++++++++++
> > Â9 files changed, 180 insertions(+)
> >
> > diff --git a/include/linux/lsm_hooks.h b/include/linux/lsm_hooks.h
> > index 080f34e..9cac282 100644
> > --- a/include/linux/lsm_hooks.h
> > +++ b/include/linux/lsm_hooks.h
> > @@ -1336,6 +1336,20 @@
> > Â * @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, in the
> > + * following form:
> > + * <0 or 1 for enforce>:<0 or 1 for
> > checkreqprot>:<hashalg>=<checksum>
>
> This sure looks like SELinux specific information. If the Spiffy
> security module has multiple values for enforcement (e.g. off,
> soft and hard) this interface definition does not work. What is a
> "checkreqprot", and what is it for?
>
> I expect that you have no interest (or incentive) in supporting
> security modules other than SELinux, and that's OK. What's I'm
> after is an interface that another security module could use if
> someone where interested (or inspired) to do so.
>
> Rather than a string with predefined positional values (something
> I was taught not to do when 1 MIPS and 1 MEG was a big computer)
> you might use
> "enforce=<value>:checkreqprot=<value>:hashalg=<checksum>"

No objection to the above, although it makes his updating code for
enforce/checkreqprot a bit uglier.

> for SELinux and define @policy_brief as
>
> A string containing colon separated name and value pairs
> that will be parsed and interpreted by the security module
> or modules.

Actually, I'm not clear it will be parsed or interpreted by the
security module(s). I think he is just fetching it and then doing a
simple comparison to check for inconsistencies between clients and the
server, and optionally admins/users can read it and interpret it as
they see fit.

>
> You already have it right for the "hashalg" field. If you want to
> be really forward looking you could use names field names that
> identify the security module that uses them, such as
>
> "selinux/enforce=1:selinux/checkreqprot=0:selinux/MD5=8675309"

That seems a bit verbose, particularly the duplicated selinux/ bit.

>
> Note that I put "selinux/" onto the MD5 as well. You may have
> multiple modules that use this interface on a system at the same
> time in the not to distant future:
>
> selinux/enforce=1:selinux/checkreqprot=0:
> selinux/MD5=8675309:spiffy/enforce=soft:
> spiffy/updatefreq=32544:spiffy/SHA256=84521
>
> If you deal with this now it won't be a major headache to deal with
> later.
>
> > + *
> > + * @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.
> > + *
> > Â * This is the main security structure.
> > Â */
> > Â
> > @@ -1568,6 +1582,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);
> > @@ -1813,6 +1828,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 af675b5..3b72053 100644
> > --- a/include/linux/security.h
> > +++ b/include/linux/security.h
> > @@ -377,6 +377,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 {
> > Â};
> > @@ -1166,6 +1168,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 b9fea39..954b391 100644
> > --- a/security/security.c
> > +++ b/security/security.c
> > @@ -1285,6 +1285,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)
> > 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 50062e7..8c9f5b7 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..58e73f5 100644
> > --- a/security/selinux/ss/policydb.c
> > +++ b/security/selinux/ss/policydb.c
> > @@ -32,11 +32,13 @@
> > Â#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"
> > Â
> > Â#define _DEBUG_HASHES
> > @@ -879,6 +881,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 +2224,73 @@ 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;
> > + struct crypto_shash *tfm;
> > + char hashalg[] = "sha256";
> > + size_t hashsize;
> > + u8 *hashval;
> > + int idx;
> > + unsigned char *p;
> > +
> > + if (policydb->policybrief)
> > + return -EINVAL;
> > +
> > + tfm = crypto_alloc_shash(hashalg, 0, 0);
> > + if (IS_ERR(tfm)) {
> > + printk(KERN_ERR "Failed to alloc crypto hash
> > %s\n", hashalg);
> > + return PTR_ERR(tfm);
> > + }
> > +
> > + hashsize = crypto_shash_digestsize(tfm);
> > + hashval = kmalloc(hashsize, GFP_KERNEL);
> > + if (hashval == NULL) {
> > + crypto_free_shash(tfm);
> > + return -ENOMEM;
> > + }
> > +
> > + {
> > + int rc;
> > +
> > + SHASH_DESC_ON_STACK(desc, tfm);
> > + desc->tfm = tfm;
> > + desc->flags = 0;
> > + rc = crypto_shash_digest(desc, fp->data, fp->len,
> > hashval);
> > + crypto_free_shash(tfm);
> > + if (rc) {
> > + printk(KERN_ERR "Failed
> > crypto_shash_digest: %d\n", rc);
> > + kfree(hashval);
> > + return rc;
> > + }
> > + }
> > +
> > + /* policy brief is in the form:
> > + Â* <0 or 1 for enforce>:<0 or 1 for
> > checkreqprot>:<hashalg>=<checksum>
> > + Â*/
> > + policydb->policybrief = kzalloc(5 + strlen(hashalg) +
> > 2*hashsize + 1,
> > + GFP_KERNEL);
> > + if (policydb->policybrief == NULL) {
> > + kfree(hashval);
> > + return -ENOMEM;
> > + }
> > +
> > + sprintf(policydb->policybrief, "%d:%d:%s=",
> > + selinux_enforcing, selinux_checkreqprot, hashalg);
> > + p = policydb->policybrief + strlen(policydb->policybrief);
> > + for (idx = 0; idx < hashsize; idx++) {
> > + snprintf(p, 3, "%02x", hashval[idx]);
> > + p += 2;
> > + }
> > + kfree(hashval);
> > +
> > + return 0;
> > +}
> > +
> > +/*
> > Â * Read the configuration data from a policy database binary
> > Â * representation file into a policy database structure.
> > Â */
> > @@ -2238,6 +2309,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)
> > diff --git a/security/selinux/ss/policydb.h
> > b/security/selinux/ss/policydb.h
> > index 725d594..31689d2f 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;
> > diff --git a/security/selinux/ss/services.c
> > b/security/selinux/ss/services.c
> > index 60d9b02..3bbe649 100644
> > --- a/security/selinux/ss/services.c
> > +++ b/security/selinux/ss/services.c
> > @@ -2170,6 +2170,68 @@ 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)
> > +{
> > + size_t policybrief_len;
> > +
> > + if (!ss_initialized || brief == NULL)
> > + return -EINVAL;
> > +
> > + read_lock(&policy_rwlock);
> > + policybrief_len = strlen(policydb.policybrief);
> > + read_unlock(&policy_rwlock);
> > +
> > + if (alloc)
> > + /* *brief must be kfreed by caller in this case */
> > + *brief = kzalloc(policybrief_len + 1, GFP_KERNEL);
> > + else
> > + /*
> > + Â* if !alloc, caller must pass a buffer that
> > + Â* can hold policybrief_len+1 chars
> > + Â*/
> > + if (*len < policybrief_len + 1) {
> > + /* put in *len the string size we need to
> > write */
> > + *len = policybrief_len;
> > + return -ENAMETOOLONG;
> > + }
> > +
> > + if (*brief == NULL)
> > + return -ENOMEM;
> > +
> > + read_lock(&policy_rwlock);
> > + *len = strlen(policydb.policybrief);
> > + strncpy(*brief, policydb.policybrief, *len);
> > + read_unlock(&policy_rwlock);
> > +
> > + return 0;
> > +}
> > +
> > +void security_policydb_update_info(u32 requested)
> > +{
> > + /* policy brief is in the form:
> > + Â* <0 or 1 for enforce>:<0 or 1 for
> > checkreqprot>:<hashalg>=<checksum>
> > + Â*/
> > +
> > + if (!ss_initialized)
> > + return;
> > +
> > + /* update global policydb, needs write lock */
> > + write_lock_irq(&policy_rwlock);
> > + if (requested == SECURITY__SETENFORCE)
> > + policydb.policybrief[0] = '0' + selinux_enforcing;
> > + else if (requested == SECURITY__SETCHECKREQPROT)
> > + policydb.policybrief[2] = '0' +
> > selinux_checkreqprot;
> > + write_unlock_irq(&policy_rwlock);
> > +}
> > +
> > +/**
> > Â * security_port_sid - Obtain the SID for a port.
> > Â * @protocol: protocol number
> > Â * @port: port number

Next message: Guenter Roeck: "Re: Threads stuck in zap_pid_ns_processes()"
Previous message: Pavel Machek: "Re: [PATCH] x86/power/64: Use char arrays for asm function names"
In reply to: Casey Schaufler: "Re: [PATCH v3 1/2] selinux: add brief info to policydb"
Next in thread: Casey Schaufler: "Re: [PATCH v3 1/2] selinux: add brief info to policydb"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]