From 78a63930cca64e78a6ba7ed30d46ec8570dd3bde Mon Sep 17 00:00:00 2001 From: Andreas Kling Date: Mon, 6 Jan 2020 21:04:57 +0100 Subject: [PATCH] Kernel+LibELF: Validate PT_LOAD and PT_TLS offsets before memcpy()'ing Before this, you could make the kernel copy memory from anywhere by setting up an ELF executable with a program header specifying file offsets outside the file. Since ELFImage didn't even know how large it was, we had no clue that we were copying things from outside the ELF. Fix this by adding a size field to ELFImage and validating program header ranges before memcpy()'ing to them. The ELF code is definitely going to need more validation and checking. --- Kernel/Process.cpp | 11 ++++++++--- Libraries/LibELF/ELFDynamicLoader.cpp | 2 +- Libraries/LibELF/ELFImage.cpp | 5 ++++- Libraries/LibELF/ELFImage.h | 12 +++++++++++- Libraries/LibELF/ELFLoader.cpp | 14 ++++++++++++-- Libraries/LibELF/ELFLoader.h | 2 +- 6 files changed, 37 insertions(+), 9 deletions(-) diff --git a/Kernel/Process.cpp b/Kernel/Process.cpp index 431d0d21d8b..9b8bd908e30 100644 --- a/Kernel/Process.cpp +++ b/Kernel/Process.cpp @@ -49,6 +49,7 @@ //#define DEBUG_IO //#define TASK_DEBUG //#define FORK_DEBUG +//#define EXEC_DEBUG //#define SIGNAL_DEBUG //#define SHARED_BUFFER_DEBUG @@ -655,7 +656,7 @@ int Process::do_exec(String path, Vector arguments, Vector envir // Okay, here comes the sleight of hand, pay close attention.. auto old_regions = move(m_regions); m_regions.append(move(executable_region)); - loader = make(region->vaddr().as_ptr()); + loader = make(region->vaddr().as_ptr(), metadata.size); loader->map_section_hook = [&](VirtualAddress vaddr, size_t size, size_t alignment, size_t offset_in_image, bool is_readable, bool is_writable, bool is_executable, const String& name) -> u8* { ASSERT(size); ASSERT(alignment == PAGE_SIZE); @@ -703,6 +704,11 @@ int Process::do_exec(String path, Vector arguments, Vector envir // NOTE: At this point, we've committed to the new executable. entry_eip = loader->entry().get(); + +#ifdef EXEC_DEBUG + kprintf("Memory layout after ELF load:"); + dump_regions(); +#endif } region->set_user_accessible(false); @@ -3818,8 +3824,7 @@ int Process::sys$module_load(const char* user_path, size_t path_length) memcpy(storage.data(), payload.data(), payload.size()); payload.clear(); - // FIXME: ELFImage should really be taking a size argument as well... - auto elf_image = make(storage.data()); + auto elf_image = make(storage.data(), storage.size()); if (!elf_image->parse()) return -ENOEXEC; diff --git a/Libraries/LibELF/ELFDynamicLoader.cpp b/Libraries/LibELF/ELFDynamicLoader.cpp index ef1e0e71edf..2a6dcb0780b 100644 --- a/Libraries/LibELF/ELFDynamicLoader.cpp +++ b/Libraries/LibELF/ELFDynamicLoader.cpp @@ -56,7 +56,7 @@ void* ELFDynamicLoader::symbol_for_name(const char* name) bool ELFDynamicLoader::load_from_image(unsigned flags) { - ELFImage elf_image((u8*)m_file_mapping); + ELFImage elf_image((u8*)m_file_mapping, m_file_size); m_valid = elf_image.is_valid() && elf_image.is_dynamic(); diff --git a/Libraries/LibELF/ELFImage.cpp b/Libraries/LibELF/ELFImage.cpp index cb93e2abc3c..33ac0ff426d 100644 --- a/Libraries/LibELF/ELFImage.cpp +++ b/Libraries/LibELF/ELFImage.cpp @@ -2,8 +2,9 @@ #include #include -ELFImage::ELFImage(const u8* buffer) +ELFImage::ELFImage(const u8* buffer, size_t size) : m_buffer(buffer) + , m_size(size) { m_valid = parse(); } @@ -59,6 +60,8 @@ void ELFImage::dump() const dbgprintf(" entry: %x\n", header().e_entry); dbgprintf(" shoff: %u\n", header().e_shoff); dbgprintf(" shnum: %u\n", header().e_shnum); + dbgprintf(" phoff: %u\n", header().e_phoff); + dbgprintf(" phnum: %u\n", header().e_phnum); dbgprintf(" shstrndx: %u\n", header().e_shstrndx); for (unsigned i = 0; i < header().e_shnum; ++i) { diff --git a/Libraries/LibELF/ELFImage.h b/Libraries/LibELF/ELFImage.h index 88b436dd383..816c8e06829 100644 --- a/Libraries/LibELF/ELFImage.h +++ b/Libraries/LibELF/ELFImage.h @@ -8,12 +8,21 @@ class ELFImage { public: - explicit ELFImage(const u8*); + explicit ELFImage(const u8*, size_t); ~ELFImage(); void dump() const; bool is_valid() const { return m_valid; } bool parse(); + bool is_within_image(const void* address, size_t size) + { + if (address < m_buffer) + return false; + if (((const u8*)address + size) > m_buffer + m_size) + return false; + return true; + } + class Section; class RelocationSection; class Symbol; @@ -190,6 +199,7 @@ private: const char* section_index_to_string(unsigned index) const; const u8* m_buffer { nullptr }; + size_t m_size { 0 }; HashMap m_sections; bool m_valid { false }; unsigned m_symbol_table_section_index { 0 }; diff --git a/Libraries/LibELF/ELFLoader.cpp b/Libraries/LibELF/ELFLoader.cpp index ab8d38fba3e..3c89246774e 100644 --- a/Libraries/LibELF/ELFLoader.cpp +++ b/Libraries/LibELF/ELFLoader.cpp @@ -9,8 +9,8 @@ //#define ELFLOADER_DEBUG -ELFLoader::ELFLoader(const u8* buffer) - : m_image(buffer) +ELFLoader::ELFLoader(const u8* buffer, size_t size) + : m_image(buffer, size) { } @@ -43,6 +43,11 @@ bool ELFLoader::layout() failed = true; return; } + if (!m_image.is_within_image(program_header.raw_data(), program_header.size_in_image())) { + dbg() << "Shenanigans! ELF PT_TLS header sneaks outside of executable."; + failed = true; + return; + } memcpy(tls_image, program_header.raw_data(), program_header.size_in_image()); #endif return; @@ -65,6 +70,11 @@ bool ELFLoader::layout() failed = true; return; } + if (!m_image.is_within_image(program_header.raw_data(), program_header.size_in_image())) { + dbg() << "Shenanigans! Writable ELF PT_LOAD header sneaks outside of executable."; + failed = true; + return; + } memcpy(program_header.vaddr().as_ptr(), program_header.raw_data(), program_header.size_in_image()); } else { auto* mapped_section = map_section_hook( diff --git a/Libraries/LibELF/ELFLoader.h b/Libraries/LibELF/ELFLoader.h index 6d80fd31b14..fd96a52bab9 100644 --- a/Libraries/LibELF/ELFLoader.h +++ b/Libraries/LibELF/ELFLoader.h @@ -13,7 +13,7 @@ class Region; class ELFLoader { public: - explicit ELFLoader(const u8*); + explicit ELFLoader(const u8*, size_t); ~ELFLoader(); bool load();