| Message ID | 20240122135507.63506-10-cgzones@googlemail.com (mailing list archive) |
|---|---|
| State | New |
| Delegated to: | Petr Lautrbach |
| Headers | show |
| Series | [01/15] checkpolicy: add libfuzz based fuzzer | expand |
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 > >
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; /* 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) { + 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]); }
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(-)