Skip to content

Commit

Permalink
Browse files Browse the repository at this point in the history
Add more validation for DeallocChunk messages
Fixes #574
Fixes #575
Fixes #576

Also set timeout on tests.
They all currently complete in <1s, so a 30s timeout is plenty.
  • Loading branch information
davidchisnall committed Apr 29, 2022
1 parent dbd6da4 commit b75b96b
Show file tree
Hide file tree
Showing 7 changed files with 253 additions and 22 deletions.
2 changes: 1 addition & 1 deletion devops/ci.yml
Expand Up @@ -209,7 +209,7 @@ jobs:
displayName: 'Compile'
- script: |
ctest -j 4 --output-on-failure -T test
ctest -j 4 --output-on-failure -T test --timeout 30
displayName: 'Test'
workingDirectory: build
failOnStderr: false
Expand Down
6 changes: 3 additions & 3 deletions experiments/process_sandbox/include/process_sandbox/sandbox.h
Expand Up @@ -19,6 +19,7 @@
#include "netpolicy.h"
#include "platform/platform.h"
#include "sandbox_fd_numbers.h"
#include "sandbox_meta_entry.h"
#include "shared_memory_region.h"

namespace sandbox
Expand Down Expand Up @@ -68,10 +69,9 @@ namespace sandbox
{
public:
/**
* Pagemap entry. This back end does not store anything here, so expose
* the default.
* Pagemap entry.
*/
using Entry = snmalloc::FrontendMetaEntry<SlabMetadata>;
using Entry = SandboxMetaEntry;

private:
/**
Expand Down
@@ -0,0 +1,60 @@
// Copyright Microsoft and Project Verona Contributors.
// SPDX-License-Identifier: MIT

#pragma once

#include <snmalloc/snmalloc_core.h>

namespace sandbox
{
/**
* Pagemap entry. Extends the front-end's version to use one bit to identify
* pagemap entries as owned by the child.
*/
class SandboxMetaEntry
: public snmalloc::FrontendMetaEntry<snmalloc::FrontendSlabMetadata>
{
/**
* Bit set if this metaentry is owned by the sandbox.
*/
static constexpr snmalloc::address_t SANDBOX_BIT = 1 << 3;

public:
/**
* Inherit all constructors.
*/
using snmalloc::FrontendMetaEntry<
snmalloc::FrontendSlabMetadata>::FrontendMetaEntry;

/**
* Does this metaentry correspond to sandbox-owned memory
*/
bool is_sandbox_owned() const
{
return (meta & SANDBOX_BIT) == SANDBOX_BIT;
}

/**
* Claim this entry for the sandbox.
*/
void claim_for_sandbox()
{
meta |= SANDBOX_BIT;
}

[[nodiscard]] bool is_unowned() const
{
auto m = meta & ~SANDBOX_BIT;
return ((m == 0) || (m == META_BOUNDARY_BIT)) &&
(remote_and_sizeclass == 0);
}

[[nodiscard]] SNMALLOC_FAST_PATH snmalloc::FrontendSlabMetadata*
get_slab_metadata() const
{
SNMALLOC_ASSERT(get_remote() != nullptr);
auto m = meta & ~(SANDBOX_BIT | META_BOUNDARY_BIT);
return snmalloc::unsafe_from_uintptr<snmalloc::FrontendSlabMetadata>(m);
}
};
}
3 changes: 2 additions & 1 deletion experiments/process_sandbox/src/child_malloc.h
Expand Up @@ -9,6 +9,7 @@
#define SNMALLOC_PROVIDE_OWN_CONFIG 1
#include <process_sandbox/helpers.h>
#include <process_sandbox/sandbox_fd_numbers.h>
#include <process_sandbox/sandbox_meta_entry.h>
#include <snmalloc/snmalloc_core.h>

namespace snmalloc
Expand Down Expand Up @@ -117,7 +118,7 @@ namespace sandbox
* This back end does not need to hold any extra metadata and so exports
* the default pagemap metadata type.
*/
using Entry = snmalloc::FrontendMetaEntry<SlabMetadata>;
using Entry = SandboxMetaEntry;

