Re: semanage: is __default__ login map required?

[Karl Macmillan]

> On May 3, 2017, at 3:47 PM, Arnold, Paul C CTR USARMY PEO STRI (US) 
>  wrote:
> 
>> On 05/03/2017 03:32 PM, Stephen Smalley wrote:
>>> On Wed, 2017-05-03 at 15:14 -0400, Stephen Smalley wrote:
>>> On Wed, 2017-05-03 at 13:36 -0400, Arnold, Paul C CTR USARMY PEO STRI
>>> (US) wrote:
 I have been having problems mapping logins since removing
 __default__
 from the policy.  Is the __default__ login map required in order
 for
 semanage to set a new mapping?
 
 The error, specifically:
 
 $ sudo semanage login -a -s existing_u existing_login
 libsemanage.dbase_llist_query: could not query record value
 semanage: Could not query user for existing_login
 
 
 Policy is based upon refpolicy, but all utils are RHEL6 dist.
>>> Not sure what is in RHEL6, but upstream it looks like the code tries
>>> to
>>> look up the old login/user information before making the change so
>>> that
>>>  it can audit the old and new values.  Probably ought to be handling
>>> an
>>> exception there and recovering cleanly.
>>> 
>>> Caution-https://github.com/SELinuxProject/selinux/blob/master/python/semanage
>>> /seobject.py#L537
>>> 
>>> Caution-https://github.com/SELinuxProject/selinux/commit/a0e538c208e5af07fecb
>>> 8c045e6341397d0df44a
>> That said, maybe the first question is why do you want to remove the
>> __default__ mapping.  Not sure that is even supported via semanage
>> login -d, and you're likely to end up having it get regenerated
>> automatically on any subsequent semodule/semanage commands even if you
>> manually remove it (unless you removed it from the source policy before
>> building in the first place).
>> 
>> Just set it to the most restrictive values possible, like user_u, s0 or
>> guest_u, s0.
>> 
> 
> Thanks Stephen.  As for why, this is for a high assurance solution for which 
> I do not make the requirements.
> 

Many high assurance systems I've built or reviewed simply map default to a user 
domain with no privileges. It's quite likely that this is for systems with 
requirements similar to what you are dealing with. 

If you want to send me a private email I can likely give you some more helpful 
details. 

Karl

> 
> __default__ is removed from source policy.  I don't think semanage can remove 
> logins defined in base policy, and I wouldn't want to due to regeneration 
> concerns you already mentioned.
> 
> Thank you for the commit reference; that makes it much clearer for me.  I'll 
> do some testing, perhaps just write another python script to add new mappings 
> if I absolutely cannot have __default__ defined.
> 
> 
> 
> Regards,
> 
> -- 
> Paul Arnold, CISSP
> Cole Engineering Services, Inc.
> 
> 

Re: [PATCH 4/9] checkpolicy: Add support for ibendportcon labels

[Daniel Jurgens]

On 5/10/2017 1:56 PM, Stephen Smalley wrote:
> On Tue, 2017-05-09 at 23:50 +0300, Dan Jurgens wrote:
>> From: Daniel Jurgens 
>>  } ibendport;
> These were pkey and ib_endport in the kernel patch, and port was
> port_num.  Either way is fine but they probably ought to be consistent.

Yes, I received an internal comment that pkey wouldn't be an especially strange 
name collision in the future.  When I repost the kernel series I'll synchronize 
the names there for consistency.

