[PATCH 0/2] userspace: Implement new format of filename trans rules

[PATCH 0/2] userspace: Implement new format of filename trans rules

[Ondrej Mosnacek]

These patches are the userspace side of the kernel change posted at [1].

The first patch changes libsepol's internal representation of filename
transition rules in a way similar to kernel commit c3a276111ea2
("selinux: optimize storage of filename transitions") [2].

The second patch then builds upon that and implements reading and
writing of a new binary policy format that uses this representation also
in the data layout.

See individual patches for more details.

NOTE: This series unfortunately breaks the build of setools. Moreover,
when an existing build of setools dynamically links against the new
libsepol, it segfaults. Sadly, there doesn't seem to be a nice way of
handling this, since setools relies on non-public libsepol policydb
API/ABI.

[1] https://lore.kernel.org/selinux/20200327151941.95619-1-omosnace@redhat.com/T/
[2] https://git.kernel.org/pub/scm/linux/kernel/git/pcmoore/selinux.git/commit/?id=c3a276111ea2572399281988b3129683e2a6b60b

Ondrej Mosnacek (2):
  libsepol,checkpolicy: optimize storage of filename transitions
  libsepol: implement POLICYDB_VERSION_COMP_FTRANS

 checkpolicy/policy_define.c                |  52 ++--
 checkpolicy/test/dispol.c                  |  27 +-
 libsepol/cil/src/cil_binary.c              |  29 +-
 libsepol/include/sepol/policydb/policydb.h |  18 +-
 libsepol/src/expand.c                      |  60 +---
 libsepol/src/kernel_to_cil.c               |  24 +-
 libsepol/src/kernel_to_conf.c              |  24 +-
 libsepol/src/policydb.c                    | 313 ++++++++++++++++-----
 libsepol/src/write.c                       | 100 +++++--
 9 files changed, 440 insertions(+), 207 deletions(-)

-- 
2.25.1

[PATCH 1/2] libsepol,checkpolicy: optimize storage of filename transitions

[Ondrej Mosnacek]

