Re: [PATCH 1/6] libsepol: do not decode out-of-bound rolebounds

From: James Carter <jwcart2@gmail.com>
To: Nicolas Iooss <nicolas.iooss@m4x.org>
Cc: SElinux list <selinux@vger.kernel.org>
Subject: Re: [PATCH 1/6] libsepol: do not decode out-of-bound rolebounds
Date: Mon, 4 Jan 2021 10:48:48 -0500	[thread overview]
Message-ID: <CAP+JOzQBQ--xZWukZcZTsZ1Eq01Fk4xCa66AotYCHKxZEG-QgQ@mail.gmail.com> (raw)
In-Reply-To: <20201230100746.2549568-1-nicolas.iooss@m4x.org>

[-- Attachment #1: Type: text/plain, Size: 1668 bytes --]

On Wed, Dec 30, 2020 at 5:11 AM Nicolas Iooss <nicolas.iooss@m4x.org> wrote:
>
> While fuzzing /usr/libexec/hll/pp, a policy module was generated with a
> role->bounds larger that the number of roles in the policy.
>
> This issue has been found while fuzzing hll/pp with the American Fuzzy
> Lop.
>
> Signed-off-by: Nicolas Iooss <nicolas.iooss@m4x.org>
> ---
>  libsepol/src/module_to_cil.c | 4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)
>
> diff --git a/libsepol/src/module_to_cil.c b/libsepol/src/module_to_cil.c
> index a87bc15e7610..c99790eb76e7 100644
> --- a/libsepol/src/module_to_cil.c
> +++ b/libsepol/src/module_to_cil.c
> @@ -2165,7 +2165,9 @@ static int role_to_cil(int indent, struct policydb *pdb, struct avrule_block *UN
>                         }
>                 }
>
> -               if (role->bounds > 0) {
> +               if (role->bounds >= pdb->p_roles.nprim) {
> +                       log_err("Warning: role %s defines an out-of-bound rolebounds", key);
> +               } else if (role->bounds > 0) {
>                         cil_println(indent, "(rolebounds %s %s)", key, pdb->p_role_val_to_name[role->bounds - 1]);
>                 }
>                 break;
> --
> 2.29.2
>

There are other places where the bounds value is used as an index and
type datums also have bounds that are used in the same way.

Correct me if I am wrong, but I think that this can only occur by
crafting a binary (and not as a result of a policy being compiled). So
I think the correct place for the check would be when the binary file
is read.

I'll have to test to be sure, but I think the attached patch might do
the proper checking.

Jim

[-- Attachment #2: bounds.patch --]
[-- Type: text/x-patch, Size: 909 bytes --]

diff --git a/libsepol/src/policydb.c b/libsepol/src/policydb.c
index ce8f3ad7..f8839539 100644
--- a/libsepol/src/policydb.c
+++ b/libsepol/src/policydb.c
@@ -1030,6 +1030,8 @@ static int role_index(hashtab_key_t key, hashtab_datum_t datum, void *datap)
 		return -EINVAL;
 	if (p->p_role_val_to_name[role->s.value - 1] != NULL)
 		return -EINVAL;
+	if (role->bounds > p->p_roles.nprim)
+		return _EINVAL;
 	p->p_role_val_to_name[role->s.value - 1] = (char *)key;
 	p->role_val_to_struct[role->s.value - 1] = role;
 
@@ -1049,6 +1051,8 @@ static int type_index(hashtab_key_t key, hashtab_datum_t datum, void *datap)
 			return -EINVAL;
 		if (p->p_type_val_to_name[typdatum->s.value - 1] != NULL)
 			return -EINVAL;
+		if (typedatum->bounds > p->p_types.nprim)
+			return -EINVAL;
 		p->p_type_val_to_name[typdatum->s.value - 1] = (char *)key;
 		p->type_val_to_struct[typdatum->s.value - 1] = typdatum;
 	}
