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

Hardened compiler flags by default #12895

Merged
merged 563 commits into from Aug 29, 2016
Merged

Hardened compiler flags by default #12895

merged 563 commits into from Aug 29, 2016

Conversation

@globin
Copy link
Member

@globin globin commented Feb 9, 2016

This adds some compiler/linker flags to harden our packages via a stdenv adapter:

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

Information from the debian wiki:

Stack protector is a mainline GCC feature, which adds safety checks against stack overwrites. This renders many potential code injection attacks into aborting situations. In the best case this turns code injection vulnerabilities into denial of service or into non-issues (depending on the application). http://en.wikipedia.org/wiki/Stack-smashing_protection
Currently this uses -fstack-protector-all instead of -fstack-protector-strong because the bootstrapping compiler is too old.` /cc @edolstra

Fortify During code generation the compiler knows a great deal of information about buffer sizes (where possible), and attempts to replace insecure unlimited length buffer function calls with length-limited ones. This is especially useful for old, crufty code. Additionally, format strings in writable memory that contain '%n' are blocked. If an application depends on such a format string, it will need to be worked around.

Format
If -Wformat is specified, also warn about uses of format functions that represent possible security problems. At present, this warns about calls to printf and scanf functions where the format string is not a string literal and there are no format arguments, as in printf (foo);. This may be a security hole if the format string came from untrusted input and contains %n.

Position Independent Executable (pie) are needed to take advantage of Address Space Layout Randomization, supported by some kernel versions. While ASLR can already be enforced for data areas in the stack and heap (brk and mmap), the code areas must be compiled as position-independent. Shared libraries already do this (-fPIC), so they gain ASLR automatically, but binary .text regions need to be build PIE to gain ASLR. When this happens, ROP attacks are much harder since there are no static locations to bounce off of during a memory corruption attack.

relro
During program load, several ELF memory sections need to be written to by the linker, but can be turned read-only before turning over control to the program. This prevents some GOT (and .dtors) overwrite attacks, but at least the part of the GOT used by the dynamic linker (.got.plt) is still vulnerable.

bindnow
During program load, all dynamic symbols are resolved, allowing for the complete GOT to be marked read-only (due to -z relro above). This prevents GOT overwrite attacks. For very large application, this can incur some performance loss during initial load while symbols are resolved, but this shouldn't be an issue for daemons.

State in other Distributions
Nearly all major distributions are setting these flags by default (except for pie), debian started working on this ~10 years ago, see links at the end.

Most packages build (https://hydra.nixos.org/jobset/nixpkgs/pr-12895#tabs-evaluations), mostly needs further testing at runtime.

I'd be happy to hear more comments, ideas, concerns!

Links
https://wiki.debian.org/Hardening
https://wiki.ubuntu.com/Security/Features
https://wiki.gentoo.org/wiki/Project:Hardened (and https://wiki.gentoo.org/wiki/Hardened/FAQ)
https://wiki.archlinux.org/index.php/DeveloperWiki:Security

(/cc @copumpkin @fpletz)

@mention-bot
Copy link

@mention-bot mention-bot commented Feb 9, 2016

By analyzing the blame information on this pull request, we identified @edolstra, @rbvermaa and @MarcWeber to be potential reviewers

@copumpkin
Copy link
Member

@copumpkin copumpkin commented Feb 9, 2016

First off, awesome effort, thank you 👍 👍 👍 👍 🍻

Second, I know you say that it hasn't been tested on Darwin, but I'm wondering if you've tested things using the clang stdenv even on Linux. If so, I have reasonable confidence that all issues Darwin might encounter would be resolved ahead of time.

Edit: is PIE off by default due to performance concerns? I believe on Mac OS, it's on by default for x86_64 due to it being cheap.

@copumpkin copumpkin mentioned this pull request Feb 9, 2016
0 of 17 tasks complete
@heydojo
Copy link
Contributor

@heydojo heydojo commented Feb 9, 2016

Is there a global switch to turn this all off. Or should I be looking at clang? (For prototyping and boards like pie zero where I want speed, don't want the penalties this creates.)

I would be interested in benchmarks to see what kind of a difference these changes make.

@globin
Copy link
Member Author

@globin globin commented Feb 9, 2016

Yes hardening_all = false;

@heydojo
Copy link
Contributor

@heydojo heydojo commented Feb 9, 2016

@globin cool tnx!

@copumpkin
Copy link
Member

@copumpkin copumpkin commented Feb 9, 2016

@globin your evaluation error with hardening.foo is due to your hardening arguments making their way down to the underlying derivation. Nix assumes that all parameters to derivation (the builtin underlying mkDerivation) should be stringified and put into the environment of the underlying script. That's not as important in our case, but is still happening with your code. When you had hardening_foo, that was a boolean so could easily be stringified. When you wrote hardening.foo, that turned into a dict-valued attribute called hardening to derivation, and when Nix tries to stringify hardening, it doesn't know what to do.

@vcunat
Copy link
Member

@vcunat vcunat commented Feb 9, 2016

Currently this uses -fstack-protector-all instead of -fstack-protector-strong because the bootstrapping compiler is too old.

There's no use for hardening during bootstrapping so you can selectively disable it in there. BTW, I didn't know gcc-4.8.3 is considered so old.

This includes a gcc update to gcc5 as we were checking all builds anyway and fixed up some stuff on the way.

We've had an unresolved discussion around gcc5 switch due to the question whether to use the new C++ ABI by default (already). (Full conformance to C++11 vs. inability to link with stuff compiled by older gcc versions.) #8729

@copumpkin
Copy link
Member

@copumpkin copumpkin commented Feb 9, 2016

I didn't know gcc-4.8.3 is considered so old.

Hey, it was released over a year and a half ago. That's ages in computer time 😄

@globin
Copy link
Member Author

@globin globin commented Feb 9, 2016

Ok sounds like I'll take the gcc bump out again.

Regarding GCC 4.8:

Since GCC 4.9, -fstack-protector-strong, an improved version of -fstack-protector is introduced, which covers all the more paranoid conditions that might lead to a stack overflow but not trade performance like -fstack-protector-all, thus it becomes default.

@copumpkin
Copy link
Member

@copumpkin copumpkin commented Feb 9, 2016

Is moving to 4.9 less controversial than 5?
On Tue, Feb 9, 2016 at 18:26 Robin Gloster notifications@github.com wrote:

Ok sounds like I'll take the gcc bump out again.

Regarding GCC 4.8:

Since GCC 4.9, -fstack-protector-strong, an improved version of
-fstack-protector is introduced, which covers all the more paranoid
conditions that might lead to a stack overflow but not trade performance
like -fstack-protector-all, thus it becomes default.


Reply to this email directly or view it on GitHub
#12895 (comment).

@globin
Copy link
Member Author

@globin globin commented Feb 9, 2016

gcc49 is the default for the system, only the pre-built bootstrap gcc is at 4.8

@globin
Copy link
Member Author

@globin globin commented Feb 11, 2016

@copumpkin PIE is off because it breaks nearly everything (see the Ubuntu link where they have turned it on), I reckon you're thinking of PIC.

@domenkozar
Copy link
Member

@domenkozar domenkozar commented Feb 15, 2016

FYI, travis is saying:

error: anonymous function at /home/travis/build/NixOS/nixpkgs/pkgs/development/compilers/webdsl/default.nix:1:1 called without required argument ‘strategoPackages’, at /home/travis/build/NixOS/nixpkgs/lib/customisation.nix:56:12
@globin globin mentioned this pull request Feb 21, 2016
@jagajaga
Copy link
Member

@jagajaga jagajaga commented Feb 22, 2016

👍

@globin globin force-pushed the hardened-stdenv branch to 46b0d51 Feb 26, 2016
@globin
Copy link
Member Author

@globin globin commented Aug 24, 2016

@mrobbetts there was a bug in yesterdays commits.

@mrobbetts
Copy link
Contributor

@mrobbetts mrobbetts commented Aug 25, 2016

@globin ah, fabulous. Rebuilding now :)

@mrobbetts
Copy link
Contributor

@mrobbetts mrobbetts commented Aug 25, 2016

Is #17999 another such case? It is failing for me now.

@obadz
Copy link
Contributor

@obadz obadz commented Aug 27, 2016

@edolstra, are you OK for us to merge staging if http://hydra.nixos.org/build/39310332#tabs-constituents looks good?

@obadz
Copy link
Contributor

@obadz obadz commented Aug 28, 2016

The tests looked pretty good but the Golang ecosystem was broken by the binutils version bump in 0c12ae5. Was fixed by 6eb4014.

If there are no objections (cc @edolstra) AND the latest eval looks clean (http://hydra.nixos.org/build/39336299#tabs-constituents), I will merge staging tomorrow.

@domenkozar
Copy link
Member

@domenkozar domenkozar commented Aug 28, 2016

There are still 65 "newly failing jobs" compared to trunk-combined: http://hydra.nixos.org/eval/1289558?compare=trunk-combined

@globin
Copy link
Member Author

@globin globin commented Aug 28, 2016

But 97 newly succeeding Jobs 🙈
I guess we should merge nonetheless as it's getting hard to fix stuff without creating conflicts on master regularly.. I'm constantly fixing up new stuff and real failures disregarding dependencies are 3 or 4 packages that look unrelated to hardening at least

@obadz
Copy link
Contributor

@obadz obadz commented Aug 28, 2016

Agreed, unless @edolstra (or someone else) has concerns, let's go ahead and merge and deal with the minor remaining issues (which likely won't require a mass rebuild) in master.

@obadz
Copy link
Contributor

@obadz obadz commented Aug 28, 2016

@domenkozar, are you concerned enough that you're saying we shouldn't merge?

@domenkozar
Copy link
Member

@domenkozar domenkozar commented Aug 28, 2016

If you're willing to help fix those later (which I'm sure you do), let's merge if there are no objections from others.

@edolstra
Copy link
Member

@edolstra edolstra commented Aug 29, 2016

Looks great to me. Thanks for all the work on this!

@obadz obadz merged commit c0fa26e into master Aug 29, 2016
0 of 2 checks passed
0 of 2 checks passed
continuous-integration/travis-ci/pr The Travis CI build could not complete due to an error
Details
continuous-integration/travis-ci/push The Travis CI build could not complete due to an error
Details
@globin globin deleted the hardened-stdenv branch Aug 29, 2016
@offlinehacker
Copy link
Contributor

@offlinehacker offlinehacker commented Sep 1, 2016

Great work guys, thanks :)

@cleverca22
Copy link
Contributor

@cleverca22 cleverca22 commented Sep 4, 2016

https://gist.github.com/cleverca22/b1300b91ea6bf8951256c60acf2844ee

__memcpy_chk appears to be missing from the libc.a in glibc.static master, but it is present on an older nixpkgs from 20f009d

edit:
ah, that function just moved to ${stdenv.cc.cc.out}/lib/libssp.a for some strange reason, -lssp solves the problem

@vcunat
Copy link
Member

@vcunat vcunat commented Sep 11, 2016

Hmm, it seems we still have glibc without stack protector: #1.

@fpletz
Copy link
Member

@fpletz fpletz commented Sep 11, 2016

Thanks for pointing that out. We had to disable stackprotector for the glibc build but didn't investigate further. There are even more hardening options available in the configure script for glibc. I'll check what we can enable. Unfortunately, this will be another mass rebuild that probably won't make it into 16.09.

@domenkozar

This comment has been minimized.

Copy link
Member

@domenkozar domenkozar commented on 98473cd Sep 15, 2016

@andrewrk
Copy link
Member

@andrewrk andrewrk commented Sep 27, 2016

Great work on this issue. Is there a way so that we can have these hardening flags applied to nixpkgs, but not always forcing gcc users to use the flags? See #18995

@copumpkin

This comment has been minimized.

Copy link
Member

@copumpkin copumpkin commented on b2b499e Apr 26, 2017

@fpletz someone on #pax is saying that ssp-buffer-size=4 is redundant with -fstack-protector-strong. Do you recall why you added this?

This comment has been minimized.

Copy link
Member Author

@fpletz fpletz replied Apr 27, 2017

@copumpkin IIRC the default of ssp-buffer-size is 8. This was supposed to activate the stack protector for functions with buffers >= 4 bytes. Did the default change in a recent GCC version?

This comment has been minimized.

Copy link
Member

@copumpkin copumpkin replied Apr 27, 2017

The objection the person raised was just that ssp-buffer-size only affects -fstack-protector (without -strong), and not -strong. Something like that. I don't actually know anything about these options, just reporting what they said.

Either way, it shouldn't hurt anything, but might be confusing.

This comment has been minimized.

Copy link
Member Author

@fpletz fpletz replied Apr 27, 2017

Interesting. Thanks for the hint. I'll double-check the docs and other hardening flags implementations.

@Ericson2314

This comment has been minimized.

Copy link
Member

@Ericson2314 Ericson2314 commented on pkgs/build-support/cc-wrapper/cc-wrapper.sh in aff1f4a Aug 24, 2017

N.B cc-wrapper conditionally adds ld flags, without -Wl, here. See comment below.

@Ericson2314

This comment has been minimized.

Copy link
Member

@Ericson2314 Ericson2314 commented on pkgs/build-support/cc-wrapper/ld-wrapper.sh in aff1f4a Aug 24, 2017

N.B. hardeningLDFlags unconditionally added here, even if it was already added in cc-wrapper itself. See comment below.

@Ericson2314

This comment has been minimized.

Copy link
Member

@Ericson2314 Ericson2314 commented on aff1f4a Aug 24, 2017

I noticed that if one is compiling and linking in one step (no -c flag to cc), then the hardening ld flags are passed twice. I'd think either they'd just be passed to ld, or they'd be passed to cc and guarded in ld-wrapper under LDFLAGS_SET. Is this intended?

I am soon going to make a PR for splitting a binutils-wrapper derivation, out of cc-wrapper, and this makes it a bit unclear how the division of labor for hardening ought to work.

This comment has been minimized.

Copy link
Member

@Ericson2314 Ericson2314 replied Aug 25, 2017

Tentative fix in #28555

@Ma27 Ma27 mentioned this pull request Mar 14, 2018
2 of 8 tasks complete
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

You can’t perform that action at this time.