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.

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

cc-wrapper: Remove redundant hardening #28555

Merged

Conversation

Ericson2314
Copy link
Member

@Ericson2314 Ericson2314 commented Aug 25, 2017

Motivation for this change

I can't find these flags (-z relno -z now) documented for GCC anywhere, and they are already passed to ld anyways.

Based on aff1f4a, I think these were accidentally passed to the C compiler from the beginning.

Things done

@Ericson2314 Ericson2314 added this to the 17.09 milestone Aug 25, 2017
@mention-bot
Copy link

@Ericson2314, thanks for your PR! By analyzing the history of the files in this pull request, we identified @edolstra, @abbradar and @vcunat to be potential reviewers.

Ericson2314 referenced this pull request Aug 25, 2017
The following parameters are now available:

  * hardeningDisable
    To disable specific hardening flags
  * hardeningEnable
    To enable specific hardening flags

Only the cc-wrapper supports this right now, but these may be reused by
other wrappers, builders or setup hooks.

cc-wrapper supports the following flags:

  * fortify
  * stackprotector
  * pie (disabled by default)
  * pic
  * strictoverflow
  * format
  * relro
  * bindnow
@Ericson2314 Ericson2314 added this to Needed by the big PR---nice to move pick off pieces of it and move here, rebasing the big PR on top in Cross compilation Aug 25, 2017
@Ericson2314 Ericson2314 moved this from Needed by the big PR---nice to move pick off pieces of it and move here, rebasing the big PR on top to Needed for binutils-wrapper in Cross compilation Aug 25, 2017
@periklis
Copy link
Contributor

afaik these are clear linker flags to control relocation on build/link-time since the addresses of the code-segments are known. I suspect these flags are passed with purpose to the gcc driver, who passes them to the linker in the end.

@Ericson2314
Copy link
Member Author

Ah, per https://gcc.gnu.org/onlinedocs/gcc/Link-Options.html

-z is passed directly on to the linker along with the keyword keyword. See the section in the documentation of your linker for permitted values and their meanings.

But if it's just a pass-through, then we don't need to do this as the linker wrapper will add it anyways.

@Ericson2314 Ericson2314 force-pushed the cc-wrapper-ld-hardening-redundancy branch 4 times, most recently from 77c5a4a to 66e0c87 Compare August 29, 2017 19:47
GCC just passes `-z ...` flags to ld unaltered, and they are already
passed to LD anyways. On the other hand, `-pie` affects gcc behavior
too.
This becomes necessary if more wrappers besides cc-wrapper start
supporting hardening flags. Also good to make the warning into an
error.

Also ensure interface is being used right: Not as a string, not just in
bash.
@Ericson2314 Ericson2314 force-pushed the cc-wrapper-ld-hardening-redundancy branch from 66e0c87 to 345885f Compare August 29, 2017 19:51
@Ericson2314
Copy link
Member Author

@globin based on the hydra job I think this will be good to merge tomorrow if my Darwin stdenv build finishes?

@globin globin merged commit 97a4883 into NixOS:staging Aug 30, 2017
@Ericson2314 Ericson2314 deleted the cc-wrapper-ld-hardening-redundancy branch August 30, 2017 15:59
@orivej
Copy link
Contributor

orivej commented Aug 31, 2017

@Ericson2314 The next commit 97a4883 broke the evaluation of nixUnstable:

$ nix-build ~/.nixpkgs/nixpkgs -A nixUnstable --show-trace                                                                                                               /tmp
error: while evaluating the attribute ‘configureFlags’ of the derivation ‘nix-1.12pre5511_c94f3d55’ at /home/uj/.nixpkgs/nixpkgs/pkgs/stdenv/generic/make-derivation.nix:98:11:
while evaluating ‘override’ at /home/uj/.nixpkgs/nixpkgs/lib/customisation.nix:78:20, called from /home/uj/.nixpkgs/nixpkgs/pkgs/tools/package-management/nix/default.nix:13:8:
while evaluating ‘makeOverridable’ at /home/uj/.nixpkgs/nixpkgs/lib/customisation.nix:72:24, called from /home/uj/.nixpkgs/nixpkgs/lib/customisation.nix:78:29:
while evaluating anonymous function at /home/uj/.nixpkgs/nixpkgs/pkgs/os-specific/linux/busybox/default.nix:1:1, called from /home/uj/.nixpkgs/nixpkgs/lib/customisation.nix:74:12:
while evaluating ‘mkDerivation’ at /home/uj/.nixpkgs/nixpkgs/pkgs/stdenv/generic/make-derivation.nix:15:5, called from /home/uj/.nixpkgs/nixpkgs/pkgs/os-specific/linux/busybox/default.nix:29:1:
assertion failed at /home/uj/.nixpkgs/nixpkgs/pkgs/stdenv/generic/make-derivation.nix:54:8

I don't understand why…

@orivej
Copy link
Contributor

orivej commented Aug 31, 2017

This change to pkgs/stdenv/generic/make-derivation.nix helps with debugging:

