openssh: enable more hardening options

[zzywysm]

openssh only enables position-independent executables at the moment. Given its privileged access and attack surface, it should have more binary hardening options enabled.

Things done

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

[AndersonTorres]

All these options are already the global defaults for Nixpkgs, as stated in the manual:

https://nixos.org/manual/nixpkgs/stable/#sec-hardening-in-nixpkgs

[zzywysm]

I don't think that the documentation accurately reflects reality, given for example the existence of this pull request: #252310

I believe that openssh should explicitly opt-in to as many hardening options as possible, and not rely on sketchy documentation guarantees.

[AndersonTorres]

1. You are precisely using "sketchy documentation guarantees" when setting hardeningEnable. Or do you truly believe fortify will fortify the code because the sketchy documentation guarantees it, at the same time you don't believe the sketchy documentation guarantees fortify is already among the default hardening flags?
2. The PR you cited talks about adding PIE to the list of default hardening flags, not about the gap between documentation and code.
3. I concede that in that PR conversation some people pointed out packages that should have hardening flags enabled but it doesn't happen in practice. However they bring evidence, not mere guessing.
4. About this, have you verified the flags you explicitly set above effectively hardened openssh?

[zzywysm]

1. I don't understand this.
2. I thought that "filing a documentation bug to acknowledge NixOS's security weaknesses" was an inferior option to "file a pull request to ensure the most security-critical package explicitly opts-into more hardening". Do you agree or disagree?
3. Some more strong evidence can be found in PR stdenv/generic/make-derivation.nix: always set NIX_HARDENING_ENABLE #206490
4. No, I felt that merely adopting the same hardening options as Ubuntu and Debian was an appropriate measure.

[AndersonTorres]

1. You are implying the documentation does not reflect the code, however you are updating the code based on the same unreliable documentation.

2. I would agree in principle, however (again) the documentation does not reflect the code. So we should read the underlying code to verify if the flags will be effectively enabled.

3. Nice. I will watch this.

4. Well, if you push (say) fortify to ensure the most security-critical package explicitly opts-into more hardening, however the underlying Nixpkgs code does not fortify openssh, then we are effectively wasting time - and worse, providing a placebo.

   So we need at least some guarantee that openssh is effectively hardened.

[Artturin]

https://nixos.org/manual/nixpkgs/stable/#sec-hardening-flags-disabled-by-default PIE is already documented as disabled by default.

The rest of the hardening flags are enabled either because of

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

Lines 113 to 115 in 68b1fd2

	# If unset, assume the default hardening flags.
	: ${NIX_HARDENING_ENABLE="fortify fortify3 stackprotector pic strictoverflow format relro bindnow"}
	export NIX_HARDENING_ENABLE

or because hardeningEnable are added to NIX_HARDENING_ENABLE = enabledHardeningOptions in stdenv. hardeningEnable is additive.

#206490 is for making the option duplication unnecessary (if there are no drawbacks).

[h7x4]

I don't think I'd call the default hardening flags a "documentation guarantee". The documentation is just a supplement, the code is the real guarantee

nixpkgs/pkgs/stdenv/generic/make-derivation.nix

Lines 198 to 204 in aa73e0d

	defaultHardeningFlags = if stdenv.hostPlatform.isMusl &&
	# Except when:
	# - static aarch64, where compilation works, but produces segfaulting dynamically linked binaries.
	# - static armv7l, where compilation fails.
	!(stdenv.hostPlatform.isAarch && stdenv.hostPlatform.isStatic)
	then supportedHardeningFlags
	else lib.remove "pie" supportedHardeningFlags;

#206490 seems to regard the mechanisms between mkDerivation and more specific builder functions, not the hardening defaults themselves. I don't think it's something to worry about, it's just setting a more sane default to reduce some unnecessary boiler plate for the other builder functions.

