Enable gcc and ld hardening by default in 0.2.3.x

Per a discussion with Roger at the Seattle hackfest I propose we enable gcc and ld hardening by default in 0.2.3.x when the ./configure is built with no options.

Shall I submit a patch that flips the options? So rather than --enable-gcc-hardening, we say --disable-gcc-hardening?

Or shall we just load the options and call it a day?

If someone else wants to bell the cat, I'm happy to defer to them.

changed milestone to %Tor: 0.2.3.x-final

added component::core tor/tor milestone::Tor: 0.2.3.x-final owner::ioerror priority::high resolution::fixed security status::closed tor-client type::defect labels

related? https://trac.torproject.org/projects/tor/ticket/5024

Implementing this would make a small part of #5024 (moved) implemented once we switch to tor 0.2.3.x for TBB, but not #5024 (moved) means all other stuff we bundle I think.

ioerror: I would totally welcome a switch to --enable-gcc-hardening becoming the new default.

The tor package from http://deb.torproject.org/torproject.org is already compiled using hardening options:

"Full RELRO Canary found NX enabled PIE enabled"

Yes, the Debian packaging sets the appropriate configure commands by default. In theory all other packagers should do so too, but because they don't, choosing this default for them when possible makes sense.

nextgens gives us this patch:

diff --git a/changes/bug5210 b/changes/bug5210
new file mode 100644
index 0000000..b07e7f1
--- /dev/null
+++ b/changes/bug5210
@@ -0,0 +1,2 @@
+  o Security fixes:
+    - Enable gcc and ld hardening by default. Fixes bug 5210.
diff --git a/configure.in b/configure.in
index 7415ce8..23dcc07 100644
--- a/configure.in
+++ b/configure.in
@@ -122,19 +122,23 @@ dnl -D_FORTIFY_SOURCE=2 -fstack-protector-all
 dnl Others suggest '/gs /safeseh /nxcompat /dynamicbase' for non-gcc on Windows
 dnl This requires that we use gcc and that we add -O2 to the CFLAGS.
 AC_ARG_ENABLE(gcc-hardening,
-     AS_HELP_STRING(--enable-gcc-hardening, enable compiler security checks),
+    AS_HELP_STRING(--disable-gcc-hardening, disable compiler security checks),
+    [enableval=no;],
+    [enableval=yes;])
 [if test x$enableval = xyes; then
     CFLAGS="$CFLAGS -D_FORTIFY_SOURCE=2 -fstack-protector-all"
     CFLAGS="$CFLAGS -fwrapv -fPIE -Wstack-protector"
     CFLAGS="$CFLAGS --param ssp-buffer-size=1"
     LDFLAGS="$LDFLAGS -pie"
-fi])
+fi]
 
 dnl Linker hardening options
 dnl Currently these options are ELF specific - you can't use this with MacOSX
 AC_ARG_ENABLE(linker-hardening,
-        AS_HELP_STRING(--enable-linker-hardening, enable linker security fixups
-[if test x$enableval = xyes; then
+    AS_HELP_STRING(--disable-linker-hardening, disable linker security fixups),
+    [enableval=no;],
+    [enableval=yes;])
+AC_CHECK_HEADER([elf.h], [if test x$enableval = xyes; then
     LDFLAGS="$LDFLAGS -z relro -z now"
 fi])

Trac:
patchv2.diff

patch against current master - 4ade55ecb9e6c3144dc6ed192fae0f613acfb467

The patch breaks currently existing setups in a subtle way, so we can't apply it as is. If you call configure with --enable-gcc-hardening, the hardening gets turned off silently - obviously that's bad.

Trac:
Status: new to needs_revision

This is the working version of the above: https://github.com/nextgens/Tor/commit/62f3121a3d209fb4f826988d53b1aac93842502c

now both --enable and --disable should now work as intended.

There's two caveats with this patch still:

• it doesn't detect whether gcc is in use and assumes that the compiler will ignore the flags it doesn't 'support'
• it checks for the presence of the header 'elf.h' rather than the format of the executable produced (ie: the auto-detection magic won't work if you cross-compile or happen to have the elf headers installed)

Detecting whether a particular flag is supported by the compiler could be done with a macro such as: http://ac-archive.sourceforge.net/ac-archive/ax_cflags_gcc_option.html

Trac:
Cc: arma, nickm to arma, nickm, nextgens@freenetproject.org

That does indeed seem to work better, but I can confirm that building on clang, which doesn't support --param ssp-buffer-size=1 and -pie, gives a ton of warnings.

Okay, so what would be a way to detect this situation? We could just say, "If clang, don't use these options," but that wouldn't be so nice if some future version of clang starts supporting them. Other ideas?

Trac:
Cc: arma, nickm, nextgens@freenetproject.org to arma, nickm, nextgens@freenetproject.org, tails@boum.org

I tried building something with AX_CFLAGS_GCC_OPTION, but it was too buggy; it didn't support --param ssp-buffer-size=1. So instead, I've hacked together a "bug5210" in my public repo, based on the one from nextgetns. Tested on linux with gcc and clang; then tried on OSX, where I had to add some more options to make clang shut up.

Please test and review and think about this stuff.

Trac:
Status: needs_revision to needs_review

asn probably has something to add here, based on experience with obfsproxy

Trac:
Cc: arma, nickm, nextgens@freenetproject.org, tails@boum.org to arma, nickm, nextgens@freenetproject.org, tails@boum.org, asn

Hi, you may be interested in my recent article about automatic binary hardening with Autoconf:

http://mainisusuallyafunction.blogspot.com/2012/05/automatic-binary-hardening-with.html

Trac:
Username: kmcallister

Quite interesting, this certainly gets around the clang annoyances. How portable do you expect your approach to be?

(What I find interesting are the performance implications. 30% slowdown on Debian does seem somewhat drastic)

Replying to kmcallister:

Hi, you may be interested in my recent article about automatic binary hardening with Autoconf:

http://mainisusuallyafunction.blogspot.com/2012/05/automatic-binary-hardening-with.html

Looks like they've converged on the same options we have. That much is good.

I'm not convinced that explicitly grepping for a warning from clang is such a good idea: warnings change in the presence of localization.

The slowdown business is something we'll need to deal with in practice as we go. If stack-protector is hideously slow in some configurations, we might need to turn it off. If -fPIE is a big deal, we may need to add in a -fomit-frame-pointer for production builds of critical-path pieces of the code.

Incidentally, I don't think we really get protection from -fPIE unless any static library we link against is also built with -fPIE, right?

Some of this won't work on windows unless we do yet more magic; said magic is however a thing for a separate ticket.

Yuck. For me, this breaks on mingw+windows for some reason. Investigating... it looks like the -pie linker option breaks stuff there, but not immediately in a way that the configure script notices. (Stuff goes okay, and then when it wants to link libevent, it dies in a fire) I wonder if this is just my junky mingw setup, or if others have this issue with this branch too?

Trac:
Status: needs_review to needs_revision

oh hey. The obvious fix for that would be to do these tests after we know what the libraries are.

Okay, I think I have something that works added to my branch "bug5210" in my public repo. I'll try it in a few more places before I merge it.

Trac:
Status: needs_revision to needs_review

Ugh. The "-pie" part still makes the windows build crap out -- it doesn't detect that on windows, '-pie' is hopeless.. I have just gone and hard-disabled it when we're building for windows. Branch "bug5210" in my public repo is still the one to look at.

Tested this on osx 10.7 with llvm-gcc 4.2 and clang 3.1, seems to work fine on both. I'll start a compile of clang 3.0 when I have power again and try compiling tor with clang 3.0 with that, too

I've tested it on all my VMs, and with clang 3. I am calling this ready to go. Getting more of it working on windows is a task for #5400 (closed).

Merging into 0.2.3. Thanks, everyone!

Trac:
Resolution: N/A to fixed
Status: needs_review to closed

Trac:
Keywords: security deleted, security tor-client added

Trac:
Component: Tor Client to Tor

closed

mentioned in issue #5402 (moved)

mentioned in issue #6173 (moved)

moved to tpo/core/tor#5210 (closed)

mentioned in issue tpo/core/tor#5402 (closed)