/**
* The pagemap that spans the entire address space. This uses a read-only
Expand Down
40 changes: 36 additions & 4 deletions experiments/process_sandbox/src/libsandbox.cc
Expand Up @@ -195,6 +195,7 @@ namespace sandbox
reply.error = 2;
break;
}
metaentry.claim_for_sandbox();
SharedAllocConfig::Pagemap::set_metaentry(
address_cast(alloc), size, metaentry);

Expand All @@ -211,7 +212,37 @@ namespace sandbox
reply.error = 1;
break;
}
SharedAllocConfig::dealloc_range(*s, ptr, size);
// The size must be a power of two, larger than the chunk size
if (!(snmalloc::bits::is_pow2(size) &&
(size >= snmalloc::MIN_CHUNK_SIZE)))
{
reply.error = 2;
break;
}
// The base must be chunk-aligned
if (
snmalloc::pointer_align_down(
ptr.unsafe_ptr(), snmalloc::MIN_CHUNK_SIZE) != ptr.unsafe_ptr())
{
reply.error = 3;
break;
}
auto address = snmalloc::address_cast(ptr);
for (size_t chunk_offset = 0; chunk_offset < size;
chunk_offset += snmalloc::MIN_CHUNK_SIZE)
{
auto& meta = SharedAllocConfig::Pagemap::get_metaentry_mut(
address + chunk_offset);
if (!meta.is_sandbox_owned())
{
reply.error = 4;
break;
}
}
if (reply.error == 0)
{
SharedAllocConfig::dealloc_range(*s, ptr, size);
}
break;
}
}
Expand Down Expand Up @@ -741,14 +772,15 @@ namespace sandbox
if (!meta.is_backend_owned())
{
auto* remote = meta.get_remote();
if (
(remote != nullptr) &&
!contains(remote, sizeof(snmalloc::RemoteAllocator)))
if (!meta.is_sandbox_owned() && (remote != nullptr))
{
delete meta.get_slab_metadata();
}
}
meta = empty;
SANDBOX_DEBUG_INVARIANT(
!meta.is_sandbox_owned(),
"Unused pagemap entry must not be sandbox owned");
}
}
shared_mem->destroy();
Expand Down
23 changes: 19 additions & 4 deletions experiments/process_sandbox/tests/sandbox-rpc-bounds.cc
Expand Up @@ -9,7 +9,7 @@

using namespace sandbox;

bool attack(const void*, const void*);
bool attack(int, const void*, const void*);

/**
* The structure that represents an instance of the sandbox.
Expand All @@ -26,11 +26,26 @@ struct EvilSandbox
EXPORTED_FUNCTION(attack, ::attack)
};

int main()
void try_attack(int issue)
{
EvilSandbox sandbox;
auto [base, top] = sandbox.lib.sandbox_heap();
SANDBOX_INVARIANT(
sandbox.attack(base, top) == false, "Sandbox attack succeeded!");
try
{
SANDBOX_INVARIANT(
sandbox.attack(issue, base, top) == false,
"Sandbox attack {} succeeded!",
issue);
}
catch (...)
{}
}

int main()
{
try_attack(565);
try_attack(574);
try_attack(575);
try_attack(576);
return 0;
}
141 changes: 132 additions & 9 deletions experiments/process_sandbox/tests/sandboxlib-rpc-bounds.cc
Expand Up @@ -10,6 +10,7 @@
#include <stdio.h>

using namespace sandbox;
using namespace snmalloc;

HostServiceResponse sendRequest(HostServiceRequest req)
{
Expand Down Expand Up @@ -44,18 +45,19 @@ void try_alloc(const void* base)
SANDBOX_INVARIANT(
sendRequest({AllocChunk, {0x100, 0, 0}}).error != 0,
"Allocating less than a chunk spuriously returned success");
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::message<>("Trying to allocate parent-owned memory");
uintptr_t ras = FrontendMetaEntry<FrontendSlabMetadata>::encode(
&header->allocator_state,
sizeclass_t::from_small_class(size_to_sizeclass_const(MIN_CHUNK_SIZE)));
message<>("Trying to allocate parent-owned memory");
SANDBOX_INVARIANT(
sendRequest({AllocChunk, {snmalloc::MIN_CHUNK_SIZE, 4096, ras}}).error != 0,
sendRequest({AllocChunk, {MIN_CHUNK_SIZE, 4096, ras}}).error != 0,
"Allocating a chunk owned by the parent spuriously returned success");
}

bool attack(const void* base, const void* top)
/**
* Attack from GitHub issue 565
*/
bool attack565(const void* base, const void* top)
{
try_alloc(base);
SANDBOX_INVARIANT(try_dealloc(nullptr, -1) != 0, "Trying to dealloc nullptr");
Expand All @@ -74,11 +76,132 @@ bool attack(const void* base, const void* top)
std::numeric_limits<size_t>::max() - 100) != 0,
"Trying to dealloc with overflow from the top");
#else
snmalloc::UNUSED(top);
UNUSED(top);
#endif
return false;
}

