stdenv: Enable PIE by default #252310
stdenv: Enable PIE by default #252310
Conversation
chivay
added
the
1.severity: security
chivay
force-pushed
the
enable-pie-kekw
branch
from
4f22302
to
42e4484
Compare
|
After some digging, I've discovered the issue behind failed ofborg builds: It turns out that the bootstrap tools are missing a Scrt1.o file from glibc. After applying following patch and swapping the bootstrap files bash was built correctly. diff --git a/pkgs/stdenv/linux/make-bootstrap-tools.nix b/pkgs/stdenv/linux/make-bootstrap-tools.nix
index d6c4da0ab2be..03f218c5d143 100644
--- a/pkgs/stdenv/linux/make-bootstrap-tools.nix
+++ b/pkgs/stdenv/linux/make-bootstrap-tools.nix
@@ -79,6 +79,7 @@ in with pkgs; rec {
cp -d ${libc.out}/lib/libnss*.so* $out/lib
cp -d ${libc.out}/lib/libresolv*.so* $out/lib
cp -d ${libc.out}/lib/crt?.o $out/lib
+ cp -d ${libc.out}/lib/Scrt*.o $out/lib
# Hacky compat with our current unpack-bootstrap-tools.sh
ln -s librt.so "$out"/lib/librt-dummy.so |
|
Looking forward to this! As a security engineer it's a bit weird to use NixOS as my main system, since it's clear that security is not the priority. Which is OK - plenty of security focused distros already, and we reproducibility fams may get Spectrum one day. But there are things one expects from "security focused" distros (like QubesOS), and there are things that constitute the status quo, the ground level. PIE is one of those things - it's a simple enhancement, supported by every major distribution already, and it's a major security improvement (TLDR: arbitrary write primitive is not enough for exploitation anymore, one needs to leak ASLR first so a separate vulnerability is usually needed). I'm happy to help testing it if necessary. But can one of the maintainers weigh in on this enhancement first? |
|
After some further experimentation I've discovered additional quirks - although bash is built correctly, some other packages like python3 end up without PIE (and for some unrelated reason also no stack canaries?) Lack of PIE seems to be caused by inconsistent grepping over this variable reveals: Adding |
|
In those situations, I advise adding at least the two following group of reviewers: (a) low-level stdenv contributors/developers, e.g. @trofi @Artturin (@amjoseph-nixpkgs does a lot of work in this area those days too and is already included, @Ericson2314 is an historical contributor and is still doing work in those areas AFAIK) (b) security team, e.g. @risicle who has been driving a lot of hardening enablement (fortify source 3 comes to mind) (c) bonus, staging people because this one will probably a massive breakage incoming, cc @vcunat @mweinelt @K900 who were recently involved in staging rounds. In either case, this should definitely get its own Hydra jobset for pre-evaluation of the breakage, and then the classical discussion ensues on how to sort out this and what to do. For record, I am in favor of this, I just think we need the human resources to opt-out/fix the hardening for broken packages. |
|
You might want to include all runtime
Otherweise we would need to re-cut |
|
I echo the general approval of this idea. Firstly we will probably find it helpful if more/most packages have tests or we find a way of automatically testing binaries for basic-execution-without-crashing. Secondly as you find problems it may be good to try narrowing them down into a test which you can add to |
Include all runtime object files in output package, enabling different kinds of build modes - non-PIE, PIE, static PIE and profile-generated. Suggested by @trofi: NixOS#252310 (comment)
Include all runtime object files in output package, enabling different kinds of build modes - non-PIE, PIE, static PIE and profile-generated. Suggested by @trofi: NixOS#252310 (comment)
|
Also please see #259070 where I've refactored how |
#206490 or a similar change should help with it. I never had the time to push that PR forward. |
|
and/or #259070 which unifies the set of defaults with those used by the build-support scripts ;) |
ofborg
bot
added
10.rebuild-darwin-stdenv
|
Result of 67 packages built:
|
|
Out of curiosity, I made a quick patch to add Should we start by doing that to make it a bit easier to start playing with diff --git a/pkgs/top-level/stage.nix b/pkgs/top-level/stage.nix
index 390aa36db03b..3f01c235ce9c 100644
--- a/pkgs/top-level/stage.nix
+++ b/pkgs/top-level/stage.nix
@@ -293,7 +293,12 @@ let
stdenv = super'.withDefaultHardeningFlags (
super'.stdenv.cc.defaultHardeningFlags ++ [
"trivialautovarinit"
- ]
+ ] ++ lib.optionals (with super'.stdenv;
+ # Always enable PIE except when using musl for:
+ # - static aarch64, where compilation works, but produces segfaulting dynamically linked binaries.
+ # - static armv7l, where compilation fails.
+ !(targetPlatform.libc == "musl" && targetPlatform.isAarch && targetPlatform.isStatic)
+ ) [ "pie" ]
) super'.stdenv;
})
] ++ overlays; |
|
I'd be generally in favour, but I think we should decide where we're making platform-specific decisions. If we disable the musl/static/aarch combination because it just plain doesn't work, we should probably knock it out using the |
|
To check if the exclusion was still needed I added I would have expected this to have been solved by #253760 (and the follow up updates of the bootstrap files). At least the comment does not seem to be completely accurate anymore 😅 |
I needed to update the aarch64 gnu bootstrap in order to check if the combinaison of musl / static / aarch64 was still broken when enabling PIE (NixOS#252310). In order to that I followed the same approach than in ea67e45. Files came from this Hydra build: https://hydra.nixos.org/build/262861772 …which used nixpkgs revision a985888 to instantiate: /nix/store/rqhq21qxwyhd633wgc53269jzp19y75m-stdenv-bootstrap-tools.drv …and then built: /nix/store/z3sgi7wh8c5pnm3xm2s3kcbwqbw0d5dp-stdenv-bootstrap-tools I downloaded these files from Hydra and prefetched them into the Nix store with the following commands: STOREPATH=dk28gq49ckmgwpnx36709ff0hxnkmqpk-stdenv-bootstrap-tools OPTIONS="--option binary-caches https://cache.nixos.org --option trusted-public-keys cache.nixos.org-1:6NCHdD59X431o0gWypbMrAURkbJ16ZPMQFGspcDShjY=" nix --extra-experimental-features nix-command store prefetch-file \ file://$(nix --extra-experimental-features nix-command store add-file --name bootstrap-tools.tar.xz $(nix-store ${OPTIONS} -r /nix/store/${STOREPATH})/on-server/bootstrap-tools.tar.xz) nix --extra-experimental-features nix-command store prefetch-file --executable \ file://$(nix --extra-experimental-features nix-command store add-path --name busybox $(nix-store ${OPTIONS} -r /nix/store/${STOREPATH})/on-server/busybox) These commands produced the following output: copying path '/nix/store/dk28gq49ckmgwpnx36709ff0hxnkmqpk-stdenv-bootstrap-tools' from 'https://cache.nixos.org'... warning: you did not specify '--add-root'; the result might be removed by the garbage collector Downloaded 'file:///nix/store/rpsfzw2ixbdrd5nm4l00y9sjq8h9lg5k-bootstrap-tools.tar.xz' to '/nix/store/ax259maqg6nfdrxznv49as902j1fzhhr-rpsfzw2ixbdrd5nm4l00y9sjq8h9lg5k-bootstrap-tools.tar.xz' (hash 'sha256-Ag5/vwGqv8q9SwdJYmmcvtqcLJjVYNwhgcqQ0BHTTdg='). warning: you did not specify '--add-root'; the result might be removed by the garbage collector Downloaded 'file:///nix/store/7p38lg5jh4rvgq6aaxqs5sw0gy357z6s-busybox' to '/nix/store/y9bhy7662zdsacn6hr6zp81mzdyb7vzf-7p38lg5jh4rvgq6aaxqs5sw0gy357z6s-busybox' (hash 'sha256-8areJJa2A0xauz5XqwZTgkHSb3qSdi7rTiCI05SaS0Y='). The sha256sums of all the on-server components: $ sha256sum /nix/store/${STOREPATH}/on-server/* 020e7fbf01aabfcabd4b074962699cbeda9c2c98d560dc2181ca90d011d34dd8 /nix/store/dk28gq49ckmgwpnx36709ff0hxnkmqpk-stdenv-bootstrap-tools/on-server/bootstrap-tools.tar.xz 8f4e1b1cafbfd3cc1517f477870f3b54170934f09b4c19ae2257acea557fbbca /nix/store/dk28gq49ckmgwpnx36709ff0hxnkmqpk-stdenv-bootstrap-tools/on-server/busybox
|
My normal approach when adding new hardening flags is to add them to |
|
This seems like a pretty simple PR that got hung up on bikeshedding which is a shame. Any way to revive this? |
|
This pull request has been mentioned on NixOS Discourse. There might be relevant details there: |
eclairevoyant
added
the
2.status: merge conflict
I'm attempting something similar, but on a NixOS configuration level (overriding nixpgks with pkgsExtraHardening) diff --git a/pkgs/top-level/stage.nix b/pkgs/top-level/stage.nix
index 6ad193c8926d..320ec1f82abe 100644
--- a/pkgs/top-level/stage.nix
+++ b/pkgs/top-level/stage.nix
@@ -330,6 +330,7 @@ let
"pacret"
"stackclashprotection"
"trivialautovarinit"
+ "pie"
]
) super'.stdenv;
glibc = super'.glibc.override rec {I'm already seeing some failures from Docker dependencies:
They both fail during check phase so I'd presume that the binaries produced are non-static and trying to find the ld.so EDIT: Well it took a while for the build to stop completely. The big ones failing at this stage of build are systemd log: The error seems not related to PIE itself (although it causes some additional noise). Maybe systemd should be using unwrapped clang for the BPF programs? ghc log: This one seems much more cursed. No idea what's going on there. EDIT: Ok, seems more obvious now. Ghc is trying to link against |
|
GHC seems to work after applying a5e76fb |
|
Turning it off for GHC specifically with intent to fix it later so it can be turned on more widely sooner seems pretty reasonable. |
|
systemd build failure fix attempt: #344292 |
Description of changes
Almost exactly 7 years ago, a large security-related PR that introducted hardening was merged - #12895.
In 2016, enabling PIE was a bit controversial as it could break some packages and introduce overhead on 32 bit architectures.
Unfortunately, this decision lead us to the current state of nixpkgs where only a handful of packages (~20) enables PIE by default.
I think that we should revisit this discussion and enable PIE by default for all packages, leaving the option to opt-out of PIE if it causes crashes or broken packages. Other large distros have already done this, see:
https://wiki.ubuntu.com/Security/Features#pie
https://wiki.gentoo.org/wiki/Hardened/FAQ
https://wiki.archlinux.org/title/Arch_package_guidelines/Security#PIE
I'm aware that this PR won't be merged as-is since I don't have enough experience with stdenv, but it should be at least a humble beginning to a larger fix.
Kudos to @BonusPlay for pointing out this issue.
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/)