diff mbox series

libsepol: check scope permissions refer to valid class

Message ID 20240615132525.36358-1-cgoettsche@seltendoof.de (mailing list archive)
State New
Delegated to: Petr Lautrbach
Headers show
Series libsepol: check scope permissions refer to valid class | expand

Commit Message

Christian Göttsche June 15, 2024, 1:25 p.m. UTC
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(-)

Comments

Christian Göttsche June 16, 2024, 6:44 p.m. UTC | #1
> 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;
James Carter June 17, 2024, 5:55 p.m. UTC | #2
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
>
>
James Carter June 17, 2024, 6:02 p.m. UTC | #3
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;
>
James Carter June 21, 2024, 6:08 p.m. UTC | #4
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 mbox series

Patch

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;