On Tue, 13 Feb 2024 at 21:56, James Carter <jwcart2@xxxxxxxxx> wrote:
>
> 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.
I tried to avoid touching anything related to the `defined` member in
the checkpolicy patchset, since my plan was to remove the member in a
couple months, when the fuzzer has verified it is redundant after the
new member `copy` was introduced.
Currently the member `defined` is only checked once in the entire code
base: in a sanity check in checkpolicy that I never saw triggered.
So it is unused during binary policy parsing and CIL policy
compilation (and also unnecessary for correct cleanup there, since the
two active fuzzers have not found any related use-after-free issue or
leak).
Thus my preference is to have in the end only the `copy` member from
my patch 10/15, which does not need to be set everywhere manually
since the default calloc'ed value of 0 is the correct default, and
it's only going to be used in three places:
libsepol/src/policydb.c:sens_destroy(),
checkpolicy/policy_define.c:define_sens() and
checkpolicy/policy_define.c:clone_level().
> Signed-off-by: James Carter <jwcart2@xxxxxxxxx>
> ---
> 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
>
