mkDerivation, bintools-wrapper: move `defaultHardeningFlags` determination to `bintools-wrapper` by risicle · Pull Request #259070 · NixOS/nixpkgs

risicle · 2023-10-04T21:49:01Z

Description of changes

New hardening flags on the horizon look like they're going to be a bit less "universal" and possibly have more tradeoffs (CET/CFI et al). So it will be useful to make custom selection of hardening flags as convenient as possible to work with. It would also be nice to make it easier for users to experiment with lesser-used hardening schemes in the hopes they'll get more exposure and bug squashing.

mkDerivation has always been an awkward place for defaultHardeningFlags to be determined - these changes make it a lot easier to create a modified stdenv with a different set of defaultHardeningFlags and as a bonus they allow us to inject the correct defaultHardeningFlags into toolchain wrapper scripts, reducing repetition.

While most hardening flags are arguably more of a c-compiler thing, it works better to put them in bintools-wrapper because cc-wrapper can easily refer to bintools but not vice-versa.

mkDerivation can still easily refer to either when it is constructed.

Note this is still different from a compiler's "supported" hardening flags (expressed via cc.hardeningUnsupportedFlags) which are used to filter out particular hardening flags at invocation time.

This also adds stdenvAdapters.withDefaultHardeningFlags as a demonstration of the new abilities.

Things done

LunNova · 2023-10-12T21:37:14Z

Is there any advantage to keeping defaults that get used in the build-support scripts when NIX_HARDENING_ENABLE is unset rather than unconditionally setting it? Seems more complicated substituting it into them like this.

LunNova · 2023-10-12T21:46:06Z

@ofborg build stdenv.cc bash

LunNova · 2023-10-12T21:47:25Z

Looks like mismatched defaults are still sneaking through in pkgs/build-support/cc-wrapper/fortran-hook.sh

risicle · 2023-10-12T22:17:28Z

Is there any advantage to keeping defaults that get used in the build-support scripts when NIX_HARDENING_ENABLE is unset rather than unconditionally setting it? Seems more complicated substituting it into them like this.

🤔 Defaults need to exist at some level (even if those defaults are "empty").

And remember, the cc & bintools wrappers don't just get used via mkDerivation - they are also used whenever a user uses a nix-supplied compiler. Deciding that in a vanilla shell environment, a nix-supplied compiler won't add any extra hardening flags would be a significant behaviour change. And significant behaviour changes are something I'm trying to avoid.

Looks like mismatched defaults are still sneaking through in pkgs/build-support/cc-wrapper/fortran-hook.sh

Damn that complicates things.

LunNova · 2023-10-12T22:25:42Z

So we have to substitute in to keep them in sync for usage in shells rather than package builds because otherwise those would have no default at all, got it.

I still think we should unconditionally set NIX_HARDENING_ENABLE in stdenv.mkDerivation on top of this change that substitutes in the correct defaults.
The current situation where it gets set for musl stdenvs or if either of hardeningDisable/hardeningEnable have something in them feels really janky. Is there any benefit we get out of that arrangement?

risicle · 2023-10-12T22:28:52Z

Looks like mismatched defaults are still sneaking through in pkgs/build-support/cc-wrapper/fortran-hook.sh

This is a problem for your PR too, because having an always-set NIX_HARDENING_ENABLE would make the for fortran the same as for the c compiler, which they're (apparently) deliberately not.

risicle · 2023-10-12T22:54:11Z

Is there any benefit we get out of that arrangement?

I think the only one is that it allows the set of default hardening flags to be different for fortran, but this of course falls apart the second someone sets e.g. hardeningEnable and it reverts to a NIX_HARDENING_ENABLE based on the mkDerivation defaults.

LunNova · 2023-10-12T23:16:01Z

#88449 / abfbbb7 seems to be the reason fortran uses different flag defaults like this. It's very fragile right now.

risicle · 2023-10-13T01:02:32Z

It looks to me like what they really wanted to do was filter out fortify and format from the fortran wrapper instead of just removing them from the defaults. And that's fine by me because it would be simpler for us to implement.

From all I can find, fortify and format aren't really applicable to fortran anyway.

LunNova · 2023-10-13T03:03:10Z

