Re: [PATCH 1/1] libsepol/cil: Improve processing of context rules

[Jeffrey Vander Stoep via Selinux]

On Thu, Mar 29, 2018 at 1:06 PM, James Carter  wrote:
> Improve the processing of netifcon, genfscon, ibpkeycon, ibendportcon,
> portcon, nodecon, fsuse, filecon, iomemcon, ioportcon, pcidevicecon,
> and devicetreecon rules.
>
> If the multiple-decls option is not used then report errors if duplicate
> context rules are found. If it is used then remove duplicate context rules
> and report errors when two rules are identical except for the context.
>
> This also changes the ordering of portcon and filecon rules. The protocol
> of portcon rules will be compared if the port numbers are the same and the
> path strings of filecon rules will be compared if the number of meta
> characters, the stem length, string length and file types are the same.
>
> Based on an initial patch by Pierre-Hugues Husson (p...@phh.me)
>
> Signed-off-by: James Carter 

Acked-by: Jeff Vander Stoep 

> ---
>  libsepol/cil/src/cil_post.c | 331 
> ++--
>  1 file changed, 318 insertions(+), 13 deletions(-)
>
> diff --git a/libsepol/cil/src/cil_post.c b/libsepol/cil/src/cil_post.c
> index a2122454..0b09cecc 100644
> --- a/libsepol/cil/src/cil_post.c
> +++ b/libsepol/cil/src/cil_post.c
> @@ -53,6 +53,83 @@
>  static int __cil_expr_to_bitmap(struct cil_list *expr, ebitmap_t *out, int 
> max, struct cil_db *db);
>  static int __cil_expr_list_to_bitmap(struct cil_list *expr_list, ebitmap_t 
> *out, int max, struct cil_db *db);
>
> +static int cats_compare(struct cil_cats *a, struct cil_cats *b)
> +{
> +   struct cil_list_item *i, *j;
> +   int rc;
> +
> +   if (a == b) return 0;
> +   if (!a) return -1;
> +   if (!b) return 1;
> +
> +   /* Expects cat expression to have been evaluated */
> +   cil_list_for_each(i, a->datum_expr) {
> +   cil_list_for_each(j, b->datum_expr) {
> +   rc = strcmp(DATUM(i->data)->fqn, DATUM(j->data)->fqn);
> +   if (!rc) return rc;
> +   }
> +   }
> +   return 0;
> +}
> +
> +static int level_compare(struct cil_level *a, struct cil_level *b)
> +{
> +   int rc;
> +
> +   if (a == b) return 0;
> +   if (!a) return -1;
> +   if (!b) return 1;
> +
> +   if (a->sens != b->sens) {
> +   rc = strcmp(DATUM(a->sens)->fqn, DATUM(b->sens)->fqn);
> +   if (rc != 0) return rc;
> +   }
> +   if (a->cats != b->cats) {
> +   return cats_compare(a->cats, b->cats);
> +   }
> +   return 0;
> +}
> +
> +static int range_compare(struct cil_levelrange *a, struct cil_levelrange *b)
> +{
> +   int rc;
> +
> +   if (a == b) return 0;
> +   if (!a) return -1;
> +   if (!b) return 1;
> +
> +   if (a->low != b->low) {
> +   rc = level_compare(a->low, b->low);
> +   if (rc != 0) return rc;
> +   }
> +   if (a->high != b->high) {
> +   return level_compare(a->high, b->high);
> +   }
> +   return 0;
> +}
> +
> +static int context_compare(struct cil_context *a, struct cil_context *b)
> +{
> +   int rc;
> +
> +   if (a->user != b->user) {
> +   rc = strcmp(DATUM(a->user)->fqn, DATUM(b->user)->fqn);
> +   if (rc != 0) return rc;
> +   }
> +   if (a->role != b->role) {
> +   rc = strcmp(DATUM(a->role)->fqn, DATUM(b->role)->fqn);
> +   if (rc != 0) return rc;
> +   }
> +   if (a->type != b->type) {
> +   rc = strcmp(DATUM(a->type)->fqn, DATUM(b->type)->fqn);
> +   if (rc != 0) return rc;
> +   }
> +   if (a->range != b->range) {
> +   return range_compare(a->range, b->range);
> +   }
> +   return 0;
> +}
> +
>  static int cil_verify_is_list(struct cil_list *list, enum cil_flavor flavor)
>  {
> struct cil_list_item *curr;
> @@ -145,6 +222,8 @@ int cil_post_filecon_compare(const void *a, const void *b)
> rc = -1;
> } else if (b_filecon->type < a_filecon->type) {
> rc = 1;
> +   } else {
> +   rc = strcmp(a_filecon->path_str, b_filecon->path_str);
> }
>
> free(a_path);
> @@ -190,6 +269,10 @@ int cil_post_portcon_compare(const void *a, const void 
> *b)
> rc = -1;
> } else if (bportcon->port_low < aportcon->port_low) {
> rc = 1;
> +   } else if (aportcon->proto < bportcon->proto) {
> +   rc = -1;
> +   } else if (aportcon->proto > bportcon->proto) {
> +   rc = 1;
> }
> }
>
> @@ -369,6 +452,102 @@ int cil_post_fsuse_compare(const void *a, const void *b)
> return rc;
>  }
>
> +int cil_post_filecon_context_compare(const void *a, const void *b)
> +{
> +   struct cil_filecon *a_filecon = *(struct cil_filecon**)a;
> +   struct cil_filecon *b_filecon = 

Re: [PATCH v3] Resolve conflicts in expandattribute.

[Jeffrey Vander Stoep via Selinux]

Merged.

On Mon, Mar 19, 2018 at 11:55 AM jwcart2  wrote:

> On 03/16/2018 01:55 PM, Tri Vo wrote:
> > This commit resolves conflicts in values of expandattribute statements
> > in policy language and expandtypeattribute in CIL.
> >
> > For example, these statements resolve to false in policy language:
> >   expandattribute hal_audio true;
> >   expandattribute hal_audio false;
> >
> > Similarly, in CIL these also resolve to false.
> >   (expandtypeattribute (hal_audio) true)
> >   (expandtypeattribute (hal_audio) false)
> >
> > Motivation
> > When Android combines multiple .cil files from system.img and vendor.img
> > it's possible to have conflicting expandattribute statements.
> >
> > This change deals with this scenario by resolving the value of the
> > corresponding expandtypeattribute to false. The rationale behind this
> > override is that true is used for reduce run-time lookups, while
> > false is used for tests which must pass.
> >
> > Signed-off-by: Tri Vo 
>
> Acked-by: James Carter 
>
> > ---
> >   checkpolicy/policy_define.c| 10 ++
> >   libsepol/cil/src/cil_resolve_ast.c | 21 ++---
> >   2 files changed, 12 insertions(+), 19 deletions(-)
> >
> > diff --git a/checkpolicy/policy_define.c b/checkpolicy/policy_define.c
> > index 2c5db55d..40cc62b0 100644
> > --- a/checkpolicy/policy_define.c
> > +++ b/checkpolicy/policy_define.c
> > @@ -1182,10 +1182,6 @@ int expand_attrib(void)
> >   goto exit;
> >   }
> >
> > - if (attr->flags & TYPE_FLAGS_EXPAND_ATTR) {
> > - yyerror2("%s already has the expandattribute
> option specified", id);
> > - goto exit;
> > - }
> >   if (ebitmap_set_bit(, attr->s.value - 1, TRUE)) {
> >   yyerror("Out of memory!");
> >   goto exit;
> > @@ -1213,6 +1209,12 @@ int expand_attrib(void)
> >   attr = hashtab_search(policydbp->p_types.table,
> >   policydbp->sym_val_to_name[SYM_TYPES][i]);
> >   attr->flags |= flags;
> > + if ((attr->flags & TYPE_FLAGS_EXPAND_ATTR_TRUE) &&
> > + (attr->flags &
> TYPE_FLAGS_EXPAND_ATTR_FALSE)) {
> > + yywarn("Expandattribute option was set to both
> true and false. "
> > + "Resolving to false.");
> > + attr->flags &= ~TYPE_FLAGS_EXPAND_ATTR_TRUE;
> > + }
> >   }
> >
> >   rc = 0;
> > diff --git a/libsepol/cil/src/cil_resolve_ast.c
> b/libsepol/cil/src/cil_resolve_ast.c
> > index d1a5ed87..02259241 100644
> > --- a/libsepol/cil/src/cil_resolve_ast.c
> > +++ b/libsepol/cil/src/cil_resolve_ast.c
> > @@ -269,9 +269,8 @@ exit:
> >   return rc;
> >   }
> >
> > -int cil_type_used(struct cil_symtab_datum *datum, int used)
> > +void cil_type_used(struct cil_symtab_datum *datum, int used)
> >   {
> > - int rc = SEPOL_ERR;
> >   struct cil_typeattribute *attr = NULL;
> >
> >   if (FLAVOR(datum) == CIL_TYPEATTRIBUTE) {
> > @@ -279,16 +278,12 @@ int cil_type_used(struct cil_symtab_datum *datum,
> int used)
> >   attr->used |= used;
> >   if ((attr->used & CIL_ATTR_EXPAND_TRUE) &&
> >   (attr->used & CIL_ATTR_EXPAND_FALSE)) {
> > - cil_log(CIL_ERR, "Conflicting use of
> expandtypeattribute. "
> > - "Expandtypeattribute may be set to
> true or false "
> > - "but not both. \n");
> > - goto exit;
> > + cil_log(CIL_WARN, "Conflicting use of
> expandtypeattribute. "
> > + "Expandtypeattribute was set to
> both true or false for %s. "
> > + "Resolving to false. \n", attr->
> datum.name);
> > + attr->used &= ~CIL_ATTR_EXPAND_TRUE;
> >   }
> >   }
> > -
> > - return SEPOL_OK;
> > -exit:
> > - return rc;
> >   }
> >
> >   int cil_resolve_permissionx(struct cil_tree_node *current, struct
> cil_permissionx *permx, void *extra_args)
> > @@ -488,11 +483,7 @@ int cil_resolve_expandtypeattribute(struct
> cil_tree_node *current, void *extra_a
> >   goto exit;
> >   }
> >   used = expandattr->expand ? CIL_ATTR_EXPAND_TRUE :
> CIL_ATTR_EXPAND_FALSE;
> > - rc = cil_type_used(attr_datum, used);
> > - if (rc != SEPOL_OK) {
> > - goto exit;
> > - }
> > -
> > + cil_type_used(attr_datum, used);
> >   cil_list_append(expandattr->attr_datums, CIL_TYPE,
> attr_datum);
> >   }
> >
> >
>
>
> --
> James Carter 
> National Security Agency
>