>> @@ -396,6 +400,7 @@ typedef struct genfs {
>>  #define OCON_FSUSE 5/* fs_use */
>>  #define OCON_NODE6 6/* IPv6 nodes */
>>  #define OCON_IBPKEY 7   /* Infiniband PKEY */
>> +#define OCON_IBENDPORT 8/* Infiniband End Port */
> These were OCON_PKEY and OCON_IB_ENDPORT in the last kernel patches I
> saw.  Ok either way but they probably ought to be consistent.
Same here.

Re: [PATCH 2/9] libsepol: Add ibpkey ocontext handling

[Daniel Jurgens]

On 5/10/2017 1:51 PM, Stephen Smalley wrote:
> On Tue, 2017-05-09 at 23:50 +0300, Dan Jurgens wrote:
>> From: Daniel Jurgens 
>>
>> Add support for reading, writing, and copying Infinabinda Pkey 
> s/Infinabinda/Infiniband/
Done
>
>> --- a/libsepol/include/sepol/policydb/services.h
>> +++ b/libsepol/include/sepol/policydb/services.h
>> @@ -188,6 +188,17 @@ extern int sepol_port_sid(uint16_t domain,
>>uint16_t port, sepol_security_id_t *
>> out_sid);
>>  
>>  /*
>> + * Return the SID of the ibpkey specified by
>> + * `domain', `type', `subnet prefix', and `pkey'.
>> + */
> Can you explain why you are passing a (domain,type) pair to this
> interface and why subnet_prefix is not fixed length as it is in
> corresponding kernel interface (security_pkey_sid)?  Will these
> arguments ever be used?  Could the length change in the future?
>
> For that matter, and I guess I should have asked this on the kernel
> patches, why are you storing and passing the subnet prefix as a
> complete IPv6 address?  Is that just for the convenience of being able
> to use inet_pton() and inet_ntop()?  Is this typical for handling of IB
> subnet prefixes?  Seems a bit wasteful.
I modeled it after sepol_port_sid, which has the unused type and domain.  They 
are not needed and I've removed them. The length was also not needed, it is 
always the same size and will never change. 

Regarding using an IPv6 address for the subnet prefix, it is for convenience. 
There is already code to deal with IPv6 addresses, not just inet_pton and 
inet_ntop, but in the CIL code as well.  The subnet prefix is just the top half 
of the IPv6 address.  Using IPv6 address to store it allowed code reuse.  When 
the policy is loaded into the kernel the lower 8 bytes are not stored, subnet 
prefix is stored as a u64, so the space is not permanently wasted.
>>
>> @@ -2583,6 +2584,7 @@ static int ocontext_selinux_isid_to_cil(struct
>> policydb *pdb, struct ocontext *i
>>  "policy",
>>  "scmp_packet",
>>  "devnull",
>> +"ibpkey",
> I thought we dropped the separate initial SID for it?
You're right.  Overlooked this when I changed that during the kernel series 
review.
>>
>> @@ -185,6 +186,21 @@ static struct policydb_compat_info
>> policydb_compat[] = {
>>   .ocon_num = OCON_NODE6 + 1,
>>   .target_platform = SEPOL_TARGET_SELINUX,
>>  },
>> +
>> +{
>> + .type = POLICY_KERN,
>> + .version = POLICYDB_VERSION_XPERMS_IOCTL,
>> + .sym_num = SYM_NUM,
>> + .ocon_num = OCON_NODE6 + 1,
>> + .target_platform = SEPOL_TARGET_SELINUX,
>> +},
> This seems duplicated?

Removed

>> @@ -2782,6 +2812,23 @@ static int ocontext_read_selinux(struct
>> policydb_compat_info *info,
>>  (>context[1], p, fp))
>>  return -1;
>>  break;
>> +case OCON_IBPKEY:
>> +rc = next_entry(buf, fp,
>> sizeof(uint32_t) * 6);
>> +if (rc < 0)
>> +return -1;
>> +
>> +c->u.ibpkey.subnet_prefix[0] =
>> buf[0];
>> +c->u.ibpkey.subnet_prefix[1] =
>> buf[1];
>> +c->u.ibpkey.subnet_prefix[2] =
>> buf[2];
>> +c->u.ibpkey.subnet_prefix[3] =
>> buf[3];
> Why load all the values rather than just confirming that [2] and [3]
> are zero as in the kernel?

Changed to confirm they are 0.

Re: [PATCH 1/9] checkpolicy: Add support for ibpkeycon labels

[Daniel Jurgens]

On 5/10/2017 1:18 PM, Stephen Smalley wrote:
> On Tue, 2017-05-09 at 23:50 +0300, Dan Jurgens wrote:
>> From: Daniel Jurgens 
>>
>>
>> +#ifdef DARWIN
>> +memcpy(>u.ibpkey.subnet_prefix[0],
>> _prefix.s6_addr[0],
>> +   sizeof(newc->u.ibpkey.subnet_prefix));
>> +#else
>> +memcpy(>u.ibpkey.subnet_prefix[0],
>> _prefix.s6_addr32[0],
>> +   sizeof(newc->u.ibpkey.subnet_prefix));
>> +#endif
> We can just always use s6_addr instead of s6_addr32 and drop the
> #ifdef.  Just pushed a commit to fix that elsewhere. Also we switched
> from #ifdef DARWIN to __APPLE__ a while ago, but that won't matter once
> you drop the #ifdef altogether.
OK
>
>> @@ -722,10 +728,11 @@ extern int
>> policydb_set_target_platform(policydb_t *p, int platform);
>>  #define POLICYDB_VERSION_CONSTRAINT_NAMES   29
>>  #define POLICYDB_VERSION_XEN_DEVICETREE 30 /* Xen-
>> specific */
>>  #define POLICYDB_VERSION_XPERMS_IOCTL   30 /* Linux-specific */
>> +#define POLICYDB_VERSION_INFINIBAND 31
> This is Linux-specific too.
I'll add a similar comment.
>
>>  
>>  /* Range of policy versions we understand*/
>>  #define POLICYDB_VERSION_MINPOLICYDB_VERSION_BASE
>> -#define POLICYDB_VERSION_MAXPOLICYDB_VERSION_XPERMS_IOCTL
>> +#define POLICYDB_VERSION_MAXPOLICYDB_VERSION_INFINIBAND
>>  
>>  /* Module versions and specific changes*/
>>  #define MOD_POLICYDB_VERSION_BASE   4
>> @@ -743,10 +750,11 @@ extern int
>> policydb_set_target_platform(policydb_t *p, int platform);
>>  #define MOD_POLICYDB_VERSION_TUNABLE_SEP14
>>  #define MOD_POLICYDB_VERSION_NEW_OBJECT_DEFAULTS15
>>  #define MOD_POLICYDB_VERSION_DEFAULT_TYPE   16
>> -#define MOD_POLICYDB_VERSION_CONSTRAINT_NAMES  17
>> +#define MOD_POLICYDB_VERSION_CONSTRAINT_NAMES   17
>> +#define MOD_POLICYDB_VERSION_INFINIBAND 18
>>  
>>  #define MOD_POLICYDB_VERSION_MIN MOD_POLICYDB_VERSION_BASE
>> -#define MOD_POLICYDB_VERSION_MAX
>> MOD_POLICYDB_VERSION_CONSTRAINT_NAMES
>> +#define MOD_POLICYDB_VERSION_MAX MOD_POLICYDB_VERSION_INFINIBAND
>>  
>>  #define POLICYDB_CONFIG_MLS1
> Hmmm...we never introduced a binary module version for xperms, since
> the only user is presently Android and they don't use binary modules
> and in general we'd like to get rid of binary modules altogether and
> switch entirely to source modules (either .te modules with a te2cil
> converter or cil modules).  But I guess you probably want to support
> this in the interim for convenient usage within existing Fedora/RHEL
> policies.
>
Yes, we want to pull this back into RHEL once it's available upstream.

Thank you for your quick review.  I'll continue going through your comments on 
the other patches and post a v1 after giving some more time for others to 
comment as well.

Re: Policy capabilities: when to use and complications with using

[Paul Moore]

On Wed, May 10, 2017 at 8:58 AM, Stephen Smalley  wrote:
> I'm not proposing introducing policy capabilities for those commits
> retroactively; I don't think that would be productive now that they are
> already in upstream kernels and policies.  I just wanted to determine
> whether or not we think similar changes in the future should be wrapped
> with policy capabilities.
>
> If so, then I think that motivates lighter weight policy capabilities,
> as otherwise for each of these changes (and others too - e.g. probably
> the prlimit change) we would have been in the same position as with
> extended_socket_class, i.e. waiting for a release of libsepol that
> defines the new policy capability, requiring refpolicy to add a
> dependency on that specific libsepol version before it could be enabled
> by default, waiting for Fedora to update to that version, etc.

That's fine with me.  As I said earlier, I'm not opposed, I just
wanted to make sure this is a definite "must have".

-- 
paul moore
www.paul-moore.com

[PATCH v2] libsepol: Expand attributes with TYPE_FLAGS_EXPAND_ATTR_TRUE set

[James Carter]

Commit 1089665e31a647a5f0ba2eabe8ac6232b384bed9 (Add attribute
expansion options) adds an expandattribute rule to the policy.conf
language which sets a type_datum flag. Currently the flag is used
only when writing out CIL policy from a policy.conf.

Make use of the flag when expanding policy to expand policy rules
and remove all type associations for an attribute that has
TYPE_FLAGS_EXPAND_ATTR_TRUE set. (The attribute will remain in the
policy, but have no types associated with it.)

Signed-off-by: James Carter 
---
v2 Just check if each attribute should be expanded in type_attr_map(). No need
to destroy the types ebitmap.

 libsepol/src/expand.c | 76 ++-
 libsepol/src/link.c   |  8 +++---
 2 files changed, 43 insertions(+), 41 deletions(-)

diff --git a/libsepol/src/expand.c b/libsepol/src/expand.c
index 54bf781..c55226b 100644
--- a/libsepol/src/expand.c
+++ b/libsepol/src/expand.c
@@ -2337,13 +2337,20 @@ static int type_attr_map(hashtab_key_t key
value = type->s.value;
 
if (type->flavor == TYPE_ATTRIB) {
-   if (ebitmap_cpy(>attr_type_map[value - 1], >types)) {
-   goto oom;
-   }
-   ebitmap_for_each_bit(>types, tnode, i) {
-   if (!ebitmap_node_get_bit(tnode, i))
-   continue;
-   if (ebitmap_set_bit(>type_attr_map[i], value - 1, 
1)) {
+   if (!(type->flags & TYPE_FLAGS_EXPAND_ATTR_TRUE)) {
+   if (ebitmap_cpy(>attr_type_map[value - 1], 
>types)) {
+   goto oom;
+   }
+   ebitmap_for_each_bit(>types, tnode, i) {
+   if (!ebitmap_node_get_bit(tnode, i))
+   continue;
+   if (ebitmap_set_bit(>type_attr_map[i], value 
- 1, 1)) {
+   goto oom;
+   }
+   }
+   } else {
+   /* Attribute is being expanded, so remove */
+   if (ebitmap_set_bit(>type_attr_map[value - 1], value 
- 1, 0)) {
goto oom;
}
}
@@ -2513,46 +2520,41 @@ int type_set_expand(type_set_t * set, ebitmap_t * t, 
policydb_t * p,
unsigned int i;
ebitmap_t types, neg_types;
ebitmap_node_t *tnode;
+   unsigned char expand = alwaysexpand || ebitmap_length(>negset) || 
set->flags;
+   type_datum_t *type;
int rc =-1;
 
ebitmap_init();
ebitmap_init(t);
 
-   if (alwaysexpand || ebitmap_length(>negset) || set->flags) {
-   /* First go through the types and OR all the attributes to 
types */
-   ebitmap_for_each_bit(>types, tnode, i) {
-   if (ebitmap_node_get_bit(tnode, i)) {
+   /* First go through the types and OR all the attributes to types */
+   ebitmap_for_each_bit(>types, tnode, i) {
+   if (!ebitmap_node_get_bit(tnode, i))
+   continue;
 
-   /*
-* invalid policies might have more types set 
in the ebitmap than
-* what's available in the type_val_to_struct 
mapping
-*/
-   if (i >= p->p_types.nprim)
-   goto err_types;
+   /*
+* invalid policies might have more types set in the ebitmap 
than
+* what's available in the type_val_to_struct mapping
+*/
+   if (i >= p->p_types.nprim)
+   goto err_types;
 
-   if (!p->type_val_to_struct[i]) {
-   goto err_types;
-   }
+   type = p->type_val_to_struct[i];
 
-   if (p->type_val_to_struct[i]->flavor ==
-   TYPE_ATTRIB) {
-   if (ebitmap_union
-   (,
->type_val_to_struct[i]->
-types)) {
-   goto err_types;
-   }
-   } else {
-   if (ebitmap_set_bit(, i, 1)) {
-   goto err_types;
-   }
-   }
+   if (!type) {
+   goto err_types;
+   }
+
+   if (type->flavor == TYPE_ATTRIB &&
+   (expand || (type->flags & TYPE_FLAGS_EXPAND_ATTR_TRUE))) {
+   

Re: [PATCH 7/9] semanage: Update semanage to allow runtime labeling of Infiniband Pkeys

[Stephen Smalley]

On Tue, 2017-05-09 at 23:50 +0300, Dan Jurgens wrote:
> From: Daniel Jurgens 
> 
> Update libsepol and libsemanage to work with pkey records. Add local
> storage for new and modified pkey records in pkeys.local. Update
> semanage
> to parse the pkey command options to add, modify, and delete pkeys.
> 
> Signed-off-by: Daniel Jurgens 
> ---
>  libsemanage/include/semanage/ibpkey_record.h  |   76 
>  libsemanage/include/semanage/ibpkeys_local.h  |   36 ++
>  libsemanage/include/semanage/ibpkeys_policy.h |   28 ++
>  libsemanage/include/semanage/semanage.h   |3 +
>  libsemanage/src/direct_api.c  |   29 ++-
>  libsemanage/src/handle.h  |   36 ++-
>  libsemanage/src/ibpkey_internal.h |   52 +++
>  libsemanage/src/ibpkey_record.c   |  187 ++
>  libsemanage/src/ibpkeys_file.c|  181 ++
>  libsemanage/src/ibpkeys_local.c   |  182 ++
>  libsemanage/src/ibpkeys_policy.c  |   52 +++
>  libsemanage/src/ibpkeys_policydb.c|   62 
>  libsemanage/src/libsemanage.map   |1 +
>  libsemanage/src/policy_components.c   |5 +-
>  libsemanage/src/semanage_store.c  |1 +
>  libsemanage/src/semanage_store.h  |1 +
>  libsemanage/src/semanageswig.i|3 +
>  libsemanage/src/semanageswig_python.i |   43 +++
>  libsemanage/utils/semanage_migrate_store  |3 +-
>  libsepol/VERSION  |2 +-
>  libsepol/include/sepol/ibpkey_record.h|   75 
>  libsepol/include/sepol/ibpkeys.h  |   44 +++
>  libsepol/include/sepol/sepol.h|2 +
>  libsepol/src/ibpkey_internal.h|   21 ++
>  libsepol/src/ibpkey_record.c  |  474
> +
>  libsepol/src/ibpkeys.c|  264 ++
>  python/semanage/semanage  |   60 +++-
>  python/semanage/seobject.py   |  253 +
>  28 files changed, 2159 insertions(+), 17 deletions(-)

That's a lot of code.  Did you look at whether you could generalize the
port record stuff at all to see if we could factor out common helpers
or anything?  I guess this is consistent with the current code, but it
seems like a lot of very similar code being duplicated and then
slightly tweaked.

>  create mode 100644 libsemanage/include/semanage/ibpkey_record.h
>  create mode 100644 libsemanage/include/semanage/ibpkeys_local.h
>  create mode 100644 libsemanage/include/semanage/ibpkeys_policy.h
>  create mode 100644 libsemanage/src/ibpkey_internal.h
>  create mode 100644 libsemanage/src/ibpkey_record.c
>  create mode 100644 libsemanage/src/ibpkeys_file.c
>  create mode 100644 libsemanage/src/ibpkeys_local.c
>  create mode 100644 libsemanage/src/ibpkeys_policy.c
>  create mode 100644 libsemanage/src/ibpkeys_policydb.c
>  create mode 100644 libsepol/include/sepol/ibpkey_record.h
>  create mode 100644 libsepol/include/sepol/ibpkeys.h
>  create mode 100644 libsepol/src/ibpkey_internal.h
>  create mode 100644 libsepol/src/ibpkey_record.c
>  create mode 100644 libsepol/src/ibpkeys.c
> 
> diff --git a/libsemanage/include/semanage/ibpkey_record.h
> b/libsemanage/include/semanage/ibpkey_record.h
> new file mode 100644
> index 000..45fe59e
> --- /dev/null
> +++ b/libsemanage/include/semanage/ibpkey_record.h
> @@ -0,0 +1,76 @@
> +/* Copyright (C) 2017 Mellanox Technologies Inc */
> +
> +#ifndef _SEMANAGE_IBPKEY_RECORD_H_
> +#define _SEMANAGE_IBPKEY_RECORD_H_
> +
> +#include 
> +#include 
> +#include 
> +
> +#ifndef _SEMANAGE_IBPKEY_DEFINED_
> +struct semanage_ibpkey;
> +struct semanage_ibpkey_key;
> +typedef struct semanage_ibpkey semanage_ibpkey_t;
> +typedef struct semanage_ibpkey_key semanage_ibpkey_key_t;
> +#define _SEMANAGE_IBPKEY_DEFINED_
> +#endif
> +
> +extern int semanage_ibpkey_compare(const semanage_ibpkey_t *ibpkey,
> +    const semanage_ibpkey_key_t
> *key);
> +
> +extern int semanage_ibpkey_compare2(const semanage_ibpkey_t *ibpkey,
> + const semanage_ibpkey_t
> *ibpkey2);
> +
> +extern int semanage_ibpkey_key_create(semanage_handle_t *handle,
> +   const char *subnet_prefix,
> +   int low, int high,
> +   semanage_ibpkey_key_t
> **key_ptr);
> +
> +extern int semanage_ibpkey_key_extract(semanage_handle_t *handle,
> +    const semanage_ibpkey_t
> *ibpkey,
> +    semanage_ibpkey_key_t
> **key_ptr);
> +
> +extern void semanage_ibpkey_key_free(semanage_ibpkey_key_t *key);
> +
> +extern int semanage_ibpkey_get_subnet_prefix(semanage_handle_t
> *handle,
> +  const semanage_ibpkey_t
> *ibpkey,
> +   

Re: [PATCH 5/9] libsepol: Add ibendport ocontext handling

[Stephen Smalley]

On Tue, 2017-05-09 at 23:50 +0300, Dan Jurgens wrote:
> From: Daniel Jurgens 
> 
> Add support for reading, writing, and copying IB end port ocontext
> data.
> Also add support for querying a IB end port sid to checkpolicy.
> 
> Signed-off-by: Daniel Jurgens 
> ---
>  checkpolicy/checkpolicy.c  |   20 ++
>  libsepol/include/sepol/policydb/services.h |   10 +++
>  libsepol/src/expand.c  |8 +
>  libsepol/src/libsepol.map.in   |1 +
>  libsepol/src/module_to_cil.c   |   15 ++
>  libsepol/src/policydb.c|   21 +-
>  libsepol/src/services.c|   39
> 
>  libsepol/src/write.c   |   14 ++
>  8 files changed, 126 insertions(+), 2 deletions(-)
> 
> diff --git a/checkpolicy/checkpolicy.c b/checkpolicy/checkpolicy.c
> index 0f12347..72431d6 100644
> --- a/checkpolicy/checkpolicy.c
> +++ b/checkpolicy/checkpolicy.c
> @@ -701,6 +701,7 @@ int main(int argc, char **argv)
>   printf("i)  display constraint expressions\n");
>   printf("j)  display validatetrans expressions\n");
>   printf("k)  Call ibpkey_sid\n");
> + printf("l)  Call ibendport_sid\n");
>  #ifdef EQUIVTYPES
>   printf("z)  Show equivalent types\n");
>  #endif
> @@ -1247,6 +1248,25 @@ int main(int argc, char **argv)
>   printf("sid %d\n", ssid);
>   }
>   break;
> + case 'l':
> + printf("device name (eg. mlx4_0)?  ");
> + FGETS(ans, sizeof(ans), stdin);
> + ans[strlen(ans) - 1] = 0;
> +
> + name = malloc((strlen(ans) + 1) *
> sizeof(char));
> + if (!name) {
> + fprintf(stderr, "couldn't malloc
> string.\n");
> + break;
> + }
> + strcpy(name, ans);
> +
> + printf("port? ");
> + FGETS(ans, sizeof(ans), stdin);
> + port = atoi(ans);
> + sepol_ibendport_sid(0, 0, name, port,
> );
> + printf("sid %d\n", ssid);
> + free(name);
> + break;
>  #ifdef EQUIVTYPES
>   case 'z':
>   identify_equiv_types();
> diff --git a/libsepol/include/sepol/policydb/services.h
> b/libsepol/include/sepol/policydb/services.h
> index 2d7aed1..aa8d718 100644
> --- a/libsepol/include/sepol/policydb/services.h
> +++ b/libsepol/include/sepol/policydb/services.h
> @@ -199,6 +199,16 @@ extern int sepol_ibpkey_sid(uint16_t domain,
>     sepol_security_id_t *out_sid);
>  
>  /*
> + * Return the SID of the ibendport specified by
> + * `domain', `type', `dev_name', and `port'.
> + */
> +extern int sepol_ibendport_sid(uint16_t domain,
> +    uint16_t type,
> +    char *dev_name,
> +    uint8_t port,
> +    sepol_security_id_t *out_sid);

Why (domain, type) arguments?

> +
> +/*
>   * Return the SIDs to use for a network interface
>   * with the name `name'.  The `if_sid' SID is returned for 
>   * the interface and the `msg_sid' SID is returned as
> diff --git a/libsepol/src/expand.c b/libsepol/src/expand.c
> index c45ecbe..061945e 100644
> --- a/libsepol/src/expand.c
> +++ b/libsepol/src/expand.c
> @@ -2226,6 +2226,14 @@ static int
> ocontext_copy_selinux(expand_state_t *state)
>   n->u.ibpkey.low_pkey = c-
> >u.ibpkey.low_pkey;
>   n->u.ibpkey.high_pkey = c-
> >u.ibpkey.high_pkey;
>   break;
> + case OCON_IBENDPORT:
> + n->u.ibendport.dev_name = strdup(c-
> >u.ibendport.dev_name);
> + if (!n->u.ibendport.dev_name) {
> + ERR(state->handle, "Out of
> memory!");
> + return -1;
> + }
> + n->u.ibendport.port = c-
> >u.ibendport.port;
> + break;
>   case OCON_PORT:
>   n->u.port.protocol = c-
> >u.port.protocol;
>   n->u.port.low_port = c-
> >u.port.low_port;
> diff --git a/libsepol/src/libsepol.map.in
> b/libsepol/src/libsepol.map.in
> index 36225d1..dd1fec2 100644
> --- a/libsepol/src/libsepol.map.in
> +++ b/libsepol/src/libsepol.map.in
> @@ -7,6 +7,7 @@ LIBSEPOL_1.0 {
>   sepol_iface_*; 
>   sepol_port_*;
>   sepol_ibpkey_*;
> + sepol_ibendport_*;
>   sepol_node_*;
>   sepol_user_*; sepol_genusers; sepol_set_delusers;
>   sepol_msg_*; sepol_debug;
> diff --git 

Re: [PATCH 4/9] checkpolicy: Add support for ibendportcon labels

[Stephen Smalley]

On Tue, 2017-05-09 at 23:50 +0300, Dan Jurgens wrote:
> From: Daniel Jurgens 
> 
> Add checkpolicy support for scanning and parsing ibendportcon labels.
> Also create a new ocontext for IB end ports.
> 
> Signed-off-by: Daniel Jurgens 
> ---
>  checkpolicy/policy_define.c|   70
> 
>  checkpolicy/policy_define.h|1 +
>  checkpolicy/policy_parse.y |   14 +-
>  checkpolicy/policy_scan.l  |2 +
>  libsepol/include/sepol/policydb/policydb.h |7 ++-
>  5 files changed, 91 insertions(+), 3 deletions(-)
> 
> diff --git a/checkpolicy/policy_define.c
> b/checkpolicy/policy_define.c
> index 6f92bc5..2926f18 100644
> --- a/checkpolicy/policy_define.c
> +++ b/checkpolicy/policy_define.c
> @@ -5085,6 +5085,76 @@ out:
>   return rc;
>  }
>  
> +int define_ibendport_context(unsigned int port)
> +{
> + ocontext_t *newc, *c, *l, *head;
> + char *id;
> + int rc = 0;
> +
> + if (policydbp->target_platform != SEPOL_TARGET_SELINUX) {
> + yyerror("ibendportcon not supported for target");
> + return -1;
> + }
> +
> + if (pass == 1) {
> + id = (char *)queue_remove(id_queue);
> + free(id);
> + parse_security_context(NULL);
> + return 0;
> + }
> +
> + newc = malloc(sizeof(*newc));
> + if (!newc) {
> + yyerror("out of memory");
> + return -1;
> + }
> + memset(newc, 0, sizeof(*newc));
> +
> + newc->u.ibendport.dev_name = queue_remove(id_queue);
> + if (!newc->u.ibendport.dev_name) {
> + yyerror("failed to read subnet management interface
> device name.");
> + rc = -1;
> + goto out;
> + }
> +
> + newc->u.ibendport.port = port;
> +
> + if (parse_security_context(>context[0])) {
> + free(newc);
> + return -1;
> + }
> +
> + /* Preserve the matching order specified in the
> configuration. */
> + head = policydbp->ocontexts[OCON_IBENDPORT];
> + for (l = NULL, c = head; c; l = c, c = c->next) {
> + unsigned int port2;
> +
> + port2 = c->u.ibendport.port;
> +
> + if (port == port2 &&
> + !strncmp(c->u.ibendport.dev_name,
> +  newc->u.ibendport.dev_name,
> +  64)) {
> + yyerror2("duplicate ibendportcon entry for
> %s port %u",
> +  newc->u.ibendport.dev_name, port);
> + rc = -1;
> + goto out;
> + }
> + }
> +
> + if (l)
> + l->next = newc;
> + else
> + policydbp->ocontexts[OCON_IBENDPORT] = newc;
> +
> + return 0;
> +
> +out:
> + free(newc->u.ibendport.dev_name);
> + free(newc);
> + return rc;
> +}
> +
>  int define_netif_context(void)
>  {
>   ocontext_t *newc, *c, *head;
> diff --git a/checkpolicy/policy_define.h
> b/checkpolicy/policy_define.h
> index b019b1a..3282aed 100644
> --- a/checkpolicy/policy_define.h
> +++ b/checkpolicy/policy_define.h
> @@ -44,6 +44,7 @@ int define_netif_context(void);
>  int define_permissive(void);
>  int define_polcap(void);
>  int define_ibpkey_context(unsigned int low, unsigned int high);
> +int define_ibendport_context(unsigned int port);
>  int define_port_context(unsigned int low, unsigned int high);
>  int define_pirq_context(unsigned int pirq);
>  int define_iomem_context(uint64_t low, uint64_t high);
> diff --git a/checkpolicy/policy_parse.y b/checkpolicy/policy_parse.y
> index f50eab1..35b7a33 100644
> --- a/checkpolicy/policy_parse.y
> +++ b/checkpolicy/policy_parse.y
> @@ -136,6 +136,7 @@ typedef int (* require_func_t)(int pass);
>  %token SAMEUSER
>  %token FSCON PORTCON NETIFCON NODECON 
>  %token IBPKEYCON
> +%token IBENDPORTCON
>  %token PIRQCON IOMEMCON IOPORTCON PCIDEVICECON DEVICETREECON
>  %token FSUSEXATTR FSUSETASK FSUSETRANS
>  %token GENFSCON
> @@ -171,7 +172,7 @@ base_policy : { if
> (define_policy(pass, 0) == -1) return -1; }
>     opt_default_rules opt_mls te_rbac users
> opt_constraints 
>   { if (pass == 1) { if
> (policydb_index_bools(policydbp)) return -1;}
>      else if (pass == 2) { if
> (policydb_index_others(NULL, policydbp, 0)) return -1;}}
> -   initial_sid_contexts opt_fs_contexts
> opt_fs_uses opt_genfs_contexts net_contexts opt_dev_contexts
> opt_ibpkey_contexts
> +   initial_sid_contexts opt_fs_contexts
> opt_fs_uses opt_genfs_contexts net_contexts opt_dev_contexts
> opt_ibpkey_contexts opt_ibendport_contexts
>   ;
>  classes  : class_def 
>   | classes class_def
> @@ -697,7 +698,7 @@ fs_contexts   : fs_context_def
>  fs_context_def   : 

Re: [PATCH 2/9] libsepol: Add ibpkey ocontext handling

[Stephen Smalley]

On Tue, 2017-05-09 at 23:50 +0300, Dan Jurgens wrote:
> From: Daniel Jurgens 
> 
> Add support for reading, writing, and copying Infinabinda Pkey 

s/Infinabinda/Infiniband/

> ocontext
> data. Also add support for querying a Pkey sid to checkpolicy.
> 
> Signed-off-by: Daniel Jurgens 
> ---
>  checkpolicy/checkpolicy.c  |   27 +
>  libsepol/include/sepol/policydb/services.h |   11 +
>  libsepol/src/expand.c  |9 
>  libsepol/src/libsepol.map.in   |1 +
>  libsepol/src/module_to_cil.c   |   39 ++
>  libsepol/src/policydb.c|   47
> ++
>  libsepol/src/services.c|   59
> 
>  libsepol/src/write.c   |   16 +++
>  8 files changed, 209 insertions(+), 0 deletions(-)
> 
> diff --git a/checkpolicy/checkpolicy.c b/checkpolicy/checkpolicy.c
> index 534fc22..0f12347 100644
> --- a/checkpolicy/checkpolicy.c
> +++ b/checkpolicy/checkpolicy.c
> @@ -22,6 +22,7 @@
>   *
>   *   Policy Module support.
>   *
> + * Copyright (C) 2017 Mellanox Technologies Inc.
>   * Copyright (C) 2004-2005 Trusted Computer Solutions, Inc.
>   * Copyright (C) 2003 - 2005 Tresys Technology, LLC
>   * Copyright (C) 2003 Red Hat, Inc., James Morris  m>
> @@ -699,6 +700,7 @@ int main(int argc, char **argv)
>   printf("h)  change a boolean value\n");
>   printf("i)  display constraint expressions\n");
>   printf("j)  display validatetrans expressions\n");
> + printf("k)  Call ibpkey_sid\n");
>  #ifdef EQUIVTYPES
>   printf("z)  Show equivalent types\n");
>  #endif
> @@ -1220,6 +1222,31 @@ int main(int argc, char **argv)
>   "\nNo validatetrans expressions
> found.\n");
>   }
>   break;
> + case 'k':
> + {
> + char *p;
> + int len;
> + struct in6_addr addr6;
> + unsigned int pkey;
> +
> + printf("subnet prefix?  ");
> + FGETS(ans, sizeof(ans), stdin);
> + ans[strlen(ans) - 1] = 0;
> + p = (char *)
> + len = sizeof(addr6);
> +
> + if (inet_pton(AF_INET6, ans, p) < 1)
> {
> + printf("error parsing subnet
> prefix\n");
> + break;
> + }
> +
> + printf("pkey? ");
> + FGETS(ans, sizeof(ans), stdin);
> + pkey = atoi(ans);
> + sepol_ibpkey_sid(0, 0, p, len, pkey,
> );
> + printf("sid %d\n", ssid);
> + }
> + break;
>  #ifdef EQUIVTYPES
>   case 'z':
>   identify_equiv_types();
> diff --git a/libsepol/include/sepol/policydb/services.h
> b/libsepol/include/sepol/policydb/services.h
> index 9162149..2d7aed1 100644
> --- a/libsepol/include/sepol/policydb/services.h
> +++ b/libsepol/include/sepol/policydb/services.h
> @@ -188,6 +188,17 @@ extern int sepol_port_sid(uint16_t domain,
>     uint16_t port, sepol_security_id_t *
> out_sid);
>  
>  /*
> + * Return the SID of the ibpkey specified by
> + * `domain', `type', `subnet prefix', and `pkey'.
> + */

Can you explain why you are passing a (domain,type) pair to this
interface and why subnet_prefix is not fixed length as it is in
corresponding kernel interface (security_pkey_sid)?  Will these
arguments ever be used?  Could the length change in the future?

For that matter, and I guess I should have asked this on the kernel
patches, why are you storing and passing the subnet prefix as a
complete IPv6 address?  Is that just for the convenience of being able
to use inet_pton() and inet_ntop()?  Is this typical for handling of IB
subnet prefixes?  Seems a bit wasteful.

> +extern int sepol_ibpkey_sid(uint16_t domain,
> +   uint16_t type,
> +   void *subnet_prefix_p,
> +   size_t splen,
> +   uint16_t pkey,
> +   sepol_security_id_t *out_sid);
> +
> +/*
>   * Return the SIDs to use for a network interface
>   * with the name `name'.  The `if_sid' SID is returned for 
>   * the interface and the `msg_sid' SID is returned as
> diff --git a/libsepol/src/expand.c b/libsepol/src/expand.c
> index 54bf781..c45ecbe 100644
> --- a/libsepol/src/expand.c
> +++ b/libsepol/src/expand.c
> @@ -4,6 +4,7 @@
>   *
>   * Copyright (C) 2004-2005 Tresys Technology, LLC
>   * Copyright (C) 2007 Red Hat, Inc.
> + * Copyright (C) 2017 Mellanox 

Re: [PATCH 1/9] checkpolicy: Add support for ibpkeycon labels

[Stephen Smalley]

On Tue, 2017-05-09 at 23:50 +0300, Dan Jurgens wrote:
> From: Daniel Jurgens 
> 
> Add checkpolicy support for scanning and parsing ibpkeycon labels.
> Also
> create a new ocontext for Infiniband Pkeys and define a new policydb
> version for infiniband support.
> 
> Signed-off-by: Daniel Jurgens 
> ---
>  checkpolicy/policy_define.c|  110
> 
>  checkpolicy/policy_define.h|1 +
>  checkpolicy/policy_parse.y |   15 -
>  checkpolicy/policy_scan.l  |3 +
>  libsepol/include/sepol/policydb/policydb.h |   32 +---
>  5 files changed, 148 insertions(+), 13 deletions(-)
> 
> diff --git a/checkpolicy/policy_define.c
> b/checkpolicy/policy_define.c
> index 949ca71..6f92bc5 100644
> --- a/checkpolicy/policy_define.c
> +++ b/checkpolicy/policy_define.c
> @@ -20,6 +20,7 @@
>   * Copyright (C) 2004-2005 Trusted Computer Solutions, Inc.
>   * Copyright (C) 2003 - 2008 Tresys Technology, LLC
>   * Copyright (C) 2007 Red Hat Inc.
> + * Copyright (C) 2017 Mellanox Techonologies Inc.
>   *   This program is free software; you can redistribute it
> and/or modify
>   *   it under the terms of the GNU General Public License as
> published by
>   *   the Free Software Foundation, version 2.
> @@ -4975,6 +4976,115 @@ int define_port_context(unsigned int low,
> unsigned int high)
>   return -1;
>  }
>  
> +int define_ibpkey_context(unsigned int low, unsigned int high)
> +{
> + ocontext_t *newc, *c, *l, *head;
> + struct in6_addr subnet_prefix;
> + char *id;
> + int rc = 0;
> +
> + if (policydbp->target_platform != SEPOL_TARGET_SELINUX) {
> + yyerror("ibpkeycon not supported for target");
> + return -1;
> + }
> +
> + if (pass == 1) {
> + id = (char *)queue_remove(id_queue);
> + free(id);
> + parse_security_context(NULL);
> + return 0;
> + }
> +
> + newc = malloc(sizeof(*newc));
> + if (!newc) {
> + yyerror("out of memory");
> + return -1;
> + }
> + memset(newc, 0, sizeof(*newc));
> +
> + id = queue_remove(id_queue);
> + if (!id) {
> + yyerror("failed to read the subnet prefix");
> + rc = -1;
> + goto out;
> + }
> +
> + rc = inet_pton(AF_INET6, id, _prefix);
> + free(id);
> + if (rc < 1) {
> + yyerror("failed to parse the subnet prefix");
> + if (rc == 0)
> + rc = -1;
> + goto out;
> + }
> +
> + if (subnet_prefix.s6_addr[2] || subnet_prefix.s6_addr[3]) {
> + yyerror("subnet prefix should be 0's in the low
> order 64 bits.");
> + rc = -1;
> + goto out;
> + }
> +
> +#ifdef DARWIN
> + memcpy(>u.ibpkey.subnet_prefix[0],
> _prefix.s6_addr[0],
> +    sizeof(newc->u.ibpkey.subnet_prefix));
> +#else
> + memcpy(>u.ibpkey.subnet_prefix[0],
> _prefix.s6_addr32[0],
> +    sizeof(newc->u.ibpkey.subnet_prefix));
> +#endif

We can just always use s6_addr instead of s6_addr32 and drop the
#ifdef.  Just pushed a commit to fix that elsewhere. Also we switched
from #ifdef DARWIN to __APPLE__ a while ago, but that won't matter once
you drop the #ifdef altogether.

> +
> + newc->u.ibpkey.low_pkey = low;
> + newc->u.ibpkey.high_pkey = high;
> +
> + if (low > high) {
> + yyerror2("low pkey %d exceeds high pkey %d", low,
> high);
> + rc = -1;
> + goto out;
> + }
> +
> + rc = parse_security_context(>context[0]);
> + if (rc)
> + goto out;
> +
> + /* Preserve the matching order specified in the
> configuration. */
> + head = policydbp->ocontexts[OCON_IBPKEY];
> + for (l = NULL, c = head; c; l = c, c = c->next) {
> + unsigned int low2, high2;
> +
> + low2 = c->u.ibpkey.low_pkey;
> + high2 = c->u.ibpkey.high_pkey;
> +
> + if (low == low2 && high == high2 &&
> + !memcmp(>u.ibpkey.subnet_prefix[0],
> + >u.ibpkey.subnet_prefix[0],
> + sizeof(c->u.ibpkey.subnet_prefix))) {
> + yyerror2("duplicate ibpkeycon entry for %d-
> %d ",
> +  low, high);
> + rc = -1;
> + goto out;
> + }
> + if (low2 <= low && high2 >= high &&
> + !memcmp(>u.ibpkey.subnet_prefix[0],
> + >u.ibpkey.subnet_prefix[0],
> + sizeof(c->u.ibpkey.subnet_prefix))) {
> + yyerror2("ibpkeycon entry for %d-%d hidden
> by earlier entry for %d-%d",
> +  low, high, low2, high2);
> + rc = -1;
> + goto out;
> + }
> + }
> +
> + if (l)
> + l->next = newc;

[PATCH] checkpolicy,libsepol: drop unnecessary usage of s6_addr32

[Stephen Smalley]

s6_addr32 is not portable; use s6_addr instead.
This obviates the need for #ifdef __APPLE__ conditionals in these cases.

Signed-off-by: Stephen Smalley 
---
 checkpolicy/policy_define.c | 6 --
 libsepol/src/node_record.c  | 8 
 2 files changed, 14 deletions(-)

diff --git a/checkpolicy/policy_define.c b/checkpolicy/policy_define.c
index 63e3c53..8fab214 100644
--- a/checkpolicy/policy_define.c
+++ b/checkpolicy/policy_define.c
@@ -5260,14 +5260,8 @@ int define_ipv6_node_context(void)
}
 
memset(newc, 0, sizeof(ocontext_t));
-
-#ifdef __APPLE__
memcpy(>u.node6.addr[0], _addr[0], 16);
memcpy(>u.node6.mask[0], _addr[0], 16);
-#else
-   memcpy(>u.node6.addr[0], _addr32[0], 16);
-   memcpy(>u.node6.mask[0], _addr32[0], 16);
-#endif
 
if (parse_security_context(>context[0])) {
free(newc);
diff --git a/libsepol/src/node_record.c b/libsepol/src/node_record.c
index 21043b6..6189c31 100644
--- a/libsepol/src/node_record.c
+++ b/libsepol/src/node_record.c
@@ -70,11 +70,7 @@ static int node_parse_addr(sepol_handle_t * handle,
return STATUS_ERR;
}
 
-#ifdef __APPLE__
memcpy(addr_bytes, in_addr.s6_addr, 16);
-#else
-   memcpy(addr_bytes, in_addr.s6_addr32, 16);
-#endif
break;
}
default:
@@ -162,11 +158,7 @@ static int node_expand_addr(sepol_handle_t * handle,
{
struct in6_addr addr;
memset(, 0, sizeof(struct in6_addr));
-#ifdef __APPLE__
memcpy(_addr[0], addr_bytes, 16);
-#else
-   memcpy(_addr32[0], addr_bytes, 16);
-#endif
if (inet_ntop(AF_INET6, , addr_str,
  INET6_ADDRSTRLEN) == NULL) {
 
-- 
2.9.3

Re: Policy capabilities: when to use and complications with using

[Stephen Smalley]

On Tue, 2017-05-09 at 17:44 -0400, Paul Moore wrote:
> On Tue, May 9, 2017 at 4:39 PM, Stephen Smalley 
> wrote:
> > On Tue, 2017-05-09 at 13:49 -0400, Paul Moore wrote:
> > > > On 05/03/2017 12:14 PM, Stephen Smalley wrote:
> > > > > 
> > > > > 1) Should we investigate lighter weight support for policy
> > > > > capabilities, and if so, how?
> > > 
> > > I agree that not having to update userspace for each new policy
> > > capability is a desirable goal, but I'm hopeful that changes
> > > requiring
> > > a new policy capability are kept to a minimum.
> > > 
> > > > > 2) Should the kernel log information about enabled/disabled
> > > > > policy
> > > > > capabilities in much the same manner as it does for undefined
> > > > > classes/permissions?
> > > 
> > > This seems like a very good idea to me.
> > > 
> > > * https://github.com/SELinuxProject/selinux-kernel/issues/32
> > > 
> > > > > 3) Should the policy compiler toolchain warn the user if a
> > > > > policy
> > > > > capability is not declared and classes/permissions are used
> > > > > in
> > > > > rules
> > > > > that will only be used if that policy capability is
> > > > > declared?  And
> > > > > similarly if a policy capability is declared but the
> > > > > corresponding
> > > > > classes/permissions are not used in any rules?
> > > 
> > > This seems to go against lighter weight policy capabilities,
> > > would it
> > > not?
> > 
> > Not necessarily.  Let's say that we left policy capabilities as
> > strings
> > in the kernel policy.  Then when introducing a new policy
> > capability,
> > we would just need to patch the kernel to implement it, and patch
> > the
> > policy (or even just insert a CIL module) to define it for testing
> > and
> > enablement, similar to what we do for new classes/permissions.  We
> > wouldn't need an updated libsepol for basic enablement (which
> > likewise
> > doesn't need to be patched for new classes/permissions).   We could
> > update checkpolicy and/or libsepol to recognize particular
> > capability
> > names and provide these warnings, but those would be to help catch
> > policy mistakes, not a prerequisite to using the new capability at
> > all.
> 
> I was referring to the additional checking/warnings, not basic
> enablement, as I thought that was the point of #3.
> 
> > The downside however is that we'd lose on build-time detection of
> > e.g.
> > a typo in a capability name.  Maybe there is a middle ground where
> > we
> > could warn if unrecognized but let them through.
> > 
> > Even if we insist on libsepol validation of the policy capability
> > names
> > (to ensure build-time detection of a typo), it might be helpful to
> > add
> > the string form of the capability names to the kernel policy.
> > Otherwise, the kernel can't log anything useful about unrecognized
> > capabilities besides their bit number in the ebitmap.
> 
> My only thought on this is that we already have the capability
> bitmap,
> and for compatibility reasons that needs to stay, at least for the
> existing capabilities, and I have a strong desire to limit the amount
> of legacy "stuff" we have to carry around in the kernel.
> 
> However, I understand your points about easier enablement in
> userspace
> and I'm sympathetic (this problem isn't going to go away, arguably in
> some way it could get worse if the number of address families grows
> frequently).  If this is something you and the rest of the userspace
> folks feel strongly about I can be persuaded, but you have to
> *really*
> want this; it needs to be more than a "nice to have".
> 
> > > > > 4) Do we need/want a policy capability for map permission and
> > > > > other
> > > > > cases where we are only adding a new permission check? Or
> > > > > should
> > > > > we
> > > > > continue to reserve them for cases not addressed via
> > > > > handle_unknown?
> > > 
> > > See James' earlier comments.  I think sticking with the current
> > > usage
> > > is the "best practice", but I think we should reserve the right
> > > to
> > > treat each change individually.  I'm hopeful that changes where
> > > we
> > > consider new policy capabilities remain infrequent enough that we
> > > can
> > > along without a concrete policy on their use.
> > 
> > So what about the two commits I cited?  Were we correct in not
> > introducing new policy capabilities for them, or should we have
> > done
> > so?  Each of them switched from checking an existing class to a new
> > one, so they wouldn't break existing userspace (i.e. cause any new
> > denials) due to handle_unknown, but they could have caused a
> > regression
> > in policy enforcement (i.e. allow something that would have been
> > denied
> > previously under the old class).
> 
> Yes, in hindsight we should have introduced new policy capabilities
> for both those changes.
> 
> Unfortunately, the netlink socket class update has been part of
> Linus'
> releases since v4.2 and I fear that introducing that policy
> capability
> now