| Message ID | 1495040944-11552-1-git-send-email-sbuisson@ddn.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Thu, 2017-05-18 at 02:09 +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@ddn.com> Looks good to me, but we need to see the patch that uses the LSM hook interface too. Also, I would split up this patch and fold your second patch into one of the parts. You could have a first patch that implements the support within SELinux and uses it from selinuxfs (no dependency on the LSM hook interface or the SELinux hook function), a second patch that adds the LSM hook interface and SELinux hook function to expose it outside of SELinux, and then a third patch to call the hook from lustre. > --- > include/linux/lsm_hooks.h | 20 +++++++++ > 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 | 88 > +++++++++++++++++++++++++++++++++++++ > security/selinux/ss/policydb.h | 3 ++ > security/selinux/ss/services.c | 67 > ++++++++++++++++++++++++++++ > 9 files changed, 202 insertions(+) > > diff --git a/include/linux/lsm_hooks.h b/include/linux/lsm_hooks.h > index 080f34e..0bc0260 100644 > --- a/include/linux/lsm_hooks.h > +++ b/include/linux/lsm_hooks.h > @@ -1336,6 +1336,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. > */ > > @@ -1568,6 +1586,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 +1832,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 54b1e39..91247fc 100644 > --- a/security/security.c > +++ b/security/security.c > @@ -1300,6 +1300,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 dddb81e..b6540f9 100644 > --- a/security/selinux/hooks.c > +++ b/security/selinux/hooks.c > @@ -6071,6 +6071,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, > @@ -6285,6 +6290,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 87d645d..b37b8e5 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 > @@ -874,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); > } > > /* > @@ -2215,6 +2224,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) > +{ > + SHASH_DESC_ON_STACK(desc, policybrief_tfm); > + struct policy_file *fp = ptr; > + u8 *hashval; > + int rc; > + > + 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 = kmalloc(policybrief_len, > GFP_KERNEL); > + if (policydb->policybrief == NULL) { > + rc = -ENOMEM; > + goto out_free; > + } > + rc = snprintf(policydb->policybrief, policybrief_len, > + "selinux(enforce=%d:checkreqprot=%d:%s=%*phN)" > , > + selinux_enforcing, selinux_checkreqprot, > + policybrief_hash_alg, policybrief_hash_size, > hashval); > + BUG_ON(rc >= policybrief_len); > + rc = 0; > + > +out_free: > + kfree(hashval); > + > + return rc; > +} > + > +/* > * Read the configuration data from a policy database binary > * representation file into a policy database structure. > */ > @@ -2233,6 +2288,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) > @@ -3451,3 +3511,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; > + 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 -- 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
On Wed, May 17, 2017 at 11:30 AM, Stephen Smalley <sds@tycho.nsa.gov> wrote: > On Thu, 2017-05-18 at 02:09 +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@ddn.com> > > Looks good to me, but we need to see the patch that uses the LSM hook > interface too. Also, I would split up this patch and fold your second > patch into one of the parts. You could have a first patch that > implements the support within SELinux and uses it from selinuxfs (no > dependency on the LSM hook interface or the SELinux hook function), a > second patch that adds the LSM hook interface and SELinux hook function > to expose it outside of SELinux, and then a third patch to call the > hook from lustre. The more I keep looking at this the more I dislike the interface. We're actually building an interface that defeats any LSM abstraction, and clients of the interface have to poll and parse to find out if something they care about changed. I think the interface should be inverted and provide an interface to register callbacks so clients can just set a callback and be notified of a changes. > >> --- >> include/linux/lsm_hooks.h | 20 +++++++++ >> 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 | 88 >> +++++++++++++++++++++++++++++++++++++ >> security/selinux/ss/policydb.h | 3 ++ >> security/selinux/ss/services.c | 67 >> ++++++++++++++++++++++++++++ >> 9 files changed, 202 insertions(+) >> >> diff --git a/include/linux/lsm_hooks.h b/include/linux/lsm_hooks.h >> index 080f34e..0bc0260 100644 >> --- a/include/linux/lsm_hooks.h >> +++ b/include/linux/lsm_hooks.h >> @@ -1336,6 +1336,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. >> */ >> >> @@ -1568,6 +1586,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 +1832,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 54b1e39..91247fc 100644 >> --- a/security/security.c >> +++ b/security/security.c >> @@ -1300,6 +1300,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 dddb81e..b6540f9 100644 >> --- a/security/selinux/hooks.c >> +++ b/security/selinux/hooks.c >> @@ -6071,6 +6071,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, >> @@ -6285,6 +6290,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 87d645d..b37b8e5 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 >> @@ -874,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); >> } >> >> /* >> @@ -2215,6 +2224,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) >> +{ >> + SHASH_DESC_ON_STACK(desc, policybrief_tfm); >> + struct policy_file *fp = ptr; >> + u8 *hashval; >> + int rc; >> + >> + 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 = kmalloc(policybrief_len, >> GFP_KERNEL); >> + if (policydb->policybrief == NULL) { >> + rc = -ENOMEM; >> + goto out_free; >> + } >> + rc = snprintf(policydb->policybrief, policybrief_len, >> + "selinux(enforce=%d:checkreqprot=%d:%s=%*phN)" >> , >> + selinux_enforcing, selinux_checkreqprot, >> + policybrief_hash_alg, policybrief_hash_size, >> hashval); >> + BUG_ON(rc >= policybrief_len); >> + rc = 0; >> + >> +out_free: >> + kfree(hashval); >> + >> + return rc; >> +} >> + >> +/* >> * Read the configuration data from a policy database binary >> * representation file into a policy database structure. >> */ >> @@ -2233,6 +2288,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) >> @@ -3451,3 +3511,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; >> + 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
On Wed, May 17, 2017 at 1:09 PM, Sebastien Buisson <sbuisson.ddn@gmail.com> 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@ddn.com> > --- > include/linux/lsm_hooks.h | 20 +++++++++ > 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 | 88 +++++++++++++++++++++++++++++++++++++ > security/selinux/ss/policydb.h | 3 ++ > security/selinux/ss/services.c | 67 ++++++++++++++++++++++++++++ > 9 files changed, 202 insertions(+) I agree with Stephen's comments regarding the patch structure, but I have a few additional comments below ... > diff --git a/security/selinux/ss/policydb.c b/security/selinux/ss/policydb.c > index 87d645d..b37b8e5 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; I'm not a fan of all these global variables, consider it a pet peeve of mine. I'm hopeful that we can drop policybrief_hash_size and policybrief_len, see my comments below. > #define _DEBUG_HASHES > > #ifdef DEBUG_HASHES > @@ -874,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); > } > > /* > @@ -2215,6 +2224,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) > +{ > + SHASH_DESC_ON_STACK(desc, policybrief_tfm); > + struct policy_file *fp = ptr; > + u8 *hashval; > + int rc; > + > + BUG_ON(policydb->policybrief); I prefer normal error checking (e.g. if statements) over BUG assertions whenever possible. > + 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>) > + */ See my comments a little later down about the brief format comments. > + policydb->policybrief = kmalloc(policybrief_len, GFP_KERNEL); > + if (policydb->policybrief == NULL) { > + rc = -ENOMEM; > + goto out_free; > + } > + rc = snprintf(policydb->policybrief, policybrief_len, > + "selinux(enforce=%d:checkreqprot=%d:%s=%*phN)", > + selinux_enforcing, selinux_checkreqprot, > + policybrief_hash_alg, policybrief_hash_size, hashval); > + BUG_ON(rc >= policybrief_len); This length check should definitely be a normal check and not a BUG_ON() assertion. > + rc = 0; > + > +out_free: > + kfree(hashval); > + > + return rc; > +} > + > +/* > * Read the configuration data from a policy database binary > * representation file into a policy database structure. > */ > @@ -2233,6 +2288,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) > @@ -3451,3 +3511,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>) > + */ Let's drop the exact brief format here and just point people at policydb_brief() where we describe the format in the functions comment block at the top. I like the thought behind this, but with multiple copies of the same information it is likely that they will fall out of sync at some point in the future. > + policybrief_len = 35 + strlen(policybrief_hash_alg) + > + 2*policybrief_hash_size + 1; This is another long term maintenance headache. I realize the comment above is supposed to make this easier, but I'm still going to have to count characters by hand whenever we play with this and that is problematic because I only have 10 fingers ;) How often is the Lustre kernel module going to be requesting the policy brief? If it is going to be often enough that a strlen() call is too costly, I think you may be better served by leveraging some of the LSM notification bits that were talked about earlier (e.g. the stuff from the IB patches) to reduce your need to update Lustre's copy of the brief. This was we can handle the brief length calculation locally in policydb_brief() and simply use a strlen() call in security_policydb_brief(). > + return 0; > +} > + > +late_initcall(init_policybrief_hash); ... > 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; > + 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>) > + */ See my earlier comments about the brief format. > + 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; > + } Turn the above into a switch/case statement and make the default return immediately. > + /* update global policydb, needs write lock */ > + write_lock_irq(&policy_rwlock); > + p = strstr(policydb.policybrief, str); > + if (p) { > + p += strlen(str); > + *p = '0' + val; To be overly cautious, let's remove the possibility for anything other than '0' or '1'. *p = (val ? '1' : '0'); > + } > + write_unlock_irq(&policy_rwlock); > +} > + > +/** > * security_port_sid - Obtain the SID for a port. > * @protocol: protocol number > * @port: port number > -- > 1.8.3.1
On Wed, 2017-05-17 at 18:19 -0400, Paul Moore wrote: > On Wed, May 17, 2017 at 1:09 PM, Sebastien Buisson > <sbuisson.ddn@gmail.com> 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@ddn.com> > > --- > > include/linux/lsm_hooks.h | 20 +++++++++ > > 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 | 88 > > +++++++++++++++++++++++++++++++++++++ > > security/selinux/ss/policydb.h | 3 ++ > > security/selinux/ss/services.c | 67 > > ++++++++++++++++++++++++++++ > > 9 files changed, 202 insertions(+) > > I agree with Stephen's comments regarding the patch structure, but I > have a few additional comments below ... > > > diff --git a/security/selinux/ss/policydb.c > > b/security/selinux/ss/policydb.c > > index 87d645d..b37b8e5 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; > > I'm not a fan of all these global variables, consider it a pet peeve > of mine. > > I'm hopeful that we can drop policybrief_hash_size and > policybrief_len, see my comments below. Hmm...well, they were introduced based on my comments on earlier versions of the patch (v2 and v3). They can be computed once during init and never change subsequently; they could even be __ro_after_init. > > #define _DEBUG_HASHES > > > > #ifdef DEBUG_HASHES > > @@ -874,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); > > } > > > > /* > > @@ -2215,6 +2224,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) > > +{ > > + SHASH_DESC_ON_STACK(desc, policybrief_tfm); > > + struct policy_file *fp = ptr; > > + u8 *hashval; > > + int rc; > > + > > + BUG_ON(policydb->policybrief); > > I prefer normal error checking (e.g. if statements) over BUG > assertions whenever possible. Technically, this truly would be a kernel-internal bug (not something a user could trigger apart from a kernel bug), since policydb_brief() is only supposed to be called once per policydb from policydb_read(). However, I could see just dropping this check altogether; we don't generally assert function preconditions like this. > > > + 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>) > > + */ > > See my comments a little later down about the brief format comments. > > > + policydb->policybrief = kmalloc(policybrief_len, > > GFP_KERNEL); > > + if (policydb->policybrief == NULL) { > > + rc = -ENOMEM; > > + goto out_free; > > + } > > + rc = snprintf(policydb->policybrief, policybrief_len, > > + "selinux(enforce=%d:checkreqprot=%d:%s=%*phN) > > ", > > + selinux_enforcing, selinux_checkreqprot, > > + policybrief_hash_alg, policybrief_hash_size, > > hashval); > > + BUG_ON(rc >= policybrief_len); > > This length check should definitely be a normal check and not a > BUG_ON() assertion. This one also would be a kernel-internal bug and would help catch inconsistencies between the policybrief_len computation and the actual string length (e.g. if someone changed the format string above without adjusting the length computation to match). It could be even stricter, e.g. BUG_ON(rc != (policybrief_len - 1)). I know that BUG_ON is generally frowned upon, but this case seems more justified, as it is a kernel-internal bug (not user triggerable apart from a kernel bug) and would catch an internal inconsistency. Returning a runtime error instead here would cause policy load to fail, but it wouldn't be the policy that was invalid but instead the kernel itself. That said, this may be obsoleted based on your other comments. > > > + rc = 0; > > + > > +out_free: > > + kfree(hashval); > > + > > + return rc; > > +} > > + > > +/* > > * Read the configuration data from a policy database binary > > * representation file into a policy database structure. > > */ > > @@ -2233,6 +2288,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) > > @@ -3451,3 +3511,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>) > > + */ > > Let's drop the exact brief format here and just point people at > policydb_brief() where we describe the format in the functions > comment > block at the top. I like the thought behind this, but with multiple > copies of the same information it is likely that they will fall out > of > sync at some point in the future. > > > + policybrief_len = 35 + strlen(policybrief_hash_alg) + > > + 2*policybrief_hash_size + 1; > > This is another long term maintenance headache. I realize the > comment > above is supposed to make this easier, but I'm still going to have to > count characters by hand whenever we play with this and that is > problematic because I only have 10 fingers ;) > > How often is the Lustre kernel module going to be requesting the > policy brief? If it is going to be often enough that a strlen() call > is too costly, I think you may be better served by leveraging some of > the LSM notification bits that were talked about earlier (e.g. the > stuff from the IB patches) to reduce your need to update Lustre's > copy > of the brief. This was we can handle the brief length calculation > locally in policydb_brief() and simply use a strlen() call in > security_policydb_brief(). v2 and v3 of the patch did essentially what you describe above (v2 had policydb_brief() store the length in the policydb for use by security_policydb_brief(), v3 just called strlen() in security_policydb_brief()). Both looked racy; they each took the policy read lock, extracted or computed the length, dropped the read lock, allocated the buffer, re-took the read lock, and copied out the brief. Since the length was stored in the policydb or computed each time, it looked like the length could change across a policy reload, in which case the code was unsafe. I noted that the length was in fact fixed (at least after init) and could be computed once during init. Then we don't need to store it in the policydb or recompute it each time, and we don't need to take the read lock twice or complicate the code to try to prevent something that cannot actually occur (a change in length at runtime). > > > + return 0; > > +} > > + > > +late_initcall(init_policybrief_hash); > > ... > > > 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; > > + 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>) > > + */ > > See my earlier comments about the brief format. > > > + 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; > > + } > > Turn the above into a switch/case statement and make the default > return immediately. > > > + /* update global policydb, needs write lock */ > > + write_lock_irq(&policy_rwlock); > > + p = strstr(policydb.policybrief, str); > > + if (p) { > > + p += strlen(str); > > + *p = '0' + val; > > To be overly cautious, let's remove the possibility for anything > other > than '0' or '1'. > > *p = (val ? '1' : '0'); > > > + } > > + write_unlock_irq(&policy_rwlock); > > +} > > + > > +/** > > * security_port_sid - Obtain the SID for a port. > > * @protocol: protocol number > > * @port: port number > > -- > > 1.8.3.1 > > -- 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
On Thu, 2017-05-18 at 10:01 -0400, Stephen Smalley wrote: > On Wed, 2017-05-17 at 18:19 -0400, Paul Moore wrote: > > On Wed, May 17, 2017 at 1:09 PM, Sebastien Buisson > > <sbuisson.ddn@gmail.com> 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@ddn.com> > > > --- > > > include/linux/lsm_hooks.h | 20 +++++++++ > > > 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 | 88 > > > +++++++++++++++++++++++++++++++++++++ > > > security/selinux/ss/policydb.h | 3 ++ > > > security/selinux/ss/services.c | 67 > > > ++++++++++++++++++++++++++++ > > > 9 files changed, 202 insertions(+) > > > > I agree with Stephen's comments regarding the patch structure, but > > I > > have a few additional comments below ... > > > > > diff --git a/security/selinux/ss/policydb.c > > > b/security/selinux/ss/policydb.c > > > index 87d645d..b37b8e5 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; > > > > I'm not a fan of all these global variables, consider it a pet > > peeve > > of mine. > > > > I'm hopeful that we can drop policybrief_hash_size and > > policybrief_len, see my comments below. > > Hmm...well, they were introduced based on my comments on earlier > versions of the patch (v2 and v3). They can be computed once during > init and never change subsequently; they could even be > __ro_after_init. > > > > #define _DEBUG_HASHES > > > > > > #ifdef DEBUG_HASHES > > > @@ -874,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); > > > } > > > > > > /* > > > @@ -2215,6 +2224,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) > > > +{ > > > + SHASH_DESC_ON_STACK(desc, policybrief_tfm); > > > + struct policy_file *fp = ptr; > > > + u8 *hashval; > > > + int rc; > > > + > > > + BUG_ON(policydb->policybrief); > > > > I prefer normal error checking (e.g. if statements) over BUG > > assertions whenever possible. > > Technically, this truly would be a kernel-internal bug (not something > a > user could trigger apart from a kernel bug), since policydb_brief() > is > only supposed to be called once per policydb from policydb_read(). > However, I could see just dropping this check altogether; we don't > generally assert function preconditions like this. > > > > > > + 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>) > > > + */ > > > > See my comments a little later down about the brief format > > comments. > > > > > + policydb->policybrief = kmalloc(policybrief_len, > > > GFP_KERNEL); > > > + if (policydb->policybrief == NULL) { > > > + rc = -ENOMEM; > > > + goto out_free; > > > + } > > > + rc = snprintf(policydb->policybrief, policybrief_len, > > > + "selinux(enforce=%d:checkreqprot=%d:%s=%*ph > > > N) > > > ", > > > + selinux_enforcing, selinux_checkreqprot, > > > + policybrief_hash_alg, > > > policybrief_hash_size, > > > hashval); > > > + BUG_ON(rc >= policybrief_len); > > > > This length check should definitely be a normal check and not a > > BUG_ON() assertion. > > This one also would be a kernel-internal bug and would help catch > inconsistencies between the policybrief_len computation and the > actual > string length (e.g. if someone changed the format string above > without > adjusting the length computation to match). It could be even > stricter, > e.g. BUG_ON(rc != (policybrief_len - 1)). > > I know that BUG_ON is generally frowned upon, but this case seems > more > justified, as it is a kernel-internal bug (not user triggerable apart > from a kernel bug) and would catch an internal inconsistency. > Returning > a runtime error instead here would cause policy load to fail, but it > wouldn't be the policy that was invalid but instead the kernel > itself. > That said, this may be obsoleted based on your other comments. > > > > > > + rc = 0; > > > + > > > +out_free: > > > + kfree(hashval); > > > + > > > + return rc; > > > +} > > > + > > > +/* > > > * Read the configuration data from a policy database binary > > > * representation file into a policy database structure. > > > */ > > > @@ -2233,6 +2288,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) > > > @@ -3451,3 +3511,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>) > > > + */ > > > > Let's drop the exact brief format here and just point people at > > policydb_brief() where we describe the format in the functions > > comment > > block at the top. I like the thought behind this, but with > > multiple > > copies of the same information it is likely that they will fall out > > of > > sync at some point in the future. > > > > > + policybrief_len = 35 + strlen(policybrief_hash_alg) + > > > + 2*policybrief_hash_size + 1; > > > > This is another long term maintenance headache. I realize the > > comment > > above is supposed to make this easier, but I'm still going to have > > to > > count characters by hand whenever we play with this and that is > > problematic because I only have 10 fingers ;) > > > > How often is the Lustre kernel module going to be requesting the > > policy brief? If it is going to be often enough that a strlen() > > call > > is too costly, I think you may be better served by leveraging some > > of > > the LSM notification bits that were talked about earlier (e.g. the > > stuff from the IB patches) to reduce your need to update Lustre's > > copy > > of the brief. This was we can handle the brief length calculation > > locally in policydb_brief() and simply use a strlen() call in > > security_policydb_brief(). > > v2 and v3 of the patch did essentially what you describe above (v2 > had > policydb_brief() store the length in the policydb for use by > security_policydb_brief(), v3 just called strlen() in > security_policydb_brief()). Both looked racy; they each took the > policy read lock, extracted or computed the length, dropped the read > lock, allocated the buffer, re-took the read lock, and copied out the > brief. Since the length was stored in the policydb or computed each > time, it looked like the length could change across a policy reload, > in > which case the code was unsafe. I noted that the length was in fact > fixed (at least after init) and could be computed once during init. > Then we don't need to store it in the policydb or recompute it each > time, and we don't need to take the read lock twice or complicate the > code to try to prevent something that cannot actually occur (a change > in length at runtime). So, if you truly want to go with the v2/v3 approach instead of this one, then I'd suggest: 1) Also using kasprintf() as in v4/v5. Then we don't need a separate length computation at all for the policydb_brief() allocation and there is no potential for inconsistency. I only recommended against using kasprintf() in v5 because we already had the length precomputed at init and therefore it wasn't buying us anything. 2) Compute the strlen() once in policydb_brief() and save it in policydb as in v2, to avoid having to recompute it each time in security_policydb_brief(). I only argued against that in v2 because I thought it should be done once during init, not that we should compute it each time in security_policydb_brief(). 3) In security_policydb_brief(), explicitly test that the length hasn't changed after taking the read lock again before copying and treat it at least as a runtime error if not a BUG if it has changed (it would again be a kernel-internal bug). I can somewhat see the argument for handling policybrief_len in this manner, since keeping the length computation in sync with the format string could be a headache. But I do think that policybrief_tfm and policybrief_hash_size should just be obtained once during init and made __ro_after_init. I don't see any downside to that from a maintenance perspective and it certainly simplifies the code and is more efficient. > > > > > > + return 0; > > > +} > > > + > > > +late_initcall(init_policybrief_hash); > > > > ... > > > > > 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; > > > + 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>) > > > + */ > > > > See my earlier comments about the brief format. > > > > > + 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; > > > + } > > > > Turn the above into a switch/case statement and make the default > > return immediately. > > > > > + /* update global policydb, needs write lock */ > > > + write_lock_irq(&policy_rwlock); > > > + p = strstr(policydb.policybrief, str); > > > + if (p) { > > > + p += strlen(str); > > > + *p = '0' + val; > > > > To be overly cautious, let's remove the possibility for anything > > other > > than '0' or '1'. > > > > *p = (val ? '1' : '0'); > > > > > + } > > > + write_unlock_irq(&policy_rwlock); > > > +} > > > + > > > +/** > > > * security_port_sid - Obtain the SID for a port. > > > * @protocol: protocol number > > > * @port: port number > > > -- > > > 1.8.3.1 > > > > -- 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
On Thu, May 18, 2017 at 11:07 AM, Stephen Smalley <sds@tycho.nsa.gov> wrote: > On Thu, 2017-05-18 at 10:01 -0400, Stephen Smalley wrote: >> On Wed, 2017-05-17 at 18:19 -0400, Paul Moore wrote: >> > On Wed, May 17, 2017 at 1:09 PM, Sebastien Buisson wrote: ... >> > > diff --git a/security/selinux/ss/policydb.c >> > > b/security/selinux/ss/policydb.c >> > > index 87d645d..b37b8e5 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; >> > >> > I'm not a fan of all these global variables, consider it a pet >> > peeve of mine. >> > >> > I'm hopeful that we can drop policybrief_hash_size and >> > policybrief_len, see my comments below. >> >> Hmm...well, they were introduced based on my comments on earlier >> versions of the patch (v2 and v3). They can be computed once during >> init and never change subsequently; they could even be >> __ro_after_init. My apologies to you and Sebastien for not reviewing these patches sooner. >> > > #define _DEBUG_HASHES >> > > >> > > #ifdef DEBUG_HASHES >> > > @@ -874,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); >> > > } >> > > >> > > /* >> > > @@ -2215,6 +2224,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) >> > > +{ >> > > + SHASH_DESC_ON_STACK(desc, policybrief_tfm); >> > > + struct policy_file *fp = ptr; >> > > + u8 *hashval; >> > > + int rc; >> > > + >> > > + BUG_ON(policydb->policybrief); >> > >> > I prefer normal error checking (e.g. if statements) over BUG >> > assertions whenever possible. >> >> Technically, this truly would be a kernel-internal bug (not something >> a user could trigger apart from a kernel bug), since policydb_brief() >> is only supposed to be called once per policydb from policydb_read(). >> However, I could see just dropping this check altogether; we don't >> generally assert function preconditions like this. My first take on this was the same, we really shouldn't need this check and I have a preference for removing it. However, policydb_brief() should be called infrequently enough that I'm not going to worry about an extra if conditional if Sebastien feels it is important. >> > >> > > + 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>) >> > > + */ >> > >> > See my comments a little later down about the brief format >> > comments. >> > >> > > + policydb->policybrief = kmalloc(policybrief_len, >> > > GFP_KERNEL); >> > > + if (policydb->policybrief == NULL) { >> > > + rc = -ENOMEM; >> > > + goto out_free; >> > > + } >> > > + rc = snprintf(policydb->policybrief, policybrief_len, >> > > + "selinux(enforce=%d:checkreqprot=%d:%s=%*ph >> > > N) >> > > ", >> > > + selinux_enforcing, selinux_checkreqprot, >> > > + policybrief_hash_alg, >> > > policybrief_hash_size, >> > > hashval); >> > > + BUG_ON(rc >= policybrief_len); >> > >> > This length check should definitely be a normal check and not a >> > BUG_ON() assertion. >> >> This one also would be a kernel-internal bug and would help catch >> inconsistencies between the policybrief_len computation and the >> actual string length (e.g. if someone changed the format string above >> without adjusting the length computation to match). It could be even >> stricter, e.g. BUG_ON(rc != (policybrief_len - 1)). >> >> I know that BUG_ON is generally frowned upon, but this case seems >> more justified, as it is a kernel-internal bug (not user triggerable apart >> from a kernel bug) and would catch an internal inconsistency. >> Returning a runtime error instead here would cause policy load to fail, but it >> wouldn't be the policy that was invalid but instead the kernel >> itself. >> That said, this may be obsoleted based on your other comments. Keep in mind that just because we do normal (not BUG) runtime checking it doesn't mean we need to return an error and fail the policy load simply writing a message to the ring buffer would be okay. >> > > +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>) >> > > + */ >> > >> > Let's drop the exact brief format here and just point people at >> > policydb_brief() where we describe the format in the functions >> > comment block at the top. I like the thought behind this, but with >> > multiple copies of the same information it is likely that they will fall out >> > of sync at some point in the future. >> > >> > > + policybrief_len = 35 + strlen(policybrief_hash_alg) + >> > > + 2*policybrief_hash_size + 1; >> > >> > This is another long term maintenance headache. I realize the >> > comment above is supposed to make this easier, but I'm still going to have >> > to count characters by hand whenever we play with this and that is >> > problematic because I only have 10 fingers ;) >> > >> > How often is the Lustre kernel module going to be requesting the >> > policy brief? If it is going to be often enough that a strlen() >> > call is too costly, I think you may be better served by leveraging some >> > of the LSM notification bits that were talked about earlier (e.g. the >> > stuff from the IB patches) to reduce your need to update Lustre's >> > copy of the brief. This was we can handle the brief length calculation >> > locally in policydb_brief() and simply use a strlen() call in >> > security_policydb_brief(). >> >> v2 and v3 of the patch did essentially what you describe above (v2 >> had policydb_brief() store the length in the policydb for use by >> security_policydb_brief(), v3 just called strlen() in >> security_policydb_brief()). Both looked racy; they each took the >> policy read lock, extracted or computed the length, dropped the read >> lock, allocated the buffer, re-took the read lock, and copied out the >> brief. Since the length was stored in the policydb or computed each >> time, it looked like the length could change across a policy reload, >> in which case the code was unsafe. I noted that the length was in fact >> fixed (at least after init) and could be computed once during init. >> Then we don't need to store it in the policydb or recompute it each >> time, and we don't need to take the read lock twice or complicate the >> code to try to prevent something that cannot actually occur (a change >> in length at runtime). I'm still interested in hearing exactly how this will be used by Lustre. Based on the way some of these hooks are designed I'm a little afraid that the Lustre kernel module is planning on hitting this quite frequently, which I don't think is a good thing, I'd much rather a notification system be used for something like that and only (re)fetching the policy brief when something has changed. In some ways it almost might be nice to have security_policydb_brief() always allocate the buffer to discourage frequent use. It would simplify the code quite a bit as well ... although it would require the notification system, but I think we are going to need that for IB anyway. > So, if you truly want to go with the v2/v3 approach instead of this > one, then I'd suggest: > 1) Also using kasprintf() as in v4/v5. Then we don't need a separate > length computation at all for the policydb_brief() allocation and there > is no potential for inconsistency. I only recommended against using > kasp; Irintf() in v5 because we already had the length precomputed at init > and therefore it wasn't buying us anything. Agreed. It makes sense. > 2) Compute the strlen() once in policydb_brief() and save it in > policydb as in v2, to avoid having to recompute it each time in > security_policydb_brief(). I only argued against that in v2 because I > thought it should be done once during init, not that we should compute > it each time in security_policydb_brief(). This is one area where understanding the callers would be helpful. > 3) In security_policydb_brief(), explicitly test that the length hasn't > changed after taking the read lock again before copying and treat it at > least as a runtime error if not a BUG if it has changed (it would again > be a kernel-internal bug). We shouldn't use BUG in security_policydb_brief() as we can safely return an error here. Once again my comments are dependent on the caller, but I share your concerns about race conditions and think we can workaround those problems in security_policydb_brief() through the use of kstrdup(GFP_ATOMIC) (the buffer should be small enough, and used infrequently enough that this should be okay) and if the buffer is provided, strscpy(). > I can somewhat see the argument for handling policybrief_len in this > manner, since keeping the length computation in sync with the format > string could be a headache. But I do think that policybrief_tfm and > policybrief_hash_size should just be obtained once during init and made > __ro_after_init. I don't see any downside to that from a maintenance > perspective and it certainly simplifies the code and is more efficient. I understand the need to fetch/allocate the policybrief_tfm value during init, no arguments there, but everything else is less clear to me at the moment (see comments above).
Hi,
2017-05-18 23:49 GMT+02:00 Paul Moore <paul@paul-moore.com>:
> My apologies to you and Sebastien for not reviewing these patches sooner.
It is ok, no problem.
Thanks for all the advice from you and Stephen. I will try to take all
this into account.
As I understand it, I should not give the choice to allocate or not
the string returned by security_policydb_brief(). The initial reason
for this was that Lustre client code is expected to retrieve policy
brief info hundreds or thousands of times per second, so saving on
memory allocation would make sense. So if security_policydb_brief()
necessarily allocates memory for the string returned, and I appreciate
it helps maintenance and avoids complexity, it should not be called so
often.
One way to tackle this is to rely on the notification system: Lustre
client code would call security_policydb_brief() only when it gets a
change notification, and stores the current policy brief info
internally.
Another way could be to add another hook to check policy brief info
validity. It would take a string as an input parameter, and return 0
if it matches the current policy. So Lustre client code would
systematically call this hook, and only call security_policydb_brief()
when the policy has changed, to store the current value internally.
I have recently identified a new need from Lustre client code. We need
to protect against the case where the policy is changed or set in
permissive mode, and then set back to its previous state, to
workaround policy check as carried out on server side based on policy
brief info sent by client. In this scenario, the policy would only be
the expected one by the time the client sends a request to the server
(for instance a file open request), but not after that when SELinux
actually checks the permissions on the client (via
security_file_open() in this example).
A solution to address this could be to add a new parameter to
security_policydb_brief() hook, in the form of a pointer to an integer
giving the current sequence number of the policy. That would
complement the policy brief info, with the notion of change to the
policy. I do not think it is desirable to include the sequence number
in the policy brief info, as it is not the essence of the policy.
Now with this sequence info in mind, the new hook to check policy
brief info validity would only need to check the sequence, instead of
the policy brief string. The current value of the sequence info should
be stored by Lustre internally, and checked after SELinux permission
checks. If a change is detected, Lustre client must stop normal
processing and return an error for the current request.
Thanks,
Sebastien.
--
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
On Tue, May 23, 2017 at 12:29 PM, Sebastien Buisson <sbuisson.ddn@gmail.com> wrote: > Hi, Hello. > 2017-05-18 23:49 GMT+02:00 Paul Moore <paul@paul-moore.com>: >> My apologies to you and Sebastien for not reviewing these patches sooner. > > It is ok, no problem. > Thanks for all the advice from you and Stephen. I will try to take all > this into account. Thanks for your patience and understanding. > As I understand it, I should not give the choice to allocate or not > the string returned by security_policydb_brief(). The initial reason > for this was that Lustre client code is expected to retrieve policy > brief info hundreds or thousands of times per second, so saving on > memory allocation would make sense. So if security_policydb_brief() > necessarily allocates memory for the string returned, and I appreciate > it helps maintenance and avoids complexity, it should not be called so > often. > One way to tackle this is to rely on the notification system: Lustre > client code would call security_policydb_brief() only when it gets a > change notification, and stores the current policy brief info > internally. This approach would not only save on the memory allocation, but it would also save on the memory copy. Granted, it's not a large chunk of memory, but it is something. FWIW, the notification code is now present in the selinux/next branch, see commit 8f408ab64be6 ("selinux lsm IB/core: Implement LSM notification system"); although you may need to augment the SELinux code to generate a LSM_POLICY_CHANGE event when the state of checkreqprot changes. Presumably you could also update the LSM_POLICY_CHANGE event so it sends the policy brief as the lone argument to save you an additional call into the LSM. > Another way could be to add another hook to check policy brief info > validity. It would take a string as an input parameter, and return 0 > if it matches the current policy. So Lustre client code would > systematically call this hook, and only call security_policydb_brief() > when the policy has changed, to store the current value internally. I'm not sure I like this approach as much as the one above, for a variety of reasons. Is this option more desirable from a Lustre point of view? > I have recently identified a new need from Lustre client code. We need > to protect against the case where the policy is changed or set in > permissive mode, and then set back to its previous state, to > workaround policy check as carried out on server side based on policy > brief info sent by client. In this scenario, the policy would only be > the expected one by the time the client sends a request to the server > (for instance a file open request), but not after that when SELinux > actually checks the permissions on the client (via > security_file_open() in this example). I'm not sure I completely understand what you are trying to describe, but more information is below. > A solution to address this could be to add a new parameter to > security_policydb_brief() hook, in the form of a pointer to an integer > giving the current sequence number of the policy. That would > complement the policy brief info, with the notion of change to the > policy. I do not think it is desirable to include the sequence number > in the policy brief info, as it is not the essence of the policy. > Now with this sequence info in mind, the new hook to check policy > brief info validity would only need to check the sequence, instead of > the policy brief string. The current value of the sequence info should > be stored by Lustre internally, and checked after SELinux permission > checks. If a change is detected, Lustre client must stop normal > processing and return an error for the current request. Assuming that the Lustre code makes use of the LSM notification system, it seems reasonable that Lustre could maintain it's own sequence number (or whatever best suits the problem) and increment that number whenever it receives a LSM_POLICY_CHANGE notification.
On Tue, 2017-05-23 at 18:29 +0200, Sebastien Buisson wrote: > Hi, > > 2017-05-18 23:49 GMT+02:00 Paul Moore <paul@paul-moore.com>: > > My apologies to you and Sebastien for not reviewing these patches > > sooner. > > It is ok, no problem. > Thanks for all the advice from you and Stephen. I will try to take > all > this into account. > > As I understand it, I should not give the choice to allocate or not > the string returned by security_policydb_brief(). The initial reason > for this was that Lustre client code is expected to retrieve policy > brief info hundreds or thousands of times per second, so saving on > memory allocation would make sense. So if security_policydb_brief() > necessarily allocates memory for the string returned, and I > appreciate > it helps maintenance and avoids complexity, it should not be called > so > often. > One way to tackle this is to rely on the notification system: Lustre > client code would call security_policydb_brief() only when it gets a > change notification, and stores the current policy brief info > internally. > Another way could be to add another hook to check policy brief info > validity. It would take a string as an input parameter, and return 0 > if it matches the current policy. So Lustre client code would > systematically call this hook, and only call > security_policydb_brief() > when the policy has changed, to store the current value internally. > > I have recently identified a new need from Lustre client code. We > need > to protect against the case where the policy is changed or set in > permissive mode, and then set back to its previous state, to > workaround policy check as carried out on server side based on policy > brief info sent by client. In this scenario, the policy would only be > the expected one by the time the client sends a request to the server > (for instance a file open request), but not after that when SELinux > actually checks the permissions on the client (via > security_file_open() in this example). > A solution to address this could be to add a new parameter to > security_policydb_brief() hook, in the form of a pointer to an > integer > giving the current sequence number of the policy. That would > complement the policy brief info, with the notion of change to the > policy. I do not think it is desirable to include the sequence number > in the policy brief info, as it is not the essence of the policy. > Now with this sequence info in mind, the new hook to check policy > brief info validity would only need to check the sequence, instead of > the policy brief string. The current value of the sequence info > should > be stored by Lustre internally, and checked after SELinux permission > checks. If a change is detected, Lustre client must stop normal > processing and return an error for the current request. Not sure about your threat model but I think you are fighting a losing battle there. A malicious admin has too many ways to defeat your checking. Relying on the seqno also seems brittle; you could easily end up causing a client to fail just because a policy update happened to be installed at the same time, even though there was nothing wrong or malicious about the policy update itself. -- 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
2017-05-23 21:11 GMT+02:00 Paul Moore <paul@paul-moore.com>: > On Tue, May 23, 2017 at 12:29 PM, Sebastien Buisson > <sbuisson.ddn@gmail.com> wrote: >> Another way could be to add another hook to check policy brief info >> validity. It would take a string as an input parameter, and return 0 >> if it matches the current policy. So Lustre client code would >> systematically call this hook, and only call security_policydb_brief() >> when the policy has changed, to store the current value internally. > > I'm not sure I like this approach as much as the one above, for a > variety of reasons. Is this option more desirable from a Lustre point > of view? It is true that now that the notification code is present in the selinux/next branch, it is worth using it. I was thinking, but I may be wrong, that future inclusion of this series of patches in some distributions' kernels like CentOS or RedHat would be easier if it did not have dependencies on other patches. This is why I thought about an alternative solution. Technically speaking, the solution based on notifications can fit the Lustre needs, letting Lustre maintain its own sequence number as you suggest. Sebastien. -- 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
2017-05-23 21:53 GMT+02:00 Stephen Smalley <sds@tycho.nsa.gov>: > On Tue, 2017-05-23 at 18:29 +0200, Sebastien Buisson wrote: >> Hi, >> >> 2017-05-18 23:49 GMT+02:00 Paul Moore <paul@paul-moore.com>: >> > My apologies to you and Sebastien for not reviewing these patches >> > sooner. >> >> It is ok, no problem. >> Thanks for all the advice from you and Stephen. I will try to take >> all >> this into account. >> >> As I understand it, I should not give the choice to allocate or not >> the string returned by security_policydb_brief(). The initial reason >> for this was that Lustre client code is expected to retrieve policy >> brief info hundreds or thousands of times per second, so saving on >> memory allocation would make sense. So if security_policydb_brief() >> necessarily allocates memory for the string returned, and I >> appreciate >> it helps maintenance and avoids complexity, it should not be called >> so >> often. >> One way to tackle this is to rely on the notification system: Lustre >> client code would call security_policydb_brief() only when it gets a >> change notification, and stores the current policy brief info >> internally. >> Another way could be to add another hook to check policy brief info >> validity. It would take a string as an input parameter, and return 0 >> if it matches the current policy. So Lustre client code would >> systematically call this hook, and only call >> security_policydb_brief() >> when the policy has changed, to store the current value internally. >> >> I have recently identified a new need from Lustre client code. We >> need >> to protect against the case where the policy is changed or set in >> permissive mode, and then set back to its previous state, to >> workaround policy check as carried out on server side based on policy >> brief info sent by client. In this scenario, the policy would only be >> the expected one by the time the client sends a request to the server >> (for instance a file open request), but not after that when SELinux >> actually checks the permissions on the client (via >> security_file_open() in this example). >> A solution to address this could be to add a new parameter to >> security_policydb_brief() hook, in the form of a pointer to an >> integer >> giving the current sequence number of the policy. That would >> complement the policy brief info, with the notion of change to the >> policy. I do not think it is desirable to include the sequence number >> in the policy brief info, as it is not the essence of the policy. >> Now with this sequence info in mind, the new hook to check policy >> brief info validity would only need to check the sequence, instead of >> the policy brief string. The current value of the sequence info >> should >> be stored by Lustre internally, and checked after SELinux permission >> checks. If a change is detected, Lustre client must stop normal >> processing and return an error for the current request. > > Not sure about your threat model but I think you are fighting a losing > battle there. A malicious admin has too many ways to defeat your > checking. I do not have much experience in the domain, so every advice or idea is appreciated. If what I want to accomplish is implemented using the notification system, what would be the possibility for a malicious admin to change the policy or the way it is enforced without triggering a notification? Of course, the places in the SELinux kernel code from where to trigger the notification have to be thought thoroughly. > Relying on the seqno also seems brittle; you could easily end up > causing a client to fail just because a policy update happened to be > installed at the same time, even though there was nothing wrong or > malicious about the policy update itself. I consider it is not a problem if one request must be resent, even if the policy update was legitimate. It would not disturb Lustre behavior, and it might be preferable to proceed to a resend instead of missing a potential fraud. Thanks, Sebastien. -- 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
diff --git a/include/linux/lsm_hooks.h b/include/linux/lsm_hooks.h index 080f34e..0bc0260 100644 --- a/include/linux/lsm_hooks.h +++ b/include/linux/lsm_hooks.h @@ -1336,6 +1336,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. */ @@ -1568,6 +1586,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 +1832,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 54b1e39..91247fc 100644 --- a/security/security.c +++ b/security/security.c @@ -1300,6 +1300,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 dddb81e..b6540f9 100644 --- a/security/selinux/hooks.c +++ b/security/selinux/hooks.c @@ -6071,6 +6071,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, @@ -6285,6 +6290,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 87d645d..b37b8e5 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 @@ -874,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); } /* @@ -2215,6 +2224,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) +{ + SHASH_DESC_ON_STACK(desc, policybrief_tfm); + struct policy_file *fp = ptr; + u8 *hashval; + int rc; + + 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 = kmalloc(policybrief_len, GFP_KERNEL); + if (policydb->policybrief == NULL) { + rc = -ENOMEM; + goto out_free; + } + rc = snprintf(policydb->policybrief, policybrief_len, + "selinux(enforce=%d:checkreqprot=%d:%s=%*phN)", + selinux_enforcing, selinux_checkreqprot, + policybrief_hash_alg, policybrief_hash_size, hashval); + BUG_ON(rc >= policybrief_len); + rc = 0; + +out_free: + kfree(hashval); + + return rc; +} + +/* * Read the configuration data from a policy database binary * representation file into a policy database structure. */ @@ -2233,6 +2288,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) @@ -3451,3 +3511,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; + 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
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@ddn.com> --- include/linux/lsm_hooks.h | 20 +++++++++ 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 | 88 +++++++++++++++++++++++++++++++++++++ security/selinux/ss/policydb.h | 3 ++ security/selinux/ss/services.c | 67 ++++++++++++++++++++++++++++ 9 files changed, 202 insertions(+)