diff --git a/devops/ci.yml b/devops/ci.yml index 618554f21..1fac4091c 100644 --- a/devops/ci.yml +++ b/devops/ci.yml @@ -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 diff --git a/experiments/process_sandbox/include/process_sandbox/sandbox.h b/experiments/process_sandbox/include/process_sandbox/sandbox.h index e9d952a4e..e3620308d 100644 --- a/experiments/process_sandbox/include/process_sandbox/sandbox.h +++ b/experiments/process_sandbox/include/process_sandbox/sandbox.h @@ -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 @@ -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; + using Entry = SandboxMetaEntry; private: /** diff --git a/experiments/process_sandbox/include/process_sandbox/sandbox_meta_entry.h b/experiments/process_sandbox/include/process_sandbox/sandbox_meta_entry.h new file mode 100644 index 000000000..f620d614b --- /dev/null +++ b/experiments/process_sandbox/include/process_sandbox/sandbox_meta_entry.h @@ -0,0 +1,60 @@ +// Copyright Microsoft and Project Verona Contributors. +// SPDX-License-Identifier: MIT + +#pragma once + +#include + +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 + { + /** + * 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(m); + } + }; +} diff --git a/experiments/process_sandbox/src/child_malloc.h b/experiments/process_sandbox/src/child_malloc.h index 8b6516e96..c532f8b8d 100644 --- a/experiments/process_sandbox/src/child_malloc.h +++ b/experiments/process_sandbox/src/child_malloc.h @@ -9,6 +9,7 @@ #define SNMALLOC_PROVIDE_OWN_CONFIG 1 #include #include +#include #include namespace snmalloc @@ -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; + using Entry = SandboxMetaEntry; /** * The pagemap that spans the entire address space. This uses a read-only diff --git a/experiments/process_sandbox/src/libsandbox.cc b/experiments/process_sandbox/src/libsandbox.cc index 953bb431a..bcb8bebf5 100644 --- a/experiments/process_sandbox/src/libsandbox.cc +++ b/experiments/process_sandbox/src/libsandbox.cc @@ -195,6 +195,7 @@ namespace sandbox reply.error = 2; break; } + metaentry.claim_for_sandbox(); SharedAllocConfig::Pagemap::set_metaentry( address_cast(alloc), size, metaentry); @@ -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; } } @@ -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(); diff --git a/experiments/process_sandbox/tests/sandbox-rpc-bounds.cc b/experiments/process_sandbox/tests/sandbox-rpc-bounds.cc index de4d56926..e0fb9562b 100644 --- a/experiments/process_sandbox/tests/sandbox-rpc-bounds.cc +++ b/experiments/process_sandbox/tests/sandbox-rpc-bounds.cc @@ -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. @@ -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; } diff --git a/experiments/process_sandbox/tests/sandboxlib-rpc-bounds.cc b/experiments/process_sandbox/tests/sandboxlib-rpc-bounds.cc index 235c1ded2..9a4e7c776 100644 --- a/experiments/process_sandbox/tests/sandboxlib-rpc-bounds.cc +++ b/experiments/process_sandbox/tests/sandboxlib-rpc-bounds.cc @@ -10,6 +10,7 @@ #include using namespace sandbox; +using namespace snmalloc; HostServiceResponse sendRequest(HostServiceRequest req) { @@ -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::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::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"); @@ -74,11 +76,132 @@ bool attack(const void* base, const void* top) std::numeric_limits::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(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(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(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);