Re: [PATCH v3] Resolve conflicts in expandattribute.

[Jeffrey Vander Stoep via Selinux]

On Fri, Mar 16, 2018 at 11:11 AM, Tri Vo  wrote:
> This commit resolves conflicts in values of expandattribute statements
> in policy language and expandtypeattribute in CIL.
>
> For example, these statements resolve to false in policy language:
>  expandattribute hal_audio true;
>  expandattribute hal_audio false;
>
> Similarly, in CIL these also resolve to false.
>  (expandtypeattribute (hal_audio) true)
>  (expandtypeattribute (hal_audio) false)
>
> A warning will be issued on this conflict.
>
> Motivation
> When Android combines multiple .cil files from system.img and vendor.img
> it's possible to have conflicting expandattribute statements.
>
> This change deals with this scenario by resolving the value of the
> corresponding expandtypeattribute to false. The rationale behind this
> override is that true is used for reduce run-time lookups, while
> false is used for tests which must pass.
>
> Signed-off-by: Tri Vo 

Acked-by: Jeff Vander Stoep 

> ---
>  checkpolicy/policy_define.c| 10 ++
>  libsepol/cil/src/cil_resolve_ast.c | 21 ++---
>  2 files changed, 12 insertions(+), 19 deletions(-)
>
> diff --git a/checkpolicy/policy_define.c b/checkpolicy/policy_define.c
> index 2c5db55d..40cc62b0 100644
> --- a/checkpolicy/policy_define.c
> +++ b/checkpolicy/policy_define.c
> @@ -1182,10 +1182,6 @@ int expand_attrib(void)
> goto exit;
> }
>
> -   if (attr->flags & TYPE_FLAGS_EXPAND_ATTR) {
> -   yyerror2("%s already has the expandattribute option 
> specified", id);
> -   goto exit;
> -   }
> if (ebitmap_set_bit(, attr->s.value - 1, TRUE)) {
> yyerror("Out of memory!");
> goto exit;
> @@ -1213,6 +1209,12 @@ int expand_attrib(void)
> attr = hashtab_search(policydbp->p_types.table,
> policydbp->sym_val_to_name[SYM_TYPES][i]);
> attr->flags |= flags;
> +   if ((attr->flags & TYPE_FLAGS_EXPAND_ATTR_TRUE) &&
> +   (attr->flags & TYPE_FLAGS_EXPAND_ATTR_FALSE)) 
> {
> +   yywarn("Expandattribute option was set to both true 
> and false. "
> +   "Resolving to false.");
> +   attr->flags &= ~TYPE_FLAGS_EXPAND_ATTR_TRUE;
> +   }
> }
>
> rc = 0;
> diff --git a/libsepol/cil/src/cil_resolve_ast.c 
> b/libsepol/cil/src/cil_resolve_ast.c
> index d1a5ed87..02259241 100644
> --- a/libsepol/cil/src/cil_resolve_ast.c
> +++ b/libsepol/cil/src/cil_resolve_ast.c
> @@ -269,9 +269,8 @@ exit:
> return rc;
>  }
>
> -int cil_type_used(struct cil_symtab_datum *datum, int used)
> +void cil_type_used(struct cil_symtab_datum *datum, int used)
>  {
> -   int rc = SEPOL_ERR;
> struct cil_typeattribute *attr = NULL;
>
> if (FLAVOR(datum) == CIL_TYPEATTRIBUTE) {
> @@ -279,16 +278,12 @@ int cil_type_used(struct cil_symtab_datum *datum, int 
> used)
> attr->used |= used;
> if ((attr->used & CIL_ATTR_EXPAND_TRUE) &&
> (attr->used & CIL_ATTR_EXPAND_FALSE)) {
> -   cil_log(CIL_ERR, "Conflicting use of 
> expandtypeattribute. "
> -   "Expandtypeattribute may be set to 
> true or false "
> -   "but not both. \n");
> -   goto exit;
> +   cil_log(CIL_WARN, "Conflicting use of 
> expandtypeattribute. "
> +   "Expandtypeattribute was set to both 
> true or false for %s. "
> +   "Resolving to false. \n", 
> attr->datum.name);
> +   attr->used &= ~CIL_ATTR_EXPAND_TRUE;
> }
> }
> -
> -   return SEPOL_OK;
> -exit:
> -   return rc;
>  }
>
>  int cil_resolve_permissionx(struct cil_tree_node *current, struct 
> cil_permissionx *permx, void *extra_args)
> @@ -488,11 +483,7 @@ int cil_resolve_expandtypeattribute(struct cil_tree_node 
> *current, void *extra_a
> goto exit;
> }
> used = expandattr->expand ? CIL_ATTR_EXPAND_TRUE : 
> CIL_ATTR_EXPAND_FALSE;
> -   rc = cil_type_used(attr_datum, used);
> -   if (rc != SEPOL_OK) {
> -   goto exit;
> -   }
> -
> +   cil_type_used(attr_datum, used);
> cil_list_append(expandattr->attr_datums, CIL_TYPE, 
> attr_datum);
> }
>
> --
> 2.16.2.804.g6dcf76e118-goog
>

