Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

openssh: enable more hardening options #260751

Closed
wants to merge 1 commit into from
Closed

openssh: enable more hardening options #260751

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion pkgs/tools/networking/openssh/common.nix
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -95,7 +95,7 @@ stdenv.mkDerivation {

enableParallelBuilding = true;

hardeningEnable = [ "pie" ];
hardeningEnable = [ "fortify" "stackprotector" "pie" "relro" "bindnow" ];
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  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?

Copy link
Contributor Author

@zzywysm zzywysm Oct 12, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  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.

Copy link
Member

@Artturin Artturin Oct 12, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

@Artturin Artturin Oct 12, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more. 

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.


doCheck = true;
enableParallelChecking = false;
Expand Down