On Mon, Oct 11, 2021 at 12:41 PM Christian Göttsche
<cgzones@xxxxxxxxxxxxxx> wrote:
>
> ==80903==ERROR: AddressSanitizer: heap-buffer-overflow on address 0x6020000005c0 at pc 0x0000005696c8 bp 0x7ffdb11ea560 sp 0x7ffdb11ea558
> READ of size 8 at 0x6020000005c0 thread T0
> #0 0x5696c7 in avtab_node_to_str ./libsepol/src/kernel_to_conf.c:1736:9
> #1 0x569013 in map_avtab_write_helper ./libsepol/src/kernel_to_conf.c:1767:10
> #2 0x5ab837 in avtab_map ./libsepol/src/avtab.c:347:10
> #3 0x561f9a in write_avtab_flavor_to_conf ./libsepol/src/kernel_to_conf.c:1798:7
> #4 0x561f9a in write_avtab_to_conf ./libsepol/src/kernel_to_conf.c:1819:8
> #5 0x55afba in sepol_kernel_policydb_to_conf ./libsepol/src/kernel_to_conf.c:3159:7
> #6 0x55a34f in LLVMFuzzerTestOneInput ./libsepol/fuzz/binpolicy-fuzzer.c:38:9
> #7 0x45aed3 in fuzzer::Fuzzer::ExecuteCallback(unsigned char const*, unsigned long) fuzzer.o
> #8 0x446a12 in fuzzer::RunOneTest(fuzzer::Fuzzer*, char const*, unsigned long) fuzzer.o
> #9 0x44c93b in fuzzer::FuzzerDriver(int*, char***, int (*)(unsigned char const*, unsigned long)) fuzzer.o
> #10 0x475dd2 in main (./out/binpolicy-fuzzer+0x475dd2)
> #11 0x7f97a83fd7ec in __libc_start_main csu/../csu/libc-start.c:332:16
> #12 0x423689 in _start (./out/binpolicy-fuzzer+0x423689)
>
> Signed-off-by: Christian Göttsche <cgzones@xxxxxxxxxxxxxx>
> ---
> libsepol/src/policydb_validate.c | 17 ++++++++++++-----
> 1 file changed, 12 insertions(+), 5 deletions(-)
>
> diff --git a/libsepol/src/policydb_validate.c b/libsepol/src/policydb_validate.c
> index f0456583..9134e541 100644
> --- a/libsepol/src/policydb_validate.c
> +++ b/libsepol/src/policydb_validate.c
> @@ -505,15 +505,22 @@ bad:
> return -1;
> }
>
> -static int validate_avtab_key_wrapper(avtab_key_t *k, __attribute__ ((unused)) avtab_datum_t *d, void *args)
> +static int validate_avtab(avtab_key_t *k, avtab_datum_t *d, void *args)
> {
I don't like the function name (not that I liked the old one either).
I think validate_avtab_key_and_datum() would be better.
> validate_t *flavors = (validate_t *)args;
> - return validate_avtab_key(k, flavors);
> +
> + if (validate_avtab_key(k, flavors))
> + return -1;
> +
> + if ((k->specified & AVTAB_TYPE) && validate_value(d->data, &flavors[SYM_TYPES]))
> + return -1;
> +
> + return 0;
> }
>
> -static int validate_avtab(sepol_handle_t *handle, avtab_t *avtab, validate_t flavors[])
> +static int validate_avtabs(sepol_handle_t *handle, avtab_t *avtab, validate_t flavors[])
I don't like this function name at all, because only one avtab is
being checked. With the name change for the function above, you can
leave this as validate_avtab().
Everything else about this patch looks good.
Thanks,
Jim
> {
> - if (avtab_map(avtab, validate_avtab_key_wrapper, flavors)) {
> + if (avtab_map(avtab, validate_avtab, flavors)) {
> ERR(handle, "Invalid avtab");
> return -1;
> }
> @@ -845,7 +852,7 @@ int validate_policydb(sepol_handle_t *handle, policydb_t *p)
> goto bad;
>
> if (p->policy_type == POLICY_KERN) {
> - if (validate_avtab(handle, &p->te_avtab, flavors))
> + if (validate_avtabs(handle, &p->te_avtab, flavors))
> goto bad;
> if (p->policyvers >= POLICYDB_VERSION_BOOL)
> if (validate_cond_list(handle, p->cond_list, flavors))
> --
> 2.33.0
>