Running checksec on the openssh binaries reports all of the flags as enabled (don't know about the other errors, but they seem irrelevant)

$ nix run nixpkgs#checksec -- --dir="$(nix build nixpkgs#openssh --print-out-paths)/bin/"
/nix/store/1pc42yi94v08520h7pi3pdzfrzc8kvzs-checksec-2.6.0/bin/.checksec-wrapped: line 6: env: command not found
/nix/store/1pc42yi94v08520h7pi3pdzfrzc8kvzs-checksec-2.6.0/bin/.checksec-wrapped: line 76: basename: command not found
RELRO           STACK CANARY      NX            PIE             RPATH      RUNPATH    Symbols           FORTIFY Fortified       Fortifiable   Filename
Full RELRO      Canary found      NX enabled    PIE enabled     No RPATH   RUNPATH     1418 Symbols       Yes   11              19              /nix/store/3fi5cc9v77pp2zrrc1kdkvwpk7xa8ibv-openssh-9.5p1/bin/ssh-keyscan
Full RELRO      Canary found      NX enabled    PIE enabled     No RPATH   RUNPATH     2276 Symbols       Yes   13              22              /nix/store/3fi5cc9v77pp2zrrc1kdkvwpk7xa8ibv-openssh-9.5p1/bin/ssh
Full RELRO      Canary found      NX enabled    PIE enabled     No RPATH   RUNPATH     1132 Symbols       Yes   13              20              /nix/store/3fi5cc9v77pp2zrrc1kdkvwpk7xa8ibv-openssh-9.5p1/bin/ssh-add
Full RELRO      Canary found      NX enabled    PIE enabled     No RPATH   RUNPATH     1437 Symbols       Yes   13              22              /nix/store/3fi5cc9v77pp2zrrc1kdkvwpk7xa8ibv-openssh-9.5p1/bin/ssh-keygen
Full RELRO      Canary found      NX enabled    PIE enabled     No RPATH   RUNPATH     785 Symbols        Yes   11              20              /nix/store/3fi5cc9v77pp2zrrc1kdkvwpk7xa8ibv-openssh-9.5p1/bin/sftp
Full RELRO      Canary found      NX enabled    PIE enabled     No RPATH   RUNPATH     764 Symbols        Yes   11              18              /nix/store/3fi5cc9v77pp2zrrc1kdkvwpk7xa8ibv-openssh-9.5p1/bin/scp
Full RELRO      Canary found      NX enabled    PIE enabled     No RPATH   RUNPATH     2804 Symbols       Yes   13              26              /nix/store/3fi5cc9v77pp2zrrc1kdkvwpk7xa8ibv-openssh-9.5p1/bin/sshd
Full RELRO      Canary found      NX enabled    PIE enabled     No RPATH   RUNPATH     1091 Symbols       Yes   13              20              /nix/store/3fi5cc9v77pp2zrrc1kdkvwpk7xa8ibv-openssh-9.5p1/bin/ssh-agent

[Artturin]

$ nix shell "nixpkgs#stdenv.cc.bintools" "nixpkgs#stdenv.cc" "nixpkgs#debian-devscripts" --command hardening-check ./result/bin/ssh
./result/bin/ssh:
 Position Independent Executable: yes
 Stack protected: yes
 Fortify Source functions: yes (some protected functions found)
 Read-only relocations: yes
 Immediate binding: yes
 Stack clash protection: unknown, no -fstack-clash-protection instructions found
 Control flow integrity: no, not found!

after applying this PR

$ nix shell "nixpkgs#stdenv.cc.bintools" "nixpkgs#stdenv.cc" "nixpkgs#debian-devscripts" --command hardening-check ./result/bin/ssh
./result/bin/ssh:
Position Independent Executable: yes
Stack protected: yes
Fortify Source functions: yes (some protected functions found)
Read-only relocations: yes
Immediate binding: yes
Stack clash protection: unknown, no -fstack-clash-protection instructions found
Control flow integrity: no, not found!

stack clash protection should be a false positive https://manpages.debian.org/testing/devscripts/hardening-check.1.en.html

When an executable was built without any character arrays being allocated on the
stack, this check will lead to false alarms (since there is no use of
__stack_chk_fail), even though it was compiled with the correct options.

Perhaps your confusion comes from the fact that it's not in the manual that hardeningEnable is additive.

[zzywysm]

With the additional context from @Artturin and @h7x4, I am much more satisfied that the documentation is correct.

I don't think that this change is bad (Artturin showed it didn't decrease security), and I'll leave it to the maintainers to decide if they like the increased explicitness or not.

[Artturin]

By grepping for hardeningEnable the only settings of it are to either enable pie or in pkgs/test/cc-wrapper/hardening.nix.
So I think we prefer a single list of hardening flags to be able to change the flags for all platforms at once in case one platform has a broken hardening.