Re: [PATCH] secilc: resolve conflicts in expandattribute.

[Jeffrey Vander Stoep via Selinux]

On Wed, Mar 14, 2018 at 4:05 PM, William Roberts
 wrote:
> On Wed, Mar 14, 2018 at 3:17 PM, Tri Vo  wrote:
>> When Android combines multiple .cil files from system.img and vendor.img
>> it's possible to have conflicting expandattribute statements, e.g.
>>  expandattribute hal_audio true;
>>  expandattribute hal_audio false;
>
> Isn't this the policy.conf version? I thought cil files had:
> expandtypeattribute, am I wrong here?

You'll need to fix checkpolicy too for consistency. See
https://github.com/SELinuxProject/selinux/blob/master/checkpolicy/policy_define.c#L1185

>
>>
>> This change deals with scenario be resolving the value of the
>> corresponding expandattribute to false. The rationale behind this
>> override is that true is used for reduce run-time lookups, while
>> false is used for tests which must pass.
>
> So in a conflict, it's always forced to false, which prevents expansion.
> That seems reasonable. I would imagine this should also update some
> document somewhere that describes this behavior, does one exist?
> I couldn't find anything, but not sure if it's on some external webpage.
> Stephen do you know?
>
>> ---
>>  libsepol/cil/src/cil_resolve_ast.c | 12 
>>  1 file changed, 4 insertions(+), 8 deletions(-)
>>
>> diff --git a/libsepol/cil/src/cil_resolve_ast.c 
>> b/libsepol/cil/src/cil_resolve_ast.c
>> index d1a5ed87..5c66f663 100644
>> --- a/libsepol/cil/src/cil_resolve_ast.c
>> +++ b/libsepol/cil/src/cil_resolve_ast.c
>> @@ -271,7 +271,6 @@ exit:
>>
>>  int cil_type_used(struct cil_symtab_datum *datum, int used)

