stdenv: fix hardeningEnable with __structuredAttrs = true;

[Ma27]

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;.

I figured that it's far easier to just write a space-separated string for NIX_HARDENING_ENABLE into env if needed rather than changing the code in add-hardening.sh which would've caused a full rebuild. The flags are well-known identifiers anyways, so there's no risk of issues coming from missing escapes.

[1] #294504
[2] #294504 (comment)
[3]

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

Line 9 in cf3e5d3

for flag in ${NIX_HARDENING_ENABLE_@suffixSalt@-}; do

[4]

nixpkgs/pkgs/build-support/cc-wrapper/setup-hook.sh

Line 114 in cf3e5d3

: ${NIX_HARDENING_ENABLE="@default_hardening_flags_str@"}

---

cc @kirillrdy for me, the postgresql from this branch looks better. Can you confirm that your issue is resolved?
cc @NixOS/nixos-release-managers I'm pretty sure this is a blocker for 24.11.
cc @NixOS/security

Things done

• Built on platform(s)
  • x86_64-linux
  • aarch64-linux
  • x86_64-darwin
  • aarch64-darwin
• For non-Linux: Is sandboxing enabled in nix.conf? (See Nix manual)
  • sandbox = relaxed
  • sandbox = true
• 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/)
• 24.11 Release Notes (or backporting 23.11 and 24.05 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.

---

Add a 👍 reaction to pull requests you find important.

[Ma27]

This is actually needed to prevent an infinite recursion.

[Ma27]

This is pretty nasty because it removes each enabledHardeningOptions set in a call to stdenv.mkDerivation.
Any suggestions on how to improve that?

[wolfgangwalther]

I figured that it's far easier to just write a space-separated string for NIX_HARDENING_ENABLE into env if needed rather than changing the code in add-hardening.sh which would've caused a full rebuild. The flags are well-known identifiers anyways, so there's no risk of issues coming from missing escapes.

Hm. We have an ongoing effort for a few months now to try to make as much of the bash code work with both structuredAttrs on and off - without putting structuredAttrs checks all over the place. Started in #318614, tracked in #205690.

I can't say anything about the timeline and the feasibility of a full rebuild right now, but we should consider how to solve it without putting those checks in various places and rather make the code using work with strings and arrays alike.

[emilazy]

(FWIW I would love it if we could turn structured attributes on by default for 25.05! Though it’s not the kind of thing we should do late in the cycle, so it’s fine if it slips.)

Edit: Ah, though I guess this is just about a stdenv rebuild vs. Nix eval‐time? I think that can just go in staging, we’re rebuilding stdenvs next cycle anyway…

[wolfgangwalther]

In other words, this only applies to derivations that have both custom hardening settings and __structuredAttrs = true;.

We have this case 4 times on master currently:

• librandombytes
• scalapack
• postgresql
• texlive

The case for postgresql was introduced in this cycle and is understood, but we'll need to look at the other cases. Seems like the respective hardening settings should not take effect right now as well?

[wolfgangwalther]

Edit: Ah, though I guess this is just about a stdenv rebuild vs. Nix eval‐time? I think that can just go in staging, we’re rebuilding stdenvs next cycle anyway…

Will that be still in 24.11 then? I think we want to avoid a performance regression for PostgreSQL for the release.

[Ma27]

Hm. We have an ongoing effort for a few months now to try to make as much of the bash code work with both structuredAttrs on and off - without putting structuredAttrs checks all over the place. Started in #318614, tracked in #205690.

So, my gut feeling is that with this being effectively a release blocker and an approach existing to solve this without a full rebuild, I think we should go that way (unless the RMs are sure that a full rebuild is OK and are willing to delay the release if it doesn't work out -- but the state postgresql is currently in is not acceptable for a stable release IMHO).

That said, see 1e84a7f for even more shortcomings. I'm willing to follow up with a changes in the bash code and a revert of the Nix changes after branch-off[1].

I think we want to avoid a performance regression for PostgreSQL for the release.

I think missing compiler hardenings is also a big no no.

I think that can just go in staging, we’re rebuilding stdenvs next cycle anyway…

As long as we're willing to delay 24.11 if we get delays in stabilizing staging, I'm fine with that as well.
I'll prepare a patch that solves this on bash-level.

[1] I've been helping with earlier __structuredAttrs efforts already, so good to see that there's a tracking issue - I guess I may take a look if I can help.

[emilazy]

The stdenv rebuild stuff that’s queued in staging for Darwin is critical for 24.11, so IMO it’s fine unless the change itself is going to be particularly risky in some way, we’re already eating those builds.

[Ma27]

OK, great. Already prepared a change for this that only touches add-hardening.sh. Currently bootstrapping the stdenv to run the hardening tests (the one I added here fails without the hardening fix).

[wolfgangwalther]

OK, great. Already prepared a change for this that only touches add-hardening.sh.

Can you open a draft PR with that already? I think we might need to look at all places where NIX_HARDENING_ENABLE is used.

[Ma27]

Opened #353142.

Was a good idea to do so, while preparing the commit I realized that I messed up the if in the CC wrapper 🙈

[risicle]

Broadly seems sensible to me.

One thing I think I've missed is the reason for exposing enabledHardeningOptions to the outside world. If we were going to do that we'd have to think hard about how it would interact with hardeningEnable and hardeningDisable in packages that also specify those - possibly adding some validation that detects contradictory combinations. If at all possible, I'd suggest not opening this can of worms at all.

Also if we were going to allow caller-specified enabledHardeningOptions, I'd suggest renaming the enabledHardeningOptions declaration in the later let block to enabledHardeningOptions' to make it clear which is being referenced in any scope.

(or am I totally misinterpreting the impact of adding it to makeDerivationArgument?)

[risicle]

putting structuredAttrs checks all over the place

Couldn't this be achieved by simply unconditionally manually converting NIX_HARDENING_ENABLE to space-separated strings somewhere in mkDerivation and avoid needing to add a check to our wrapper scripts?

[wolfgangwalther]

putting structuredAttrs checks all over the place

Couldn't this be achieved by simply unconditionally manually converting NIX_HARDENING_ENABLE to space-separated strings somewhere in mkDerivation

So essentially this PR minus the lib.optionalAttrs __structuredAttrs in

  env = (attrs.env or {}) // lib.optionalAttrs __structuredAttrs {
    NIX_HARDENING_ENABLE = builtins.concatStringsSep " " enabledHardeningOptions;
  };

(and then remove the other NIX_HARDENING_ENABLE = enabledHardeningOptions; line / block)

Yeah, that should work as well, I guess?

and avoid needing to add a check to our wrapper scripts?

Well, the idea would be to avoid checks in bash scripts, too, see discussion in #353142.

But treating NIX_HARDENING_ENABLE as an environment variable (which can only be string both with and without structuredAttrs) instead of as a "derivation argument" (not sure about the right term here) is certainly less invasive?

[Ma27]

One thing I think I've missed is the reason for exposing enabledHardeningOptions to the outside world. If we were going to do that we'd have to think hard about how it would interact with hardeningEnable and hardeningDisable in packages that also specify those - possibly adding some validation that detects contradictory combinations. If at all possible, I'd suggest not opening this can of worms at all.

AFAIU mkDerivationArgument isn't exposed at all?
This is only done because we need to evaluate the hardening settings before handing over to makeDerivationArgument which takes a validated env already (my understanding at least - the code there is kinda nasty to read IMHO).

Anyways, I think there's consensus to try a different approach.

I think that your suggestion with just concatenating NIX_HARDENED_ENABLE is reasonable now that we've agreed on going through staging anyways. I'll try that out to see if I missed a reason against that and push to #353142 then.

[kirillrdy]

running nixpkgs-review

        at /home/kirillvr/.cache/nixpkgs-review/pr-353131/nixpkgs/pkgs/stdenv/generic/make-derivation.nix:554:27:
          553|
          554|   env = (attrs.env or {}) // lib.optionalAttrs __structuredAttrs {
             |                           ^
          555|     NIX_HARDENING_ENABLE = builtins.concatStringsSep " " enabledHardeningOptions;

       error: expected a set but found a string with context: "/nix/store/7awmks5w8v9yjgqbpmv3qa79k2qqg52i-wee-slack-env/lib/python3.12/site-packages"

[kirillrdy]

@Ma27 this indeed fixes regression in postgresql !

[Ma27]

Let's move the discussion to #353142: just pushed a way easier version there that also passes the hardening test-case I wrote and doesn't have the issues we've seen here (at the cost of a full rebuild).

[Ma27]

Closing in favor of #353142.