Loadable policy module infrastructure
PATCH 1/3 Loadable policy module infrastructure
libsepol/include/sepol/avtab.h +0 -3
@@ -101,9 +101,6 @@avtab_ptr_t avtab_insert_nonunique (avtab_t * h,avtab_key_t * key,avtab_datum_t * datum); -avtab_ptr_t avtab_insert_with_parse_context (avtab_t *h,avtab_key_t *key, -avtab_datum_t *datum,void *parse_context); -avtab_ptr_t avtab_search_node (avtab_t * h,avtab_key_t * key,int specified);avtab_ptr_t avtab_search_node_next (avtab_ptr_t node,int specified);
libsepol/include/sepol/conditional.h +12 -5
@@ -74,27 +74,34 @@typedef struct cond_node {int cur_state;cond_expr_t *expr; +/* these true/false lists point into te_avtab when that is used */ +cond_av_list_t *true_list; +cond_av_list_t *false_list; +/* and these are using during parsing and for modules */ +avrule_t *avtrue_list; +avrule_t *avfalse_list;/* these fields are not written to binary policy */ int nbools;uint32_t bool_ids[COND_MAX_BOOLS ];uint32_t expr_pre_comp;/* */ -cond_av_list_t *true_list; -cond_av_list_t *false_list;struct cond_node *next; }cond_node_t ;int cond_evaluate_expr (policydb_t *p,cond_expr_t *expr); +cond_expr_t *cond_copy_expr (cond_expr_t *expr);int cond_normalize_expr (policydb_t *p,cond_node_t *cn); -cond_node_t *cond_node_search (policydb_t *p,cond_node_t *cn); +void cond_node_destroy (cond_node_t *node); +cond_node_t *cond_node_search (policydb_t *p,cond_node_t *list ,cond_node_t *cn);int evaluate_conds (policydb_t *p); -avtab_datum_t *cond_av_list_search (avtab_key_t *key,cond_av_list_t *cond_list);void cond_optimize_lists (cond_list_t *cl); +avtab_datum_t *cond_av_list_search (avtab_key_t *key,cond_av_list_t *cond_list);int cond_policydb_init (policydb_t * p);void cond_policydb_destroy (policydb_t * p); +void cond_list_destroy (cond_list_t *list);int cond_init_bool_indexes (policydb_t * p);int cond_destroy_bool (hashtab_key_t key,hashtab_datum_t datum,void *p);
@@ -102,7 +109,7 @@int cond_index_bool (hashtab_key_t key,hashtab_datum_t datum,void *datap);int cond_read_bool (policydb_t *p,hashtab_t h,struct policy_file *fp); -int cond_read_list (policydb_t *p,void *fp); +int cond_read_list (policydb_t *p,cond_list_t **list,void *fp);void cond_compute_av (avtab_t *ctab,avtab_key_t *key,struct sepol_av_decision *avd);
libsepol/include/sepol/ebitmap.h +1 -0
@@ -47,6 +47,7 @@int ebitmap_cmp (ebitmap_t * e1,ebitmap_t * e2);int ebitmap_or (ebitmap_t * dst,ebitmap_t * e1,ebitmap_t * e2); +int ebitmap_or_eq (ebitmap_t *dst,ebitmap_t *e1);int ebitmap_cpy (ebitmap_t * dst,ebitmap_t * src);int ebitmap_contains (ebitmap_t * e1,ebitmap_t * e2);int ebitmap_get_bit (ebitmap_t * e,unsigned int bit);
libsepol/include/sepol/expand.h +40 -0
@@ -0,0 +1,40 @@ +/* Authors: Jason Tang <[email protected]> +* Joshua Brindle <[email protected]> +* Karl MacMillan <[email protected]> +* +* A set of utility functions that aid policy decision when dealing +* with hierarchal items. +* +* Copyright (C) 2005 Tresys Technology, LLC +* +* This library is free software; you can redistribute it and/or +* modify it under the terms of the GNU Lesser General Public +* License as published by the Free Software Foundation; either +* version 2.1 of the License, or (at your option) any later version. +* +* This library is distributed in the hope that it will be useful, +* but WITHOUT ANY WARRANTY; without even the implied warranty of +* MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU +* Lesser General Public License for more details. +* +* You should have received a copy of the GNU Lesser General Public +* License along with this library; if not, write to the Free Software +* Foundation, Inc., 51 Franklin St, Fifth Floor, Boston, MA 02110-1301 USA +*/ + +#include <sepol/conditional.h> + +#ifndef _SEPOL_EXPAND_H +#define _SEPOL_EXPAND_H + +int expand_module (policydb_t *base,policydb_t *out, +int verbose,char *error_buf,size_t error_buf_size); +int expand_convert_type_set (policydb_t *p,uint32_t *typemap,type_set_t *set,ebitmap_t *types); +int type_set_expand (type_set_t *set,ebitmap_t *t,policydb_t *p); +int role_set_expand (role_set_t *x,ebitmap_t *r,policydb_t *p); +int expand_rule (policydb_t *source_pol, +avrule_t *source_rule,avtab_t *dest_avtab, +cond_av_list_t **cond,cond_av_list_t **other, +int enabled,char **error_msg); + +#endif
libsepol/include/sepol/flask_types.h +2 -0
@@ -43,9 +43,11 @@* generated header file flask.h. */ typedef uint16_t sepol_security_class_t ; +#define SEPOL_SECCLASS_NULL 0x0000/* no class */ #define SELINUX_MAGIC 0xf97cff8c +#define SELINUX_MOD_MAGIC 0xf97cff8dtypedef uint32_t sepol_security_id_t ;#define SEPOL_SECSID_NULL 0
libsepol/include/sepol/hierarchy.h +5 -8
@@ -1,6 +1,5 @@/* Authors: Jason Tang <[email protected]> * Joshua Brindle <[email protected]> -* Karl MacMillan <[email protected]> * * A set of utility functions that aid policy decision when dealing * with hierarchal items.
@@ -22,12 +21,10 @@* Foundation, Inc., 51 Franklin St, Fifth Floor, Boston, MA 02110-1301 USA */ -#ifndef _HIERARCHY_H_ -#define _HIERARCHY_H_ - -#include <sepol/avtab.h> -#include <sepol/hashtab.h> - -int hierarchy_check_constraints (policydb_t *p,char *error_msg,uint32_t error_len); +#ifndef _SEPOL_HIERARCHY_H +#define _SEPOL_HIERARCHY_H_ +int hierarchy_is_valid_name (char *); +int hierarchy_check_constraints (policydb_t *p,char *error_msg, +uint32_t error_len);#endif
libsepol/include/sepol/policydb.h +144 -4
257 lines not on this page · raw message
libsepol/src/assertion.c +95 -0
95 lines not on this page · raw message
libsepol/src/avtab.c +3 -25
@@ -46,7 +46,7 @@ AVTAB_HASH_MASK) static avtab_ptr_t -avtab_insert_node (avtab_t *h,int hvalue,avtab_ptr_t prev,avtab_ptr_t cur,avtab_key_t *key,avtab_datum_t *datum) +avtab_insert_node (avtab_t *h,int hvalue,avtab_ptr_t prev,avtab_key_t *key,avtab_datum_t *datum) {avtab_ptr_t newnode; newnode = (avtab_ptr_t )malloc (sizeof (struct avtab_node ));
@@ -68,7 +68,6 @@ } -int avtab_insert (avtab_t * h,avtab_key_t * key,avtab_datum_t * datum) {int hvalue;
@@ -97,7 +96,7 @@break ; } - newnode =avtab_insert_node (h, hvalue, prev, cur, key, datum); + newnode =avtab_insert_node (h, hvalue, prev, key, datum);if (!newnode)return -ENOMEM ;
@@ -135,28 +134,7 @@ key->target_class < cur->key.target_class) break; } - newnode =avtab_insert_node (h, hvalue, prev, cur, key, datum); - -return newnode; -} - -/* Unlike avtab_insert(), this function stores a caller-provided parse_context pointer, AND -* allow multiple insertions of the same key/specified mask into the table, AND returns -* a pointer to the new node added, all as needed by the conditional avtab. -*/ -avtab_ptr_t -avtab_insert_with_parse_context (avtab_t *h,avtab_key_t *key,avtab_datum_t *datum,void *parse_context) -{ -avtab_ptr_t newnode; - -if (!h) -return NULL ; - - newnode =avtab_insert_nonunique (h, key, datum); -if (!newnode) -return NULL ; - - newnode->parse_context = parse_context; + newnode =avtab_insert_node (h, hvalue, prev, key, datum);return newnode; }
libsepol/src/conditional.c +49 -11
142 lines not on this page · raw message
libsepol/src/ebitmap.c +35 -1
@@ -1,5 +1,26 @@ -/* Author : Stephen Smalley, <[email protected]> */ +/* Author : Stephen Smalley, <[email protected]> +* +* Updated: Karl MacMillan <[email protected]> +* +* Added support for binary policy modules +* +* Copyright (C) 2005 Tresys Technology, LLC +* +* This library is free software; you can redistribute it and/or +* modify it under the terms of the GNU Lesser General Public +* License as published by the Free Software Foundation; either +* version 2.1 of the License, or (at your option) any later version. +* +* This library is distributed in the hope that it will be useful, +* but WITHOUT ANY WARRANTY; without even the implied warranty of +* MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU +* Lesser General Public License for more details. +* +* You should have received a copy of the GNU Lesser General Public +* License along with this library; if not, write to the Free Software +* Foundation, Inc., 51 Franklin St, Fifth Floor, Boston, MA 02110-1301 USA +*/ /* FLASK */
@@ -58,6 +79,19 @@return 0 ; } +int ebitmap_or_eq (ebitmap_t *dst,ebitmap_t *e1) +{ +int ret; +ebitmap_t tmp; + +if (ebitmap_or (&tmp, dst, e1)) +return -1 ; +ebitmap_destroy (dst); + ret =ebitmap_cpy (dst, &tmp); +ebitmap_destroy (&tmp); + +return ret; +}int ebitmap_cmp (ebitmap_t * e1,ebitmap_t * e2) {
libsepol/src/genbools.c +2 -2
@@ -126,7 +125,7 @@/* Preserve the policy version of the original policy for the new policy. */ -sepol_set_policyvers (policydb.policyvers); +sepol_set_policyvers (POLICY_KERN , policydb.policyvers);if (load_booleans (&policydb, booleans) <0 ) { __sepol_debug_printf("%s: Warning! Error while reading %s\n",
@@ -189,7 +176,7 @@/* Preserve the policy version of the original policy for the new policy. */ -sepol_set_policyvers (policydb.policyvers); +sepol_set_policyvers (POLICY_KERN , policydb.policyvers);for (i =0 ; i < nel; i++) { datum =hashtab_search (policydb.p_bools.table, names[i]);
libsepol/src/genusers.c +5 -5
@@ -81,7 +81,7 @@ usrdatum =hashtab_search (policydb->p_users.table, q);if (usrdatum) {/* Replacing an existing user definition. */ -ebitmap_init (&usrdatum->roles); +ebitmap_init (&usrdatum->roles.roles); usrdatum->defined =1 ; }else {char *id =strdup (q);
@@ -98,7 +98,7 @@ }memset (usrdatum,0 ,sizeof (user_datum_t)); usrdatum->value = ++policydb->p_users.nprim; -ebitmap_init (&usrdatum->roles); +ebitmap_init (&usrdatum->roles.roles); usrdatum->defined =1 ; rc =hashtab_insert (policydb->p_users.table, id, (hashtab_datum_t ) usrdatum);
@@ -158,7 +158,7 @@/* Set the role and every role it dominates */ for (bit =ebitmap_startbit (&roldatum->dominates); bit <ebitmap_length (&roldatum->dominates); bit++) {if (ebitmap_get_bit (&roldatum->dominates, bit)) -if (ebitmap_set_bit (&usrdatum->roles, bit,1 )) { +if (ebitmap_set_bit (&usrdatum->roles.roles, bit,1 )) {__sepol_debug_printf ("%s: out of memory for %s on line %u\n" , path, buffer, lineno); errno =ENOMEM ;
@@ -309,7 +309,7 @@ usrdatum = (user_datum_t *) datum;ebitmap_set_bit (free_users, usrdatum->value -1 ,1 ); -ebitmap_destroy (&usrdatum->roles); +ebitmap_destroy (&usrdatum->roles.roles);free (datum); pol->p_users.nprim--; }
@@ -389,7 +389,7 @@/* Set the policy version for the new binary policy image we are about to generate so that it stays the same as the original, even if we support a newer one. */ -sepol_set_policyvers (policydb.policyvers); +sepol_set_policyvers (policydb.policyvers,POLICY_KERN );/* Compute the length for the new binary policy image. */ pf.type =PF_LEN ;
libsepol/src/hierarchy.c +2 -2
@@ -302,12 +302,12 @@return 1 ; } -if (ebitmap_or (&eb, &r->types, &rp->types)) { +if (ebitmap_or (&eb, &r->types.types, &rp->types.types)) {/* Memory error */ return -1 ; } -if (!ebitmap_cmp (&eb, &rp->types)) { +if (!ebitmap_cmp (&eb, &rp->types.types)) {ebitmap_destroy (&eb);/* This is a violation of the hiearchal constraint, return error condition */ snprintf(a->errmsg, ERRMSG_LEN, "Role hierarchy violation, %s exceeds %s",
libsepol/src/policydb.c +745 -271
1,288 lines not on this page · raw message
libsepol/src/private.h +6 -6
@@ -19,40 +19,40 @@/* Policy compatibility information. */ struct policydb_compat_info { +int type;int version;int sym_num;int ocon_num; }; -extern struct policydb_compat_info *policydb_lookup_compat (int version); +extern struct policydb_compat_info *policydb_lookup_compat (int version,int type);extern void __sepol_debug_printf (const char *fmt, ...);/* Reading from a policy "file". */ static inline void *next_entry (struct policy_file * fp,size_t bytes) { -static unsigned char buffer[BUFSIZ ];size_t nread; -if (bytes >sizeof buffer) +if (bytes >sizeof (fp->buffer))return NULL ;switch (fp->type) {case PF_USE_STDIO : - nread =fread (buffer, bytes,1 , fp->fp); + nread =fread (fp->buffer, bytes,1 , fp->fp);if (nread !=1 )return NULL ;break ;case PF_USE_MEMORY :if (bytes > fp->len)return NULL ; -memcpy (buffer, fp->data, bytes); +memcpy (fp->buffer, fp->data, bytes); fp->data += bytes; fp->len -= bytes;break ;default :return NULL ; } -return buffer; +return fp->buffer; }extern int mls_enabled;
libsepol/src/services.c +4 -4
@@ -1370,13 +1347,13 @@ }memset (mysids,0 , maxnel*sizeof (sepol_security_id_t)); -for (i =ebitmap_startbit (&user->roles); i <ebitmap_length (&user->roles); i++) { -if (!ebitmap_get_bit (&user->roles, i)) +for (i =ebitmap_startbit (&user->roles.roles); i <ebitmap_length (&user->roles.roles); i++) { +if (!ebitmap_get_bit (&user->roles.roles, i))continue ; role = policydb->role_val_to_struct[i]; usercon.role = i+1 ; -for (j =ebitmap_startbit (&role->types); j <ebitmap_length (&role->types); j++) { -if (!ebitmap_get_bit (&role->types, j)) +for (j =ebitmap_startbit (&role->types.types); j <ebitmap_length (&role->types.types); j++) { +if (!ebitmap_get_bit (&role->types.types, j))continue ; usercon.type = j+1 ;if (usercon.type == fromcon->type)
libsepol/src/util.c +135 -0
135 lines not on this page · raw message
libsepol/src/write.c +400 -134
704 lines not on this page · raw message
--
This message was distributed to subscribers of the selinux mailing list. If you no longer wish to subscribe, send mail to [email protected] with the words "unsubscribe selinux" without quotes as the message.
libsepol/include/sepol/ebitmap.h line 53
Quoted 11 lines · Joshua Brindle
diff -burNd
a1/libsepol/include/sepol/ebitmap.hb/libsepol/include/sepol/ebitmap.hlibsepol/include/sepol/ebitmap.h +1
-0@@ -47,6 +47,7 @@int ebitmap_cmp (ebitmap_t * e1,ebitmap_t * e2);int ebitmap_or (ebitmap_t * dst,ebitmap_t * e1,ebitmap_t * e2); +int ebitmap_or_eq (ebitmap_t *dst,ebitmap_t *e1);int ebitmap_cpy (ebitmap_t * dst,ebitmap_t * src);int ebitmap_contains (ebitmap_t * e1,ebitmap_t * e2);int ebitmap_get_bit (ebitmap_t * e,unsigned int bit);
Poor name, suggests equality test rather than assignment.
libsepol/include/sepol/hierarchy.h line 3
Quoted 8 lines
diff -burNd
a1/libsepol/include/sepol/hierarchy.hb/libsepol/include/sepol/hierarchy.hlibsepol/include/sepol/hierarchy.h +0
-1@@ -1,6 +1,5 @@/* Authors: Jason Tang <[email protected]> * Joshua Brindle <[email protected]> -* Karl MacMillan <[email protected]> *
Did you mean to remove attribution for Karl?
Quoted 16 lines
diff -burNd
a1/libsepol/include/sepol/policydb.hb/libsepol/include/sepol/policydb.hlibsepol/include/sepol/policydb.h +5
-0@@ -192,6 +259,11 @@struct genfs *next; } genfs_t; +typedef struct policycon { +uint32_t component_type; +struct ocontext *head; +}policycon_t ; +/* symbol table array indices */ #define SYM_COMMONS 0#define SYM_CLASSES 1
libsepol/include/sepol/policydb.h line 280
Quoted 6 lines
libsepol/include/sepol/policydb.h +3
-0@@ -203,6 +275,10 @@#define SYM_CATS 7#define SYM_NUM 8 +/* this #define gives the index into policycon tables for attributes */ +#define SYM_ATTRIBS (SYM_BOOLS + 1) +#define POLICYCON_NUM (SYM_ATTRIBS + 1)
Did you mean to overlap indices with levels and categories (albeit in a different table)?
Quoted 9 lines
diff -burNd
a1/libsepol/src/assertion.cb/libsepol/src/assertion.clibsepol/src/assertion.c +6
-0+static int check_assertion_helper (policydb_t *p,unsigned int stype,unsigned int ttype, +class_perm_node_t *perm,avtab_t *avtab,unsigned long line) +{ +avtab_key_t avkey; +avtab_datum_t *avdatump; +class_perm_node_t *curperm;
libsepol/src/assertion.c line 42
Quoted 10 lines
libsepol/src/assertion.c +10
-0+ + curperm = perm; + +while (curperm) { + avkey.source_type = stype +1 ; + avkey.target_type = ttype +1 ; + avkey.target_class = curperm->class; + avdatump =avtab_search (avtab, &avkey,AVTAB_AV ); +if (!avdatump) + goto cont;
Strange loop construction. Use a for loop and normal continue statement.
libsepol/src/assertion.c line 49
Quoted 7 lines
libsepol/src/assertion.c +7
-0+ +if ((avdatump->specified &AVTAB_ALLOWED ) && (avtab_allowed (avdatump) & curperm->data)) { +fprintf (stderr,"assertion on line %lu violated by allow %s %s:%s ;\n" , + line, p->p_type_val_to_name[stype], p->p_type_val_to_name[ttype], + p->p_class_val_to_name[curperm->class -1 ]); +return -1 ; + }
Given the avrules representation, what about backtracing (via additional links from avtab) to the actual policy.conf rule that caused the violation? The current output can often be confusing, particularly when the original rule used an attribute.
libsepol/src/assertion.c line 66
Quoted 10 lines
libsepol/src/assertion.c +10
-0+int check_assertions (policydb_t *p,avrule_t *avrules,avtab_t *avtab) +{ +avrule_t *a; +unsigned int i, j; +int errors =0 ; + + a = avrules; +while (a) { +if (!(a->specified &AVRULE_NEVERALLOW )) + goto cont;
Strange loop construction, again.
Quoted 6 lines
diff -burNd
a1/libsepol/src/conditional.cb/libsepol/src/conditional.clibsepol/src/conditional.c +0
-0@@ -220,6 +228,30 @@return s[0 ]; }
libsepol/src/conditional.c line 239
Quoted 10 lines
libsepol/src/conditional.c +9
-0+cond_expr_t *cond_copy_expr (cond_expr_t *expr) +{ +cond_expr_t *cur, *head, *tail, *new_expr; + tail = head =NULL ; + cur = expr; +while (cur) { + new_expr = (cond_expr_t *)malloc (sizeof (cond_expr_t)); +if (!new_expr) +return NULL ;
What frees any nodes that were previously allocated by this loop?
Quoted 10 lines
libsepol/src/conditional.c +1
-1@@ -382,7 +418,7 @@ } } -static void cond_node_destroy (cond_node_t *node) +void cond_node_destroy (cond_node_t *node) {cond_expr_t *cur_expr, *next_expr;
libsepol/src/conditional.c line 436
Quoted 9 lines
libsepol/src/conditional.c +2
-1@@ -393,12 +429,13 @@ next_expr = cur_expr->next;free (cur_expr); } +avrule_list_destroy (node->avtrue_list); +avrule_list_destroy (node->avfalse_list);cond_av_list_destroy (node->true_list);cond_av_list_destroy (node->false_list); -free (node); }
cond_node_destroy() no longer frees the node itself; are all callers handling it now? What about cond_read_node()?
Quoted 10 lines
diff -burNd
a1/libsepol/src/ebitmap.cb/libsepol/src/ebitmap.clibsepol/src/ebitmap.c +3
-0@@ -58,6 +79,19 @@return 0 ; } +int ebitmap_or_eq (ebitmap_t *dst,ebitmap_t *e1) +{ +int ret;
libsepol/src/ebitmap.c line 94
Quoted 10 lines
libsepol/src/ebitmap.c +10
-0+ebitmap_t tmp; + +if (ebitmap_or (&tmp, dst, e1)) +return -1 ; +ebitmap_destroy (dst); + ret =ebitmap_cpy (dst, &tmp); +ebitmap_destroy (&tmp); + +return ret; +}
Why require the extra memory allocation and copying from tmp vs. just overwriting dst with tmp here?
Quoted 16 lines
diff -burNd
a1/libsepol/src/policydb.cb/libsepol/src/policydb.clibsepol/src/policydb.c +9
-0@@ -401,6 +539,25 @@return 0 ; } +int policydb_index_types (policydb_t *p) +{ +free (p->p_type_val_to_name); + p->p_type_val_to_name = (char **)malloc (p->p_types.nprim *sizeof (char *)); +if (!p->p_type_val_to_name) +return -1 ; + +free (p->type_val_to_struct); + p->type_val_to_struct = (type_datum_t**)
libsepol/src/policydb.c line 560
Quoted 10 lines
libsepol/src/policydb.c +10
-0+malloc (p->p_types.nprim *sizeof (type_datum_t *)); +if (!p->type_val_to_struct) +return -1 ; + +if (hashtab_map (p->p_types.table, type_index, p)) +return -1 ; + +return 0 ; +} +
I see no callers of this function in this patchset.
libsepol/src/policydb.c line 578
Quoted 9 lines
libsepol/src/policydb.c +1
-1@@ -415,7 +572,7 @@ p->p_users.nprim, p->p_roles.nprim, p->p_types.nprim, p->p_bools.nprim); -if (mls_enabled) +if (sepol_mls_enabled ())printf (", %d sens, %d cats" , p->p_levels.nprim, p->p_cats.nprim);
Why did you change this? Within libsepol, there is no reason to use the function interface for getting the value of mls_enabled.
libsepol/src/policydb.c line 879
Quoted 6 lines
libsepol/src/policydb.c +2
-0@@ -699,8 +875,11 @@ {role_datum_t *role;user_datum_t *usrdatum; +ebitmap_t types, roles; +int ret =1 ;
ret looks unused.
@@ -715,8 +894,11 @@
libsepol/src/policydb.c line 903
Quoted 12 lines
libsepol/src/policydb.c +5
-2* Role must be authorized for the type. */ role = p->role_val_to_struct[c->role -1 ]; -if (!ebitmap_get_bit (&role->types, - c->type -1 )) +if (type_set_expand (&role->types, &types, p)) { +printf ("something bad happened while expanding types\n" ); +return 0 ; + } +if (!ebitmap_get_bit (&types, c->type -1 ))/* role may not be associated with type */ return 0 ;
Performing the set expansion on every isvalid check seems heavyweight and prone to possible memory allocation failure. I'd recommend pre- expansion and caching in the struct prior to any isvalid calls.
Quoted 4 lines
libsepol/src/policydb.c +0
-0@@ -727,8 +909,11 @@if (!usrdatum)return 0 ;
libsepol/src/policydb.c line 919
Quoted 10 lines
libsepol/src/policydb.c +5
-2-if (!ebitmap_get_bit (&usrdatum->roles, - c->role -1 )) +if (role_set_expand (&usrdatum->roles, &roles, p)) { +printf ("something bad happened while expanding roles\n" ); +return 0 ; + } +if (!ebitmap_get_bit (&roles, c->role -1 ))/* user may not be associated with role */ return 0 ; }
Ditto.
More to come as time permits. BTW, diff -p would be nicer.
Signature · 8 lines
Stephen Smalley National Security Agency -- This message was distributed to subscribers of the selinux mailing list. If you no longer wish to subscribe, send mail to [email protected] with the words "unsubscribe selinux" without quotes as the message.
I assume that these will be addressed by later patches, but not clear why they were omitted from this patchset:
$ grep FIX 01-module.patch + /* FIX ME: do the alternate read here for modules */ + /* FIX ME: policy-server will need the SID +/* FIX ME: do policycon write here */ + /* FIX ME: write module version here */
Signature · 8 lines
Stephen Smalley National Security Agency -- This message was distributed to subscribers of the selinux mailing list. If you no longer wish to subscribe, send mail to [email protected] with the words "unsubscribe selinux" without quotes as the message.
Review
Quoted 8 lines · Stephen Smalley
$ grep FIX 01-module.patch(quoted diff) +4
-0+ /* FIX ME: do the alternate read here for modules */ + /* FIX ME: policy-server will need the SID +/* FIX ME: do policycon write here */ + /* FIX ME: write module version here */
They were excluded in an effort to make these patches smaller. They will be included in the patches that implement that module language and linking code.
--
This message was distributed to subscribers of the selinux mailing list. If you no longer wish to subscribe, send mail to [email protected] with the words "unsubscribe selinux" without quotes as the message.
Quoted 11 lines · Joshua Brindle
diff -burNd
a1/libsepol/src/util.cb/libsepol/src/util.clibsepol/src/util.c +8
-0+int type_set_or (type_set_t *dst,type_set_t *a,type_set_t *b) +{ +type_set_init (dst); + +if (ebitmap_or (&dst->types, &a->types, &b->types)) { +fprintf (stderr,"Memory error\n" ); +return -1 ; + }
libsepol/src/util.c line 48
Quoted 10 lines
libsepol/src/util.c +10
-0+if (ebitmap_or (&dst->negset, &a->negset, &b->negset)) { +fprintf (stderr,"Memory error\n" ); +return -1 ; + } + + dst->flags |= a->flags; + dst->flags |= b->flags; + +return 0 ; +}
I don't see any users of this function or even an exported function prototype for it (or for a number of other functions), but am uncertain about its semantics if it is going to be used. The simple or'ing of the negset and flags means that:
- anything excluded from either set will be excluded from the result even if it was present in the other set,
- if either set is a set complement, then the result will be the complement of the union.
That's quite different from an ebitmap_or() of the expanded type sets.
Signature · 8 lines
Stephen Smalley National Security Agency -- This message was distributed to subscribers of the selinux mailing list. If you no longer wish to subscribe, send mail to [email protected] with the words "unsubscribe selinux" without quotes as the message.
Quoted 32 lines · Stephen Smalley
Quoted 11 lines · Joshua Brindle
diff -burNd
a1/libsepol/src/util.cb/libsepol/src/util.clibsepol/src/util.c +8
-0+int type_set_or (type_set_t *dst,type_set_t *a,type_set_t *b) +{ +type_set_init (dst); + +if (ebitmap_or (&dst->types, &a->types, &b->types)) { +fprintf (stderr,"Memory error\n" ); +return -1 ; + }libsepol/src/util.c line 48
Quoted 10 lines
libsepol/src/util.c +10
-0+if (ebitmap_or (&dst->negset, &a->negset, &b->negset)) { +fprintf (stderr,"Memory error\n" ); +return -1 ; + } + + dst->flags |= a->flags; + dst->flags |= b->flags; + +return 0 ; +}I don't see any users of this function or even an exported function prototype for it (or for a number of other functions), but am uncertain about its semantics if it is going to be used. The simple or'ing of the negset and flags means that:
- anything excluded from either set will be excluded from the result even if it was present in the other set,
- if either set is a set complement, then the result will be the complement of the union.
Quoted 2 lines
That's quite different from an
ebitmap_or()of the expanded type sets.
You are right, the semantics of this are funny, this is used in the linking code to essentially copy avrules from modules to the base. The best thing to do is probably get rid of this or function and replace it with a copy function so that we don't run into bugs.
Joshua
--
This message was distributed to subscribers of the selinux mailing list. If you no longer wish to subscribe, send mail to [email protected] with the words "unsubscribe selinux" without quotes as the message.
Quoted 6 lines · Stephen Smalley
Quoted 5 lines · Joshua Brindle
diff -burNd
a1/libsepol/include/sepol/ebitmap.hb/libsepol/include/sepol/ebitmap.hlibsepol/include/sepol/ebitmap.h +0
-0@@ -47,6 +47,7 @@
Quoted 9 lines
libsepol/include/sepol/ebitmap.h line 53
Quoted 6 lines
libsepol/include/sepol/ebitmap.h +1
-0int ebitmap_cmp (ebitmap_t * e1,ebitmap_t * e2);int ebitmap_or (ebitmap_t * dst,ebitmap_t * e1,ebitmap_t * e2); +int ebitmap_or_eq (ebitmap_t *dst,ebitmap_t *e1);int ebitmap_cpy (ebitmap_t * dst,ebitmap_t * src);int ebitmap_contains (ebitmap_t * e1,ebitmap_t * e2);int ebitmap_get_bit (ebitmap_t * e,unsigned int bit);Poor name, suggests equality test rather than assignment.
would ebitmap_inplace_or be better?
Quoted 11 lines
libsepol/include/sepol/hierarchy.h line 3
Quoted 8 lines
diff -burNd
a1/libsepol/include/sepol/hierarchy.hb/libsepol/include/sepol/hierarchy.hlibsepol/include/sepol/hierarchy.h +0
-1@@ -1,6 +1,5 @@/* Authors: Jason Tang <[email protected]> * Joshua Brindle <[email protected]> -* Karl MacMillan <[email protected]> * Did you mean to remove attribution for Karl?
Ofcourse! :)
This happened when I copied over the new hierarchy.h which Karl hadn't worked on, I'll add him back in the next patch.
Quoted 19 lines
Quoted 19 lines
diff -burNd
a1/libsepol/include/sepol/policydb.hb/libsepol/include/sepol/policydb.hlibsepol/include/sepol/policydb.h +5
-0@@ -192,6 +259,11 @@struct genfs *next; } genfs_t; +typedef struct policycon { +uint32_t component_type; +struct ocontext *head; +}policycon_t ; +/* symbol table array indices */ #define SYM_COMMONS 0#define SYM_CLASSES 1@@ -203,6 +275,10 @@#define SYM_CATS 7#define SYM_NUM 8
Quoted 7 lines
libsepol/include/sepol/policydb.h line 280
Quoted 3 lines
libsepol/include/sepol/policydb.h +3
-0+/* this #define gives the index into policycon tables for attributes */ +#define SYM_ATTRIBS (SYM_BOOLS + 1) +#define POLICYCON_NUM (SYM_ATTRIBS + 1)Did you mean to overlap indices with levels and categories (albeit in a different table)?
Yes we meant to overlap them but we could just add extra symbol tables, I guess the original thinking was to keep the same number of tables if possible. We could leave them overlapping until we add support for levels and categories in modules, however, and the version will take care of the offsets.
Quoted 14 lines
Quoted 14 lines
diff -burNd
a1/libsepol/src/assertion.cb/libsepol/src/assertion.clibsepol/src/assertion.c +11
-0+static int check_assertion_helper (policydb_t *p,unsigned int stype,unsigned int ttype, +class_perm_node_t *perm,avtab_t *avtab,unsigned long line) +{ +avtab_key_t avkey; +avtab_datum_t *avdatump; +class_perm_node_t *curperm; + + curperm = perm; + +while (curperm) { + avkey.source_type = stype +1 ;
Quoted 8 lines
libsepol/src/assertion.c line 42
Quoted 5 lines
libsepol/src/assertion.c +5
-0+ avkey.target_type = ttype +1 ; + avkey.target_class = curperm->class; + avdatump =avtab_search (avtab, &avkey,AVTAB_AV ); +if (!avdatump) + goto cont;Strange loop construction. Use a for loop and normal continue statement.
ok
Quoted 9 lines
libsepol/src/assertion.c line 49
Quoted 8 lines
libsepol/src/assertion.c +7
-0+ +if ((avdatump->specified &AVTAB_ALLOWED ) && (avtab_allowed (avdatump) & curperm->data)) { +fprintf (stderr,"assertion on line %lu violated by allow %s %s:%s ;\n" , + line, p->p_type_val_to_name[stype], p->p_type_val_to_name[ttype], + p->p_class_val_to_name[curperm->class -1 ]); +return -1 ; + }
Quoted 5 lines
Given the avrules representation, what about backtracing (via additional links from avtab) to the actual policy.conf rule that caused the violation? The current output can often be confusing, particularly when the original rule used an attribute.
By the time we check the assertions the avrules are gone. Also, we'd have to store a list of avrule entries that contributed to specific avtab entry, and then go through them and decide which ones contributed in a way that would violate the assertion. It's doable but complex.
Quoted 4 lines
Quoted 4 lines
libsepol/src/assertion.c +4
-0+int check_assertions (policydb_t *p,avrule_t *avrules,avtab_t *avtab) +{ +avrule_t *a; +unsigned int i, j;
Quoted 9 lines
libsepol/src/assertion.c line 66
Quoted 6 lines
libsepol/src/assertion.c +6
-0+int errors =0 ; + + a = avrules; +while (a) { +if (!(a->specified &AVRULE_NEVERALLOW )) + goto cont;Strange loop construction, again.
this was taken largely from the original assertion checking functions incheckpolicy.c and I just added the goto cont; for the unlikely !neverallow case without rewriting the whole loop.
Quoted 7 lines
Quoted 7 lines
diff -burNd
a1/libsepol/src/conditional.cb/libsepol/src/conditional.clibsepol/src/conditional.c +0
-0@@ -220,6 +228,30 @@return s[0 ]; }
Quoted 12 lines
libsepol/src/conditional.c line 239
Quoted 9 lines
libsepol/src/conditional.c +9
-0+cond_expr_t *cond_copy_expr (cond_expr_t *expr) +{ +cond_expr_t *cur, *head, *tail, *new_expr; + tail = head =NULL ; + cur = expr; +while (cur) { + new_expr = (cond_expr_t *)malloc (sizeof (cond_expr_t)); +if (!new_expr) +return NULL ;What frees any nodes that were previously allocated by this loop?
They should always be freed by cond_node_destroy
Quoted 9 lines
Quoted 9 lines
libsepol/src/conditional.c +1
-1@@ -382,7 +418,7 @@ } } -static void cond_node_destroy (cond_node_t *node) +void cond_node_destroy (cond_node_t *node) {cond_expr_t *cur_expr, *next_expr;
Quoted 14 lines
libsepol/src/conditional.c line 436
Quoted 10 lines
libsepol/src/conditional.c +2
-1@@ -393,12 +429,13 @@ next_expr = cur_expr->next;free (cur_expr); } +avrule_list_destroy (node->avtrue_list); +avrule_list_destroy (node->avfalse_list);cond_av_list_destroy (node->true_list);cond_av_list_destroy (node->false_list); -free (node); }
cond_node_destroy()no longer frees the node itself; are all callers handling it now? What aboutcond_read_node()?
cond_list_destroy now frees the node, policy_parse.y calls it and currently leaks the node, will fix. The reason it was changed was for consistency, the other list destroyers free the node.
Quoted 7 lines
Quoted 7 lines
diff -burNd
a1/libsepol/src/ebitmap.cb/libsepol/src/ebitmap.clibsepol/src/ebitmap.c +0
-0@@ -58,6 +79,19 @@return 0 ; }
Quoted 17 lines
libsepol/src/ebitmap.c line 94
Quoted 13 lines
libsepol/src/ebitmap.c +13
-0+int ebitmap_or_eq (ebitmap_t *dst,ebitmap_t *e1) +{ +int ret; +ebitmap_t tmp; + +if (ebitmap_or (&tmp, dst, e1)) +return -1 ; +ebitmap_destroy (dst); + ret =ebitmap_cpy (dst, &tmp); +ebitmap_destroy (&tmp); + +return ret; +}Why require the extra memory allocation and copying from tmp vs. just overwriting dst with tmp here?
This function is called from the linking code by passing in a reference to an ebitmap inside, say, a role set so it is unsafe to assign.
Quoted 20 lines
Quoted 20 lines
diff -burNd
a1/libsepol/src/policydb.cb/libsepol/src/policydb.clibsepol/src/policydb.c +13
-0@@ -401,6 +539,25 @@return 0 ; } +int policydb_index_types (policydb_t *p) +{ +free (p->p_type_val_to_name); + p->p_type_val_to_name = (char **)malloc (p->p_types.nprim *sizeof (char *)); +if (!p->p_type_val_to_name) +return -1 ; + +free (p->type_val_to_struct); + p->type_val_to_struct = (type_datum_t **) +malloc (p->p_types.nprim *sizeof (type_datum_t *)); +if (!p->type_val_to_struct) +return -1 ; +
Quoted 9 lines
libsepol/src/policydb.c line 560
Quoted 6 lines
libsepol/src/policydb.c +6
-0+if (hashtab_map (p->p_types.table, type_index, p)) +return -1 ; + +return 0 ; +} +I see no callers of this function in this patchset.
The standalone hierarchy checker calls this, which will be used with semodule and policy server to verify policy consistency post-expand. It will be included in the next patch.
Quoted 4 lines
Quoted 4 lines
libsepol/src/policydb.c +0
-0@@ -415,7 +572,7 @@ p->p_users.nprim, p->p_roles.nprim, p->p_types.nprim, p->p_bools.nprim);
Quoted 9 lines
libsepol/src/policydb.c line 578
Quoted 5 lines
libsepol/src/policydb.c +1
-1-if (mls_enabled) +if (sepol_mls_enabled ())printf (", %d sens, %d cats" , p->p_levels.nprim, p->p_cats.nprim);Why did you change this? Within libsepol, there is no reason to use the function interface for getting the value of
mls_enabled.
fair enough, will revert, the reasoning was that it might be necessary to return false while linking or expanding or something, we can change this back when it's necessary though. The optimizer will inline the variable reference anyway.
Quoted 18 lines
libsepol/src/policydb.c line 879
Quoted 6 lines
libsepol/src/policydb.c +2
-0@@ -699,8 +875,11 @@ {role_datum_t *role;user_datum_t *usrdatum; +ebitmap_t types, roles; +int ret =1 ;ret looks unused.
(quoted diff) +0
-0@@ -715,8 +894,11 @@Quoted 8 lines
libsepol/src/policydb.c +3
-2* Role must be authorized for the type. */ role = p->role_val_to_struct[c->role -1 ]; -if (!ebitmap_get_bit (&role->types, - c->type -1 )) +if (type_set_expand (&role->types, &types, p)) { +printf ("something bad happened while expanding types\n" ); +return 0 ;
Quoted 9 lines
libsepol/src/policydb.c line 903
Quoted 4 lines
libsepol/src/policydb.c +2
-0+ } +if (!ebitmap_get_bit (&types, c->type -1 ))/* role may not be associated with type */ return 0 ;Performing the set expansion on every isvalid check seems heavyweight and prone to possible memory allocation failure. I'd recommend pre- expansion and caching in the struct prior to any isvalid calls.
Yes, pre-expansion (or even checking context validity after expansion) would be better, we were more worried about correctness than performance this time, we'll optimize these things later.
Quoted 17 lines
Quoted 4 lines
libsepol/src/policydb.c +0
-0@@ -727,8 +909,11 @@if (!usrdatum)return 0 ;libsepol/src/policydb.c line 919
Quoted 10 lines
libsepol/src/policydb.c +5
-2-if (!ebitmap_get_bit (&usrdatum->roles, - c->role -1 )) +if (role_set_expand (&usrdatum->roles, &roles, p)) { +printf ("something bad happened while expanding roles\n" ); +return 0 ; + } +if (!ebitmap_get_bit (&roles, c->role -1 ))/* user may not be associated with role */ return 0 ; }Ditto.
Quoted 2 lines
More to come as time permits. BTW, diff -p would be nicer.
note taken to add -p next time.
Joshua Brindle
--
This message was distributed to subscribers of the selinux mailing list. If you no longer wish to subscribe, send mail to [email protected] with the words "unsubscribe selinux" without quotes as the message.
Quoted 1 line · Joshua Brindle
would
ebitmap_inplace_orbe better?
I think you can do better.
Quoted 1 line
This function is called from the linking code by passing in a reference to an ebitmap inside, say, a role set so it is unsafe to assign.
I don't follow. In either case, you are destroying dst and mutating it in place; current code does it by ebitmap_cpy(dst, &tmp) and then destroys tmp, but you could just as easily memcpy(dst, &tmp, sizeof *dst) and not destroy tmp, avoiding any extra allocation and copy. In either case, dst->node and dst->highbit fields are mutated.
Quoted 1 line
Yes, pre-expansion (or even checking context validity after expansion) would be better, we were more worried about correctness than performance this time, we'll optimize these things later
It isn't significantly different than having to index (to generate the val_to_struct arrays) prior to calling this function. And deferring the expansion means you have a new failure mode from this function due to OOM condition that didn't exist previously.
Signature · 8 lines
Stephen Smalley National Security Agency -- This message was distributed to subscribers of the selinux mailing list. If you no longer wish to subscribe, send mail to [email protected] with the words "unsubscribe selinux" without quotes as the message.
libsepol/src/assertion.c line 48
Quoted 8 lines · Joshua Brindle
diff -burNd
a1/libsepol/src/assertion.cb/libsepol/src/assertion.clibsepol/src/assertion.c +5
-0+if ((avdatump->specified &AVTAB_ALLOWED ) && (avtab_allowed (avdatump) & curperm->data)) { +fprintf (stderr,"assertion on line %lu violated by allow %s %s:%s ;\n" , + line, p->p_type_val_to_name[stype], p->p_type_val_to_name[ttype], + p->p_class_val_to_name[curperm->class -1 ]); +return -1 ;
The original code in checkpolicy also displayed the access vector as a string, which this omits.
Signature · 8 lines
Stephen Smalley National Security Agency -- This message was distributed to subscribers of the selinux mailing list. If you no longer wish to subscribe, send mail to [email protected] with the words "unsubscribe selinux" without quotes as the message.
Quoted 13 lines · Stephen Smalley
libsepol/src/assertion.c line 48
Quoted 8 lines · Joshua Brindle
diff -burNd
a1/libsepol/src/assertion.cb/libsepol/src/assertion.clibsepol/src/assertion.c +5
-0+if ((avdatump->specified &AVTAB_ALLOWED ) && (avtab_allowed (avdatump) & curperm->data)) { +fprintf (stderr,"assertion on line %lu violated by allow %s %s:%s ;\n" , + line, p->p_type_val_to_name[stype], p->p_type_val_to_name[ttype], + p->p_class_val_to_name[curperm->class -1 ]); +return -1 ;The original code in checkpolicy also displayed the access vector as a string, which this omits.
you caught me!
I just added this back in, it'll be in the next patchset.
--
This message was distributed to subscribers of the selinux mailing list. If you no longer wish to subscribe, send mail to [email protected] with the words "unsubscribe selinux" without quotes as the message.
The call to sepol_set_policyvers() in sepol_genusers() passes the arguments in the wrong order, thereby ending up with an invalid policy version, which causes a failure upon the attempt to generate the new binary policy image. This breaks load_policy and /sbin/init in FC4 (to be precise, it prevents proper loading of local user definitions).
Unrelated note: While the diff -b reduces the "noise" in the patch, it also leads to inappropriate indentation in the resulting code because you don't end up adjusting indentation when it is appropriate to do so due to your code modifications. So you should just diff normally and review the resulting diff.
Signature · 8 lines
Stephen Smalley National Security Agency -- This message was distributed to subscribers of the selinux mailing list. If you no longer wish to subscribe, send mail to [email protected] with the words "unsubscribe selinux" without quotes as the message.