Change from int to void. The return value is no longer useful.

>>  {
>> -   int rc = SEPOL_ERR;
>> struct cil_typeattribute *attr = NULL;
>>
>> if (FLAVOR(datum) == CIL_TYPEATTRIBUTE) {
>> @@ -279,16 +278,13 @@ int cil_type_used(struct cil_symtab_datum *datum, int 
>> used)
>> attr->used |= used;
>> if ((attr->used & CIL_ATTR_EXPAND_TRUE) &&
>> (attr->used & CIL_ATTR_EXPAND_FALSE)) {
>> -   cil_log(CIL_ERR, "Conflicting use of 
>> expandtypeattribute. "
>> -   "Expandtypeattribute may be set to 
>> true or false "
>> -   "but not both. \n");
>> -   goto exit;
>> +   cil_log(CIL_WARN, "Conflicting use of 
>> expandtypeattribute. "
>> +   "Expandtypeattribute was set to both 
>> true or false for %s. "
>> +   "Resolving to false. \n", 
>> attr->datum.name);
>> +   attr->used ^= CIL_ATTR_EXPAND_TRUE;
>
> Sure, this saves an operation, but:
>
> attr->used &= ~CIL_ATTR_EXPAND_TRUE;

+1

>
> Is less fragile and the usual unset idiom. I won't request this changed unless
> either you agree or someone else has the same opinion as me.
>
> One could always argue that conditional code that relies on the entry
> condition is the whole
> point of conditional code :-P
>
>> }
>> }
>> -
>> return SEPOL_OK;
>> -exit:
>> -   return rc;
>>  }
>>
>>  int cil_resolve_permissionx(struct cil_tree_node *current, struct 
>> cil_permissionx *permx, void *extra_args)
>> --
>> 2.16.2.804.g6dcf76e118-goog
>>
>>
>

