Re: Playing around with security hardening compiler flags

Am 2024-11-17 19:28, schrieb Dimitry Andric:
> On 17 Nov 2024, at 16:30, Alexander Leidinger <Alexander@Leidinger.net> 
> wrote:
>> 
>> Hi,
>> 
>> after reading
>>    
>> https://security.googleblog.com/2024/11/retrofitting-spatial-safety-to-hundreds.html
>>    https://libcxx.llvm.org/Hardening.html
>>    
>> https://best.openssf.org/Compiler-Hardening-Guides/Compiler-Options-Hardening-Guide-for-C-and-C++.html
>> I played around a bit with some of the flags there (in CFLAGS).
>> 
>> What doesn't work:
>> - -fstrict-flex-arrays=3   (variable array issue in IIRC a tool for 
>> ath)
>> - -fstrict-flex-arrays=2   (issue in another area, haven't checked 
>> further)
>> 
>> What works and results in a world+kernel which is able to boot:
>> - -D_GLIBCXX_ASSERTIONS
>> - -fstrict-flex-arrays=1
>> - -fstack-clash-protection
>> - -D_LIBCPP_HARDENING_MODE=_LIBCPP_HARDENING_MODE_EXTENSIVE
> 
> FWIW the default hardening mode for libc++ is already extensive. There 
> is also a debug mode, but that is not suitable for general use. I have 
> not yet considered any WITH/WITHOUT options to fiddle with this, since 
> it is an option with 4 possible values: none, fast, extensive, and 
> debug.

Great, personally I don't need more.

> _GLIBCXX_ASSERTIONS is a similar directive for libstdc++, so it won't 
> make much difference for the base system, but it could be good for some 
> ports. (Not sure about the overhead though.)
> 
> I am unsure about the usefulness of -fstrict-flex-arrays, I have not 
> really played with this option. I would expect more warnings to come 
> out?

 From the 3rd link above:
---snip---
By default, GCC and Clang treat all trailing arrays (arrays that are 
placed as the last member or a structure) as flexible-sized arrays, 
regardless of declared size for the purposes of __builtin_object_size() 
calculations used by _FORTIFY_SOURCE60. This disables various bounds 
checks that do not always need to be disabled.
[...]
In this guide we recommend using the standard C99 flexible array 
notation [] instead of non-standard [0] or misleading [1], and then 
using -fstrict-flex-arrays=3 to improve bounds checking in such cases. 
In this case, code that uses [0] for a flexible array will need to be 
modified to use [] instead. Code that uses [1] for a flexible arrays 
needs to be modified to use [] and also extensively modified to 
eliminate off-by-one errors. Using [1] is not just misleading64, it’s 
error-prone; beware that existing code using [1] to indicate a flexible 
array may currently have off-by-one errors65.

Once in place, bounds-checking can occur in arrays with fixed declared 
sizes at the end of a struct. In addition, the source code unambiguously 
indicates, in a standard way, the cases where a flexible array is in 
use. There is normally no significant performance trade-off for this 
option (once any necessary changes have been made).
---snip---

Compiler Flag 	        Supported since 	Description
-fstrict-flex-arrays=3 	GCC 13.0.0
                         Clang 16.0.0 	Consider trailing array (at the 
end of struct) as flexible array only if declared as []
-fstrict-flex-arrays=2 	GCC 13.0.0
                         Clang 15.0.0 	Consider trailing array as a 
flexible array only if declared as [], or [0]
-fstrict-flex-arrays=1 	GCC 13.0.0
                         Clang 15.0.0 	Consider trailing array as a 
flexible array only if declared as [], [0], or [1]
-fstrict-flex-arrays=0 	GCC 13.0.0
                         Clang 15.0.0 	Consider any trailing array (at 
the end of a struct) a flexible array (the default)

We fail to build with =3 (with IIRC failure to access array[0]) and =2 
(with IIRC failure to access array[3]), but the build works with =1. So 
I expect a few more checks to be enabled than with the default of =0. 
Ideally we may want to get up to =3.

> Last but not least, -fstack-clash-protection might be useful, but I 
> think it might need some additional runtime support? E.g. in libc?

Just from reading what is written in the 3rd link above about it, it may 
be more a question if the correct runtime value for our stack gap is 
compiled in (or the right sysctl is used to query it at runtime during a 
compiler run), than libc support.

I quickly gobbled-up this (tabs are probably mis-converted to spaces 
during copy&paste of the diff here):
---snip---
diff --git share/mk/bsd.sys.mk share/mk/bsd.sys.mk
index 63774e85716..cc13b5ccc46 100644
--- share/mk/bsd.sys.mk
+++ share/mk/bsd.sys.mk
@@ -304,12 +304,12 @@ CXXFLAGS.clang+=   -Wno-c++11-extensions
  FORTIFY_SOURCE?=       0
  .if ${MK_SSP} != "no"
  # Don't use -Wstack-protector as it breaks world with -Werror.
-SSP_CFLAGS?=   -fstack-protector-strong
+SSP_CFLAGS?=   -fstack-protector-strong -fstack-clash-protection
  CFLAGS+=       ${SSP_CFLAGS}
  .endif # SSP
  .if ${FORTIFY_SOURCE} > 0
-CFLAGS+=       -D_FORTIFY_SOURCE=${FORTIFY_SOURCE}
-CXXFLAGS+=     -D_FORTIFY_SOURCE=${FORTIFY_SOURCE}
+CFLAGS+=       -D_FORTIFY_SOURCE=${FORTIFY_SOURCE} 
-fstrict-flex-arrays=1
+CXXFLAGS+=     -D_FORTIFY_SOURCE=${FORTIFY_SOURCE} 
-D_GLIBCXX_ASSERTIONS -fstrict-flex-arrays=1
  .endif

  # Additional flags passed in CFLAGS and CXXFLAGS when MK_DEBUG_FILES is
---snip---

As we don't have the gcc libstdc++ in the tree, it may be debatable if 
it needed to enable those assertions, but given the interest in IIRC 
hackers@ about libstd++ and libc++ it may not be that faaaaar off.

Any opinions? More discussion here, or rather opening a review for it?

Bye,
Alexander.

-- 
http://www.Leidinger.net Alexander@Leidinger.net: PGP 0x8F31830F9F2772BF
http://www.FreeBSD.org    netchild@FreeBSD.org  : PGP 0x8F31830F9F2772BF