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)
|
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 ;) |
wegank
added
the
2.status: stale
|
With this change, some builds using clang log |
stale
bot
removed
the
2.status: stale
I could be wrong but it seems that it might be happening when building shared libraries. It is a bit ugly but won't break anything. Ideally, hardening scripts should drop this flag when |
|
I suppose we would pass |
|
I think clang defaults to pie on now, (llvm/llvm-project@ca68038) so do we need to add an arg at all when the CC is clang? Other distros have been shipping GCCs built with |
|
For clang version < 15, I guess. That commit re-applies a reverted commit, so I'm not sure what the default is for older versions. |
We probably don't need to in newer versions, but handling different compilers and versions could get messy. |
SigmaSquadron
added
the
1.severity: significant
|
Might be best to just leave old Clang versions alone as they’re only used for picky packages anyway. Energy put into them is often best directed to unpinning them across the tree so we can eventually drop them. |
|
Some packages even fail due to |
|
Semi-Related: #356058; the python drv doesn't respect |
@FliegendeWurst could you give an example of a failing package? |
|
Probably better to just turn off the silly |
For example |
|
Another option would be to just configure compilers to enable PIE by default rather than injecting |
|
From a quick test, seems non-trivial. After adding: diff --git a/pkgs/development/compilers/gcc/common/configure-flags.nix b/pkgs/development/compilers/gcc/common/configure-flags.nix
index 25d4f1f53bae..83cd78f49fce 100644
--- a/pkgs/development/compilers/gcc/common/configure-flags.nix
+++ b/pkgs/development/compilers/gcc/common/configure-flags.nix
@@ -161,6 +161,7 @@ let
"--without-included-gettext"
"--with-system-zlib"
"--enable-static"
+ "--enable-default-pie"
"--enable-languages=${
lib.concatStringsSep ","
( lib.optional langC "c"I'm getting a build failure in xgcc during configure stage |
This comment was marked as off-topic.
This comment was marked as off-topic.
| "strictoverflow" | ||
| ] ++ lib.optional (with stdenvNoCC; | ||
| # Musl-based platforms will keep "pie", other platforms will not. | ||
| # If you change this, make sure to update section `{#sec-hardening-in-nixpkgs}` |
There was a problem hiding this comment.
I can't help but notice that this is not updated in this PR. Will it be updated somewhere else?
|
I think this got superseded by PR #439314 which is now in |
17 participants
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.
Fixes #205031
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/)