From 79d76f5289bdf7d074936f5df44b107d245675fd Mon Sep 17 00:00:00 2001 From: Larry Gritz Date: Fri, 15 Nov 2024 13:15:37 -0800 Subject: [PATCH] build: Add hardening options (#4538) Get rid of the FORTIFY options, which were too specific to one compiler feature. Add OIIO_HARDENING that takes a level meaning: - 0 : do nothing, not recommended. - 1 : enable features that have no (or nearly no) performance impact, recommended default for optimized, shipping code. - 2 : enable features that trade off performance for security, recommended for debugging or deploying in security-sensitive environments. - 3 : enable features that have a significant performance impact, only recommended for debugging. Default to 1 for optimized builds, 3 for debug builds (so will be thoroughly tested by our sanitizer and other CI tests). Users that have more stringent security requirements may choose to build with 2 even for shipping code (they should benchmark to see if that is acceptable to them). These levels turn on a variety of compiler options that are recommended for additional safety. We will add more as they are developed. There are also some warning suppressions that needed to be added to code in a few areas where it was unavoidable to use some constructs that trigger the elevated safety checks. Signed-off-by: Larry Gritz --- src/cmake/compiler.cmake | 82 ++++++++++++++++++++++++--------- src/cmake/set_utils.cmake | 4 +- src/libutil/SHA1.cpp | 7 ++- src/tiff.imageio/tiffinput.cpp | 9 ++++ src/tiff.imageio/tiffoutput.cpp | 6 +++ 5 files changed, 82 insertions(+), 26 deletions(-) diff --git a/src/cmake/compiler.cmake b/src/cmake/compiler.cmake index b3a602ddc8..9367b3e356 100644 --- a/src/cmake/compiler.cmake +++ b/src/cmake/compiler.cmake @@ -86,6 +86,9 @@ elseif (CMAKE_CXX_COMPILER_ID MATCHES "Intel") set (CMAKE_COMPILER_IS_INTEL 1) message (VERBOSE "Using Intel as the compiler") endif () +if (CMAKE_COMPILER_IS_GNUCC OR CMAKE_COMPILER_IS_CLANG) + set (COMPILER_IS_GCC_OR_ANY_CLANG TRUE) +endif () ########################################################################### @@ -102,12 +105,12 @@ else () endif() option (EXTRA_WARNINGS "Enable lots of extra pedantic warnings" OFF) if (NOT MSVC) - add_compile_options ("-Wall") + add_compile_options (-Wall -Wformat -Wformat=2) if (EXTRA_WARNINGS) - add_compile_options ("-Wextra") + add_compile_options (-Wextra) endif () if (STOP_ON_WARNING) - add_compile_options ("-Werror") + add_compile_options (-Werror) endif () endif () @@ -462,25 +465,60 @@ endif () ########################################################################### -# Fortification and hardening options -# -# In modern gcc and clang, FORTIFY_SOURCE provides buffer overflow checks -# (with some compiler-assisted deduction of buffer lengths) for the following -# functions: memcpy, mempcpy, memmove, memset, strcpy, stpcpy, strncpy, -# strcat, strncat, sprintf, vsprintf, snprintf, vsnprintf, gets. -# -# We try to avoid these unsafe functions anyway, but it's good to have the -# extra protection, at least as an extra set of checks during CI. Some users -# may also wish to enable it at some level if they are deploying it in a -# security-sensitive environment. FORTIFY_SOURCE=3 may have minor performance -# impact, though FORTIFY_SOURCE=2 should not have a measurable effect on -# performance versus not doing any fortification. All fortification levels are -# not available in all compilers. - -set (FORTIFY_SOURCE "0" CACHE STRING "Turn on Fortification level (0, 1, 2, 3)") -if (FORTIFY_SOURCE AND (CMAKE_COMPILER_IS_GNUCC OR CMAKE_COMPILER_IS_CLANG)) - message (STATUS "Compiling with _FORTIFY_SOURCE=${FORTIFY_SOURCE}") - add_compile_options (-D_FORTIFY_SOURCE=${FORTIFY_SOURCE}) +# Safety/security hardening options +# +# Explanation of levels: +# 0 : do nothing, not recommended. +# 1 : enable features that have no (or nearly no) performance impact, +# recommended default for optimized, shipping code. +# 2 : enable features that trade off performance for security, recommended +# for debugging or deploying in security-sensitive environments. +# 3 : enable features that have a significant performance impact, only +# recommended for debugging. +# +# Some documentation: +# https://best.openssf.org/Compiler-Hardening-Guides/Compiler-Options-Hardening-Guide-for-C-and-C++.html +# https://www.gnu.org/software/libc/manual/html_node/Source-Fortification.html +# https://gcc.gnu.org/onlinedocs/libstdc++/manual/using_macros.html +# https://libcxx.llvm.org/Hardening.html +# +if (${CMAKE_BUILD_TYPE} STREQUAL "Debug") + set (${PROJ_NAME}_HARDENING_DEFAULT 3) +else () + set (${PROJ_NAME}_HARDENING_DEFAULT 1) +endif () +set_cache (${PROJ_NAME}_HARDENING ${${PROJ_NAME}_HARDENING_DEFAULT} + "Turn on security hardening features 0, 1, 2, 3") +# Implementation: +if (HARDENING GREATER_EQUAL 1) + # Features that should not detectably affect performance + if (COMPILER_IS_GCC_OR_ANY_CLANG) + # Enable PIE and pie to build as position-independent executables, + # needed for address space randomiztion used by some kernels. + add_compile_options (-fPIE -pie) + add_link_options (-fPIE -pie) + # Protect against stack overwrites. Is allegedly not a performance + # tradeoff. + add_compile_options (-fstack-protector-strong) + add_link_options (-fstack-protector-strong) + endif () + # Defining _FORTIFY_SOURCE provides buffer overflow checks in modern gcc & + # clang with some compiler-assisted deduction of buffer lengths) for the + # many C functions such as memcpy, strcpy, sprintf, etc. See: + add_compile_definitions (_FORTIFY_SOURCE=${${PROJ_NAME}_HARDENING}) + # Setting _LIBCPP_HARDENING_MODE enables various hardening features in + # clang/llvm's libc++ 18.0 and later. + add_compiler_definitions (_LIBCPP_HARDENING_MODE=_LIBCPP_HARDENING_MODE_FAST) +endif () +if (HARDENING GREATER_EQUAL 2) + # Features that might impact performance measurably + add_compile_definitions (_GLIBCXX_ASSERTIONS) + add_compiler_definitions (_LIBCPP_HARDENING_MODE=_LIBCPP_HARDENING_MODE_EXTENSIVE) +endif () +if (HARDENING GREATER_EQUAL 3) + # Debugging features that might impact performance significantly + add_compile_definitions (_GLIBCXX_DEBUG) + add_compiler_definitions (_LIBCPP_HARDENING_MODE=_LIBCPP_HARDENING_MODE_DEBUG) endif () diff --git a/src/cmake/set_utils.cmake b/src/cmake/set_utils.cmake index 42a29cdc07..982c243a6e 100644 --- a/src/cmake/set_utils.cmake +++ b/src/cmake/set_utils.cmake @@ -98,9 +98,9 @@ macro (super_set name value) set (${name} ${_sce_val} ${_sce_extra_args}) endif () if (_sce_VERBOSE) - message (STATUS "${name} = ${${name}}") + message (STATUS "${name} = ${${name}} (${_sce_DOC})") else () - message (VERBOSE "${name} = ${${name}}") + message (VERBOSE "${name} = ${${name}} (${_sce_DOC})") endif () if (_sce_ADVANCED) mark_as_advanced (${name}) diff --git a/src/libutil/SHA1.cpp b/src/libutil/SHA1.cpp index b371fb5bac..0ac8959bec 100644 --- a/src/libutil/SHA1.cpp +++ b/src/libutil/SHA1.cpp @@ -269,10 +269,13 @@ bool CSHA1::ReportHash(TCHAR* tszReport, REPORT_TYPE rtReportType) const _sntprintf(tszTemp, 15, _T("%02X"), m_digest[0]); Strutil::safe_strcpy(tszReport, tszTemp, HASH_TEMP_BUFFER_SIZE - 1); - const TCHAR* lpFmt = ((rtReportType == REPORT_HEX) ? _T(" %02X") : _T("%02X")); + // const TCHAR* lpFmt = ((rtReportType == REPORT_HEX) ? _T(" %02X") : _T("%02X")); for(size_t i = 1; i < 20; ++i) { - _sntprintf(tszTemp, 15, lpFmt, m_digest[i]); + if (rtReportType == REPORT_HEX) + _sntprintf(tszTemp, 15, _T(" %02X"), m_digest[i]); + else + _sntprintf(tszTemp, 15, _T("%02X"), m_digest[i]); Strutil::safe_strcat(tszReport, tszTemp, HASH_TEMP_BUFFER_SIZE - 1); } } diff --git a/src/tiff.imageio/tiffinput.cpp b/src/tiff.imageio/tiffinput.cpp index 3e4331db3f..3d7cea962d 100644 --- a/src/tiff.imageio/tiffinput.cpp +++ b/src/tiff.imageio/tiffinput.cpp @@ -478,7 +478,10 @@ class TIFFInput final : public ImageInput { { TIFFInput* self = (TIFFInput*)user_data; spin_lock lock(self->m_last_error_mutex); + OIIO_PRAGMA_WARNING_PUSH + OIIO_GCC_PRAGMA(GCC diagnostic ignored "-Wformat-nonliteral") self->m_last_error = Strutil::vsprintf(fmt, ap); + OIIO_PRAGMA_WARNING_POP return 1; } @@ -488,7 +491,10 @@ class TIFFInput final : public ImageInput { { TIFFInput* self = (TIFFInput*)user_data; spin_lock lock(self->m_last_error_mutex); + OIIO_PRAGMA_WARNING_PUSH + OIIO_GCC_PRAGMA(GCC diagnostic ignored "-Wformat-nonliteral") self->m_last_error = Strutil::vsprintf(fmt, ap); + OIIO_PRAGMA_WARNING_POP return 1; } #endif @@ -534,7 +540,10 @@ oiio_tiff_last_error() static void my_error_handler(const char* /*str*/, const char* format, va_list ap) { + OIIO_PRAGMA_WARNING_PUSH + OIIO_GCC_PRAGMA(GCC diagnostic ignored "-Wformat-nonliteral") oiio_tiff_last_error() = Strutil::vsprintf(format, ap); + OIIO_PRAGMA_WARNING_POP } diff --git a/src/tiff.imageio/tiffoutput.cpp b/src/tiff.imageio/tiffoutput.cpp index 18fc769587..35482d7622 100644 --- a/src/tiff.imageio/tiffoutput.cpp +++ b/src/tiff.imageio/tiffoutput.cpp @@ -231,7 +231,10 @@ class TIFFOutput final : public ImageOutput { { TIFFOutput* self = (TIFFOutput*)user_data; spin_lock lock(self->m_last_error_mutex); + OIIO_PRAGMA_WARNING_PUSH + OIIO_GCC_PRAGMA(GCC diagnostic ignored "-Wformat-nonliteral") self->m_last_error = Strutil::vsprintf(fmt, ap); + OIIO_PRAGMA_WARNING_POP return 1; } @@ -241,7 +244,10 @@ class TIFFOutput final : public ImageOutput { { TIFFOutput* self = (TIFFOutput*)user_data; spin_lock lock(self->m_last_error_mutex); + OIIO_PRAGMA_WARNING_PUSH + OIIO_GCC_PRAGMA(GCC diagnostic ignored "-Wformat-nonliteral") self->m_last_error = Strutil::vsprintf(fmt, ap); + OIIO_PRAGMA_WARNING_POP return 1; } #endif