On Mon, Feb 26, 2024 at 11:33 AM Christian Göttsche
<cgzones@xxxxxxxxxxxxxx> wrote:
>
> On Wed, 21 Feb 2024 at 22:08, 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.
> >
> > Signed-off-by: James Carter <jwcart2@xxxxxxxxx>
> > ---
> > v2: Change the field name from "defined" to "notdefined" and change
> > the logic to match.
>
> Thanks, in my opinion this is a much nicer approach.
>
> Maybe check in libsepol/src/policydb_validate.c:validate_level_datum()
> that notdefined is FALSE?
>
That is a good idea.
> > 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 ++++--
> > 5 files changed, 14 insertions(+), 14 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..66d93999 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;
>
> Maybe add a small comment that it's only used as an optimization in
> checkpolicy and is 0 for fully parsed or generated policies?
>
Also a good idea.
Thanks,
Jim
> > } 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;
> > --
> > 2.43.0
> >
