stdenv: fix custom hardening settings when using `__structuredAttrs = true;` #353142

Ma27 · 2024-11-02T14:51:16Z

Alternative to #353131

Things done

Add a 👍 reaction to pull requests you find important.

pkgs/build-support/cc-wrapper/add-hardening.sh

wolfgangwalther · 2024-11-02T15:04:12Z

We should also look at the following:

pkgs/build-support/bintools-wrapper/add-hardening.sh:for flag in ${NIX_HARDENING_ENABLE_@suffixSalt@-}; do

pkgs/build-support/bintools-wrapper/add-flags.sh:    NIX_HARDENING_ENABLE

pkgs/build-support/cc-wrapper/fortran-hook.sh:: ${NIX_HARDENING_ENABLE="@default_hardening_flags_str@"}
pkgs/build-support/cc-wrapper/setup-hook.sh:: ${NIX_HARDENING_ENABLE="@default_hardening_flags_str@"}
pkgs/build-support/bintools-wrapper/setup-hook.sh:: ${NIX_HARDENING_ENABLE="@default_hardening_flags_str@"}

pkgs/applications/networking/cluster/linkerd/generic.nix:    if [[ $NIX_HARDENING_ENABLE =~ "pie" ]]; then
pkgs/build-support/go/module.nix:      if [[ $NIX_HARDENING_ENABLE =~ "pie" ]]; then
pkgs/build-support/go/package.nix:      if [[ $NIX_HARDENING_ENABLE =~ "pie" ]]; then

pkgs/development/compilers/graalvm/community-edition/buildGraalvm.nix:          "-ENIX_HARDENING_ENABLE"
pkgs/development/compilers/swift/foundation/default.nix:    NIX_HARDENING_ENABLE=''${NIX_HARDENING_ENABLE/fortify/}
pkgs/development/libraries/libffi/3.3.nix:    NIX_HARDENING_ENABLE=''${NIX_HARDENING_ENABLE/fortify/}
pkgs/development/libraries/libffi/default.nix:    NIX_HARDENING_ENABLE=''${NIX_HARDENING_ENABLE/fortify3/}
pkgs/development/libraries/libffi/default.nix:    NIX_HARDENING_ENABLE=''${NIX_HARDENING_ENABLE/fortify/}
pkgs/development/tools/misc/ltrace/default.nix:    NIX_HARDENING_ENABLE="" \
pkgs/stdenv/generic/make-derivation.nix:      NIX_HARDENING_ENABLE = enabledHardeningOptions;

(stripped the grep output for trivial stuff)

Ma27 · 2024-11-02T15:22:03Z

pkgs/build-support/cc-wrapper/fortran-hook.sh:: ${NIX_HARDENING_ENABLE="@default_hardening_flags_str@"}
pkgs/build-support/cc-wrapper/setup-hook.sh:: ${NIX_HARDENING_ENABLE="@default_hardening_flags_str@"}
pkgs/build-support/bintools-wrapper/setup-hook.sh:: ${NIX_HARDENING_ENABLE="@default_hardening_flags_str@"}

We're only setting defaults here: I think it's OK to continue using strings here until we kill support for non-structuredAttrs (if that ever happens), right?

pkgs/development/tools/misc/ltrace/default.nix: NIX_HARDENING_ENABLE="" \

turns off hardening for checkPhase. Same as above, as long as we support strings, that's OK I think.

pkgs/build-support/bintools-wrapper/add-hardening.sh:for flag in ${NIX_HARDENING_ENABLE_@suffixSalt@-}; do

This is the line we're currently fixing.

pkgs/build-support/bintools-wrapper/add-flags.sh: NIX_HARDENING_ENABLE

Hmm, this will indeed break (I think).
Will look into it.

Regarding the rest: what's the suggested course of action here? These will need to be touched when structuredAttrs=true is the new default or shall we do that earlier (haven't thought about the how though).

wolfgangwalther · 2024-11-02T15:31:26Z

pkgs/build-support/bintools-wrapper/add-hardening.sh:for flag in ${NIX_HARDENING_ENABLE_@suffixSalt@-}; do

This is the line we're currently fixing.

No, I omitted the line we are fixing already. This is exactly the same lin in a different file.

pkgs/build-support/bintools-wrapper/add-flags.sh: NIX_HARDENING_ENABLE

Hmm, this will indeed break (I think). Will look into it.

I started to look into that, but I really don't get what mangleVarList does and can't tell whether it's affected or not.

Regarding the rest: what's the suggested course of action here? These will need to be touched when structuredAttrs=true is the new default or shall we do that earlier (haven't thought about the how though).

I'd say we don't need to touch anything that isn't structuredAttrs=true right now. We can do that later. Many of those seem to be specific to that derivation, so not critical until that derivation becomes structured.

We should still look into those two, though:

pkgs/build-support/go/module.nix:      if [[ $NIX_HARDENING_ENABLE =~ "pie" ]]; then
pkgs/build-support/go/package.nix:      if [[ $NIX_HARDENING_ENABLE =~ "pie" ]]; then