It looks like filtering out the flags from NIX_HARDENING_ENABLE means having fortran and c builds in the same shell will make the c build less secure, so it seems incorrect to do that too.

Would it make sense to filter out incompatible flags for a particular language at the point of use instead?

edit: I'm struggling to understand how the hardening flags work with fortran. add-hardening.sh adds the flags to hardeningCFlagsBefore/After. cc-wrapper.sh then uses those in the actual compiler call. Do gfortan calls still go through cc-wrapper.sh? Can we tell at the right point if a particular invocation is for fortran and remove incompatible flags there?

LunNova · 2023-10-13T08:12:21Z

#92412 (comment) - so let's test blas / lapack cross-compile

risicle · 2023-10-13T17:30:50Z

I'm struggling to understand how the hardening flags work with fortran.

Yeah I was too.

Do gfortan calls still go through cc-wrapper.sh

Yes looks like it :)

Can we tell at the right point if a particular invocation is for fortran and remove incompatible flags there?

Maybe. I think at this point it's a matter of seeing which is the least-ugly option.

Then there is of course the hardening_unsupported_flags/cc.hardeningUnsupportedFlags mechanism already present for doing flag filtering, which could surely be repurposed for this.

SuperSandro2000 · 2023-10-14T20:55:16Z

pkgs/build-support/cc-wrapper/default.nix

Suggested change

inherit expand-response-params;

inherit nixSupport;

inherit defaultHardeningFlags;

inherit expand-response-params nixSupport defaultHardeningFlags;

nit

SuperSandro2000 · 2023-10-14T20:59:32Z

pkgs/stdenv/generic/make-derivation.nix

Suggested change

