Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1640013 > unrolled thread

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

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  Re: [PATCH v3 1/2] selinux: add brief info to policydb Casey Schaufler <casey@schaufler-ca.com> - 2017-05-11 22:50 +0200
    RE: [PATCH v3 1/2] selinux: add brief info to policydb "Roberts, William C" <william.c.roberts@intel.com> - 2017-05-12 20:40 +0200
    Re: [PATCH v3 1/2] selinux: add brief info to policydb Paul Moore <paul@paul-moore.com> - 2017-05-13 00:30 +0200
      Re: [PATCH v3 1/2] selinux: add brief info to policydb William Roberts <bill.c.roberts@gmail.com> - 2017-05-13 01:00 +0200

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

On 5/11/2017 1:22 PM, Stephen Smalley wrote:
> 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@ddn.com>
>>> ---
>>>  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.

Sure, but can you imagine trying to use find(1) if the
options where positional?
 

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

OK, in which case human eyes *need* the name as well as the value.
That, and strcmp(value, "enforce=0") is no harder than strcmp(value, "0").

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

True that, on the other hand

	"selinux(enforce=1:checkreqprot=0:MD5=8675309)"

would be harder to parse. Either works for me.

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

[toc] | [next] | [standalone]

#1640682

> -----Original Message-----
> From: owner-linux-security-module@vger.kernel.org [mailto:owner-linux-
> security-module@vger.kernel.org] On Behalf Of Casey Schaufler
> Sent: Thursday, May 11, 2017 1:46 PM
> To: Stephen Smalley <sds@tycho.nsa.gov>; Sebastien Buisson
> <sbuisson.ddn@gmail.com>; linux-security-module@vger.kernel.org; linux-
> kernel@vger.kernel.org; selinux@tycho.nsa.gov
> Cc: Sebastien Buisson <sbuisson@ddn.com>; james.l.morris@oracle.com
> Subject: Re: [PATCH v3 1/2] selinux: add brief info to policydb
> 
> On 5/11/2017 1:22 PM, Stephen Smalley wrote:
> > 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@ddn.com>
> >>> ---
> >>>  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.
> 
> Sure, but can you imagine trying to use find(1) if the options where positional?
> 
> 
> >> 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.
> 
> OK, in which case human eyes *need* the name as well as the value.
> That, and strcmp(value, "enforce=0") is no harder than strcmp(value, "0").
> 
> >> 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.
> 
> True that, on the other hand
> 
> 	"selinux(enforce=1:checkreqprot=0:MD5=8675309)"
> 
> would be harder to parse. Either works for me.
> 
> >> 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) {

Kind of a weird way to do else if...

> >>> +			/* 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);

Isn't this: policybrief_len from above? No need to recompute.

> >>> +	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
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-security-module" in
> the body of a message to majordomo@vger.kernel.org More majordomo info at
> http://vger.kernel.org/majordomo-info.html

[toc] | [prev] | [next] | [standalone]

#1640790

On Thu, May 11, 2017 at 4:45 PM, Casey Schaufler <casey@schaufler-ca.com> wrote:
> On 5/11/2017 1:22 PM, Stephen Smalley wrote:
>> 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@ddn.com>
>>>> ---
>>>>  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.
>
> Sure, but can you imagine trying to use find(1) if the
> options where positional?

Perhaps I'm suffering from audit induced PTSD, but I think we need to
operate under the assumption that we are going to need to augment this
at some point in the future (no good feature goes un-abused) and I'd
much rather have some sort of name-value pairing to keep my sanity.  I
also feel rather strongly that we should make it very explicit in the
comments that the ordering of the fields in the string may change.

>>> 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.
>
> OK, in which case human eyes *need* the name as well as the value.
> That, and strcmp(value, "enforce=0") is no harder than strcmp(value, "0").

The initial use case may not require parsing the string, but who knows
what will end up using this five years from now.  Once again, I agree
with Casey, let's make sure it easily parsed and readable by admins;
imagine this as the output of a file under /proc or /sys.

>>> 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.
>
> True that, on the other hand
>
>         "selinux(enforce=1:checkreqprot=0:MD5=8675309)"
>
> would be harder to parse. Either works for me.

I'm not sure I care too much about how the name-value pairs get
namespaced, but considering the LSM stacking work that is happening,
we should namespace it somehow.

-- 
paul moore
www.paul-moore.com

[toc] | [prev] | [next] | [standalone]

#1640794

On Fri, May 12, 2017 at 3:22 PM, Paul Moore <paul@paul-moore.com> wrote:
>
> On Thu, May 11, 2017 at 4:45 PM, Casey Schaufler <casey@schaufler-ca.com> wrote:
> > On 5/11/2017 1:22 PM, Stephen Smalley wrote:
> >> 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@ddn.com>
> >>>> ---
> >>>>  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.
> >
> > Sure, but can you imagine trying to use find(1) if the
> > options where positional?
>
> Perhaps I'm suffering from audit induced PTSD, but I think we need to
> operate under the assumption that we are going to need to augment this
> at some point in the future (no good feature goes un-abused) and I'd
> much rather have some sort of name-value pairing to keep my sanity.  I
> also feel rather strongly that we should make it very explicit in the
> comments that the ordering of the fields in the string may change.
>
> >>> 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.
> >
> > OK, in which case human eyes *need* the name as well as the value.
> > That, and strcmp(value, "enforce=0") is no harder than strcmp(value, "0").
>
> The initial use case may not require parsing the string, but who knows
> what will end up using this five years from now.  Once again, I agree
> with Casey, let's make sure it easily parsed and readable by admins;
> imagine this as the output of a file under /proc or /sys.
>
> >>> 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.
> >
> > True that, on the other hand
> >
> >         "selinux(enforce=1:checkreqprot=0:MD5=8675309)"
> >
> > would be harder to parse. Either works for me.
>
> I'm not sure I care too much about how the name-value pairs get
> namespaced, but considering the LSM stacking work that is happening,
> we should namespace it somehow.
>
> --
> paul moore
> www.paul-moore.com

(Take 2, clicks plain text mode) Darn, I cleaned my inbox out, and
forgot to comment. Couldn't you replace the hex string conversion and
kzalloc
with asprintf and the kernel format specifier extension phN as
documented under Documentation/printk-formats.txt.

It looks like asprintf flows down into vsnprintf and into the
pointer() routine properly.

asprintf prototype:
http://elixir.free-electrons.com/linux/latest/source/include/linux/kernel.h#L426

As an aside, I wonder if audit can ditch there hex escaping routine
and use something like *pE
as mentioned in that Documentation under "Raw buffer as an escaped string:"

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web