Re: [PATCH 10/15] libsepol: add copy member to level_datum

From: James Carter <jwcart2@gmail.com>
To: "Christian Göttsche" <cgzones@googlemail.com>
Cc: selinux@vger.kernel.org
Subject: Re: [PATCH 10/15] libsepol: add copy member to level_datum
Date: Mon, 12 Feb 2024 17:30:16 -0500	[thread overview]
Message-ID: <CAP+JOzR6ph5pN_yLyvVh2+0deknKOmD_MZs39sd0ZbEnfL3O+A@mail.gmail.com> (raw)
In-Reply-To: <20240122135507.63506-10-cgzones@googlemail.com>

On Mon, Jan 22, 2024 at 9:02 AM Christian Göttsche
<cgzones@googlemail.com> wrote:
>
> Add a new member to the struct level_datum to indicate whether the
> member `level` is owned by the current instance, and free it on cleanup
> only then.
>
> This helps to implement a fix for a use-after-free issue in the
> checkpolicy(8) compiler.
>
> Signed-off-by: Christian Göttsche <cgzones@googlemail.com>
> ---
>  libsepol/include/sepol/policydb/policydb.h | 1 +
>  libsepol/src/policydb.c                    | 6 ++++--
>  libsepol/src/policydb_validate.c           | 3 +++
>  3 files changed, 8 insertions(+), 2 deletions(-)
>
> diff --git a/libsepol/include/sepol/policydb/policydb.h b/libsepol/include/sepol/policydb/policydb.h
> index 6682069e..06231574 100644
> --- a/libsepol/include/sepol/policydb/policydb.h
> +++ b/libsepol/include/sepol/policydb/policydb.h
> @@ -218,6 +218,7 @@ 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 copy;     /* whether the level is a non-owned copy (compile time only) */
>  } level_datum_t;
>

I don't think this field needs to be added. See below.

>  /* Category attributes */
> diff --git a/libsepol/src/policydb.c b/libsepol/src/policydb.c
> index f10a8a95..322ab745 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->copy) {

I believe that the following can be made to work:
if (!levdatum->isalias || levdatum->defined) {

To work, clone_level() and define_level() will need to be modified, so
that defined is not set until right before finishing the call.

Jim

> +               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..e3af7ccd 100644
> --- a/libsepol/src/policydb_validate.c
> +++ b/libsepol/src/policydb_validate.c
> @@ -623,6 +623,9 @@ static int validate_level_datum(__attribute__ ((unused)) hashtab_key_t k, hashta
>         level_datum_t *level = d;
>         validate_t *flavors = args;
>
> +       if (level->copy)
> +               return -1;
> +
>         return validate_mls_level(level->level, &flavors[SYM_LEVELS], &flavors[SYM_CATS]);
>  }
>
> --
> 2.43.0
>
>