hardening flags: enable fortify3 by default

[risicle]

Description of changes

See #212498 where fortify3 support was added for more information.

This is mostly to get the discussion going ahead of 23.05. I personally think we should follow other distributions' lead (fedora and opensuse) in enabling it by default - they have reported few if any problems with it AFAIK.

See #217390 for some tests around this and #219421 for an attempt to add basic fortify support for musl.

Things done

• Built on platform(s)
  • x86_64-linux
  • aarch64-linux
  • x86_64-darwin
  • aarch64-darwin
• For non-Linux: Is sandbox = true set in nix.conf? (See Nix manual)
• Tested, as applicable:
  • NixOS test(s) (look inside nixos/tests)
  • and/or package tests
  • or, for functions and "core" functionality, tests in lib/tests or pkgs/test
  • made sure NixOS tests are linked to the relevant packages
• Tested compilation of all packages that depend on this change using nix-shell -p nixpkgs-review --run "nixpkgs-review rev HEAD". Note: all changes have to be committed, also see nixpkgs-review usage
• Tested basic functionality of all binary files (usually in ./result/bin/)
• 23.05 Release Notes (or backporting 22.11 Release notes)
  • (Package updates) Added a release notes entry if the change is major or breaking
  • (Module updates) Added a release notes entry if the change is significant
  • (Module addition) Added a release notes entry if adding a new NixOS module
• Fits CONTRIBUTING.md.

[trofi]

I think it's a reasonable change.

There is a mild chance to produce crashing binaries as a result, but it should be fixable on one-by-one case and should not be too widespread (my guesstimate is less than 10 affected packages).

[vcunat]

This is meant to miss 23.05, right?

[risicle]

This is meant to miss 23.05, right?

Open for discussion as far as I'm concerned.

[vcunat]

FYI, freeze for similar kind of stuff is within two weeks latest.

[lheckemann]

I'm very much in favour of this, but I think I prefer merging this after the branch-off so that we don't end up having to put a lot of fixes in both 23.05 and master after the release, so this lands in 23.11 having had plenty of testing on unstable.

[risicle]

(just a straight rebase)

[emilazy]

Some links to information/discussion about this:

• https://developers.redhat.com/articles/2023/02/06/how-improve-application-security-using-fortifysource3
• https://fedoraproject.org/wiki/Changes/Add_FORTIFY_SOURCE%3D3_to_distribution_build_flags
• https://pagure.io/fesco/issue/2917

