| Message ID | 20240615132525.36358-1-cgoettsche@seltendoof.de (mailing list archive) |
|---|---|
| State | Accepted |
| Commit | 589e2dba72f4 |
| Delegated to: | Petr Lautrbach |
| Headers | show |
| Series | libsepol: check scope permissions refer to valid class | expand |
> From: Christian Göttsche <cgzones@googlemail.com> > > Validate that the permission maps in the scope index refer to a valid > class datum. Otherwise since commit 52e5c306 ("libsepol: move > unchanged > data out of loop") this can lead to a NULL dereference in the class > existence check during linking. > > Reported-by: oss-fuzz (issue 69655) > Signed-off-by: Christian Göttsche <cgzones@googlemail.com> > --- > libsepol/src/policydb_validate.c | 8 ++++++-- > 1 file changed, 6 insertions(+), 2 deletions(-) > > diff --git a/libsepol/src/policydb_validate.c > b/libsepol/src/policydb_validate.c > index 9746f562..0216410c 100644 > --- a/libsepol/src/policydb_validate.c > +++ b/libsepol/src/policydb_validate.c > @@ -1467,6 +1467,8 @@ bad: > > static int validate_scope_index(sepol_handle_t *handle, const > scope_index_t *scope_index, validate_t flavors[]) > { > + uint32_t i; > + > if (!ebitmap_is_empty(&scope_index->scope[SYM_COMMONS])) > goto bad; > if (validate_ebitmap(&scope_index->p_classes_scope, > &flavors[SYM_CLASSES])) > @@ -1483,8 +1485,10 @@ static int validate_scope_index(sepol_handle_t > *handle, const scope_index_t *sco > goto bad; > if (validate_ebitmap(&scope_index->p_cat_scope, > &flavors[SYM_CATS])) > goto bad; > - if (scope_index->class_perms_len > flavors[SYM_CLASSES].nprim) Since classes should not have gaps, this could be a simple off-by-one, cause class_perms_len is 0-based while nprim is 1-based. So scope_index->class_perms_len >= flavors[SYM_CLASSES].nprim might suffice, but I am unable to test currently though. > - goto bad; > + > + for (i = 0; i < scope_index->class_perms_len; i++) > + if (validate_value(i + 1, &flavors[SYM_CLASSES])) > + goto bad; > > return 0;
On Sat, Jun 15, 2024 at 9:25 AM Christian Göttsche <cgoettsche@seltendoof.de> wrote: > > From: Christian Göttsche <cgzones@googlemail.com> > > Validate that the permission maps in the scope index refer to a valid > class datum. Otherwise since commit 52e5c306 ("libsepol: move unchanged > data out of loop") this can lead to a NULL dereference in the class > existence check during linking. > > Reported-by: oss-fuzz (issue 69655) > Signed-off-by: Christian Göttsche <cgzones@googlemail.com> Acked-by: James Carter <jwcart2@gmail.com> > --- > libsepol/src/policydb_validate.c | 8 ++++++-- > 1 file changed, 6 insertions(+), 2 deletions(-) > > diff --git a/libsepol/src/policydb_validate.c b/libsepol/src/policydb_validate.c > index 9746f562..0216410c 100644 > --- a/libsepol/src/policydb_validate.c > +++ b/libsepol/src/policydb_validate.c > @@ -1467,6 +1467,8 @@ bad: > > static int validate_scope_index(sepol_handle_t *handle, const scope_index_t *scope_index, validate_t flavors[]) > { > + uint32_t i; > + > if (!ebitmap_is_empty(&scope_index->scope[SYM_COMMONS])) > goto bad; > if (validate_ebitmap(&scope_index->p_classes_scope, &flavors[SYM_CLASSES])) > @@ -1483,8 +1485,10 @@ static int validate_scope_index(sepol_handle_t *handle, const scope_index_t *sco > goto bad; > if (validate_ebitmap(&scope_index->p_cat_scope, &flavors[SYM_CATS])) > goto bad; > - if (scope_index->class_perms_len > flavors[SYM_CLASSES].nprim) > - goto bad; > + > + for (i = 0; i < scope_index->class_perms_len; i++) > + if (validate_value(i + 1, &flavors[SYM_CLASSES])) > + goto bad; > > return 0; > > -- > 2.45.1 > >
On Sun, Jun 16, 2024 at 2:44 PM Christian Göttsche <cgoettsche@seltendoof.de> wrote: > > > From: Christian Göttsche <cgzones@googlemail.com> > > > > Validate that the permission maps in the scope index refer to a valid > > class datum. Otherwise since commit 52e5c306 ("libsepol: move > > unchanged > > data out of loop") this can lead to a NULL dereference in the class > > existence check during linking. > > > > Reported-by: oss-fuzz (issue 69655) > > Signed-off-by: Christian Göttsche <cgzones@googlemail.com> > > --- > > libsepol/src/policydb_validate.c | 8 ++++++-- > > 1 file changed, 6 insertions(+), 2 deletions(-) > > > > diff --git a/libsepol/src/policydb_validate.c > > b/libsepol/src/policydb_validate.c > > index 9746f562..0216410c 100644 > > --- a/libsepol/src/policydb_validate.c > > +++ b/libsepol/src/policydb_validate.c > > @@ -1467,6 +1467,8 @@ bad: > > > > static int validate_scope_index(sepol_handle_t *handle, const > > scope_index_t *scope_index, validate_t flavors[]) > > { > > + uint32_t i; > > + > > if (!ebitmap_is_empty(&scope_index->scope[SYM_COMMONS])) > > goto bad; > > if (validate_ebitmap(&scope_index->p_classes_scope, > > &flavors[SYM_CLASSES])) > > @@ -1483,8 +1485,10 @@ static int validate_scope_index(sepol_handle_t > > *handle, const scope_index_t *sco > > goto bad; > > if (validate_ebitmap(&scope_index->p_cat_scope, > > &flavors[SYM_CATS])) > > goto bad; > > - if (scope_index->class_perms_len > flavors[SYM_CLASSES].nprim) > > Since classes should not have gaps, this could be a simple off-by-one, > cause class_perms_len is 0-based while nprim is 1-based. So > > scope_index->class_perms_len >= flavors[SYM_CLASSES].nprim > > might suffice, but I am unable to test currently though. > I did some testing and the above does not suffice. In the fuzzer testcase, nprim = 32 and class_perms_len = 1, but p_class_val_to_name[0] is NULL. The patch you sent works. Thanks, Jim > > - goto bad; > > + > > + for (i = 0; i < scope_index->class_perms_len; i++) > > + if (validate_value(i + 1, &flavors[SYM_CLASSES])) > > + goto bad; > > > > return 0; >
On Mon, Jun 17, 2024 at 1:55 PM James Carter <jwcart2@gmail.com> wrote: > > On Sat, Jun 15, 2024 at 9:25 AM Christian Göttsche > <cgoettsche@seltendoof.de> wrote: > > > > From: Christian Göttsche <cgzones@googlemail.com> > > > > Validate that the permission maps in the scope index refer to a valid > > class datum. Otherwise since commit 52e5c306 ("libsepol: move unchanged > > data out of loop") this can lead to a NULL dereference in the class > > existence check during linking. > > > > Reported-by: oss-fuzz (issue 69655) > > Signed-off-by: Christian Göttsche <cgzones@googlemail.com> > > Acked-by: James Carter <jwcart2@gmail.com> > Merged. Thanks, Jim > > --- > > libsepol/src/policydb_validate.c | 8 ++++++-- > > 1 file changed, 6 insertions(+), 2 deletions(-) > > > > diff --git a/libsepol/src/policydb_validate.c b/libsepol/src/policydb_validate.c > > index 9746f562..0216410c 100644 > > --- a/libsepol/src/policydb_validate.c > > +++ b/libsepol/src/policydb_validate.c > > @@ -1467,6 +1467,8 @@ bad: > > > > static int validate_scope_index(sepol_handle_t *handle, const scope_index_t *scope_index, validate_t flavors[]) > > { > > + uint32_t i; > > + > > if (!ebitmap_is_empty(&scope_index->scope[SYM_COMMONS])) > > goto bad; > > if (validate_ebitmap(&scope_index->p_classes_scope, &flavors[SYM_CLASSES])) > > @@ -1483,8 +1485,10 @@ static int validate_scope_index(sepol_handle_t *handle, const scope_index_t *sco > > goto bad; > > if (validate_ebitmap(&scope_index->p_cat_scope, &flavors[SYM_CATS])) > > goto bad; > > - if (scope_index->class_perms_len > flavors[SYM_CLASSES].nprim) > > - goto bad; > > + > > + for (i = 0; i < scope_index->class_perms_len; i++) > > + if (validate_value(i + 1, &flavors[SYM_CLASSES])) > > + goto bad; > > > > return 0; > > > > -- > > 2.45.1 > > > >
diff --git a/libsepol/src/policydb_validate.c b/libsepol/src/policydb_validate.c index 9746f562..0216410c 100644 --- a/libsepol/src/policydb_validate.c +++ b/libsepol/src/policydb_validate.c @@ -1467,6 +1467,8 @@ bad: static int validate_scope_index(sepol_handle_t *handle, const scope_index_t *scope_index, validate_t flavors[]) { + uint32_t i; + if (!ebitmap_is_empty(&scope_index->scope[SYM_COMMONS])) goto bad; if (validate_ebitmap(&scope_index->p_classes_scope, &flavors[SYM_CLASSES])) @@ -1483,8 +1485,10 @@ static int validate_scope_index(sepol_handle_t *handle, const scope_index_t *sco goto bad; if (validate_ebitmap(&scope_index->p_cat_scope, &flavors[SYM_CATS])) goto bad; - if (scope_index->class_perms_len > flavors[SYM_CLASSES].nprim) - goto bad; + + for (i = 0; i < scope_index->class_perms_len; i++) + if (validate_value(i + 1, &flavors[SYM_CLASSES])) + goto bad; return 0;