On Thu, Feb 15, 2024 at 1:41 PM Christian Göttsche
<cgzones@xxxxxxxxxxxxxx> wrote:
>
> 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().
>
I like the idea of not having to set the value outside of checkpolicy.
I don't like the idea of having both a "defined" and a "copy" field in
the struct.
Checkpolicy needs the "defined" field to check if a sensitivity has a
level statement associated with it.
I sent v2 of the patch where I change the field to "notdefined". It is
a bit awkward, but it does mean that the "notdefined" field is never
set except for in checkpolicy.
Thanks for the comments,
Jim
> > 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
> >