It could very well be that some go modules/packages are built with structuredAttrs on, in which case this is relevant, I think.

wolfgangwalther · 2024-11-02T15:40:11Z

We should still look into those two, though:
pkgs/build-support/go/module.nix:      if [[ $NIX_HARDENING_ENABLE =~ "pie" ]]; then
pkgs/build-support/go/package.nix:      if [[ $NIX_HARDENING_ENABLE =~ "pie" ]]; then
It could very well be that some go modules/packages are built with structuredAttrs on, in which case this is relevant, I think.

And there is one more thing wrong with this check, too: if "pie" is part of a longer hardening flag, e.g. "applepie", this will do the wrong thing.

So I think we should do the following for this:

if [[ " ${NIX_HARDENING_ENABLE[*]} " =~ " pie " ]]; then

This works with strings and arrays and avoids the applepie problem.

wolfgangwalther · 2024-11-02T15:47:55Z

It could very well be that some go modules/packages are built with structuredAttrs on

I can't find any in nixpkgs, but we should consider still fixing it now, because some out of tree users might do so.

… true;` Replaces / Closes NixOS#353131 A while ago `postgresql` switched to using structured attrs[1]. In the PR it was reported that this made postgresql notably slower when importing SQL dumps[2]. After a bit of debugging it turned out that the hardening was entirely missing and the following combination of settings was the culprit: hardeningEnable = [ "pie" ]; __structuredAttrs = true; I.e. the combination of custom hardening settings and structured attrs. What happened here is that internally the default and enabled hardening flags get written into `NIX_HARDENING_ENABLE`. However, the value is a list and the setting is not in the `env` section. This means that in the structured-attrs case we get something like declare -ax NIX_HARDENING_ENABLE=([0]="bindnow" [1]="format" [2]="fortify" [3]="fortify3" [4]="pic" [5]="relro" [6]="stackprotector" [7]="strictoverflow" [8]="zerocallusedregs" [9]="pie") i.e. an actual array rather than a string with all hardening flags being space-separated which is what the hardening code of the cc-wrapper expects[3]. This only happens if `hardeningEnable` or `hardeningDisable` are explicitly set by a derivation: if none of those are set, `NIX_HARDENING_ENABLE` won't be set by `stdenv.mkDerivation` and the default hardening flags are configured by the setup hook of the cc-wrapper[4]. In other words, this _only_ applies to derivations that have both custom hardening settings _and_ `__structuredAttrs = true;`. All values of `NIX_HARDENING_ENABLE` are well-known, so we don't have to worry about escaping issues. Just forcing it to a string by concatenating the list everytime solves the issue without additional issues like eval errors when inheriting `env` from a structuredAttrs derivation[5]. The price we're paying is a full rebuild. [1] NixOS#294504 [2] NixOS#294504 (comment) [3] https://github.com/NixOS/nixpkgs/blob/cf3e5d3744dc26c3498aa5dadfa0e078c632cede/pkgs/build-support/cc-wrapper/add-hardening.sh#L9 [4] https://github.com/NixOS/nixpkgs/blob/cf3e5d3744dc26c3498aa5dadfa0e078c632cede/pkgs/build-support/cc-wrapper/setup-hook.sh#L114 [5] NixOS@1e84a7f

wolfgangwalther · 2024-11-02T21:45:27Z

pkgs/stdenv/generic/make-derivation.nix

@@ -413,7 +413,7 @@ else let
      enableParallelChecking = attrs.enableParallelChecking or true;
      enableParallelInstalling = attrs.enableParallelInstalling or true;
    } // optionalAttrs (hardeningDisable != [] || hardeningEnable != [] || stdenv.hostPlatform.isMusl) {
-      NIX_HARDENING_ENABLE = enabledHardeningOptions;


I'm confused why this seems to work without env.. Does it?

I would have expected structuredAttrs to require passing it via env.

Aren't non-env things also exposed as bash variables in the builder, just not into the environment?
I mean, the NIX_HARDENING_ENABLE part was seen by add-hardening.sh before, but itw as an actual array, not a string.

Aren't non-env things also exposed as bash variables in the builder, just not into the environment?

Kind of an "aha" moment for me. But yeah env = "environment variables". I had quite a misconception here. Not sure what actually.

kirillrdy · 2024-11-03T04:11:48Z

can confirm that this PR fixes postgresql performance regression

Ma27 · 2024-11-03T10:30:19Z

So, IMHO this is ready now: it has a regression test that broke for me on master and passes on this commit and seems like the least disruptive change (except for the rebuild amount of course).

cc @risicle @emilazy @wolfgangwalther
also cc @vcunat as fyi that there will be a fat rebuild on staging that needs to land in 24.11.

wolfgangwalther

Just finished rebuilding the world - LGTM.

I confirmed the test breaks on master, passes on this branch. I also played around with the test, changing stuff to confirm it works as intended.

I double checked manually, that the NIX_HARDENING_ENABLE variable is consistently set with both structuredAttrs and without.

I am still building the 4 packages that we know of which set hardening flags manually and have structuredAttrs enabled already:

librandombytes
scalapack
postgresql
texlive

Build failures here shouldn't stop us from doing this fix, though. We'd need to fix packages, too, but we can proceed with this PR already.

wolfgangwalther · 2024-11-03T12:59:32Z

I am still building the 4 packages that we know of which set hardening flags manually and have structuredAttrs enabled already:

librandombytes

scalapack

postgresql

texlive

Those all built fine for me. Let's ship this.

risicle · 2024-11-03T15:51:14Z

This reminded me that I'd really love to get the faster uniq parts of #119286 into lib so that I'd feel comfortable using unique filters more extensively in the hardening flags so we end up with a normalized result in NIX_HARDENING_ENABLE. This could avoid rebuilds arising from e.g. specifying a flag in hardeningEnable that's already enabled by default or adding one that's not supported on the platform to hardeningDisable.

vcunat · 2024-11-04T09:24:24Z

fyi that there will be a fat rebuild on staging that needs to land in 24.11.

Rebuilding everything is fine. I think we had a stdenv rebuild in master..staging already anyway. Now it's primarily being careful about possible regressions ;-)

Ma27 mentioned this pull request Nov 2, 2024

stdenv: fix hardeningEnable with __structuredAttrs = true; #353131

Closed

13 tasks

Ma27 marked this pull request as draft November 2, 2024 14:52

nix-owners bot requested a review from Ericson2314 November 2, 2024 14:52

wolfgangwalther reviewed Nov 2, 2024

View reviewed changes

pkgs/build-support/cc-wrapper/add-hardening.sh Outdated Show resolved Hide resolved

Ma27 force-pushed the structuredAttrs-hardening-bash branch from c84bb84 to aaeeef5 Compare November 2, 2024 21:36

github-actions bot added the 6.topic: stdenv Standard environment label Nov 2, 2024

Ma27 marked this pull request as ready for review November 2, 2024 21:37

nix-owners bot requested review from Artturin, emilazy, philiptaron, reckenrode and RossComputerGuy November 2, 2024 21:38

wolfgangwalther reviewed Nov 2, 2024

View reviewed changes

Ma27 added this to the 24.11 milestone Nov 3, 2024

wolfgangwalther approved these changes Nov 3, 2024

View reviewed changes

philiptaron merged commit a307173 into NixOS:staging Nov 3, 2024
14 of 16 checks passed

Ma27 deleted the structuredAttrs-hardening-bash branch November 3, 2024 13:12

wolfgangwalther mentioned this pull request Nov 3, 2024

[Tracking Issue] __structuredAttrs TODOs and FIXMEs #205690

Open

2 tasks

ofborg bot added 8.has: package (new) This PR adds a new package 10.rebuild-darwin: 101-500 10.rebuild-linux: 501+ labels Nov 3, 2024

ofborg bot added the 10.rebuild-linux: 5001+ label Nov 3, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

stdenv: fix custom hardening settings when using `__structuredAttrs = true;` #353142

stdenv: fix custom hardening settings when using `__structuredAttrs = true;` #353142

Ma27 commented Nov 2, 2024

wolfgangwalther commented Nov 2, 2024 •

edited

Loading

Ma27 commented Nov 2, 2024

wolfgangwalther commented Nov 2, 2024

wolfgangwalther commented Nov 2, 2024

wolfgangwalther commented Nov 2, 2024

wolfgangwalther Nov 2, 2024

Ma27 Nov 2, 2024

wolfgangwalther Nov 2, 2024

kirillrdy commented Nov 3, 2024

Ma27 commented Nov 3, 2024

wolfgangwalther left a comment

wolfgangwalther commented Nov 3, 2024

risicle commented Nov 3, 2024

vcunat commented Nov 4, 2024

stdenv: fix custom hardening settings when using __structuredAttrs = true; #353142

stdenv: fix custom hardening settings when using __structuredAttrs = true; #353142

Conversation

Ma27 commented Nov 2, 2024

Things done

wolfgangwalther commented Nov 2, 2024 • edited Loading

Ma27 commented Nov 2, 2024

wolfgangwalther commented Nov 2, 2024

wolfgangwalther commented Nov 2, 2024

wolfgangwalther commented Nov 2, 2024

wolfgangwalther Nov 2, 2024

Choose a reason for hiding this comment

Ma27 Nov 2, 2024

Choose a reason for hiding this comment

wolfgangwalther Nov 2, 2024

Choose a reason for hiding this comment

kirillrdy commented Nov 3, 2024

Ma27 commented Nov 3, 2024

wolfgangwalther left a comment

Choose a reason for hiding this comment

wolfgangwalther commented Nov 3, 2024

risicle commented Nov 3, 2024

vcunat commented Nov 4, 2024

stdenv: fix custom hardening settings when using `__structuredAttrs = true;` #353142

stdenv: fix custom hardening settings when using `__structuredAttrs = true;` #353142

wolfgangwalther commented Nov 2, 2024 •

edited

Loading