Re: [RFC PATCH v2 10/36] libsepol: add checks for read sizes

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On Tue, Nov 9, 2021 at 1:59 PM Christian Göttsche
<cgzones@xxxxxxxxxxxxxx> wrote:
>
> On Tue, 9 Nov 2021 at 19:46, James Carter <jwcart2@xxxxxxxxx> wrote:
> >
> > On Fri, Nov 5, 2021 at 12:11 PM Christian Göttsche
> > <cgzones@xxxxxxxxxxxxxx> wrote:
> > >
> > > Add checks for invalid read sizes from a binary policy to guard
> > > allocations.
> > >
> > > The common and class permission counts needs to be limited more strict
> > > otherwise a too high count of common or class permissions can lead to
> > > permission values with a too high value, which can lead to overflows
> > > in shift operations.
> > >
> > > In the fuzzer build the value will also be bounded to avoid oom reports.
> > >
> > >     ==29857== ERROR: libFuzzer: out-of-memory (malloc(17179868160))
> > >        To change the out-of-memory limit use -rss_limit_mb=<N>
> > >
> > >         #0 0x52dc61 in __sanitizer_print_stack_trace (./out/binpolicy-fuzzer+0x52dc61)
> > >         #1 0x475618 in fuzzer::PrintStackTrace() fuzzer.o
> > >         #2 0x458855 in fuzzer::Fuzzer::HandleMalloc(unsigned long) fuzzer.o
> > >         #3 0x45876a in fuzzer::MallocHook(void const volatile*, unsigned long) fuzzer.o
> > >         #4 0x534557 in __sanitizer::RunMallocHooks(void const*, unsigned long) (./out/binpolicy-fuzzer+0x534557)
> > >         #5 0x4aa7d7 in __asan::Allocator::Allocate(unsigned long, unsigned long, __sanitizer::BufferedStackTrace*, __asan::AllocType, bool) (./out/binpolicy-fuzzer+0x4aa7d7)
> > >         #6 0x4aa143 in __asan::asan_malloc(unsigned long, __sanitizer::BufferedStackTrace*) (./out/binpolicy-fuzzer+0x4aa143)
> > >         #7 0x5259cb in malloc (./out/binpolicy-fuzzer+0x5259cb)
> > >         #8 0x580b5d in mallocarray ./libsepol/src/./private.h:93:9
> > >         #9 0x57c2ed in scope_read ./libsepol/src/policydb.c:4120:7
> > >         #10 0x576b0d in policydb_read ./libsepol/src/policydb.c:4462:9
> > >         #11 0x55a214 in LLVMFuzzerTestOneInput ./libsepol/fuzz/binpolicy-fuzzer.c:26:6
> > >         #12 0x45aed3 in fuzzer::Fuzzer::ExecuteCallback(unsigned char const*, unsigned long) fuzzer.o
> > >         #13 0x446a12 in fuzzer::RunOneTest(fuzzer::Fuzzer*, char const*, unsigned long) fuzzer.o
> > >         #14 0x44c93b in fuzzer::FuzzerDriver(int*, char***, int (*)(unsigned char const*, unsigned long)) fuzzer.o
> > >         #15 0x475dd2 in main (./out/binpolicy-fuzzer+0x475dd2)
> > >         #16 0x7ffad6e107ec in __libc_start_main csu/../csu/libc-start.c:332:16
> > >         #17 0x423689 in _start (./out/binpolicy-fuzzer+0x423689)
> > >
> > >     ==19462== ERROR: libFuzzer: out-of-memory (malloc(18253611008))
> > >        To change the out-of-memory limit use -rss_limit_mb=<N>
> > >
> > >         #0 0x52dc61 in __sanitizer_print_stack_trace (./out/binpolicy-fuzzer+0x52dc61)
> > >         #1 0x475618 in fuzzer::PrintStackTrace() fuzzer.o
> > >         #2 0x458855 in fuzzer::Fuzzer::HandleMalloc(unsigned long) fuzzer.o
> > >         #3 0x45876a in fuzzer::MallocHook(void const volatile*, unsigned long) fuzzer.o
> > >         #4 0x534557 in __sanitizer::RunMallocHooks(void const*, unsigned long) (./out/binpolicy-fuzzer+0x534557)
> > >         #5 0x4aa7d7 in __asan::Allocator::Allocate(unsigned long, unsigned long, __sanitizer::BufferedStackTrace*, __asan::AllocType, bool) (./out/binpolicy-fuzzer+0x4aa7d7)
> > >         #6 0x4aa999 in __asan::asan_calloc(unsigned long, unsigned long, __sanitizer::BufferedStackTrace*) (./out/binpolicy-fuzzer+0x4aa999)
> > >         #7 0x525b63 in __interceptor_calloc (./out/binpolicy-fuzzer+0x525b63)
> > >         #8 0x570938 in policydb_index_others ./libsepol/src/policydb.c:1245:6
> > >         #9 0x5771f3 in policydb_read ./src/policydb.c:4481:6
> > >         #10 0x55a214 in LLVMFuzzerTestOneInput ./libsepol/fuzz/binpolicy-fuzzer.c:26:6
> > >         #11 0x45aed3 in fuzzer::Fuzzer::ExecuteCallback(unsigned char const*, unsigned long) fuzzer.o
> > >         #12 0x446a12 in fuzzer::RunOneTest(fuzzer::Fuzzer*, char const*, unsigned long) fuzzer.o
> > >         #13 0x44c93b in fuzzer::FuzzerDriver(int*, char***, int (*)(unsigned char const*, unsigned long)) fuzzer.o
> > >         #14 0x475dd2 in main (./out/binpolicy-fuzzer+0x475dd2)
> > >         #15 0x7f4d933157ec in __libc_start_main csu/../csu/libc-start.c:332:16
> > >         #16 0x423689 in _start (./out/binpolicy-fuzzer+0x423689)
> > >
> > > Signed-off-by: Christian Göttsche <cgzones@xxxxxxxxxxxxxx>
> > > ---
> > >  libsepol/src/policydb.c | 10 +++++++++-
> > >  1 file changed, 9 insertions(+), 1 deletion(-)
> > >
> > > diff --git a/libsepol/src/policydb.c b/libsepol/src/policydb.c
> > > index dcea1807..1408405d 100644
> > > --- a/libsepol/src/policydb.c
> > > +++ b/libsepol/src/policydb.c
> > > @@ -2103,6 +2103,8 @@ static int common_read(policydb_t * p, hashtab_t h, struct policy_file *fp)
> > >         if (symtab_init(&comdatum->permissions, PERM_SYMTAB_SIZE))
> > >                 goto bad;
> > >         comdatum->permissions.nprim = le32_to_cpu(buf[2]);
> > > +       if (comdatum->permissions.nprim > 32)
> >
> > Should use PERM_SYMTAB_SIZE here (like in patch 22).
>
> I thought about that, but it seemed a bit unrelated.
> In patch 25 (https://patchwork.kernel.org/project/selinux/patch/20211105154542.38434-26-cgzones@xxxxxxxxxxxxxx/)
> the number of permissions in commons and classes is checked against
> the logical limit (which is imposed by the implementation via a bitset
> on a 32 bit integer).
> The check here is to prevent overflows on subsequent left-shifts by
> the checked value on a 32 bit integer.
> Maybe `sizeof(uint32_t)` is more expressive?
> However I do not object your comment, so I fine to change to PERM_SYMTAB_SIZE.
>

I think that you make a good point, but I think that PERM_SYMTAB_SIZE,
while not perfect, is the best choice.

Thanks,
Jim


> > > +               goto bad;
> > >         nel = le32_to_cpu(buf[3]);
> > >
> > >         key = malloc(len + 1);
> > > @@ -2251,6 +2253,8 @@ static int class_read(policydb_t * p, hashtab_t h, struct policy_file *fp)
> > >         if (symtab_init(&cladatum->permissions, PERM_SYMTAB_SIZE))
> > >                 goto bad;
> > >         cladatum->permissions.nprim = le32_to_cpu(buf[3]);
> > > +       if (cladatum->permissions.nprim > 32)
> >
> > Same here.
> >
> >
> > > +               goto bad;
> > >         nel = le32_to_cpu(buf[4]);
> > >
> > >         ncons = le32_to_cpu(buf[5]);
> > > @@ -3980,6 +3984,8 @@ static int avrule_decl_read(policydb_t * p, avrule_decl_t * decl,
> > >                 if (rc < 0)
> > >                         return -1;
> > >                 nprim = le32_to_cpu(buf[0]);
> > > +               if (is_saturated(nprim))
> > > +                       return -1;
> > >                 nel = le32_to_cpu(buf[1]);
> > >                 for (j = 0; j < nel; j++) {
> > >                         if (read_f[i] (p, decl->symtab[i].table, fp)) {
> > > @@ -4106,7 +4112,7 @@ static int scope_read(policydb_t * p, int symnum, struct policy_file *fp)
> > >                 goto cleanup;
> > >         scope->scope = le32_to_cpu(buf[0]);
> > >         scope->decl_ids_len = le32_to_cpu(buf[1]);
> > > -       if (scope->decl_ids_len == 0) {
> > > +       if (zero_or_saturated(scope->decl_ids_len)) {
> > >                 ERR(fp->handle, "invalid scope with no declaration");
> > >                 goto cleanup;
> > >         }
> > > @@ -4396,6 +4402,8 @@ int policydb_read(policydb_t * p, struct policy_file *fp, unsigned verbose)
> > >                 if (rc < 0)
> > >                         goto bad;
> > >                 nprim = le32_to_cpu(buf[0]);
> > > +               if (is_saturated(nprim))
> > > +                       goto bad;
> > >                 nel = le32_to_cpu(buf[1]);
> > >                 if (nel && !nprim) {
> > >                         ERR(fp->handle, "unexpected items in symbol table with no symbol");
> > > --
> > > 2.33.1
> > >




[Index of Archives]     [Selinux Refpolicy]     [Linux SGX]     [Fedora Users]     [Fedora Desktop]     [Yosemite Photos]     [Yosemite Camping]     [Yosemite Campsites]     [KDE Users]     [Gnome Users]

  Powered by Linux