| Message ID | 20240213205605.830719-1-jwcart2@gmail.com (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Petr Lautrbach |
| Headers | show |
| Series | checkpolicy, libsepol: Fix potential double free of mls_level_t | expand |
On Tue, 13 Feb 2024 at 21:56, James Carter <jwcart2@gmail.com> 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(). > Signed-off-by: James Carter <jwcart2@gmail.com> > --- > 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 >
On Thu, Feb 15, 2024 at 1:41 PM Christian Göttsche <cgzones@googlemail.com> wrote: > > On Tue, 13 Feb 2024 at 21:56, James Carter <jwcart2@gmail.com> 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@gmail.com> > > --- > > 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 > >
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))
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. Signed-off-by: James Carter <jwcart2@gmail.com> --- checkpolicy/policy_define.c | 11 +++++++---- libsepol/src/expand.c | 1 + libsepol/src/policydb.c | 7 +++++-- 3 files changed, 13 insertions(+), 6 deletions(-)