On Mon, Oct 11, 2021 at 12:41 PM Christian Göttsche
<cgzones@xxxxxxxxxxxxxx> wrote:
>
> Validate the level map of the policy to ensure no level refers to a non
> existent category.
>
> READ of size 8 at 0x602000000c58 thread T0
> #0 0x568d2c in cats_ebitmap_len ./libsepol/src/kernel_to_conf.c:1003:14
> #1 0x568d2c in cats_ebitmap_to_str ./libsepol/src/kernel_to_conf.c:1038:19
> #2 0x55e371 in write_level_rules_to_conf ./libsepol/src/kernel_to_conf.c:1106:11
> #3 0x55e371 in write_mls_rules_to_conf ./libsepol/src/kernel_to_conf.c:1140:7
> #4 0x55adb1 in sepol_kernel_policydb_to_conf ./libsepol/src/kernel_to_conf.c:3103:7
> #5 0x55a34f in LLVMFuzzerTestOneInput ./libsepol/fuzz/binpolicy-fuzzer.c:38:9
> #6 0x45aed3 in fuzzer::Fuzzer::ExecuteCallback(unsigned char const*, unsigned long) fuzzer.o
> #7 0x446a12 in fuzzer::RunOneTest(fuzzer::Fuzzer*, char const*, unsigned long) fuzzer.o
> #8 0x44c93b in fuzzer::FuzzerDriver(int*, char***, int (*)(unsigned char const*, unsigned long)) fuzzer.o
> #9 0x475dd2 in main (./out/binpolicy-fuzzer+0x475dd2)
> #10 0x7f741d0d67ec in __libc_start_main csu/../csu/libc-start.c:332:16
> #11 0x423689 in _start (./out/binpolicy-fuzzer+0x423689)
>
> Signed-off-by: Christian Göttsche <cgzones@xxxxxxxxxxxxxx>
> ---
> libsepol/src/policydb_validate.c | 26 ++++++++++++++++++++++++++
> 1 file changed, 26 insertions(+)
>
> diff --git a/libsepol/src/policydb_validate.c b/libsepol/src/policydb_validate.c
> index 5804d247..ca0dcca3 100644
> --- a/libsepol/src/policydb_validate.c
> +++ b/libsepol/src/policydb_validate.c
> @@ -310,6 +310,29 @@ bad:
> return -1;
> }
>
> +static int validate_mls_level(mls_level_t *level, validate_t *sens, validate_t *cats)
> +{
> + if (level->sens == 0)
> + return 0;
> + if (validate_value(level->sens, sens))
> + goto bad;
> + if (validate_ebitmap(&level->cat, cats))
> + goto bad;
> +
> + return 0;
> +
> + bad:
> + return -1;
> +}
> +
> +static int validate_level(__attribute__ ((unused))hashtab_key_t k, hashtab_datum_t d, void *args)
I would prefer validate_level_datum for the name of this function.
> +{
> + level_datum_t *level = d;
> + validate_t *flavors = args;
> +
> + return validate_mls_level(level->level, &flavors[SYM_LEVELS], &flavors[SYM_CATS]);
> +}
> +
> static int validate_datum_arrays(sepol_handle_t *handle, policydb_t *p, validate_t flavors[])
> {
> unsigned int i;
> @@ -368,6 +391,9 @@ static int validate_datum_arrays(sepol_handle_t *handle, policydb_t *p, validate
> }
> }
>
> + if (hashtab_map(p->p_levels.table, validate_level, flavors))
> + goto bad;
> +
This should not be in validate_datum_arrays(). This also applies to
patches 17 and 33.
I think the best course would be to pull out the validate_X_datum()
calls out of validate_datum_arrays() so that the only thing being
checked is that the gaps in the X_val_to_struct arrays correspond to
any gaps in the X_val_to_name arrays.
Have a function called validate_global_symtabs() or something that
walks all of the symtabs and does the checks on the datums (including
aliases). So this hashtab_map() call would be in the function, along
with calls to hashtab_map using modified validate_X_datum() functions
that can be used with hashtab_map().
I think something like:
if (hashtab_map(p->p_commons.table, validate_common_datum, flavors))
goto bad;
if (hashtab_map(p->p_classes.table, validate_class_datum, flavors))
goto bad;
if (hashtab_map(p->p_roles.table, validate_role_datum, flavors))
goto bad;
if (hashtab_map(p->p_types.table, validate_type_datum, flavors))
goto bad;
if (hashtab_map(p->p_users.table, validate_user_datum, flavors))
goto bad;
if (hashtab_map(p->p_bools.table, validate_bool_datum, flavors))
goto bad;
if (hashtab_map(p->p_levels.table, validate_level_datum, flavors))
goto bad;
if (hashtab_map(p->p_cats.table, validate_cat_datum, flavors))
goto bad;
Thanks,
Jim