In preparation to support a new policy format with a more optimal
representation of filename transition rules, this patch applies an
equivalent change from kernel commit c3a276111ea2 ("selinux: optimize
storage of filename transitions").

See the kernel commit's description [1] for the rationale behind this
representation. This change doesn't bring any measurable difference of
policy build performance (semodule -B) on Fedora.

[1] https://git.kernel.org/pub/scm/linux/kernel/git/pcmoore/selinux.git/commit/?id=c3a276111ea2572399281988b3129683e2a6b60b

Signed-off-by: Ondrej Mosnacek <omosnace@redhat.com>
---
 checkpolicy/policy_define.c                |  52 +++------
 checkpolicy/test/dispol.c                  |  27 +++--
 libsepol/cil/src/cil_binary.c              |  29 ++---
 libsepol/include/sepol/policydb/policydb.h |  15 ++-
 libsepol/src/expand.c                      |  60 +++-------
 libsepol/src/kernel_to_cil.c               |  24 +++-
 libsepol/src/kernel_to_conf.c              |  24 +++-
 libsepol/src/policydb.c                    | 126 ++++++++++++++-------
 libsepol/src/write.c                       |  46 ++++----
 9 files changed, 223 insertions(+), 180 deletions(-)

diff --git a/checkpolicy/policy_define.c b/checkpolicy/policy_define.c
index c6733fa4..01a90438 100644
--- a/checkpolicy/policy_define.c
+++ b/checkpolicy/policy_define.c
@@ -3303,10 +3303,9 @@ int define_filename_trans(void)
 	ebitmap_t e_stypes, e_ttypes;
 	ebitmap_t e_tclasses;
 	ebitmap_node_t *snode, *tnode, *cnode;
-	filename_trans_t *ft;
-	filename_trans_datum_t *ftdatum;
 	filename_trans_rule_t *ftr;
 	type_datum_t *typdatum;
+	char *dup_name;
 	uint32_t otype;
 	unsigned int c, s, t;
 	int add, rc;
@@ -3388,40 +3387,21 @@ int define_filename_trans(void)
 	ebitmap_for_each_positive_bit(&e_tclasses, cnode, c) {
 		ebitmap_for_each_positive_bit(&e_stypes, snode, s) {
 			ebitmap_for_each_positive_bit(&e_ttypes, tnode, t) {
-				ft = calloc(1, sizeof(*ft));
-				if (!ft) {
-					yyerror("out of memory");
-					goto bad;
-				}
-				ft->stype = s+1;
-				ft->ttype = t+1;
-				ft->tclass = c+1;
-				ft->name = strdup(name);
-				if (!ft->name) {
-					yyerror("out of memory");
-					goto bad;
-				}
-
-				ftdatum = hashtab_search(policydbp->filename_trans,
-							 (hashtab_key_t)ft);
-				if (ftdatum) {
-					yyerror2("duplicate filename transition for: filename_trans %s %s %s:%s",
-						 name,
-						 policydbp->p_type_val_to_name[s],
-						 policydbp->p_type_val_to_name[t],
-						 policydbp->p_class_val_to_name[c]);
-					goto bad;
-				}
-
-				ftdatum = calloc(1, sizeof(*ftdatum));
-				if (!ftdatum) {
-					yyerror("out of memory");
-					goto bad;
-				}
-				rc = hashtab_insert(policydbp->filename_trans,
-						    (hashtab_key_t)ft,
-						    ftdatum);
-				if (rc) {
+				dup_name = NULL;
+				rc = policydb_filetrans_insert(
+					policydbp, s+1, t+1, c+1, name,
+					&dup_name, otype, NULL
+				);
+				free(dup_name);
+				if (rc != SEPOL_OK) {
+					if (rc == SEPOL_EEXIST) {
+						yyerror2("duplicate filename transition for: filename_trans %s %s %s:%s",
+							name,
+							policydbp->p_type_val_to_name[s],
+							policydbp->p_type_val_to_name[t],
+							policydbp->p_class_val_to_name[c]);
+						goto bad;
+					}
 					yyerror("out of memory");
 					goto bad;
 				}
diff --git a/checkpolicy/test/dispol.c b/checkpolicy/test/dispol.c
index d72d9fb3..7c74e9cf 100644
--- a/checkpolicy/test/dispol.c
+++ b/checkpolicy/test/dispol.c
@@ -329,26 +329,39 @@ static void display_role_trans(policydb_t *p, FILE *fp)
 struct filenametr_display_args {
 	policydb_t *p;
 	FILE *fp;
+	filename_trans_key_t *ft;
 };
 
-static int filenametr_display(hashtab_key_t key,
-			      hashtab_datum_t datum,
-			      void *ptr)
+static int filenametr_display_sub(hashtab_key_t key,
+				  hashtab_datum_t datum,
+				  void *ptr)
 {
-	struct filename_trans *ft = (struct filename_trans *)key;
-	struct filename_trans_datum *ftdatum = datum;
 	struct filenametr_display_args *args = ptr;
+	filename_trans_key_t *ft = args->ft;
 	policydb_t *p = args->p;
 	FILE *fp = args->fp;
+	uint32_t stype = (uintptr_t)key;
+	uint32_t otype = (uintptr_t)datum;
 
-	display_id(p, fp, SYM_TYPES, ft->stype - 1, "");
+	display_id(p, fp, SYM_TYPES, stype - 1, "");
 	display_id(p, fp, SYM_TYPES, ft->ttype - 1, "");
 	display_id(p, fp, SYM_CLASSES, ft->tclass - 1, ":");
-	display_id(p, fp, SYM_TYPES, ftdatum->otype - 1, "");
+	display_id(p, fp, SYM_TYPES, otype - 1, "");
 	fprintf(fp, " %s\n", ft->name);
 	return 0;
 }
 
+static int filenametr_display(hashtab_key_t key,
+			      hashtab_datum_t datum,
+			      void *ptr)
+{
+	struct filenametr_display_args *args = ptr;
+	hashtab_t subhash = datum;
+
+	args->ft = (filename_trans_key_t *)key;
+	return hashtab_map(subhash, filenametr_display_sub, args);
+}
+
 
 static void display_filename_trans(policydb_t *p, FILE *fp)
 {
diff --git a/libsepol/cil/src/cil_binary.c b/libsepol/cil/src/cil_binary.c
index 62178d99..bedff628 100644
--- a/libsepol/cil/src/cil_binary.c
+++ b/libsepol/cil/src/cil_binary.c
@@ -1131,13 +1131,13 @@ int __cil_typetransition_to_avtab(policydb_t *pdb, const struct cil_db *db, stru
 	class_datum_t *sepol_obj = NULL;
 	struct cil_list *class_list;
 	type_datum_t *sepol_result = NULL;
-	filename_trans_t *newkey = NULL;
-	filename_trans_datum_t *newdatum = NULL, *otype = NULL;
 	ebitmap_t src_bitmap, tgt_bitmap;
 	ebitmap_node_t *node1, *node2;
 	unsigned int i, j;
+	uint32_t otype;
 	struct cil_list_item *c;
 	char *name = DATUM(typetrans->name)->name;
+	char *dup_name;
 
 	if (name == CIL_KEY_STAR) {
 		struct cil_type_rule trans;
@@ -1176,22 +1176,16 @@ int __cil_typetransition_to_avtab(policydb_t *pdb, const struct cil_db *db, stru
 				rc = __cil_get_sepol_class_datum(pdb, DATUM(c->data), &sepol_obj);
 				if (rc != SEPOL_OK) goto exit;
 
-				newkey = cil_calloc(1, sizeof(*newkey));
-				newdatum = cil_calloc(1, sizeof(*newdatum));
-				newkey->stype = sepol_src->s.value;
-				newkey->ttype = sepol_tgt->s.value;
-				newkey->tclass = sepol_obj->s.value;
-				newkey->name = cil_strdup(name);
-				newdatum->otype = sepol_result->s.value;
-
-				rc = hashtab_insert(pdb->filename_trans,
-						    (hashtab_key_t)newkey,
-						    newdatum);
+				dup_name = NULL;
+				rc = policydb_filetrans_insert(
+					pdb, sepol_src->s.value, sepol_tgt->s.value,
+					sepol_obj->s.value, name, &dup_name,
+					sepol_result->s.value, &otype
+				);
+				free(dup_name);
 				if (rc != SEPOL_OK) {
 					if (rc == SEPOL_EEXIST) {
-						otype = hashtab_search(pdb->filename_trans,
-								(hashtab_key_t)newkey);
-						if (newdatum->otype != otype->otype) {
+						if (sepol_result->s.value!= otype) {
 							cil_log(CIL_ERR, "Conflicting name type transition rules\n");
 						} else {
 							rc = SEPOL_OK;
@@ -1199,9 +1193,6 @@ int __cil_typetransition_to_avtab(policydb_t *pdb, const struct cil_db *db, stru
 					} else {
 						cil_log(CIL_ERR, "Out of memory\n");
 					}
-					free(newkey->name);
-					free(newkey);
-					free(newdatum);
 					if (rc != SEPOL_OK) {
 						goto exit;
 					}
diff --git a/libsepol/include/sepol/policydb/policydb.h b/libsepol/include/sepol/policydb/policydb.h
index 81b63fef..c3180c61 100644
--- a/libsepol/include/sepol/policydb/policydb.h
+++ b/libsepol/include/sepol/policydb/policydb.h
@@ -162,15 +162,16 @@ typedef struct role_allow {
 } role_allow_t;
 
 /* filename_trans rules */
-typedef struct filename_trans {
-	uint32_t stype;
+typedef struct filename_trans_key {
 	uint32_t ttype;
 	uint32_t tclass;
 	char *name;
-} filename_trans_t;
+} filename_trans_key_t;
 
 typedef struct filename_trans_datum {
-	uint32_t otype;		/* expected of new object */
+	ebitmap_t stypes;
+	uint32_t otype;
+	struct filename_trans_datum *next;
 } filename_trans_datum_t;
 
 /* Type attributes */
@@ -591,6 +592,7 @@ typedef struct policydb {
 
 	/* file transitions with the last path component */
 	hashtab_t filename_trans;
+	uint32_t filename_trans_count;
 
 	ebitmap_t *type_attr_map;
 
@@ -650,6 +652,11 @@ extern int policydb_load_isids(policydb_t * p, sidtab_t * s);
 
 extern int policydb_sort_ocontexts(policydb_t *p);
 
+extern int policydb_filetrans_insert(policydb_t *p, uint32_t stype,
+				     uint32_t ttype, uint32_t tclass,
+				     const char *name, char **name_alloc,
+				     uint32_t otype, uint32_t *present_otype);
+
 /* Deprecated */
 extern int policydb_context_isvalid(const policydb_t * p,
 				    const context_struct_t * c);
diff --git a/libsepol/src/expand.c b/libsepol/src/expand.c
index 529e1d35..28f93acb 100644
--- a/libsepol/src/expand.c
+++ b/libsepol/src/expand.c
@@ -1371,16 +1371,15 @@ static int copy_role_trans(expand_state_t * state, role_trans_rule_t * rules)
 static int expand_filename_trans(expand_state_t *state, filename_trans_rule_t *rules)
 {
 	unsigned int i, j;
-	filename_trans_t key, *new_trans;
-	filename_trans_datum_t *otype;
 	filename_trans_rule_t *cur_rule;
 	ebitmap_t stypes, ttypes;
 	ebitmap_node_t *snode, *tnode;
+	char *name;
 	int rc;
 
 	cur_rule = rules;
 	while (cur_rule) {
-		uint32_t mapped_otype;
+		uint32_t mapped_otype, present_otype;
 
 		ebitmap_init(&stypes);
 		ebitmap_init(&ttypes);
@@ -1401,15 +1400,17 @@ static int expand_filename_trans(expand_state_t *state, filename_trans_rule_t *r
 
 		ebitmap_for_each_positive_bit(&stypes, snode, i) {
 			ebitmap_for_each_positive_bit(&ttypes, tnode, j) {
-				key.stype = i + 1;
-				key.ttype = j + 1;
-				key.tclass = cur_rule->tclass;
-				key.name = cur_rule->name;
-				otype = hashtab_search(state->out->filename_trans,
-						       (hashtab_key_t) &key);
-				if (otype) {
+				name = NULL;
+
+				rc = policydb_filetrans_insert(
+					state->out, i + 1, j + 1,
+					cur_rule->tclass, cur_rule->name,
+					&name, mapped_otype, &present_otype
+				);
+				free(name);
+				if (rc == SEPOL_EEXIST) {
 					/* duplicate rule, ignore */
-					if (otype->otype == mapped_otype)
+					if (present_otype == mapped_otype)
 						continue;
 
 					ERR(state->handle, "Conflicting name-based type_transition %s %s:%s \"%s\":  %s vs %s",
@@ -1417,44 +1418,11 @@ static int expand_filename_trans(expand_state_t *state, filename_trans_rule_t *r
 					    state->out->p_type_val_to_name[j],
 					    state->out->p_class_val_to_name[cur_rule->tclass - 1],
 					    cur_rule->name,
-					    state->out->p_type_val_to_name[otype->otype - 1],
+					    state->out->p_type_val_to_name[present_otype - 1],
 					    state->out->p_type_val_to_name[mapped_otype - 1]);
 					return -1;
-				}
-
-				new_trans = calloc(1, sizeof(*new_trans));
-				if (!new_trans) {
-					ERR(state->handle, "Out of memory!");
-					return -1;
-				}
-
-				new_trans->name = strdup(cur_rule->name);
-				if (!new_trans->name) {
-					ERR(state->handle, "Out of memory!");
-					free(new_trans);
-					return -1;
-				}
-				new_trans->stype = i + 1;
-				new_trans->ttype = j + 1;
-				new_trans->tclass = cur_rule->tclass;
-
-				otype = calloc(1, sizeof(*otype));
-				if (!otype) {
-					ERR(state->handle, "Out of memory!");
-					free(new_trans->name);
-					free(new_trans);
-					return -1;
-				}
-				otype->otype = mapped_otype;
-
-				rc = hashtab_insert(state->out->filename_trans,
-						    (hashtab_key_t)new_trans,
-						    otype);
-				if (rc) {
+				} else if (rc < 0) {
 					ERR(state->handle, "Out of memory!");
-					free(otype);
-					free(new_trans->name);
-					free(new_trans);
 					return -1;
 				}
 			}
diff --git a/libsepol/src/kernel_to_cil.c b/libsepol/src/kernel_to_cil.c
index ede78a20..718d3481 100644
--- a/libsepol/src/kernel_to_cil.c
+++ b/libsepol/src/kernel_to_cil.c
@@ -1821,21 +1821,35 @@ struct map_filename_trans_args {
 
 static int map_filename_trans_to_str(hashtab_key_t key, void *data, void *arg)
 {
-	filename_trans_t *ft = (filename_trans_t *)key;
+	filename_trans_key_t *ft = (filename_trans_key_t *)key;
 	filename_trans_datum_t *datum = data;
 	struct map_filename_trans_args *map_args = arg;
 	struct policydb *pdb = map_args->pdb;
 	struct strs *strs = map_args->strs;
 	char *src, *tgt, *class, *filename, *new;
+	struct ebitmap_node *node;
+	uint32_t bit;
+	int rc;
 
-	src = pdb->p_type_val_to_name[ft->stype - 1];
 	tgt = pdb->p_type_val_to_name[ft->ttype - 1];
 	class = pdb->p_class_val_to_name[ft->tclass - 1];
 	filename = ft->name;
-	new =  pdb->p_type_val_to_name[datum->otype - 1];
+	do {
+		new = pdb->p_type_val_to_name[datum->otype - 1];
+
+		ebitmap_for_each_positive_bit(&datum->stypes, node, bit) {
+			src = pdb->p_type_val_to_name[bit];
+			rc = strs_create_and_add(strs,
+						 "(typetransition %s %s %s %s %s)",
+						 5, src, tgt, class, filename, new);
+			if (rc)
+				return rc;
+		}
+
+		datum = datum->next;
+	} while (datum);
 
-	return strs_create_and_add(strs, "(typetransition %s %s %s %s %s)", 5,
-				   src, tgt, class, filename, new);
+	return 0;
 }
 
 static int write_filename_trans_rules_to_cil(FILE *out, struct policydb *pdb)
diff --git a/libsepol/src/kernel_to_conf.c b/libsepol/src/kernel_to_conf.c
index 9de64832..9fb3ed22 100644
--- a/libsepol/src/kernel_to_conf.c
+++ b/libsepol/src/kernel_to_conf.c
@@ -1801,21 +1801,35 @@ struct map_filename_trans_args {
 
 static int map_filename_trans_to_str(hashtab_key_t key, void *data, void *arg)
 {
-	filename_trans_t *ft = (filename_trans_t *)key;
+	filename_trans_key_t *ft = (filename_trans_key_t *)key;
 	filename_trans_datum_t *datum = data;
 	struct map_filename_trans_args *map_args = arg;
 	struct policydb *pdb = map_args->pdb;
 	struct strs *strs = map_args->strs;
 	char *src, *tgt, *class, *filename, *new;
+	struct ebitmap_node *node;
+	uint32_t bit;
+	int rc;
 
-	src = pdb->p_type_val_to_name[ft->stype - 1];
 	tgt = pdb->p_type_val_to_name[ft->ttype - 1];
 	class = pdb->p_class_val_to_name[ft->tclass - 1];
 	filename = ft->name;
-	new =  pdb->p_type_val_to_name[datum->otype - 1];
+	do {
+		new = pdb->p_type_val_to_name[datum->otype - 1];
+
+		ebitmap_for_each_positive_bit(&datum->stypes, node, bit) {
+			src = pdb->p_type_val_to_name[bit];
+			rc = strs_create_and_add(strs,
+						 "type_transition %s %s:%s %s \"%s\";",
+						 5, src, tgt, class, new, filename);
+			if (rc)
+				return rc;
+		}
+
+		datum = datum->next;
+	} while (datum);
 
-	return strs_create_and_add(strs, "type_transition %s %s:%s %s \"%s\";", 5,
-				   src, tgt, class, new, filename);
+	return 0;
 }
 
 static int write_filename_trans_rules_to_conf(FILE *out, struct policydb *pdb)
diff --git a/libsepol/src/policydb.c b/libsepol/src/policydb.c
index 5b289a52..6b121d66 100644
--- a/libsepol/src/policydb.c
+++ b/libsepol/src/policydb.c
@@ -789,12 +789,12 @@ partial_name_hash(unsigned long c, unsigned long prevhash)
 
 static unsigned int filenametr_hash(hashtab_t h, const_hashtab_key_t k)
 {
-	const struct filename_trans *ft = (const struct filename_trans *)k;
+	const filename_trans_key_t *ft = (const filename_trans_key_t *)k;
 	unsigned long hash;
 	unsigned int byte_num;
 	unsigned char focus;
 
-	hash = ft->stype ^ ft->ttype ^ ft->tclass;
+	hash = ft->ttype ^ ft->tclass;
 
 	byte_num = 0;
 	while ((focus = ft->name[byte_num++]))
@@ -805,14 +805,10 @@ static unsigned int filenametr_hash(hashtab_t h, const_hashtab_key_t k)
 static int filenametr_cmp(hashtab_t h __attribute__ ((unused)),
 			  const_hashtab_key_t k1, const_hashtab_key_t k2)
 {
-	const struct filename_trans *ft1 = (const struct filename_trans *)k1;
-	const struct filename_trans *ft2 = (const struct filename_trans *)k2;
+	const filename_trans_key_t *ft1 = (const filename_trans_key_t *)k1;
+	const filename_trans_key_t *ft2 = (const filename_trans_key_t *)k2;
 	int v;
 
-	v = ft1->stype - ft2->stype;
-	if (v)
-		return v;
-
 	v = ft1->ttype - ft2->ttype;
 	if (v)
 		return v;
@@ -1409,9 +1405,12 @@ common_destroy, class_destroy, role_destroy, type_destroy, user_destroy,
 static int filenametr_destroy(hashtab_key_t key, hashtab_datum_t datum,
 			      void *p __attribute__ ((unused)))
 {
-	struct filename_trans *ft = (struct filename_trans *)key;
+	filename_trans_key_t *ft = (filename_trans_key_t *)key;
+	filename_trans_datum_t *fd = datum;
+
 	free(ft->name);
 	free(key);
+	ebitmap_destroy(&fd->stypes);
 	free(datum);
 	return 0;
 }
@@ -2595,12 +2594,77 @@ int role_allow_read(role_allow_t ** r, struct policy_file *fp)
 	return 0;
 }
 
+int policydb_filetrans_insert(policydb_t *p, uint32_t stype, uint32_t ttype,
+			      uint32_t tclass, const char *name,
+			      char **name_alloc, uint32_t otype,
+			      uint32_t *present_otype)
+{
+	filename_trans_key_t *ft, key;
+	filename_trans_datum_t *datum, *last;
+
+	key.ttype = ttype;
+	key.tclass = tclass;
+	key.name = (char *)name;
+
+	last = NULL;
+	datum = hashtab_search(p->filename_trans, (hashtab_key_t)&key);
+	while (datum) {
+		if (ebitmap_get_bit(&datum->stypes, stype - 1)) {
+			if (present_otype)
+				*present_otype = datum->otype;
+			return SEPOL_EEXIST;
+		}
+		if (datum->otype == otype)
+			break;
+		last = datum;
+		datum = datum->next;
+	}
+	if (!datum) {
+		if (!*name_alloc) {
+			*name_alloc = strdup(name);
+			if (!*name_alloc)
+				return SEPOL_ENOMEM;
+		}
+
+		datum = malloc(sizeof(*datum));
+		if (!datum)
+			return SEPOL_ENOMEM;
+
+		ebitmap_init(&datum->stypes);
+		datum->otype = otype;
+		datum->next = NULL;
+
+		if (last) {
+			last->next = datum;
+		} else {
+			ft = malloc(sizeof(*ft));
+			if (!ft) {
+				free(datum);
+				return SEPOL_ENOMEM;
+			}
+
+			ft->ttype = ttype;
+			ft->tclass = tclass;
+			ft->name = *name_alloc;
+
+			if (hashtab_insert(p->filename_trans, (hashtab_key_t)ft,
+					   (hashtab_datum_t)datum)) {
+				free(datum);
+				free(ft);
+				return SEPOL_ENOMEM;
+			}
+			*name_alloc = NULL;
+		}
+	}
+
+	p->filename_trans_count++;
+	return ebitmap_set_bit(&datum->stypes, stype - 1, 1);
+}
+
 int filename_trans_read(policydb_t *p, struct policy_file *fp)
 {
 	unsigned int i;
-	uint32_t buf[4], nel, len;
-	filename_trans_t *ft;
-	filename_trans_datum_t *otype;
+	uint32_t buf[4], nel, len, stype, ttype, tclass, otype;
 	int rc;
 	char *name;
 
@@ -2610,16 +2674,8 @@ int filename_trans_read(policydb_t *p, struct policy_file *fp)
 	nel = le32_to_cpu(buf[0]);
 
 	for (i = 0; i < nel; i++) {
-		ft = NULL;
-		otype = NULL;
 		name = NULL;
 
-		ft = calloc(1, sizeof(*ft));
-		if (!ft)
-			goto err;
-		otype = calloc(1, sizeof(*otype));
-		if (!otype)
-			goto err;
 		rc = next_entry(buf, fp, sizeof(uint32_t));
 		if (rc < 0)
 			goto err;
@@ -2631,8 +2687,6 @@ int filename_trans_read(policydb_t *p, struct policy_file *fp)
 		if (!name)
 			goto err;
 
-		ft->name = name;
-
 		rc = next_entry(name, fp, len);
 		if (rc < 0)
 			goto err;
@@ -2641,13 +2695,13 @@ int filename_trans_read(policydb_t *p, struct policy_file *fp)
 		if (rc < 0)
 			goto err;
 
-		ft->stype = le32_to_cpu(buf[0]);
-		ft->ttype = le32_to_cpu(buf[1]);
-		ft->tclass = le32_to_cpu(buf[2]);
-		otype->otype = le32_to_cpu(buf[3]);
+		stype  = le32_to_cpu(buf[0]);
+		ttype  = le32_to_cpu(buf[1]);
+		tclass = le32_to_cpu(buf[2]);
+		otype  = le32_to_cpu(buf[3]);
 
-		rc = hashtab_insert(p->filename_trans, (hashtab_key_t) ft,
-				    otype);
+		rc = policydb_filetrans_insert(p, stype, ttype, tclass, name,
+					       &name, otype, NULL);
 		if (rc) {
 			if (rc != SEPOL_EEXIST)
 				goto err;
@@ -2659,21 +2713,17 @@ int filename_trans_read(policydb_t *p, struct policy_file *fp)
 			 */
 			WARN(fp->handle,
 			     "Duplicate name-based type_transition %s %s:%s \"%s\":  %s, ignoring",
-			     p->p_type_val_to_name[ft->stype - 1],
-			     p->p_type_val_to_name[ft->ttype - 1],
-			     p->p_class_val_to_name[ft->tclass - 1],
-			     ft->name,
-			     p->p_type_val_to_name[otype->otype - 1]);
-			free(ft);
-			free(name);
-			free(otype);
+			     p->p_type_val_to_name[stype - 1],
+			     p->p_type_val_to_name[ttype - 1],
+			     p->p_class_val_to_name[tclass - 1],
+			     name,
+			     p->p_type_val_to_name[otype - 1]);
 			/* continue, ignoring this one */
 		}
+		free(name);
 	}
 	return 0;
 err:
-	free(ft);
-	free(otype);
 	free(name);
 	return -1;
 }
diff --git a/libsepol/src/write.c b/libsepol/src/write.c
index 1fd6a16a..d3aee8d5 100644
--- a/libsepol/src/write.c
+++ b/libsepol/src/write.c
@@ -571,44 +571,50 @@ static int role_allow_write(role_allow_t * r, struct policy_file *fp)
 
 static int filename_write_helper(hashtab_key_t key, void *data, void *ptr)
 {
-	uint32_t buf[4];
+	uint32_t bit, buf[4];
 	size_t items, len;
-	struct filename_trans *ft = (struct filename_trans *)key;
-	struct filename_trans_datum *otype = data;
+	filename_trans_key_t *ft = (filename_trans_key_t *)key;
+	filename_trans_datum_t *datum = data;
+	ebitmap_node_t *node;
 	void *fp = ptr;
 
 	len = strlen(ft->name);
-	buf[0] = cpu_to_le32(len);
-	items = put_entry(buf, sizeof(uint32_t), 1, fp);
-	if (items != 1)
-		return POLICYDB_ERROR;
+	do {
+		ebitmap_for_each_positive_bit(&datum->stypes, node, bit) {
+			buf[0] = cpu_to_le32(len);
+			items = put_entry(buf, sizeof(uint32_t), 1, fp);
+			if (items != 1)
+				return POLICYDB_ERROR;
 
-	items = put_entry(ft->name, sizeof(char), len, fp);
-	if (items != len)
-		return POLICYDB_ERROR;
+			items = put_entry(ft->name, sizeof(char), len, fp);
+			if (items != len)
+				return POLICYDB_ERROR;
 
-	buf[0] = cpu_to_le32(ft->stype);
-	buf[1] = cpu_to_le32(ft->ttype);
-	buf[2] = cpu_to_le32(ft->tclass);
-	buf[3] = cpu_to_le32(otype->otype);
-	items = put_entry(buf, sizeof(uint32_t), 4, fp);
-	if (items != 4)
-		return POLICYDB_ERROR;
+			buf[0] = cpu_to_le32(bit + 1);
+			buf[1] = cpu_to_le32(ft->ttype);
+			buf[2] = cpu_to_le32(ft->tclass);
+			buf[3] = cpu_to_le32(datum->otype);
+			items = put_entry(buf, sizeof(uint32_t), 4, fp);
+			if (items != 4)
+				return POLICYDB_ERROR;
+		}
+
+		datum = datum->next;
+	} while (datum);
 
 	return 0;
 }
 
 static int filename_trans_write(struct policydb *p, void *fp)
 {
-	size_t nel, items;
+	size_t items;
 	uint32_t buf[1];
 	int rc;
 
 	if (p->policyvers < POLICYDB_VERSION_FILENAME_TRANS)
 		return 0;
 
-	nel =  p->filename_trans->nel;
-	buf[0] = cpu_to_le32(nel);
+	buf[0] = cpu_to_le32(p->filename_trans_count);
 	items = put_entry(buf, sizeof(uint32_t), 1, fp);
 	if (items != 1)
 		return POLICYDB_ERROR;
-- 
2.25.1

[PATCH 2/2] libsepol: implement POLICYDB_VERSION_COMP_FTRANS

[Ondrej Mosnacek]

Implement a new, more space-efficient form of storing filename
transitions in the binary policy. The internal structures have already
been converted to this new representation; this patch just implements
reading/writing an equivalent representation from/to the binary policy.

This new format reduces the size of Fedora policy from 7.6 MB to only
3.3 MB (with policy optimization enabled in both cases). With the
unconfined module disabled, the size is reduced from 3.3 MB to 2.4 MB.

Signed-off-by: Ondrej Mosnacek <omosnace@redhat.com>
---
 libsepol/include/sepol/policydb/policydb.h |   3 +-
 libsepol/src/policydb.c                    | 217 +++++++++++++++++----
 libsepol/src/write.c                       |  72 ++++++-
 3 files changed, 241 insertions(+), 51 deletions(-)

diff --git a/libsepol/include/sepol/policydb/policydb.h b/libsepol/include/sepol/policydb/policydb.h
index c3180c61..9ef43abc 100644
--- a/libsepol/include/sepol/policydb/policydb.h
+++ b/libsepol/include/sepol/policydb/policydb.h
@@ -755,10 +755,11 @@ extern int policydb_set_target_platform(policydb_t *p, int platform);
 #define POLICYDB_VERSION_XPERMS_IOCTL	30 /* Linux-specific */
 #define POLICYDB_VERSION_INFINIBAND		31 /* Linux-specific */
 #define POLICYDB_VERSION_GLBLUB		32
+#define POLICYDB_VERSION_COMP_FTRANS	33 /* compressed filename transitions */
 
 /* Range of policy versions we understand*/
 #define POLICYDB_VERSION_MIN	POLICYDB_VERSION_BASE
-#define POLICYDB_VERSION_MAX	POLICYDB_VERSION_GLBLUB
+#define POLICYDB_VERSION_MAX	POLICYDB_VERSION_COMP_FTRANS
 
 /* Module versions and specific changes*/
 #define MOD_POLICYDB_VERSION_BASE		4
diff --git a/libsepol/src/policydb.c b/libsepol/src/policydb.c
index 6b121d66..f108e4af 100644
--- a/libsepol/src/policydb.c
+++ b/libsepol/src/policydb.c
@@ -200,6 +200,13 @@ static struct policydb_compat_info policydb_compat[] = {
 	 .ocon_num = OCON_IBENDPORT + 1,
 	 .target_platform = SEPOL_TARGET_SELINUX,
 	},
+	{
+	 .type = POLICY_KERN,
+	 .version = POLICYDB_VERSION_COMP_FTRANS,
+	 .sym_num = SYM_NUM,
+	 .ocon_num = OCON_IBENDPORT + 1,
+	 .target_platform = SEPOL_TARGET_SELINUX,
+	},
 	{
 	 .type = POLICY_BASE,
 	 .version = MOD_POLICYDB_VERSION_BASE,
@@ -2661,73 +2668,201 @@ int policydb_filetrans_insert(policydb_t *p, uint32_t stype, uint32_t ttype,
 	return ebitmap_set_bit(&datum->stypes, stype - 1, 1);
 }
 
-int filename_trans_read(policydb_t *p, struct policy_file *fp)
+static int filename_trans_read_one_old(policydb_t *p, struct policy_file *fp)
 {
-	unsigned int i;
-	uint32_t buf[4], nel, len, stype, ttype, tclass, otype;
+	uint32_t buf[4], len, stype, ttype, tclass, otype;
+	char *name = NULL;
 	int rc;
-	char *name;
 
 	rc = next_entry(buf, fp, sizeof(uint32_t));
 	if (rc < 0)
 		return -1;
-	nel = le32_to_cpu(buf[0]);
+	len = le32_to_cpu(buf[0]);
+	if (zero_or_saturated(len))
+		return -1;
 
-	for (i = 0; i < nel; i++) {
-		name = NULL;
+	name = calloc(len + 1, sizeof(*name));
+	if (!name)
+		return -1;
 
-		rc = next_entry(buf, fp, sizeof(uint32_t));
-		if (rc < 0)
-			goto err;
-		len = le32_to_cpu(buf[0]);
-		if (zero_or_saturated(len))
+	rc = next_entry(name, fp, len);
+	if (rc < 0)
+		goto err;
+
+	rc = next_entry(buf, fp, sizeof(uint32_t) * 4);
+	if (rc < 0)
+		goto err;
+
+	stype  = le32_to_cpu(buf[0]);
+	ttype  = le32_to_cpu(buf[1]);
+	tclass = le32_to_cpu(buf[2]);
+	otype  = le32_to_cpu(buf[3]);
+
+	rc = policydb_filetrans_insert(p, stype, ttype, tclass, name, &name,
+				       otype, NULL);
+	if (rc) {
+		if (rc != SEPOL_EEXIST)
 			goto err;
+		/*
+		 * Some old policies were wrongly generated with
+		 * duplicate filename transition rules.  For backward
+		 * compatibility, do not reject such policies, just
+		 * issue a warning and ignore the duplicate.
+		 */
+		WARN(fp->handle,
+		     "Duplicate name-based type_transition %s %s:%s \"%s\":  %s, ignoring",
+		     p->p_type_val_to_name[stype - 1],
+		     p->p_type_val_to_name[ttype - 1],
+		     p->p_class_val_to_name[tclass - 1],
+		     name,
+		     p->p_type_val_to_name[otype - 1]);
+		/* continue, ignoring this one */
+	}
+	free(name);
+	return 0;
+err:
+	free(name);
+	return -1;
+}
+
+static int filename_trans_check_datum(filename_trans_datum_t *datum)
+{
+	ebitmap_t stypes, otypes;
+
+	ebitmap_init(&stypes);
+	ebitmap_init(&otypes);
+
+	while (datum) {
+		if (ebitmap_get_bit(&otypes, datum->otype))
+			return -1;
+
+		if (ebitmap_set_bit(&otypes, datum->otype, 1))
+			return -1;
 
-		name = calloc(len + 1, sizeof(*name));
-		if (!name)
+		if (ebitmap_match_any(&stypes, &datum->stypes))
+			return -1;
+
+		if (ebitmap_union(&stypes, &datum->stypes))
+			return -1;
+	}
+	return 0;
+}
+
+static int filename_trans_read_one_new(policydb_t *p, struct policy_file *fp)
+{
+	filename_trans_key_t *ft = NULL;
+	filename_trans_datum_t **dst, *datum, *first = NULL;
+	unsigned int i;
+	uint32_t buf[3], len, ttype, tclass, ndatum;
+	char *name = NULL;
+	int rc;
+
+	rc = next_entry(buf, fp, sizeof(uint32_t));
+	if (rc < 0)
+		return -1;
+	len = le32_to_cpu(buf[0]);
+	if (zero_or_saturated(len))
+		return -1;
+
+	name = calloc(len + 1, sizeof(*name));
+	if (!name)
+		return -1;
+
+	rc = next_entry(name, fp, len);
+	if (rc < 0)
+		goto err;
+
+	rc = next_entry(buf, fp, sizeof(uint32_t) * 3);
+	if (rc < 0)
+		goto err;
+
+	ttype = le32_to_cpu(buf[0]);
+	tclass = le32_to_cpu(buf[1]);
+	ndatum = le32_to_cpu(buf[2]);
+	if (ndatum == 0)
+		goto err;
+
+	dst = &first;
+	for (i = 0; i < ndatum; i++) {
+		datum = malloc(sizeof(*datum));
+		if (!datum)
 			goto err;
 
-		rc = next_entry(name, fp, len);
+		*dst = datum;
+
+		/* ebitmap_read() will at least init the bitmap */
+		rc = ebitmap_read(&datum->stypes, fp);
 		if (rc < 0)
 			goto err;
 
-		rc = next_entry(buf, fp, sizeof(uint32_t) * 4);
+		rc = next_entry(buf, fp, sizeof(uint32_t));
 		if (rc < 0)
 			goto err;
 
-		stype  = le32_to_cpu(buf[0]);
-		ttype  = le32_to_cpu(buf[1]);
-		tclass = le32_to_cpu(buf[2]);
-		otype  = le32_to_cpu(buf[3]);
+		datum->otype = le32_to_cpu(buf[0]);
 
-		rc = policydb_filetrans_insert(p, stype, ttype, tclass, name,
-					       &name, otype, NULL);
-		if (rc) {
-			if (rc != SEPOL_EEXIST)
-				goto err;
-			/*
-			 * Some old policies were wrongly generated with
-			 * duplicate filename transition rules.  For backward
-			 * compatibility, do not reject such policies, just
-			 * issue a warning and ignore the duplicate.
-			 */
-			WARN(fp->handle,
-			     "Duplicate name-based type_transition %s %s:%s \"%s\":  %s, ignoring",
-			     p->p_type_val_to_name[stype - 1],
-			     p->p_type_val_to_name[ttype - 1],
-			     p->p_class_val_to_name[tclass - 1],
-			     name,
-			     p->p_type_val_to_name[otype - 1]);
-			/* continue, ignoring this one */
-		}
-		free(name);
+		dst = &datum->next;
 	}
+	*dst = NULL;
+
+	if (ndatum > 1 && filename_trans_check_datum(first))
+		goto err;
+
+	ft = malloc(sizeof(*ft));
+	if (!ft)
+		goto err;
+
+	ft->ttype = ttype;
+	ft->tclass = tclass;
+	ft->name = name;
+
+	rc = hashtab_insert(p->filename_trans, (hashtab_key_t)ft,
+			    (hashtab_datum_t)first);
+	if (rc)
+		goto err;
+
+	p->filename_trans_count++;
 	return 0;
 err:
+	free(ft);
 	free(name);
+	while (first) {
+		datum = first;
+		first = first->next;
+
+		ebitmap_destroy(&datum->stypes);
+		free(datum);
+	}
 	return -1;
 }
 
+int filename_trans_read(policydb_t *p, struct policy_file *fp)
+{
+	unsigned int i;
+	uint32_t buf[1], nel;
+	int rc;
+
+	rc = next_entry(buf, fp, sizeof(uint32_t));
+	if (rc < 0)
+		return -1;
+	nel = le32_to_cpu(buf[0]);
+
+	if (p->policyvers < POLICYDB_VERSION_COMP_FTRANS) {
+		for (i = 0; i < nel; i++) {
+			rc = filename_trans_read_one_old(p, fp);
+			if (rc < 0)
+				return -1;
+		}
+	} else {
+		for (i = 0; i < nel; i++) {
+			rc = filename_trans_read_one_new(p, fp);
+			if (rc < 0)
+				return -1;
+		}
+	}
+	return 0;
+}
+
 static int ocontext_read_xen(struct policydb_compat_info *info,
 	policydb_t *p, struct policy_file *fp)
 {
diff --git a/libsepol/src/write.c b/libsepol/src/write.c
index d3aee8d5..9c74d9f5 100644
--- a/libsepol/src/write.c
+++ b/libsepol/src/write.c
@@ -569,7 +569,7 @@ static int role_allow_write(role_allow_t * r, struct policy_file *fp)
 	return POLICYDB_SUCCESS;
 }
 
-static int filename_write_helper(hashtab_key_t key, void *data, void *ptr)
+static int filename_write_one_old(hashtab_key_t key, void *data, void *ptr)
 {
 	uint32_t bit, buf[4];
 	size_t items, len;
@@ -605,6 +605,54 @@ static int filename_write_helper(hashtab_key_t key, void *data, void *ptr)
 	return 0;
 }
 
+static int filename_write_one_new(hashtab_key_t key, void *data, void *ptr)
+{
+	uint32_t buf[3];
+	size_t items, len, ndatum;
+	filename_trans_key_t *ft = (filename_trans_key_t *)key;
+	filename_trans_datum_t *datum;
+	void *fp = ptr;
+
+	len = strlen(ft->name);
+	buf[0] = cpu_to_le32(len);
+	items = put_entry(buf, sizeof(uint32_t), 1, fp);
+	if (items != 1)
+		return POLICYDB_ERROR;
+
+	items = put_entry(ft->name, sizeof(char), len, fp);
+	if (items != len)
+		return POLICYDB_ERROR;
+
+	ndatum = 0;
+	datum = data;
+	do {
+		ndatum++;
+		datum = datum->next;
+	} while (datum);
+
+	buf[0] = cpu_to_le32(ft->ttype);
+	buf[1] = cpu_to_le32(ft->tclass);
+	buf[2] = cpu_to_le32(ndatum);
+	items = put_entry(buf, sizeof(uint32_t), 3, fp);
+	if (items != 3)
+		return POLICYDB_ERROR;
+
+	datum = data;
+	do {
+		if (ebitmap_write(&datum->stypes, fp))
+			return POLICYDB_ERROR;
+
+		buf[0] = cpu_to_le32(datum->otype);
+		items = put_entry(buf, sizeof(uint32_t), 1, fp);
+		if (items != 1)
+			return POLICYDB_ERROR;
+
+		datum = datum->next;
+	} while (datum);
+
+	return 0;
+}
+
 static int filename_trans_write(struct policydb *p, void *fp)
 {
 	size_t items;
@@ -614,16 +662,22 @@ static int filename_trans_write(struct policydb *p, void *fp)
 	if (p->policyvers < POLICYDB_VERSION_FILENAME_TRANS)
 		return 0;
 
-	buf[0] = cpu_to_le32(p->filename_trans_count);
-	items = put_entry(buf, sizeof(uint32_t), 1, fp);
-	if (items != 1)
-		return POLICYDB_ERROR;
+	if (p->policyvers < POLICYDB_VERSION_COMP_FTRANS) {
+		buf[0] = cpu_to_le32(p->filename_trans_count);
+		items = put_entry(buf, sizeof(uint32_t), 1, fp);
+		if (items != 1)
+			return POLICYDB_ERROR;
 
-	rc = hashtab_map(p->filename_trans, filename_write_helper, fp);
-	if (rc)
-		return rc;
+		rc = hashtab_map(p->filename_trans, filename_write_one_old, fp);
+	} else {
+		buf[0] = cpu_to_le32(p->filename_trans->nel);
+		items = put_entry(buf, sizeof(uint32_t), 1, fp);
+		if (items != 1)
+			return POLICYDB_ERROR;
 
-	return 0;
+		rc = hashtab_map(p->filename_trans, filename_write_one_new, fp);
+	}
+	return rc;
 }
 
 static int role_set_write(role_set_t * x, struct policy_file *fp)
-- 
2.25.1

Re: [PATCH 2/2] libsepol: implement POLICYDB_VERSION_COMP_FTRANS

[Stephen Smalley]

On 3/27/20 11:21 AM, Ondrej Mosnacek wrote:
> Implement a new, more space-efficient form of storing filename
> transitions in the binary policy. The internal structures have already
> been converted to this new representation; this patch just implements
> reading/writing an equivalent representation from/to the binary policy.
> 
> This new format reduces the size of Fedora policy from 7.6 MB to only
> 3.3 MB (with policy optimization enabled in both cases). With the
> unconfined module disabled, the size is reduced from 3.3 MB to 2.4 MB.
> 
> Signed-off-by: Ondrej Mosnacek <omosnace@redhat.com>
> ---

Haven't looked at the code yet, but something is wrong with the handling 
when it needs to dowgrade to an older policy version for a kernel that 
doesn't yet support this new version:

$ sudo semodule -B
libsepol.mls_read_range_helper: range overflow
libsepol.context_read_and_validate: error reading MLS range of context
libsepol.policydb_to_image: new policy image is invalid
libsepol.policydb_to_image: could not create policy image
SELinux:  Could not downgrade policy file 
/etc/selinux/targeted/policy/policy.33, searching for an older version.

Re: [PATCH 2/2] libsepol: implement POLICYDB_VERSION_COMP_FTRANS

[Ondrej Mosnacek]

On Fri, Mar 27, 2020 at 6:08 PM Stephen Smalley <sds@tycho.nsa.gov> wrote:
> On 3/27/20 11:21 AM, Ondrej Mosnacek wrote:
> > Implement a new, more space-efficient form of storing filename
> > transitions in the binary policy. The internal structures have already
> > been converted to this new representation; this patch just implements
> > reading/writing an equivalent representation from/to the binary policy.
> >
> > This new format reduces the size of Fedora policy from 7.6 MB to only
> > 3.3 MB (with policy optimization enabled in both cases). With the
> > unconfined module disabled, the size is reduced from 3.3 MB to 2.4 MB.
> >
> > Signed-off-by: Ondrej Mosnacek <omosnace@redhat.com>
> > ---
>
> Haven't looked at the code yet, but something is wrong with the handling
> when it needs to dowgrade to an older policy version for a kernel that
> doesn't yet support this new version:
>
> $ sudo semodule -B
> libsepol.mls_read_range_helper: range overflow
> libsepol.context_read_and_validate: error reading MLS range of context
> libsepol.policydb_to_image: new policy image is invalid
> libsepol.policydb_to_image: could not create policy image
> SELinux:  Could not downgrade policy file
> /etc/selinux/targeted/policy/policy.33, searching for an older version.

Hm, haven't tried that... I reproduced it on my end and I believe I
have found the bug - filename_trans_read_one_new() is counting
p->filename_trans_count in a completely wrong way. It needs to add up
the cardinalities of all stype bitmaps, not just count the hashtab
entries...

I'll post a v2 tomorrow, in the meantime you can test with this patch on top:
https://github.com/WOnder93/selinux/commit/738263d5be83323da7b4008e37140ec7ef99db8d.patch

-- 
Ondrej Mosnacek <omosnace at redhat dot com>
Software Engineer, Security Technologies
Red Hat, Inc.

Re: [PATCH 0/2] userspace: Implement new format of filename trans rules

[Stephen Smalley]

On 3/27/20 11:21 AM, Ondrej Mosnacek wrote:
> These patches are the userspace side of the kernel change posted at [1].
> 
> The first patch changes libsepol's internal representation of filename
> transition rules in a way similar to kernel commit c3a276111ea2
> ("selinux: optimize storage of filename transitions") [2].
> 
> The second patch then builds upon that and implements reading and
> writing of a new binary policy format that uses this representation also
> in the data layout.
> 
> See individual patches for more details.
> 
> NOTE: This series unfortunately breaks the build of setools. Moreover,
> when an existing build of setools dynamically links against the new
> libsepol, it segfaults. Sadly, there doesn't seem to be a nice way of
> handling this, since setools relies on non-public libsepol policydb
> API/ABI.

I think this has happened before a few years ago when we made a 
different change to those structures, and required updates on the 
setools side.

Maybe we need to figure out what setools needs to be encapsulated and 
exported as part of the libsepol public ABI/API, and then stop having it 
peer into libsepol internals?

Re: [PATCH 0/2] userspace: Implement new format of filename trans rules

[Chris PeBenito]

On 3/27/20 3:21 PM, Stephen Smalley wrote:
> On 3/27/20 11:21 AM, Ondrej Mosnacek wrote:
>> These patches are the userspace side of the kernel change posted at [1].
>>
>> The first patch changes libsepol's internal representation of filename
>> transition rules in a way similar to kernel commit c3a276111ea2
>> ("selinux: optimize storage of filename transitions") [2].
>>
>> The second patch then builds upon that and implements reading and
>> writing of a new binary policy format that uses this representation also
>> in the data layout.
>>
>> See individual patches for more details.
>>
>> NOTE: This series unfortunately breaks the build of setools. Moreover,
>> when an existing build of setools dynamically links against the new
>> libsepol, it segfaults. Sadly, there doesn't seem to be a nice way of
>> handling this, since setools relies on non-public libsepol policydb
>> API/ABI.
> 
> I think this has happened before a few years ago when we made a 
> different change to those structures, and required updates on the 
> setools side.
> 
> Maybe we need to figure out what setools needs to be encapsulated and 
> exported as part of the libsepol public ABI/API, and then stop having it 
> peer into libsepol internals?

I'd be fine with that :)

Ultimately SETools does 2 thing for the policy access:
* iterate over the entire policy, reading data out of the various objects
* use linkages between objects in the policy, e.g. get the 
type/attributes from an AV rule, get types from an attribute, etc. using 
the stuff like class_val_to_struct as needed.

So if sufficient functionality to do dispol was exported, that would get 
close to all that was needed.  There are some optimizations I made by 
digging at the structs a bit more than the above, but that could 
potentially be dropped if libsepol has sufficient support.  See 
<https://github.com/SELinuxProject/setools/blob/master/setools/policyrep/selinuxpolicy.pxi#L673> 
for the policy loading code.


-- 
Chris PeBenito

Re: [PATCH 0/2] userspace: Implement new format of filename trans rules

[James Carter]

On Mon, Mar 30, 2020 at 9:06 AM Chris PeBenito <pebenito@ieee.org> wrote:
>
> On 3/27/20 3:21 PM, Stephen Smalley wrote:
> > On 3/27/20 11:21 AM, Ondrej Mosnacek wrote:
> >> These patches are the userspace side of the kernel change posted at [1].
> >>
> >> The first patch changes libsepol's internal representation of filename
> >> transition rules in a way similar to kernel commit c3a276111ea2
> >> ("selinux: optimize storage of filename transitions") [2].
> >>
> >> The second patch then builds upon that and implements reading and
> >> writing of a new binary policy format that uses this representation also
> >> in the data layout.
> >>
> >> See individual patches for more details.
> >>
> >> NOTE: This series unfortunately breaks the build of setools. Moreover,
> >> when an existing build of setools dynamically links against the new
> >> libsepol, it segfaults. Sadly, there doesn't seem to be a nice way of
> >> handling this, since setools relies on non-public libsepol policydb
> >> API/ABI.
> >
> > I think this has happened before a few years ago when we made a
> > different change to those structures, and required updates on the
> > setools side.
> >
> > Maybe we need to figure out what setools needs to be encapsulated and
> > exported as part of the libsepol public ABI/API, and then stop having it
> > peer into libsepol internals?
>
> I'd be fine with that :)
>

I am a little confused. I would consider peering into the libsepol
internals to be something like assuming the memory layout of
structures and pulling things out based on those assumptions, but
setools is just using public header files and functions. It seems to
me to be using the public API. Now I think that too much of the
internals are being exposed, but I am not sure we can do anything
about that now.

It doesn't seem like it would be too hard to update setools to work
with this change, unless there is a requirement that it still work
with older versions of libsepol. I am not very familiar with cython
and I don't know how to make it work differently depending on what
version of libsepol is on the system.

Speaking of versions, since we are actually modifying structs in the
header files instead of just adding new things, don't we need change
the version of libsepol or something?

> Ultimately SETools does 2 thing for the policy access:
> * iterate over the entire policy, reading data out of the various objects
> * use linkages between objects in the policy, e.g. get the
> type/attributes from an AV rule, get types from an attribute, etc. using
> the stuff like class_val_to_struct as needed.
>
> So if sufficient functionality to do dispol was exported, that would get
> close to all that was needed.  There are some optimizations I made by
> digging at the structs a bit more than the above, but that could
> potentially be dropped if libsepol has sufficient support.  See
> <https://github.com/SELinuxProject/setools/blob/master/setools/policyrep/selinuxpolicy.pxi#L673>
> for the policy loading code.
>

I think the fact that the CIL, kernel to CIL, kernel to conf, and
module to CIL code is all in libsepol speaks to the fact of how
tightly integrated they are to the rest of libsepol. One argument that
could be made is that the policyrep stuff in setools really belongs in
libsepol.

Thinking about how libsepol could be encapsulated leads me to a couple
of possibilities. One way would be functions that could return lists
of rules. The policy module code gives us avrule_t, role_trans_rule_t,
role_allow_t, filename_trans_rule_t, range_trans_rule_t, and others.
Those structures are probably unlikely to change and, at least in this
case, creating a function that walks the filename_trans hashtable and
returns a list of filename_trans_rule_t certainly seems like it
wouldn't be too hard. Another possible way to encapsulate would be
create a way to walk the various hashtables element by element (I
think hashtab_map() requires too much knowledge of the internal
structures), returning an opaque structure to track where you are in
the hashtable and functions that allow you to get each part of the
rule being stored. There are other ways that it could be done, but I
could rewrite kernel to and module to stuff with either of those. CIL
itself would require some functions to insert rules into the policydb
which probably wouldn't be too hard. None of this would be too hard,
but it would take some time. The real question is would it really be
valuable?

Jim

Re: [PATCH 0/2] userspace: Implement new format of filename trans rules

[Stephen Smalley]

On Wed, Apr 29, 2020 at 3:01 PM James Carter <jwcart2@gmail.com> wrote:
>
> On Mon, Mar 30, 2020 at 9:06 AM Chris PeBenito <pebenito@ieee.org> wrote:
> >
> > On 3/27/20 3:21 PM, Stephen Smalley wrote:
> > > On 3/27/20 11:21 AM, Ondrej Mosnacek wrote:
> > >> These patches are the userspace side of the kernel change posted at [1].
> > >>
> > >> The first patch changes libsepol's internal representation of filename
> > >> transition rules in a way similar to kernel commit c3a276111ea2
> > >> ("selinux: optimize storage of filename transitions") [2].
> > >>
> > >> The second patch then builds upon that and implements reading and
> > >> writing of a new binary policy format that uses this representation also
> > >> in the data layout.
> > >>
> > >> See individual patches for more details.
> > >>
> > >> NOTE: This series unfortunately breaks the build of setools. Moreover,
> > >> when an existing build of setools dynamically links against the new
> > >> libsepol, it segfaults. Sadly, there doesn't seem to be a nice way of
> > >> handling this, since setools relies on non-public libsepol policydb
> > >> API/ABI.
> > >
> > > I think this has happened before a few years ago when we made a
> > > different change to those structures, and required updates on the
> > > setools side.
> > >
> > > Maybe we need to figure out what setools needs to be encapsulated and
> > > exported as part of the libsepol public ABI/API, and then stop having it
> > > peer into libsepol internals?
> >
> > I'd be fine with that :)
> >
>
> I am a little confused. I would consider peering into the libsepol
> internals to be something like assuming the memory layout of
> structures and pulling things out based on those assumptions, but
> setools is just using public header files and functions. It seems to
> me to be using the public API. Now I think that too much of the
> internals are being exposed, but I am not sure we can do anything
> about that now.

The sepol/policydb/*.h files are considered private and are only for
static users of libsepol
like checkpolicy.  Only the top-level sepol/*.h headers are part of
the shared library API/ABI.

> It doesn't seem like it would be too hard to update setools to work
> with this change, unless there is a requirement that it still work
> with older versions of libsepol. I am not very familiar with cython
> and I don't know how to make it work differently depending on what
> version of libsepol is on the system.
>
> Speaking of versions, since we are actually modifying structs in the
> header files instead of just adding new things, don't we need change
> the version of libsepol or something?
>
> > Ultimately SETools does 2 thing for the policy access:
> > * iterate over the entire policy, reading data out of the various objects
> > * use linkages between objects in the policy, e.g. get the
> > type/attributes from an AV rule, get types from an attribute, etc. using
> > the stuff like class_val_to_struct as needed.
> >
> > So if sufficient functionality to do dispol was exported, that would get
> > close to all that was needed.  There are some optimizations I made by
> > digging at the structs a bit more than the above, but that could
> > potentially be dropped if libsepol has sufficient support.  See
> > <https://github.com/SELinuxProject/setools/blob/master/setools/policyrep/selinuxpolicy.pxi#L673>
> > for the policy loading code.
> >
>
> I think the fact that the CIL, kernel to CIL, kernel to conf, and
> module to CIL code is all in libsepol speaks to the fact of how
> tightly integrated they are to the rest of libsepol. One argument that
> could be made is that the policyrep stuff in setools really belongs in
> libsepol.
>
> Thinking about how libsepol could be encapsulated leads me to a couple
> of possibilities. One way would be functions that could return lists
> of rules. The policy module code gives us avrule_t, role_trans_rule_t,
> role_allow_t, filename_trans_rule_t, range_trans_rule_t, and others.
> Those structures are probably unlikely to change and, at least in this
> case, creating a function that walks the filename_trans hashtable and
> returns a list of filename_trans_rule_t certainly seems like it
> wouldn't be too hard. Another possible way to encapsulate would be
> create a way to walk the various hashtables element by element (I
> think hashtab_map() requires too much knowledge of the internal
> structures), returning an opaque structure to track where you are in
> the hashtable and functions that allow you to get each part of the
> rule being stored. There are other ways that it could be done, but I
> could rewrite kernel to and module to stuff with either of those. CIL
> itself would require some functions to insert rules into the policydb
> which probably wouldn't be too hard. None of this would be too hard,
> but it would take some time. The real question is would it really be
> valuable?

Re: [PATCH 0/2] userspace: Implement new format of filename trans rules

[Stephen Smalley]

On Wed, Apr 29, 2020 at 3:01 PM James Carter <jwcart2@gmail.com> wrote:
> I think the fact that the CIL, kernel to CIL, kernel to conf, and
> module to CIL code is all in libsepol speaks to the fact of how
> tightly integrated they are to the rest of libsepol. One argument that
> could be made is that the policyrep stuff in setools really belongs in
> libsepol.
>
> Thinking about how libsepol could be encapsulated leads me to a couple
> of possibilities. One way would be functions that could return lists
> of rules. The policy module code gives us avrule_t, role_trans_rule_t,
> role_allow_t, filename_trans_rule_t, range_trans_rule_t, and others.
> Those structures are probably unlikely to change and, at least in this
> case, creating a function that walks the filename_trans hashtable and
> returns a list of filename_trans_rule_t certainly seems like it
> wouldn't be too hard. Another possible way to encapsulate would be
> create a way to walk the various hashtables element by element (I
> think hashtab_map() requires too much knowledge of the internal
> structures), returning an opaque structure to track where you are in
> the hashtable and functions that allow you to get each part of the
> rule being stored. There are other ways that it could be done, but I
> could rewrite kernel to and module to stuff with either of those. CIL
> itself would require some functions to insert rules into the policydb
> which probably wouldn't be too hard. None of this would be too hard,
> but it would take some time. The real question is would it really be
> valuable?

I don't think we want to directly expose the existing data structures
from include/sepol/policydb/*.h (or at least not without a careful
audit) since those are often tightly coupled to policy compiler
internals and/or the kernel or module policy formats. Creating an
abstraction for each with a proper API in new definitions in
include/sepol/*.h would be preferable albeit more work. There was a
proposal a long time ago from the setools developers to create an
iterator interface and accessor functions for each data type, see
https://lore.kernel.org/selinux/200603212246.k2LMkRNq028071@gotham.columbia.tresys.com/.

Re: [PATCH 0/2] userspace: Implement new format of filename trans rules

[Ondrej Mosnacek]

On Thu, Apr 30, 2020 at 3:23 PM Stephen Smalley
<stephen.smalley.work@gmail.com> wrote:
> On Wed, Apr 29, 2020 at 3:01 PM James Carter <jwcart2@gmail.com> wrote:
> > I think the fact that the CIL, kernel to CIL, kernel to conf, and
> > module to CIL code is all in libsepol speaks to the fact of how
> > tightly integrated they are to the rest of libsepol. One argument that
> > could be made is that the policyrep stuff in setools really belongs in
> > libsepol.
> >
> > Thinking about how libsepol could be encapsulated leads me to a couple
> > of possibilities. One way would be functions that could return lists
> > of rules. The policy module code gives us avrule_t, role_trans_rule_t,
> > role_allow_t, filename_trans_rule_t, range_trans_rule_t, and others.
> > Those structures are probably unlikely to change and, at least in this
> > case, creating a function that walks the filename_trans hashtable and
> > returns a list of filename_trans_rule_t certainly seems like it
> > wouldn't be too hard. Another possible way to encapsulate would be
> > create a way to walk the various hashtables element by element (I
> > think hashtab_map() requires too much knowledge of the internal
> > structures), returning an opaque structure to track where you are in
> > the hashtable and functions that allow you to get each part of the
> > rule being stored. There are other ways that it could be done, but I
> > could rewrite kernel to and module to stuff with either of those. CIL
> > itself would require some functions to insert rules into the policydb
> > which probably wouldn't be too hard. None of this would be too hard,
> > but it would take some time. The real question is would it really be
> > valuable?
>
> I don't think we want to directly expose the existing data structures
> from include/sepol/policydb/*.h (or at least not without a careful
> audit) since those are often tightly coupled to policy compiler
> internals and/or the kernel or module policy formats. Creating an
> abstraction for each with a proper API in new definitions in
> include/sepol/*.h would be preferable albeit more work. There was a
> proposal a long time ago from the setools developers to create an
> iterator interface and accessor functions for each data type, see
> https://lore.kernel.org/selinux/200603212246.k2LMkRNq028071@gotham.columbia.tresys.com/.

We could also partially mitigate the problem by linking libsepol
statically into setools at build time. I suggested this in [1] for
Fedora at least, but in general there is the problem that you need to
know the exact path to libsepol.a in setup.py [2]. I don't have enough
experience with python libraries to know if this can be implemented
directly in the upstream setup.py script in a reasonably generic way.

If linked statically, at least it wouldn't segfault after an update to
an incompatible libsepol.so. It would still fail to build with the new
headers, but at least this turns from a user-visible bug to only
packager's & maintainer's problem.

[1] https://bugzilla.redhat.com/show_bug.cgi?id=1826000
[2] https://stackoverflow.com/questions/4597228/how-to-statically-link-a-library-when-compiling-a-python-module-extension/49139257#49139257

-- 
Ondrej Mosnacek <omosnace at redhat dot com>
Software Engineer, Security Technologies
Red Hat, Inc.

Re: [PATCH 0/2] userspace: Implement new format of filename trans rules

[Chris PeBenito]

On 4/30/20 9:22 AM, Stephen Smalley wrote:
> On Wed, Apr 29, 2020 at 3:01 PM James Carter <jwcart2@gmail.com> wrote:
>> I think the fact that the CIL, kernel to CIL, kernel to conf, and
>> module to CIL code is all in libsepol speaks to the fact of how
>> tightly integrated they are to the rest of libsepol. One argument that
>> could be made is that the policyrep stuff in setools really belongs in
>> libsepol.
>>
>> Thinking about how libsepol could be encapsulated leads me to a couple
>> of possibilities. One way would be functions that could return lists
>> of rules. The policy module code gives us avrule_t, role_trans_rule_t,
>> role_allow_t, filename_trans_rule_t, range_trans_rule_t, and others.
>> Those structures are probably unlikely to change and, at least in this
>> case, creating a function that walks the filename_trans hashtable and
>> returns a list of filename_trans_rule_t certainly seems like it
>> wouldn't be too hard. Another possible way to encapsulate would be
>> create a way to walk the various hashtables element by element (I
>> think hashtab_map() requires too much knowledge of the internal
>> structures), returning an opaque structure to track where you are in
>> the hashtable and functions that allow you to get each part of the
>> rule being stored. There are other ways that it could be done, but I
>> could rewrite kernel to and module to stuff with either of those. CIL
>> itself would require some functions to insert rules into the policydb
>> which probably wouldn't be too hard. None of this would be too hard,
>> but it would take some time. The real question is would it really be
>> valuable?
> 
> I don't think we want to directly expose the existing data structures
> from include/sepol/policydb/*.h (or at least not without a careful
> audit) since those are often tightly coupled to policy compiler
> internals and/or the kernel or module policy formats. Creating an
> abstraction for each with a proper API in new definitions in
> include/sepol/*.h would be preferable albeit more work. There was a
> proposal a long time ago from the setools developers to create an
> iterator interface and accessor functions for each data type, see
> https://lore.kernel.org/selinux/200603212246.k2LMkRNq028071@gotham.columbia.tresys.com/.

I agree.  The hardest thing with writing the policyrep in setools was stuff like 
the value_to_datum indirections, type_attr_map, etc. and knowing when to use 
value vs value-1.  An API that has a new set of structs would be ideal.

Unfortunately, since setools policyrep is now written in Cython, we can't simply 
move the code over to libsepol.  My guess is dispol has the most useful building 
blocks for making a new API.


-- 
Chris PeBenito

Re: [PATCH 0/2] userspace: Implement new format of filename trans rules

[Ondrej Mosnacek]

On Thu, Apr 30, 2020 at 4:24 PM Chris PeBenito <pebenito@ieee.org> wrote:
> On 4/30/20 9:22 AM, Stephen Smalley wrote:
> > On Wed, Apr 29, 2020 at 3:01 PM James Carter <jwcart2@gmail.com> wrote:
> >> I think the fact that the CIL, kernel to CIL, kernel to conf, and
> >> module to CIL code is all in libsepol speaks to the fact of how
> >> tightly integrated they are to the rest of libsepol. One argument that
> >> could be made is that the policyrep stuff in setools really belongs in
> >> libsepol.
> >>
> >> Thinking about how libsepol could be encapsulated leads me to a couple
> >> of possibilities. One way would be functions that could return lists
> >> of rules. The policy module code gives us avrule_t, role_trans_rule_t,
> >> role_allow_t, filename_trans_rule_t, range_trans_rule_t, and others.
> >> Those structures are probably unlikely to change and, at least in this
> >> case, creating a function that walks the filename_trans hashtable and
> >> returns a list of filename_trans_rule_t certainly seems like it
> >> wouldn't be too hard. Another possible way to encapsulate would be
> >> create a way to walk the various hashtables element by element (I
> >> think hashtab_map() requires too much knowledge of the internal
> >> structures), returning an opaque structure to track where you are in
> >> the hashtable and functions that allow you to get each part of the
> >> rule being stored. There are other ways that it could be done, but I
> >> could rewrite kernel to and module to stuff with either of those. CIL
> >> itself would require some functions to insert rules into the policydb
> >> which probably wouldn't be too hard. None of this would be too hard,
> >> but it would take some time. The real question is would it really be
> >> valuable?
> >
> > I don't think we want to directly expose the existing data structures
> > from include/sepol/policydb/*.h (or at least not without a careful
> > audit) since those are often tightly coupled to policy compiler
> > internals and/or the kernel or module policy formats. Creating an
> > abstraction for each with a proper API in new definitions in
> > include/sepol/*.h would be preferable albeit more work. There was a
> > proposal a long time ago from the setools developers to create an
> > iterator interface and accessor functions for each data type, see
> > https://lore.kernel.org/selinux/200603212246.k2LMkRNq028071@gotham.columbia.tresys.com/.
>
> I agree.  The hardest thing with writing the policyrep in setools was stuff like
> the value_to_datum indirections, type_attr_map, etc. and knowing when to use
> value vs value-1.  An API that has a new set of structs would be ideal.
>
> Unfortunately, since setools policyrep is now written in Cython, we can't simply
> move the code over to libsepol.  My guess is dispol has the most useful building
> blocks for making a new API.

Since you mention dispol... I also had the idea that setools could
just use the existing public interface to convert the whole policydb
to CIL and simply parse that as a string (this should be pretty
straightforward even in pure Python). However, based on my experiments
this would likely make setools a lot slower...

-- 
Ondrej Mosnacek <omosnace at redhat dot com>
Software Engineer, Security Technologies
Red Hat, Inc.

Re: [PATCH 0/2] userspace: Implement new format of filename trans rules

[Chris PeBenito]

On 4/30/20 10:20 AM, Ondrej Mosnacek wrote:
> On Thu, Apr 30, 2020 at 3:23 PM Stephen Smalley
> <stephen.smalley.work@gmail.com> wrote:
>> On Wed, Apr 29, 2020 at 3:01 PM James Carter <jwcart2@gmail.com> wrote:
>>> I think the fact that the CIL, kernel to CIL, kernel to conf, and
>>> module to CIL code is all in libsepol speaks to the fact of how
>>> tightly integrated they are to the rest of libsepol. One argument that
>>> could be made is that the policyrep stuff in setools really belongs in
>>> libsepol.
>>>
>>> Thinking about how libsepol could be encapsulated leads me to a couple
>>> of possibilities. One way would be functions that could return lists
>>> of rules. The policy module code gives us avrule_t, role_trans_rule_t,
>>> role_allow_t, filename_trans_rule_t, range_trans_rule_t, and others.
>>> Those structures are probably unlikely to change and, at least in this
>>> case, creating a function that walks the filename_trans hashtable and
>>> returns a list of filename_trans_rule_t certainly seems like it
>>> wouldn't be too hard. Another possible way to encapsulate would be
>>> create a way to walk the various hashtables element by element (I
>>> think hashtab_map() requires too much knowledge of the internal
>>> structures), returning an opaque structure to track where you are in
>>> the hashtable and functions that allow you to get each part of the
>>> rule being stored. There are other ways that it could be done, but I
>>> could rewrite kernel to and module to stuff with either of those. CIL
>>> itself would require some functions to insert rules into the policydb
>>> which probably wouldn't be too hard. None of this would be too hard,
>>> but it would take some time. The real question is would it really be
>>> valuable?
>>
>> I don't think we want to directly expose the existing data structures
>> from include/sepol/policydb/*.h (or at least not without a careful
>> audit) since those are often tightly coupled to policy compiler
>> internals and/or the kernel or module policy formats. Creating an
>> abstraction for each with a proper API in new definitions in
>> include/sepol/*.h would be preferable albeit more work. There was a
>> proposal a long time ago from the setools developers to create an
>> iterator interface and accessor functions for each data type, see
>> https://lore.kernel.org/selinux/200603212246.k2LMkRNq028071@gotham.columbia.tresys.com/.
> 
> We could also partially mitigate the problem by linking libsepol
> statically into setools at build time. I suggested this in [1] for
> Fedora at least, but in general there is the problem that you need to
> know the exact path to libsepol.a in setup.py [2]. I don't have enough
> experience with python libraries to know if this can be implemented
> directly in the upstream setup.py script in a reasonably generic way.
> 
> If linked statically, at least it wouldn't segfault after an update to
> an incompatible libsepol.so. It would still fail to build with the new
> headers, but at least this turns from a user-visible bug to only
> packager's & maintainer's problem.

It used to be static and people complained about that.  I moved it to dynamic 
knowing this would be an issue, but an uncommon one.


-- 
Chris PeBenito

Re: [PATCH 0/2] userspace: Implement new format of filename trans rules

[Chris PeBenito]

On 4/30/20 10:34 AM, Ondrej Mosnacek wrote:
> On Thu, Apr 30, 2020 at 4:24 PM Chris PeBenito <pebenito@ieee.org> wrote:
>> On 4/30/20 9:22 AM, Stephen Smalley wrote:
>>> On Wed, Apr 29, 2020 at 3:01 PM James Carter <jwcart2@gmail.com> wrote:
>>>> I think the fact that the CIL, kernel to CIL, kernel to conf, and
>>>> module to CIL code is all in libsepol speaks to the fact of how
>>>> tightly integrated they are to the rest of libsepol. One argument that
>>>> could be made is that the policyrep stuff in setools really belongs in
>>>> libsepol.
>>>>
>>>> Thinking about how libsepol could be encapsulated leads me to a couple
>>>> of possibilities. One way would be functions that could return lists
>>>> of rules. The policy module code gives us avrule_t, role_trans_rule_t,
>>>> role_allow_t, filename_trans_rule_t, range_trans_rule_t, and others.
>>>> Those structures are probably unlikely to change and, at least in this
>>>> case, creating a function that walks the filename_trans hashtable and
>>>> returns a list of filename_trans_rule_t certainly seems like it
>>>> wouldn't be too hard. Another possible way to encapsulate would be
>>>> create a way to walk the various hashtables element by element (I
>>>> think hashtab_map() requires too much knowledge of the internal
>>>> structures), returning an opaque structure to track where you are in
>>>> the hashtable and functions that allow you to get each part of the
>>>> rule being stored. There are other ways that it could be done, but I
>>>> could rewrite kernel to and module to stuff with either of those. CIL
>>>> itself would require some functions to insert rules into the policydb
>>>> which probably wouldn't be too hard. None of this would be too hard,
>>>> but it would take some time. The real question is would it really be
>>>> valuable?
>>>
>>> I don't think we want to directly expose the existing data structures
>>> from include/sepol/policydb/*.h (or at least not without a careful
>>> audit) since those are often tightly coupled to policy compiler
>>> internals and/or the kernel or module policy formats. Creating an
>>> abstraction for each with a proper API in new definitions in
>>> include/sepol/*.h would be preferable albeit more work. There was a
>>> proposal a long time ago from the setools developers to create an
>>> iterator interface and accessor functions for each data type, see
>>> https://lore.kernel.org/selinux/200603212246.k2LMkRNq028071@gotham.columbia.tresys.com/.
>>
>> I agree.  The hardest thing with writing the policyrep in setools was stuff like
>> the value_to_datum indirections, type_attr_map, etc. and knowing when to use
>> value vs value-1.  An API that has a new set of structs would be ideal.
>>
>> Unfortunately, since setools policyrep is now written in Cython, we can't simply
>> move the code over to libsepol.  My guess is dispol has the most useful building
>> blocks for making a new API.
> 
> Since you mention dispol... I also had the idea that setools could
> just use the existing public interface to convert the whole policydb
> to CIL and simply parse that as a string (this should be pretty
> straightforward even in pure Python). However, based on my experiments
> this would likely make setools a lot slower...

This is an interesting idea.  I'm not familiar with the CIL API; secilc.c looks 
like it uses public API.  Can I use the existing CIL library functions to parse 
it, or does the resultant db lack public accessor functions?

-- 
Chris PeBenito

Re: [PATCH 0/2] userspace: Implement new format of filename trans rules

[James Carter]

On Thu, Apr 30, 2020 at 9:23 AM Stephen Smalley
<stephen.smalley.work@gmail.com> wrote:
>
> On Wed, Apr 29, 2020 at 3:01 PM James Carter <jwcart2@gmail.com> wrote:
> > I think the fact that the CIL, kernel to CIL, kernel to conf, and
> > module to CIL code is all in libsepol speaks to the fact of how
> > tightly integrated they are to the rest of libsepol. One argument that
> > could be made is that the policyrep stuff in setools really belongs in
> > libsepol.
> >
> > Thinking about how libsepol could be encapsulated leads me to a couple
> > of possibilities. One way would be functions that could return lists
> > of rules. The policy module code gives us avrule_t, role_trans_rule_t,
> > role_allow_t, filename_trans_rule_t, range_trans_rule_t, and others.
> > Those structures are probably unlikely to change and, at least in this
> > case, creating a function that walks the filename_trans hashtable and
> > returns a list of filename_trans_rule_t certainly seems like it
> > wouldn't be too hard. Another possible way to encapsulate would be
> > create a way to walk the various hashtables element by element (I
> > think hashtab_map() requires too much knowledge of the internal
> > structures), returning an opaque structure to track where you are in
> > the hashtable and functions that allow you to get each part of the
> > rule being stored. There are other ways that it could be done, but I
> > could rewrite kernel to and module to stuff with either of those. CIL
> > itself would require some functions to insert rules into the policydb
> > which probably wouldn't be too hard. None of this would be too hard,
> > but it would take some time. The real question is would it really be
> > valuable?
>
> I don't think we want to directly expose the existing data structures
> from include/sepol/policydb/*.h (or at least not without a careful
> audit) since those are often tightly coupled to policy compiler
> internals and/or the kernel or module policy formats. Creating an
> abstraction for each with a proper API in new definitions in
> include/sepol/*.h would be preferable albeit more work. There was a
> proposal a long time ago from the setools developers to create an
> iterator interface and accessor functions for each data type, see
> https://lore.kernel.org/selinux/200603212246.k2LMkRNq028071@gotham.columbia.tresys.com/.

I am so used to everything just using the stuff in
include/sepol/policydb/ that I forget that it exists. There are a
number of iterators there (for users, ports, nodes, etc), so it seems
like creating an iterator and other functions for filename transition
rules and converting setools to use that would be the first step. Over
time other iterators can be created and setools converted to use those
as well.

Jim

Re: [PATCH 0/2] userspace: Implement new format of filename trans rules

[James Carter]

On Thu, Apr 30, 2020 at 11:20 AM Chris PeBenito <pebenito@ieee.org> wrote:
>
> On 4/30/20 10:34 AM, Ondrej Mosnacek wrote:
> > On Thu, Apr 30, 2020 at 4:24 PM Chris PeBenito <pebenito@ieee.org> wrote:
> >> On 4/30/20 9:22 AM, Stephen Smalley wrote:
> >>> On Wed, Apr 29, 2020 at 3:01 PM James Carter <jwcart2@gmail.com> wrote:
> >>>> I think the fact that the CIL, kernel to CIL, kernel to conf, and
> >>>> module to CIL code is all in libsepol speaks to the fact of how
> >>>> tightly integrated they are to the rest of libsepol. One argument that
> >>>> could be made is that the policyrep stuff in setools really belongs in
> >>>> libsepol.
> >>>>
> >>>> Thinking about how libsepol could be encapsulated leads me to a couple
> >>>> of possibilities. One way would be functions that could return lists
> >>>> of rules. The policy module code gives us avrule_t, role_trans_rule_t,
> >>>> role_allow_t, filename_trans_rule_t, range_trans_rule_t, and others.
> >>>> Those structures are probably unlikely to change and, at least in this
> >>>> case, creating a function that walks the filename_trans hashtable and
> >>>> returns a list of filename_trans_rule_t certainly seems like it
> >>>> wouldn't be too hard. Another possible way to encapsulate would be
> >>>> create a way to walk the various hashtables element by element (I
> >>>> think hashtab_map() requires too much knowledge of the internal
> >>>> structures), returning an opaque structure to track where you are in
> >>>> the hashtable and functions that allow you to get each part of the
> >>>> rule being stored. There are other ways that it could be done, but I
> >>>> could rewrite kernel to and module to stuff with either of those. CIL
> >>>> itself would require some functions to insert rules into the policydb
> >>>> which probably wouldn't be too hard. None of this would be too hard,
> >>>> but it would take some time. The real question is would it really be
> >>>> valuable?
> >>>
> >>> I don't think we want to directly expose the existing data structures
> >>> from include/sepol/policydb/*.h (or at least not without a careful
> >>> audit) since those are often tightly coupled to policy compiler
> >>> internals and/or the kernel or module policy formats. Creating an
> >>> abstraction for each with a proper API in new definitions in
> >>> include/sepol/*.h would be preferable albeit more work. There was a
> >>> proposal a long time ago from the setools developers to create an
> >>> iterator interface and accessor functions for each data type, see
> >>> https://lore.kernel.org/selinux/200603212246.k2LMkRNq028071@gotham.columbia.tresys.com/.
> >>
> >> I agree.  The hardest thing with writing the policyrep in setools was stuff like
> >> the value_to_datum indirections, type_attr_map, etc. and knowing when to use
> >> value vs value-1.  An API that has a new set of structs would be ideal.
> >>
> >> Unfortunately, since setools policyrep is now written in Cython, we can't simply
> >> move the code over to libsepol.  My guess is dispol has the most useful building
> >> blocks for making a new API.
> >
> > Since you mention dispol... I also had the idea that setools could
> > just use the existing public interface to convert the whole policydb
> > to CIL and simply parse that as a string (this should be pretty
> > straightforward even in pure Python). However, based on my experiments
> > this would likely make setools a lot slower...
>
> This is an interesting idea.  I'm not familiar with the CIL API; secilc.c looks
> like it uses public API.  Can I use the existing CIL library functions to parse
> it, or does the resultant db lack public accessor functions?
>

The resultant db does, in fact, lack public assessor functions. They
could be created, but since there is already a way to convert the
cil_db to a policydb, the general solution would be to create the
functions for the policydb.

Jim

Re: [PATCH 0/2] userspace: Implement new format of filename trans rules

[Ondrej Mosnacek]

On Thu, Apr 30, 2020 at 5:20 PM Chris PeBenito <pebenito@ieee.org> wrote:
> On 4/30/20 10:34 AM, Ondrej Mosnacek wrote:
> > On Thu, Apr 30, 2020 at 4:24 PM Chris PeBenito <pebenito@ieee.org> wrote:
> >> On 4/30/20 9:22 AM, Stephen Smalley wrote:
> >>> On Wed, Apr 29, 2020 at 3:01 PM James Carter <jwcart2@gmail.com> wrote:
> >>>> I think the fact that the CIL, kernel to CIL, kernel to conf, and
> >>>> module to CIL code is all in libsepol speaks to the fact of how
> >>>> tightly integrated they are to the rest of libsepol. One argument that
> >>>> could be made is that the policyrep stuff in setools really belongs in
> >>>> libsepol.
> >>>>
> >>>> Thinking about how libsepol could be encapsulated leads me to a couple
> >>>> of possibilities. One way would be functions that could return lists
> >>>> of rules. The policy module code gives us avrule_t, role_trans_rule_t,
> >>>> role_allow_t, filename_trans_rule_t, range_trans_rule_t, and others.
> >>>> Those structures are probably unlikely to change and, at least in this
> >>>> case, creating a function that walks the filename_trans hashtable and
> >>>> returns a list of filename_trans_rule_t certainly seems like it
> >>>> wouldn't be too hard. Another possible way to encapsulate would be
> >>>> create a way to walk the various hashtables element by element (I
> >>>> think hashtab_map() requires too much knowledge of the internal
> >>>> structures), returning an opaque structure to track where you are in
> >>>> the hashtable and functions that allow you to get each part of the
> >>>> rule being stored. There are other ways that it could be done, but I
> >>>> could rewrite kernel to and module to stuff with either of those. CIL
> >>>> itself would require some functions to insert rules into the policydb
> >>>> which probably wouldn't be too hard. None of this would be too hard,
> >>>> but it would take some time. The real question is would it really be
> >>>> valuable?
> >>>
> >>> I don't think we want to directly expose the existing data structures
> >>> from include/sepol/policydb/*.h (or at least not without a careful
> >>> audit) since those are often tightly coupled to policy compiler
> >>> internals and/or the kernel or module policy formats. Creating an
> >>> abstraction for each with a proper API in new definitions in
> >>> include/sepol/*.h would be preferable albeit more work. There was a
> >>> proposal a long time ago from the setools developers to create an
> >>> iterator interface and accessor functions for each data type, see
> >>> https://lore.kernel.org/selinux/200603212246.k2LMkRNq028071@gotham.columbia.tresys.com/.
> >>
> >> I agree.  The hardest thing with writing the policyrep in setools was stuff like
> >> the value_to_datum indirections, type_attr_map, etc. and knowing when to use
> >> value vs value-1.  An API that has a new set of structs would be ideal.
> >>
> >> Unfortunately, since setools policyrep is now written in Cython, we can't simply
> >> move the code over to libsepol.  My guess is dispol has the most useful building
> >> blocks for making a new API.
> >
> > Since you mention dispol... I also had the idea that setools could
> > just use the existing public interface to convert the whole policydb
> > to CIL and simply parse that as a string (this should be pretty
> > straightforward even in pure Python). However, based on my experiments
> > this would likely make setools a lot slower...
>
> This is an interesting idea.  I'm not familiar with the CIL API; secilc.c looks
> like it uses public API.  Can I use the existing CIL library functions to parse
> it, or does the resultant db lack public accessor functions?

What I had in mind was actually to just use
sepol_kernel_policydb_to_cil() to dump the textual CIL into a
temporary file (maybe using tmpfile(3)), and then parse the contents
in Python. The CIL format is really easy to parse (especially in
Python) so a lack of existing functions for that wouldn't be much of
an issue. Yes, this would be a little dirty, but you'd avoid the
trouble of maintaining a stable binary interface between libsepol and
setools.

-- 
Ondrej Mosnacek <omosnace at redhat dot com>
Software Engineer, Security Technologies
Red Hat, Inc.