From 43383228e7751f0326d511cecc19e6d98d6e785d Mon Sep 17 00:00:00 2001 From: Jeremy Rifkin <51220084+jeremy-rifkin@users.noreply.github.com> Date: Sun, 23 Jul 2023 23:01:20 -0400 Subject: [PATCH] Bake an absolute path for addr2line into the library to prevent against path injection (#19) --- CMakeLists.txt | 25 +++++++++-- README.md | 43 +++++++++++------- lint.sh | 2 +- src/symbols/symbols_with_addr2line.cpp | 62 +++++++++++++++++++++----- 4 files changed, 101 insertions(+), 31 deletions(-) diff --git a/CMakeLists.txt b/CMakeLists.txt index 698c5f0..93f9e33 100644 --- a/CMakeLists.txt +++ b/CMakeLists.txt @@ -67,6 +67,11 @@ option(CPPTRACE_UNWIND_WITH_NOTHING "" OFF) option(CPPTRACE_DEMANGLE_WITH_CXXABI "" OFF) option(CPPTRACE_DEMANGLE_WITH_NOTHING "" OFF) +set(CPPTRACE_BACKTRACE_PATH "" CACHE STRING "Path to backtrace.h, if the compiler doesn't already know it. Check /usr/lib/gcc/x86_64-linux-gnu/*/include.") +set(CPPTRACE_HARD_MAX_FRAMES "" CACHE STRING "Hard limit on unwinding depth. Default is 100.") +set(CPPTRACE_ADDR2LINE_PATH "" CACHE STRING "Absolute path to the addr2line executable you want to use.") +option(CPPTRACE_ADDR2LINE_SEARCH_SYSTEM_PATH "" OFF) + option(CPPTRACE_BUILD_TEST "" OFF) option(CPPTRACE_BUILD_DEMO "" OFF) option(CPPTRACE_BUILD_TEST_RDYNAMIC "" OFF) @@ -75,9 +80,6 @@ option(CPPTRACE_BUILD_SPEEDTEST "" OFF) option(CPPTRACE_BUILD_SPEEDTEST_DWARF4 "" OFF) option(CPPTRACE_BUILD_SPEEDTEST_DWARF5 "" OFF) -set(CPPTRACE_BACKTRACE_PATH "" CACHE STRING "Path to backtrace.h, if the compiler doesn't already know it. Check /usr/lib/gcc/x86_64-linux-gnu/*/include.") -set(CPPTRACE_HARD_MAX_FRAMES "" CACHE STRING "Hard limit on unwinding depth. Default is 100.") - if(NOT "${CPPTRACE_BACKTRACE_PATH}" STREQUAL "") # quotes used over <> because of a macro substitution issue where # @@ -276,6 +278,23 @@ if(CPPTRACE_GET_SYMBOLS_WITH_LIBDL) endif() if(CPPTRACE_GET_SYMBOLS_WITH_ADDR2LINE) + # set(CPPTRACE_ADDR2LINE_PATH "" CACHE STRING "Absolute path to the addr2line executable you want to use.") + # option(CPPTRACE_ADDR2LINE_SEARCH_SYSTEM_PATH "" OFF) + if(CPPTRACE_ADDR2LINE_SEARCH_SYSTEM_PATH) + target_compile_definitions(cpptrace PUBLIC CPPTRACE_ADDR2LINE_SEARCH_SYSTEM_PATH) + else() + if("${CPPTRACE_ADDR2LINE_PATH}" STREQUAL "") + if(APPLE) + find_program(CPPTRACE_ADDR2LINE_PATH_FINAL atos PATHS ENV PATH REQUIRED) + else() + find_program(CPPTRACE_ADDR2LINE_PATH_FINAL addr2line PATHS ENV PATH REQUIRED) + endif() + else() + set(CPPTRACE_ADDR2LINE_PATH_FINAL "${CPPTRACE_ADDR2LINE_PATH}") + endif() + message(STATUS "Cpptrace: Using ${CPPTRACE_ADDR2LINE_PATH_FINAL} for addr2line path") + target_compile_definitions(cpptrace PUBLIC CPPTRACE_ADDR2LINE_PATH="${CPPTRACE_ADDR2LINE_PATH_FINAL}") + endif() target_compile_definitions(cpptrace PUBLIC CPPTRACE_GET_SYMBOLS_WITH_ADDR2LINE) if(UNIX) target_link_libraries(cpptrace PRIVATE dl) diff --git a/README.md b/README.md index fbf8b66..844618a 100644 --- a/README.md +++ b/README.md @@ -185,6 +185,10 @@ can hold addresses for 100 frames (beyond the `skip` frames). This is configurab *: Requires installation +Note for addr2line: By default cmake will resolve an absolute path to addr2line to bake into the library. This path can +be configured with `CPPTRACE_ADDR2LINE_PATH`, or `CPPTRACE_ADDR2LINE_SEARCH_SYSTEM_PATH` can be used to have the library +search the system path for `addr2line` at runtime. This is not the default to prevent against path injection attacks. + **Demangling** Lastly, depending on other back-ends used a demangler back-end may be needed. A demangler back-end is not needed when @@ -215,24 +219,29 @@ more back-ends can be added. Ideally this library can "just work" on systems, wi Summary of all library configuration options: Back-ends: -- `CPPTRACE_FULL_TRACE_WITH_LIBBACKTRACE` -- `CPPTRACE_FULL_TRACE_WITH_STACKTRACE` -- `CPPTRACE_GET_SYMBOLS_WITH_LIBBACKTRACE` -- `CPPTRACE_GET_SYMBOLS_WITH_LIBDL` -- `CPPTRACE_GET_SYMBOLS_WITH_ADDR2LINE` -- `CPPTRACE_GET_SYMBOLS_WITH_DBGHELP` -- `CPPTRACE_GET_SYMBOLS_WITH_NOTHING` -- `CPPTRACE_UNWIND_WITH_UNWIND` -- `CPPTRACE_UNWIND_WITH_EXECINFO` -- `CPPTRACE_UNWIND_WITH_WINAPI` -- `CPPTRACE_UNWIND_WITH_NOTHING` -- `CPPTRACE_DEMANGLE_WITH_CXXABI` -- `CPPTRACE_DEMANGLE_WITH_NOTHING` +- `CPPTRACE_FULL_TRACE_WITH_LIBBACKTRACE=On/Off` +- `CPPTRACE_FULL_TRACE_WITH_STACKTRACE=On/Off` +- `CPPTRACE_GET_SYMBOLS_WITH_LIBBACKTRACE=On/Off` +- `CPPTRACE_GET_SYMBOLS_WITH_LIBDL=On/Off` +- `CPPTRACE_GET_SYMBOLS_WITH_ADDR2LINE=On/Off` +- `CPPTRACE_GET_SYMBOLS_WITH_DBGHELP=On/Off` +- `CPPTRACE_GET_SYMBOLS_WITH_NOTHING=On/Off` +- `CPPTRACE_UNWIND_WITH_UNWIND=On/Off` +- `CPPTRACE_UNWIND_WITH_EXECINFO=On/Off` +- `CPPTRACE_UNWIND_WITH_WINAPI=On/Off` +- `CPPTRACE_UNWIND_WITH_NOTHING=On/Off` +- `CPPTRACE_DEMANGLE_WITH_CXXABI=On/Off` +- `CPPTRACE_DEMANGLE_WITH_NOTHING=On/Off` -General: -- `CPPTRACE_BACKTRACE_PATH`: Path to libbacktrace backtrace.h, needed when compiling with clang -- `CPPTRACE_HARD_MAX_FRAMES`: Some back-ends write to a fixed-size buffer. This is the size of that buffer. Default is - `100`. +Back-end configuration: +- `CPPTRACE_BACKTRACE_PATH=`: Path to libbacktrace backtrace.h, needed when compiling with clang +- `CPPTRACE_HARD_MAX_FRAMES=`: Some back-ends write to a fixed-size buffer. This is the size of that buffer. + Default is `100`. +- `CPPTRACE_ADDR2LINE_PATH=`: Specify the absolute path to the addr2line binary for cpptrace to invoke. By + default the config script will search for a binary and use that absolute path (this is to prevent against path + injection). +- `CPPTRACE_ADDR2LINE_SEARCH_SYSTEM_PATH=On/Off`: Specifies whether cpptrace should let the system search the PATH + environment variable directories for the binary. Testing: - `CPPTRACE_BUILD_TEST` Build a small test program diff --git a/lint.sh b/lint.sh index 2cc2ae5..4cae5d0 100644 --- a/lint.sh +++ b/lint.sh @@ -4,7 +4,7 @@ status=0 while read f do echo checking $f - flags="-DCPPTRACE_FULL_TRACE_WITH_LIBBACKTRACE -DCPPTRACE_FULL_TRACE_WITH_STACKTRACE -DCPPTRACE_GET_SYMBOLS_WITH_LIBBACKTRACE -DCPPTRACE_GET_SYMBOLS_WITH_LIBDL -DCPPTRACE_GET_SYMBOLS_WITH_ADDR2LINE -DCPPTRACE_GET_SYMBOLS_WITH_NOTHING -DCPPTRACE_UNWIND_WITH_EXECINFO -DCPPTRACE_UNWIND_WITH_UNWIND -DCPPTRACE_UNWIND_WITH_NOTHING -DCPPTRACE_DEMANGLE_WITH_CXXABI -DCPPTRACE_DEMANGLE_WITH_NOTHING" + flags="-DCPPTRACE_FULL_TRACE_WITH_LIBBACKTRACE -DCPPTRACE_FULL_TRACE_WITH_STACKTRACE -DCPPTRACE_GET_SYMBOLS_WITH_LIBBACKTRACE -DCPPTRACE_GET_SYMBOLS_WITH_LIBDL -DCPPTRACE_GET_SYMBOLS_WITH_ADDR2LINE -DCPPTRACE_GET_SYMBOLS_WITH_NOTHING -DCPPTRACE_UNWIND_WITH_EXECINFO -DCPPTRACE_UNWIND_WITH_UNWIND -DCPPTRACE_UNWIND_WITH_NOTHING -DCPPTRACE_DEMANGLE_WITH_CXXABI -DCPPTRACE_DEMANGLE_WITH_NOTHING -DCPPTRACE_ADDR2LINE_SEARCH_SYSTEM_PATH" clang-tidy $f -- -std=c++11 -Iinclude $@ $flags ret=$? if [ $ret -ne 0 ]; then diff --git a/src/symbols/symbols_with_addr2line.cpp b/src/symbols/symbols_with_addr2line.cpp index fc16877..6537d9e 100644 --- a/src/symbols/symbols_with_addr2line.cpp +++ b/src/symbols/symbols_with_addr2line.cpp @@ -72,11 +72,17 @@ namespace cpptrace { if(pid == 0) { // child close(STDOUT_FILENO); close(STDERR_FILENO); // atos --help writes to stderr - // TODO: path - #if !IS_APPLE - execlp("addr2line", "addr2line", "--help", nullptr); + #ifdef CPPTRACE_ADDR2LINE_SEARCH_SYSTEM_PATH + #if !IS_APPLE + execlp("addr2line", "addr2line", "--help", nullptr); + #else + execlp("atos", "atos", "--help", nullptr); + #endif #else - execlp("atos", "atos", "--help", nullptr); + #ifndef CPPTRACE_ADDR2LINE_PATH + #error "CPPTRACE_ADDR2LINE_PATH must be defined if CPPTRACE_ADDR2LINE_SEARCH_SYSTEM_PATH is not" + #endif + execl(CPPTRACE_ADDR2LINE_PATH, CPPTRACE_ADDR2LINE_PATH, "--help", nullptr); #endif _exit(magic); } @@ -115,11 +121,30 @@ namespace cpptrace { close(input_pipe.read_end); close(input_pipe.write_end); close(STDERR_FILENO); // TODO: Might be worth conditionally enabling or piping - // TODO: Prevent against path injection? - #if !IS_APPLE - execlp("addr2line", "addr2line", "-e", executable.c_str(), "-f", "-C", "-p", nullptr); + #ifdef CPPTRACE_ADDR2LINE_SEARCH_SYSTEM_PATH + #if !IS_APPLE + execlp("addr2line", "addr2line", "-e", executable.c_str(), "-f", "-C", "-p", nullptr); + #else + execlp("atos", "atos", "-o", executable.c_str(), nullptr); + #endif #else - execlp("atos", "atos", "-o", executable.c_str(), nullptr); + #ifndef CPPTRACE_ADDR2LINE_PATH + #error "CPPTRACE_ADDR2LINE_PATH must be defined if CPPTRACE_ADDR2LINE_SEARCH_SYSTEM_PATH is not" + #endif + #if !IS_APPLE + execl( + CPPTRACE_ADDR2LINE_PATH, + CPPTRACE_ADDR2LINE_PATH, + "-e", + executable.c_str(), + "-f", + "-C", + "-p", + nullptr + ); + #else + execl(CPPTRACE_ADDR2LINE_PATH, CPPTRACE_ADDR2LINE_PATH, "-o", executable.c_str(), nullptr); + #endif #endif _exit(1); // TODO: Diagnostic? } @@ -218,7 +243,14 @@ namespace cpptrace { if(!checked) { // TODO: Popen is a hack. Implement properly with CreateProcess and pipes later. checked = true; - FILE* p = popen("addr2line --version", "r"); + #ifdef CPPTRACE_ADDR2LINE_SEARCH_SYSTEM_PATH + FILE* p = popen("addr2line --version", "r"); + #else + #ifndef CPPTRACE_ADDR2LINE_PATH + #error "CPPTRACE_ADDR2LINE_PATH must be defined if CPPTRACE_ADDR2LINE_SEARCH_SYSTEM_PATH is not" + #endif + FILE* p = popen(CPPTRACE_ADDR2LINE_PATH " --version", "r"); + #endif if(p) { has_addr2line = pclose(p) == 0; } @@ -229,7 +261,17 @@ namespace cpptrace { std::string resolve_addresses(const std::string& addresses, const std::string& executable) { // TODO: Popen is a hack. Implement properly with CreateProcess and pipes later. ///fprintf(stderr, ("addr2line -e " + executable + " -fCp " + addresses + "\n").c_str()); - FILE* p = popen(("addr2line -e " + executable + " -fCp " + addresses).c_str(), "r"); + #ifdef CPPTRACE_ADDR2LINE_SEARCH_SYSTEM_PATH + FILE* p = popen(("addr2line -e " + executable + " -fCp " + addresses).c_str(), "r"); + #else + #ifndef CPPTRACE_ADDR2LINE_PATH + #error "CPPTRACE_ADDR2LINE_PATH must be defined if CPPTRACE_ADDR2LINE_SEARCH_SYSTEM_PATH is not" + #endif + FILE* p = popen( + (CPPTRACE_ADDR2LINE_PATH " -e " + executable + " -fCp " + addresses).c_str(), + "r" + ); + #endif std::string output; constexpr int buffer_size = 4096; char buffer[buffer_size];