* [PATCH v2] checkpolicy, libsepol: Fix potential double free of mls_level_t
@ 2024-02-21 21:07 James Carter
2024-02-26 16:33 ` Christian Göttsche
0 siblings, 1 reply; 3+ messages in thread
From: James Carter @ 2024-02-21 21:07 UTC (permalink / raw)
To: selinux; +Cc: cgzones, James Carter
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@gmail.com>
---
v2: Change the field name from "defined" to "notdefined" and change
the logic to match.
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;
} 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
^ permalink raw reply related [flat|nested] 3+ messages in thread
* Re: [PATCH v2] checkpolicy, libsepol: Fix potential double free of mls_level_t
2024-02-21 21:07 [PATCH v2] checkpolicy, libsepol: Fix potential double free of mls_level_t James Carter
@ 2024-02-26 16:33 ` Christian Göttsche
2024-02-28 13:48 ` James Carter
0 siblings, 1 reply; 3+ messages in thread
From: Christian Göttsche @ 2024-02-26 16:33 UTC (permalink / raw)
To: James Carter; +Cc: selinux
On Wed, 21 Feb 2024 at 22:08, James Carter <jwcart2@gmail.com> 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@gmail.com>
> ---
> 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?
> 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?
> } 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
>
^ permalink raw reply [flat|nested] 3+ messages in thread
* Re: [PATCH v2] checkpolicy, libsepol: Fix potential double free of mls_level_t
2024-02-26 16:33 ` Christian Göttsche
@ 2024-02-28 13:48 ` James Carter
0 siblings, 0 replies; 3+ messages in thread
From: James Carter @ 2024-02-28 13:48 UTC (permalink / raw)
To: Christian Göttsche; +Cc: selinux
On Mon, Feb 26, 2024 at 11:33 AM Christian Göttsche
<cgzones@googlemail.com> wrote:
>
> On Wed, 21 Feb 2024 at 22:08, James Carter <jwcart2@gmail.com> 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@gmail.com>
> > ---
> > 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
> >
^ permalink raw reply [flat|nested] 3+ messages in thread
end of thread, other threads:[~2024-02-28 13:49 UTC | newest]
Thread overview: 3+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2024-02-21 21:07 [PATCH v2] checkpolicy, libsepol: Fix potential double free of mls_level_t James Carter
2024-02-26 16:33 ` Christian Göttsche
2024-02-28 13:48 ` James Carter
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.