From 06da50afc71a5ab2bc63de54c66930a2dbe379cd Mon Sep 17 00:00:00 2001 From: Brian Gianforcaro Date: Fri, 1 Jan 2021 15:27:42 -0800 Subject: [PATCH] Build + LibC: Enable -fstack-protector-strong in user space Modify the user mode runtime to insert stack canaries to find stack corruptions. The `-fstack-protector-strong` variant was chosen because it catches more issues than vanilla `-fstack-protector`, but doesn't have substantial performance impact like `-fstack-protector-all`. Details: -fstack-protector enables stack protection for vulnerable functions that contain: * A character array larger than 8 bytes. * An 8-bit integer array larger than 8 bytes. * A call to alloca() with either a variable size or a constant size bigger than 8 bytes. -fstack-protector-strong enables stack protection for vulnerable functions that contain: * An array of any size and type. * A call to alloca(). * A local variable that has its address taken. Example of it catching corrupting in the `stack-smash` test: ``` courage ~ $ ./user/Tests/LibC/stack-smash [+] Starting the stack smash ... Error: Stack protector failure, stack smashing detected! Shell: Job 1 (/usr/Tests/LibC/stack-smash) Aborted ``` --- CMakeLists.txt | 4 +- Libraries/LibC/CMakeLists.txt | 12 +++++- Libraries/LibC/cxxabi.cpp | 8 ---- Libraries/LibC/ssp.cpp | 55 +++++++++++++++++++++++++++ Libraries/LibC/sys/internals.h | 1 + Userland/DynamicLoader/CMakeLists.txt | 3 ++ Userland/Tests/LibC/stack-smash.cpp | 54 ++++++++++++++++++++++++++ 7 files changed, 124 insertions(+), 13 deletions(-) create mode 100644 Libraries/LibC/ssp.cpp create mode 100644 Userland/Tests/LibC/stack-smash.cpp diff --git a/CMakeLists.txt b/CMakeLists.txt index 7597e97fab6..df039d3f826 100644 --- a/CMakeLists.txt +++ b/CMakeLists.txt @@ -125,9 +125,7 @@ if (CMAKE_SYSTEM_NAME MATCHES Darwin) set(CMAKE_SKIP_RPATH TRUE) endif() -#FIXME: -fstack-protector - -add_compile_options(-Os -g1 -fno-exceptions -Wno-address-of-packed-member -Wundef -Wcast-qual -Wwrite-strings -Wimplicit-fallthrough -Wno-nonnull-compare -Wno-deprecated-copy -Wno-expansion-to-defined) +add_compile_options(-Os -g1 -fno-exceptions -fstack-protector-strong -Wno-address-of-packed-member -Wundef -Wcast-qual -Wwrite-strings -Wimplicit-fallthrough -Wno-nonnull-compare -Wno-deprecated-copy -Wno-expansion-to-defined) add_compile_options(-ffile-prefix-map=${CMAKE_SOURCE_DIR}=.) add_compile_definitions(DEBUG SANITIZE_PTRS) diff --git a/Libraries/LibC/CMakeLists.txt b/Libraries/LibC/CMakeLists.txt index 3366957d5dd..971880ca00e 100644 --- a/Libraries/LibC/CMakeLists.txt +++ b/Libraries/LibC/CMakeLists.txt @@ -70,13 +70,21 @@ add_custom_command( COMMAND ${INSTALL_COMMAND} -D $ ${CMAKE_INSTALL_PREFIX}/usr/lib/crt0_shared.o ) +set_source_files_properties (ssp.cpp PROPERTIES COMPILE_FLAGS + "-fno-stack-protector") +add_library(ssp STATIC ssp.cpp) +add_custom_command( + TARGET ssp + COMMAND ${INSTALL_COMMAND} -D $ ${CMAKE_INSTALL_PREFIX}/usr/lib/ssp.o +) + set(SOURCES ${LIBC_SOURCES} ${AK_SOURCES} ${ELF_SOURCES}) serenity_libc_static(LibCStatic c) -target_link_libraries(LibCStatic crt0) +target_link_libraries(LibCStatic crt0 ssp) add_dependencies(LibCStatic LibM) set(CMAKE_CXX_FLAGS "${CMAKE_CXX_FLAGS} -static-libstdc++") serenity_libc(LibC c) -target_link_libraries(LibC crt0) +target_link_libraries(LibC crt0 ssp) add_dependencies(LibC LibM) diff --git a/Libraries/LibC/cxxabi.cpp b/Libraries/LibC/cxxabi.cpp index cc920dec89d..05978bfe9b7 100644 --- a/Libraries/LibC/cxxabi.cpp +++ b/Libraries/LibC/cxxabi.cpp @@ -24,7 +24,6 @@ * OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE. */ -#include #include #include #include @@ -87,11 +86,4 @@ void __cxa_finalize(void* dso_handle) ASSERT_NOT_REACHED(); } -extern u32 __stack_chk_guard; -u32 __stack_chk_guard = (u32)0xc6c7c8c9; - -[[noreturn]] void __stack_chk_fail() -{ - ASSERT_NOT_REACHED(); -} } // extern "C" diff --git a/Libraries/LibC/ssp.cpp b/Libraries/LibC/ssp.cpp new file mode 100644 index 00000000000..c2715d6ba01 --- /dev/null +++ b/Libraries/LibC/ssp.cpp @@ -0,0 +1,55 @@ +/* + * Copyright (c) 2021, Brian Gianforcaro + * All rights reserved. + * + * Redistribution and use in source and binary forms, with or without + * modification, are permitted provided that the following conditions are met: + * + * 1. Redistributions of source code must retain the above copyright notice, this + * list of conditions and the following disclaimer. + * + * 2. Redistributions in binary form must reproduce the above copyright notice, + * this list of conditions and the following disclaimer in the documentation + * and/or other materials provided with the distribution. + * + * THIS SOFTWARE IS PROVIDED BY THE COPYRIGHT HOLDERS AND CONTRIBUTORS "AS IS" + * AND ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT LIMITED TO, THE + * IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS FOR A PARTICULAR PURPOSE ARE + * DISCLAIMED. IN NO EVENT SHALL THE COPYRIGHT HOLDER OR CONTRIBUTORS BE LIABLE + * FOR ANY DIRECT, INDIRECT, INCIDENTAL, SPECIAL, EXEMPLARY, OR CONSEQUENTIAL + * DAMAGES (INCLUDING, BUT NOT LIMITED TO, PROCUREMENT OF SUBSTITUTE GOODS OR + * SERVICES; LOSS OF USE, DATA, OR PROFITS; OR BUSINESS INTERRUPTION) HOWEVER + * CAUSED AND ON ANY THEORY OF LIABILITY, WHETHER IN CONTRACT, STRICT LIABILITY, + * OR TORT (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT OF THE USE + * OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE. + */ + +#include +#include +#include +#include +#include + +#if defined __SSP__ || defined __SSP_ALL__ +# error "file must not be compiled with stack protection enabled on it. Use -fno-stack-protector" +#endif + +extern "C" { + +extern u32 __stack_chk_guard; +u32 __stack_chk_guard = (u32)0xc6c7c8c9; + +[[noreturn]] void __stack_chk_fail() +{ + dbgprintf("Error: USERSPACE(%d) Stack protector failure, stack smashing detected!\n", getpid()); + if (__stdio_is_initialized) + fprintf(stderr, "Error: Stack protector failure, stack smashing detected!\n"); + abort(); +} + +[[noreturn]] void __stack_chk_fail_local() +{ + __stack_chk_fail(); +} + +} // extern "C" diff --git a/Libraries/LibC/sys/internals.h b/Libraries/LibC/sys/internals.h index 70443404e1d..c23785a02e7 100644 --- a/Libraries/LibC/sys/internals.h +++ b/Libraries/LibC/sys/internals.h @@ -43,5 +43,6 @@ int __cxa_atexit(AtExitFunction exit_function, void* parameter, void* dso_handle void __cxa_finalize(void* dso_handle); [[noreturn]] void __cxa_pure_virtual() __attribute__((weak)); [[noreturn]] void __stack_chk_fail(); +[[noreturn]] void __stack_chk_fail_local(); __END_DECLS diff --git a/Userland/DynamicLoader/CMakeLists.txt b/Userland/DynamicLoader/CMakeLists.txt index 7c763f05575..f870efe9ca3 100644 --- a/Userland/DynamicLoader/CMakeLists.txt +++ b/Userland/DynamicLoader/CMakeLists.txt @@ -18,6 +18,9 @@ set(SOURCES ${LOADER_SOURCES} ${AK_SOURCES} ${ELF_SOURCES} ${LIBC_SOURCES1} ${LI set(CMAKE_CXX_FLAGS "${CMAKE_CXX_FLAGS} -fno-rtti -nostdlib -pie -fpic -DNO_TLS") +set_source_files_properties (../../Libraries/LibC/ssp.cpp PROPERTIES COMPILE_FLAGS + "-fno-stack-protector") + add_executable(Loader.so ${SOURCES}) target_link_options(Loader.so PRIVATE LINKER:--no-dynamic-linker) install(TARGETS Loader.so RUNTIME DESTINATION usr/lib/) diff --git a/Userland/Tests/LibC/stack-smash.cpp b/Userland/Tests/LibC/stack-smash.cpp new file mode 100644 index 00000000000..00f6f44aa5d --- /dev/null +++ b/Userland/Tests/LibC/stack-smash.cpp @@ -0,0 +1,54 @@ +/* + * Copyright (c) 2021, Brian Gianforcaro + * All rights reserved. + * + * Redistribution and use in source and binary forms, with or without + * modification, are permitted provided that the following conditions are met: + * + * 1. Redistributions of source code must retain the above copyright notice, this + * list of conditions and the following disclaimer. + * + * 2. Redistributions in binary form must reproduce the above copyright notice, + * this list of conditions and the following disclaimer in the documentation + * and/or other materials provided with the distribution. + * + * THIS SOFTWARE IS PROVIDED BY THE COPYRIGHT HOLDERS AND CONTRIBUTORS "AS IS" + * AND ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT LIMITED TO, THE + * IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS FOR A PARTICULAR PURPOSE ARE + * DISCLAIMED. IN NO EVENT SHALL THE COPYRIGHT HOLDER OR CONTRIBUTORS BE LIABLE + * FOR ANY DIRECT, INDIRECT, INCIDENTAL, SPECIAL, EXEMPLARY, OR CONSEQUENTIAL + * DAMAGES (INCLUDING, BUT NOT LIMITED TO, PROCUREMENT OF SUBSTITUTE GOODS OR + * SERVICES; LOSS OF USE, DATA, OR PROFITS; OR BUSINESS INTERRUPTION) HOWEVER + * CAUSED AND ON ANY THEORY OF LIABILITY, WHETHER IN CONTRACT, STRICT LIABILITY, + * OR TORT (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT OF THE USE + * OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE. + */ + +#include + +// Note: Needs to be 'noline' so stack canary isn't optimized out. +static void __attribute__((noinline)) smasher(char* string) +{ +#pragma GCC diagnostic push +#pragma GCC diagnostic ignored "-Warray-bounds" + for (int i = 0; i < 256; i++) { + string[i] = 'A'; + } +#pragma GCC diagnostic pop +} + +// Note: Needs to be 'noline' so stack canary isn't optimized out. +static void __attribute__((noinline)) stack_to_smash() +{ + char string[8] = {}; + smasher(string); +} + +int main() +{ + puts("[+] Starting the stack smash..."); + stack_to_smash(); + puts("[+] Stack smash wasn't detected!"); + + return 0; +}