Since performance came up on Matrix: per the above links, there is no measured impact on the SPEC2000/SPEC2017 benchmarks (in fact a slight improvement in parts of the former), and the average binary size change is ~0.00%. Any performance impacts would be edge-cases where the compiler cannot hoist/optimize away the checks inside a tight loop. Since this is on track for wide adoption (accepted by Fedora, already enabled for OpenSUSE ALP and for Gentoo's hardened profile, not sure about Debian/Ubuntu) I think any such relevant edge-cases will be found regardless of what we do; I believe the benefits are clearly worth the extremely minimal cost.

[vcunat]

Thanks for providing the references. I think I have seen something like that but I couldn't find it on the links at the top of this PR.

Here's a -small evaluation: https://hydra.nixos.org/eval/1797023?compare=1797074#tabs-now-fail
if you disregard the timeouts, it already did find some build regression(s). Any idea if most of the regressions we expect will be trivial to resolve? (in which case we could most likely handle them during staging-next, as a separate jobset would be much more stress on the infra and probably slower, too)

[risicle]

Those all seem to trickle back to libfido2. Have pushed (but not tested) a disabling of fortify3 there.

[risicle]

Any idea if most of the regressions we expect will be trivial to resolve?

Personally I would just add hardeningDisable = [ "fortify3" ]; to them

[prusnak]

libfido2 changes ACK

[vcunat]

I went through the references, and it really does sound good to me.

And yes, on my last post I somehow forgot that recognized issues should be trivially solved by that line, so I think it's better to deal with regressions during staging-next instead of a separate jobset.

[vcunat]

Those redefinition errors happen in quite a lot of packages. So disabling fortify3 really seems the best way for them? (before "we" copy&paste it on so many places)

[emilazy]

Is it an actual incompatibility or packages trying to "opt in" to _FORTIFY_SOURCE at a lower level? Maybe we can override them and only disable it on packages it actually breaks.

[vcunat]

Of course I meant disabling individually, like the second commit in this PR.

[emilazy]

Sorry, rephrase: maybe we can handle the cases where _FORTIFY_SOURCE is being redefined by figuring out some way to automatically override it with level 3, and then the only packages that would need modification/opting out would be ones where fortify3 actually breaks things regardless?

[vcunat]

I suspect that some global -Wno-error=foo would work, at least on some of the cases.

[vcunat]

Maybe some inspiration could be found in Fedora or OpenSUSE, too 🤷🏽

[emilazy]

It looks like in the case of libfido2, it's just passing -D_FORTIFY_SOURCE=2 as a flag to the compiler. Maybe the wrapper can filter those out when fortify3 is enabled.

[risicle]

These warnings are quite widespread and annoying. I also note that, because our hardening flags are prepended to the invocation's command line, in cases where the build process also sets _FORTIFY_SOURCE their setting wins.

So.. a few options to resolve this:

• Detect -D_FORTIFY_SOURCE= in the invocation's params and simply omit adding our own -D_FORTIFY_SOURCE= in this case, allowing their setting to win, but suppressing the warnings (this would be the simplest implementation and the -pie handling already inspects params in add-hardening.sh so we wouldn't be the first..).
• Scrub the invocation's params to remove -D_FORTIFY_SOURCE= flags, effectively forcing our setting.
• Split hardeningCFlags into before and after arrays, adding our -U_FORTIFY_SOURCE/-D_FORTIFY_SOURCE= combo to the after array, forcing our setting.

Opinions? I think in principal I'd prefer forcing our setting, but the simplicity of the first option is seductive and is closest to what is already happening.

[vcunat]

I expect that packages setting this to 2 didn't mean to lower fortification and won't cause issues when 3 is forced. (in 99% cases)

[trofi]

Fun fact: vim does use -D_FORTIFY_SOURCE=1 because anything above breaks it (without any plans to fix it): vim/vim#5581

With more widespread -D_FORTIFY_SOURCE=3 we'll probably see a small amount of projects backing to -D_FORTIFY_SOURCE=2 as well.

[risicle]

See #246067 for an implementation of the first (least intrusive) option.

[risicle]

In #246067, @amjoseph-nixpkgs and @emilazy seem to prefer the alternative idea of appending our -D_FORTIFY_SOURCE flag, however I'm a bit concerned about how few things cc-wrapper currently adds to the end of the command line. The general pattern is to let the submitted command-line "win".

In particular I'm thinking less about cc as used in derivations, but more cc used as a command-line development tool. Trying to test something with e.g. -D_FORTIFY_SOURCE=0 and having it silently disobeyed would be a confusing and annoying behaviour to developers. Sure, you can control it with the NIX_HARDENING_ENABLE env var.. but only if you know about it.

Still, I'll put up a PR with that approach..

[emilazy]

Hmm, the command-line usage example does point to a tension here, since my intuitive expectation there would be that it overrides. It's arcane enough that I'm not sure whether it overrides my concern about packages silently (or deliberately due to log noise) getting downgraded hardening, though. The Nix toolchains are already a little weird for non-Nix use.

I think ideally we would get upstream to stop overriding hardening flags or patch them out ourselves, but I realize that that's a lot of work.

[risicle]

Have added some tests covering the flags-before/after behaviour to #217390

[trofi]

Caused rtorrent regression. Possible fix: #247086

[risicle]

#246244 is currently looking like the least-unpopular option.