[PATCH] checkpolicy, libsepol: Fix potential double free of mls_level_t

[James Carter <jwcart2@gmail.com>]

In checkpolicy, sensitivities that have aliases will temporarily
share the mls_level_t structure until a level statement defines the
categories for the level and the alias is updated to have its own
mls_level_t structure. Currently, this does not cause a problem
because checkpolicy does very little clean-up before exiting when
an error is detected. But if the policydb is destroyed before exiting
due to an error after a sensitivity and its alias is declared, but
before a level statement involving either of them, then a double
free of the shared mls_level_t will occur.

The defined field of the level_datum_t is set after a level statement
is processed for the level_datum_t. This means that we know the alias
has its own mls_level_t if the defined field is set. This means that
the defined field can be used to determine whether or not the
mls_level_t pointed to by an alias level_datum_t should be destroyed.

Since the defined field is not set when reading or expanding a policy,
update libsepol to set the defined field.

Signed-off-by: James Carter <jwcart2@gmail.com>
---
 checkpolicy/policy_define.c | 11 +++++++----
 libsepol/src/expand.c       |  1 +
 libsepol/src/policydb.c     |  7 +++++--
 3 files changed, 13 insertions(+), 6 deletions(-)

diff --git a/checkpolicy/policy_define.c b/checkpolicy/policy_define.c
index 260e609d..542bb978 100644
--- a/checkpolicy/policy_define.c
+++ b/checkpolicy/policy_define.c
@@ -1006,9 +1006,10 @@ static int clone_level(hashtab_key_t key __attribute__ ((unused)), hashtab_datum
 	mls_level_t *level = (mls_level_t *) arg, *newlevel;
 
 	if (levdatum->level == level) {
-		levdatum->defined = 1;
-		if (!levdatum->isalias)
+		if (!levdatum->isalias) {
+			levdatum->defined = 1;
 			return 0;
+		}
 		newlevel = (mls_level_t *) malloc(sizeof(mls_level_t));
 		if (!newlevel)
 			return -1;
@@ -1017,6 +1018,7 @@ static int clone_level(hashtab_key_t key __attribute__ ((unused)), hashtab_datum
 			return -1;
 		}
 		levdatum->level = newlevel;
+		levdatum->defined = 1;
 	}
 	return 0;
 }
@@ -1057,8 +1059,6 @@ int define_level(void)
 	}
 	free(id);
 
-	levdatum->defined = 1;
-
 	while ((id = queue_remove(id_queue))) {
 		cat_datum_t *cdatum;
 		int range_start, range_end, i;
@@ -1121,6 +1121,9 @@ int define_level(void)
 		free(id);
 	}
 
+	if (!levdatum->isalias)
+		levdatum->defined = 1;
+
 	if (hashtab_map
 	    (policydbp->p_levels.table, clone_level, levdatum->level)) {
 		yyerror("out of memory");
diff --git a/libsepol/src/expand.c b/libsepol/src/expand.c
index e63414b1..0e16c502 100644
--- a/libsepol/src/expand.c
+++ b/libsepol/src/expand.c
@@ -1191,6 +1191,7 @@ static int sens_copy_callback(hashtab_key_t key, hashtab_datum_t datum,
 		goto out_of_mem;
 	}
 	new_level->isalias = level->isalias;
+	new_level->defined = 1;
 	state->out->p_levels.nprim++;
 
 	if (hashtab_insert(state->out->p_levels.table,
diff --git a/libsepol/src/policydb.c b/libsepol/src/policydb.c
index f10a8a95..0c950bf1 100644
--- a/libsepol/src/policydb.c
+++ b/libsepol/src/policydb.c
@@ -1390,8 +1390,10 @@ static int sens_destroy(hashtab_key_t key, hashtab_datum_t datum, void *p
 	if (key)
 		free(key);
 	levdatum = (level_datum_t *) datum;
-	mls_level_destroy(levdatum->level);
-	free(levdatum->level);
+	if (!levdatum->isalias || levdatum->defined) {
+		mls_level_destroy(levdatum->level);
+		free(levdatum->level);
+	}
 	level_datum_destroy(levdatum);
 	free(levdatum);
 	return 0;
@@ -3357,6 +3359,7 @@ static int sens_read(policydb_t * p
 		goto bad;
 
 	levdatum->isalias = le32_to_cpu(buf[1]);
+	levdatum->defined = 1;
 
 	levdatum->level = malloc(sizeof(mls_level_t));
 	if (!levdatum->level || mls_read_level(levdatum->level, fp))
-- 
2.43.0