On Wed, Feb 28, 2024 at 3:43 PM James Carter <jwcart2@xxxxxxxxx> wrote:
>
> In checkpolicy, a sensitivity that has one or more aliases will
> temporarily share the mls_level_t structure with its aliases until
> a level statement is processed for the sensitivity (or one of the
> aliases) and the aliases are updated to have their own mls_level_t
> structure. If the policydb is destroyed while they are sharing the
> mls_level_t structure, then a double free of the shared mls_level_t
> will occur. This does not currently occur only because checkpolicy
> does very little clean-up before exiting.
>
> The "defined" field of the level_datum_t is set after a level
> statement is processed for a sensitivity and its aliases. This means
> that we know an alias has its own mls_level_t if the "defined" field
> is set. The double free can be avoided by not destroying the
> mls_leve_t structure for an alias unless the "defined" field is set.
>
> Since the "defined" field is only set to false while the mls_level_t
> structure is being shared, it would be clearer to rename the field
> as "notdefined". It would only be set during the time the sensitivity
> and its aliases are sharing the mls_level_t structure. Outside of
> checkpolicy, the "notdefined" field will always be set to 0.
>
> Also, do more validation of the level_datum_t when validating the
> policydb.
>
> Signed-off-by: James Carter <jwcart2@xxxxxxxxx>
This patch has been merged.
Jim
> ---
> v3: Add a comment about the "notdefined" field in the definition of level_datum_t.
> Do more validation of the level_datum_t when validating the policydb.
> v2: Change the field name from "defined" to "notdefined" and change
> the logic to match.
>
> checkpolicy/checkpolicy.c | 7 ++---
> checkpolicy/policy_define.c | 10 ++++---
> libsepol/cil/src/cil_binary.c | 3 --
> libsepol/include/sepol/policydb/policydb.h | 2 +-
> libsepol/src/policydb.c | 6 ++--
> libsepol/src/policydb_validate.c | 35 ++++++++++++++++++----
> 6 files changed, 44 insertions(+), 19 deletions(-)
>
> diff --git a/checkpolicy/checkpolicy.c b/checkpolicy/checkpolicy.c
> index fcec6e51..d7cafaa4 100644
> --- a/checkpolicy/checkpolicy.c
> +++ b/checkpolicy/checkpolicy.c
> @@ -370,10 +370,9 @@ static int check_level(hashtab_key_t key, hashtab_datum_t datum, void *arg __att
> {
> level_datum_t *levdatum = (level_datum_t *) datum;
>
> - if (!levdatum->isalias && !levdatum->defined) {
> - fprintf(stderr,
> - "Error: sensitivity %s was not used in a level definition!\n",
> - key);
> + if (!levdatum->isalias && levdatum->notdefined) {
> + fprintf(stderr, "Error: sensitivity %s was not used in a level definition!\n",
> + key);
> return -1;
> }
> return 0;
> diff --git a/checkpolicy/policy_define.c b/checkpolicy/policy_define.c
> index 260e609d..ac215086 100644
> --- a/checkpolicy/policy_define.c
> +++ b/checkpolicy/policy_define.c
> @@ -743,6 +743,7 @@ int define_sens(void)
> level_datum_init(datum);
> datum->isalias = FALSE;
> datum->level = level;
> + datum->notdefined = TRUE;
>
> ret = declare_symbol(SYM_LEVELS, id, datum, &value, &value);
> switch (ret) {
> @@ -780,6 +781,7 @@ int define_sens(void)
> level_datum_init(aliasdatum);
> aliasdatum->isalias = TRUE;
> aliasdatum->level = level;
> + aliasdatum->notdefined = TRUE;
>
> ret = declare_symbol(SYM_LEVELS, id, aliasdatum, NULL, &value);
> switch (ret) {
> @@ -1006,9 +1008,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->notdefined = FALSE;
> return 0;
> + }
> newlevel = (mls_level_t *) malloc(sizeof(mls_level_t));
> if (!newlevel)
> return -1;
> @@ -1017,6 +1020,7 @@ static int clone_level(hashtab_key_t key __attribute__ ((unused)), hashtab_datum
> return -1;
> }
> levdatum->level = newlevel;
> + levdatum->notdefined = FALSE;
> }
> return 0;
> }
> @@ -1057,8 +1061,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;
> diff --git a/libsepol/cil/src/cil_binary.c b/libsepol/cil/src/cil_binary.c
> index a8e3616a..95bd18ba 100644
> --- a/libsepol/cil/src/cil_binary.c
> +++ b/libsepol/cil/src/cil_binary.c
> @@ -907,7 +907,6 @@ static int cil_sensalias_to_policydb(policydb_t *pdb, struct cil_alias *cil_alia
> goto exit;
> }
> sepol_alias->level = mls_level;
> - sepol_alias->defined = 1;
> sepol_alias->isalias = 1;
>
> return SEPOL_OK;
> @@ -3163,8 +3162,6 @@ int cil_sepol_level_define(policydb_t *pdb, struct cil_sens *cil_sens)
> }
> }
>
> - sepol_level->defined = 1;
> -
> return SEPOL_OK;
>
> exit:
> diff --git a/libsepol/include/sepol/policydb/policydb.h b/libsepol/include/sepol/policydb/policydb.h
> index 6682069e..56d2cb01 100644
> --- a/libsepol/include/sepol/policydb/policydb.h
> +++ b/libsepol/include/sepol/policydb/policydb.h
> @@ -217,7 +217,7 @@ typedef struct user_datum {
> typedef struct level_datum {
> mls_level_t *level; /* sensitivity and associated categories */
> unsigned char isalias; /* is this sensitivity an alias for another? */
> - unsigned char defined;
> + unsigned char notdefined; /* Only set to non-zero in checkpolicy */
> } level_datum_t;
>
> /* Category attributes */
> diff --git a/libsepol/src/policydb.c b/libsepol/src/policydb.c
> index f10a8a95..0c23a7a2 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->notdefined) {
> + mls_level_destroy(levdatum->level);
> + free(levdatum->level);
> + }
> level_datum_destroy(levdatum);
> free(levdatum);
> return 0;
> diff --git a/libsepol/src/policydb_validate.c b/libsepol/src/policydb_validate.c
> index d86f885e..1d8de44e 100644
> --- a/libsepol/src/policydb_validate.c
> +++ b/libsepol/src/policydb_validate.c
> @@ -618,14 +618,39 @@ static int validate_mls_level(const mls_level_t *level, const validate_t *sens,
> return -1;
> }
>
> -static int validate_level_datum(__attribute__ ((unused)) hashtab_key_t k, hashtab_datum_t d, void *args)
> +static int validate_level_datum(sepol_handle_t *handle, const level_datum_t *level, validate_t flavors[], const policydb_t *p)
> {
> - level_datum_t *level = d;
> - validate_t *flavors = args;
> + if (level->notdefined != 0)
> + goto bad;
> +
> + if (validate_mls_level(level->level, &flavors[SYM_LEVELS], &flavors[SYM_CATS]))
> + goto bad;
> +
> + if (level->isalias) {
> + const mls_level_t *l1 = level->level;
> + const mls_level_t *l2;
> + const level_datum_t *actual = (level_datum_t *) hashtab_search(p->p_levels.table, p->p_sens_val_to_name[l1->sens - 1]);
> + if (!actual)
> + goto bad;
> + l2 = actual->level;
> + if (!ebitmap_cmp(&l1->cat, &l2->cat))
> + goto bad;
> + }
>
> - return validate_mls_level(level->level, &flavors[SYM_LEVELS], &flavors[SYM_CATS]);
> + return 0;
> +
> + bad:
> + ERR(handle, "Invalid level datum");
> + return -1;
> }
>
> +static int validate_level_datum_wrapper(__attribute__ ((unused)) hashtab_key_t k, hashtab_datum_t d, void *args)
> +{
> + map_arg_t *margs = args;
> +
> + return validate_level_datum(margs->handle, d, margs->flavors, margs->policy);
> +}
> +
> static int validate_mls_range(const mls_range_t *range, const validate_t *sens, const validate_t *cats)
> {
> if (validate_mls_level(&range->level[0], sens, cats))
> @@ -774,7 +799,7 @@ static int validate_datum_array_entries(sepol_handle_t *handle, const policydb_t
> if (hashtab_map(p->p_users.table, validate_user_datum_wrapper, &margs))
> goto bad;
>
> - if (p->mls && hashtab_map(p->p_levels.table, validate_level_datum, flavors))
> + if (p->mls && hashtab_map(p->p_levels.table, validate_level_datum_wrapper, &margs))
> goto bad;
>
> if (hashtab_map(p->p_cats.table, validate_datum, &flavors[SYM_CATS]))
> --
> 2.43.2
>
