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

WIP: rewrite hardening in plain nix #19512

Closed
wants to merge 7 commits into from
Closed

WIP: rewrite hardening in plain nix #19512

wants to merge 7 commits into from

Conversation

groxxda
Copy link
Contributor

@groxxda groxxda commented Oct 13, 2016 •
edited
Loading

Motivation for this change

Doing it in nix makes it more lazy.

See commit messages for more details

Fixes #18995

This PR is not ready to merge.
I post it to get feedback on the changes.

TODO:

  • Add hardening for other cc derivations
  • Handle pie hardening (probably via .spec file for gcc, sadly unsupported on clang)
  • Make all flag more efficient (see 4bf0576 for details)

CC @fpletz @globin

Things done
  • Tested using sandboxing
    (nix.useSandbox on NixOS,
    or option build-use-sandbox in nix.conf
    on non-NixOS)
  • Built on platform(s)
    • NixOS
    • OS X
    • Linux
  • Tested compilation of all pkgs that depend on this change using nix-shell -p nox --run "nox-review wip"
  • Tested execution of all binary files (usually in ./result/bin/)
  • Fits CONTRIBUTING.md.

@groxxda
stdenv: rewrite hardening from bash to nix
eedf427 
The logic is now done in nix. This adds the benefit that hardening is
now more lazy. Previously a change to add-hardening.sh caused a full
rebuild.
@groxxda
stdenv: add hardeningSupported (only gcc5 for now)
cfb5d1e 
This adds an attribute to the cc derivation that indicates which
hardening flags are supported.

Fixes #18995:
nix-shell -p uses stdenvNoCC to build the environment. Thus
stdenv.cc.cc.hardeningSupported is undefined and no hardening flags are
turned on.
@groxxda
gcc6: add supported hardening flags
4b93591
@groxxda
stdenv: support special 'all' hardening flag
4bf0576 
Since it was supported before, add it here.

The implementation favors readability over performance:
The case hardeningDisable=["all"] is unnecessarily unperformant.
@groxxda
gcc, icl: our bootstrap gcc has supportHardening = [], so no need for…
73f0e91 
… this anymore
@groxxda
Copy link
Contributor Author

groxxda commented Oct 13, 2016

@fpletz can I help with the remaining todos?

@fpletz fpletz added 0.kind: enhancement Add something new 9.needs: reporter feedback This issue needs the person who filed it to respond 1.severity: mass-rebuild This PR causes a large number of packages to rebuild 2.status: work-in-progress This PR isn't done 9.needs: port to stable A PR needs a backport to the stable release. labels Oct 13, 2016
@fpletz fpletz self-assigned this Oct 13, 2016
@fpletz
Copy link
Member

fpletz commented Oct 13, 2016

Yes, please start if you can. I will have time again to look at these issues at the weekend. Too much dayjob-stuff to do.

@groxxda
Copy link
Contributor Author

groxxda commented Oct 13, 2016

@fpletz can you share your work for spec files somewhere?

@aneeshusa
Copy link
Contributor

I'm a big fan of doing more in Nix (see my cmakeFlags/configureFlags PRs), but since this adds more processing to mkDerivation, I believe this will require #18660 to allow easy overrides of the hardening flags.

@aneeshusa
Copy link
Contributor

aneeshusa commented Oct 14, 2016 •
edited
Loading

We need to keep supporting "all" for backwards compatibility right now, but a nice interface improvement we can get is also supporting hardeningEnable = true and hardeningDisable = true, instead of using "all" as a sentinel.

Edit: this could also be hardeningEnable = true and hardeningEnable = false, and not supporting a boolean value for hardeningDisable.

@groxxda
Copy link
Contributor Author

groxxda commented Oct 14, 2016

@fpletz I think our best shot to handle pie gracefully is to modify (ld,cc)-wrapper instead of using spec files. Something along [pseudo code]

for flag in flags:
  if flag matches "-?-(static|shared)":
    disable_pie = true
if disable_pie:
  flags.remove("-pie")

just before the debug print..

@spacekitteh
Copy link
Contributor

spacekitteh commented Nov 4, 2016 •
edited
Loading

Cool. How's this coming?

Also, @grahamc security

@grahamc grahamc added the 1.severity: security Issues which raise a security issue, or PRs that fix one label Nov 5, 2016
@bjornfor
Copy link
Contributor

bjornfor commented Dec 8, 2016

Friendly bump. Are the remaining TODOs blockers?

@copumpkin
Copy link
Member

Any progress here?

@fpletz
Copy link
Member

fpletz commented Aug 15, 2017

Closing in favour of #28029.

@fpletz fpletz closed this Aug 15, 2017
@groxxda groxxda deleted the hardeningInNix branch September 21, 2017 13:44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees

@fpletz fpletz

Labels
0.kind: enhancement Add something new 1.severity: mass-rebuild This PR causes a large number of packages to rebuild 1.severity: security Issues which raise a security issue, or PRs that fix one 2.status: work-in-progress This PR isn't done 9.needs: port to stable A PR needs a backport to the stable release. 9.needs: reporter feedback This issue needs the person who filed it to respond
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
7 participants
@groxxda @fpletz @aneeshusa @spacekitteh @bjornfor @copumpkin @grahamc