bool attack574(const void*, const void*)
{
int error = 0;
auto resp = sendRequest({AllocChunk, {MIN_CHUNK_SIZE, 0, 0}});
if (resp.error != 0)
{
message("failed, error == {}", resp.error);
// Allocation should not fail here
return true;
}
char* p = reinterpret_cast<char*>(resp.ret);
// parent segfault here before sending a valid resp.
// p contains a pointer to a middle of some chunk, and we can use this address
// of course, instead of doing so, we can simply free p+0x100, and we get a
// similar segfault
message("Freeing {} + 0x100", p);
error = try_dealloc(p + 0x100, snmalloc::MIN_CHUNK_SIZE);
if (error == 0)
{
message("Freeing misaligned chunk succeeded");
return true;
}
message("Dealloc failed, error == {}", error);
return false;
}

bool attack575(const void*, const void*)
{
int error = 0;
auto resp = sendRequest({AllocChunk, {MIN_CHUNK_SIZE, 0, 0}});
if (resp.error != 0)
{
message("failed, error == {}", resp.error);
// Allocation should not fail here
return true;
}
char* p = reinterpret_cast<char*>(resp.ret);
message("Freeing {}", p);
error = try_dealloc(p, snmalloc::MIN_CHUNK_SIZE);
if (error != 0)
{
message("Freeing {} failed", p);
return true;
}
message("Freeing {}", p);
error = try_dealloc(p, snmalloc::MIN_CHUNK_SIZE);
if (error == 0)
{
message("Freeing {} for the second time succeeded", p);
return true;
}
return false;
}

bool attack576(const void*, const void*)
{
int error = 0;
auto resp = sendRequest({AllocChunk, {MIN_CHUNK_SIZE * 2, 0, 0}});
if (resp.error != 0)
{
message("failed, error == {}", resp.error);
// Allocation should not fail here
return true;
}
char* p = reinterpret_cast<char*>(resp.ret);
message("Allocated chunk: {} bytes from {}", MIN_CHUNK_SIZE * 2, p);
// parent segfault here before sending a valid resp.
// p contains a pointer to a middle of some chunk, and we can use this address
// of course, instead of doing so, we can simply free p+0x100, and we get a
// similar segfault
message("Freeing {} + 0x100", p);
message(
"Freeing too-small chunk, {} bytes from {}",
snmalloc::MIN_CHUNK_SIZE - 100,
p);
error = try_dealloc(p, snmalloc::MIN_CHUNK_SIZE - 100);
if (error == 0)
{
message("Freeing too-small chunk succeeded");
return true;
}
message("Dealloc failed, error == {}", error);
message("Freeing the second half of the allocation");
error = try_dealloc(p + snmalloc::MIN_CHUNK_SIZE, snmalloc::MIN_CHUNK_SIZE);
if (error != 0)
{
message("Freeing the second slab failed: {}", error);
return true;
}
message(
"Freeing too-large chunk, {} bytes from {}",
snmalloc::MIN_CHUNK_SIZE * 2,
p);
error = try_dealloc(p, snmalloc::MIN_CHUNK_SIZE * 2);
if (error == 0)
{
message("Freeing too-large chunk succeeded");
return true;
}
message("Dealloc failed, error == {}", error);
return false;
}

static bool attack(int issue, const void* base, const void* top)
{
switch (issue)
{
case 565:
return attack565(base, top);
case 574:
return attack574(base, top);
case 575:
return attack575(base, top);
case 576:
return attack576(base, top);
}
// Invalid PR number, pretend that the attack succeeded so that we hit an
// assert fail in the parent.
return true;
}

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

0 comments on commit b75b96b

Please sign in to comment.