-    let allHardeningFlags = [
-      "fortify" "stackprotector" "pie" "pic" "strictoverflow" "format" "relro"
-      "bindnow"
-    ];
-    in assert lib.all
-      (flag: lib.elem flag allHardeningFlags)
-      (hardeningEnable ++ hardeningDisable);
-
-    let
+    let unsupportedHardeningFlags = lib.subtractLists [
+      "fortify" "stackprotector" "pie" "pic" "strictoverflow" "format" "relro" "bindnow"
+    ] (hardeningEnable ++ hardeningDisable);
+    in if builtins.length unsupportedHardeningFlags != 0
+    then abort ("unsupported hardening flags: " + lib.escapeShellArgs unsupportedHardeningFlags)
+    else let

nix-build ~/.nixpkgs/nixpkgs -A gnat --show-trace fails with intelligible

while evaluating ‘mkDerivation’ at /home/uj/.nixpkgs/nixpkgs/pkgs/stdenv/generic/make-derivation.nix:15:5, called from /home/uj/.nixpkgs/nixpkgs/pkgs/development/compilers/gcc/4.5/default.nix:128:1:
evaluation aborted with the following error message: ‘unsupported hardening flags: 'all'’

but nix-build ~/.nixpkgs/nixpkgs -A nixUnstable --show-trace fails with mysterious

while evaluating ‘override’ at /home/uj/.nixpkgs/nixpkgs/lib/customisation.nix:78:20, called from /home/uj/.nixpkgs/nixpkgs/pkgs/tools/package-management/nix/default.nix:13:8:
while evaluating ‘makeOverridable’ at /home/uj/.nixpkgs/nixpkgs/lib/customisation.nix:72:24, called from /home/uj/.nixpkgs/nixpkgs/lib/customisation.nix:78:29:
while evaluating anonymous function at /home/uj/.nixpkgs/nixpkgs/pkgs/os-specific/linux/busybox/default.nix:1:1, called from /home/uj/.nixpkgs/nixpkgs/lib/customisation.nix:74:12:
while evaluating ‘mkDerivation’ at /home/uj/.nixpkgs/nixpkgs/pkgs/stdenv/generic/make-derivation.nix:15:5, called from /home/uj/.nixpkgs/nixpkgs/pkgs/os-specific/linux/busybox/default.nix:29:1:
evaluation aborted with the following error message: ‘unsupported hardening flags: 'fortify'’

@Ericson2314
Copy link
Member Author

Ah, and I was just about to post

$ nix-instantiate --eval --show-trace -E '((import ./. {}).busybox.override { enableStatic = true; }).hardeningDisable'

So, what do you think is going on? "fortify" is definitely on my whitelist.

"all"not being there (though it definitely should be) somewhat spooks me too, as nix-env -qaP -f . --out-path --show-trace succeed.

@orivej
Copy link
Contributor

orivej commented Aug 31, 2017

Is this a bug in Nix?

$ nix-repl
Welcome to Nix version 1.11.13. Type :? for help.

nix-repl> :a import <nixpkgs> { }
Added 7763 variables.

nix-repl> lib.subtractLists ["a" "b"] ([ "a" "b" ])
[ ]

nix-repl> lib.subtractLists ["a" "b"] ([ "a" ] ++ (lib.optional true ["b"]))
[ [ ... ] ]

nix-repl> builtins.length (lib.subtractLists ["a" "b"] ([ "a" ] ++ (lib.optional true ["b"])))
1

nix-repl> builtins.head (lib.subtractLists ["a" "b"] ([ "a" ] ++ (lib.optional true ["b"])))
[ "b" ]

@orivej
Copy link
Contributor

orivej commented Aug 31, 2017

No, it's my mistake of testing with optional instead of optionals.

@orivej
Copy link
Contributor

orivej commented Sep 1, 2017

busybox is subject to exactly this problem with its

hardeningDisable = [ "format" ] ++ lib.optional enableStatic [ "fortify" ];

that evaluates to [ "format" [ "fortify" ] ].

orivej added a commit to orivej/nixpkgs that referenced this pull request Sep 1, 2017
- allow "all" in hardeningDisable
- fix busybox flags
- print detailed error message

Discussed at NixOS#28555 (comment)
globin pushed a commit that referenced this pull request Sep 3, 2017
- allow "all" in hardeningDisable
- fix busybox flags
- print detailed error message

Discussed at #28555 (comment)

(cherry picked from commit d70006c)
@Ericson2314 Ericson2314 added 8.has: port to stable A PR already has a backport to the stable release. 9.needs: port to stable A PR needs a backport to the stable release. labels Sep 3, 2017
@samueldr samueldr removed the 9.needs: port to stable A PR needs a backport to the stable release. label Apr 17, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
8.has: port to stable A PR already has a backport to the stable release.
Projects
No open projects
Cross compilation
Needed for binutils-wrapper
Development

Successfully merging this pull request may close these issues.

None yet

6 participants