knownHardeningFlags = [

// when updating consider update the default list in bintoos-wrapper

knownHardeningFlags = [

or similar

pkgs/build-support/bintools-wrapper/default.nix

chivay · 2023-10-19T17:05:59Z

pkgs/stdenv/generic/make-derivation.nix

While we're at it, maybe we could also extract hardcoded constants in add-hardening.sh?

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

Line 28 in 8caf3dc

declare -a allHardeningFlags=(fortify stackprotector pie pic strictoverflow format)

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

Line 18 in 8caf3dc

declare -a allHardeningFlags=(pie relro bindnow)

Flippin heck I forgot about those.

Honestly think this is too tricky to do without introducing a bizarre two-way dependency between mkDerivation and the wrappers. And attempting to bring the knownHardeningFlags into the wrappers alongside defaultHardeningFlags causes all sorts of problems around bootstrapping and use of unwrapped compilers/binutils because all of a sudden mkDerivation has an empty knownHardeningFlags which causes it to throw errors if any derivation happens to use hardeningDisable or hardeningEnable.

ghost

While most hardening flags are arguably more of a c-compiler thing, it works better to put them in bintools-wrapper

I'm okay with this if there are any (even just one) hardening flags that are passed to the linker rather than the c-compiler. Is there such a flag?

If the linker really handles zero hardening flags, and is unlikely to handle any in the future, then I think the bintools-wrapper is a pretty unintuitive place to stash this default.

because cc-wrapper can easily refer to bintools but not vice-versa.

Why not make it a top-level non-derivation attribute in the packageset like pkgs.default-gcc-version? Then it can be overridden with a simple overlay rather than having to build a whole new stdenv.

See also my comment below; even if we keep the defaultHardeningFlags in the bintools-wrapper we should make sure the defaulting logic happens in Nix rather than bash so you can change the defaults without rebuilding gcc.

pkgs/build-support/bintools-wrapper/setup-hook.sh

risicle · 2023-10-22T10:58:33Z

I'm okay with this if there are any (even just one) hardening flags that are passed to the linker rather than the c-compiler. Is there such a flag?

There are three already and likely more to come.

Why not make it a top-level non-derivation attribute in the packageset like pkgs.default-gcc-version? Then it can be overridden with a simple overlay rather than having to build a whole new stdenv.

It doesn't feel like there's a huge amount of prior art for polluting pkgs with build-related settings. default-gcc-version feels like it's there because it's used in all-packages.nix itself.

pkgs/build-support/bintools-wrapper/default.nix

risicle · 2023-10-23T21:12:46Z

Thanks for the reviews - my instinct is that it's too close to the release to merge kind of change but I don't know what others think.

ghost · 2023-10-23T22:21:30Z

Thanks for the reviews - my instinct is that it's too close to the release to merge kind of change but I don't know what others think.

I say go for it, and tell @vcunat that if it causes problems just revert it (i.e. if it does cause problems it isn't worth spending significant amount of time to fix them).

vcunat · 2023-10-24T05:27:20Z

I wouldn't merge it for 23.11 (anymore). Certainly not with the only argument that we can revert. There are reasons why we have merging restrictions #258640 (comment)

ghost · 2023-10-24T23:35:48Z

There are reasons why we have merging restrictions

This is not a breaking change.

But waiting until after 23.11 is fine too.

chayleaf · 2023-11-20T07:41:19Z

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

is relro not here on purpose?

That is a very good question. It predates me, but it's also missing bindnow so presumably this debug statement was meant to only print the cc-wrapper-related flags and not the bintools ones?

~~BTW it would be awesome if this list of flags could be changed without causing rebuilds for NIX_DEBUG=0 (which is the default).~~

Edit: nevermind, this is cc-wrapper, I thought it was bintools-wrapper. I'm working on a PR which will accomplish this.

ghost · 2023-11-30T23:02:26Z

Now that staging is branched off we should merge this.

Are there any objections?

risicle · 2023-11-30T23:55:57Z

I'd prefer to wait for the dust to settle a bit more after the release (and for this have more of my attention in case it causes any fallout)

…ion to bintools-wrapper this makes it a lot easier to create a modified stdenv with a different set of defaultHardeningFlags and as a bonus allows us to inject the correct defaultHardeningFlags into toolchain wrapper scripts, reducing repetition. while most hardening flags are arguably more of a compiler thing, it works better to put them in bintools-wrapper because cc-wrapper can easily refer to bintools but not vice-versa. mkDerivation can still easily refer to either when it is constructed. this also switches fortran-hook.sh to use the same defaults for NIX_HARDENING_ENABLE as for C. previously NIX_HARDENING_ENABLE defaults were apparently used to avoid passing problematic flags to a fortran compiler, but this falls apart as soon as mkDerivation sets its own NIX_HARDENING_ENABLE - cc.hardeningUnsupportedFlags is a more appropriate mechanism for this as it actively filters out flags from being used by the wrapper, so switch to using that instead. this is still an imperfect mechanism because it doesn't handle a compiler which has both langFortran *and* langC very well - applying the superset of the two's hardeningUnsupportedFlags to either compiler's invocation. however this is nothing new - cc-wrapper already poorly handles a langFortran+langC compiler, applying two setup hooks that have contradictory options.

risicle · 2023-12-09T22:28:46Z

Let's do this.

Am currently re-testing on macos and linux.

risicle · 2023-12-10T19:04:57Z

Cherry-picking #253186 (which could also do with review) to correct the tests, tests.hardeningFlags do the expected thing for me on nixos x86_64, pkgsCross.aarch64-multiplatform, pkgsLLVM and pkgsi686Linux variants (required packages for those tests appear broken on pkgsMusl & pkgsStatic for now). tests.hardeningFlags tests that are expected to work on darwin (*ExecTest) do on macos 12 x86_64.

risicle · 2023-12-12T21:52:31Z

A release-notes entry for this: #273840

sternenseemann · 2024-01-12T16:50:02Z

Fix for eval regression: #280537

risicle added the 6.topic: stdenv Standard environment label Oct 4, 2023

risicle requested a review from Ericson2314 as a code owner October 4, 2023 21:49

risicle requested a review from a user October 4, 2023 21:49

risicle mentioned this pull request Oct 5, 2023

stdenv: Enable PIE by default #252310

Closed

12 tasks

LunNova mentioned this pull request Oct 12, 2023

tracking issue: by default packages aren't built with -fpie/ASLR support #205031

Closed

6 tasks

risicle force-pushed the ris-cc-default-hardening-flags branch from 6606a10 to 76f7703 Compare October 14, 2023 19:49

SuperSandro2000 reviewed Oct 14, 2023

View reviewed changes

pkgs/build-support/bintools-wrapper/default.nix Outdated Show resolved Hide resolved

risicle mentioned this pull request Oct 18, 2023

stdenv/generic/make-derivation.nix: always set NIX_HARDENING_ENABLE #206490

Closed

13 tasks

chivay reviewed Oct 19, 2023

View reviewed changes

risicle force-pushed the ris-cc-default-hardening-flags branch from 76f7703 to 52b481a Compare October 21, 2023 16:51

ghost reviewed Oct 22, 2023

View reviewed changes

pkgs/build-support/bintools-wrapper/setup-hook.sh Outdated Show resolved Hide resolved

vifino mentioned this pull request Oct 22, 2023

wolfssl: ASM/AES-NI/SP Math support; easier variant override #262594

Merged

13 tasks

ghost approved these changes Oct 23, 2023

View reviewed changes

pkgs/build-support/bintools-wrapper/default.nix Outdated Show resolved Hide resolved

risicle force-pushed the ris-cc-default-hardening-flags branch 2 times, most recently from 8354218 to 49bb1b7 Compare October 23, 2023 19:01

chivay approved these changes Oct 23, 2023

View reviewed changes

LunNova mentioned this pull request Oct 23, 2023

NixOS 23.11 — Release Schedule #258640

Closed

chayleaf reviewed Nov 20, 2023

View reviewed changes

risicle added 2 commits December 9, 2023 16:30

stdenvAdapters: add withDefaultHardeningFlags

dc2247a

risicle force-pushed the ris-cc-default-hardening-flags branch from 49bb1b7 to dc2247a Compare December 9, 2023 16:45

risicle merged commit aac4bc3 into NixOS:staging Dec 10, 2023

risicle mentioned this pull request Dec 12, 2023

release-notes: add note on new ability to set defaultHardeningFlags #273840

Merged

13 tasks

-    inherit expand-response-params;
-    inherit nixSupport;
-    inherit defaultHardeningFlags;
+    inherit expand-response-params nixSupport defaultHardeningFlags;

	knownHardeningFlags = [
	// when updating consider update the default list in bintoos-wrapper
	knownHardeningFlags = [

Uh oh!

Conversation

risicle commented Oct 4, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description of changes

Things done

Uh oh!

LunNova commented Oct 12, 2023

Uh oh!

LunNova commented Oct 12, 2023

Uh oh!

LunNova commented Oct 12, 2023

Uh oh!

risicle commented Oct 12, 2023

Uh oh!

LunNova commented Oct 12, 2023

Uh oh!

risicle commented Oct 12, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

risicle commented Oct 12, 2023

Uh oh!

LunNova commented Oct 12, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

risicle commented Oct 13, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

LunNova commented Oct 13, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

LunNova commented Oct 13, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

risicle commented Oct 13, 2023

Uh oh!

SuperSandro2000 Oct 14, 2023

Choose a reason for hiding this comment

Uh oh!

SuperSandro2000 Oct 14, 2023

Choose a reason for hiding this comment

Uh oh!

Uh oh!

chivay Oct 19, 2023

Choose a reason for hiding this comment

Uh oh!

risicle Oct 19, 2023

Choose a reason for hiding this comment

Uh oh!

risicle Oct 19, 2023

Choose a reason for hiding this comment

Uh oh!

ghost left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

risicle commented Oct 22, 2023

Uh oh!

Uh oh!

risicle commented Oct 23, 2023

Uh oh!

ghost commented Oct 23, 2023 • edited by ghost Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

vcunat commented Oct 24, 2023

Uh oh!

ghost commented Oct 24, 2023

Uh oh!

chayleaf Nov 20, 2023

Choose a reason for hiding this comment

Uh oh!

risicle Nov 20, 2023

Choose a reason for hiding this comment

Uh oh!

ghost Nov 30, 2023 • edited by ghost Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ghost commented Nov 30, 2023

Uh oh!

risicle commented Oct 4, 2023 •

edited

Loading

risicle commented Oct 12, 2023 •

edited

Loading

LunNova commented Oct 12, 2023 •

edited

Loading

risicle commented Oct 13, 2023 •

edited

Loading

LunNova commented Oct 13, 2023 •

edited

Loading

LunNova commented Oct 13, 2023 •

edited

Loading

ghost commented Oct 23, 2023 •

edited by ghost

Loading

ghost Nov 30, 2023 •

edited by ghost

Loading