Re: [PATCH] libselinux: android: support exact match for a property key

[Jeffrey Vander Stoep via Selinux]

Please hold off on submission. We're discussing if this is really necessary.

On Thu, Oct 19, 2017 at 4:49 PM, Jaekyun Seok via Selinux
 wrote:
> Performs exact match if a property key of property contexts ends with '$'
> instead of prefix match.
>
> This will enable to define an exact rule which can avoid unexpected
> context assignment.
>
> Signed-off-by: Jaekyun Seok 
> ---
>  libselinux/src/label_backends_android.c | 9 +++--
>  1 file changed, 7 insertions(+), 2 deletions(-)
>
> diff --git a/libselinux/src/label_backends_android.c 
> b/libselinux/src/label_backends_android.c
> index cb8aae26..4611d396 100644
> --- a/libselinux/src/label_backends_android.c
> +++ b/libselinux/src/label_backends_android.c
> @@ -258,8 +258,13 @@ static struct selabel_lookup_rec *property_lookup(struct 
> selabel_handle *rec,
> }
>
> for (i = 0; i < data->nspec; i++) {
> -   if (strncmp(spec_arr[i].property_key, key,
> -   strlen(spec_arr[i].property_key)) == 0) {
> +   size_t property_key_len = strlen(spec_arr[i].property_key);
> +   if (spec_arr[i].property_key[property_key_len - 1] == '$' &&
> +   strlen(key) == property_key_len - 1 &&
> +   strncmp(spec_arr[i].property_key, key, property_key_len - 
> 1) == 0) {
> +   break;
> +   }
> +   if (strncmp(spec_arr[i].property_key, key, property_key_len) 
> == 0) {
> break;
> }
> if (strncmp(spec_arr[i].property_key, "*", 1) == 0)
> --
> 2.15.0.rc0.271.g36b669edcc-goog
>
>

Re: [PATCH 2/3] security: bpf: Add eBPF LSM hooks and security field to eBPF map

[Jeffrey Vander Stoep via Selinux]

On Thu, Aug 31, 2017 at 7:05 PM, Alexei Starovoitov
 wrote:
> On Thu, Aug 31, 2017 at 01:56:34PM -0700, Chenbo Feng wrote:
>> From: Chenbo Feng 
>>
>> Introduce a pointer into struct bpf_map to hold the security information
>> about the map. The actual security struct varies based on the security
>> models implemented. Place the LSM hooks before each of the unrestricted
>> eBPF operations, the map_update_elem and map_delete_elem operations are
>> checked by security_map_modify. The map_lookup_elem and map_get_next_key
>> operations are checked by securtiy_map_read.
>>
>> Signed-off-by: Chenbo Feng 
>
> ...
>
>> @@ -410,6 +418,10 @@ static int map_lookup_elem(union bpf_attr *attr)
>>   if (IS_ERR(map))
>>   return PTR_ERR(map);
>>
>> + err = security_map_read(map);
>> + if (err)
>> + return -EACCES;
>> +
>>   key = memdup_user(ukey, map->key_size);
>>   if (IS_ERR(key)) {
>>   err = PTR_ERR(key);
>> @@ -490,6 +502,10 @@ static int map_update_elem(union bpf_attr *attr)
>>   if (IS_ERR(map))
>>   return PTR_ERR(map);
>>
>> + err = security_map_modify(map);
>
> I don't feel these extra hooks are really thought through.
> With such hook you'll disallow map_update for given map. That's it.
> The key/values etc won't be used in such security decision.
> In such case you don't need such hooks in update/lookup at all.
> Only in map_creation and object_get calls where FD can be received.
> In other words I suggest to follow standard unix practices:
> Do permissions checks in open() and allow read/write() if FD is valid.
> Same here. Do permission checks in prog_load/map_create/obj_pin/get
> and that will be enough to jail bpf subsystem.
> bpf cmds that need to be fast (like lookup and update) should not
> have security hooks.
>

I do think we want to distinguish between read/write (or read/modify)
for these objects. Essentially, we want to implement the example
described in patch 0/3 where eBPF objects can be passed to less
privileged processes which can read, but not modify the map. What
would be the best way to do this? Add a mode field to the bpf_map
struct?

sysfs symlinks in genfscon

[Jeffrey Vander Stoep via Selinux]

Genfs_contexts does not label symlinks in sysfs, instead it leaves
them with the default “sysfs” label. Is this a bug?

Re: Permissions for eBPF objects

[Jeffrey Vander Stoep via Selinux]

On Fri, Aug 25, 2017 at 12:26 PM, Stephen Smalley <s...@tycho.nsa.gov> wrote:
> On Fri, 2017-08-25 at 11:01 -0700, Jeffrey Vander Stoep via Selinux
> wrote:
>> I’d like to get your thoughts on adding LSM permission checks on BPF
>> objects.
>>
>> By default, the ability to create and use eBPF maps/programs requires
>> CAP_SYS_ADMIN [1]. Alternatively, all processes can be granted access
>> to bpf() functions. This seems like poor granularity. [2]
>>
>> Like files and sockets, eBPF maps and programs can be passed between
>> processes by FD and have a number of functions that map cleanly to
>> permissions.
>>
>> Let me know what you think. Are there simpler alternative approaches
>> that we haven’t considered?
>
> Is it possible to create the map/program in one process (with
> CAP_SYS_ADMIN), pass the resulting fd to netd, and then use it there
> (without requiring CAP_SYS_ADMIN in netd itself)?

That might work. Any use of bpf() requires CAP_SYS_ADMIN but netd
could potentially just apply the prog_fd to a socket:

   setsockopt(sockfd, SOL_SOCKET, SO_ATTACH_BPF,
  _fd, sizeof(prog_fd));

>
> What level of granularity would be useful?  Would it go beyond just
> being able to use bpf() at all?

"use" might be sufficient. At least initially.

I could see some others coming in handy. For example, a simple mapping
of functionality to permissions gives:
map_create, map_update, map_delete, map_read, prog_load, prog_use.

Of course there's no sense in breaking "use" into multiple permissions if
we expect the entire set to always be granted together.

>
>>
>> Thanks!
>> Jeff
>>
>> [1] http://man7.org/linux/man-pages/man2/bpf.2.html NOTES section
>> [2] We are considering eBPF for network filtering by netd. Giving
>> netd
>> CAP_SYS_ADMIN would considerably increase netd’s privileges.
>> Alternatively allowing all processes permission to use bpf() goes
>> against the principle of least privilege exposing a lot of kernel
>> attack surface to processes that do not actually need it.
>>

Permissions for eBPF objects

[Jeffrey Vander Stoep via Selinux]

I’d like to get your thoughts on adding LSM permission checks on BPF
objects.

By default, the ability to create and use eBPF maps/programs requires
CAP_SYS_ADMIN [1]. Alternatively, all processes can be granted access to
bpf() functions. This seems like poor granularity. [2]

Like files and sockets, eBPF maps and programs can be passed between
processes by FD and have a number of functions that map cleanly to
permissions.

Let me know what you think. Are there simpler alternative approaches that
we haven’t considered?

Thanks!
Jeff

[1] http://man7.org/linux/man-pages/man2/bpf.2.html NOTES section
[2] We are considering eBPF for network filtering by netd. Giving netd
CAP_SYS_ADMIN would considerably increase netd’s privileges. Alternatively
allowing all processes permission to use bpf() goes against the principle
of least privilege exposing a lot of kernel attack surface to processes
that do not actually need it.

Re: Permissions for eBPF objects

[Jeffrey Vander Stoep via Selinux]

Disregard this email. Re-sending in plain-text mode to prevent rejection by
netdev list.

On Fri, Aug 25, 2017 at 10:56 AM Jeffrey Vander Stoep 
wrote:

> I’d like to get your thoughts on adding LSM permission checks on BPF
> objects.
>
> By default, the ability to create and use eBPF maps/programs requires
> CAP_SYS_ADMIN [1]. Alternatively, all processes can be granted access to
> bpf() functions. This seems like poor granularity. [2]
>
> Like files and sockets, eBPF maps and programs can be passed between
> processes by FD and have a number of functions that map cleanly to
> permissions.
>
> Let me know what you think. Are there simpler alternative approaches that
> we haven’t considered?
>
> Thanks!
> Jeff
>
> [1] http://man7.org/linux/man-pages/man2/bpf.2.html NOTES section
> [2] We are considering eBPF for network filtering by netd. Giving netd
> CAP_SYS_ADMIN would considerably increase netd’s privileges. Alternatively
> allowing all processes permission to use bpf() goes against the principle
> of least privilege exposing a lot of kernel attack surface to processes
> that do not actually need it.
>

Permissions for eBPF objects

[Jeffrey Vander Stoep via Selinux]

I’d like to get your thoughts on adding LSM permission checks on BPF objects.

By default, the ability to create and use eBPF maps/programs requires
CAP_SYS_ADMIN [1]. Alternatively, all processes can be granted access
to bpf() functions. This seems like poor granularity. [2]

Like files and sockets, eBPF maps and programs can be passed between
processes by FD and have a number of functions that map cleanly to
permissions.

Let me know what you think. Are there simpler alternative approaches
that we haven’t considered?

Thanks!
Jeff

[1] http://man7.org/linux/man-pages/man2/bpf.2.html NOTES section
[2] We are considering eBPF for network filtering by netd. Giving netd
CAP_SYS_ADMIN would considerably increase netd’s privileges.
Alternatively allowing all processes permission to use bpf() goes
against the principle of least privilege exposing a lot of kernel
attack surface to processes that do not actually need it.

Re: ioctl help

[Jeffrey Vander Stoep via Selinux]

The command number is just a uint16. It's up to each driver to interpret
it. So it's CHIOEXCHANGE for a particular driver and CM_IOCSPTS for a
different driver.

Ideally, you're whitelisting ioctls on a per-driver basis, so this model
works.

On Thu, May 25, 2017 at 2:21 AM Dominick Grift 
wrote:

> On Thu, May 25, 2017 at 07:49:19AM +0200, Dominick Grift wrote:
> > On Wed, May 24, 2017 at 04:11:44PM -0400, Stephen Smalley wrote:
> > > On Wed, 2017-05-24 at 14:08 +0200, Dominick Grift wrote:
> > > > I was looking again at ioctl whitelisting, and excuse me if I
> > > > overlooked some documentation, but I am having a hard time
> > > > implementing this.
> > > > what I did was I just wanted to basically test blacklisting a single
> > > > ioctl (no particular one)
> > > >
> > > > So i looked into androids sepolicy and just picked a semi-random
> > > > ioctl from their "
> https://android.googlesource.com/platform/system/se
> > > > policy/+/master/public/ioctl_defines"
> > > >
> > > > for example: PHONE_CAPABILITIES_CHECK 0x40087182
> > > >
> > > > However the xpermissions statement only allows 0x to 0x when
> > > > i tried: (xpermission alg_socket_ioctl (ioctl alg_socket (not
> > > > (0x40087182
> > > >
> > > > My question is how do i convert these to something i can use with the
> > > > xpermission statement in CIL, and why can seandroid sepolicy get away
> > > > with using 0x12345678 where i have to use 0x1234? I could not find
> > > > any scripts that converts these in the android tree.
> > >
> > > FWIW, I added a simple test of ioctl whitelisting to the selinux-
> > > testsuite, although that was done in source policy and depends on the
> > > binary module format support for xperms.
> > >
> > > With regard to your question though, only the low 16 bits of the ioctl
> > > value (the type/driver and number/function fields) are actually used;
> > > the upper 16 bits encode the direction (read/write) and size of any
> > > argument to the ioctl and are therefore not relevant for whitelisting.
> > > So you can just use 0x7182.  checkpolicy just ignores the upper bits,
> > > which I guess is convenient so that they can use ioctl macro lists
> > > generated from kernel header definitions, and Android builds by using
> > > checkpolicy -C to convert policy.conf to CIL.
> >
> > Thanks. I considered that but then I thought I saw various different
> ioctls with the same last 16 bits so that got me confused
>
> With the above in mind, how should i interpret the following
>
> define(`CHIOEXCHANGE', `0x401c6302')
> define(`CM_IOCSPTS', `0x40086302')
>
> is "0x6203" CHIOEXCHANGE, is it CM_IOCSPTS, or are the two the same with
> exception of direction and/or size
>
> >
> > --
> > Key fingerprint = 5F4D 3CDB D3F8 3652 FBD8  02D5 3B6C 5F1D 2C7B 6B02
> > https://sks-keyservers.net/pks/lookup?op=get=0x3B6C5F1D2C7B6B02
> > Dominick Grift
>
>
>
> --
> Key fingerprint = 5F4D 3CDB D3F8 3652 FBD8  02D5 3B6C 5F1D 2C7B 6B02
> https://sks-keyservers.net/pks/lookup?op=get=0x3B6C5F1D2C7B6B02
> Dominick Grift
>

Re: [PATCH] Add attribute expansion options

[Jeffrey Vander Stoep via Selinux]

Yes, we are generating CIL from policy.conf files.

On Fri, May 5, 2017 at 1:28 PM James Carter  wrote:

> On 05/04/2017 05:36 PM, Jeff Vander Stoep wrote:
> > This commit adds attribute expansion statements to the policy
> > language allowing compiler defaults to be overridden.
> >
> > Always expands an attribute example:
> > expandattribute { foo } true;
> > CIL example:
> > (expandtypeattribute (foo) true)
> >
> > Never expand an attribute example:
> > expandattribute { bar } false;
> > CIL example:
> > (expandtypeattribute (bar) false)
> >
>
> It works for secilc and for checkpolicy -C (which outputs CIL), but the
> expandattribute rules are ignored when building the kernel policy from a
> policy.conf. Are you generating CIL from policy.conf files? If not, then it
> might make sense to only have this feature in CIL.
>
> Jim
>
> > Adding the annotations directly to policy was chosen over other
> > methods as it is consistent with how targeted runtime optimizations
> > are specified in other languages. For example, in C the "inline"
> > command.
> >
> > Motivation
> >
> > expandattribute true:
> > Android has been moving away from a monolithic policy binary to
> > a two part split policy representing the Android platform and the
> > underlying vendor-provided hardware interface. The goal is a stable
> > API allowing these two parts to be updated independently of each
> > other. Attributes provide an important mechanism for compatibility.
> > For example, when the vendor provides a HAL for the platform,
> > permissions needed by clients of the HAL can be granted to an
> > attribute. Clients need only be assigned the attribute and do not
> > need to be aware of the underlying types and permissions being
> > granted.
> >
> > Inheriting permissions via attribute creates a convenient mechanism
> > for independence between vendor and platform policy, but results
> > in the creation of many attributes, and the potential for performance
> > issues when processes are clients of many HALs. [1] Annotating these
> > attributes for expansion at compile time allows us to retain the
> > compatibility benefits of using attributes without the performance
> > costs. [2]
> >
> > expandattribute false:
> > Commit 0be23c3f15fd added the capability to aggresively remove unused
> > attributes. This is generally useful as too many attributes assigned
> > to a type results in lengthy policy look up times when there is a
> > cache miss. However, removing attributes can also result in loss of
> > information used in external tests. On Android, we're considering
> > stripping neverallow rules from on-device policy. This is consistent
> > with the kernel policy binary which also did not contain neverallows.
> > Removing neverallow rules results in a 5-10% decrease in on-device
> > policy build and load and a policy size decrease of ~250k. Neverallow
> > rules are still asserted at build time and during device
> > certification (CTS). If neverallow rules are absent when secilc is
> > run, some attributes are being stripped from policy and neverallow
> > tests in CTS may be violated. [3] This change retains the aggressive
> > attribute stripping behavior but adds an override mechanism to
> > preserve attributes marked as necessary.
> >
> > [1] https://github.com/SELinuxProject/cil/issues/9
> > [2] Annotating all HAL client attributes for expansion resulted in
> >  system_server's dropping from 19 attributes to 8. Because these
> >  attributes were not widely applied to other types, the final
> >  policy size change was negligible.
> > [3] data_file_type and service_manager_type are stripped from AOSP
> >  policy when using secilc's -G option. This impacts 11 neverallow
> >  tests in CTS.
> >
> > Test: Build and boot Marlin with all hal_*_client attributes marked
> >  for expansion. Verify (using seinfo and sesearch) that permissions
> >  are correctly expanded from attributes to types.
> > Test: Mark types being stripped by secilc with "preserve" and verify
> >  that they are retained in policy and applied to the same types.
> >
> > Signed-off-by: Jeff Vander Stoep 
> > ---
> >   checkpolicy/policy_define.c| 82
> ++
> >   checkpolicy/policy_define.h|  1 +
> >   checkpolicy/policy_parse.y |  5 ++
> >   checkpolicy/policy_scan.l  |  2 +
> >   libsepol/cil/src/cil.c | 15 ++
> >   libsepol/cil/src/cil_build_ast.c   | 72
> ++
> >   libsepol/cil/src/cil_build_ast.h   |  2 +
> >   libsepol/cil/src/cil_copy_ast.c| 26 ++
> >   libsepol/cil/src/cil_flavor.h  |  1 +
> >   libsepol/cil/src/cil_internal.h| 16 --
> >   libsepol/cil/src/cil_post.c|  8 +++
> >   libsepol/cil/src/cil_reset_ast.c   |  1 +
> >   libsepol/cil/src/cil_resolve_ast.c |