hardening flags: add FORTIFY_SOURCE=3 support
#212498
Conversation
it's very unclear what the status of FORTIFY_SOURCE=3 support in clang is
using the same odd mechanism as is used to disable fortify mid-build
ofborg
bot
added
10.rebuild-darwin-stdenv
ofborg
bot
requested review from
lovek323,
Synthetica9,
vcunat,
dtzWill,
primeos,
veprbl and
sternenseemann
mweinelt
added
the
12.approvals: 1
|
I will probably merge this next week unless there are any objections - want this to get a lot of testing before we approach release-time. |
|
Based on what I've read I wasn't sure if the level 3 is a suitable default (for us), but that certainly doesn't matter for this PR. |
|
I did not bisect precisely but I have a strong suspiction it's fortify. |
|
Hmm. The only unusual things this package does is set |
|
Yeah, vanilla unpatched |
|
You could try using the |
|
Finally bisected it down to 0eedcfc That explains at least the warning change. I'll work on a natural fix. |
|
Proposed the change as #217379 |
|
I still don't understand how this PR is related, because it doesn't actually enable |
|
Good point. Could it be that we lost --- a/pkgs/development/libraries/libomxil-bellagio/default.nix
+++ b/pkgs/development/libraries/libomxil-bellagio/default.nix
@@ -27,6 +27,8 @@ stdenv.mkDerivation rec {
if stdenv.cc.isGNU then "-Wno-error=array-bounds -Wno-error=stringop-overflow=8"
else "-Wno-error=absolute-value -Wno-error=enum-conversion -Wno-error=logical-not-parentheses -Wno-error=non-literal-null-conversion";
+ NIX_DEBUG="1";
+
meta = with lib; {
homepage = "https://omxil.sourceforge.net/";
description = "An opensource implementation of the Khronos OpenMAX Integration Layer API to access multimedia components"; |
| if [[ -z "${hardeningEnableMap[fortify3]-}" ]]; then | ||
| unset -v "hardeningEnableMap['fortify']" | ||
| fi | ||
|
|
||
| if (( "${NIX_DEBUG:-0}" >= 1 )); then | ||
| declare -a allHardeningFlags=(fortify stackprotector pie pic strictoverflow format) |
There was a problem hiding this comment.
I wonder if we should add fortify3 flag here to avoid ignorance.
There was a problem hiding this comment.
Yes probably I think the first time I scanned this I assumed it was another default setting, not just debugging output.
|
At this point I should probably reveal #217390 that I've been working on. |
|
...however, I haven't had the resources & time lately to do the rebuild required to test this PR with those tests. |
| if [[ -z "${hardeningEnableMap[fortify3]-}" ]]; then | ||
| unset -v "hardeningEnableMap['fortify']" | ||
| fi |
There was a problem hiding this comment.
If I read it correctly it unsets fortify if fortify3 is not present (i.e. always).
| done | ||
|
|
||
| # make fortify and fortify3 mutually exclusive | ||
| if [[ -z "${hardeningEnableMap[fortify3]-}" ]]; then |
There was a problem hiding this comment.
I think I've got the sense of this flipped. This should be -n shouldn't it?
|
Surprisingly |
`-DFORTIFY_SOURCE=3` managed to expose stack overread problem as:
#212498 (comment)
The change pull fix proposed upstream to avoid stack overread.
Description of changes
https://developers.redhat.com/articles/2022/09/17/gccs-new-fortification-level
GCC 12 makes available the new
-D_FORTIFY_SOURCE=3mode, which significantly improves coverage over the existing protections. Other distributions have reportedly already enabled the flag universally with few problems. It would be nice to be ready to enable this flag once we've switched to GCC 12 as our default compiler.The problem in doing this however is that the existing hardening flag mechanisms are based around flags being purely boolean and independent - we've never had to handle flags with multiple modes before.
After a lot of thought, what I've done here is to introduce a new flag,
fortify3.fortifycontinues to meanFORTIFY_SOURCE=2mode - its behaviour remains unchanged for backwards compatibility. There are a couple of special handling rules to deconflict the meaning of the two:fortify3in a positive sense (i.e.hardeningEnable,defaultHardeningFlags,NIX_HARDENING_ENABLE),fortify3supersedes plainfortify.fortifyin a negative sense (i.e.hardeningDisable,hardeningUnsupportedFlags,hardening_unsupported_flags),fortifyimplies thatfortify3should also be disabled.Interestingly, the "negative" rule has to be implemented twice, once at the nix-level for
hardeningDisableand once at the bash-level forhardening_unsupported_flags.In this PR,
fortify3is then added tohardeningUnsupportedFlagsfor all compilers but GCC 12 (importantly also the bootstrap compilers). According to the above article, clang also supposedly supportsFORTIFY_SOURCE=3, but reliable information on clang's fortify-support at any level is very hard to come by so I'm not going down that rabbit hole.I've done a lot of building on nixos x86_64 with this:
fortify3enabled by default (came across no failures)fortify3enabled by default & default compiler GCC 12 (a couple of failures, fixes included)I was very surprised at how ready both GCC 12 and
fortify3support appear to be, at least for nixos x86_64.I also built a substantial amount on x86_64 darwin, including packages using
gccStdenv, not finding any breakages.This PR itself shouldn't change how any packages are built as it neither adds
fortify3to thedefaultHardeningFlagsor has a default compiler that supportsfortify3, but once we're ready we'll have to add a straightforward commit such as risicle@665b354 to do so (:point_left: consider testing with this cherry-picked on top)Edit: have also built some
pkgsMusl,pkgsStaticandpkgsCross.aarch64-multiplatformpackages on x86_64 linux under various configurations of compiler/fortify3-enablement/disablement.Things done
sandbox = trueset innix.conf? (See Nix manual)nix-shell -p nixpkgs-review --run "nixpkgs-review rev HEAD". Note: all changes have to be committed, also see nixpkgs-review usage./result/bin/)nixos/doc/manual/md-to-db.shto update generated release notes